Bad race condition in the new autofs protocol somewhere

Bad race condition in the new autofs protocol somewhere

[Olivier Galibert]

The setup:
  /people is a NIS automount.  /people/gadda points to m179:/disk05/disk11/gadda
  /hosts is a two-level automount, /hosts/xx/yy points to xx:/yy using:

in auto.master:
  /hosts file:/etc/auto.hosts

in /etc/auto.hosts:
  * -fstype=autofs,-Dhost=& file=/etc/auto.hosts.sub

in /etc/auto.hosts.sub:
  * ${host}:/&

/people/gadda/normalisation is a symlink to /hosts/m179/disk03/gadda/lemonde

I have a small test program:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/time.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <time.h>
#include <unistd.h>

int main(void)
{
  for(;;) {
    struct timeval tv1, tv2;
    struct timespec ts;
    gettimeofday(&tv1, 0);
    int fd = open("/people/gadda/normalisation/tempo4.general", O_RDONLY);
    gettimeofday(&tv2, 0);
    if(fd < 0)
      printf("%d.%06d - %d.%06d failure\n", (int)tv1.tv_sec, (int)tv1.tv_usec, (int)tv2.tv_sec, (int)tv2.tv_usec);
    else
      close(fd);

    ts.tv_sec = 0;
    ts.tv_nsec = lrand48() % 100000;
    nanosleep(&ts, 0);    
  }
}

I.e. try opening an existing file "through" the symlink and the
automounts, give the time before and after the open syscall if it
fails.  Wait for a random time no more than 0.1s and try again.

Actually the random time is no more than 0.1ms, oops.  Whatever.

Then I start the automounter with a 1 second timeout, and I get random
failures.

1170867758.212109 - 1170867758.222789 failure
1170867758.668086 - 1170867758.678607 failure
1170867760.007998 - 1170867760.057324 failure

The log from an extra-verbose, but otherwise unchanged, git-of-today
version of autofs is:

