Re: [PATCH v4 1/5] VFS: don't count references through ->d_parent

[NeilBrown]

On Mon, 19 Oct 2026, NeilBrown wrote:
> From: NeilBrown <neil@xxxxxxxxxx>
> 
> The number of negative child dentries that a given dentry can have is
> effectively unbounded, so counting the references through ->d_parent can
> overflow ->d_lockref.count.
> 
> We only use this count so that we can know when there are no remaining
> references including from children.  However most children appear on the
> ->d_children list and the few remaining children (inactive
> DCACHE_DENTRY_CURSORs) only exist in a context were a separate reference
> is held on the parent.
> 
> So if we count whether d_children is nonempty (i.e.  dget() when it
> becomes non-empty, dput() when it comes empty) and require children not
> on ->d_children to have a reference on the parent some other way (which
> they already do), then ->d_lockref.count will not overflow and we can
> still know exactly when all references, including through ->d_parent,
> are gone.

sashiko discovered a real bug here as autofs depends on d_count()
including the number of child dentries.  This is easily fixed.

sashiko also reported a false-positive with the fsnotify changes because
it didn't account for group->mark_mutex providing exclusion.

It also suggested a useful improvement for d_next_sibling_sched() to
always skip all cursors.

I'll resend after the merge window closes.

NeilBrown


