[PATCH] autofs4 deadlock during expire - kernel 2.6

[PATCH] autofs4 deadlock during expire - kernel 2.6

[Ian Kent]

This is a corrected patch for the autofs4 daedlock problem I posted about 
yesterday.

As before comments please.

diff -Nur autofs4-2.6.orig/fs/autofs4/autofs_i.h autofs4-2.6.deadlock/fs/autofs4/autofs_i.h
--- autofs4-2.6.orig/fs/autofs4/autofs_i.h	2003-09-09 03:50:14.000000000 +0800
+++ autofs4-2.6.deadlock/fs/autofs4/autofs_i.h	2003-09-22 22:48:11.000000000 +0800
@@ -82,6 +82,7 @@
 	char *name;
 	/* This is for status reporting upon return */
 	int status;
+	struct task_struct *owner;
 	int wait_ctr;
 };
 
diff -Nur autofs4-2.6.orig/fs/autofs4/waitq.c autofs4-2.6.deadlock/fs/autofs4/waitq.c
--- autofs4-2.6.orig/fs/autofs4/waitq.c	2003-09-09 03:50:31.000000000 +0800
+++ autofs4-2.6.deadlock/fs/autofs4/waitq.c	2003-09-24 00:10:38.000000000 +0800
@@ -165,6 +165,7 @@
 		wq->status = -EINTR; /* Status return if interrupted */
 		memcpy(wq->name, name->name, name->len);
 		wq->next = sbi->queues;
+		wq->owner = current;
 		sbi->queues = wq;
 
 		DPRINTK(("autofs_wait: new wait id = 0x%08lx, name = %.*s, nfy=%d\n",
@@ -206,6 +207,11 @@
 
 		interruptible_sleep_on(&wq->queue);
 
+		if (waitqueue_active(&wq->queue) && current != wq->owner) {
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule_timeout(wq->wait_ctr * (HZ/10));
+		}
+
 		spin_lock_irqsave(&current->sighand->siglock, irqflags);
 		current->blocked = oldset;
 		recalc_sigpending();

-- 

   ,-._|\    Ian Kent
  /      \   Perth, Western Australia
  *_.--._/   E-mail: raven@themaw.net
        v    Web: http://themaw.net/

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Wed, 2003-09-24 at 15:01, Ian Kent wrote:
> This is a corrected patch for the autofs4 daedlock problem I posted about 
> @@ -206,6 +207,11 @@
>  
>  		interruptible_sleep_on(&wq->queue);
>  
> +		if (waitqueue_active(&wq->queue) && current != wq->owner) {
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule_timeout(wq->wait_ctr * (HZ/10));
> +		}
> +

this really really looks like you're trying to pamper over a bug by
changing the timing somewhere instead of fixing it...

also are you sure the deadlock isn't because of the racey use of
interruptible_sleep_on ?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Ian Kent]

On Wed, 24 Sep 2003, Arjan van de Ven wrote:

> On Wed, 2003-09-24 at 15:01, Ian Kent wrote:
> > This is a corrected patch for the autofs4 daedlock problem I posted about 
> > @@ -206,6 +207,11 @@
> >  
> >  		interruptible_sleep_on(&wq->queue);
> >  
> > +		if (waitqueue_active(&wq->queue) && current != wq->owner) {
> > +			set_current_state(TASK_INTERRUPTIBLE);
> > +			schedule_timeout(wq->wait_ctr * (HZ/10));
> > +		}
> > +
> 
> this really really looks like you're trying to pamper over a bug by
> changing the timing somewhere instead of fixing it...

Agreed.

> 
> also are you sure the deadlock isn't because of the racey use of
> interruptible_sleep_on ?
> 

OK so maybe I should have suggestions instead of comments.

Please elaborate.

-- 

   ,-._|\    Ian Kent
  /      \   Perth, Western Australia
  *_.--._/   E-mail: raven@themaw.net
        v    Web: http://themaw.net/

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Arjan van de Ven]