Feb  7 18:02:24 m78 automount[16532]: Starting automounter version 5.0.1-rc3, master map auto.master
Feb  7 18:02:24 m78 automount[16532]: using kernel protocol version 5.00
Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /hosts with timeout 1, freq 1 seconds
Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /people with timeout 1, freq 1 seconds
Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /corpora with timeout 1, freq 1 seconds
Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /w3 with timeout 1, freq 1 seconds
Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /vol with timeout 1, freq 1 seconds
Feb  7 18:02:33 m78 automount[16532]: 1170867753.258726 got packet type 3
Feb  7 18:02:33 m78 automount[16532]: attempting to mount entry /people/gadda
Feb  7 18:02:33 m78 automount[16532]: mount(nfs): mounted m179:/disk05/disk11/gadda on /people/gadda
Feb  7 18:02:33 m78 automount[16532]: mounted /people/gadda
Feb  7 18:02:33 m78 automount[16532]: 1170867753.355598 got packet type 3
Feb  7 18:02:33 m78 automount[16532]: attempting to mount entry /hosts/m179
Feb  7 18:02:33 m78 automount[16532]: mounted indirect mount on /hosts/m179 with timeout 1, freq 1 seconds
Feb  7 18:02:33 m78 automount[16532]: mounted /hosts/m179
Feb  7 18:02:33 m78 automount[16532]: 1170867753.456997 got packet type 3
Feb  7 18:02:33 m78 automount[16532]: attempting to mount entry /hosts/m179/disk03
Feb  7 18:02:33 m78 automount[16532]: mount(nfs): mounted m179:/disk03 on /hosts/m179/disk03
Feb  7 18:02:33 m78 automount[16532]: mounted /hosts/m179/disk03
Feb  7 18:02:35 m78 automount[16532]: mount still busy /people
Feb  7 18:02:35 m78 automount[16532]: mount still busy /hosts/m179
Feb  7 18:02:36 m78 automount[16532]: mount still busy /hosts
Feb  7 18:02:37 m78 automount[16532]: mount still busy /people
Feb  7 18:02:38 m78 automount[16532]: 1170867758.212137 got packet type 4
Feb  7 18:02:38 m78 automount[16532]: 1170867758.212282 expiring path /people/gadda
Feb  7 18:02:38 m78 automount[16532]: 1170867758.212983 unmounting dir = /people/gadda
Feb  7 18:02:38 m78 automount[16532]: 1170867758.222742 expired /people/gadda
Feb  7 18:02:38 m78 automount[16532]: 1170867758.224147 got packet type 3
Feb  7 18:02:38 m78 automount[16532]: attempting to mount entry /people/gadda
Feb  7 18:02:38 m78 automount[16532]: mount(nfs): mounted m179:/disk05/disk11/gadda on /people/gadda
Feb  7 18:02:38 m78 automount[16532]: mounted /people/gadda
Feb  7 18:02:38 m78 automount[16532]: 1170867758.668221 got packet type 4
Feb  7 18:02:38 m78 automount[16532]: 1170867758.668379 expiring path /hosts/m179/disk03
Feb  7 18:02:38 m78 automount[16532]: 1170867758.669053 unmounting dir = /hosts/m179/disk03
Feb  7 18:02:38 m78 automount[16532]: 1170867758.678567 expired /hosts/m179/disk03
Feb  7 18:02:38 m78 automount[16532]: 1170867758.680159 got packet type 3
Feb  7 18:02:38 m78 automount[16532]: attempting to mount entry /hosts/m179/disk03
Feb  7 18:02:38 m78 automount[16532]: mount(nfs): mounted m179:/disk03 on /hosts/m179/disk03
Feb  7 18:02:38 m78 automount[16532]: mounted /hosts/m179/disk03
Feb  7 18:02:38 m78 automount[16532]: mount still busy /hosts/m179
Feb  7 18:02:39 m78 automount[16532]: mount still busy /people
Feb  7 18:02:39 m78 automount[16532]: mount still busy /hosts
Feb  7 18:02:40 m78 automount[16532]: 1170867760.004392 got packet type 4
Feb  7 18:02:40 m78 automount[16532]: 1170867760.004561 expiring path /people/gadda
Feb  7 18:02:40 m78 automount[16532]: 1170867760.005341 unmounting dir = /people/gadda
Feb  7 18:02:40 m78 automount[16532]: 1170867760.057275 expired /people/gadda
Feb  7 18:02:40 m78 automount[16532]: 1170867760.060045 got packet type 3
Feb  7 18:02:40 m78 automount[16532]: attempting to mount entry /people/gadda
Feb  7 18:02:40 m78 automount[16532]: mount(nfs): mounted m179:/disk05/disk11/gadda on /people/gadda
Feb  7 18:02:40 m78 automount[16532]: mounted /people/gadda
Feb  7 18:02:41 m78 automount[16532]: 1170867761.200013 got packet type 4
Feb  7 18:02:41 m78 automount[16532]: 1170867761.200177 expiring path /people/gadda
Feb  7 18:02:41 m78 automount[16532]: 1170867761.200877 unmounting dir = /people/gadda
Feb  7 18:02:41 m78 automount[16532]: 1170867761.210517 expired /people/gadda
Feb  7 18:02:41 m78 automount[16532]: 1170867761.687942 got packet type 4
Feb  7 18:02:41 m78 automount[16532]: 1170867761.688101 expiring path /hosts/m179/disk03
Feb  7 18:02:41 m78 automount[16532]: 1170867761.688816 unmounting dir = /hosts/m179/disk03
Feb  7 18:02:41 m78 automount[16532]: 1170867761.698293 expired /hosts/m179/disk03
Feb  7 18:02:42 m78 automount[16532]: mount still busy /hosts
Feb  7 18:02:43 m78 automount[16532]: mount still busy /hosts
Feb  7 18:02:44 m78 automount[16532]: umounted indirect mount /hosts/m179
Feb  7 18:02:44 m78 automount[16532]: shut down path /hosts/m179

So we can see that the requests fail from losing either of the
mountpoints and having a race between the umount, the remount and the
open.

Kernel is fedora core 5 2.6.18-2200.fc5.  I can reproduce the problem
at will and in a very short time, and m78 is currently dedicated to
fixing it, so what should I do at that point?

Note that autofs4 fails too on that race, but my guess is that autofs5
has a higher probability of being fixed.

  OG.

Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Ian Kent]

