[PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[FUJITA Tomonori]

This reverts commit 1cd96c242a829d52f7a5ae98f554ca9775429685.

commit 1cd96c242a829d52f7a5ae98f554ca9775429685
Author: Boaz Harrosh <bharrosh@panasas.com>
Date:   Tue Mar 24 12:35:07 2009 +0100

    block: WARN in __blk_put_request() for potential bio leak

    Put a WARN_ON in __blk_put_request if it is about to
    leak bio(s). This is a serious bug that can happen in error
    handling code paths.

    For this to work I have fixed a couple of places in block/ where
    request->bio != NULL ownership was not honored. And a small cleanup
    at sg_io() while at it.


With 2.6.30-rc, BSG SMP requests get the following warnings:

WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()

However, this is false. There is no bio leak wrt BSG SMP
requests. Probably the better fix is calling blk_end_request_all() in
the BSG SMP path.

blk_end_request_all() is not very useful for the BSG SMP path (we call
it to just unlink rq->bio) however calling blk_end_request_all() in
all bio users is consistent.

blk_end_request_all() is not available in 2.6.30-rc so seems that the
simplest fix is removing WARN_ON for now.

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

diff --git a/block/blk-core.c b/block/blk-core.c
index c89883b..4aacb3c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1064,9 +1064,6 @@ void __blk_put_request(struct request_queue *q, struct request *req)
 
 	elv_completed_request(q, req);
 
-	/* this is a bio leak */
-	WARN_ON(req->bio != NULL);
-
 	/*
 	 * Request may not have originated from ll_rw_blk. if not,
 	 * it didn't come out of our reserved rq pools
-- 
1.6.0.6

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[Boaz Harrosh]

On 06/09/2009 01:44 PM, FUJITA Tomonori wrote:
> This reverts commit 1cd96c242a829d52f7a5ae98f554ca9775429685.
> 
> commit 1cd96c242a829d52f7a5ae98f554ca9775429685
> Author: Boaz Harrosh <bharrosh@panasas.com>
> Date:   Tue Mar 24 12:35:07 2009 +0100
> 
>     block: WARN in __blk_put_request() for potential bio leak
> 
>     Put a WARN_ON in __blk_put_request if it is about to
>     leak bio(s). This is a serious bug that can happen in error
>     handling code paths.
> 
>     For this to work I have fixed a couple of places in block/ where
>     request->bio != NULL ownership was not honored. And a small cleanup
>     at sg_io() while at it.
> 
> 
> With 2.6.30-rc, BSG SMP requests get the following warnings:
> 
> WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()
> 
> However, this is false. There is no bio leak wrt BSG SMP
> requests. Probably the better fix is calling blk_end_request_all() in
> the BSG SMP path.
> 
> blk_end_request_all() is not very useful for the BSG SMP path (we call
> it to just unlink rq->bio) however calling blk_end_request_all() in
> all bio users is consistent.
> 
> blk_end_request_all() is not available in 2.6.30-rc so seems that the
> simplest fix is removing WARN_ON for now.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Please do not revert. This is the point of all this.

If there is no leak, You should NULL out the req->bio
for now, and for 2.6.31 change the code to do 
blk_end_request_all(). That's what blk_end_request does,
since you are doing your own completion then set req->bio
to null after you're done. (And before put_request)

This stuff is good for error paths to catch leaks, please
leave it?

> ---
>  block/blk-core.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index c89883b..4aacb3c 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1064,9 +1064,6 @@ void __blk_put_request(struct request_queue *q, struct request *req)
>  
>  	elv_completed_request(q, req);
>  
> -	/* this is a bio leak */
> -	WARN_ON(req->bio != NULL);
> -
>  	/*
>  	 * Request may not have originated from ll_rw_blk. if not,
>  	 * it didn't come out of our reserved rq pools

Please NO
Boaz

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[FUJITA Tomonori]

On Tue, 09 Jun 2009 14:53:51 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 06/09/2009 01:44 PM, FUJITA Tomonori wrote:
> > This reverts commit 1cd96c242a829d52f7a5ae98f554ca9775429685.
> > 
> > commit 1cd96c242a829d52f7a5ae98f554ca9775429685
> > Author: Boaz Harrosh <bharrosh@panasas.com>
> > Date:   Tue Mar 24 12:35:07 2009 +0100
> > 
> >     block: WARN in __blk_put_request() for potential bio leak
> > 
> >     Put a WARN_ON in __blk_put_request if it is about to
> >     leak bio(s). This is a serious bug that can happen in error
> >     handling code paths.
> > 
> >     For this to work I have fixed a couple of places in block/ where
> >     request->bio != NULL ownership was not honored. And a small cleanup
> >     at sg_io() while at it.
> > 
> > 
> > With 2.6.30-rc, BSG SMP requests get the following warnings:
> > 
> > WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()
> > 
> > However, this is false. There is no bio leak wrt BSG SMP
> > requests. Probably the better fix is calling blk_end_request_all() in
> > the BSG SMP path.
> > 
> > blk_end_request_all() is not very useful for the BSG SMP path (we call
> > it to just unlink rq->bio) however calling blk_end_request_all() in
> > all bio users is consistent.
> > 
> > blk_end_request_all() is not available in 2.6.30-rc so seems that the
> > simplest fix is removing WARN_ON for now.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> Please do not revert. This is the point of all this.
> 
> If there is no leak, You should NULL out the req->bio
> for now, and for 2.6.31 change the code to do 
> blk_end_request_all(). That's what blk_end_request does,
> since you are doing your own completion then set req->bio
> to null after you're done. (And before put_request)
> 
> This stuff is good for error paths to catch leaks, please
> leave it?

Has this your good stuff found any bio leak bugs in mainline? In
addition, breaking working code is not a proper development style.

Anyway, setting req->bio in bsg works. Either is fine by me.


Jens, can you please send either patch to Linus now?

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] bsg: setting rq->bio to NULL

Due to commit 1cd96c242a829d52f7a5ae98f554ca9775429685 ("block: WARN
in __blk_put_request() for potential bio leak"), BSG SMP requests get
the false warnings:

WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()

This sets rq->bio to NULL to avoid that false warnings.


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

diff --git a/block/bsg.c b/block/bsg.c
index 206060e..dd81be4 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -315,6 +315,7 @@ out:
 	blk_put_request(rq);
 	if (next_rq) {
 		blk_rq_unmap_user(next_rq->bio);
+		next_rq->bio = NULL;
 		blk_put_request(next_rq);
 	}
 	return ERR_PTR(ret);
@@ -448,6 +449,7 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 		hdr->dout_resid = rq->data_len;
 		hdr->din_resid = rq->next_rq->data_len;
 		blk_rq_unmap_user(bidi_bio);
+		rq->next_rq->bio = NULL;
 		blk_put_request(rq->next_rq);
 	} else if (rq_data_dir(rq) == READ)
 		hdr->din_resid = rq->data_len;
@@ -466,6 +468,7 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	blk_rq_unmap_user(bio);
 	if (rq->cmd != rq->__cmd)
 		kfree(rq->cmd);
+	rq->bio = NULL;
 	blk_put_request(rq);
 
 	return ret;
-- 
1.5.6.5

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[Jens Axboe]

On Tue, Jun 09 2009, FUJITA Tomonori wrote:
> On Tue, 09 Jun 2009 14:53:51 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> > On 06/09/2009 01:44 PM, FUJITA Tomonori wrote:
> > > This reverts commit 1cd96c242a829d52f7a5ae98f554ca9775429685.
> > > 
> > > commit 1cd96c242a829d52f7a5ae98f554ca9775429685
> > > Author: Boaz Harrosh <bharrosh@panasas.com>
> > > Date:   Tue Mar 24 12:35:07 2009 +0100
> > > 
> > >     block: WARN in __blk_put_request() for potential bio leak
> > > 
> > >     Put a WARN_ON in __blk_put_request if it is about to
> > >     leak bio(s). This is a serious bug that can happen in error
> > >     handling code paths.
> > > 
> > >     For this to work I have fixed a couple of places in block/ where
> > >     request->bio != NULL ownership was not honored. And a small cleanup
> > >     at sg_io() while at it.
> > > 
> > > 
> > > With 2.6.30-rc, BSG SMP requests get the following warnings:
> > > 
> > > WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()
> > > 
> > > However, this is false. There is no bio leak wrt BSG SMP
> > > requests. Probably the better fix is calling blk_end_request_all() in
> > > the BSG SMP path.
> > > 
> > > blk_end_request_all() is not very useful for the BSG SMP path (we call
> > > it to just unlink rq->bio) however calling blk_end_request_all() in
> > > all bio users is consistent.
> > > 
> > > blk_end_request_all() is not available in 2.6.30-rc so seems that the
> > > simplest fix is removing WARN_ON for now.
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > 
> > Please do not revert. This is the point of all this.
> > 
> > If there is no leak, You should NULL out the req->bio
> > for now, and for 2.6.31 change the code to do 
> > blk_end_request_all(). That's what blk_end_request does,
> > since you are doing your own completion then set req->bio
> > to null after you're done. (And before put_request)
> > 
> > This stuff is good for error paths to catch leaks, please
> > leave it?
> 
> Has this your good stuff found any bio leak bugs in mainline? In
> addition, breaking working code is not a proper development style.

That was my original question either, code like this has an ugly
tendency to cause unnecessary problems while never catching any bad
usage.

> Anyway, setting req->bio in bsg works. Either is fine by me.
> 
> 
> Jens, can you please send either patch to Linus now?

Sure, I'll push it out now. There's nothing pending for 2.6.30 ATM.

-- 
Jens Axboe

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[Boaz Harrosh]

On 06/09/2009 04:10 PM, FUJITA Tomonori wrote:
> On Tue, 09 Jun 2009 14:53:51 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> Please do not revert. This is the point of all this.
>>
>> If there is no leak, You should NULL out the req->bio
>> for now, and for 2.6.31 change the code to do 
>> blk_end_request_all(). That's what blk_end_request does,
>> since you are doing your own completion then set req->bio
>> to null after you're done. (And before put_request)
>>
>> This stuff is good for error paths to catch leaks, please
>> leave it?
> 
> Has this your good stuff found any bio leak bugs in mainline? In
> addition, breaking working code is not a proper development style.
> 

It has found for me in error paths. That's why I put it.

I Issue bsg bidi commands every day all day, and never seen this.
What driver are you using? and can you post the stack trace.

The driver does something wrong. bsg over scsi-ml does not have this
problem. Why?

> Anyway, setting req->bio in bsg works. Either is fine by me.
> 
> 
> Jens, can you please send either patch to Linus now?
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] bsg: setting rq->bio to NULL
> 
> Due to commit 1cd96c242a829d52f7a5ae98f554ca9775429685 ("block: WARN
> in __blk_put_request() for potential bio leak"), BSG SMP requests get
> the false warnings:
> 
> WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()
> 
> This sets rq->bio to NULL to avoid that false warnings.
> 
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  block/bsg.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 206060e..dd81be4 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -315,6 +315,7 @@ out:
>  	blk_put_request(rq);
>  	if (next_rq) {
>  		blk_rq_unmap_user(next_rq->bio);

I do not understand this here please explain? We have called blk_rq_map_user()
and have bailed out on error later, without calling blk_execute_rq*. Now usually
the bios are *double* referenced, one for the usual call of blk_end_request() that will
release bios once, and second for the blk_rq_unmap_user() that will release second time.
But here you only call blk_rq_unmap_user() don't you need to call blk_end_request() also?

> +		next_rq->bio = NULL;
>  		blk_put_request(next_rq);
>  	}
>  	return ERR_PTR(ret);
> @@ -448,6 +449,7 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
>  		hdr->dout_resid = rq->data_len;
>  		hdr->din_resid = rq->next_rq->data_len;
>  		blk_rq_unmap_user(bidi_bio);
> +		rq->next_rq->bio = NULL;

These should have been NULL here, NO? otherwise they are a leak. Setting to NULL
will hide the leak.

>  		blk_put_request(rq->next_rq);
>  	} else if (rq_data_dir(rq) == READ)
>  		hdr->din_resid = rq->data_len;
> @@ -466,6 +468,7 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
>  	blk_rq_unmap_user(bio);
>  	if (rq->cmd != rq->__cmd)
>  		kfree(rq->cmd);
> +	rq->bio = NULL;

Same here

>  	blk_put_request(rq);
>  
>  	return ret;

Boaz

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[FUJITA Tomonori]

On Tue, 09 Jun 2009 16:32:15 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 06/09/2009 04:10 PM, FUJITA Tomonori wrote:
> > On Tue, 09 Jun 2009 14:53:51 +0300
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> >> Please do not revert. This is the point of all this.
> >>
> >> If there is no leak, You should NULL out the req->bio
> >> for now, and for 2.6.31 change the code to do 
> >> blk_end_request_all(). That's what blk_end_request does,
> >> since you are doing your own completion then set req->bio
> >> to null after you're done. (And before put_request)
> >>
> >> This stuff is good for error paths to catch leaks, please
> >> leave it?
> > 
> > Has this your good stuff found any bio leak bugs in mainline? In
> > addition, breaking working code is not a proper development style.
> > 
> 
> It has found for me in error paths. That's why I put it.
>
> I Issue bsg bidi commands every day all day, and never seen this.
> What driver are you using? and can you post the stack trace.

See the original mail. I already said, "BSG SMP requests get the
following warnings". I use mptsas however all BSG SMP users hit this
bug. The stack trace is not useful because the bsg users don't call
blk_put_request directly.

But If you want to see:

Call Trace:
 [<ffffffff80320349>] ? __blk_put_request+0x52/0xc0
 [<ffffffff8022fd26>] warn_slowpath_common+0x77/0xa4
 [<ffffffff8022fd62>] warn_slowpath_null+0xf/0x11
 [<ffffffff80320349>] __blk_put_request+0x52/0xc0
 [<ffffffff803206d6>] ? blk_put_request+0x20/0x46
 [<ffffffff803206e4>] blk_put_request+0x2e/0x46
 [<ffffffff80327f64>] blk_complete_sgv4_hdr_rq+0x1a8/0x1b7
 [<ffffffff80328a36>] bsg_ioctl+0x1b4/0x1eb
 [<ffffffff8032dfea>] ? __up_read+0x1c/0x9a
 [<ffffffff804aab20>] ? _spin_unlock_irqrestore+0x3f/0x47
 [<ffffffff802a0f10>] vfs_ioctl+0x2a/0x78
 [<ffffffff802a13cc>] do_vfs_ioctl+0x46e/0x4aa
 [<ffffffff80246384>] ? up_read+0x26/0x2b
 [<ffffffff8020b8e9>] ? retint_swapgs+0xe/0x13
 [<ffffffff802a144a>] sys_ioctl+0x42/0x65
 [<ffffffff8020aeab>] system_call_fastpath+0x16/0x1b

> The driver does something wrong. bsg over scsi-ml does not have this
> problem. Why?

Because scsi-ml calls blk_end_request() but BSG SMP users don't.


> > Anyway, setting req->bio in bsg works. Either is fine by me.
> > 
> > 
> > Jens, can you please send either patch to Linus now?
> > 
> > =
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Subject: [PATCH] bsg: setting rq->bio to NULL
> > 
> > Due to commit 1cd96c242a829d52f7a5ae98f554ca9775429685 ("block: WARN
> > in __blk_put_request() for potential bio leak"), BSG SMP requests get
> > the false warnings:
> > 
> > WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()
> > 
> > This sets rq->bio to NULL to avoid that false warnings.
> > 
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  block/bsg.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/block/bsg.c b/block/bsg.c
> > index 206060e..dd81be4 100644
> > --- a/block/bsg.c
> > +++ b/block/bsg.c
> > @@ -315,6 +315,7 @@ out:
> >  	blk_put_request(rq);
> >  	if (next_rq) {
> >  		blk_rq_unmap_user(next_rq->bio);
> 
> I do not understand this here please explain? We have called blk_rq_map_user()
> and have bailed out on error later, without calling blk_execute_rq*. Now usually
> the bios are *double* referenced, one for the usual call of blk_end_request() that will
> release bios once, and second for the blk_rq_unmap_user() that will release second time.
> But here you only call blk_rq_unmap_user() don't you need to call blk_end_request() also?

If I understand correctly, blk_end_request() doesn't release bios of a
request that blk_rq_map_user was called against.

You can test this without any SAS hardware. Let me know when you find
a bio leak here.

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[Boaz Harrosh]

On 06/10/2009 02:00 AM, FUJITA Tomonori wrote:
> On Tue, 09 Jun 2009 16:32:15 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> On 06/09/2009 04:10 PM, FUJITA Tomonori wrote:
>>> On Tue, 09 Jun 2009 14:53:51 +0300
>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>
>>>> Please do not revert. This is the point of all this.
>>>>
>>>> If there is no leak, You should NULL out the req->bio
>>>> for now, and for 2.6.31 change the code to do 
>>>> blk_end_request_all(). That's what blk_end_request does,
>>>> since you are doing your own completion then set req->bio
>>>> to null after you're done. (And before put_request)
>>>>
>>>> This stuff is good for error paths to catch leaks, please
>>>> leave it?
>>> Has this your good stuff found any bio leak bugs in mainline? In
>>> addition, breaking working code is not a proper development style.
>>>
>> It has found for me in error paths. That's why I put it.
>>
>> I Issue bsg bidi commands every day all day, and never seen this.
>> What driver are you using? and can you post the stack trace.
> 
> See the original mail. I already said, "BSG SMP requests get the
> following warnings". I use mptsas however all BSG SMP users hit this
> bug. The stack trace is not useful because the bsg users don't call
> blk_put_request directly.
> 
> But If you want to see:
> 
> Call Trace:
>  [<ffffffff80320349>] ? __blk_put_request+0x52/0xc0
>  [<ffffffff8022fd26>] warn_slowpath_common+0x77/0xa4
>  [<ffffffff8022fd62>] warn_slowpath_null+0xf/0x11
>  [<ffffffff80320349>] __blk_put_request+0x52/0xc0
>  [<ffffffff803206d6>] ? blk_put_request+0x20/0x46
>  [<ffffffff803206e4>] blk_put_request+0x2e/0x46
>  [<ffffffff80327f64>] blk_complete_sgv4_hdr_rq+0x1a8/0x1b7
>  [<ffffffff80328a36>] bsg_ioctl+0x1b4/0x1eb
>  [<ffffffff8032dfea>] ? __up_read+0x1c/0x9a
>  [<ffffffff804aab20>] ? _spin_unlock_irqrestore+0x3f/0x47
>  [<ffffffff802a0f10>] vfs_ioctl+0x2a/0x78
>  [<ffffffff802a13cc>] do_vfs_ioctl+0x46e/0x4aa
>  [<ffffffff80246384>] ? up_read+0x26/0x2b
>  [<ffffffff8020b8e9>] ? retint_swapgs+0xe/0x13
>  [<ffffffff802a144a>] sys_ioctl+0x42/0x65
>  [<ffffffff8020aeab>] system_call_fastpath+0x16/0x1b
> 
>> The driver does something wrong. bsg over scsi-ml does not have this
>> problem. Why?
> 
> Because scsi-ml calls blk_end_request() but BSG SMP users don't.
> 
> 

That is a violation of block API. All block drivers must call
blk_end_request(). If they do not, then they can not for example be
called from inside Kernel. They relay on special bsg behavior
that always uses map_user.

This must be fixed. So you see that WARN_ON was right on the spot ;)

>>> Anyway, setting req->bio in bsg works. Either is fine by me.
>>>
>>>
>>> Jens, can you please send either patch to Linus now?
>>>
>>> =
>>> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>> Subject: [PATCH] bsg: setting rq->bio to NULL
>>>
>>> Due to commit 1cd96c242a829d52f7a5ae98f554ca9775429685 ("block: WARN
>>> in __blk_put_request() for potential bio leak"), BSG SMP requests get
>>> the false warnings:
>>>
>>> WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()
>>>
>>> This sets rq->bio to NULL to avoid that false warnings.
>>>
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>> ---
>>>  block/bsg.c |    3 +++
>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/block/bsg.c b/block/bsg.c
>>> index 206060e..dd81be4 100644
>>> --- a/block/bsg.c
>>> +++ b/block/bsg.c
>>> @@ -315,6 +315,7 @@ out:
>>>  	blk_put_request(rq);
>>>  	if (next_rq) {
>>>  		blk_rq_unmap_user(next_rq->bio);
>> I do not understand this here please explain? We have called blk_rq_map_user()
>> and have bailed out on error later, without calling blk_execute_rq*. Now usually
>> the bios are *double* referenced, one for the usual call of blk_end_request() that will
>> release bios once, and second for the blk_rq_unmap_user() that will release second time.
>> But here you only call blk_rq_unmap_user() don't you need to call blk_end_request() also?
> 
> If I understand correctly, blk_end_request() doesn't release bios of a
> request that blk_rq_map_user was called against.
> 

This is the part I do not understand. The comment in map_user says that the bio
ref count is taken twice, so when reference drops once at  blk_end_request()
the bio is not yet freed, but will so in unmap_user. But if blk_end_request()
is not called then there is no double drop of reference. I do want to understand,
how is the bio gets freed, is it forced some how?

> You can test this without any SAS hardware. Let me know when you find
> a bio leak here.

How can I test this without SAS hardware? Please point me to documentation.

I do believe you that there is no leak, I just want to understand why?
And any way, all "BSG SMP" drivers must be fixed to call blk_end_request().
They can not be dependent on the specific ULD that calls them.

Thanks
Boaz

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[FUJITA Tomonori]

On Wed, 10 Jun 2009 11:15:10 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 06/10/2009 02:00 AM, FUJITA Tomonori wrote:
> > On Tue, 09 Jun 2009 16:32:15 +0300
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> >> On 06/09/2009 04:10 PM, FUJITA Tomonori wrote:
> >>> On Tue, 09 Jun 2009 14:53:51 +0300
> >>> Boaz Harrosh <bharrosh@panasas.com> wrote:
> >>>
> >>>> Please do not revert. This is the point of all this.
> >>>>
> >>>> If there is no leak, You should NULL out the req->bio
> >>>> for now, and for 2.6.31 change the code to do 
> >>>> blk_end_request_all(). That's what blk_end_request does,
> >>>> since you are doing your own completion then set req->bio
> >>>> to null after you're done. (And before put_request)
> >>>>
> >>>> This stuff is good for error paths to catch leaks, please
> >>>> leave it?
> >>> Has this your good stuff found any bio leak bugs in mainline? In
> >>> addition, breaking working code is not a proper development style.
> >>>
> >> It has found for me in error paths. That's why I put it.
> >>
> >> I Issue bsg bidi commands every day all day, and never seen this.
> >> What driver are you using? and can you post the stack trace.
> > 
> > See the original mail. I already said, "BSG SMP requests get the
> > following warnings". I use mptsas however all BSG SMP users hit this
> > bug. The stack trace is not useful because the bsg users don't call
> > blk_put_request directly.
> > 
> > But If you want to see:
> > 
> > Call Trace:
> >  [<ffffffff80320349>] ? __blk_put_request+0x52/0xc0
> >  [<ffffffff8022fd26>] warn_slowpath_common+0x77/0xa4
> >  [<ffffffff8022fd62>] warn_slowpath_null+0xf/0x11
> >  [<ffffffff80320349>] __blk_put_request+0x52/0xc0
> >  [<ffffffff803206d6>] ? blk_put_request+0x20/0x46
> >  [<ffffffff803206e4>] blk_put_request+0x2e/0x46
> >  [<ffffffff80327f64>] blk_complete_sgv4_hdr_rq+0x1a8/0x1b7
> >  [<ffffffff80328a36>] bsg_ioctl+0x1b4/0x1eb
> >  [<ffffffff8032dfea>] ? __up_read+0x1c/0x9a
> >  [<ffffffff804aab20>] ? _spin_unlock_irqrestore+0x3f/0x47
> >  [<ffffffff802a0f10>] vfs_ioctl+0x2a/0x78
> >  [<ffffffff802a13cc>] do_vfs_ioctl+0x46e/0x4aa
> >  [<ffffffff80246384>] ? up_read+0x26/0x2b
> >  [<ffffffff8020b8e9>] ? retint_swapgs+0xe/0x13
> >  [<ffffffff802a144a>] sys_ioctl+0x42/0x65
> >  [<ffffffff8020aeab>] system_call_fastpath+0x16/0x1b
> > 
> >> The driver does something wrong. bsg over scsi-ml does not have this
> >> problem. Why?
> > 
> > Because scsi-ml calls blk_end_request() but BSG SMP users don't.
> > 
> > 
> 
> That is a violation of block API. All block drivers must call
> blk_end_request().

Where can I find a documentation about it?

If not, can you stop claiming a new rule?

If you want to propose a new rule, you need to fix the existing users
first.


> If they do not, then they can not for example be
> called from inside Kernel. They relay on special bsg behavior
> that always uses map_user.

Of course, we send SMP requests only via BSG. SMP is the first reason
why we invented BSG.


> This must be fixed. So you see that WARN_ON was right on the spot ;)
> 
> >>> Anyway, setting req->bio in bsg works. Either is fine by me.
> >>>
> >>>
> >>> Jens, can you please send either patch to Linus now?
> >>>
> >>> =
> >>> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> >>> Subject: [PATCH] bsg: setting rq->bio to NULL
> >>>
> >>> Due to commit 1cd96c242a829d52f7a5ae98f554ca9775429685 ("block: WARN
> >>> in __blk_put_request() for potential bio leak"), BSG SMP requests get
> >>> the false warnings:
> >>>
> >>> WARNING: at block/blk-core.c:1068 __blk_put_request+0x52/0xc0()
> >>>
> >>> This sets rq->bio to NULL to avoid that false warnings.
> >>>
> >>>
> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> >>> ---
> >>>  block/bsg.c |    3 +++
> >>>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/block/bsg.c b/block/bsg.c
> >>> index 206060e..dd81be4 100644
> >>> --- a/block/bsg.c
> >>> +++ b/block/bsg.c
> >>> @@ -315,6 +315,7 @@ out:
> >>>  	blk_put_request(rq);
> >>>  	if (next_rq) {
> >>>  		blk_rq_unmap_user(next_rq->bio);
> >> I do not understand this here please explain? We have called blk_rq_map_user()
> >> and have bailed out on error later, without calling blk_execute_rq*. Now usually
> >> the bios are *double* referenced, one for the usual call of blk_end_request() that will
> >> release bios once, and second for the blk_rq_unmap_user() that will release second time.
> >> But here you only call blk_rq_unmap_user() don't you need to call blk_end_request() also?
> > 
> > If I understand correctly, blk_end_request() doesn't release bios of a
> > request that blk_rq_map_user was called against.
> > 
> 
> This is the part I do not understand. The comment in map_user says that the bio
> ref count is taken twice, so when reference drops once at  blk_end_request()
> the bio is not yet freed, but will so in unmap_user. But if blk_end_request()
> is not called then there is no double drop of reference. I do want to understand,
> how is the bio gets freed, is it forced some how?

Read the code. I think that you totally misunderstand these functions.


> > You can test this without any SAS hardware. Let me know when you find
> > a bio leak here.
> 
> How can I test this without SAS hardware? Please point me to documentation.

You don't need SAS hardware to play with blk_rq_map_user() and
blk_rq_unmap_user(). All you need to do is writing a module that play
with these functions.


> I do believe you that there is no leak, I just want to understand why?
> And any way, all "BSG SMP" drivers must be fixed to call blk_end_request().
> They can not be dependent on the specific ULD that calls them.

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[Boaz Harrosh]

On 06/10/2009 11:29 AM, FUJITA Tomonori wrote:
> 
> Read the code. I think that you totally misunderstand these functions.
> 

Thanks for the detail explanation. This really help me a lot

Boaz

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[Boaz Harrosh]

On 06/10/2009 11:29 AM, FUJITA Tomonori wrote:
> On Wed, 10 Jun 2009 11:15:10 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
>> That is a violation of block API. All block drivers must call
>> blk_end_request().
> 
> Where can I find a documentation about it?
> 

No documentation

> If not, can you stop claiming a new rule?
> 

The fact that ALL block drivers call it for proper service of ALL ULDS,
does it not make it a rule?

> If you want to propose a new rule, you need to fix the existing users
> first.
> 

Sorry I missed them. I couldn't even imagine that there will be any.
Will send patches, sorry.

> 
>> If they do not, then they can not for example be
>> called from inside Kernel. They relay on special bsg behavior
>> that always uses map_user.
> 
> Of course, we send SMP requests only via BSG. SMP is the first reason
> why we invented BSG.
> 

Of course what? Show me the big fat comment that says:
  only use from BSG!!! other wise broken block driver.

> 
<snip>
> 
> You don't need SAS hardware to play with blk_rq_map_user() and
> blk_rq_unmap_user(). All you need to do is writing a module that play
> with these functions.
> 
> 
>> I do believe you that there is no leak, I just want to understand why?
>> And any way, all "BSG SMP" drivers must be fixed to call blk_end_request().
>> They can not be dependent on the specific ULD that calls them.

Boaz

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[FUJITA Tomonori]

On Wed, 10 Jun 2009 11:45:14 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 06/10/2009 11:29 AM, FUJITA Tomonori wrote:
> > On Wed, 10 Jun 2009 11:15:10 +0300
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> >> That is a violation of block API. All block drivers must call
> >> blk_end_request().
> > 
> > Where can I find a documentation about it?
> > 
> 
> No documentation
> 
> > If not, can you stop claiming a new rule?
> > 
> 
> The fact that ALL block drivers call it for proper service of ALL ULDS,
> does it not make it a rule?

BSG SMP code is not ULD.


> > If you want to propose a new rule, you need to fix the existing users
> > first.
> > 
> 
> Sorry I missed them. I couldn't even imagine that there will be any.
> Will send patches, sorry.

I'll take care about it.


> >> If they do not, then they can not for example be
> >> called from inside Kernel. They relay on special bsg behavior
> >> that always uses map_user.
> > 
> > Of course, we send SMP requests only via BSG. SMP is the first reason
> > why we invented BSG.
> > 
> 
> Of course what? Show me the big fat comment that says:
>   only use from BSG!!! other wise broken block driver.

I think that the majority of SCSI people know that.

FYI, you live in a land where very few live.

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[Boaz Harrosh]

On 06/10/2009 11:52 AM, FUJITA Tomonori wrote:
> On Wed, 10 Jun 2009 11:45:14 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>>>
>> The fact that ALL block drivers call it for proper service of ALL ULDS,
>> does it not make it a rule?
> 
> BSG SMP code is not ULD.
> 

BSG is certainly a ULD, Upper-Layer-Driver that can issue block commands
through block layer and block-queue.

SMP is a block device class that has it's own LLDs.

The way I understand

|---------|      |--------------|      |-----------|
| ULD     |----->|  block layer |----->| block LLD |
|---------|      |--------------|      |-----------|

So I meant block ULD or block layer user code. That is what I meant
by ULD, what do you mean?

<snip>
> 
> FYI, you live in a land where very few live.
> 

I thought I was working on the Linux-Kernel. Is there another
place I should be?

Boaz

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[FUJITA Tomonori]

On Wed, 10 Jun 2009 12:21:19 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 06/10/2009 11:52 AM, FUJITA Tomonori wrote:
> > On Wed, 10 Jun 2009 11:45:14 +0300
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> >>>
> >> The fact that ALL block drivers call it for proper service of ALL ULDS,
> >> does it not make it a rule?
> > 
> > BSG SMP code is not ULD.
> > 
> 
> BSG is certainly a ULD, Upper-Layer-Driver that can issue block commands
> through block layer and block-queue.
> 
> SMP is a block device class that has it's own LLDs.
> 
> The way I understand
> 
> |---------|      |--------------|      |-----------|
> | ULD     |----->|  block layer |----->| block LLD |
> |---------|      |--------------|      |-----------|
> 
> So I meant block ULD or block layer user code. That is what I meant
> by ULD, what do you mean?

To me, ULD means SCSI upper layer driver, which is documented.

Where can I find a document about your definition of Block ULD, block
LLD, etc?


> <snip>
> > 
> > FYI, you live in a land where very few live.
> > 
> 
> I thought I was working on the Linux-Kernel. Is there another
> place I should be?

To me, looks like there is a different place for you.

BTW, if you try to find a bio leak in the BSG SMP path, I guess that
it's worth looking at the bounce path. There might be a leak.

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[Boaz Harrosh]

On 06/10/2009 12:36 PM, FUJITA Tomonori wrote:
> On Wed, 10 Jun 2009 12:21:19 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> On 06/10/2009 11:52 AM, FUJITA Tomonori wrote:
>>> On Wed, 10 Jun 2009 11:45:14 +0300
>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>
>>>> The fact that ALL block drivers call it for proper service of ALL ULDS,
>>>> does it not make it a rule?
>>> BSG SMP code is not ULD.
>>>
>> BSG is certainly a ULD, Upper-Layer-Driver that can issue block commands
>> through block layer and block-queue.
>>
>> SMP is a block device class that has it's own LLDs.
>>
>> The way I understand
>>
>> |---------|      |--------------|      |-----------|
>> | ULD     |----->|  block layer |----->| block LLD |
>> |---------|      |--------------|      |-----------|
>>
>> So I meant block ULD or block layer user code. That is what I meant
>> by ULD, what do you mean?
> 
> To me, ULD means SCSI upper layer driver, which is documented.
> 
> Where can I find a document about your definition of Block ULD, block
> LLD, etc?
> 
> 
> To me, looks like there is a different place for you.
> 

Sorry then, My bad. I though ULD/LLD is just Upper/Lower drivers of any
sub-system.
If it is only used for SCSI, then sorry I will not use it for any thing else.

> BTW, if you try to find a bio leak in the BSG SMP path, I guess that
> it's worth looking at the bounce path. There might be a leak.

once a request was put on the queue, or even before that, once some
memory was mapped. blk_end_request must be called by someone.
More and more code relays on that. Not doing so is just plain playing
with fire. Will you not agree?

Boaz

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[FUJITA Tomonori]

On Wed, 10 Jun 2009 12:48:01 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> > BTW, if you try to find a bio leak in the BSG SMP path, I guess that
> > it's worth looking at the bounce path. There might be a leak.
> 
> once a request was put on the queue, or even before that, once some
> memory was mapped. blk_end_request must be called by someone.
> More and more code relays on that. Not doing so is just plain playing
> with fire. Will you not agree?

As I said, I'll fix SMP BSG path to call blk_end_request_all().

IMO, the block layer API is vague before Tejun's recent
cleanups. Seems that everyone uses it (and its internal) in their own
ways. But now we should use it in the consistent way.

Re: [PATCH] Revert "block: WARN in __blk_put_request() for potential bio leak"

[FUJITA Tomonori]

On Wed, 10 Jun 2009 11:34:28 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 06/10/2009 11:29 AM, FUJITA Tomonori wrote:
> > 
> > Read the code. I think that you totally misunderstand these functions.
> > 
> 
> Thanks for the detail explanation. This really help me a lot

I already explained before this mail (though I was wrong wrt the
bounce path, I think), see the forth and fifth lines from the bottom:

http://marc.info/?l=linux-scsi&m=124458845603617&w=2

You just repeated the same question without reading my mail or the
code. I don't know how to help such people.