On Wed, Sep 24, 2003 at 09:38:16PM +0800, Ian Kent wrote:
> > interruptible_sleep_on ?
> > 
> 
> OK so maybe I should have suggestions instead of comments.

instead of interruptible_sleep_on(), it looks like you really want
to use completions for this code..
see kernel/workqueue.c for how those are used

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Ian Kent]

On Wed, 24 Sep 2003, Arjan van de Ven wrote:

> On Wed, Sep 24, 2003 at 09:38:16PM +0800, Ian Kent wrote:
> > > interruptible_sleep_on ?
> > > 
> > 
> > OK so maybe I should have suggestions instead of comments.
> 
> instead of interruptible_sleep_on(), it looks like you really want
> to use completions for this code..
> see kernel/workqueue.c for how those are used
> 

I did try that but thinking again ...

The actual expire really needs to be interrutible.
I didn't like the idea that the additional waiters would be 
uninterruptible but perhaps I have no choice.

Given that do you think that using an interruptible_sleep_on for the 
expire and a completion for the additional waiters could give an 
acceptable solution?

-- 

   ,-._|\    Ian Kent
  /      \   Perth, Western Australia
  *_.--._/   E-mail: raven@themaw.net
        v    Web: http://themaw.net/

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Arjan van de Ven]

On Wed, Sep 24, 2003 at 10:46:43PM +0800, Ian Kent wrote:
> The actual expire really needs to be interrutible.
> I didn't like the idea that the additional waiters would be 
> uninterruptible but perhaps I have no choice.
> 
> Given that do you think that using an interruptible_sleep_on for the 
> expire and a completion for the additional waiters could give an 
> acceptable solution?

No...
what happens if the person waking you does so inbetween the
        if ( wq->name ) {
test and the
                interruptible_sleep_on(&wq->queue);
.. ?

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Mike Waychison]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

Ian Kent wrote:
> On Wed, 24 Sep 2003, Arjan van de Ven wrote:
> 
> 
>>On Wed, 2003-09-24 at 15:01, Ian Kent wrote:
>>
>>>This is a corrected patch for the autofs4 daedlock problem I posted about 
>>>@@ -206,6 +207,11 @@
>>> 
>>> 		interruptible_sleep_on(&wq->queue);
>>> 
>>>+		if (waitqueue_active(&wq->queue) && current != wq->owner) {
>>>+			set_current_state(TASK_INTERRUPTIBLE);
>>>+			schedule_timeout(wq->wait_ctr * (HZ/10));
>>>+		}
>>>+
>>
>>this really really looks like you're trying to pamper over a bug by
>>changing the timing somewhere instead of fixing it...
> 
> 
> Agreed.
> 
> 
>>also are you sure the deadlock isn't because of the racey use of
>>interruptible_sleep_on ?
>>

I think the deadlock itself needs to be properly identified.

Could you explain where the deadlock is actually occuring?  I briefed 
over the automount 4 code as well as autofs4 and I don't see the 
deadlock.  The 'owner' in the case of an expiry will be a child process 
of the daemon, within a call to ioctl(EXPIRE_MULTI), correct?  Having it 
be released from the waitqueue first should not affect flow of execution 
and released from deadlock.

I don't see how having it wake up before before any other racing 
processes solves anything.

I think Arjan is right in that the race is do to the nautilus process 
entering the sleep_on after the a call to wake_up(&wq->queue).  I don't 
know if a change to using a workqueue is best..   how about refactoring 
that chunk of code to use wait_event_interruptible on the queue, which 
should be clear of any waitqueue/sleep_on races.

> 
> 
> OK so maybe I should have suggestions instead of comments.
> 
> Please elaborate.
> 

How about you try out this quick patch I threw together.

Mike Waychison

