* [PATCH 1/2] autofs: revert take more care to not update last_used on path walk
@ 2017-11-27 1:48 ` Ian Kent
0 siblings, 0 replies; 5+ messages in thread
From: Ian Kent @ 2017-11-27 1:48 UTC (permalink / raw)
To: Andrew Morton
Cc: Colin Walters, Ondrej Holy, autofs mailing list, NeilBrown,
Kernel Mailing List, David Howells, Al Viro
While the patch of commit 092a53452b helped (partially) resolve a
problem where automounts were not expiring due to aggressive accesses
from user space it has a side effect for very large environments.
This change helps with the expire problem by making the expire more
aggressive but, for very large environments, that means more mount
requests from clients. When there are a lot of clients that can mean
fairly significant server load increases.
It turns out I put the last_used in this position to solve this
very problem and failed to update my own thinking of the autofs
expire policy. So the patch being reverted introduces a regression
which should be fixed.
Reverts: 092a53452b
Signed-off-by: Ian Kent <raven@themaw.net>
Cc: Neil Brown <neilb@suse.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
---
fs/autofs4/root.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index d79ced925861..82e8f6edfb48 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -281,8 +281,8 @@ static int autofs4_mount_wait(const struct path *path, bool rcu_walk)
pr_debug("waiting for mount name=%pd\n", path->dentry);
status = autofs4_wait(sbi, path, NFY_MOUNT);
pr_debug("mount wait done status=%d\n", status);
- ino->last_used = jiffies;
}
+ ino->last_used = jiffies;
return status;
}
@@ -321,21 +321,16 @@ static struct dentry *autofs4_mountpoint_changed(struct path *path)
*/
if (autofs_type_indirect(sbi->type) && d_unhashed(dentry)) {
struct dentry *parent = dentry->d_parent;
+ struct autofs_info *ino;
struct dentry *new;
new = d_lookup(parent, &dentry->d_name);
if (!new)
return NULL;
- if (new == dentry)
- dput(new);
- else {
- struct autofs_info *ino;
-
- ino = autofs4_dentry_ino(new);
- ino->last_used = jiffies;
- dput(path->dentry);
- path->dentry = new;
- }
+ ino = autofs4_dentry_ino(new);
+ ino->last_used = jiffies;
+ dput(path->dentry);
+ path->dentry = new;
}
return path->dentry;
}
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] autofs: revert fix AT_NO_AUTOMOUNT not being honored
2017-11-27 1:48 ` Ian Kent
@ 2017-11-27 1:48 ` Ian Kent
-1 siblings, 0 replies; 5+ messages in thread
From: Ian Kent @ 2017-11-27 1:48 UTC (permalink / raw)
To: Andrew Morton
Cc: Colin Walters, Ondrej Holy, autofs mailing list, NeilBrown,
Kernel Mailing List, David Howells, Al Viro
Commit 42f4614821 allowed the fstatat(2) system call to properly honor
the AT_NO_AUTOMOUNT flag but introduced a semantic change.
In order to honor AT_NO_AUTOMOUNT a semantic change was made to the
negative dentry case for stat family system calls in follow_automount().
This changed the unconditional triggering of an automount in this case
to no longer be done and an error returned instead.
This has caused more problems than I expected so reverting the change
is needed.
In a discussion with Neil Brown it was concluded that the automount(8)
daemon can implement this change without kernel modifications. So that
will be done instead and the autofs module documentation updated with
a description of the problem and what needs to be done by module users
for this specific case.
Reverts: 42f4614821
Signed-off-by: Ian Kent <raven@themaw.net>
Cc: Neil Brown <neilb@suse.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: David Howells <dhowells@redhat.com>
Cc: Colin Walters <walters@redhat.com>
Cc: Ondrej Holy <oholy@redhat.com>
---
fs/namei.c | 15 +++------------
include/linux/fs.h | 3 ++-
2 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index f0c7a7b9b6ca..9cc91fb7f156 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1129,18 +1129,9 @@ static int follow_automount(struct path *path, struct nameidata *nd,
* of the daemon to instantiate them before they can be used.
*/
if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
- LOOKUP_OPEN | LOOKUP_CREATE |
- LOOKUP_AUTOMOUNT))) {
- /* Positive dentry that isn't meant to trigger an
- * automount, EISDIR will allow it to be used,
- * otherwise there's no mount here "now" so return
- * ENOENT.
- */
- if (path->dentry->d_inode)
- return -EISDIR;
- else
- return -ENOENT;
- }
+ LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT)) &&
+ path->dentry->d_inode)
+ return -EISDIR;
if (path->dentry->d_sb->s_user_ns != &init_user_ns)
return -EACCES;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2995a271ec46..9c4a43e391ac 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3088,7 +3088,8 @@ static inline int vfs_lstat(const char __user *name, struct kstat *stat)
static inline int vfs_fstatat(int dfd, const char __user *filename,
struct kstat *stat, int flags)
{
- return vfs_statx(dfd, filename, flags, stat, STATX_BASIC_STATS);
+ return vfs_statx(dfd, filename, flags | AT_NO_AUTOMOUNT,
+ stat, STATX_BASIC_STATS);
}
static inline int vfs_fstat(int fd, struct kstat *stat)
{
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] autofs: revert fix AT_NO_AUTOMOUNT not being honored
@ 2017-11-27 1:48 ` Ian Kent
0 siblings, 0 replies; 5+ messages in thread
From: Ian Kent @ 2017-11-27 1:48 UTC (permalink / raw)
To: Andrew Morton
Cc: Colin Walters, Ondrej Holy, autofs mailing list, NeilBrown,
Kernel Mailing List, David Howells, Al Viro
Commit 42f4614821 allowed the fstatat(2) system call to properly honor
the AT_NO_AUTOMOUNT flag but introduced a semantic change.
In order to honor AT_NO_AUTOMOUNT a semantic change was made to the
negative dentry case for stat family system calls in follow_automount().
This changed the unconditional triggering of an automount in this case
to no longer be done and an error returned instead.
This has caused more problems than I expected so reverting the change
is needed.
In a discussion with Neil Brown it was concluded that the automount(8)
daemon can implement this change without kernel modifications. So that
will be done instead and the autofs module documentation updated with
a description of the problem and what needs to be done by module users
for this specific case.
Reverts: 42f4614821
Signed-off-by: Ian Kent <raven@themaw.net>
Cc: Neil Brown <neilb@suse.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: David Howells <dhowells@redhat.com>
Cc: Colin Walters <walters@redhat.com>
Cc: Ondrej Holy <oholy@redhat.com>
---
fs/namei.c | 15 +++------------
include/linux/fs.h | 3 ++-
2 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index f0c7a7b9b6ca..9cc91fb7f156 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1129,18 +1129,9 @@ static int follow_automount(struct path *path, struct nameidata *nd,
* of the daemon to instantiate them before they can be used.
*/
if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY |
- LOOKUP_OPEN | LOOKUP_CREATE |
- LOOKUP_AUTOMOUNT))) {
- /* Positive dentry that isn't meant to trigger an
- * automount, EISDIR will allow it to be used,
- * otherwise there's no mount here "now" so return
- * ENOENT.
- */
- if (path->dentry->d_inode)
- return -EISDIR;
- else
- return -ENOENT;
- }
+ LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT)) &&
+ path->dentry->d_inode)
+ return -EISDIR;
if (path->dentry->d_sb->s_user_ns != &init_user_ns)
return -EACCES;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2995a271ec46..9c4a43e391ac 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3088,7 +3088,8 @@ static inline int vfs_lstat(const char __user *name, struct kstat *stat)
static inline int vfs_fstatat(int dfd, const char __user *filename,
struct kstat *stat, int flags)
{
- return vfs_statx(dfd, filename, flags, stat, STATX_BASIC_STATS);
+ return vfs_statx(dfd, filename, flags | AT_NO_AUTOMOUNT,
+ stat, STATX_BASIC_STATS);
}
static inline int vfs_fstat(int fd, struct kstat *stat)
{
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] autofs: revert take more care to not update last_used on path walk
2017-11-27 1:48 ` Ian Kent
(?)
(?)
@ 2017-11-28 22:48 ` NeilBrown
-1 siblings, 0 replies; 5+ messages in thread
From: NeilBrown @ 2017-11-28 22:48 UTC (permalink / raw)
To: Ian Kent, Andrew Morton
Cc: Colin Walters, Ondrej Holy, autofs mailing list,
Kernel Mailing List, David Howells, Al Viro
[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]
On Mon, Nov 27 2017, Ian Kent wrote:
> While the patch of commit 092a53452b helped (partially) resolve a
> problem where automounts were not expiring due to aggressive accesses
> from user space it has a side effect for very large environments.
>
> This change helps with the expire problem by making the expire more
> aggressive but, for very large environments, that means more mount
> requests from clients. When there are a lot of clients that can mean
> fairly significant server load increases.
>
> It turns out I put the last_used in this position to solve this
> very problem and failed to update my own thinking of the autofs
> expire policy. So the patch being reverted introduces a regression
> which should be fixed.
>
> Reverts: 092a53452b
I would add:
Fixes: 092a53452bb7 ("autofs: take more care to not update last_used on path walk")
Cc: stable@vger.kernel.org (v4.11+)
to ensure these are picked up as needed (different Fixes line for second
of course).
Reviewed-by: NeilBrown <neilb@suse.com>
Thanks,
NeilBrown
>
> Signed-off-by: Ian Kent <raven@themaw.net>
> Cc: Neil Brown <neilb@suse.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> ---
> fs/autofs4/root.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index d79ced925861..82e8f6edfb48 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -281,8 +281,8 @@ static int autofs4_mount_wait(const struct path *path, bool rcu_walk)
> pr_debug("waiting for mount name=%pd\n", path->dentry);
> status = autofs4_wait(sbi, path, NFY_MOUNT);
> pr_debug("mount wait done status=%d\n", status);
> - ino->last_used = jiffies;
> }
> + ino->last_used = jiffies;
> return status;
> }
>
> @@ -321,21 +321,16 @@ static struct dentry *autofs4_mountpoint_changed(struct path *path)
> */
> if (autofs_type_indirect(sbi->type) && d_unhashed(dentry)) {
> struct dentry *parent = dentry->d_parent;
> + struct autofs_info *ino;
> struct dentry *new;
>
> new = d_lookup(parent, &dentry->d_name);
> if (!new)
> return NULL;
> - if (new == dentry)
> - dput(new);
> - else {
> - struct autofs_info *ino;
> -
> - ino = autofs4_dentry_ino(new);
> - ino->last_used = jiffies;
> - dput(path->dentry);
> - path->dentry = new;
> - }
> + ino = autofs4_dentry_ino(new);
> + ino->last_used = jiffies;
> + dput(path->dentry);
> + path->dentry = new;
> }
> return path->dentry;
> }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread