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

[Ian Kent <raven@themaw.net>]

On Wed, 2026-10-21 at 19:47 +0800, Ian Kent wrote:
> 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.

Ok, so I "do" use this to check if an individual directory is in use
but your right about it being a straight forward change. All that
should be needed is special casing empty directories and removing the
increment/decrement of ->ino_count in ->mkdir() and ->rmdir(). You
shouldn't need to worry about directories that aren't empty since they
shouldn't be covered by a mount so autofs won't be looking to expire
them.

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