[PATCH 1/4] autofs: dont call do_expire_wait() in autofs_d_manage()

[PATCH 1/4] autofs: dont call do_expire_wait() in autofs_d_manage()

[Ian Kent]

Calling do_expire_wait() in autofs_d_manage() isn't really necessary.

If in rcu-walk mode -ECHILD will be returned and if in ref-walk mode
and the dentry might be picked for expire (or is expiring) 0 will be
returned otherwise it waits for the expire.

But waiting is meant to be done in autofs_d_automount() so simplify
autofs_d_manage() by testing the expire status and returning only
what's needed.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/root.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/autofs/root.c b/fs/autofs/root.c
index 5aaa1732bf1e..a3b7c72a298d 100644
--- a/fs/autofs/root.c
+++ b/fs/autofs/root.c
@@ -410,9 +410,12 @@ static int autofs_d_manage(const struct path *path, bool rcu_walk)
 		return 0;
 	}
 
-	/* Wait for pending expires */
-	if (do_expire_wait(path, rcu_walk) == -ECHILD)
-		return -ECHILD;
+	/* Check for (possible) pending expire */
+	if (ino->flags & AUTOFS_INF_WANT_EXPIRE) {
+		if (rcu_walk)
+			return -ECHILD;
+		return 0;
+	}
 
 	/*
 	 * This dentry may be under construction so wait on mount
@@ -432,8 +435,6 @@ static int autofs_d_manage(const struct path *path, bool rcu_walk)
 		 */
 		struct inode *inode;
 
-		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
-			return 0;
 		if (path_is_mountpoint(path))
 			return 0;
 		inode = d_inode_rcu(dentry);

[PATCH 2/4] autofs: remove rcu_walk parameter from autofs_expire_wait()

[Ian Kent]

Now that do_expire_wait() isn't called from autofs_d_manage() the
rcu_walk boolean parameter can be removed. Now autofs_expire_wait()
and autofs_lookup_expiring() are no longer called from rcu-walk
context either so remove the extra parameter from them too.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/autofs_i.h  |    2 +-
 fs/autofs/dev-ioctl.c |    2 +-
 fs/autofs/expire.c    |    5 +----
 fs/autofs/root.c      |   18 ++++++------------
 4 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/fs/autofs/autofs_i.h b/fs/autofs/autofs_i.h
index 054f97b07754..5fc0c31b1fd5 100644
--- a/fs/autofs/autofs_i.h
+++ b/fs/autofs/autofs_i.h
@@ -152,7 +152,7 @@ void autofs_free_ino(struct autofs_info *);
 
 /* Expiration */
 int is_autofs_dentry(struct dentry *);
-int autofs_expire_wait(const struct path *path, int rcu_walk);
+int autofs_expire_wait(const struct path *path);
 int autofs_expire_run(struct super_block *, struct vfsmount *,
 		      struct autofs_sb_info *,
 		      struct autofs_packet_expire __user *);
