lkml.org 
[lkml]   [2014]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH 3/6] autofs4: allow RCU-walk to walk through autofs4.
    From
    Date
    On Thu, 2014-07-10 at 09:41 +1000, NeilBrown wrote:
    > Any attempt to look up a pathname that passes though an
    > autofs4 mount is currently forced out of RCU-walk into
    > REF-walk.
    >
    > This can significantly hurt performance of many-thread work
    > loads on many-core systems, especially if the automounted
    > filesystem supports RCU-walk but doesn't get to benefit from
    > it.
    >
    > So if autofs4_d_manage is called with rcu_walk set, only
    > fail with -ECHILD if it is necessary to wait longer than
    > a spinlock, and avoid even the spinlock in one trivial case.

    I've looked more closely at this one now and mostly it looks good to me.

    But I'm probably a bit slow, there's just one place where I can't work
    out the reasoning ....

    >
    > Signed-off-by: NeilBrown <neilb@suse.de>
    > ---
    > fs/autofs4/autofs_i.h | 2 +-
    > fs/autofs4/dev-ioctl.c | 2 +-
    > fs/autofs4/expire.c | 4 +++-
    > fs/autofs4/root.c | 44 +++++++++++++++++++++++++++++---------------
    > 4 files changed, 34 insertions(+), 18 deletions(-)
    >
    > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
    > index 22a280151e45..99dbb05d6148 100644
    > --- a/fs/autofs4/autofs_i.h
    > +++ b/fs/autofs4/autofs_i.h
    > @@ -148,7 +148,7 @@ void autofs4_free_ino(struct autofs_info *);
    >
    > /* Expiration */
    > int is_autofs4_dentry(struct dentry *);
    > -int autofs4_expire_wait(struct dentry *dentry);
    > +int autofs4_expire_wait(struct dentry *dentry, int rcu_walk);
    > int autofs4_expire_run(struct super_block *, struct vfsmount *,
    > struct autofs_sb_info *,
    > struct autofs_packet_expire __user *);
    > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
    > index 5b570b6efa28..aaf96cb25452 100644
    > --- a/fs/autofs4/dev-ioctl.c
    > +++ b/fs/autofs4/dev-ioctl.c
    > @@ -450,7 +450,7 @@ static int autofs_dev_ioctl_requester(struct file *fp,
    > ino = autofs4_dentry_ino(path.dentry);
    > if (ino) {
    > err = 0;
    > - autofs4_expire_wait(path.dentry);
    > + autofs4_expire_wait(path.dentry, 0);
    > spin_lock(&sbi->fs_lock);
    > param->requester.uid = from_kuid_munged(current_user_ns(), ino->uid);
    > param->requester.gid = from_kgid_munged(current_user_ns(), ino->gid);
    > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
    > index a7be57e39be7..7e2f22ce6954 100644
    > --- a/fs/autofs4/expire.c
    > +++ b/fs/autofs4/expire.c
    > @@ -467,7 +467,7 @@ found:
    > return expired;
    > }
    >
    > -int autofs4_expire_wait(struct dentry *dentry)
    > +int autofs4_expire_wait(struct dentry *dentry, int rcu_walk)
    > {
    > struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
    > struct autofs_info *ino = autofs4_dentry_ino(dentry);
    > @@ -477,6 +477,8 @@ int autofs4_expire_wait(struct dentry *dentry)
    > spin_lock(&sbi->fs_lock);
    > if (ino->flags & AUTOFS_INF_EXPIRING) {
    > spin_unlock(&sbi->fs_lock);
    > + if (rcu_walk)
    > + return -ECHILD;
    >
    > DPRINTK("waiting for expire %p name=%.*s",
    > dentry, dentry->d_name.len, dentry->d_name.name);
    > diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
    > index cc87c1abac97..1ad119407e2f 100644
    > --- a/fs/autofs4/root.c
    > +++ b/fs/autofs4/root.c
    > @@ -208,7 +208,8 @@ next:
    > return NULL;
    > }
    >
    > -static struct dentry *autofs4_lookup_expiring(struct dentry *dentry)
    > +static struct dentry *autofs4_lookup_expiring(struct dentry *dentry,
    > + bool rcu_walk)
    > {
    > struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
    > struct dentry *parent = dentry->d_parent;
    > @@ -225,6 +226,11 @@ static struct dentry *autofs4_lookup_expiring(struct dentry *dentry)
    > struct dentry *expiring;
    > 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;
    >
    > @@ -260,13 +266,15 @@ next:
    > return NULL;
    > }
    >
    > -static int autofs4_mount_wait(struct dentry *dentry)
    > +static int autofs4_mount_wait(struct dentry *dentry, bool rcu_walk)
    > {
    > struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
    > struct autofs_info *ino = autofs4_dentry_ino(dentry);
    > int status = 0;
    >
    > if (ino->flags & AUTOFS_INF_PENDING) {
    > + if (rcu_walk)
    > + return -ECHILD;
    > DPRINTK("waiting for mount name=%.*s",
    > dentry->d_name.len, dentry->d_name.name);
    > status = autofs4_wait(sbi, dentry, NFY_MOUNT);
    > @@ -276,20 +284,22 @@ static int autofs4_mount_wait(struct dentry *dentry)
    > return status;
    > }
    >
    > -static int do_expire_wait(struct dentry *dentry)
    > +static int do_expire_wait(struct dentry *dentry, bool rcu_walk)
    > {
    > struct dentry *expiring;
    >
    > - expiring = autofs4_lookup_expiring(dentry);
    > + expiring = autofs4_lookup_expiring(dentry, rcu_walk);
    > + if (IS_ERR(expiring))
    > + return PTR_ERR(expiring);
    > if (!expiring)
    > - return autofs4_expire_wait(dentry);
    > + return autofs4_expire_wait(dentry, rcu_walk);
    > else {
    > /*
    > * If we are racing with expire the request might not
    > * be quite complete, but the directory has been removed
    > * so it must have been successful, just wait for it.
    > */
    > - autofs4_expire_wait(expiring);
    > + autofs4_expire_wait(expiring, 0);
    > autofs4_del_expiring(expiring);
    > dput(expiring);
    > }
    > @@ -341,7 +351,7 @@ static struct vfsmount *autofs4_d_automount(struct path *path)
    > * and the directory was removed, so just go ahead and try
    > * the mount.
    > */
    > - status = do_expire_wait(dentry);
    > + status = do_expire_wait(dentry, 0);
    > if (status && status != -EAGAIN)
    > return NULL;
    >
    > @@ -349,7 +359,7 @@ static struct vfsmount *autofs4_d_automount(struct path *path)
    > spin_lock(&sbi->fs_lock);
    > if (ino->flags & AUTOFS_INF_PENDING) {
    > spin_unlock(&sbi->fs_lock);
    > - status = autofs4_mount_wait(dentry);
    > + status = autofs4_mount_wait(dentry, 0);
    > if (status)
    > return ERR_PTR(status);
    > goto done;
    > @@ -390,7 +400,7 @@ static struct vfsmount *autofs4_d_automount(struct path *path)
    > }
    > ino->flags |= AUTOFS_INF_PENDING;
    > spin_unlock(&sbi->fs_lock);
    > - status = autofs4_mount_wait(dentry);
    > + status = autofs4_mount_wait(dentry, 0);
    > spin_lock(&sbi->fs_lock);
    > ino->flags &= ~AUTOFS_INF_PENDING;
    > if (status) {
    > @@ -426,21 +436,25 @@ static int autofs4_d_manage(struct dentry *dentry, bool rcu_walk)
    > return 0;
    > }
    >
    > - /* We need to sleep, so we need pathwalk to be in ref-mode */
    > - if (rcu_walk)
    > - return -ECHILD;
    > -
    > /* Wait for pending expires */
    > - do_expire_wait(dentry);
    > + if (do_expire_wait(dentry, rcu_walk) == -ECHILD)
    > + return -ECHILD;
    >
    > /*
    > * This dentry may be under construction so wait on mount
    > * completion.
    > */
    > - status = autofs4_mount_wait(dentry);
    > + status = autofs4_mount_wait(dentry, rcu_walk);
    > if (status)
    > return status;
    >
    > + if (rcu_walk)
    > + /* No need to consider returning -EISDIR as
    > + * there is no risk that ->d_automount will
    > + * be called
    > + */
    > + return status;
    > +

    This bit here.

    The test below says (accepting that d_mountpoint() is unreliable in a
    multi-namespace environment and needs to be fixed), if this dentry isn't
    being expired and it's not a mount and the directory isn't empty then we
    can walk straight through the dentry.

    For certain automount map constructs it's possible for a dentry that is
    not itself a mount point to be expired in the same way a mount or mount
    tree is expired.

    What's the thinking behind this one please?

    > spin_lock(&sbi->fs_lock);
    > /*
    > * If the dentry has been selected for expire while we slept
    >
    >




    \
     
     \ /
      Last update: 2014-07-16 07:42    [W:4.394 / U:0.284 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site