[PATCH] bsg: fix the deadlock on discarding done commands

[PATCH] bsg: fix the deadlock on discarding done commands

[FUJITA Tomonori]

The previous commit to fix a blocking read bug put a bug that leads a
deadlock on discarding done commands. We don't need bsg_device's lock
there.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 block/bsg.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 2f78d7d..9b5f6d7 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -466,11 +466,8 @@ static int bsg_complete_all_commands(str
 	 */
 	ret = 0;
 	do {
-		spin_lock_irq(&bd->lock);
-		if (!bd->queued_cmds) {
-			spin_unlock_irq(&bd->lock);
+		if (!bd->queued_cmds)
 			break;
-		}
 
 		bc = bsg_get_done_cmd(bd);
 		if (IS_ERR(bc))
-- 
1.4.3.2

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[Jens Axboe]

On Sun, Jun 03 2007, FUJITA Tomonori wrote:
> The previous commit to fix a blocking read bug put a bug that leads a
> deadlock on discarding done commands. We don't need bsg_device's lock
> there.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  block/bsg.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 2f78d7d..9b5f6d7 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -466,11 +466,8 @@ static int bsg_complete_all_commands(str
>  	 */
>  	ret = 0;
>  	do {
> -		spin_lock_irq(&bd->lock);
> -		if (!bd->queued_cmds) {
> -			spin_unlock_irq(&bd->lock);
> +		if (!bd->queued_cmds)
>  			break;
> -		}
>  
>  		bc = bsg_get_done_cmd(bd);
>  		if (IS_ERR(bc))

int read should be atomic, but it probably still needs serialization.
I'd be more comfortable with just adding the appropriate
spin_unlock_irq() before bsg_get_done_cmd(). It's not a fast path
anyway, just for device going away.

-- 
Jens Axboe

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
Date: Mon, 4 Jun 2007 16:04:15 +0200

> On Sun, Jun 03 2007, FUJITA Tomonori wrote:
> > The previous commit to fix a blocking read bug put a bug that leads a
> > deadlock on discarding done commands. We don't need bsg_device's lock
> > there.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  block/bsg.c |    5 +----
> >  1 files changed, 1 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/bsg.c b/block/bsg.c
> > index 2f78d7d..9b5f6d7 100644
> > --- a/block/bsg.c
> > +++ b/block/bsg.c
> > @@ -466,11 +466,8 @@ static int bsg_complete_all_commands(str
> >  	 */
> >  	ret = 0;
> >  	do {
> > -		spin_lock_irq(&bd->lock);
> > -		if (!bd->queued_cmds) {
> > -			spin_unlock_irq(&bd->lock);
> > +		if (!bd->queued_cmds)
> >  			break;
> > -		}
> >  
> >  		bc = bsg_get_done_cmd(bd);
> >  		if (IS_ERR(bc))
> 
> int read should be atomic, but it probably still needs serialization.
> I'd be more comfortable with just adding the appropriate
> spin_unlock_irq() before bsg_get_done_cmd(). It's not a fast path
> anyway, just for device going away.

Ok, here's a patch though only one is accessing to a bsg device here,
I think. Anyway, sorry about the silly bug.

BTW, any chance to push bsg to mainline? The scsi mid-layer bidi is
not ready, but the block layer bidi is ready with the rq->next_rq
patch (and I have the bsg bidi patch). Then I can ask James to push
the SMP pass through patches to mainline.


diff --git a/block/bsg.c b/block/bsg.c
index 2f78d7d..5f4abc9 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -471,6 +471,7 @@ static int bsg_complete_all_commands(str
 			spin_unlock_irq(&bd->lock);
 			break;
 		}
+		spin_unlock_irq(&bd->lock);
 
 		bc = bsg_get_done_cmd(bd);
 		if (IS_ERR(bc))
-- 
1.4.3.2

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[Jens Axboe]

On Tue, Jun 05 2007, FUJITA Tomonori wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
> Date: Mon, 4 Jun 2007 16:04:15 +0200
> 
> > On Sun, Jun 03 2007, FUJITA Tomonori wrote:
> > > The previous commit to fix a blocking read bug put a bug that leads a
> > > deadlock on discarding done commands. We don't need bsg_device's lock
> > > there.
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > ---
> > >  block/bsg.c |    5 +----
> > >  1 files changed, 1 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/block/bsg.c b/block/bsg.c
> > > index 2f78d7d..9b5f6d7 100644
> > > --- a/block/bsg.c
> > > +++ b/block/bsg.c
> > > @@ -466,11 +466,8 @@ static int bsg_complete_all_commands(str
> > >  	 */
> > >  	ret = 0;
> > >  	do {
> > > -		spin_lock_irq(&bd->lock);
> > > -		if (!bd->queued_cmds) {
> > > -			spin_unlock_irq(&bd->lock);
> > > +		if (!bd->queued_cmds)
> > >  			break;
> > > -		}
> > >  
> > >  		bc = bsg_get_done_cmd(bd);
> > >  		if (IS_ERR(bc))
> > 
> > int read should be atomic, but it probably still needs serialization.
> > I'd be more comfortable with just adding the appropriate
> > spin_unlock_irq() before bsg_get_done_cmd(). It's not a fast path
> > anyway, just for device going away.
> 
> Ok, here's a patch though only one is accessing to a bsg device here,
> I think. Anyway, sorry about the silly bug.

Thanks, applied.

> BTW, any chance to push bsg to mainline? The scsi mid-layer bidi is
> not ready, but the block layer bidi is ready with the rq->next_rq
> patch (and I have the bsg bidi patch). Then I can ask James to push
> the SMP pass through patches to mainline.

Sure, might as well get on with it. I'll push it once 2.6.23 opens.

-- 
Jens Axboe

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
Date: Thu, 7 Jun 2007 13:23:09 +0200

> On Tue, Jun 05 2007, FUJITA Tomonori wrote:
> > From: Jens Axboe <jens.axboe@oracle.com>
> > Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
> > Date: Mon, 4 Jun 2007 16:04:15 +0200
> > 
> > > On Sun, Jun 03 2007, FUJITA Tomonori wrote:
> > > > The previous commit to fix a blocking read bug put a bug that leads a
> > > > deadlock on discarding done commands. We don't need bsg_device's lock
> > > > there.
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > ---
> > > >  block/bsg.c |    5 +----
> > > >  1 files changed, 1 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/block/bsg.c b/block/bsg.c
> > > > index 2f78d7d..9b5f6d7 100644
> > > > --- a/block/bsg.c
> > > > +++ b/block/bsg.c
> > > > @@ -466,11 +466,8 @@ static int bsg_complete_all_commands(str
> > > >  	 */
> > > >  	ret = 0;
> > > >  	do {
> > > > -		spin_lock_irq(&bd->lock);
> > > > -		if (!bd->queued_cmds) {
> > > > -			spin_unlock_irq(&bd->lock);
> > > > +		if (!bd->queued_cmds)
> > > >  			break;
> > > > -		}
> > > >  
> > > >  		bc = bsg_get_done_cmd(bd);
> > > >  		if (IS_ERR(bc))
> > > 
> > > int read should be atomic, but it probably still needs serialization.
> > > I'd be more comfortable with just adding the appropriate
> > > spin_unlock_irq() before bsg_get_done_cmd(). It's not a fast path
> > > anyway, just for device going away.
> > 
> > Ok, here's a patch though only one is accessing to a bsg device here,
> > I think. Anyway, sorry about the silly bug.
> 
> Thanks, applied.
> 
> > BTW, any chance to push bsg to mainline? The scsi mid-layer bidi is
> > not ready, but the block layer bidi is ready with the rq->next_rq
> > patch (and I have the bsg bidi patch). Then I can ask James to push
> > the SMP pass through patches to mainline.
> 
> Sure, might as well get on with it. I'll push it once 2.6.23 opens.

Cool, thanks.

I'll submit the bsg bidi patches shortly.

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[Jens Axboe]

On Sat, Jun 09 2007, FUJITA Tomonori wrote:
> I'll submit the bsg bidi patches shortly.

Great - I see them, will review and integrate next week.

-- 
Jens Axboe

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
Date: Thu, 7 Jun 2007 13:23:09 +0200

> On Tue, Jun 05 2007, FUJITA Tomonori wrote:
> > From: Jens Axboe <jens.axboe@oracle.com>
> > Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
> > Date: Mon, 4 Jun 2007 16:04:15 +0200
> > 
> > > On Sun, Jun 03 2007, FUJITA Tomonori wrote:
> > > > The previous commit to fix a blocking read bug put a bug that leads a
> > > > deadlock on discarding done commands. We don't need bsg_device's lock
> > > > there.
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > ---
> > > >  block/bsg.c |    5 +----
> > > >  1 files changed, 1 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/block/bsg.c b/block/bsg.c
> > > > index 2f78d7d..9b5f6d7 100644
> > > > --- a/block/bsg.c
> > > > +++ b/block/bsg.c
> > > > @@ -466,11 +466,8 @@ static int bsg_complete_all_commands(str
> > > >  	 */
> > > >  	ret = 0;
> > > >  	do {
> > > > -		spin_lock_irq(&bd->lock);
> > > > -		if (!bd->queued_cmds) {
> > > > -			spin_unlock_irq(&bd->lock);
> > > > +		if (!bd->queued_cmds)
> > > >  			break;
> > > > -		}
> > > >  
> > > >  		bc = bsg_get_done_cmd(bd);
> > > >  		if (IS_ERR(bc))
> > > 
> > > int read should be atomic, but it probably still needs serialization.
> > > I'd be more comfortable with just adding the appropriate
> > > spin_unlock_irq() before bsg_get_done_cmd(). It's not a fast path
> > > anyway, just for device going away.
> > 
> > Ok, here's a patch though only one is accessing to a bsg device here,
> > I think. Anyway, sorry about the silly bug.
> 
> Thanks, applied.
> 
> > BTW, any chance to push bsg to mainline? The scsi mid-layer bidi is
> > not ready, but the block layer bidi is ready with the rq->next_rq
> > patch (and I have the bsg bidi patch). Then I can ask James to push
> > the SMP pass through patches to mainline.
> 
> Sure, might as well get on with it. I'll push it once 2.6.23 opens.

James, any chance that the SMP passthru patches will be got into
2.6.23?

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[James Bottomley]

On Sat, 2007-06-09 at 01:33 +0900, FUJITA Tomonori wrote:
> James, any chance that the SMP passthru patches will be got into
> 2.6.23?

Yes, assuming we get sufficient progress on the required bidi bits.

I think we've got several weeks yet to get the 2.6.23 queue in shape

James

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[FUJITA Tomonori]

From: James Bottomley <James.Bottomley@SteelEye.com>
Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
Date: Fri, 08 Jun 2007 11:38:08 -0500

> On Sat, 2007-06-09 at 01:33 +0900, FUJITA Tomonori wrote:
> > James, any chance that the SMP passthru patches will be got into
> > 2.6.23?
> 
> Yes, assuming we get sufficient progress on the required bidi bits.
> 
> I think we've got several weeks yet to get the 2.6.23 queue in shape

Great, thanks. I'll update and send the SMP passthru patches on the
top of Jens' tree when Jens applies the bsg bidi patchset.

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
Date: Fri, 8 Jun 2007 17:20:49 +0200

> On Sat, Jun 09 2007, FUJITA Tomonori wrote:
> > I'll submit the bsg bidi patches shortly.
> 
> Great - I see them, will review and integrate next week.

Any progress on this?

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[Jens Axboe]

On Wed, Jun 20 2007, FUJITA Tomonori wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
> Date: Fri, 8 Jun 2007 17:20:49 +0200
> 
> > On Sat, Jun 09 2007, FUJITA Tomonori wrote:
> > > I'll submit the bsg bidi patches shortly.
> > 
> > Great - I see them, will review and integrate next week.
> 
> Any progress on this?

Please resend the bsg bidi patches so I can integrate them, thanks.

-- 
Jens Axboe

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
Date: Mon, 9 Jul 2007 14:26:44 +0200

> On Wed, Jun 20 2007, FUJITA Tomonori wrote:
> > From: Jens Axboe <jens.axboe@oracle.com>
> > Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
> > Date: Fri, 8 Jun 2007 17:20:49 +0200
> > 
> > > On Sat, Jun 09 2007, FUJITA Tomonori wrote:
> > > > I'll submit the bsg bidi patches shortly.
> > > 
> > > Great - I see them, will review and integrate next week.
> > 
> > Any progress on this?
> 
> Please resend the bsg bidi patches so I can integrate them, thanks.

Great!

I've just sent the patchset against the latest your bsg branch.

BTW, seems that you forget to add my sign-off in the last two patches
in the bsg branch.

Thanks.

Re: [PATCH] bsg: fix the deadlock on discarding done commands

[Jens Axboe]

On Mon, Jul 09 2007, FUJITA Tomonori wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
> Date: Mon, 9 Jul 2007 14:26:44 +0200
> 
> > On Wed, Jun 20 2007, FUJITA Tomonori wrote:
> > > From: Jens Axboe <jens.axboe@oracle.com>
> > > Subject: Re: [PATCH] bsg: fix the deadlock on discarding done commands
> > > Date: Fri, 8 Jun 2007 17:20:49 +0200
> > > 
> > > > On Sat, Jun 09 2007, FUJITA Tomonori wrote:
> > > > > I'll submit the bsg bidi patches shortly.
> > > > 
> > > > Great - I see them, will review and integrate next week.
> > > 
> > > Any progress on this?
> > 
> > Please resend the bsg bidi patches so I can integrate them, thanks.
> 
> Great!
> 
> I've just sent the patchset against the latest your bsg branch.

Thanks!

> BTW, seems that you forget to add my sign-off in the last two patches
> in the bsg branch.

Oh, my mistake, will add them. I just rebased it to 2.6.22 this morning.

-- 
Jens Axboe