[-- Attachment #2: autofs4_wait_event.patch --]
[-- Type: text/plain, Size: 430 bytes --]

===== waitq.c 1.6 vs edited =====
--- 1.6/fs/autofs4/waitq.c	Fri Feb  7 12:25:20 2003
+++ edited/waitq.c	Wed Sep 24 15:48:30 2003
@@ -204,7 +204,7 @@
 		recalc_sigpending();
 		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
 
-		interruptible_sleep_on(&wq->queue);
+		wait_event_interruptible(wq->queue, wq->name == NULL);
 
 		spin_lock_irqsave(&current->sighand->siglock, irqflags);
 		current->blocked = oldset;

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Ian Kent]

On Wed, 24 Sep 2003, Mike Waychison wrote:

>
> I think the deadlock itself needs to be properly identified.
>
> Could you explain where the deadlock is actually occuring?  I briefed
> over the automount 4 code as well as autofs4 and I don't see the
> deadlock.  The 'owner' in the case of an expiry will be a child process
> of the daemon, within a call to ioctl(EXPIRE_MULTI), correct?  Having it
> be released from the waitqueue first should not affect flow of execution
> and released from deadlock.

Yes. I am having trouble defining the actual problem also.

I believe that the deadlock occures because of the sequence of calls
between the expire and Naultilus, each execution path taking and releasing
the BKL.

I will try and get more evidence as I work on it.

>
> I don't see how having it wake up before before any other racing
> processes solves anything.

I thought this was a side effect of the O(1) scheduler and that the
design of the wait handling left it open to a sequence of calls problem.
I first got the impression that it was related to the scheduler (and
felt that it was a deadlock) when I bumped the priority of the expire to
see what would happen and it work fine every time I tried it.

>
> I think Arjan is right in that the race is do to the nautilus process
> entering the sleep_on after the a call to wake_up(&wq->queue).  I don't

That needs fixing for sure, apart from anything else.

> know if a change to using a workqueue is best..   how about refactoring
> that chunk of code to use wait_event_interruptible on the queue, which
> should be clear of any waitqueue/sleep_on races.

Exaclty what I thought after pondering Arjans' mail last night.
The expire must be interruptible.
This will show if there actually is a timing problem (I hope).

>
> >
> >
> > OK so maybe I should have suggestions instead of comments.
> >
> > Please elaborate.
> >
>
> How about you try out this quick patch I threw together.

Oops. Replied without looking at the patch.

I'll check it out after work tonight.

Thanks to all for the help.

-- 

   ,-._|\    Ian Kent
  /      \   Perth, Western Australia
  *_.--._/   E-mail: raven@themaw.net
        v    Web: http://themaw.net/

Re: [autofs] Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Ian Kent]

On Thu, 25 Sep 2003, Ian Kent wrote:

Seem to have lost the original message.
Please forgive the double indent.

I have tried the patch you recommended and have exactly the same symptoms.

> On Wed, 24 Sep 2003, Mike Waychison wrote:
> 
> >
> > I think the deadlock itself needs to be properly identified.
> >
> > Could you explain where the deadlock is actually occuring?  I briefed
> > over the automount 4 code as well as autofs4 and I don't see the
> > deadlock.  The 'owner' in the case of an expiry will be a child process
> > of the daemon, within a call to ioctl(EXPIRE_MULTI), correct?  Having it
> > be released from the waitqueue first should not affect flow of execution
> > and released from deadlock.

I have had some time to clear my thoughts on this now.

First, the expire is done using a while in the daemon. It continues until 
the ioctl returns non zero.

I believe the sequence of events is:

Expire locates an expireable dentry and sends a message to the daemon to 
do the umount and returns 0. The ioctl runs with the BKL.
The Naultilus process triggers a revalidate during the expire and waits 
till that iteration of the expire finishes before continuing. The 
revalidate leads to a lookup which takes the BKL.
Meanwhile the expire enters the ioctl again but is blocked on the BKL.
The lookup sends a mount request to the daemon which it cannot hear 
because it is still waiting for the expire to finish.

