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 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.

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.

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


-- 
Chuck Lever