On Wed, 2007-02-07 at 18:34 +0100, Olivier Galibert wrote:
> The setup:
>   /people is a NIS automount.  /people/gadda points to m179:/disk05/disk11/gadda
>   /hosts is a two-level automount, /hosts/xx/yy points to xx:/yy using:
> 
> in auto.master:
>   /hosts file:/etc/auto.hosts
> 
> in /etc/auto.hosts:
>   * -fstype=autofs,-Dhost=& file=/etc/auto.hosts.sub
> 
> in /etc/auto.hosts.sub:
>   * ${host}:/&
> 
> /people/gadda/normalisation is a symlink to /hosts/m179/disk03/gadda/lemonde
> 
> I have a small test program:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/time.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <time.h>
> #include <unistd.h>
> 
> int main(void)
> {
>   for(;;) {
>     struct timeval tv1, tv2;
>     struct timespec ts;
>     gettimeofday(&tv1, 0);
>     int fd = open("/people/gadda/normalisation/tempo4.general", O_RDONLY);
>     gettimeofday(&tv2, 0);
>     if(fd < 0)
>       printf("%d.%06d - %d.%06d failure\n", (int)tv1.tv_sec, (int)tv1.tv_usec, (int)tv2.tv_sec, (int)tv2.tv_usec);
>     else
>       close(fd);
> 
>     ts.tv_sec = 0;
>     ts.tv_nsec = lrand48() % 100000;
>     nanosleep(&ts, 0);    
>   }
> }
> 
> I.e. try opening an existing file "through" the symlink and the
> automounts, give the time before and after the open syscall if it
> fails.  Wait for a random time no more than 0.1s and try again.
> 
> Actually the random time is no more than 0.1ms, oops.  Whatever.
> 
> Then I start the automounter with a 1 second timeout, and I get random
> failures.
> 
> 1170867758.212109 - 1170867758.222789 failure
> 1170867758.668086 - 1170867758.678607 failure
> 1170867760.007998 - 1170867760.057324 failure
> 
> The log from an extra-verbose, but otherwise unchanged, git-of-today
> version of autofs is:
> 
> Feb  7 18:02:24 m78 automount[16532]: Starting automounter version 5.0.1-rc3, master map auto.master
> Feb  7 18:02:24 m78 automount[16532]: using kernel protocol version 5.00
> Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /hosts with timeout 1, freq 1 seconds
> Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /people with timeout 1, freq 1 seconds
> Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /corpora with timeout 1, freq 1 seconds
> Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /w3 with timeout 1, freq 1 seconds
> Feb  7 18:02:24 m78 automount[16532]: mounted indirect mount on /vol with timeout 1, freq 1 seconds
> Feb  7 18:02:33 m78 automount[16532]: 1170867753.258726 got packet type 3
> Feb  7 18:02:33 m78 automount[16532]: attempting to mount entry /people/gadda
> Feb  7 18:02:33 m78 automount[16532]: mount(nfs): mounted m179:/disk05/disk11/gadda on /people/gadda
> Feb  7 18:02:33 m78 automount[16532]: mounted /people/gadda
> Feb  7 18:02:33 m78 automount[16532]: 1170867753.355598 got packet type 3
> Feb  7 18:02:33 m78 automount[16532]: attempting to mount entry /hosts/m179
> Feb  7 18:02:33 m78 automount[16532]: mounted indirect mount on /hosts/m179 with timeout 1, freq 1 seconds
> Feb  7 18:02:33 m78 automount[16532]: mounted /hosts/m179
> Feb  7 18:02:33 m78 automount[16532]: 1170867753.456997 got packet type 3
> Feb  7 18:02:33 m78 automount[16532]: attempting to mount entry /hosts/m179/disk03
> Feb  7 18:02:33 m78 automount[16532]: mount(nfs): mounted m179:/disk03 on /hosts/m179/disk03
> Feb  7 18:02:33 m78 automount[16532]: mounted /hosts/m179/disk03
> Feb  7 18:02:35 m78 automount[16532]: mount still busy /people
> Feb  7 18:02:35 m78 automount[16532]: mount still busy /hosts/m179
> Feb  7 18:02:36 m78 automount[16532]: mount still busy /hosts
> Feb  7 18:02:37 m78 automount[16532]: mount still busy /people
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.212137 got packet type 4
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.212282 expiring path /people/gadda
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.212983 unmounting dir = /people/gadda
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.222742 expired /people/gadda
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.224147 got packet type 3
> Feb  7 18:02:38 m78 automount[16532]: attempting to mount entry /people/gadda
> Feb  7 18:02:38 m78 automount[16532]: mount(nfs): mounted m179:/disk05/disk11/gadda on /people/gadda
> Feb  7 18:02:38 m78 automount[16532]: mounted /people/gadda
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.668221 got packet type 4
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.668379 expiring path /hosts/m179/disk03
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.669053 unmounting dir = /hosts/m179/disk03
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.678567 expired /hosts/m179/disk03
> Feb  7 18:02:38 m78 automount[16532]: 1170867758.680159 got packet type 3
> Feb  7 18:02:38 m78 automount[16532]: attempting to mount entry /hosts/m179/disk03
> Feb  7 18:02:38 m78 automount[16532]: mount(nfs): mounted m179:/disk03 on /hosts/m179/disk03
> Feb  7 18:02:38 m78 automount[16532]: mounted /hosts/m179/disk03
> Feb  7 18:02:38 m78 automount[16532]: mount still busy /hosts/m179
> Feb  7 18:02:39 m78 automount[16532]: mount still busy /people
> Feb  7 18:02:39 m78 automount[16532]: mount still busy /hosts
> Feb  7 18:02:40 m78 automount[16532]: 1170867760.004392 got packet type 4
> Feb  7 18:02:40 m78 automount[16532]: 1170867760.004561 expiring path /people/gadda
> Feb  7 18:02:40 m78 automount[16532]: 1170867760.005341 unmounting dir = /people/gadda
> Feb  7 18:02:40 m78 automount[16532]: 1170867760.057275 expired /people/gadda
> Feb  7 18:02:40 m78 automount[16532]: 1170867760.060045 got packet type 3
> Feb  7 18:02:40 m78 automount[16532]: attempting to mount entry /people/gadda
> Feb  7 18:02:40 m78 automount[16532]: mount(nfs): mounted m179:/disk05/disk11/gadda on /people/gadda
> Feb  7 18:02:40 m78 automount[16532]: mounted /people/gadda
> Feb  7 18:02:41 m78 automount[16532]: 1170867761.200013 got packet type 4
> Feb  7 18:02:41 m78 automount[16532]: 1170867761.200177 expiring path /people/gadda
> Feb  7 18:02:41 m78 automount[16532]: 1170867761.200877 unmounting dir = /people/gadda
> Feb  7 18:02:41 m78 automount[16532]: 1170867761.210517 expired /people/gadda
> Feb  7 18:02:41 m78 automount[16532]: 1170867761.687942 got packet type 4
> Feb  7 18:02:41 m78 automount[16532]: 1170867761.688101 expiring path /hosts/m179/disk03
> Feb  7 18:02:41 m78 automount[16532]: 1170867761.688816 unmounting dir = /hosts/m179/disk03
> Feb  7 18:02:41 m78 automount[16532]: 1170867761.698293 expired /hosts/m179/disk03
> Feb  7 18:02:42 m78 automount[16532]: mount still busy /hosts
> Feb  7 18:02:43 m78 automount[16532]: mount still busy /hosts
> Feb  7 18:02:44 m78 automount[16532]: umounted indirect mount /hosts/m179
> Feb  7 18:02:44 m78 automount[16532]: shut down path /hosts/m179
> 
> So we can see that the requests fail from losing either of the
> mountpoints and having a race between the umount, the remount and the
> open.
> 
> Kernel is fedora core 5 2.6.18-2200.fc5.  I can reproduce the problem
> at will and in a very short time, and m78 is currently dedicated to
> fixing it, so what should I do at that point?
> 
> Note that autofs4 fails too on that race, but my guess is that autofs5
> has a higher probability of being fixed.