While this is a plausible description with the code as it is, I 
have found that even if I bracket the revalidate with a BKL the problem 
still occurs. 
So there is something else I am missing. I don't know what it is.

> > I don't see how having it wake up before before any other racing
> > processes solves anything.

If my description above where correct then forcing the expire to finish 
would lead to the correct behaviour.

> 
> I thought this was a side effect of the O(1) scheduler and that the
> design of the wait handling left it open to a sequence of calls problem.
> I first got the impression that it was related to the scheduler (and
> felt that it was a deadlock) when I bumped the priority of the expire to
> see what would happen and it work fine every time I tried it.
> 

I still think that it is a O(1) effect. When the sleeping processes awake 
it is possible that the scheduler does not pick the expire process as it 
was the most recent of the processes to run.

> >
> > I think Arjan is right in that the race is do to the nautilus process
> > entering the sleep_on after the a call to wake_up(&wq->queue).  I don't
> 
> That needs fixing for sure, apart from anything else.
> 
> > know if a change to using a workqueue is best..   how about refactoring
> > that chunk of code to use wait_event_interruptible on the queue, which
> > should be clear of any waitqueue/sleep_on races.

Again if my explanation is close to correct then the question is what is 
the proper way to force the execution order? Using a completion as well as 
the wait perhaps?

-- 

   ,-._|\    Ian Kent
  /      \   Perth, Western Australia
  *_.--._/   E-mail: raven@themaw.net
        v    Web: http://themaw.net/

[PATCH] autofs4 deadlock during expire - kernel 2.6

[Ian Kent]

I have noticed that a deadlock can occur in the autofs4 module in the 2.6 
kernel and RedHat kernels with the O(1) scheduler patch. The easiest way 
to reproduce it is with an autofs-4.0.0 tree mount with at least 10 mounts 
within a Nautilus desktop environment with the module debug flag set. 
Perhaps this is because of the longish amount of time spent in the expire 
state.

The scenario:

1) An expire locates an expirable dentry and signals the user space 
daemon to umount it.
2) During the umount operation Nautilus notices and scans entries in the 
directory tree triggering a revalidate/lookup.
3) autofs4 places the requesting process on the umount wait queue. 
4) At completion of the expire both process are released from the wait 
queue.
5) If the lookup gets a quantum first deadlock occurs and expiration stops 
leaving an autofs mount that is permanently busy.

The following patch fixes this by ensuring that the expire gets a lookin 
before the revalidate/lookup continues.

The problem of the remount that this causes remains and I an not sure how 
to deal with that at this stage.

Comments and suggestions please.

Is this acceptable for inclusion in the kernel?
If so what should I do to make this happen?

diff -Nur autofs4-2.6.orig/fs/autofs4/autofs_i.h autofs4-2.6.deadlock/fs/autofs4/autofs_i.h
--- autofs4-2.6.orig/fs/autofs4/autofs_i.h	2003-09-09 03:50:14.000000000 +0800
+++ autofs4-2.6.deadlock/fs/autofs4/autofs_i.h	2003-09-22 22:48:11.000000000 +0800
@@ -82,6 +82,7 @@
 	char *name;
 	/* This is for status reporting upon return */
 	int status;
+	struct task_struct *owner;
 	int wait_ctr;
 };
 
diff -Nur autofs4-2.6.orig/fs/autofs4/waitq.c autofs4-2.6.deadlock/fs/autofs4/waitq.c
--- autofs4-2.6.orig/fs/autofs4/waitq.c	2003-09-09 03:50:31.000000000 +0800
+++ autofs4-2.6.deadlock/fs/autofs4/waitq.c	2003-09-23 00:02:29.209789432 +0800
@@ -206,6 +206,11 @@
 
 		interruptible_sleep_on(&wq->queue);
 
