[PATCH 1/7] autofs4 - don't make expiring dentry negative - Kernel

This is a discussion on [PATCH 1/7] autofs4 - don't make expiring dentry negative - Kernel ; Correct the error of making a positive dentry negative after it has been instantiated. The code that makes this error attempts to re-use the dentry from a concurrent expire and mount to resolve a race and the dentry used for ...

+ Reply to Thread
Results 1 to 11 of 11

Thread: [PATCH 1/7] autofs4 - don't make expiring dentry negative

  1. [PATCH 1/7] autofs4 - don't make expiring dentry negative

    Correct the error of making a positive dentry negative after it has been
    instantiated.

    The code that makes this error attempts to re-use the dentry from a
    concurrent expire and mount to resolve a race and the dentry used for
    the lookup must be negative for mounts to trigger in the required
    cases. The fact is that the dentry doesn't need to be re-used because
    all that is needed is to preserve the flag that indicates an expire is
    still incomplete at the time of the mount request.

    This change uses the the dentry to check the flag and wait for the
    expire to complete then discards it instead of attempting to re-use it.

    Signed-off-by: Ian Kent

    ---

    fs/autofs4/autofs_i.h | 6 +--
    fs/autofs4/inode.c | 6 +--
    fs/autofs4/root.c | 115 ++++++++++++++++++-------------------------------
    3 files changed, 49 insertions(+), 78 deletions(-)


    diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
    index c3d352d..69b1497 100644
    --- a/fs/autofs4/autofs_i.h
    +++ b/fs/autofs4/autofs_i.h
    @@ -52,7 +52,7 @@ struct autofs_info {

    int flags;

    - struct list_head rehash;
    + struct list_head expiring;

    struct autofs_sb_info *sbi;
    unsigned long last_used;
    @@ -112,8 +112,8 @@ struct autofs_sb_info {
    struct mutex wq_mutex;
    spinlock_t fs_lock;
    struct autofs_wait_queue *queues; /* Wait queue pointer */
    - spinlock_t rehash_lock;
    - struct list_head rehash_list;
    + spinlock_t lookup_lock;
    + struct list_head expiring_list;
    };

    static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
    diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
    index 2fdcf5e..94bfc15 100644
    --- a/fs/autofs4/inode.c
    +++ b/fs/autofs4/inode.c
    @@ -47,7 +47,7 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
    ino->dentry = NULL;
    ino->size = 0;

    - INIT_LIST_HEAD(&ino->rehash);
    + INIT_LIST_HEAD(&ino->expiring);

    ino->last_used = jiffies;
    atomic_set(&ino->count, 0);
    @@ -338,8 +338,8 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
    mutex_init(&sbi->wq_mutex);
    spin_lock_init(&sbi->fs_lock);
    sbi->queues = NULL;
    - spin_lock_init(&sbi->rehash_lock);
    - INIT_LIST_HEAD(&sbi->rehash_list);
    + spin_lock_init(&sbi->lookup_lock);
    + INIT_LIST_HEAD(&sbi->expiring_list);
    s->s_blocksize = 1024;
    s->s_blocksize_bits = 10;
    s->s_magic = AUTOFS_SUPER_MAGIC;
    diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
    index edf5b6b..2e8959c 100644
    --- a/fs/autofs4/root.c
    +++ b/fs/autofs4/root.c
    @@ -493,10 +493,10 @@ void autofs4_dentry_release(struct dentry *de)
    struct autofs_sb_info *sbi = autofs4_sbi(de->d_sb);

    if (sbi) {
    - spin_lock(&sbi->rehash_lock);
    - if (!list_empty(&inf->rehash))
    - list_del(&inf->rehash);
    - spin_unlock(&sbi->rehash_lock);
    + spin_lock(&sbi->lookup_lock);
    + if (!list_empty(&inf->expiring))
    + list_del(&inf->expiring);
    + spin_unlock(&sbi->lookup_lock);
    }

    inf->dentry = NULL;
    @@ -518,7 +518,7 @@ static struct dentry_operations autofs4_dentry_operations = {
    .d_release = autofs4_dentry_release,
    };

    -static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
    +static struct dentry *autofs4_lookup_expiring(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
    {
    unsigned int len = name->len;
    unsigned int hash = name->hash;
    @@ -526,14 +526,14 @@ static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct
    struct list_head *p, *head;

    spin_lock(&dcache_lock);
    - spin_lock(&sbi->rehash_lock);
    - head = &sbi->rehash_list;
    + spin_lock(&sbi->lookup_lock);
    + head = &sbi->expiring_list;
    list_for_each(p, head) {
    struct autofs_info *ino;
    struct dentry *dentry;
    struct qstr *qstr;

    - ino = list_entry(p, struct autofs_info, rehash);
    + ino = list_entry(p, struct autofs_info, expiring);
    dentry = ino->dentry;

    spin_lock(&dentry->d_lock);
    @@ -555,33 +555,17 @@ static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct
    goto next;

    if (d_unhashed(dentry)) {
    - struct inode *inode = dentry->d_inode;
    -
    - ino = autofs4_dentry_ino(dentry);
    - list_del_init(&ino->rehash);
    + list_del_init(&ino->expiring);
    dget(dentry);
    - /*
    - * Make the rehashed dentry negative so the VFS
    - * behaves as it should.
    - */
    - if (inode) {
    - dentry->d_inode = NULL;
    - list_del_init(&dentry->d_alias);
    - spin_unlock(&dentry->d_lock);
    - spin_unlock(&sbi->rehash_lock);
    - spin_unlock(&dcache_lock);
    - iput(inode);
    - return dentry;
    - }
    spin_unlock(&dentry->d_lock);
    - spin_unlock(&sbi->rehash_lock);
    + spin_unlock(&sbi->lookup_lock);
    spin_unlock(&dcache_lock);
    return dentry;
    }
    next:
    spin_unlock(&dentry->d_lock);
    }
    - spin_unlock(&sbi->rehash_lock);
    + spin_unlock(&sbi->lookup_lock);
    spin_unlock(&dcache_lock);

    return NULL;
    @@ -591,7 +575,7 @@ next:
    static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
    {
    struct autofs_sb_info *sbi;
    - struct dentry *unhashed;
    + struct dentry *expiring;
    int oz_mode;

    DPRINTK("name = %.*s",
    @@ -607,44 +591,40 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
    DPRINTK("pid = %u, pgrp = %u, catatonic = %d, oz_mode = %d",
    current->pid, task_pgrp_nr(current), sbi->catatonic, oz_mode);

    - unhashed = autofs4_lookup_unhashed(sbi, dentry->d_parent, &dentry->d_name);
    - if (!unhashed) {
    - /*
    - * Mark the dentry incomplete but don't hash it. We do this
    - * to serialize our inode creation operations (symlink and
    - * mkdir) which prevents deadlock during the callback to
    - * the daemon. Subsequent user space lookups for the same
    - * dentry are placed on the wait queue while the daemon
    - * itself is allowed passage unresticted so the create
    - * operation itself can then hash the dentry. Finally,
    - * we check for the hashed dentry and return the newly
    - * hashed dentry.
    - */
    - dentry->d_op = &autofs4_root_dentry_operations;
    -
    - dentry->d_fsdata = NULL;
    - d_instantiate(dentry, NULL);
    - } else {
    - struct autofs_info *ino = autofs4_dentry_ino(unhashed);
    - DPRINTK("rehash %p with %p", dentry, unhashed);
    + expiring = autofs4_lookup_expiring(sbi, dentry->d_parent, &dentry->d_name);
    + if (expiring) {
    + struct autofs_info *ino = autofs4_dentry_ino(expiring);
    /*
    * 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, so just wait for it.
    - * We need to ensure the AUTOFS_INF_EXPIRING flag is clear
    - * before continuing as revalidate may fail when calling
    - * try_to_fill_dentry (returning EAGAIN) if we don't.
    */
    while (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
    DPRINTK("wait for incomplete expire %p name=%.*s",
    - unhashed, unhashed->d_name.len,
    - unhashed->d_name.name);
    - autofs4_wait(sbi, unhashed, NFY_NONE);
    + expiring, expiring->d_name.len,
    + expiring->d_name.name);
    + autofs4_wait(sbi, expiring, NFY_NONE);
    DPRINTK("request completed");
    }
    - dentry = unhashed;
    + dput(expiring);
    }

    + /*
    + * Mark the dentry incomplete but don't hash it. We do this
    + * to serialize our inode creation operations (symlink and
    + * mkdir) which prevents deadlock during the callback to
    + * the daemon. Subsequent user space lookups for the same
    + * dentry are placed on the wait queue while the daemon
    + * itself is allowed passage unresticted so the create
    + * operation itself can then hash the dentry. Finally,
    + * we check for the hashed dentry and return the newly
    + * hashed dentry.
    + */
    + dentry->d_op = &autofs4_root_dentry_operations;
    +
    + dentry->d_fsdata = NULL;
    + d_instantiate(dentry, NULL);
    +
    if (!oz_mode) {
    spin_lock(&dentry->d_lock);
    dentry->d_flags |= DCACHE_AUTOFS_PENDING;
    @@ -668,8 +648,6 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
    if (sigismember (sigset, SIGKILL) ||
    sigismember (sigset, SIGQUIT) ||
    sigismember (sigset, SIGINT)) {
    - if (unhashed)
    - dput(unhashed);
    return ERR_PTR(-ERESTARTNOINTR);
    }
    }
    @@ -699,15 +677,9 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
    else
    dentry = ERR_PTR(-ENOENT);

    - if (unhashed)
    - dput(unhashed);
    -
    return dentry;
    }

    - if (unhashed)
    - return dentry;
    -
    return NULL;
    }

    @@ -769,9 +741,8 @@ static int autofs4_dir_symlink(struct inode *dir,
    * that the file no longer exists. However, doing that means that the
    * VFS layer can turn the dentry into a negative dentry. We don't want
    * this, because the unlink is probably the result of an expire.
    - * We simply d_drop it and add it to a rehash candidates list in the
    - * super block, which allows the dentry lookup to reuse it retaining
    - * the flags, such as expire in progress, in case we're racing with expire.
    + * We simply d_drop it and add it to a expiring list in the super block,
    + * which allows the dentry lookup to check for an incomplete expire.
    *
    * If a process is blocked on the dentry waiting for the expire to finish,
    * it will invalidate the dentry and try to mount with a new one.
    @@ -801,9 +772,9 @@ static int autofs4_dir_unlink(struct inode *dir, struct dentry *dentry)
    dir->i_mtime = CURRENT_TIME;

    spin_lock(&dcache_lock);
    - spin_lock(&sbi->rehash_lock);
    - list_add(&ino->rehash, &sbi->rehash_list);
    - spin_unlock(&sbi->rehash_lock);
    + spin_lock(&sbi->lookup_lock);
    + list_add(&ino->expiring, &sbi->expiring_list);
    + spin_unlock(&sbi->lookup_lock);
    spin_lock(&dentry->d_lock);
    __d_drop(dentry);
    spin_unlock(&dentry->d_lock);
    @@ -829,9 +800,9 @@ static int autofs4_dir_rmdir(struct inode *dir, struct dentry *dentry)
    spin_unlock(&dcache_lock);
    return -ENOTEMPTY;
    }
    - spin_lock(&sbi->rehash_lock);
    - list_add(&ino->rehash, &sbi->rehash_list);
    - spin_unlock(&sbi->rehash_lock);
    + spin_lock(&sbi->lookup_lock);
    + list_add(&ino->expiring, &sbi->expiring_list);
    + spin_unlock(&sbi->lookup_lock);
    spin_lock(&dentry->d_lock);
    __d_drop(dentry);
    spin_unlock(&dentry->d_lock);
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. [PATCH 2/7] autofs4 - revert - redo lookup in ttfd

    This patch series enables the use of a single dentry for lookups
    prior to the dentry being hashed and so we no longer need to
    redo the lookup. This patch reverts the patch of commit
    033790449ba9c4dcf8478a87693d33df625c23b5.

    Signed-off-by: Ian Kent

    ---

    fs/autofs4/root.c | 21 ---------------------
    1 files changed, 0 insertions(+), 21 deletions(-)


    diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
    index 2e8959c..8511cb2 100644
    --- a/fs/autofs4/root.c
    +++ b/fs/autofs4/root.c
    @@ -242,7 +242,6 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
    {
    struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
    struct autofs_info *ino = autofs4_dentry_ino(dentry);
    - struct dentry *new;
    int status;

    /* Block on any pending expiry here; invalidate the dentry
    @@ -320,26 +319,6 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
    dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
    spin_unlock(&dentry->d_lock);

    - /*
    - * The dentry that is passed in from lookup may not be the one
    - * we end up using, as mkdir can create a new one. If this
    - * happens, and another process tries the lookup at the same time,
    - * it will set the PENDING flag on this new dentry, but add itself
    - * to our waitq. Then, if after the lookup succeeds, the first
    - * process that requested the mount performs another lookup of the
    - * same directory, it will show up as still pending! So, we need
    - * to redo the lookup here and clear pending on that dentry.
    - */
    - if (d_unhashed(dentry)) {
    - new = d_lookup(dentry->d_parent, &dentry->d_name);
    - if (new) {
    - spin_lock(&new->d_lock);
    - new->d_flags &= ~DCACHE_AUTOFS_PENDING;
    - spin_unlock(&new->d_lock);
    - dput(new);
    - }
    - }
    -
    return 0;
    }

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. [PATCH 4/7] autofs4 - don't release directory mutex if called in oz_mode

    Since we now delay hashing of dentrys until the ->mkdir() call,
    droping and re-taking the directory mutex within the ->lookup()
    function when we are being called by user space is not needed.
    This can lead to a race when other processes are attempting to
    access the same directory during mount point directory creation.

    In this case we need to hang onto the mutex to ensure we don't
    get user processes trying to create a mount request for a newly
    created dentry after the mount point entry has already been
    created. This ensures that when we need to check a dentry passed
    to autofs4_wait(), if it is hashed, it is always the mount point
    dentry and not a new dentry created by another lookup during
    directory creation.

    Signed-off-by: Ian Kent

    ---

    fs/autofs4/root.c | 11 +++++------
    1 files changed, 5 insertions(+), 6 deletions(-)


    diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
    index 7fa303d..236eb9b 100644
    --- a/fs/autofs4/root.c
    +++ b/fs/autofs4/root.c
    @@ -683,12 +683,11 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
    spin_lock(&dentry->d_lock);
    dentry->d_flags |= DCACHE_AUTOFS_PENDING;
    spin_unlock(&dentry->d_lock);
    - }
    -
    - if (dentry->d_op && dentry->d_op->d_revalidate) {
    - mutex_unlock(&dir->i_mutex);
    - (dentry->d_op->d_revalidate)(dentry, nd);
    - mutex_lock(&dir->i_mutex);
    + if (dentry->d_op && dentry->d_op->d_revalidate) {
    + mutex_unlock(&dir->i_mutex);
    + (dentry->d_op->d_revalidate)(dentry, nd);
    + mutex_lock(&dir->i_mutex);
    + }
    }

    /*
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. [PATCH 3/7] autofs4 - use look aside list for lookups

    A while ago a patch to resolve a deadlock during directory creation
    was merged. This delayed the hashing of lookup dentrys until the
    ->mkdir() (or ->symlink()) operation completed to ensure we always
    went through ->lookup() instead of also having processes go through
    ->revalidate() so our VFS locking remained consistent.

    Now we are seeing a couple of side affects of that change in situations
    with heavy mount activity.

    Two cases have been identified:

    1) When a mount request is triggered, due to the delayed hashing, the
    directory created by user space for the mount point doesn't have the
    DCACHE_AUTOFS_PENDING flag set. In the case of an autofs multi-mount
    where a tree of mount point directories are created this can lead to
    the path walk continuing rather than the dentry being sent to the wait
    queue to wait for request completion. This is because, if the pending
    flag isn't set, the criteria for deciding this is a mount in progress
    fails to hold, namely that the dentry is not a mount point and has no
    subdirectories.

    2) A mount request dentry is initially created negative and unhashed.
    It remains this way until the ->mkdir() callback completes. Since it is
    unhashed a fresh dentry is used when the user space mount request creates
    the mount point directory. This leaves the original dentry negative and
    unhashed. But revalidate has no way to tell the VFS that the dentry has
    changed, other than to force another ->lookup() by returning false, which
    is at best wastefull and at worst not possible. This results in an
    -ENOENT return from the original path walk when in fact the mount succeeded.

    To resolve this we need to ensure that the same dentry is used in all
    calls to ->lookup() during the course of a mount request. This patch
    achieves that by adding the initial dentry to a look aside list and
    removes it at ->mkdir() or ->symlink() completion (or when the dentry
    is released), since these are the only create operations autofs4 supports.

    Signed-off-by: Ian Kent

    ---

    fs/autofs4/autofs_i.h | 2 +
    fs/autofs4/inode.c | 25 ++++---
    fs/autofs4/root.c | 169 ++++++++++++++++++++++++++++++++++++++++---------
    3 files changed, 156 insertions(+), 40 deletions(-)


    diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
    index 69b1497..2dce233 100644
    --- a/fs/autofs4/autofs_i.h
    +++ b/fs/autofs4/autofs_i.h
    @@ -52,6 +52,7 @@ struct autofs_info {

    int flags;

    + struct list_head active;
    struct list_head expiring;

    struct autofs_sb_info *sbi;
    @@ -113,6 +114,7 @@ struct autofs_sb_info {
    spinlock_t fs_lock;
    struct autofs_wait_queue *queues; /* Wait queue pointer */
    spinlock_t lookup_lock;
    + struct list_head active_list;
    struct list_head expiring_list;
    };

    diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
    index 94bfc15..e3e7099 100644
    --- a/fs/autofs4/inode.c
    +++ b/fs/autofs4/inode.c
    @@ -24,8 +24,10 @@

    static void ino_lnkfree(struct autofs_info *ino)
    {
    - kfree(ino->u.symlink);
    - ino->u.symlink = NULL;
    + if (ino->u.symlink) {
    + kfree(ino->u.symlink);
    + ino->u.symlink = NULL;
    + }
    }

    struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
    @@ -41,16 +43,18 @@ struct autofs_info *autofs4_init_ino(struct autofs_info *ino,
    if (ino == NULL)
    return NULL;

    - ino->flags = 0;
    - ino->mode = mode;
    - ino->inode = NULL;
    - ino->dentry = NULL;
    - ino->size = 0;
    -
    - INIT_LIST_HEAD(&ino->expiring);
    + if (!reinit) {
    + ino->flags = 0;
    + ino->inode = NULL;
    + ino->dentry = NULL;
    + ino->size = 0;
    + INIT_LIST_HEAD(&ino->active);
    + INIT_LIST_HEAD(&ino->expiring);
    + atomic_set(&ino->count, 0);
    + }

    + ino->mode = mode;
    ino->last_used = jiffies;
    - atomic_set(&ino->count, 0);

    ino->sbi = sbi;

    @@ -339,6 +343,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
    spin_lock_init(&sbi->fs_lock);
    sbi->queues = NULL;
    spin_lock_init(&sbi->lookup_lock);
    + INIT_LIST_HEAD(&sbi->active_list);
    INIT_LIST_HEAD(&sbi->expiring_list);
    s->s_blocksize = 1024;
    s->s_blocksize_bits = 10;
    diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
    index 8511cb2..7fa303d 100644
    --- a/fs/autofs4/root.c
    +++ b/fs/autofs4/root.c
    @@ -473,6 +473,8 @@ void autofs4_dentry_release(struct dentry *de)

    if (sbi) {
    spin_lock(&sbi->lookup_lock);
    + if (!list_empty(&inf->active))
    + list_del(&inf->active);
    if (!list_empty(&inf->expiring))
    list_del(&inf->expiring);
    spin_unlock(&sbi->lookup_lock);
    @@ -497,6 +499,58 @@ static struct dentry_operations autofs4_dentry_operations = {
    .d_release = autofs4_dentry_release,
    };

    +static struct dentry *autofs4_lookup_active(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
    +{
    + unsigned int len = name->len;
    + unsigned int hash = name->hash;
    + const unsigned char *str = name->name;
    + struct list_head *p, *head;
    +
    + spin_lock(&dcache_lock);
    + spin_lock(&sbi->lookup_lock);
    + head = &sbi->active_list;
    + list_for_each(p, head) {
    + struct autofs_info *ino;
    + struct dentry *dentry;
    + struct qstr *qstr;
    +
    + ino = list_entry(p, struct autofs_info, active);
    + dentry = ino->dentry;
    +
    + spin_lock(&dentry->d_lock);
    +
    + /* Already gone? */
    + if (atomic_read(&dentry->d_count) == 0)
    + goto next;
    +
    + qstr = &dentry->d_name;
    +
    + if (dentry->d_name.hash != hash)
    + goto next;
    + if (dentry->d_parent != parent)
    + goto next;
    +
    + if (qstr->len != len)
    + goto next;
    + if (memcmp(qstr->name, str, len))
    + goto next;
    +
    + if (d_unhashed(dentry)) {
    + dget(dentry);
    + spin_unlock(&dentry->d_lock);
    + spin_unlock(&sbi->lookup_lock);
    + spin_unlock(&dcache_lock);
    + return dentry;
    + }
    +next:
    + spin_unlock(&dentry->d_lock);
    + }
    + spin_unlock(&sbi->lookup_lock);
    + spin_unlock(&dcache_lock);
    +
    + return NULL;
    +}
    +
    static struct dentry *autofs4_lookup_expiring(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
    {
    unsigned int len = name->len;
    @@ -554,7 +608,8 @@ next:
    static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
    {
    struct autofs_sb_info *sbi;
    - struct dentry *expiring;
    + struct autofs_info *ino;
    + struct dentry *expiring, *unhashed;
    int oz_mode;

    DPRINTK("name = %.*s",
    @@ -572,12 +627,12 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s

    expiring = autofs4_lookup_expiring(sbi, dentry->d_parent, &dentry->d_name);
    if (expiring) {
    - struct autofs_info *ino = autofs4_dentry_ino(expiring);
    /*
    * 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, so just wait for it.
    */
    + ino = autofs4_dentry_ino(expiring);
    while (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
    DPRINTK("wait for incomplete expire %p name=%.*s",
    expiring, expiring->d_name.len,
    @@ -588,21 +643,41 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
    dput(expiring);
    }

    - /*
    - * Mark the dentry incomplete but don't hash it. We do this
    - * to serialize our inode creation operations (symlink and
    - * mkdir) which prevents deadlock during the callback to
    - * the daemon. Subsequent user space lookups for the same
    - * dentry are placed on the wait queue while the daemon
    - * itself is allowed passage unresticted so the create
    - * operation itself can then hash the dentry. Finally,
    - * we check for the hashed dentry and return the newly
    - * hashed dentry.
    - */
    - dentry->d_op = &autofs4_root_dentry_operations;
    + unhashed = autofs4_lookup_active(sbi, dentry->d_parent, &dentry->d_name);
    + if (unhashed)
    + dentry = unhashed;
    + else {
    + /*
    + * Mark the dentry incomplete but don't hash it. We do this
    + * to serialize our inode creation operations (symlink and
    + * mkdir) which prevents deadlock during the callback to
    + * the daemon. Subsequent user space lookups for the same
    + * dentry are placed on the wait queue while the daemon
    + * itself is allowed passage unresticted so the create
    + * operation itself can then hash the dentry. Finally,
    + * we check for the hashed dentry and return the newly
    + * hashed dentry.
    + */
    + dentry->d_op = &autofs4_root_dentry_operations;

    - dentry->d_fsdata = NULL;
    - d_instantiate(dentry, NULL);
    + /*
    + * And we need to ensure that the same dentry is used for
    + * all following lookup calls until it is hashed so that
    + * the dentry flags are persistent throughout the request.
    + */
    + ino = autofs4_init_ino(NULL, sbi, 0555);
    + if (!ino)
    + return ERR_PTR(-ENOMEM);
    +
    + dentry->d_fsdata = ino;
    + ino->dentry = dentry;
    +
    + spin_lock(&sbi->lookup_lock);
    + list_add(&ino->active, &sbi->active_list);
    + spin_unlock(&sbi->lookup_lock);
    +
    + d_instantiate(dentry, NULL);
    + }

    if (!oz_mode) {
    spin_lock(&dentry->d_lock);
    @@ -627,12 +702,16 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
    if (sigismember (sigset, SIGKILL) ||
    sigismember (sigset, SIGQUIT) ||
    sigismember (sigset, SIGINT)) {
    + if (unhashed)
    + dput(unhashed);
    return ERR_PTR(-ERESTARTNOINTR);
    }
    }
    - spin_lock(&dentry->d_lock);
    - dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
    - spin_unlock(&dentry->d_lock);
    + if (!oz_mode) {
    + spin_lock(&dentry->d_lock);
    + dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
    + spin_unlock(&dentry->d_lock);
    + }
    }

    /*
    @@ -656,9 +735,15 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
    else
    dentry = ERR_PTR(-ENOENT);

    + if (unhashed)
    + dput(unhashed);
    +
    return dentry;
    }

    + if (unhashed)
    + return unhashed;
    +
    return NULL;
    }

    @@ -679,20 +764,30 @@ static int autofs4_dir_symlink(struct inode *dir,
    return -EACCES;

    ino = autofs4_init_ino(ino, sbi, S_IFLNK | 0555);
    - if (ino == NULL)
    - return -ENOSPC;
    + if (!ino)
    + return -ENOMEM;

    - ino->size = strlen(symname);
    - ino->u.symlink = cp = kmalloc(ino->size + 1, GFP_KERNEL);
    + spin_lock(&sbi->lookup_lock);
    + if (!list_empty(&ino->active))
    + list_del_init(&ino->active);
    + spin_unlock(&sbi->lookup_lock);

    - if (cp == NULL) {
    - kfree(ino);
    - return -ENOSPC;
    + cp = kmalloc(ino->size + 1, GFP_KERNEL);
    + if (!cp) {
    + if (!dentry->d_fsdata)
    + kfree(ino);
    + return -ENOMEM;
    }

    strcpy(cp, symname);

    inode = autofs4_get_inode(dir->i_sb, ino);
    + if (!inode) {
    + kfree(cp);
    + if (!dentry->d_fsdata)
    + kfree(ino);
    + return -ENOMEM;
    + }
    d_add(dentry, inode);

    if (dir == dir->i_sb->s_root->d_inode)
    @@ -708,6 +803,8 @@ static int autofs4_dir_symlink(struct inode *dir,
    atomic_inc(&p_ino->count);
    ino->inode = inode;

    + ino->size = strlen(symname);
    + ino->u.symlink = cp;
    dir->i_mtime = CURRENT_TIME;

    return 0;
    @@ -752,7 +849,8 @@ static int autofs4_dir_unlink(struct inode *dir, struct dentry *dentry)

    spin_lock(&dcache_lock);
    spin_lock(&sbi->lookup_lock);
    - list_add(&ino->expiring, &sbi->expiring_list);
    + if (list_empty(&ino->expiring))
    + list_add(&ino->expiring, &sbi->expiring_list);
    spin_unlock(&sbi->lookup_lock);
    spin_lock(&dentry->d_lock);
    __d_drop(dentry);
    @@ -780,7 +878,8 @@ static int autofs4_dir_rmdir(struct inode *dir, struct dentry *dentry)
    return -ENOTEMPTY;
    }
    spin_lock(&sbi->lookup_lock);
    - list_add(&ino->expiring, &sbi->expiring_list);
    + if (list_empty(&ino->expiring))
    + list_add(&ino->expiring, &sbi->expiring_list);
    spin_unlock(&sbi->lookup_lock);
    spin_lock(&dentry->d_lock);
    __d_drop(dentry);
    @@ -816,10 +915,20 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
    dentry, dentry->d_name.len, dentry->d_name.name);

    ino = autofs4_init_ino(ino, sbi, S_IFDIR | 0555);
    - if (ino == NULL)
    - return -ENOSPC;
    + if (!ino)
    + return -ENOMEM;
    +
    + spin_lock(&sbi->lookup_lock);
    + if (!list_empty(&ino->active))
    + list_del_init(&ino->active);
    + spin_unlock(&sbi->lookup_lock);

    inode = autofs4_get_inode(dir->i_sb, ino);
    + if (!inode) {
    + if (!dentry->d_fsdata)
    + kfree(ino);
    + return -ENOMEM;
    + }
    d_add(dentry, inode);

    if (dir == dir->i_sb->s_root->d_inode)
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. [PATCH 7/7] autofs4 - fix pending mount race.

    Close a race between a pending mount that is about to finish and a new
    lookup for the same directory.

    Process P1 triggers a mount of directory foo.
    It sets DCACHE_AUTOFS_PENDING in the ->lookup routine, creates a waitq
    entry for 'foo', and calls out to the daemon to perform the mount.
    The autofs daemon will then create the directory 'foo', using a new dentry
    that will be hashed in the dcache.

    Before the mount completes, another process, P2, tries to walk into the
    'foo' directory. The vfs path walking code finds an entry for 'foo' and
    calls the revalidate method. Revalidate finds that the entry is not
    PENDING (because PENDING was never set on the dentry created by the mkdir),
    but it does find the directory is empty. Revalidate calls try_to_fill_dentry,
    which sets the PENDING flag and then calls into the autofs4 wait code to
    trigger or wait for a mount of 'foo'. The wait code finds the entry for
    'foo' and goes to sleep waiting for the completion of the mount.

    Yet another process, P3, tries to walk into the 'foo' directory. This
    process again finds a dentry in the dcache for 'foo', and calls into
    the autofs revalidate code.

    The revalidate code finds that the PENDING flag is set, and so calls
    try_to_fill_dentry.

    a) try_to_fill_dentry sets the PENDING flag redundantly for this dentry,
    then calls into the autofs4 wait code.
    b) the autofs4 wait code takes the waitq mutex and searches for an entry
    for 'foo'

    Between a and b, P1 is woken up because the mount completed.
    P1 takes the wait queue mutex, clears the PENDING flag from the dentry,
    and removes the waitqueue entry for 'foo' from the list.

    When it releases the waitq mutex, P3 (eventually) acquires it. At this
    time, it looks for an existing waitq for 'foo', finds none, and so
    creates a new one and calls out to the daemon to mount the 'foo' directory.

    Now, the reason that three processes are required to trigger this race
    is that, because the PENDING flag is not set on the dentry created by
    mkdir, the window for the race would be way to slim for it to ever occur.
    Basically, between the testing of d_mountpoint(dentry) and the taking of
    the waitq mutex, the mount would have to complete and the daemon would
    have to be woken up, and that in turn would have to wake up P1. This is
    simply impossible. Add the third process, though, and it becomes slightly
    more likely.

    Signed-off-by: Jeff Moyer
    Signed-off-by: Ian Kent

    ---

    fs/autofs4/waitq.c | 135 +++++++++++++++++++++++++++++++++++++---------------
    1 files changed, 97 insertions(+), 38 deletions(-)


    diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
    index 5208cfb..cd21fd4 100644
    --- a/fs/autofs4/waitq.c
    +++ b/fs/autofs4/waitq.c
    @@ -207,19 +207,106 @@ autofs4_find_wait(struct autofs_sb_info *sbi, struct qstr *qstr)
    return wq;
    }

    +/*
    + * Check if we have a valid request.
    + * Returns
    + * 1 if the request should continue.
    + * In this case we can return an autofs_wait_queue entry if one is
    + * found or NULL to idicate a new wait needs to be created.
    + * 0 or a negative errno if the request shouldn't continue.
    + */
    +static int validate_request(struct autofs_wait_queue **wait,
    + struct autofs_sb_info *sbi,
    + struct qstr *qstr,
    + struct dentry*dentry, enum autofs_notify notify)
    +{
    + struct autofs_wait_queue *wq;
    + struct autofs_info *ino;
    +
    + /* Wait in progress, continue; */
    + wq = autofs4_find_wait(sbi, qstr);
    + if (wq) {
    + *wait = wq;
    + return 1;
    + }
    +
    + *wait = NULL;
    +
    + /* If we don't yet have any info this is a new request */
    + ino = autofs4_dentry_ino(dentry);
    + if (!ino)
    + return 1;
    +
    + /*
    + * If we've been asked to wait on an existing expire (NFY_NONE)
    + * but there is no wait in the queue ...
    + */
    + if (notify == NFY_NONE) {
    + /*
    + * Either we've betean the pending expire to post it's
    + * wait or it finished while we waited on the mutex.
    + * So we need to wait till either, the wait appears
    + * or the expire finishes.
    + */
    +
    + while (ino->flags & AUTOFS_INF_EXPIRING) {
    + mutex_unlock(&sbi->wq_mutex);
    + schedule_timeout_interruptible(HZ/10);
    + if (mutex_lock_interruptible(&sbi->wq_mutex))
    + return -EINTR;
    +
    + wq = autofs4_find_wait(sbi, qstr);
    + if (wq) {
    + *wait = wq;
    + return 1;
    + }
    + }
    +
    + /*
    + * Not ideal but the status has already gone. Of the two
    + * cases where we wait on NFY_NONE neither depend on the
    + * return status of the wait.
    + */
    + return 0;
    + }
    +
    + /*
    + * If we've been asked to trigger a mount and the request
    + * completed while we waited on the mutex ...
    + */
    + if (notify == NFY_MOUNT) {
    + /*
    + * If the dentry isn't hashed just go ahead and try the
    + * mount again with a new wait (not much else we can do).
    + */
    + if (!d_unhashed(dentry)) {
    + /*
    + * But if the dentry is hashed, that means that we
    + * got here through the revalidate path. Thus, we
    + * need to check if the dentry has been mounted
    + * while we waited on the wq_mutex. If it has,
    + * simply return success.
    + */
    + if (d_mountpoint(dentry))
    + return 0;
    + }
    + }
    +
    + return 1;
    +}
    +
    int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
    enum autofs_notify notify)
    {
    - struct autofs_info *ino;
    struct autofs_wait_queue *wq;
    struct qstr qstr;
    char *name;
    - int status, type;
    + int status, ret, type;

    /* In catatonic mode, we don't wait for nobody */
    if (sbi->catatonic)
    return -ENOENT;
    -
    +
    name = kmalloc(NAME_MAX + 1, GFP_KERNEL);
    if (!name)
    return -ENOMEM;
    @@ -237,43 +324,15 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
    qstr.name = name;
    qstr.hash = full_name_hash(name, qstr.len);

    - if (mutex_lock_interruptible(&sbi->wq_mutex)) {
    - kfree(qstr.name);
    + if (mutex_lock_interruptible(&sbi->wq_mutex))
    return -EINTR;
    - }
    -
    - wq = autofs4_find_wait(sbi, &qstr);
    - ino = autofs4_dentry_ino(dentry);
    - if (!wq && ino && notify == NFY_NONE) {
    - /*
    - * Either we've betean the pending expire to post it's
    - * wait or it finished while we waited on the mutex.
    - * So we need to wait till either, the wait appears
    - * or the expire finishes.
    - */

    - while (ino->flags & AUTOFS_INF_EXPIRING) {
    - mutex_unlock(&sbi->wq_mutex);
    - schedule_timeout_interruptible(HZ/10);
    - if (mutex_lock_interruptible(&sbi->wq_mutex)) {
    - kfree(qstr.name);
    - return -EINTR;
    - }
    - wq = autofs4_find_wait(sbi, &qstr);
    - if (wq)
    - break;
    - }
    -
    - /*
    - * Not ideal but the status has already gone. Of the two
    - * cases where we wait on NFY_NONE neither depend on the
    - * return status of the wait.
    - */
    - if (!wq) {
    - kfree(qstr.name);
    + ret = validate_request(&wq, sbi, &qstr, dentry, notify);
    + if (ret <= 0) {
    + if (ret == 0)
    mutex_unlock(&sbi->wq_mutex);
    - return 0;
    - }
    + kfree(qstr.name);
    + return ret;
    }

    if (!wq) {
    @@ -391,9 +450,9 @@ int autofs4_wait_release(struct autofs_sb_info *sbi, autofs_wqt_t wait_queue_tok
    }

    *wql = wq->next; /* Unlink from chain */
    - mutex_unlock(&sbi->wq_mutex);
    kfree(wq->name.name);
    wq->name.name = NULL; /* Do not wait on this queue */
    + mutex_unlock(&sbi->wq_mutex);

    wq->status = status;

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. [PATCH 5/7] autofs4 - use lookup intent flags to trigger mounts

    When an open(2) call is made on an autofs mount point directory that
    already exists and the O_DIRECTORY flag is not used the needed mount
    callback to the daemon is not done. This leads to the path walk
    continuing resulting in a callback to the daemon with an incorrect
    key. open(2) is called without O_DIRECTORY by the "find" utility but
    this should be handled properly anyway.

    This happens because autofs needs to use the lookup flags to decide
    when to callback to the daemon to perform a mount to prevent mount
    storms. For example, an autofs indirect mount map that has the "browse"
    option will have the mount point directories are pre-created and the
    stat(2) call made by a color ls against each directory will cause all
    these directories to be mounted. It is unfortunate we need to resort
    to this but mount maps can be quite large. Additionally, if a user
    manually umounts an autofs indirect mount the directory isn't removed
    which also leads to this situation.

    To resolve this autofs needs to use the lookup intent flags to enable
    it to make this decision. This patch adds this check and triggers a
    call back if any of the lookup intent flags are set as all these calls
    warrant a mount attempt be requested.

    I know that external VFS code which uses the lookup flags is something
    that the VFS would like to eliminate but I have no choice as I can't
    see any other way to do this. A VFS dentry or inode operation callback
    which returns the lookup "type" (requires a definition) would be
    sufficient. But this change is needed now and I'm not aware of the form
    that coming VFS changes will take so I'm not willing to propose anything
    along these lines.

    If anyone can provide an alternate method I would be happy to use it.

    Signed-off-by: Ian Kent

    ---

    fs/autofs4/root.c | 7 +++++--
    1 files changed, 5 insertions(+), 2 deletions(-)


    diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
    index 236eb9b..7f3ebf1 100644
    --- a/fs/autofs4/root.c
    +++ b/fs/autofs4/root.c
    @@ -31,6 +31,9 @@ static int autofs4_root_readdir(struct file * filp, void * dirent, filldir_t fil
    static struct dentry *autofs4_lookup(struct inode *,struct dentry *, struct nameidata *);
    static void *autofs4_follow_link(struct dentry *, struct nameidata *);

    +#define TRIGGER_FLAGS (LOOKUP_CONTINUE | LOOKUP_DIRECTORY)
    +#define TRIGGER_INTENTS (LOOKUP_OPEN | LOOKUP_ACCESS | LOOKUP_CREATE)
    +
    const struct file_operations autofs4_root_operations = {
    .open = dcache_dir_open,
    .release = dcache_dir_close,
    @@ -291,7 +294,7 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
    return status;
    }
    /* Trigger mount for path component or follow link */
    - } else if (flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY) ||
    + } else if (flags & (TRIGGER_FLAGS | TRIGGER_INTENTS) ||
    current->link_count) {
    DPRINTK("waiting for mount name=%.*s",
    dentry->d_name.len, dentry->d_name.name);
    @@ -336,7 +339,7 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
    nd->flags);

    /* If it's our master or we shouldn't trigger a mount we're done */
    - lookup_type = nd->flags & (LOOKUP_CONTINUE | LOOKUP_DIRECTORY);
    + lookup_type = nd->flags & (TRIGGER_FLAGS | TRIGGER_INTENTS);
    if (oz_mode || !lookup_type)
    goto done;

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH 7/7] autofs4 - fix pending mount race.

    On Tue, 17 Jun 2008 20:24:12 +0800 Ian Kent wrote:

    > Close a race between a pending mount that is about to finish and a new
    > lookup for the same directory.
    >
    > Process P1 triggers a mount of directory foo.
    > It sets DCACHE_AUTOFS_PENDING in the ->lookup routine, creates a waitq
    > entry for 'foo', and calls out to the daemon to perform the mount.
    > The autofs daemon will then create the directory 'foo', using a new dentry
    > that will be hashed in the dcache.
    >
    > Before the mount completes, another process, P2, tries to walk into the
    > 'foo' directory. The vfs path walking code finds an entry for 'foo' and
    > calls the revalidate method. Revalidate finds that the entry is not
    > PENDING (because PENDING was never set on the dentry created by the mkdir),
    > but it does find the directory is empty. Revalidate calls try_to_fill_dentry,
    > which sets the PENDING flag and then calls into the autofs4 wait code to
    > trigger or wait for a mount of 'foo'. The wait code finds the entry for
    > 'foo' and goes to sleep waiting for the completion of the mount.
    >
    > Yet another process, P3, tries to walk into the 'foo' directory. This
    > process again finds a dentry in the dcache for 'foo', and calls into
    > the autofs revalidate code.
    >
    > The revalidate code finds that the PENDING flag is set, and so calls
    > try_to_fill_dentry.
    >
    > a) try_to_fill_dentry sets the PENDING flag redundantly for this dentry,
    > then calls into the autofs4 wait code.
    > b) the autofs4 wait code takes the waitq mutex and searches for an entry
    > for 'foo'
    >
    > Between a and b, P1 is woken up because the mount completed.
    > P1 takes the wait queue mutex, clears the PENDING flag from the dentry,
    > and removes the waitqueue entry for 'foo' from the list.
    >
    > When it releases the waitq mutex, P3 (eventually) acquires it. At this
    > time, it looks for an existing waitq for 'foo', finds none, and so
    > creates a new one and calls out to the daemon to mount the 'foo' directory.
    >
    > Now, the reason that three processes are required to trigger this race
    > is that, because the PENDING flag is not set on the dentry created by
    > mkdir, the window for the race would be way to slim for it to ever occur.
    > Basically, between the testing of d_mountpoint(dentry) and the taking of
    > the waitq mutex, the mount would have to complete and the daemon would
    > have to be woken up, and that in turn would have to wake up P1. This is
    > simply impossible. Add the third process, though, and it becomes slightly
    > more likely.
    >
    > ...
    >
    > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
    > index 5208cfb..cd21fd4 100644
    > --- a/fs/autofs4/waitq.c
    > +++ b/fs/autofs4/waitq.c
    > @@ -207,19 +207,106 @@ autofs4_find_wait(struct autofs_sb_info *sbi, struct qstr *qstr)
    > return wq;
    > }
    >
    > +/*
    > + * Check if we have a valid request.
    > + * Returns
    > + * 1 if the request should continue.
    > + * In this case we can return an autofs_wait_queue entry if one is
    > + * found or NULL to idicate a new wait needs to be created.
    > + * 0 or a negative errno if the request shouldn't continue.
    > + */
    > +static int validate_request(struct autofs_wait_queue **wait,
    > + struct autofs_sb_info *sbi,
    > + struct qstr *qstr,
    > + struct dentry*dentry, enum autofs_notify notify)
    > +{
    > + struct autofs_wait_queue *wq;
    > + struct autofs_info *ino;
    > +
    > + /* Wait in progress, continue; */
    > + wq = autofs4_find_wait(sbi, qstr);
    > + if (wq) {
    > + *wait = wq;
    > + return 1;


    Returns 1 with the mutex held.

    > + }
    > +
    > + *wait = NULL;
    > +
    > + /* If we don't yet have any info this is a new request */
    > + ino = autofs4_dentry_ino(dentry);
    > + if (!ino)
    > + return 1;
    > +
    > + /*
    > + * If we've been asked to wait on an existing expire (NFY_NONE)
    > + * but there is no wait in the queue ...
    > + */
    > + if (notify == NFY_NONE) {
    > + /*
    > + * Either we've betean the pending expire to post it's
    > + * wait or it finished while we waited on the mutex.
    > + * So we need to wait till either, the wait appears
    > + * or the expire finishes.
    > + */


    Wanna have another go at that comment? The grammar and spelling should
    cause an oops or something.

    > + while (ino->flags & AUTOFS_INF_EXPIRING) {
    > + mutex_unlock(&sbi->wq_mutex);
    > + schedule_timeout_interruptible(HZ/10);
    > + if (mutex_lock_interruptible(&sbi->wq_mutex))
    > + return -EINTR;


    Returns -EFOO with the mutex not held.

    > +
    > + wq = autofs4_find_wait(sbi, qstr);
    > + if (wq) {
    > + *wait = wq;
    > + return 1;
    > + }
    > + }
    > +
    > + /*
    > + * Not ideal but the status has already gone. Of the two
    > + * cases where we wait on NFY_NONE neither depend on the
    > + * return status of the wait.
    > + */
    > + return 0;


    Returns zero with the mutex held.

    > + }
    > +
    > + /*
    > + * If we've been asked to trigger a mount and the request
    > + * completed while we waited on the mutex ...
    > + */
    > + if (notify == NFY_MOUNT) {
    > + /*
    > + * If the dentry isn't hashed just go ahead and try the
    > + * mount again with a new wait (not much else we can do).
    > + */
    > + if (!d_unhashed(dentry)) {
    > + /*
    > + * But if the dentry is hashed, that means that we
    > + * got here through the revalidate path. Thus, we
    > + * need to check if the dentry has been mounted
    > + * while we waited on the wq_mutex. If it has,
    > + * simply return success.
    > + */
    > + if (d_mountpoint(dentry))
    > + return 0;
    > + }
    > + }
    > +
    > + return 1;
    > +}
    >
    > ...
    >
    > + ret = validate_request(&wq, sbi, &qstr, dentry, notify);
    > + if (ret <= 0) {
    > + if (ret == 0)
    > mutex_unlock(&sbi->wq_mutex);
    > - return 0;
    > - }
    > + kfree(qstr.name);
    > + return ret;
    > }


    Leave the mutex held if it returned 1. Doesn't unlock the mutex if it
    returned -EFOO. Presumably callers of this function will unlock the
    mutex if it returned zero.

    Or something like that. My brain just exploded.

    Please double-check the locking protocol here and then document the
    sorry thing.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH 6/7] autofs4 - use struct qstr in waitq.c

    On Tue, 17 Jun 2008 20:24:06 +0800
    Ian Kent wrote:

    > From: Jeff Moyer
    >
    > The autofs_wait_queue already contains all of the fields of the
    > struct qstr, so change it into a qstr.
    >
    > This patch, from Jeff Moyer, has been modified a liitle by myself.
    >
    > Signed-off-by: Jeff Moyer
    > Signed-off-by: Ian Kent


    So this patch which had been happily sitting in -mm for a month has
    suddenly broken because linux-next's three-day-old
    4bce7ce7c7d0d57b78dacc3a2bd87ec63b2d9b4c has removed LOOKUP_ACCESS.

    This is suboptimal.

    Now what do I do?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [PATCH 6/7] autofs4 - use struct qstr in waitq.c


    On Tue, 2008-07-22 at 16:13 -0700, Andrew Morton wrote:
    > On Tue, 17 Jun 2008 20:24:06 +0800
    > Ian Kent wrote:
    >
    > > From: Jeff Moyer
    > >
    > > The autofs_wait_queue already contains all of the fields of the
    > > struct qstr, so change it into a qstr.
    > >
    > > This patch, from Jeff Moyer, has been modified a liitle by myself.
    > >
    > > Signed-off-by: Jeff Moyer
    > > Signed-off-by: Ian Kent

    >
    > So this patch which had been happily sitting in -mm for a month has
    > suddenly broken because linux-next's three-day-old
    > 4bce7ce7c7d0d57b78dacc3a2bd87ec63b2d9b4c has removed LOOKUP_ACCESS.
    >
    > This is suboptimal.
    >
    > Now what do I do?


    Ummm .. I'm confused.

    Your patch autofs4-use-lookup-intent-flags-to-trigger-mounts-fix.patch
    allows the linux-next kernel to build with all the autofs4 patches
    currently posted for inclusion in mm but the patch you mention here
    isn't concerned with the lookup flags?

    The removal of LOOKUP_ACCESS is quite interesting. AFAIKS it effectively
    prevents the patch
    autofs4-use-lookup-intent-flags-to-trigger-mounts.patch from also
    resolving an issue with recursive autofs mounts while still resolving
    the issue that the patch was actually meant to address.

    It's hard to get exited about the former issue as Al Viro has NACKed a
    previous patch that added the LOOKUP_ACCESS check, indicating the
    availability of the lookup flags will be changing. Also there is a
    question as to whether autofs will support the use mount points in
    automount maps that themselves refer to an automount path (the recursive
    bit).

    Ian

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [PATCH 6/7] autofs4 - use struct qstr in waitq.c

    On Wed, 23 Jul 2008 13:08:40 +0800 Ian Kent wrote:

    >
    > On Tue, 2008-07-22 at 16:13 -0700, Andrew Morton wrote:
    > > On Tue, 17 Jun 2008 20:24:06 +0800
    > > Ian Kent wrote:
    > >
    > > > From: Jeff Moyer
    > > >
    > > > The autofs_wait_queue already contains all of the fields of the
    > > > struct qstr, so change it into a qstr.
    > > >
    > > > This patch, from Jeff Moyer, has been modified a liitle by myself.
    > > >
    > > > Signed-off-by: Jeff Moyer
    > > > Signed-off-by: Ian Kent

    > >
    > > So this patch which had been happily sitting in -mm for a month has
    > > suddenly broken because linux-next's three-day-old
    > > 4bce7ce7c7d0d57b78dacc3a2bd87ec63b2d9b4c has removed LOOKUP_ACCESS.
    > >
    > > This is suboptimal.
    > >
    > > Now what do I do?

    >
    > Ummm .. I'm confused.
    >
    > Your patch autofs4-use-lookup-intent-flags-to-trigger-mounts-fix.patch
    > allows the linux-next kernel to build with all the autofs4 patches
    > currently posted for inclusion in mm but the patch you mention here
    > isn't concerned with the lookup flags?


    Yeah, I picked the wrong patch to reply to.

    > The removal of LOOKUP_ACCESS is quite interesting. AFAIKS it effectively
    > prevents the patch
    > autofs4-use-lookup-intent-flags-to-trigger-mounts.patch from also
    > resolving an issue with recursive autofs mounts while still resolving
    > the issue that the patch was actually meant to address.
    >
    > It's hard to get exited about the former issue as Al Viro has NACKed a
    > previous patch that added the LOOKUP_ACCESS check, indicating the
    > availability of the lookup flags will be changing. Also there is a
    > question as to whether autofs will support the use mount points in
    > automount maps that themselves refer to an automount path (the recursive
    > bit).


    So what do we do?

    Seems that a great pile of newish-looking stuff hit the vfs tree yesterday.
    Al, is that material supposed to be going into 2.6.27?

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: [PATCH 6/7] autofs4 - use struct qstr in waitq.c


    On Tue, 2008-07-22 at 22:17 -0700, Andrew Morton wrote:
    > On Wed, 23 Jul 2008 13:08:40 +0800 Ian Kent wrote:
    >
    > >
    > > On Tue, 2008-07-22 at 16:13 -0700, Andrew Morton wrote:
    > > > On Tue, 17 Jun 2008 20:24:06 +0800
    > > > Ian Kent wrote:
    > > >
    > > > > From: Jeff Moyer
    > > > >
    > > > > The autofs_wait_queue already contains all of the fields of the
    > > > > struct qstr, so change it into a qstr.
    > > > >
    > > > > This patch, from Jeff Moyer, has been modified a liitle by myself.
    > > > >
    > > > > Signed-off-by: Jeff Moyer
    > > > > Signed-off-by: Ian Kent
    > > >
    > > > So this patch which had been happily sitting in -mm for a month has
    > > > suddenly broken because linux-next's three-day-old
    > > > 4bce7ce7c7d0d57b78dacc3a2bd87ec63b2d9b4c has removed LOOKUP_ACCESS.
    > > >
    > > > This is suboptimal.
    > > >
    > > > Now what do I do?

    > >
    > > Ummm .. I'm confused.
    > >
    > > Your patch autofs4-use-lookup-intent-flags-to-trigger-mounts-fix.patch
    > > allows the linux-next kernel to build with all the autofs4 patches
    > > currently posted for inclusion in mm but the patch you mention here
    > > isn't concerned with the lookup flags?

    >
    > Yeah, I picked the wrong patch to reply to.


    Hahaha,

    >
    > > The removal of LOOKUP_ACCESS is quite interesting. AFAIKS it effectively
    > > prevents the patch
    > > autofs4-use-lookup-intent-flags-to-trigger-mounts.patch from also
    > > resolving an issue with recursive autofs mounts while still resolving
    > > the issue that the patch was actually meant to address.
    > >
    > > It's hard to get exited about the former issue as Al Viro has NACKed a
    > > previous patch that added the LOOKUP_ACCESS check, indicating the
    > > availability of the lookup flags will be changing. Also there is a
    > > question as to whether autofs will support the use mount points in
    > > automount maps that themselves refer to an automount path (the recursive
    > > bit).

    >
    > So what do we do?


    Yeah, best thing to do is to go with the patch that you did to resolve
    it. I'm going to need to deal with changes to the availability of the
    lookup flags to modules as they come up anyway.

    I don't want to hold things up due to the recursive mount issue as Al
    has pointed out it doesn't work correctly now anyway. I haven't had time
    to look into it as it isn't at the top of my priority list.

    Ian


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread