On Thu, 26 Mar 2009, Ian Kent wrote:
> On Wed, 2009-03-25 at 20:53 -0700, Sage Weil wrote:
> > So, it sounds like this fix would need to go in along with an autofs patch
> > that moves the upcall back into lookup exclusively, now that a revalidate
> > failure does the right thing (always calls lookup). Hopefully that would
> > mean a net simplification on the autofs side as well?
>
> Here is a quick and totally untested patch.
> I doubt very much that I've got this right yet but it is something like
> what I will need to do.
>
> I can easily build this as a module and install it.
> I wonder what would happen if I was to use this without your two
> patches. That would give me some quick feedback on potential problems.
> Thoughts?
Answer to self!
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.
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.
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