+		if (waitqueue_active(&wq->queue) && current != wq->owner) {
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule_timeout(wq->wait_ctr * (HZ/10));
+		}
+
 		spin_lock_irqsave(&current->sighand->siglock, irqflags);
 		current->blocked = oldset;
 		recalc_sigpending();

-- 

   ,-._|\    Ian Kent
  /      \   Perth, Western Australia
  *_.--._/   E-mail: raven@themaw.net
        v    Web: http://themaw.net/

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Mike Waychison]

Ian Kent wrote:
> I have noticed that a deadlock can occur in the autofs4 module in the 2.6 
> kernel and RedHat kernels with the O(1) scheduler patch. The easiest way 
> to reproduce it is with an autofs-4.0.0 tree mount with at least 10 mounts 
> within a Nautilus desktop environment with the module debug flag set. 
> Perhaps this is because of the longish amount of time spent in the expire 
> state.
> 
> The scenario:
> 
> 1) An expire locates an expirable dentry and signals the user space 
> daemon to umount it.
> 2) During the umount operation Nautilus notices and scans entries in the 
> directory tree triggering a revalidate/lookup.
> 3) autofs4 places the requesting process on the umount wait queue. 
> 4) At completion of the expire both process are released from the wait 
> queue.
> 5) If the lookup gets a quantum first deadlock occurs and expiration stops 
> leaving an autofs mount that is permanently busy.
> 
> The following patch fixes this by ensuring that the expire gets a lookin 
> before the revalidate/lookup continues.

I'm not extremely familiar with the autofs4 implementation, but why is 
the expiring process in the wait queue to start off with?  Doesn't 
autofs4 let the daemon's pgrp bypass the waitqueue completely?

> 
> The problem of the remount that this causes remains and I an not sure how 
> to deal with that at this stage.

This is purely a userspace issue.  Nautilus trampling all over the 
filesystem is greatly unjustified.  If they want a decent filesystem 
change notification system put in place, let them propose something. 
Caching mountpoints entries is a bad idea.

> 
> Comments and suggestions please.
> 
> Is this acceptable for inclusion in the kernel?
> If so what should I do to make this happen?
> 
> diff -Nur autofs4-2.6.orig/fs/autofs4/autofs_i.h autofs4-2.6.deadlock/fs/autofs4/autofs_i.h
> --- autofs4-2.6.orig/fs/autofs4/autofs_i.h	2003-09-09 03:50:14.000000000 +0800
> +++ autofs4-2.6.deadlock/fs/autofs4/autofs_i.h	2003-09-22 22:48:11.000000000 +0800
> @@ -82,6 +82,7 @@
>  	char *name;
>  	/* This is for status reporting upon return */
>  	int status;
> +	struct task_struct *owner;

This new field isn't even set anywhere!!

>  	int wait_ctr;
>  };
>  
> diff -Nur autofs4-2.6.orig/fs/autofs4/waitq.c autofs4-2.6.deadlock/fs/autofs4/waitq.c
> --- autofs4-2.6.orig/fs/autofs4/waitq.c	2003-09-09 03:50:31.000000000 +0800
> +++ autofs4-2.6.deadlock/fs/autofs4/waitq.c	2003-09-23 00:02:29.209789432 +0800
> @@ -206,6 +206,11 @@
>  
>  		interruptible_sleep_on(&wq->queue);
>  
> +		if (waitqueue_active(&wq->queue) && current != wq->owner) {
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule_timeout(wq->wait_ctr * (HZ/10));
> +		}
> +

This doesn't avoid the problem, it just makes it go away 99.99% of the 
time.  I would suggest using a complete_all from the owner of the wq, 
and let all other waiters wait for the completion.

Again, why is the umount on this queue?

Mike Waychison

Re: [PATCH] autofs4 deadlock during expire - kernel 2.6

[Ian Kent]

On Tue, 23 Sep 2003, Mike Waychison wrote:

