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

["Chuck Lever" <cel@kernel.org>]

On Tue, Aug 25, 2026, at 5:56 PM, NeilBrown wrote:
> On Thu, 20 Aug 2026, Chuck Lever wrote:
>> 
>> On Sat, Aug 15, 2026, at 12:21 AM, NeilBrown wrote:
>> > From: NeilBrown <neil@brown.name>
>> >
>> > 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@brown.name>
>> > ---
>> >  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.

I have a fix for that, I believe, that applies on top of this one.
I can post it if interested.


> Thanks,
> NeilBrown
>
>
>> 
>> Reviewed-by: Chuck Lever <cel@kernel.org>
>> 
>> 
>> -- 
>> Chuck Lever
>> 
>>

-- 
Chuck Lever