On Tue, 09 Jun 2009 16:32:15 +0300
Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
> On 06/09/2009 04:10 PM, FUJITA Tomonori wrote:
> > On Tue, 09 Jun 2009 14:53:51 +0300
> > Boaz Harrosh <bharrosh@xxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
> > ---
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