> I'm not extremely familiar with the autofs4 implementation, but why is 
> the expiring process in the wait queue to start off with?  Doesn't 
> autofs4 let the daemon's pgrp bypass the waitqueue completely?
> 

The daemon issues expire requests to the kernel module every timeout 
interval. When the kernel module identifies a eligible dentry to expire it 
posts a message to the user space daemon and waits on a wait queue for the 
daemon to signal completion. Any other process that do a lookup on the 
dentry being expired wait on the queue for completion. Upon completion 
the waiters are awoken. The O(1) scheduler never seems to pick the 
process that posted the expire whereas the 2.4 kernel does. I have tried 
an exclusive wait queue without success (among other things). Delaying 
waiters other than the owner seems a fairly effective method of 
letting the owner complete its task before the other waiters continue.

> > 
> > The problem of the remount that this causes remains and I an not sure how 
> > to deal with that at this stage.
> 
> This is purely a userspace issue.  Nautilus trampling all over the 
> filesystem is greatly unjustified.  If they want a decent filesystem 
> change notification system put in place, let them propose something. 
> Caching mountpoints entries is a bad idea.

Yes and maybe I can do something about in the userspace daemon, but it is 
a problem that exists and we need to ba aware of.

I have subscribed to the Nautilus list and got some pointers to the parts 
of the code that may be in question. I haven't had a chance to examine it 
yet.

> 
> > 
> > Comments and suggestions please.
> > 
> > Is this acceptable for inclusion in the kernel?
> > If so what should I do to make this happen?
> > 
> > diff -Nur autofs4-2.6.orig/fs/autofs4/autofs_i.h autofs4-2.6.deadlock/fs/autofs4/autofs_i.h
> > --- autofs4-2.6.orig/fs/autofs4/autofs_i.h	2003-09-09 03:50:14.000000000 +0800
> > +++ autofs4-2.6.deadlock/fs/autofs4/autofs_i.h	2003-09-22 22:48:11.000000000 +0800
> > @@ -82,6 +82,7 @@
> >  	char *name;
> >  	/* This is for status reporting upon return */
> >  	int status;
> > +	struct task_struct *owner;
> 
> This new field isn't even set anywhere!!

Right you are, sorry, I'll repost a corrected patch.

It's late and I'll have to test it again to make sure it works, so that 
won't be till tomorrow.

Clearly this will only work for the case of one additional waiter that is 
scheduled before the wait owner. The case that occurs here.

> 
> >  	int wait_ctr;
> >  };
> >  
> > diff -Nur autofs4-2.6.orig/fs/autofs4/waitq.c autofs4-2.6.deadlock/fs/autofs4/waitq.c
> > --- autofs4-2.6.orig/fs/autofs4/waitq.c	2003-09-09 03:50:31.000000000 +0800
> > +++ autofs4-2.6.deadlock/fs/autofs4/waitq.c	2003-09-23 00:02:29.209789432 +0800
> > @@ -206,6 +206,11 @@
> >  
> >  		interruptible_sleep_on(&wq->queue);
> >  
> > +		if (waitqueue_active(&wq->queue) && current != wq->owner) {
> > +			set_current_state(TASK_INTERRUPTIBLE);
> > +			schedule_timeout(wq->wait_ctr * (HZ/10));
> > +		}
> > +
> 
> This doesn't avoid the problem, it just makes it go away 99.99% of the 
> time.  I would suggest using a complete_all from the owner of the wq, 
> and let all other waiters wait for the completion.

I thought of that also. I tried to work a completion into the code without 
success. Anyway, an exclusive wait queue, similar in concept, didn't 
solve the problem. It was still be necessary to delay releasing 
other waiters.

Perhaps you would like to offer some code as an example of how best to do 
this with a completion or other means.

-- 

   ,-._|\    Ian Kent
  /      \   Perth, Western Australia
  *_.--._/   E-mail: raven@themaw.net
        v    Web: http://themaw.net/