This looks like it may be a kernel bug that Jeff Moyer and I have been
working on. I'm testing now.

It may be better to update to a later kernel so I don't have to port the
patch to several different kernels. Is that possible?

Ian

Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Olivier Galibert]

On Thu, Feb 08, 2007 at 03:07:41AM +0900, Ian Kent wrote:
> It may be better to update to a later kernel so I don't have to port the
> patch to several different kernels. Is that possible?

Sure, 2.6.20 or -git?

  OG.

Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Ian Kent]

On Wed, 2007-02-07 at 19:18 +0100, Olivier Galibert wrote:
> On Thu, Feb 08, 2007 at 03:07:41AM +0900, Ian Kent wrote:
> > It may be better to update to a later kernel so I don't have to port the
> > patch to several different kernels. Is that possible?
> 
> Sure, 2.6.20 or -git?

2.6.20 has all the patches I've proposed so far except for the one we're
working on so that would be best for me.

Seems there may still be a problem with the patch so I'll let you know
what's happening as soon as I can.

Ian

[PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Ian Kent]

On Thu, 2007-02-08 at 11:33 +0900, Ian Kent wrote:
> On Wed, 2007-02-07 at 19:18 +0100, Olivier Galibert wrote:
> > On Thu, Feb 08, 2007 at 03:07:41AM +0900, Ian Kent wrote:
> > > It may be better to update to a later kernel so I don't have to port the
> > > patch to several different kernels. Is that possible?
> > 
> > Sure, 2.6.20 or -git?
> 
> 2.6.20 has all the patches I've proposed so far except for the one we're
> working on so that would be best for me.
> 
> Seems there may still be a problem with the patch so I'll let you know
> what's happening as soon as I can.

I think I'm just about done.

Could you try using the two patches here against 2.6.20 please:

Ian

---

