> Latest here works OK.
>
> I haven't finished checking yet but it looks like the patch below works
> OK. I started with a 2.6.29 build with your two patches but it was a
> little broken so I fell back to a Fedora 2.6.27 based kernel without the
> two revalidate pacthes to debug it. So I still need to test the result
> against 2.6.29 again. I also don't have any real way to test for the three
> process race we discussed where the revalidate isn't followed by a
> ->lookup() but with both of your patches applied that shouldn't be a
> problem (as we discussed).
>
> I've not run checkpatch.pl against the patch either at this stage.
That's good news...
> There is a further issue and that is regarding the autofs module.
>
> I can't see updating autofs for this being practical (although I haven't
> actually looked yet). I suspect quite a bit of work would be needed. The
> fact is that autofs isn't used much any more and it really should be
> replaced with the autofs4 module at some point. But that's a fairly tricky
> exercise and will likely cause some user space breakage. It will require
> an updated module-init-tools to add "alais autofs4 autofs" for modprobe
> backward compatibility and will break for any explicit checks for the
> presence of the "autofs4" module.
Hmm. Well, I assume autofs needs to work properly before this gets
changed, though, right? Should I see what I can do with it? I took a
quick look, and I don't think it will take too much to make it behave.
It looks like the main thing is to make the lookup call to try_fill_dentry
return any existing dentry in place of the one the vfs provides.
sage
> Suddenly this fairly simple change gets bigger than Ben Hur, sorry!
> Thoughts?
>
> Ian
>
> autofs4 - always use lookup for lookup
>
> From: Ian Kent <raven@xxxxxxxxxx>
>
> We need to be able to hold the directory mutex during revalidate in
> some cases. To do this autofs needs to send ->d_revalidate() to lookup
> if the mutex is held since it cannot tell if it is the lock owner
> or if the lock is held by another path of execution.
> ---
>
> fs/autofs4/autofs_i.h | 15 ---
> fs/autofs4/root.c | 229 ++++++++++++++++++++++++++++---------------------
> 2 files changed, 131 insertions(+), 113 deletions(-)
>
>
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index b7ff33c..1fd9183 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -95,6 +95,7 @@ struct autofs_info {
>
> #define AUTOFS_INF_EXPIRING (1<<0) /* dentry is in the process of expiring */
> #define AUTOFS_INF_MOUNTPOINT (1<<1) /* mountpoint status for direct expire */
> +#define AUTOFS_INF_REVALIDATE (1<<2) /* revalidate sent this mount request */
>
> struct autofs_wait_queue {
> wait_queue_head_t queue;
> @@ -156,20 +157,6 @@ static inline int autofs4_oz_mode(struct autofs_sb_info *sbi) {
> return sbi->catatonic || task_pgrp_nr(current) == sbi->oz_pgrp;
> }
>
> -/* Does a dentry have some pending activity? */
> -static inline int autofs4_ispending(struct dentry *dentry)
> -{
> - struct autofs_info *inf = autofs4_dentry_ino(dentry);
> -
> - if (dentry->d_flags & DCACHE_AUTOFS_PENDING)
> - return 1;
> -
> - if (inf->flags & AUTOFS_INF_EXPIRING)
> - return 1;
> -
> - return 0;
> -}
> -
> static inline void autofs4_copy_atime(struct file *src, struct file *dst)
> {
> dst->f_path.dentry->d_inode->i_atime =
> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index e8c55d2..90c1c87 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -72,6 +72,14 @@ const struct inode_operations autofs4_dir_inode_operations = {
> .rmdir = autofs4_dir_rmdir,
> };
>
> +static unsigned int autofs4_need_lookup(unsigned int flags)
> +{
> + unsigned int res = 0;
> + if (flags & (TRIGGER_FLAGS | TRIGGER_INTENTS))
> + res = 1;
> + return res;
> +}
> +
> static int autofs4_dir_open(struct inode *inode, struct file *file)
> {
> struct dentry *dentry = file->f_path.dentry;
> @@ -103,7 +111,7 @@ out:
> return dcache_dir_open(inode, file);
> }
>
> -static int try_to_fill_dentry(struct dentry *dentry, int flags)
> +static int try_to_fill_dentry(struct dentry *dentry)
> {
> struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
> struct autofs_info *ino = autofs4_dentry_ino(dentry);
> @@ -116,55 +124,18 @@ static int try_to_fill_dentry(struct dentry *dentry, int flags)
> * Wait for a pending mount, triggering one if there
> * isn't one already
> */
> - if (dentry->d_inode == NULL) {
> - DPRINTK("waiting for mount name=%.*s",
> - dentry->d_name.len, dentry->d_name.name);
> -
> - status = autofs4_wait(sbi, dentry, NFY_MOUNT);
> -
> - DPRINTK("mount done status=%d", status);
> -
> - /* Turn this into a real negative dentry? */
> - if (status == -ENOENT) {
> - spin_lock(&dentry->d_lock);
> - dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> - spin_unlock(&dentry->d_lock);
> - return status;
> - } else if (status) {
> - /* Return a negative dentry, but leave it "pending" */
> - return status;
> - }
> - /* Trigger mount for path component or follow link */
> - } else if (dentry->d_flags & DCACHE_AUTOFS_PENDING ||
> - flags & (TRIGGER_FLAGS | TRIGGER_INTENTS) ||
> - current->link_count) {
> - DPRINTK("waiting for mount name=%.*s",
> - dentry->d_name.len, dentry->d_name.name);
> + DPRINTK("waiting for mount name=%.*s",
> + dentry->d_name.len, dentry->d_name.name);
>
> - spin_lock(&dentry->d_lock);
> - dentry->d_flags |= DCACHE_AUTOFS_PENDING;
> - spin_unlock(&dentry->d_lock);
> - status = autofs4_wait(sbi, dentry, NFY_MOUNT);
> + status = autofs4_wait(sbi, dentry, NFY_MOUNT);
>
> - DPRINTK("mount done status=%d", status);
> -
> - if (status) {
> - spin_lock(&dentry->d_lock);
> - dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> - spin_unlock(&dentry->d_lock);
> - return status;
> - }
> - }
> + DPRINTK("mount done status=%d", status);
>
> /* Initialize expiry counter after successful mount */
> - if (ino)
> + if (ino && !status)
> ino->last_used = jiffies;
>
> - spin_lock(&dentry->d_lock);
> - dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> - spin_unlock(&dentry->d_lock);
> -
> - return 0;
> + return status;
> }
>
> /* For autofs direct mounts the follow link triggers the mount */
> @@ -216,7 +187,16 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
> (!d_mountpoint(dentry) && __simple_empty(dentry))) {
> spin_unlock(&dcache_lock);
>
> - status = try_to_fill_dentry(dentry, 0);
> + spin_lock(&dentry->d_lock);
> + dentry->d_flags |= DCACHE_AUTOFS_PENDING;
> + spin_unlock(&dentry->d_lock);
> +
> + status = try_to_fill_dentry(dentry);
> +
> + spin_lock(&dentry->d_lock);
> + dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> + spin_unlock(&dentry->d_lock);
> +
> if (status)
> goto out_error;
>
> @@ -255,42 +235,55 @@ static int autofs4_revalidate(struct dentry *dentry, struct nameidata *nd)
> {
> struct inode *dir = dentry->d_parent->d_inode;
> struct autofs_sb_info *sbi = autofs4_sbi(dir->i_sb);
> - int oz_mode = autofs4_oz_mode(sbi);
> - int flags = nd ? nd->flags : 0;
> - int status = 1;
> -
> - /* Pending dentry */
> - spin_lock(&sbi->fs_lock);
> - if (autofs4_ispending(dentry)) {
> - /* The daemon never causes a mount to trigger */
> - spin_unlock(&sbi->fs_lock);
> + unsigned int flags = nd ? nd->flags : 0;
> + int status;
>
> - if (oz_mode)
> - return 1;
> + DPRINTK("name = %.*s oz_mode = %d flags = %d",
> + dentry->d_name.len, dentry->d_name.name,
> + autofs4_oz_mode(sbi), nd ? nd->flags : 0);
>
> - /*
> - * If the directory has gone away due to an expire
> - * we have been called as ->d_revalidate() and so
> - * we need to return false and proceed to ->lookup().
> - */
> - if (autofs4_expire_wait(dentry) == -EAGAIN)
> - return 0;
> + /* The daemon never causes a mount to trigger */
> + if (autofs4_oz_mode(sbi))
> + return 1;
>
> + spin_lock(&dentry->d_lock);
> + if (dentry->d_inode == NULL || dentry->d_flags & DCACHE_AUTOFS_PENDING) {
> + unsigned int lock_held = mutex_is_locked(&dir->i_mutex);
> + unsigned int pending = dentry->d_flags & DCACHE_AUTOFS_PENDING;
> + spin_unlock(&dentry->d_lock);
> /*
> - * A zero status is success otherwise we have a
> - * negative error code.
> + * If the directory mutex lock is held we must hide the
> + * dentry and proceed to ->lookup() to wait for mount
> + * completion since we cannot know whether the lock was
> + * taken by this code path or is held by another.
> + * Otherwise we can just wait for mount completion.
> */
> - status = try_to_fill_dentry(dentry, flags);
> - if (status == 0)
> - return 1;
> + status = 0;
> + if (lock_held) {
> + /* Already pending, send to ->lookup() */
> + d_drop(dentry);
> + } else {
> + /*
> + * The mutex isn't held so if there's a pending
> + * mount we wait. If not then we are on the tail
> + * end of a mount fail, invalidate the dentry and
> + * return true so we don't go to lookup again.
> + */
> + if (pending) {
> + if (autofs4_expire_wait(dentry) != -EAGAIN) {
> + status = try_to_fill_dentry(dentry);
> + if (!status)
> + status = 1;
> + }
> + } else {
> + d_invalidate(dentry);
> + status = 1;
> + }
> + }
>
> return status;
> }
> - spin_unlock(&sbi->fs_lock);
> -
> - /* Negative dentry.. invalidate if "old" */
> - if (dentry->d_inode == NULL)
> - return 0;
> + spin_unlock(&dentry->d_lock);
>
> /* Check for a non-mountpoint directory with no contents */
> spin_lock(&dcache_lock);
> @@ -299,19 +292,24 @@ static int autofs4_revalidate(struct dentry *dentry, struct nameidata *nd)
> __simple_empty(dentry)) {
> DPRINTK("dentry=%p %.*s, emptydir",
> dentry, dentry->d_name.len, dentry->d_name.name);
> - spin_unlock(&dcache_lock);
> -
> - /* The daemon never causes a mount to trigger */
> - if (oz_mode)
> - return 1;
>
> - /*
> - * A zero status is success otherwise we have a
> - * negative error code.
> - */
> - status = try_to_fill_dentry(dentry, flags);
> - if (status == 0)
> - return 1;
> + /* Always send new mount requests to lookup */
> + status = 1;
> + spin_lock(&sbi->fs_lock);
> + if (autofs4_need_lookup(flags) || current->link_count) {
> + struct autofs_info *ino = autofs4_dentry_ino(dentry);
> + ino->flags |= AUTOFS_INF_REVALIDATE;
> + spin_lock(&sbi->lookup_lock);
> + if (list_empty(&ino->active))
> + list_add(&ino->active, &sbi->active_list);
> + spin_unlock(&sbi->lookup_lock);
> + spin_lock(&dentry->d_lock);
> + __d_drop(dentry);
> + spin_unlock(&dentry->d_lock);
> + status = 0;
> + }
> + spin_unlock(&sbi->fs_lock);
> + spin_unlock(&dcache_lock);
>
> return status;
> }
> @@ -471,6 +469,7 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
> struct autofs_info *ino;
> struct dentry *expiring, *unhashed;
> int oz_mode;
> + int status = 0;
>
> DPRINTK("name = %.*s",
> dentry->d_name.len, dentry->d_name.name);
> @@ -486,9 +485,18 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
> current->pid, task_pgrp_nr(current), sbi->catatonic, oz_mode);
>
> unhashed = autofs4_lookup_active(sbi, dentry->d_parent, &dentry->d_name);
> - if (unhashed)
> + if (unhashed) {
> dentry = unhashed;
> - else {
> + /*
> + * For requests from revalidate we need to rehash the dentry
> + * here as we don't end up calling mkdir().
> + */
> + spin_lock(&sbi->fs_lock);
> + ino = autofs4_dentry_ino(dentry);
> + if (ino->flags & AUTOFS_INF_REVALIDATE)
> + d_rehash(dentry);
> + spin_unlock(&sbi->fs_lock);
> + } else {
> /*
> * Mark the dentry incomplete but don't hash it. We do this
> * to serialize our inode creation operations (symlink and
> @@ -522,6 +530,10 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
> }
>
> if (!oz_mode) {
> + spin_lock(&dentry->d_lock);
> + dentry->d_flags |= DCACHE_AUTOFS_PENDING;
> + spin_unlock(&dentry->d_lock);
> +
> mutex_unlock(&dir->i_mutex);
> expiring = autofs4_lookup_expiring(sbi,
> dentry->d_parent,
> @@ -541,19 +553,33 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
> dput(expiring);
> }
>
> + status = try_to_fill_dentry(dentry);
> +
> + mutex_lock(&dir->i_mutex);
> +
> spin_lock(&dentry->d_lock);
> - dentry->d_flags |= DCACHE_AUTOFS_PENDING;
> + dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> spin_unlock(&dentry->d_lock);
> - if (dentry->d_op && dentry->d_op->d_revalidate)
> - (dentry->d_op->d_revalidate)(dentry, nd);
> - mutex_lock(&dir->i_mutex);
> +
> + /*
> + * If we got a new lookup request from revaidate, remove
> + * it from the active queue and rehash the dentry.
> + */
> + spin_lock(&sbi->fs_lock);
> + if (ino->flags & AUTOFS_INF_REVALIDATE) {
> + spin_lock(&sbi->lookup_lock);
> + if (!list_empty(&ino->active))
> + list_del_init(&ino->active);
> + spin_unlock(&sbi->lookup_lock);
> + }
> + spin_unlock(&sbi->fs_lock);
> }
>
> /*
> - * If we are still pending, check if we had to handle
> + * If we had a mount fail, check if we had to handle
> * a signal. If so we can force a restart..
> */
> - if (dentry->d_flags & DCACHE_AUTOFS_PENDING) {
> + if (status) {
> /* See if we were interrupted */
> if (signal_pending(current)) {
> sigset_t *sigset = ¤t->pending.signal;
> @@ -565,11 +591,6 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
> return ERR_PTR(-ERESTARTNOINTR);
> }
> }
> - if (!oz_mode) {
> - spin_lock(&dentry->d_lock);
> - dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> - spin_unlock(&dentry->d_lock);
> - }
> }
>
> /*
> @@ -599,7 +620,17 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
> return dentry;
> }
>
> - if (unhashed)
> + /*
> + * If we had a mount failure, return status to user space.
> + * If the mount succeeded and we used a dentry from the
> + * active queue return it.
> + */
> + if (status) {
> + dentry = ERR_PTR(status);
> + if (unhashed)
> + dput(unhashed);
> + return dentry;
> + } else if (unhashed)
> return unhashed;
>
> return NULL;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
