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

[Ian Kent <raven@themaw.net>]

On Wed, 2026-10-21 at 20:54 +1100, NeilBrown wrote:
> On Mon, 19 Oct 2026, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > 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.
Umm ... oops!

Yes it does but that's purely to establish if there's any additional
references held indicating an open file or working directory within
the directory although I'm not sure I actually use it for individual
directories. The mount reference count changes that Al did some time
ago should mean that ino_count can be eliminated and the mount count
alone used instead.

In fact I still need to post a fix for may_umount_tree() that uses
those changes ... maybe now's the time so you have example usage.

Sigh ... time ... time ... time ...

Ian
> 
> 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@brown.name>
> > ---
> >  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
> > 
> > 
> >