--- linux-2.6.20/fs/autofs4/autofs_i.h.lookup-expire-race	2007-02-05 03:44:54.000000000 +0900
+++ linux-2.6.20/fs/autofs4/autofs_i.h	2007-02-12 12:15:17.000000000 +0900
@@ -52,6 +52,8 @@ struct autofs_info {
 
 	int		flags;
 
+	struct list_head rehash;
+
 	struct autofs_sb_info *sbi;
 	unsigned long last_used;
 	atomic_t count;
@@ -110,6 +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;
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
--- linux-2.6.20/fs/autofs4/root.c.lookup-expire-race	2007-02-05 03:44:54.000000000 +0900
+++ linux-2.6.20/fs/autofs4/root.c	2007-02-12 12:14:51.000000000 +0900
@@ -263,7 +263,7 @@ static int try_to_fill_dentry(struct den
 		 */
 		status = d_invalidate(dentry);
 		if (status != -EBUSY)
-			return -ENOENT;
+			return -EAGAIN;
 	}
 
 	DPRINTK("dentry=%p %.*s ino=%p",
@@ -413,7 +413,16 @@ static int autofs4_revalidate(struct den
 		 */
 		status = try_to_fill_dentry(dentry, flags);
 		if (status == 0)
-				return 1;
+			return 1;
+
+		/*
+		 * A status of EAGAIN here means that the dentry has gone
+		 * away while waiting for an expire to complete. If we are
+		 * racing with expire lookup will wait for it so this must
+		 * be a revalidate and we need to send it to lookup.
+		 */
+		if (status == -EAGAIN)
+			return 0;
 
 		return status;
 	}
@@ -459,9 +468,18 @@ void autofs4_dentry_release(struct dentr
 	de->d_fsdata = NULL;
 
 	if (inf) {
+		struct autofs_sb_info *sbi = autofs4_sbi(de->d_sb);
+
 		inf->dentry = NULL;
 		inf->inode = NULL;
 
+		if (sbi) {
+			spin_lock(&sbi->rehash_lock);
+			if (!list_empty(&inf->rehash))
+				list_del(&inf->rehash);
+			spin_unlock(&sbi->rehash_lock);
+		}
+
 		autofs4_free_ino(inf);
 	}
 }
@@ -478,10 +496,80 @@ static struct dentry_operations autofs4_
 	.d_release	= autofs4_dentry_release,
 };
 
+static struct dentry *autofs4_lookup_unhashed(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->rehash_lock);
+	head = &sbi->rehash_list;
+	list_for_each(p, head) {
+		struct autofs_info *ino;
+		struct dentry *dentry;
+		struct qstr *qstr;
+
+		ino = list_entry(p, struct autofs_info, rehash);
+		dentry = ino->dentry;
+
+		spin_lock(&dentry->d_lock);
+
+		/* Bad luck, we've already been dentry_iput */
+		if (!dentry->d_inode)
+			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)) {
+			struct autofs_info *ino = autofs4_dentry_ino(dentry);
+			struct inode *inode = dentry->d_inode;
+
+			list_del_init(&ino->rehash);
+			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(&dcache_lock);
+			return dentry;
+		}
+next:
+		spin_unlock(&dentry->d_lock);
+	}
+	spin_unlock(&sbi->rehash_lock);
+	spin_unlock(&dcache_lock);
+
+	return NULL;
+}
+
 /* Lookups in the root directory */
 static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
 {
 	struct autofs_sb_info *sbi;
+	struct dentry *unhashed;
 	int oz_mode;
 
 	DPRINTK("name = %.*s",
@@ -497,25 +585,46 @@ static struct dentry *autofs4_lookup(str
 	DPRINTK("pid = %u, pgrp = %u, catatonic = %d, oz_mode = %d",
 		 current->pid, process_group(current), sbi->catatonic, oz_mode);
 
-	/*
-	 * Mark the dentry incomplete, but add it. This is needed so
-	 * that the VFS layer knows about the dentry, and we can count
-	 * on catching any lookups through the revalidate.
-	 *
-	 * Let all the hard work be done by the revalidate function that
-	 * needs to be able to do this anyway..
-	 *
-	 * We need to do this before we release the directory semaphore.
-	 */
-	dentry->d_op = &autofs4_root_dentry_operations;
+	unhashed = autofs4_lookup_unhashed(sbi, dentry->d_parent, &dentry->d_name);
+	if (!unhashed) {
+		/*
+		 * Mark the dentry incomplete, but add it. This is needed so
+		 * that the VFS layer knows about the dentry, and we can count
+		 * on catching any lookups through the revalidate.
+		 *
+		 * Let all the hard work be done by the revalidate function that
+		 * needs to be able to do this anyway..
+		 *
+		 * We need to do this before we release the directory semaphore.
+		 */
+		dentry->d_op = &autofs4_root_dentry_operations;
+
+		dentry->d_fsdata = NULL;
+		d_add(dentry, NULL);
+	} else {
+		struct autofs_info *ino = autofs4_dentry_ino(unhashed);
+		DPRINTK("rehash %p with %p", dentry, unhashed);
+		/*
+		 * 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.
+		 */
+		if (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);
+			DPRINTK("request completed");
+		}
+		d_rehash(unhashed);
+		dentry = unhashed;
+	}
 
 	if (!oz_mode) {
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags |= DCACHE_AUTOFS_PENDING;
 		spin_unlock(&dentry->d_lock);
 	}
-	dentry->d_fsdata = NULL;
-	d_add(dentry, NULL);
 
 	if (dentry->d_op && dentry->d_op->d_revalidate) {
 		mutex_unlock(&dir->i_mutex);
@@ -534,6 +643,8 @@ static struct dentry *autofs4_lookup(str
 			if (sigismember (sigset, SIGKILL) ||
 			    sigismember (sigset, SIGQUIT) ||
 			    sigismember (sigset, SIGINT)) {
+			    if (unhashed)
+				dput(unhashed);
 			    return ERR_PTR(-ERESTARTNOINTR);
 			}
 		}
@@ -548,8 +659,14 @@ static struct dentry *autofs4_lookup(str
 	 * doesn't do the right thing for all system calls, but it should
 	 * be OK for the operations we permit from an autofs.
 	 */
-	if (dentry->d_inode && d_unhashed(dentry))
+	if (dentry->d_inode && d_unhashed(dentry)) {
+		if (unhashed)
+			dput(unhashed);
 		return ERR_PTR(-ENOENT);
+	}
+
+	if (unhashed)
+		return dentry;
 
 	return NULL;
 }
@@ -611,9 +728,10 @@ static int autofs4_dir_symlink(struct in
  * Normal filesystems would do a "d_delete()" to tell the VFS dcache
  * 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 since the unlink is probably the result of an expire.
- * We simply d_drop it, which allows the dentry lookup to remount it
- * if necessary.
+ * 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.
  *
  * 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.
@@ -642,7 +760,14 @@ static int autofs4_dir_unlink(struct ino
 
 	dir->i_mtime = CURRENT_TIME;
 
-	d_drop(dentry);
+	spin_lock(&dcache_lock);
+	spin_lock(&sbi->rehash_lock);
+	list_add(&ino->rehash, &sbi->rehash_list);
+	spin_unlock(&sbi->rehash_lock);
+	spin_lock(&dentry->d_lock);
+	__d_drop(dentry);
+	spin_unlock(&dentry->d_lock);
+	spin_unlock(&dcache_lock);
 
 	return 0;
 }
@@ -653,6 +778,9 @@ static int autofs4_dir_rmdir(struct inod
 	struct autofs_info *ino = autofs4_dentry_ino(dentry);
 	struct autofs_info *p_ino;
 	
+	DPRINTK("dentry %p, removing %.*s",
+		dentry, dentry->d_name.len, dentry->d_name.name);
+
 	if (!autofs4_oz_mode(sbi))
 		return -EACCES;
 
@@ -661,6 +789,9 @@ static int autofs4_dir_rmdir(struct inod
 		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(&dentry->d_lock);
 	__d_drop(dentry);
 	spin_unlock(&dentry->d_lock);
--- linux-2.6.20/fs/autofs4/inode.c.lookup-expire-race	2007-02-05 03:44:54.000000000 +0900
+++ linux-2.6.20/fs/autofs4/inode.c	2007-02-12 12:16:27.000000000 +0900
@@ -48,6 +48,8 @@ struct autofs_info *autofs4_init_ino(str
 	ino->dentry = NULL;
 	ino->size = 0;
 
+	INIT_LIST_HEAD(&ino->rehash);
+
 	ino->last_used = jiffies;
 	atomic_set(&ino->count, 0);
 
@@ -158,14 +160,13 @@ void autofs4_kill_sb(struct super_block 
 	if (!sbi)
 		goto out_kill_sb;
 
-	sb->s_fs_info = NULL;
-
-	if ( !sbi->catatonic )
+	if (!sbi->catatonic)
 		autofs4_catatonic_mode(sbi); /* Free wait queues, close pipe */
 
 	/* Clean up and release dangling references */
 	autofs4_force_release(sbi);
 
+	sb->s_fs_info = NULL;
 	kfree(sbi);
 
 out_kill_sb:
@@ -336,6 +337,8 @@ int autofs4_fill_super(struct super_bloc
 	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);
 	s->s_blocksize = 1024;
 	s->s_blocksize_bits = 10;
 	s->s_magic = AUTOFS_SUPER_MAGIC;

Re: [PATCH 2/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Ian Kent]

On Mon, 2007-02-12 at 15:43 +0900, Ian Kent wrote:
> On Thu, 2007-02-08 at 11:33 +0900, Ian Kent wrote:
> > On Wed, 2007-02-07 at 19:18 +0100, Olivier Galibert wrote:
> > > On Thu, Feb 08, 2007 at 03:07:41AM +0900, Ian Kent wrote:
> > > > It may be better to update to a later kernel so I don't have to port the
> > > > patch to several different kernels. Is that possible?
> > > 
> > > Sure, 2.6.20 or -git?
> > 
> > 2.6.20 has all the patches I've proposed so far except for the one we're
> > working on so that would be best for me.
> > 
> > Seems there may still be a problem with the patch so I'll let you know
> > what's happening as soon as I can.
> 
> I think I'm just about done.
> 
> Could you try using the two patches here against 2.6.20 please:
> 
> Ian

---

--- linux-2.6.20/fs/autofs4/root.c.lookup-check-unhased	2007-02-12 13:49:46.000000000 +0900
+++ linux-2.6.20/fs/autofs4/root.c	2007-02-12 13:54:58.000000000 +0900
@@ -655,14 +655,29 @@ static struct dentry *autofs4_lookup(str
 
 	/*
 	 * If this dentry is unhashed, then we shouldn't honour this
-	 * lookup even if the dentry is positive.  Returning ENOENT here
-	 * doesn't do the right thing for all system calls, but it should
-	 * be OK for the operations we permit from an autofs.
+	 * lookup.  Returning ENOENT here doesn't do the right thing
+	 * for all system calls, but it should be OK for the operations
+	 * we permit from an autofs.
 	 */
 	if (dentry->d_inode && d_unhashed(dentry)) {
+		/*
+		 * A user space application can (and has done in the past)
+		 * remove and re-create this directory during the callback.
+		 * This can leave us with an unhashed dentry, but a
+		 * successful mount!  So we need to perform another
+		 * cached lookup in case the dentry now exists.
+		 */
+		struct dentry *parent = dentry->d_parent;
+		struct dentry *new = d_lookup(parent, &dentry->d_name);
+		if (new != NULL)
+			dentry = new;
+		else
+			dentry = ERR_PTR(-ENOENT);
+
 		if (unhashed)
 			dput(unhashed);
-		return ERR_PTR(-ENOENT);
+
+		return dentry;
 	}
 
 	if (unhashed)

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Olivier Galibert]

On Mon, Feb 12, 2007 at 03:43:14PM +0900, Ian Kent wrote:
> On Thu, 2007-02-08 at 11:33 +0900, Ian Kent wrote:
> > On Wed, 2007-02-07 at 19:18 +0100, Olivier Galibert wrote:
> > > On Thu, Feb 08, 2007 at 03:07:41AM +0900, Ian Kent wrote:
> > > > It may be better to update to a later kernel so I don't have to port the
> > > > patch to several different kernels. Is that possible?
> > > 
> > > Sure, 2.6.20 or -git?
> > 
> > 2.6.20 has all the patches I've proposed so far except for the one we're
> > working on so that would be best for me.
> > 
> > Seems there may still be a problem with the patch so I'll let you know
> > what's happening as soon as I can.
> 
> I think I'm just about done.
> 
> Could you try using the two patches here against 2.6.20 please:

The patch works beautifully, no more failures with my test rig, until
the point where the kernel crashes.  Since the crashes happen without
the patch too, you're off the hook, but that means I can't deploy it
for harsher testing yet.

No wonder Dave Jones is prudent about updating kernels in fc :-)

  OG.

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Ian Kent]

On Mon, 2007-02-12 at 14:57 +0100, Olivier Galibert wrote:
> On Mon, Feb 12, 2007 at 03:43:14PM +0900, Ian Kent wrote:
> > On Thu, 2007-02-08 at 11:33 +0900, Ian Kent wrote:
> > > On Wed, 2007-02-07 at 19:18 +0100, Olivier Galibert wrote:
> > > > On Thu, Feb 08, 2007 at 03:07:41AM +0900, Ian Kent wrote:
> > > > > It may be better to update to a later kernel so I don't have to port the
> > > > > patch to several different kernels. Is that possible?
> > > > 
> > > > Sure, 2.6.20 or -git?
> > > 
> > > 2.6.20 has all the patches I've proposed so far except for the one we're
> > > working on so that would be best for me.
> > > 
> > > Seems there may still be a problem with the patch so I'll let you know
> > > what's happening as soon as I can.
> > 
> > I think I'm just about done.
> > 
> > Could you try using the two patches here against 2.6.20 please:
> 
> The patch works beautifully, no more failures with my test rig, until
> the point where the kernel crashes.  Since the crashes happen without
> the patch too, you're off the hook, but that means I can't deploy it
> for harsher testing yet.
> 
> No wonder Dave Jones is prudent about updating kernels in fc :-)

Indeed.
Which kernel can you use?
I believe that 2200 had another problem so can you use an fc5 kernel
later than that?

Ian

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Olivier Galibert]

On Tue, Feb 13, 2007 at 09:52:39AM +0900, Ian Kent wrote:
> Indeed.
> Which kernel can you use?
> I believe that 2200 had another problem so can you use an fc5 kernel
> later than that?

I've ported your patch to 2257 (nothing special, only moved lines),
and it seems to work beautifully.  I'm enlarging the testing.

  OG.

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Ian Kent]

On Tue, 2007-02-13 at 09:50 +0100, Olivier Galibert wrote:
> On Tue, Feb 13, 2007 at 09:52:39AM +0900, Ian Kent wrote:
> > Indeed.
> > Which kernel can you use?
> > I believe that 2200 had another problem so can you use an fc5 kernel
> > later than that?
> 
> I've ported your patch to 2257 (nothing special, only moved lines),
> and it seems to work beautifully.  I'm enlarging the testing.

OK. But be aware that there may (probably) be another patch missing from
that kernel. If you run into problems we'll go further into it.

Ian

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Chuck Ebbert]

Olivier Galibert wrote:
> On Tue, Feb 13, 2007 at 09:52:39AM +0900, Ian Kent wrote:
>> Indeed.
>> Which kernel can you use?
>> I believe that 2200 had another problem so can you use an fc5 kernel
>> later than that?
> 
> I've ported your patch to 2257 (nothing special, only moved lines),
> and it seems to work beautifully.  I'm enlarging the testing.
> 

If you get the patches into -stable they will end up in Fedora
kernels automatically. 2288 (based on 2.6.19) is in testing now...

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Olivier Galibert]

On Tue, Feb 13, 2007 at 09:07:27AM -0500, Chuck Ebbert wrote:
> Olivier Galibert wrote:
> > On Tue, Feb 13, 2007 at 09:52:39AM +0900, Ian Kent wrote:
> >> Indeed.
> >> Which kernel can you use?
> >> I believe that 2200 had another problem so can you use an fc5 kernel
> >> later than that?
> > 
> > I've ported your patch to 2257 (nothing special, only moved lines),
> > and it seems to work beautifully.  I'm enlarging the testing.
> > 
> 
> If you get the patches into -stable they will end up in Fedora
> kernels automatically. 2288 (based on 2.6.19) is in testing now...

Don't they require autofs5 to be of any use though?  That's not going
to be in fc until it's out of beta I guess.

  OG.

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Chuck Ebbert]

