Re: [PATCH v2 13/18] VFS: don't move dentries in d_sib list when they have the same parent

[NeilBrown]

On Thu, 20 Aug 2026, Chuck Lever wrote:
> 
> On Sat, Aug 15, 2026, at 12:21 AM, NeilBrown wrote:
> > From: NeilBrown <neil@xxxxxxxxxx>
> >
> > When __d_move() moves or exchanges dentries it currently always moves
> > both dentries to the head of the ->d_children list of the respective
> > parents.
> >
> > When they have the same parent, this simply moves them from where they
> > are to the start in the same list.  So it achieves nothing useful.
> >
> > A future patch will allow d_for_each_positive_child() to drop and retake
> > the parent's d_lock during the iteration.  With the current __d_move
> > behaviour this would allow a dentry to be moved to the front and so
> > missed, even though it is still in the same directory.  This might be
> > unexpected.
> >
> > With this change the only dentries that d_for_each_positive_child()
> > might miss are those moved out of the directory, or those moved in after
> > the iteration started.  These are unavoidable and should not be
> > unexpected.
> >
> > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> > ---
> >  fs/dcache.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index ae726f3ff0cb..50fbbcceca01 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -3052,6 +3052,9 @@ static void copy_name(struct dentry *dentry, 
> > struct dentry *target)
> >   * entries should not be moved in this way. Caller must hold 
> > rename_lock, the
> >   * i_rwsem of the source and target directories (exclusively), and the 
> > sb->
> >   * s_vfs_rename_mutex if they differ. See lock_rename().
> > + *
> > + * If @dentry and @target have the same parent, then neither is
> > + * moved in the d_sib list.
> >   */
> >  static void __d_move(struct dentry *dentry, struct dentry *target,
> >  		     bool exchange)
> > @@ -3119,15 +3122,20 @@ static void __d_move(struct dentry *dentry, 
> > struct dentry *target,
> >  	} else {
> >  		target->d_parent = old_parent;
> >  		swap_names(dentry, target);
> > -		if (!hlist_unhashed(&target->d_sib))
> > -			__hlist_del(&target->d_sib);
> > -		hlist_add_head(&target->d_sib, &target->d_parent->d_children);
> > +		if (target->d_parent != dentry->d_parent) {
> > +			if (!hlist_unhashed(&target->d_sib))
> > +				__hlist_del(&target->d_sib);
> > +			hlist_add_head(&target->d_sib,
> > +				       &target->d_parent->d_children);
> > +		}
> >  		__d_rehash(target);
> >  		fsnotify_update_flags(target);
> >  	}
> > -	if (!hlist_unhashed(&dentry->d_sib))
> > -		__hlist_del(&dentry->d_sib);
> > -	hlist_add_head(&dentry->d_sib, &dentry->d_parent->d_children);
> > +	if (dentry->d_parent != old_parent) {
> > +		if (!hlist_unhashed(&dentry->d_sib))
> > +			__hlist_del(&dentry->d_sib);
> > +		hlist_add_head(&dentry->d_sib, &dentry->d_parent->d_children);
> > +	}
> > 
> >  	/*
> >  	 * Adjust parent refcounts if either d_children ended up empty.
> > -- 
> > 2.50.0.107.gf914562f5916.dirty
> 
> Both new guards are correct. In the exchange branch target->d_parent has
> already been set to old_parent, so testing it against dentry->d_parent
> asks whether the two dentries started in the same directory. In the
> common branch dentry->d_parent has been set to target's parent, so
> testing it against old_parent asks the same question. IS_ROOT()
> still takes the move, which it needs to because a root dentry has an
> unhashed d_sib, and the BUG_ON(p) above guarantees target->d_parent
> is not dentry.
> 
> The opening sentence needs a qualifier. For an ordinary move only
> dentry->d_sib is relocated. target->d_sib is touched only on the
> exchange path, so "both dentries" describes just that case.

Thanks, I'll adjust that.

> 
> This patch also does more for libfs than the patch description claims.
> I would like the description to say so, because it makes the patch
> worth applying on its own.
> 
> offset_readdir() resolves a stale cookie with mas_find_rev() and then
> walks d_children from the dentry it lands on. That is correct only
> while offset order is the reverse of d_children order. d_alloc()
> inserts at the head and mtree_alloc_cyclic() hands out increasing
> offsets, so the two agree in a directory that is only created into.
> 
> A rename within one directory breaks the agreement today.
> simple_offset_rename() gives the surviving dentry the offset of the
> entry it replaced, and d_move() then sends that dentry to the head of
> d_children. Create a, b and c in that order and the offsets are 3, 4
> and 5, with d_children holding c, b, a. After rename("a", "b") the
> dentry for a carries offset 4 and sits at the head, so d_children
> holds a, c while offset order still says c, a. Stop a readdir with a
> reported and c pending, remove c, and the next call resolves the
> cookie to a and reports a a second time.
> 
> With this patch a stays where it is, d_children holds c, a, and the
> two orders agree again. Thus this is a fix for tmpfs readdir, not
> just preparation for 14/18.

Thanks for the review, but I think the above is a false hope.
In that particular case my patch helps, but in a different case it
doesn't.
Suppose instead of rename("a", "b"), I did rename("a", "c").
Before the renames d_children holds (name,offset) pairs of

 ("c", 5), ("b",4"), ("a",3)

after the rename which with my patch doesn't move dentries but does
still copy the name and offset from "c" to "a", and unhashes the
original "c" - which we can show with [] - we have

 ["c", -], ("b",4), ("c", 5)

which has the wrong ordering.

Thanks,
NeilBrown


> 
> Reviewed-by: Chuck Lever <cel@xxxxxxxxxx>
> 
> 
> -- 
> Chuck Lever
> 
>