Re: [PATCH v1 02/12] Add and use d_for_each_positive_child family of iterators

On Tue, 11 Aug 2026, Miklos Szeredi wrote:
> On Mon, 3 Aug 2026 at 03:38, NeilBrown <[email protected]> wrote:
> 
> > coda_flag_children() doesn't need rcu_read_lock() as holding any
> > spinlock prevents an RCU critical section from finishing.
> 
> I think prep changes should go in separate patches.
> 
> The conversion should be an equivalent transformation.

Ok, that's reasonable.

> 
> > nfs_clear_verifier_directory() is changed slightly so the parent lock is
> > dropped and retaken between clearing the verifier on the parent and on
> > all the children.  I think this is safe.
> 
> This can also be a prep patch.
> 
> > get_next_postive_dentry() in autofs now holds the parent lock a bit
> > less.  The only non-trivial code that is no locked any more is
> >    parent = p->d_parent
> > This cannot need the lock as autofs doesn't support rename and
> > a ref is held on a descendent of parent, so ->d_parent cannot
> > change.  And if it could, the lock wouldn't help.
> 
> And this too.
> 
> > diff --git a/fs/autofs/expire.c b/fs/autofs/expire.c
> > index 5c2d459e1e48..7b0a22629415 100644
> > --- a/fs/autofs/expire.c
> > +++ b/fs/autofs/expire.c
> > @@ -70,12 +70,9 @@ static int autofs_mount_busy(struct vfsmount *mnt,
> >         return status;
> >  }
> >
> > -/* p->d_lock held */
> >  static struct dentry *positive_after(struct dentry *p, struct dentry *child)
> 
> Move this primitive to libfs.c?

By the end of the series it is nearly identical to scan_positives().
So maybe I could add a patch which exports that and uses it instead
of positive_after().

> 
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -2182,14 +2182,10 @@ static bool drop_negative_children(struct dentry *dentry)
> >         if (!d_is_dir(dentry))
> >                 goto out;
> >
> > -       spin_lock(&dentry->d_lock);
> > -       hlist_for_each_entry(child, &dentry->d_children, d_sib) {
> > -               if (d_really_is_positive(child)) {
> > -                       all_negative = false;
> > -                       break;
> > -               }
> > +       d_for_each_positive_child(child, dentry) {
> > +               all_negative = false;
> > +               break;
> >         }
> > -       spin_unlock(&dentry->d_lock);
> 
> simple_empty()?

Maybe...
They aren't quite the same in that simple_empty() locks the found
positive dentry and checks again.  drop_negative_children() doesn't pay
the cost of that spin lock.
Does that matter?  Is the spinlock needed?  Should I remove it from
simple_empty()?

Also they are called in different circumstances.

simple_empty() is used for an in-dcache filesystem.  It is called with
the directory locked (i_rwsem) so nothing can be added/removed and it
gives an answer about the directory (and so shouldn't need the spinlock
as it has the i_rwsem).

drop_negative_children() is called on a caching filesystem so it is
reporting on the cache, not on the directory.  Also i_rwsem isn't held
(so maybe it does need the spinlock???)

I wonder if ceph really wants a version of shrink_dcache_parent() which
only drops negative children.....  looking at the history is probably
doesn't and probably isn't bothered by TOCTOU races between checking for
positives, and calling shrink_dcache_parent().

I think I would rather keep the to separate.

Thanks again,
NeilBrown


> 
> Thanks,
> Miklos
>