Olivier Galibert wrote:
>> If you get the patches into -stable they will end up in Fedora
>> kernels automatically. 2288 (based on 2.6.19) is in testing now...
> 
> Don't they require autofs5 to be of any use though?  That's not going
> to be in fc until it's out of beta I guess.

I didn't realize you needed other updates... never mind.

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Ian Kent]

On Tue, 2007-02-13 at 16:54 +0100, Olivier Galibert wrote:
> On Tue, Feb 13, 2007 at 09:07:27AM -0500, Chuck Ebbert wrote:
> > Olivier Galibert wrote:
> > > On Tue, Feb 13, 2007 at 09:52:39AM +0900, Ian Kent wrote:
> > >> Indeed.
> > >> Which kernel can you use?
> > >> I believe that 2200 had another problem so can you use an fc5 kernel
> > >> later than that?
> > > 
> > > I've ported your patch to 2257 (nothing special, only moved lines),
> > > and it seems to work beautifully.  I'm enlarging the testing.
> > > 
> > 
> > If you get the patches into -stable they will end up in Fedora
> > kernels automatically. 2288 (based on 2.6.19) is in testing now...
> 
> Don't they require autofs5 to be of any use though?  That's not going
> to be in fc until it's out of beta I guess.

Not really?

[raven@fc6 ~]$ cat /etc/redhat-release 
Fedora Core release 6 (Zod)
[raven@fc6 ~]$ rpm -q autofs
autofs-5.0.1-0.rc3.16
[raven@fc6 ~]$ rpm -q --changelog autofs| head -10
* Sun Feb 11 2007 Ian Kent <ikent@redhat.com> - 5.0.1-0.rc3.16
- update the "task done race" patch to fix a deadlock.

