From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Kent <raven@themaw.net>
Subject: [PATCH] autofs4 deadlock during expire - kernel 2.6
Date: Wed, 24 Sep 2003 21:01:56 +0800 (WST)
Sender: linux-kernel-owner@vger.kernel.org
Message-ID: <Pine.LNX.4.44.0309242059120.4975-100000@raven.themaw.net>
Mime-Version: 1.0
Return-path: <linux-kernel-owner+linux-kernel=40quimby.gnus.org@vger.kernel.org>
List-Id: <autofs.vger.kernel.org>
Content-Type: TEXT/PLAIN; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: Kernel Mailing List <linux-kernel@vger.kernel.org>
Cc: Maneesh Soni <maneesh@in.ibm.com>, autofs mailing list <autofs@linux.kernel.org>, Jeremy Fitzhardinge <jeremy@goop.org>


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/

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Arjan van de Ven <arjanv@redhat.com>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
Date: Wed, 24 Sep 2003 15:09:23 +0200
Sender: linux-kernel-owner@vger.kernel.org
Message-ID: <1064408962.5074.2.camel@laptop.fenrus.com>
References: <Pine.LNX.4.44.0309242059120.4975-100000@raven.themaw.net>
Reply-To: arjanv@redhat.com
Mime-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-Q+31+NTK/aqQ7VVAJFPi"
Return-path: <linux-kernel-owner+linux-kernel=40quimby.gnus.org@vger.kernel.org>
In-Reply-To: <Pine.LNX.4.44.0309242059120.4975-100000@raven.themaw.net>
List-Id: <autofs.vger.kernel.org>
To: Ian Kent <raven@themaw.net>
Cc: Kernel Mailing List <linux-kernel@vger.kernel.org>, Maneesh Soni <maneesh@in.ibm.com>, autofs mailing list <autofs@linux.kernel.org>, Jeremy Fitzhardinge <jeremy@goop.org>