> 
> __d_move() allows that dentries might not be on any d_children list as
> is the case for IS_ROOT() dentries.  Rather than track when this is the
> case we dget() either parent if they have an empty d_children before
> children are moved around, then dput() if they turn out to be empty
> afterwards.  This will increment dentry if it IS_ROOT(), and then
> decrement it afterwards, and will decrement the refcount on old_parent
> if its ->d_children becomes empty.
> 
> To help with this, dput_dlock() is added which warns rather than
> decrementing to zero (which should never happen).
> 
> When a DCACHE_CURSOR dentry is added to d_children, it is only ever
> added "before" or "behind" an existing child, so it will never be the
> first child added, and so never needs to adjust the refcount on the
> parent.  If it remains on the list until dput() it could be the last
> child to be removed, in which case normal handling applies in
> dentry_unlist() (where it has been moved from dentry_kill().  When a
> cursor is deliberately removed from the d_children list the refcount
> must be decremented if that leaves the list empty.
> 
> hlist_move_behind() and hlist_move_before() are added to simplify moving
> a cursor within the d_children list.  This results in some duplicate code
> as they share a common prefix with d_detach_cursor() (hlist_del)
> which is currently factored out.  We can expect the compiler to optimise
> this away and don't need to clutter the C code with it.
> 
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> ---
>  fs/dcache.c            | 40 +++++++++++++++++++++++++++++++++-------
>  fs/internal.h          | 10 ++++++++++
>  fs/libfs.c             | 14 +++++++-------
>  include/linux/dcache.h | 17 ++++++++++++++++-
>  include/linux/list.h   | 30 ++++++++++++++++++++++++++++++
>  5 files changed, 96 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index f44c7b39c3b3..3a4d0a025447 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -670,6 +670,8 @@ static inline void dentry_unlist(struct dentry *dentry)
>  		return;
>  	}
>  	__hlist_del(&dentry->d_sib);
> +	if (hlist_empty(&dentry->d_parent->d_children))
> +		dentry->d_parent->d_lockref.count--;
>  	/*
>  	 * Cursors can move around the list of children.  While we'd been
>  	 * a normal list member, it didn't matter - ->d_sib.next would've
> @@ -843,7 +845,7 @@ static struct dentry *dentry_kill(struct dentry *dentry)
>  	spin_unlock(&dentry->d_lock);
>  	if (likely(can_free))
>  		dentry_free(dentry);
> -	if (parent && --parent->d_lockref.count) {
> +	if (parent && parent->d_lockref.count) {
>  		spin_unlock(&parent->d_lock);
>  		return NULL;
>  	}
> @@ -1997,7 +1999,9 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
>  	 * don't need child lock because it is not subject
>  	 * to concurrency here
>  	 */
> -	dentry->d_parent = dget_dlock(parent);
> +	dentry->d_parent = parent;
> +	if (hlist_empty(&parent->d_children))
> +		dget_dlock(parent);
>  	hlist_add_head(&dentry->d_sib, &parent->d_children);
>  	spin_unlock(&parent->d_lock);
>  
> @@ -2016,7 +2020,7 @@ struct dentry *d_alloc_cursor(struct dentry * parent)
>  	struct dentry *dentry = d_alloc_anon(parent->d_sb);
>  	if (dentry) {
>  		dentry->d_flags |= DCACHE_DENTRY_CURSOR | DCACHE_NORCU;
> -		dentry->d_parent = dget(parent);
> +		dentry->d_parent = parent;
>  	}
>  	return dentry;
>  }
> @@ -2778,7 +2782,9 @@ struct dentry *d_alloc_parallel(struct dentry *parent,
>  
>  	new->d_flags |= DCACHE_PAR_LOOKUP;
>  	spin_lock(&parent->d_lock);
> -	new->d_parent = dget_dlock(parent);
> +	new->d_parent = parent;
> +	if (hlist_empty(&parent->d_children))
> +		dget_dlock(parent);
>  	hlist_add_head(&new->d_sib, &parent->d_children);
>  	if (parent->d_flags & DCACHE_DISCONNECTED)
>  		new->d_flags |= DCACHE_DISCONNECTED;
> @@ -3106,12 +3112,21 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
>  
>  	/* ... and switch them in the tree */
>  	dentry->d_parent = target->d_parent;
> +
> +	/*
> +	 * Ensure ref count on parents reflect d_children being non-empty,
> +	 * which they almost certainly are.  If either end up being empty,
> +	 * this is handled below after the moves.
> +	 */
> +	if (hlist_empty(&old_parent->d_children))
> +		dget_dlock(old_parent);
> +	if (dentry->d_parent != old_parent &&
> +	    hlist_empty(&dentry->d_parent->d_children))
> +		dget_dlock(dentry->d_parent);
> +
>  	if (!exchange) {
>  		copy_name(dentry, target);
>  		target->d_hash.pprev = NULL;
> -		dentry->d_parent->d_lockref.count++;
> -		if (dentry != old_parent) /* wasn't IS_ROOT */
> -			WARN_ON(!--old_parent->d_lockref.count);
>  	} else {
>  		target->d_parent = old_parent;
>  		swap_names(dentry, target);
> @@ -3124,6 +3139,17 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
>  	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.
> +	 * This should only ever be old_parent.
> +	 */
> +	if (hlist_empty(&old_parent->d_children))
> +		dput_dlock(old_parent);
> +	if (dentry->d_parent != old_parent &&
> +	    hlist_empty(&dentry->d_parent->d_children))
> +		dput_dlock(dentry->d_parent);
> +
>  	__d_rehash(dentry);
>  	fsnotify_update_flags(dentry);
>  	fscrypt_handle_d_move(dentry);
> diff --git a/fs/internal.h b/fs/internal.h
> index c658c8a5ebd5..59b2b8030b6c 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -235,6 +235,16 @@ extern struct dentry *__d_lookup(const struct dentry *, const struct qstr *);
>  extern struct dentry *__d_lookup_rcu(const struct dentry *parent,
>  				const struct qstr *name, unsigned *seq);
>  
> +static inline void d_detach_cursor(struct dentry *cursor)
> +{
> +	if (hlist_unhashed(&cursor->d_sib))
> +		return;
> +	hlist_del_init(&cursor->d_sib);
> +	if (hlist_empty(&cursor->d_parent->d_children))
> +		/* That was the last child, must drop implied reference */
> +		dput_dlock(cursor->d_parent);
> +}
> +
>  /*
>   * pipe.c
>   */
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 27d7dc16fcb0..60586fe65895 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -128,9 +128,7 @@ static struct dentry *scan_positives(struct dentry *cursor,
>  			count = 1;
>  		}
>  		if (need_resched()) {
> -			if (!hlist_unhashed(&cursor->d_sib))
> -				__hlist_del(&cursor->d_sib);
> -			hlist_add_behind(&cursor->d_sib, &d->d_sib);
> +			hlist_move_behind(&cursor->d_sib, &d->d_sib);
>  			p = &cursor->d_sib.next;
>  			spin_unlock(&dentry->d_lock);
>  			cond_resched();
> @@ -166,9 +164,10 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence)
>  			to = scan_positives(cursor, &dentry->d_children.first,
>  					    offset - 2, NULL);
>  		spin_lock(&dentry->d_lock);
> -		hlist_del_init(&cursor->d_sib);
>  		if (to)
> -			hlist_add_behind(&cursor->d_sib, &to->d_sib);
> +			hlist_move_behind(&cursor->d_sib, &to->d_sib);
> +		else
> +			d_detach_cursor(cursor);
>  		spin_unlock(&dentry->d_lock);
>  		dput(to);
>  
> @@ -210,9 +209,10 @@ int dcache_readdir(struct file *file, struct dir_context *ctx)
>  		p = &next->d_sib.next;
>  	}
>  	spin_lock(&dentry->d_lock);
> -	hlist_del_init(&cursor->d_sib);
>  	if (next)
> -		hlist_add_before(&cursor->d_sib, &next->d_sib);
> +		hlist_move_before(&cursor->d_sib, &next->d_sib);
> +	else
> +		d_detach_cursor(cursor);
>  	spin_unlock(&dentry->d_lock);
>  	dput(next);
>  
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 4b1ff99608e0..3b595adff7b4 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -324,7 +324,7 @@ extern char *dentry_path(const struct dentry *, char *, int);
>  /* Allocation counts.. */
>  
>  /**
> - * dget_dlock -	get a reference to a dentry
> + * dget_dlock -	get a reference to a dentry while locked
>   * @dentry: dentry to get a reference to
>   *
>   * Given a live dentry, increment the reference count and return the dentry.
> @@ -339,6 +339,21 @@ static inline struct dentry *dget_dlock(struct dentry *dentry)
>  	return dentry;
>  }
>  
> +/**
> + * dput_dlock -	put a reference to a dentry while locked
> + * @dentry: dentry to get a reference to
> + *
> + * Given a live dentry, decrement the reference count and return the dentry.
> + * Caller must hold @dentry->d_lock.  The dentry must still have
> + * a reference after the decrement.  This can be used when two
> + * references are held and one must be dropped.
> + */
> +static inline struct dentry *dput_dlock(struct dentry *dentry)
> +{
> +	if (!WARN_ON(dentry->d_lockref.count <= 1))
> +		dentry->d_lockref.count--;
> +	return dentry;
> +}
>  
>  /**
>   * dget - get a reference to a dentry
> diff --git a/include/linux/list.h b/include/linux/list.h
> index 77fb62f79928..a94eec20275f 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -1123,6 +1123,36 @@ static inline void hlist_add_behind(struct hlist_node *n,
>  		WRITE_ONCE(n->next->pprev, &n->next);
>  }
>  
> +/**
> + * hlist_move_behind - move an hlist node to a new location in list
> + * @n: hlist node to be moved
> + * @prev: hlist node after which @n should be added.
> + *
> + * @n may be on the list or unhashed but must not be @prev.
> + * It is removed if necessary and re-added after @prev.
> + */
> +static inline void hlist_move_behind(struct hlist_node *n,
> +				     struct hlist_node *prev)
> +{
> +	hlist_del_init(n);
> +	hlist_add_behind(n, prev);
> +}
> +
> +/**
> + * hlist_move_before - move an hlist node to a new location in list
> + * @n: hlist node to be moved
> + * @next: hlist node before which @n should be added.
> + *
> + * @n may be on the list or unhashed but must not be @next.
> + * It is removed if necessary and re-added before @next.
> + */
> +static inline void hlist_move_before(struct hlist_node *n,
> +				     struct hlist_node *next)
> +{
> +	hlist_del_init(n);
> +	hlist_add_before(n, next);
> +}
> +
>  /**
>   * hlist_add_fake - create a fake hlist consisting of a single headless node
>   * @n: Node to make a fake list out of
> 
> base-commit: f909e534f5e34feb2ea2701d54df63e21aa506fc
> -- 
> 2.50.0.107.gf914562f5916.dirty
> 
> 
>