* Wed Feb 07 2007 Ian Kent <ikent@redhat.com> - 5.0.1-0.rc3.14
- fix race when setting task done (bz 227268).

* Tue Jan 30 2007 Ian Kent <ikent@redhat.com> - 5.0.1-0.rc3.12
- make double quote handing consistent (at least as much as we can).
- fix handling of trailing white space in wildcard lookup (forward port bz 199720).
- check fqdn of each interface when matching export access list (bz 213700).
[raven@fc6 ~]$ 

The Rawhide or FC-6 srpm should build fine on FC5.

Ian

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Olivier Galibert]

On Wed, Feb 14, 2007 at 02:35:15AM +0900, Ian Kent wrote:
> On Tue, 2007-02-13 at 16:54 +0100, Olivier Galibert wrote:
> > Don't they require autofs5 to be of any use though?  That's not going
> > to be in fc until it's out of beta I guess.
> 
> Not really?
> 
> [raven@fc6 ~]$ cat /etc/redhat-release 
> Fedora Core release 6 (Zod)
> [raven@fc6 ~]$ rpm -q autofs
> autofs-5.0.1-0.rc3.16

Oh cool, my guess was wrong.


> The Rawhide or FC-6 srpm should build fine on FC5.

The .spec that comes with the tgz works without a hitch too.  I guess
fc6 is going to have a *really* working automounter RSN then.

  OG.

Re: [PATCH 1/2] Re: [autofs] Bad race condition in the new autofs protocol somewhere

[Ian Kent]

On Tue, 2007-02-13 at 09:07 -0500, Chuck Ebbert wrote:
> Olivier Galibert wrote:
> > On Tue, Feb 13, 2007 at 09:52:39AM +0900, Ian Kent wrote:
> >> Indeed.
> >> Which kernel can you use?
> >> I believe that 2200 had another problem so can you use an fc5 kernel
> >> later than that?
> > 
> > I've ported your patch to 2257 (nothing special, only moved lines),
> > and it seems to work beautifully.  I'm enlarging the testing.
> > 
> 
> If you get the patches into -stable they will end up in Fedora
> kernels automatically. 2288 (based on 2.6.19) is in testing now...

This has just been done.
I'll be submitting the new patches soon, if all goes well.

Ian