Re: [PATCH 3/4] vfs: check for autofs expiring dentry in follow_automount()

On Thu, Mar 26, 2020 at 01:23:29PM +0800, Ian Kent wrote:
>follow_automount() checks if a stat family system call path walk is
>being done on a positive dentry and and returns -EISDIR to indicate
>the dentry should be used as is without attempting an automount.
>
>But if autofs is expiring the dentry at the time it should be
>remounted following the expire.
>
>There are two cases, in the case of a "nobrowse" indirect autofs
>mount it would have been mounted on lookup anyway. In the case of
>a "browse" indirect or direct autofs mount re-mounting it will
>maintain the mount which is what user space would be expected.
>
>This will defer expiration of the mount which might lead to mounts
>unexpectedly remaining mounted under heavy stat activity but there's
>no other choice in order to maintain consistency for user space.
>
>Signed-off-by: Ian Kent <[email protected]>
>---
> fs/autofs/root.c |   10 +++++++++-
> fs/namei.c       |   13 +++++++++++--
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
>diff --git a/fs/autofs/root.c b/fs/autofs/root.c
>index a1c9c32e104f..308cc49aca1d 100644
>--- a/fs/autofs/root.c
>+++ b/fs/autofs/root.c
>@@ -406,9 +406,17 @@ static int autofs_d_manage(const struct path *path, bool rcu_walk)
>
> 	/* Check for (possible) pending expire */
> 	if (ino->flags & AUTOFS_INF_WANT_EXPIRE) {
>+		/* dentry possibly going to be picked for expire,
>+		 * proceed to ref-walk mode.
>+		 */
> 		if (rcu_walk)
> 			return -ECHILD;
>-		return 0;
>+
>+		/* ref-walk mode, return 1 so follow_automount()
>+		 * can to wait on the expire outcome and possibly

'can to wait' ?
Do you mean: "can wait", "will wait", "knows to wait",
or something else?

>+		 * attempt a re-mount.
>+		 */
>+		return 1;
> 	}
>
> 	/*
>diff --git a/fs/namei.c b/fs/namei.c
>index db6565c99825..34a03928d32d 100644
>--- a/fs/namei.c
>+++ b/fs/namei.c
>@@ -1227,11 +1227,20 @@ static int follow_automount(struct path *path, struct nameidata *nd,
> 	 * mounted directory.  Also, autofs may mark negative dentries
> 	 * as being automount points.  These will need the attentions
> 	 * of the daemon to instantiate them before they can be used.
>+	 *
>+	 * Also if ->d_manage() returns 1 the dentry transit needs
>+	 * managing, for autofs it tells us the dentry might be expired,
>+	 * so proceed to ->d_automount().

I'm wondering if this should be two sentences.
Does this version reflect reality?

+	 * Also if ->d_manage() returns 1 the dentry transit needs
+	 * to be managed. For autofs, a return of 1 tells us the
+	 * dentry might be expired, so proceed to ->d_automount().

Kind regards
Vince
> 	 */
> 	if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
> 			   LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT)) &&
>-	    path->dentry->d_inode)
>-		return -EISDIR;
>+	    path->dentry->d_inode) {
>+		if (path->dentry->d_flags & DCACHE_MANAGE_TRANSIT) {
>+			if (!path->dentry->d_op->d_manage(path, false))
>+				return -EISDIR;
>+		} else
>+			return -EISDIR;
>+	}
>
> 	nd->total_link_count++;
> 	if (nd->total_link_count >= 40)
>

--