diff --git a/fs/autofs/dev-ioctl.c b/fs/autofs/dev-ioctl.c
index a3cdb0036c5d..a892a517c695 100644
--- a/fs/autofs/dev-ioctl.c
+++ b/fs/autofs/dev-ioctl.c
@@ -437,7 +437,7 @@ static int autofs_dev_ioctl_requester(struct file *fp,
 	ino = autofs_dentry_ino(path.dentry);
 	if (ino) {
 		err = 0;
-		autofs_expire_wait(&path, 0);
+		autofs_expire_wait(&path);
 		spin_lock(&sbi->fs_lock);
 		param->requester.uid =
 			from_kuid_munged(current_user_ns(), ino->uid);
diff --git a/fs/autofs/expire.c b/fs/autofs/expire.c
index a1c7701007e7..f67da46f6992 100644
--- a/fs/autofs/expire.c
+++ b/fs/autofs/expire.c
@@ -486,7 +486,7 @@ static struct dentry *autofs_expire_indirect(struct super_block *sb,
 	return expired;
 }
 
-int autofs_expire_wait(const struct path *path, int rcu_walk)
+int autofs_expire_wait(const struct path *path)
 {
 	struct dentry *dentry = path->dentry;
 	struct autofs_sb_info *sbi = autofs_sbi(dentry->d_sb);
@@ -497,9 +497,6 @@ int autofs_expire_wait(const struct path *path, int rcu_walk)
 	/* Block on any pending expire */
 	if (!(ino->flags & AUTOFS_INF_WANT_EXPIRE))
 		return 0;
-	if (rcu_walk)
-		return -ECHILD;
-
 retry:
 	spin_lock(&sbi->fs_lock);
 	state = ino->flags & (AUTOFS_INF_WANT_EXPIRE | AUTOFS_INF_EXPIRING);
diff --git a/fs/autofs/root.c b/fs/autofs/root.c
index a3b7c72a298d..a1c9c32e104f 100644
--- a/fs/autofs/root.c
+++ b/fs/autofs/root.c
@@ -177,8 +177,7 @@ static struct dentry *autofs_lookup_active(struct dentry *dentry)
 	return NULL;
 }
 
-static struct dentry *autofs_lookup_expiring(struct dentry *dentry,
-					     bool rcu_walk)
+static struct dentry *autofs_lookup_expiring(struct dentry *dentry)
 {
 	struct autofs_sb_info *sbi = autofs_sbi(dentry->d_sb);
 	struct dentry *parent = dentry->d_parent;
@@ -197,11 +196,6 @@ static struct dentry *autofs_lookup_expiring(struct dentry *dentry,
 		struct dentry *expiring;
 		const struct qstr *qstr;
 
-		if (rcu_walk) {
-			spin_unlock(&sbi->lookup_lock);
-			return ERR_PTR(-ECHILD);
-		}
-
 		ino = list_entry(p, struct autofs_info, expiring);
 		expiring = ino->dentry;
 
@@ -257,16 +251,16 @@ static int autofs_mount_wait(const struct path *path, bool rcu_walk)
 	return status;
 }
 
-static int do_expire_wait(const struct path *path, bool rcu_walk)
+static int do_expire_wait(const struct path *path)
 {
 	struct dentry *dentry = path->dentry;
 	struct dentry *expiring;
 
-	expiring = autofs_lookup_expiring(dentry, rcu_walk);
+	expiring = autofs_lookup_expiring(dentry);
 	if (IS_ERR(expiring))
 		return PTR_ERR(expiring);
 	if (!expiring)
-		return autofs_expire_wait(path, rcu_walk);
+		return autofs_expire_wait(path);
 	else {
 		const struct path this = { .mnt = path->mnt, .dentry = expiring };
 		/*
@@ -274,7 +268,7 @@ static int do_expire_wait(const struct path *path, bool rcu_walk)
 		 * be quite complete, but the directory has been removed
 		 * so it must have been successful, just wait for it.
 		 */
-		autofs_expire_wait(&this, 0);
+		autofs_expire_wait(&this);
 		autofs_del_expiring(expiring);
 		dput(expiring);
 	}
@@ -327,7 +321,7 @@ static struct vfsmount *autofs_d_automount(struct path *path)
 	 * and the directory was removed, so just go ahead and try
 	 * the mount.
 	 */
-	status = do_expire_wait(path, 0);
+	status = do_expire_wait(path);
 	if (status && status != -EAGAIN)
 		return NULL;
 

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

[Ian Kent]

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 <raven@themaw.net>
---
 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
+		 * 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().
 	 */
 	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)

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

[McIntyre, Vincent (CASS, Marsfield)]

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 <raven@themaw.net>
>---
> 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)
>

-- 

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

[Ian Kent]

On Fri, 2020-03-27 at 05:18 +0000, McIntyre, Vincent (CASS, Marsfield)
wrote:
> 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 <raven@themaw.net>
> > ---
> > 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?

Oops, yes, "can wait" is what that needs to be.

> 
> > +		 * 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().

It does, I'll update that comment too.

Even mentioning the dentry needs to be managed is purely because
that's what its been called, aka. ->d_manage().

Just for info. this is meant to fix a case were a stat() family
system call is being done at the same time the dentry is being
expired (although statfs() is a bit different).

This results in a ping/pong of returning the stat() of the
mounted file system and stat of the autofs file system.

What I'm trying to do is ensure a consistent stat() return
based on the state of the mount at the time, at least to the
extent that I can anyway.

There are actually a number of cases and, unavoidably, there
remains inconsistency because stat family system calls are not
meant to trigger mounts to avoid mount storms. So they will still
return the stat of the autofs file system if not mounted at the
time of the call and the stat of the mounted file system if they
do have an mount on them at the time.

Thanks
Ian

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

[PATCH 4/4] autofs: add comment about autofs_mountpoint_changed()

[Ian Kent]

The function autofs_mountpoint_changed() is unusual, add a comment
about two cases for which it is used.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/root.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/autofs/root.c b/fs/autofs/root.c
index 308cc49aca1d..a972bbaecb46 100644
--- a/fs/autofs/root.c
+++ b/fs/autofs/root.c
@@ -280,9 +280,24 @@ static struct dentry *autofs_mountpoint_changed(struct path *path)
 	struct dentry *dentry = path->dentry;
 	struct autofs_sb_info *sbi = autofs_sbi(dentry->d_sb);
 
-	/*
-	 * If this is an indirect mount the dentry could have gone away
-	 * as a result of an expire and a new one created.
+	/* If this is an indirect mount the dentry could have gone away
+	 * and a new one created.
+	 *
+	 * This is unusual and I can't remember the case for which it
+	 * was originally added now. But a example of how this can
+	 * happen is an autofs indirect mount that has the "browse"
+	 * option set and also has the "symlink" option in the autofs
+	 * map entry. In this case the daemon will remove the browse
+	 * directory and create a symlink as the mount (pointing to a
+	 * local path) leaving the struct path stale.
+	 *
+	 * Another not so obvious case is when a mount in an autofs
+	 * indirect mount that uses the "nobrowse" option is being
+	 * expired and the mount has been umounted but the mount point
+	 * directory remains when a stat family system call is made.
+	 * In this case the mount point is removed (by the daemon) and
+	 * a new mount triggered leading to a stale dentry in the struct
+	 * path of the waiting process.
 	 */
 	if (autofs_type_indirect(sbi->type) && d_unhashed(dentry)) {
 		struct dentry *parent = dentry->d_parent;

Re: [PATCH 4/4] autofs: add comment about autofs_mountpoint_changed()

[McIntyre, Vincent (CASS, Marsfield)]

One nit, below.
Vince

On Thu, Mar 26, 2020 at 01:23:36PM +0800, Ian Kent wrote:
>The function autofs_mountpoint_changed() is unusual, add a comment
>about two cases for which it is used.
>
>Signed-off-by: Ian Kent <raven@themaw.net>
>---
> fs/autofs/root.c |   21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
>diff --git a/fs/autofs/root.c b/fs/autofs/root.c
>index 308cc49aca1d..a972bbaecb46 100644
>--- a/fs/autofs/root.c
>+++ b/fs/autofs/root.c
>@@ -280,9 +280,24 @@ static struct dentry *autofs_mountpoint_changed(struct path *path)
> 	struct dentry *dentry = path->dentry;
> 	struct autofs_sb_info *sbi = autofs_sbi(dentry->d_sb);
>
>-	/*
>-	 * If this is an indirect mount the dentry could have gone away
>-	 * as a result of an expire and a new one created.
>+	/* If this is an indirect mount the dentry could have gone away
>+	 * and a new one created.
>+	 *
>+	 * This is unusual and I can't remember the case for which it
>+	 * was originally added now. But a example of how this can

'an' example

>+	 * happen is an autofs indirect mount that has the "browse"
>+	 * option set and also has the "symlink" option in the autofs
>+	 * map entry. In this case the daemon will remove the browse
>+	 * directory and create a symlink as the mount (pointing to a
>+	 * local path) leaving the struct path stale.
>+	 *
>+	 * Another not so obvious case is when a mount in an autofs
>+	 * indirect mount that uses the "nobrowse" option is being
>+	 * expired and the mount has been umounted but the mount point
>+	 * directory remains when a stat family system call is made.
>+	 * In this case the mount point is removed (by the daemon) and
>+	 * a new mount triggered leading to a stale dentry in the struct
>+	 * path of the waiting process.
> 	 */
> 	if (autofs_type_indirect(sbi->type) && d_unhashed(dentry)) {
> 		struct dentry *parent = dentry->d_parent;
>

-- 

Re: [PATCH 4/4] autofs: add comment about autofs_mountpoint_changed()

[Ian Kent]

On Fri, 2020-03-27 at 05:19 +0000, McIntyre, Vincent (CASS, Marsfield)
wrote:
> One nit, below.

Yeah, thanks for that, you effort looking at the patches is
appreciated, I'll fix it.

> Vince
> 
> On Thu, Mar 26, 2020 at 01:23:36PM +0800, Ian Kent wrote:
> > The function autofs_mountpoint_changed() is unusual, add a comment
> > about two cases for which it is used.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> > fs/autofs/root.c |   21 ++++++++++++++++++---
> > 1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/autofs/root.c b/fs/autofs/root.c
> > index 308cc49aca1d..a972bbaecb46 100644
> > --- a/fs/autofs/root.c
> > +++ b/fs/autofs/root.c
> > @@ -280,9 +280,24 @@ static struct dentry
> > *autofs_mountpoint_changed(struct path *path)
> > 	struct dentry *dentry = path->dentry;
> > 	struct autofs_sb_info *sbi = autofs_sbi(dentry->d_sb);
> > 
> > -	/*
> > -	 * If this is an indirect mount the dentry could have gone away
> > -	 * as a result of an expire and a new one created.
> > +	/* If this is an indirect mount the dentry could have gone away
> > +	 * and a new one created.
> > +	 *
> > +	 * This is unusual and I can't remember the case for which it
> > +	 * was originally added now. But a example of how this can
> 
> 'an' example
> 
> > +	 * happen is an autofs indirect mount that has the "browse"
> > +	 * option set and also has the "symlink" option in the autofs
> > +	 * map entry. In this case the daemon will remove the browse
> > +	 * directory and create a symlink as the mount (pointing to a
> > +	 * local path) leaving the struct path stale.
> > +	 *
> > +	 * Another not so obvious case is when a mount in an autofs
> > +	 * indirect mount that uses the "nobrowse" option is being
> > +	 * expired and the mount has been umounted but the mount point
> > +	 * directory remains when a stat family system call is made.
> > +	 * In this case the mount point is removed (by the daemon) and
> > +	 * a new mount triggered leading to a stale dentry in the
> > struct
> > +	 * path of the waiting process.
> > 	 */
> > 	if (autofs_type_indirect(sbi->type) && d_unhashed(dentry)) {
> > 		struct dentry *parent = dentry->d_parent;
> > 
> 
> -- 

[PATCH 1/4] autofs: dont call do_expire_wait() in autofs_d_manage()

[Ian Kent]

Calling do_expire_wait() in autofs_d_manage() isn't really necessary.

If in rcu-walk mode -ECHILD will be returned and if in ref-walk mode
and the dentry might be picked for expire (or is expiring) 0 will be
returned otherwise it waits for the expire.

But waiting is meant to be done in autofs_d_automount() so simplify
autofs_d_manage() by testing the expire status and returning only
what's needed.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/root.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/autofs/root.c b/fs/autofs/root.c
index 5aaa1732bf1e..a3b7c72a298d 100644
--- a/fs/autofs/root.c
+++ b/fs/autofs/root.c
@@ -410,9 +410,12 @@ static int autofs_d_manage(const struct path *path, bool rcu_walk)
 		return 0;
 	}
 
-	/* Wait for pending expires */
-	if (do_expire_wait(path, rcu_walk) == -ECHILD)
-		return -ECHILD;
+	/* Check for (possible) pending expire */
+	if (ino->flags & AUTOFS_INF_WANT_EXPIRE) {
+		if (rcu_walk)
+			return -ECHILD;
+		return 0;
+	}
 
 	/*
 	 * This dentry may be under construction so wait on mount
@@ -432,8 +435,6 @@ static int autofs_d_manage(const struct path *path, bool rcu_walk)
 		 */
 		struct inode *inode;
 
-		if (ino->flags & AUTOFS_INF_WANT_EXPIRE)
-			return 0;
 		if (path_is_mountpoint(path))
 			return 0;
 		inode = d_inode_rcu(dentry);

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

[Ian Kent]

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 <raven@themaw.net>
---
 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..b3f748e4df08 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 wait on the expire outcome and possibly
+		 * attempt a re-mount.
+		 */
+		return 1;
 	}
 
 	/*
diff --git a/fs/namei.c b/fs/namei.c
index db6565c99825..869e0d4bb4d9 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
+	 * to be managing. For autofs, a return of 1 it tells us the
+	 * dentry might be expired, so proceed to ->d_automount().
 	 */
 	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)

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

[McIntyre, Vincent (CASS, Marsfield)]

On Tue, Mar 31, 2020 at 07:07:02AM +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 <raven@themaw.net>
>---
> 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..b3f748e4df08 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 wait on the expire outcome and possibly
>+		 * attempt a re-mount.
>+		 */
>+		return 1;
> 	}
>
> 	/*
>diff --git a/fs/namei.c b/fs/namei.c
>index db6565c99825..869e0d4bb4d9 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
>+	 * to be managing. For autofs, a return of 1 it tells us the

Unclear. Do you mean "to be managed." ? Or "managing." ?

Cheers
Vince

>+	 * dentry might be expired, so proceed to ->d_automount().
> 	 */
> 	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)
>

-- 

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

[Ian Kent]

On Mon, 2020-03-30 at 23:20 +0000, McIntyre, Vincent (CASS, Marsfield)
wrote:
> On Tue, Mar 31, 2020 at 07:07:02AM +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 <raven@themaw.net>
> > ---
> > 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..b3f748e4df08 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 wait on the expire outcome and possibly
> > +		 * attempt a re-mount.
> > +		 */
> > +		return 1;
> > 	}
> > 
> > 	/*
> > diff --git a/fs/namei.c b/fs/namei.c
> > index db6565c99825..869e0d4bb4d9 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
> > +	 * to be managing. For autofs, a return of 1 it tells us the
> 
> Unclear. Do you mean "to be managed." ? Or "managing." ?

Right, and I didn't label these v2 either, bit stressed at the
moment I guess.

Ian
> 
> Cheers
> Vince
> 
> > +	 * dentry might be expired, so proceed to ->d_automount().
> > 	 */
> > 	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)
> > 
> 
> --