--=-Q+31+NTK/aqQ7VVAJFPi
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On Wed, 2003-09-24 at 15:01, Ian Kent wrote:
> This is a corrected patch for the autofs4 daedlock problem I posted about=
=20
> @@ -206,6 +207,11 @@
> =20
>  		interruptible_sleep_on(&wq->queue);
> =20
> +		if (waitqueue_active(&wq->queue) && current !=3D 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 ?

--=-Q+31+NTK/aqQ7VVAJFPi
Content-Type: application/pgp-signature; name=signature.asc
Content-Description: This is a digitally signed message part

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD8DBQA/cZeCxULwo51rQBIRAkDHAJ4qCLI7pnWgAclLCYY1SGcdAW8pigCgjSCT
G07Mo5b63x517h+48RMPTjw=
=kfzT
-----END PGP SIGNATURE-----

--=-Q+31+NTK/aqQ7VVAJFPi--

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Kent <raven@themaw.net>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
Date: Wed, 24 Sep 2003 21:38:16 +0800 (WST)
Sender: linux-kernel-owner@vger.kernel.org
Message-ID: <Pine.LNX.4.44.0309242136080.6713-100000@raven.themaw.net>
References: <1064408962.5074.2.camel@laptop.fenrus.com>
Mime-Version: 1.0
Return-path: <linux-kernel-owner+linux-kernel=40quimby.gnus.org@vger.kernel.org>
In-Reply-To: <1064408962.5074.2.camel@laptop.fenrus.com>
List-Id: <autofs.vger.kernel.org>
Content-Type: TEXT/PLAIN; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: Arjan van de Ven <arjanv@redhat.com>
Cc: Kernel Mailing List <linux-kernel@vger.kernel.org>, Maneesh Soni <maneesh@in.ibm.com>, autofs mailing list <autofs@linux.kernel.org>, Jeremy Fitzhardinge <jeremy@goop.org>

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/

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Arjan van de Ven <arjanv@redhat.com>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
Date: Wed, 24 Sep 2003 15:57:20 +0200
Sender: linux-kernel-owner@vger.kernel.org
Message-ID: <20030924155720.C31236@devserv.devel.redhat.com>
References: <1064408962.5074.2.camel@laptop.fenrus.com> <Pine.LNX.4.44.0309242136080.6713-100000@raven.themaw.net>
Mime-Version: 1.0
Return-path: <linux-kernel-owner+linux-kernel=40quimby.gnus.org@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <Pine.LNX.4.44.0309242136080.6713-100000@raven.themaw.net>; from raven@themaw.net on Wed, Sep 24, 2003 at 09:38:16PM +0800
List-Id: <autofs.vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: Ian Kent <raven@themaw.net>
Cc: Arjan van de Ven <arjanv@redhat.com>, Kernel Mailing List <linux-kernel@vger.kernel.org>, Maneesh Soni <maneesh@in.ibm.com>, autofs mailing list <autofs@linux.kernel.org>, Jeremy Fitzhardinge <jeremy@goop.org>

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

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Kent <raven@themaw.net>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
Date: Wed, 24 Sep 2003 22:46:43 +0800 (WST)
Sender: linux-kernel-owner@vger.kernel.org
Message-ID: <Pine.LNX.4.44.0309242240500.6956-100000@raven.themaw.net>
References: <20030924155720.C31236@devserv.devel.redhat.com>
Mime-Version: 1.0
Return-path: <linux-kernel-owner+linux-kernel=40quimby.gnus.org@vger.kernel.org>
In-Reply-To: <20030924155720.C31236@devserv.devel.redhat.com>
List-Id: <autofs.vger.kernel.org>
Content-Type: TEXT/PLAIN; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: Arjan van de Ven <arjanv@redhat.com>
Cc: Kernel Mailing List <linux-kernel@vger.kernel.org>, Maneesh Soni <maneesh@in.ibm.com>, autofs mailing list <autofs@linux.kernel.org>, Jeremy Fitzhardinge <jeremy@goop.org>

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/

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mike Waychison <michael.waychison@sun.com>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
Date: Wed, 24 Sep 2003 11:54:37 -0400
Sender: linux-kernel-owner@vger.kernel.org
Message-ID: <3F71BE3D.6030501@sun.com>
References: <1064408962.5074.2.camel@laptop.fenrus.com> <Pine.LNX.4.44.0309242136080.6713-100000@raven.themaw.net>
Mime-Version: 1.0
Content-Type: multipart/mixed;
 boundary="------------050604060400060505010602"
Return-path: <linux-kernel-owner+linux-kernel=40quimby.gnus.org@vger.kernel.org>
In-Reply-To: <Pine.LNX.4.44.0309242136080.6713-100000@raven.themaw.net>
List-Id: <autofs.vger.kernel.org>
To: Ian Kent <raven@themaw.net>
Cc: Arjan van de Ven <arjanv@redhat.com>, Kernel Mailing List <linux-kernel@vger.kernel.org>, Maneesh Soni <maneesh@in.ibm.com>, autofs mailing list <autofs@linux.kernel.org>, Jeremy Fitzhardinge <jeremy@goop.org>

This is a multi-part message in MIME format.
--------------050604060400060505010602
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

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

--------------050604060400060505010602
Content-Type: text/plain;
 name="autofs4_wait_event.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="autofs4_wait_event.patch"

===== 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;

--------------050604060400060505010602--

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Kent <raven@themaw.net>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
Date: Thu, 25 Sep 2003 09:44:16 +0800 (WST)
Sender: autofs-bounces@linux.kernel.org
Message-ID: <Pine.LNX.4.33.0309250924160.18646-100000@wombat.indigo.net.au>
References: <3F71BE3D.6030501@sun.com>
Mime-Version: 1.0
Return-path: <autofs-bounces@linux.kernel.org>
In-Reply-To: <3F71BE3D.6030501@sun.com>
List-Id: <autofs.vger.kernel.org>
List-Unsubscribe: <http://linux.kernel.org/mailman/listinfo/autofs>,
	<mailto:autofs-request@linux.kernel.org?subject=unsubscribe>
List-Archive: <http://linux.kernel.org/pipermail/autofs>
List-Post: <mailto:autofs@linux.kernel.org>
List-Help: <mailto:autofs-request@linux.kernel.org?subject=help>
List-Subscribe: <http://linux.kernel.org/mailman/listinfo/autofs>,
	<mailto:autofs-request@linux.kernel.org?subject=subscribe>
Errors-To: autofs-bounces@linux.kernel.org
Content-Type: TEXT/PLAIN; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: Mike Waychison <michael.waychison@sun.com>
Cc: autofs mailing list <autofs@linux.kernel.org>, Arjan van de Ven <arjanv@redhat.com>, Kernel Mailing List <linux-kernel@vger.kernel.org>, Jeremy Fitzhardinge <jeremy@goop.org>

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/

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+willy=40w.ods.org@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S263365AbTIWNlh (ORCPT <rfc822;willy@w.ods.org>);
	Tue, 23 Sep 2003 09:41:37 -0400
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S263371AbTIWNlg
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Tue, 23 Sep 2003 09:41:36 -0400
Received: from mail-01.iinet.net.au ([203.59.3.33]:45024 "HELO
	mail.iinet.net.au") by vger.kernel.org with SMTP id S263365AbTIWNl1
	(ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 23 Sep 2003 09:41:27 -0400
Date: Tue, 23 Sep 2003 21:47:23 +0800 (WST)
From: Ian Kent <raven@themaw.net>
To: Kernel Mailing List <linux-kernel@vger.kernel.org>
cc: Maneesh Soni <maneesh@in.ibm.com>, Jeremy Fitzhardinge <jeremy@goop.org>
Subject: [PATCH] autofs4 deadlock during expire - kernel 2.6
Message-ID: <Pine.LNX.4.44.0309232125010.2341-100000@raven.themaw.net>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org


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/


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+willy=40w.ods.org@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S261627AbTIWPc7 (ORCPT <rfc822;willy@w.ods.org>);
	Tue, 23 Sep 2003 11:32:59 -0400
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261660AbTIWPc7
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Tue, 23 Sep 2003 11:32:59 -0400
Received: from CPE0010e000064f-CM014270111571.cpe.net.cable.rogers.com ([65.49.101.54]:23819
	"EHLO belle.augustahouse.net") by vger.kernel.org with ESMTP
	id S261627AbTIWPcy (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 23 Sep 2003 11:32:54 -0400
Message-ID: <3F706764.1090203@sun.com>
Date: Tue, 23 Sep 2003 11:31:48 -0400
From: Mike Waychison <michael.waychison@sun.com>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3.1) Gecko/20030618 Debian/1.3.1-3
X-Accept-Language: en
MIME-Version: 1.0
To: Ian Kent <raven@themaw.net>
Cc: Kernel Mailing List <linux-kernel@vger.kernel.org>,
       Maneesh Soni <maneesh@in.ibm.com>,
       Jeremy Fitzhardinge <jeremy@goop.org>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
References: <Pine.LNX.4.44.0309232125010.2341-100000@raven.themaw.net>
In-Reply-To: <Pine.LNX.4.44.0309232125010.2341-100000@raven.themaw.net>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

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


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+willy=40w.ods.org@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S261757AbTIWQSj (ORCPT <rfc822;willy@w.ods.org>);
	Tue, 23 Sep 2003 12:18:39 -0400
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261803AbTIWQSj
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Tue, 23 Sep 2003 12:18:39 -0400
Received: from mail-01.iinet.net.au ([203.59.3.33]:27339 "HELO
	mail.iinet.net.au") by vger.kernel.org with SMTP id S261757AbTIWQSg
	(ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 23 Sep 2003 12:18:36 -0400
Date: Wed, 24 Sep 2003 00:24:34 +0800 (WST)
From: Ian Kent <raven@themaw.net>
To: Mike Waychison <michael.waychison@sun.com>
cc: Kernel Mailing List <linux-kernel@vger.kernel.org>,
       Maneesh Soni <maneesh@in.ibm.com>,
       Jeremy Fitzhardinge <jeremy@goop.org>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
In-Reply-To: <3F706764.1090203@sun.com>
Message-ID: <Pine.LNX.4.44.0309232348480.2808-100000@raven.themaw.net>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

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/


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+willy=40w.ods.org@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S261486AbTIXQYc (ORCPT <rfc822;willy@w.ods.org>);
	Wed, 24 Sep 2003 12:24:32 -0400
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261487AbTIXQYc
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Wed, 24 Sep 2003 12:24:32 -0400
Received: from pix-525-pool.redhat.com ([66.187.233.200]:60047 "EHLO
	devserv.devel.redhat.com") by vger.kernel.org with ESMTP
	id S261486AbTIXQY3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 24 Sep 2003 12:24:29 -0400
Date: Wed, 24 Sep 2003 17:11:59 +0200
From: Arjan van de Ven <arjanv@redhat.com>
To: Ian Kent <raven@themaw.net>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
Message-ID: <20030924171159.A7214@devserv.devel.redhat.com>
References: <20030924155720.C31236@devserv.devel.redhat.com> <Pine.LNX.4.44.0309242240500.6956-100000@raven.themaw.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.2.5.1i
In-Reply-To: <Pine.LNX.4.44.0309242240500.6956-100000@raven.themaw.net>; from raven@themaw.net on Wed, Sep 24, 2003 at 10:46:43PM +0800
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

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);
.. ?

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+willy=40w.ods.org@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S261650AbTIYBoY (ORCPT <rfc822;willy@w.ods.org>);
	Wed, 24 Sep 2003 21:44:24 -0400
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261651AbTIYBoY
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Wed, 24 Sep 2003 21:44:24 -0400
Received: from wombat.indigo.net.au ([202.0.185.19]:8206 "EHLO
	wombat.indigo.net.au") by vger.kernel.org with ESMTP
	id S261650AbTIYBoW (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 24 Sep 2003 21:44:22 -0400
Date: Thu, 25 Sep 2003 09:44:16 +0800 (WST)
From: Ian Kent <raven@themaw.net>
X-X-Sender: <raven@wombat.indigo.net.au>
To: Mike Waychison <michael.waychison@sun.com>
cc: Arjan van de Ven <arjanv@redhat.com>,
       Kernel Mailing List <linux-kernel@vger.kernel.org>,
       Maneesh Soni <maneesh@in.ibm.com>,
       autofs mailing list <autofs@linux.kernel.org>,
       Jeremy Fitzhardinge <jeremy@goop.org>
Subject: Re: [PATCH] autofs4 deadlock during expire - kernel 2.6
In-Reply-To: <3F71BE3D.6030501@sun.com>
Message-ID: <Pine.LNX.4.33.0309250924160.18646-100000@wombat.indigo.net.au>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

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/


