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

[NeilBrown]

On Thu, 22 Oct 2026, Ian Kent wrote:
> 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@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.
> > 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.

I'd rather make an "obviously correct" transformation, where instead of
assuming d_count(dentry) includes "ino_count", we assume it includes
"ino_count > 1".
i.e.  1 if not-empty, zero if empty.  That is exactly how d_count()
handles children after that patch.

That turned into a longer patch that I hoped because the local variable
"ino_count" which was already misleadingly named, became if more
disconnected from the the actual ino_count.

Below this is what I have.
Thanks,
NeilBrown


--- a/fs/autofs/expire.c
+++ b/fs/autofs/expire.c
@@ -208,15 +208,18 @@ static int autofs_tree_busy(struct vfsmount *mnt,
 			}
 		} else {
 			struct autofs_info *ino = autofs_dentry_ino(p);
-			unsigned int ino_count = READ_ONCE(ino->count);
+			unsigned int use_count;
+
+			/* allow for 1 if there are any children */
+			use_count = READ_ONCE(ino->count) > 0;
 
 			/* allow for dget above and top is already dgot */
 			if (p == top)
-				ino_count += 2;
+				use_count += 2;
 			else
-				ino_count++;
+				use_count++;
 
-			if (d_count(p) > ino_count) {
+			if (d_count(p) > use_count) {
 				top_ino->last_used = jiffies;
 				dput(p);
 				return 1;
@@ -321,7 +324,7 @@ static struct dentry *should_expire(struct dentry *dentry,
 				    unsigned int how)
 {
 	struct autofs_info *ino = autofs_dentry_ino(dentry);
-	unsigned int ino_count;
+	unsigned int use_count;
 
 	/* No point expiring a pending mount */
 	if (ino->flags & AUTOFS_INF_PENDING)
@@ -375,9 +378,11 @@ static struct dentry *should_expire(struct dentry *dentry,
 	if (!(how & AUTOFS_EXP_LEAVES)) {
 		/* Not a forced expire? */
 		if (!(how & AUTOFS_EXP_FORCED)) {
+			/* dentry has any children? */
+			use_count = READ_ONCE(ino->count) > 0;
 			/* ref-walk currently on this dentry? */
-			ino_count = READ_ONCE(ino->count) + 1;
-			if (d_count(dentry) > ino_count)
+			use_count += 1;
+			if (d_count(dentry) > use_count)
 				return NULL;
 		}
 
@@ -392,9 +397,11 @@ static struct dentry *should_expire(struct dentry *dentry,
 
 		/* Not a forced expire? */
 		if (!(how & AUTOFS_EXP_FORCED)) {
+			/* dentry has any children? */
+			use_count = READ_ONCE(ino->count) > 0;
 			/* ref-walk currently on this dentry? */
-			ino_count = READ_ONCE(ino->count) + 1;
-			if (d_count(dentry) > ino_count)
+			use_count += 1;
+			if (d_count(dentry) > use_count)
 				return NULL;
 		}