[PATCHSET 0/2] Don't let blk_put_request leak BIOs

[PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Boaz Harrosh]

Hi Jens

You never commented on these patches. Please have a look?

The issue is that if we map some memory into a request but then
do not execute it. Then calling blk_put_request() will leak the bio(s)
unless one does an ugly code like:
-	struct bio *bio;
-
-	while ((bio = rq->bio) != NULL) {
-		rq->bio = bio->bi_next;
-		bio_endio(bio, 0);
-	}

This problem arise in OSD when we can fail to setup the write
or the read side and then we must cleanup the other half.
Same problem exist in bsg, on bidi commands. But there the bio
is just leaked on the error path, it does not do the ugly loop above.

My proposed solution is that blk_put_request() should see if
there is a left-over bio and do the deallocation of the bio's

A side effect of this is if before, do to some bugs, drivers failed
to complete the request fully, the bio would leak. Now it will not
any more.

All above is the theory, in practice some code was abusing the use
of request->bio and it needed fixing. I tried to audit all code path
that call blk_put_request() and fix them.

But it is still dangerous and we should run with this patch in
linux-next and the block tree and see if the WARN_ON at patch [PATCH 1/2]
does not trigger. I have marked this patch as TESTING. The final patch
submitted to Linus should be Minus this WARN_ON.

James
I'm also sending the osd patch through Jens's tree. So there will not
be ordering dependency problems from two trees. Is that OK? do I need
your ACK-BY?

The patches are:
[PATCH 1/2] TESTING: Don't let blk_put_request leak BIOs
    This is for linux-next. Before final submission to Linus a patch
    minus the WARN_ON (and TESTING:) should be submitted.

[PATCH 2/2] libosd: Don't let osd abuse block internals, now that it's fixed
    This can go through Jens tree, I will make sure it will not conflict
    with any scsi-misc patches.

Thank you in advance
Boaz

[PATCH 1/2] TESTING: Don't let blk_put_request leak BIOs

[Boaz Harrosh]

If a block ULD had allocated a request and mapped some memory into it,
but then for some reason failed to execute the request through one of
the blk_execute_request_xxx routines. Then the associated bio would leak,
unless ULD resorts to low-level loops intimate of block internals.

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

[TESTING]
  This code will also catch situations where LLD failed to complete
  the request before aborting it. Such situations are a BUG. Should we
  use WARN_ON_ONCE() in that case. The situation above is possible and
  can happen normally in memory pressure situations so maybe we should
  devise a bit-flag that ULD denotes that the request was aborted and
  only WARN_ON if flag was not set.
  For the duration of linux-next I'm leaving the WARN_ON to catch any
  problems like found above, and possible memory leaks. Before submission
  a complimentary patch should remove the WARN_ON. (Or this patch can be
  rebased)

  Please comment on possible pitfalls.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/blk-core.c   |   17 +++++++++++++++++
 block/blk-merge.c  |    2 ++
 block/scsi_ioctl.c |   21 ++++-----------------
 3 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 29bcfac..9ee243e 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1055,6 +1055,22 @@ void part_round_stats(int cpu, struct hd_struct *part)
 EXPORT_SYMBOL_GPL(part_round_stats);
 
 /*
+ * If one of the blk_rq_map_xxx() was called but the request was not
+ * executed by the block layer, then we must release BIOs. Otherwise they
+ * will leak.
+ */
+static void _abort_unexecuted_bios(struct request *req)
+{
+	struct bio *bio;
+
+	WARN_ON(req->bio != NULL);
+	while (unlikely((bio = req->bio) != NULL)) {
+		req->bio = bio->bi_next;
+		bio_endio(bio, 0);
+	}
+}
+
+/*
  * queue lock must be held
  */
 void __blk_put_request(struct request_queue *q, struct request *req)
@@ -1066,6 +1082,7 @@ void __blk_put_request(struct request_queue *q, struct request *req)
 
 	elv_completed_request(q, req);
 
+	_abort_unexecuted_bios(req);
 	/*
 	 * Request may not have originated from ll_rw_blk. if not,
 	 * it didn't come out of our reserved rq pools
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 5a244f0..e39cb24 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -403,6 +403,8 @@ static int attempt_merge(struct request_queue *q, struct request *req,
 	if (blk_rq_cpu_valid(next))
 		req->cpu = next->cpu;
 
+	/* owner-ship of bio passed from next to req */
+	next->bio = NULL;
 	__blk_put_request(q, next);
 	return 1;
 }
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index ee9c67d..626ee27 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -214,21 +214,10 @@ static int blk_fill_sghdr_rq(struct request_queue *q, struct request *rq,
 	return 0;
 }
 
-/*
- * unmap a request that was previously mapped to this sg_io_hdr. handles
- * both sg and non-sg sg_io_hdr.
- */
-static int blk_unmap_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr)
-{
-	blk_rq_unmap_user(rq->bio);
-	blk_put_request(rq);
-	return 0;
-}
-
 static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 				 struct bio *bio)
 {
-	int r, ret = 0;
+	int ret = 0;
 
 	/*
 	 * fill in all the output members
@@ -253,12 +242,10 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 			ret = -EFAULT;
 	}
 
-	rq->bio = bio;
-	r = blk_unmap_sghdr_rq(rq, hdr);
-	if (ret)
-		r = ret;
+	blk_rq_unmap_user(bio);
+	blk_put_request(rq);
 
-	return r;
+	return ret;
 }
 
 static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
-- 
1.6.2.1

[PATCH 2/2] libosd: Don't let osd abuse block internals, now that it's fixed

[Boaz Harrosh]

blk_put_request will delete any BIOs that where mapped but not
executed, so osd_initiator does not have to take care of this situation
any more.

This patch is dependent on block patch titled:

  [BLOCK] Don't let blk_put_request leak BIOs

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |   17 +----------------
 1 files changed, 1 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 45c154f..6c921ce 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -339,20 +339,6 @@ struct osd_request *osd_start_request(struct osd_dev *dev, gfp_t gfp)
 }
 EXPORT_SYMBOL(osd_start_request);
 
-/*
- * If osd_finalize_request() was called but the request was not executed through
- * the block layer, then we must release BIOs.
- */
-static void _abort_unexecuted_bios(struct request *rq)
-{
-	struct bio *bio;
-
-	while ((bio = rq->bio) != NULL) {
-		rq->bio = bio->bi_next;
-		bio_endio(bio, 0);
-	}
-}
-
 static void _osd_free_seg(struct osd_request *or __unused,
 	struct _osd_req_data_segment *seg)
 {
@@ -374,11 +360,10 @@ void osd_end_request(struct osd_request *or)
 
 	if (rq) {
 		if (rq->next_rq) {
-			_abort_unexecuted_bios(rq->next_rq);
 			blk_put_request(rq->next_rq);
+			rq->next_rq = NULL;
 		}
 
-		_abort_unexecuted_bios(rq);
 		blk_put_request(rq);
 	}
 	_osd_request_free(or);
-- 
1.6.2.1

Re: [PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Jens Axboe]

On Thu, Mar 19 2009, Boaz Harrosh wrote:
> Hi Jens
> 
> You never commented on these patches. Please have a look?
> 
> The issue is that if we map some memory into a request but then
> do not execute it. Then calling blk_put_request() will leak the bio(s)
> unless one does an ugly code like:
> -	struct bio *bio;
> -
> -	while ((bio = rq->bio) != NULL) {
> -		rq->bio = bio->bi_next;
> -		bio_endio(bio, 0);
> -	}

Sorry, I think this is a horrible design. blk_put_request() doesn't care
about any data attachments, in fact (if possible) it should go BUG() if
the request hasn't been completed in some way or other. It deals with
the deallocation part, blk_get_request() doesn't attach any data. The
end result with code like the above is an assymmetric API.

> This problem arise in OSD when we can fail to setup the write
> or the read side and then we must cleanup the other half.
> Same problem exist in bsg, on bidi commands. But there the bio
> is just leaked on the error path, it does not do the ugly loop above.

These drivers should just be fixed, then.

So if you have spotted this problem in eg bsg, then please send a patch
for that to Tomo!

-- 
Jens Axboe

Re: [PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Boaz Harrosh]

Jens Axboe wrote:
> On Thu, Mar 19 2009, Boaz Harrosh wrote:
>> Hi Jens
>>
>> You never commented on these patches. Please have a look?
>>
>> The issue is that if we map some memory into a request but then
>> do not execute it. Then calling blk_put_request() will leak the bio(s)
>> unless one does an ugly code like:
>> -	struct bio *bio;
>> -
>> -	while ((bio = rq->bio) != NULL) {
>> -		rq->bio = bio->bi_next;
>> -		bio_endio(bio, 0);
>> -	}

This is the code I have today in osd_initiator.c (In scsi-misc tree)

> 
> Sorry, I think this is a horrible design. blk_put_request() doesn't care
> about any data attachments, in fact (if possible) it should go BUG() if
> the request hasn't been completed in some way or other. It deals with
> the deallocation part, blk_get_request() doesn't attach any data. The
> end result with code like the above is an assymmetric API.
> 
>> This problem arise in OSD when we can fail to setup the write
>> or the read side and then we must cleanup the other half.
>> Same problem exist in bsg, on bidi commands. But there the bio
>> is just leaked on the error path, it does not do the ugly loop above.
> 
> These drivers should just be fixed, then.
> 
> So if you have spotted this problem in eg bsg, then please send a patch
> for that to Tomo!
> 

Then the above code taken from today's osd_initiator is good? People where
complaining that it is not good, because it is messing up with internal
block structures.

What about just exporting an blk_rq_abort(struct request *req) that is
the same as PATCH 1/2. But is not called from blk_put_request ?

Boaz

Re: [PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Jens Axboe]

On Thu, Mar 19 2009, Boaz Harrosh wrote:
> Jens Axboe wrote:
> > On Thu, Mar 19 2009, Boaz Harrosh wrote:
> >> Hi Jens
> >>
> >> You never commented on these patches. Please have a look?
> >>
> >> The issue is that if we map some memory into a request but then
> >> do not execute it. Then calling blk_put_request() will leak the bio(s)
> >> unless one does an ugly code like:
> >> -	struct bio *bio;
> >> -
> >> -	while ((bio = rq->bio) != NULL) {
> >> -		rq->bio = bio->bi_next;
> >> -		bio_endio(bio, 0);
> >> -	}
> 
> This is the code I have today in osd_initiator.c (In scsi-misc tree)
> 
> > 
> > Sorry, I think this is a horrible design. blk_put_request() doesn't care
> > about any data attachments, in fact (if possible) it should go BUG() if
> > the request hasn't been completed in some way or other. It deals with
> > the deallocation part, blk_get_request() doesn't attach any data. The
> > end result with code like the above is an assymmetric API.
> > 
> >> This problem arise in OSD when we can fail to setup the write
> >> or the read side and then we must cleanup the other half.
> >> Same problem exist in bsg, on bidi commands. But there the bio
> >> is just leaked on the error path, it does not do the ugly loop above.
> > 
> > These drivers should just be fixed, then.
> > 
> > So if you have spotted this problem in eg bsg, then please send a patch
> > for that to Tomo!
> > 
> 
> Then the above code taken from today's osd_initiator is good? People where
> complaining that it is not good, because it is messing up with internal
> block structures.
> 
> What about just exporting an blk_rq_abort(struct request *req) that is
> the same as PATCH 1/2. But is not called from blk_put_request ?

Well no, the approach isn't that good either. How did you map these
request? Most logical API would have something to unmap them again.

-- 
Jens Axboe

Re: [PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Boaz Harrosh]

Jens Axboe wrote:
> On Thu, Mar 19 2009, Boaz Harrosh wrote:
>> Jens Axboe wrote:
>>> On Thu, Mar 19 2009, Boaz Harrosh wrote:
>>>> Hi Jens
>>>>
>>>> You never commented on these patches. Please have a look?
>>>>
>>>> The issue is that if we map some memory into a request but then
>>>> do not execute it. Then calling blk_put_request() will leak the bio(s)
>>>> unless one does an ugly code like:
>>>> -	struct bio *bio;
>>>> -
>>>> -	while ((bio = rq->bio) != NULL) {
>>>> -		rq->bio = bio->bi_next;
>>>> -		bio_endio(bio, 0);
>>>> -	}
>> This is the code I have today in osd_initiator.c (In scsi-misc tree)
>>
>>> Sorry, I think this is a horrible design. blk_put_request() doesn't care
>>> about any data attachments, in fact (if possible) it should go BUG() if
>>> the request hasn't been completed in some way or other. It deals with
>>> the deallocation part, blk_get_request() doesn't attach any data. The
>>> end result with code like the above is an assymmetric API.
>>>
>>>> This problem arise in OSD when we can fail to setup the write
>>>> or the read side and then we must cleanup the other half.
>>>> Same problem exist in bsg, on bidi commands. But there the bio
>>>> is just leaked on the error path, it does not do the ugly loop above.
>>> These drivers should just be fixed, then.
>>>
>>> So if you have spotted this problem in eg bsg, then please send a patch
>>> for that to Tomo!
>>>
>> Then the above code taken from today's osd_initiator is good? People where
>> complaining that it is not good, because it is messing up with internal
>> block structures.
>>
>> What about just exporting an blk_rq_abort(struct request *req) that is
>> the same as PATCH 1/2. But is not called from blk_put_request ?
> 
> Well no, the approach isn't that good either. How did you map these
> request? Most logical API would have something to unmap them again.
> 

I'm not sure I understand.
I use a combination of map functions to build a complex bio. But more
specifically I do not want to use any unmap function because I do not
want to call the end_bio() function I want to not allow any bouncing
unmapping or hooking to occur. "Abort" is the only write name for it.

Boaz

Re: [PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Jens Axboe]

On Thu, Mar 19 2009, Boaz Harrosh wrote:
> Jens Axboe wrote:
> > On Thu, Mar 19 2009, Boaz Harrosh wrote:
> >> Jens Axboe wrote:
> >>> On Thu, Mar 19 2009, Boaz Harrosh wrote:
> >>>> Hi Jens
> >>>>
> >>>> You never commented on these patches. Please have a look?
> >>>>
> >>>> The issue is that if we map some memory into a request but then
> >>>> do not execute it. Then calling blk_put_request() will leak the bio(s)
> >>>> unless one does an ugly code like:
> >>>> -	struct bio *bio;
> >>>> -
> >>>> -	while ((bio = rq->bio) != NULL) {
> >>>> -		rq->bio = bio->bi_next;
> >>>> -		bio_endio(bio, 0);
> >>>> -	}
> >> This is the code I have today in osd_initiator.c (In scsi-misc tree)
> >>
> >>> Sorry, I think this is a horrible design. blk_put_request() doesn't care
> >>> about any data attachments, in fact (if possible) it should go BUG() if
> >>> the request hasn't been completed in some way or other. It deals with
> >>> the deallocation part, blk_get_request() doesn't attach any data. The
> >>> end result with code like the above is an assymmetric API.
> >>>
> >>>> This problem arise in OSD when we can fail to setup the write
> >>>> or the read side and then we must cleanup the other half.
> >>>> Same problem exist in bsg, on bidi commands. But there the bio
> >>>> is just leaked on the error path, it does not do the ugly loop above.
> >>> These drivers should just be fixed, then.
> >>>
> >>> So if you have spotted this problem in eg bsg, then please send a patch
> >>> for that to Tomo!
> >>>
> >> Then the above code taken from today's osd_initiator is good? People where
> >> complaining that it is not good, because it is messing up with internal
> >> block structures.
> >>
> >> What about just exporting an blk_rq_abort(struct request *req) that is
> >> the same as PATCH 1/2. But is not called from blk_put_request ?
> > 
> > Well no, the approach isn't that good either. How did you map these
> > request? Most logical API would have something to unmap them again.
> > 
> 
> I'm not sure I understand.
> I use a combination of map functions to build a complex bio. But more
> specifically I do not want to use any unmap function because I do not
> want to call the end_bio() function I want to not allow any bouncing
> unmapping or hooking to occur. "Abort" is the only write name for it.

I'm not sure I understand what you are saying here either. So if you
build these bios up manually, then you will need to tear them down
manually as well.

Doing panic style cleanup at blk_put_request() time is definitely not
the right approach. And you can't just export a blk_rq_abort() type
functionality, since you don't don't know what you are trying to abort.
You assume that bio_endio() will always do the right thing, that may not
be enough. Hence you need the caller to cleanup after themselves.

-- 
Jens Axboe

Re: [PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Boaz Harrosh]

Jens Axboe wrote:
> On Thu, Mar 19 2009, Boaz Harrosh wrote:
>> Jens Axboe wrote:
>>> On Thu, Mar 19 2009, Boaz Harrosh wrote:
>>>> Jens Axboe wrote:
>>>>> On Thu, Mar 19 2009, Boaz Harrosh wrote:
>>>>>> Hi Jens
>>>>>>
>>>>>> You never commented on these patches. Please have a look?
>>>>>>
>>>>>> The issue is that if we map some memory into a request but then
>>>>>> do not execute it. Then calling blk_put_request() will leak the bio(s)
>>>>>> unless one does an ugly code like:
>>>>>> -	struct bio *bio;
>>>>>> -
>>>>>> -	while ((bio = rq->bio) != NULL) {
>>>>>> -		rq->bio = bio->bi_next;
>>>>>> -		bio_endio(bio, 0);
>>>>>> -	}
>>>> This is the code I have today in osd_initiator.c (In scsi-misc tree)
>>>>
>>>>> Sorry, I think this is a horrible design. blk_put_request() doesn't care
>>>>> about any data attachments, in fact (if possible) it should go BUG() if
>>>>> the request hasn't been completed in some way or other. It deals with
>>>>> the deallocation part, blk_get_request() doesn't attach any data. The
>>>>> end result with code like the above is an assymmetric API.
>>>>>
>>>>>> This problem arise in OSD when we can fail to setup the write
>>>>>> or the read side and then we must cleanup the other half.
>>>>>> Same problem exist in bsg, on bidi commands. But there the bio
>>>>>> is just leaked on the error path, it does not do the ugly loop above.
>>>>> These drivers should just be fixed, then.
>>>>>
>>>>> So if you have spotted this problem in eg bsg, then please send a patch
>>>>> for that to Tomo!
>>>>>
>>>> Then the above code taken from today's osd_initiator is good? People where
>>>> complaining that it is not good, because it is messing up with internal
>>>> block structures.
>>>>
>>>> What about just exporting an blk_rq_abort(struct request *req) that is
>>>> the same as PATCH 1/2. But is not called from blk_put_request ?
>>> Well no, the approach isn't that good either. How did you map these
>>> request? Most logical API would have something to unmap them again.
>>>
>> I'm not sure I understand.
>> I use a combination of map functions to build a complex bio. But more
>> specifically I do not want to use any unmap function because I do not
>> want to call the end_bio() function I want to not allow any bouncing
>> unmapping or hooking to occur. "Abort" is the only write name for it.
> 
> I'm not sure I understand what you are saying here either. So if you
> build these bios up manually, then you will need to tear them down
> manually as well.
> 
> Doing panic style cleanup at blk_put_request() time is definitely not
> the right approach. And you can't just export a blk_rq_abort() type
> functionality, since you don't don't know what you are trying to abort.
> You assume that bio_endio() will always do the right thing, that may not
> be enough. Hence you need the caller to cleanup after themselves.
> 

So current (above) code in osd_initiator.c is correct and should stay
as it is? that's fine by me.

Thanks for you consideration
Boaz

Re: [PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Jens Axboe]

On Thu, Mar 19 2009, Boaz Harrosh wrote:
> Jens Axboe wrote:
> > On Thu, Mar 19 2009, Boaz Harrosh wrote:
> >> Jens Axboe wrote:
> >>> On Thu, Mar 19 2009, Boaz Harrosh wrote:
> >>>> Jens Axboe wrote:
> >>>>> On Thu, Mar 19 2009, Boaz Harrosh wrote:
> >>>>>> Hi Jens
> >>>>>>
> >>>>>> You never commented on these patches. Please have a look?
> >>>>>>
> >>>>>> The issue is that if we map some memory into a request but then
> >>>>>> do not execute it. Then calling blk_put_request() will leak the bio(s)
> >>>>>> unless one does an ugly code like:
> >>>>>> -	struct bio *bio;
> >>>>>> -
> >>>>>> -	while ((bio = rq->bio) != NULL) {
> >>>>>> -		rq->bio = bio->bi_next;
> >>>>>> -		bio_endio(bio, 0);
> >>>>>> -	}
> >>>> This is the code I have today in osd_initiator.c (In scsi-misc tree)
> >>>>
> >>>>> Sorry, I think this is a horrible design. blk_put_request() doesn't care
> >>>>> about any data attachments, in fact (if possible) it should go BUG() if
> >>>>> the request hasn't been completed in some way or other. It deals with
> >>>>> the deallocation part, blk_get_request() doesn't attach any data. The
> >>>>> end result with code like the above is an assymmetric API.
> >>>>>
> >>>>>> This problem arise in OSD when we can fail to setup the write
> >>>>>> or the read side and then we must cleanup the other half.
> >>>>>> Same problem exist in bsg, on bidi commands. But there the bio
> >>>>>> is just leaked on the error path, it does not do the ugly loop above.
> >>>>> These drivers should just be fixed, then.
> >>>>>
> >>>>> So if you have spotted this problem in eg bsg, then please send a patch
> >>>>> for that to Tomo!
> >>>>>
> >>>> Then the above code taken from today's osd_initiator is good? People where
> >>>> complaining that it is not good, because it is messing up with internal
> >>>> block structures.
> >>>>
> >>>> What about just exporting an blk_rq_abort(struct request *req) that is
> >>>> the same as PATCH 1/2. But is not called from blk_put_request ?
> >>> Well no, the approach isn't that good either. How did you map these
> >>> request? Most logical API would have something to unmap them again.
> >>>
> >> I'm not sure I understand.
> >> I use a combination of map functions to build a complex bio. But more
> >> specifically I do not want to use any unmap function because I do not
> >> want to call the end_bio() function I want to not allow any bouncing
> >> unmapping or hooking to occur. "Abort" is the only write name for it.
> > 
> > I'm not sure I understand what you are saying here either. So if you
> > build these bios up manually, then you will need to tear them down
> > manually as well.
> > 
> > Doing panic style cleanup at blk_put_request() time is definitely not
> > the right approach. And you can't just export a blk_rq_abort() type
> > functionality, since you don't don't know what you are trying to abort.
> > You assume that bio_endio() will always do the right thing, that may not
> > be enough. Hence you need the caller to cleanup after themselves.
> > 
> 
> So current (above) code in osd_initiator.c is correct and should stay
> as it is? that's fine by me.

It's still not pretty, any reason you can't just use blk_end_request()?
Why do you need to unroll the bios manually?

-- 
Jens Axboe

Re: [PATCHSET 0/2] Don't let blk_put_request leak BIOs

[Boaz Harrosh]

Jens Axboe wrote:
>> So current (above) code in osd_initiator.c is correct and should stay
>> as it is? that's fine by me.
> 
> It's still not pretty, any reason you can't just use blk_end_request()?
> Why do you need to unroll the bios manually?
> 

I have stared at blk_end_request() multiple times, and I'm afraid to call it
it does to many things. accounting, end_that_request_last, lots of not needed
stuff. This is a request that was never on any submission Q, it is scary.
Also I'm not sure what is the request state it can be very partially built
at this stage.

But mainly I do not want that bio->bi_end_io(bio, error); will be called.
I don't want any read bouncing to happen, just return them to free store.

I will try the blk_end_request(,-EIO) and see if it works for all cases

Thanks
Boaz

[PATCH] WARN_ON if blk_put_request leaks BIOs

[Boaz Harrosh]

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.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/blk-core.c   |    3 +++
 block/blk-merge.c  |    2 ++
 block/scsi_ioctl.c |   21 ++++-----------------
 3 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index f9118c0..b3aabdd 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1031,6 +1031,9 @@ 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
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 5a244f0..e39cb24 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -403,6 +403,8 @@ static int attempt_merge(struct request_queue *q, struct request *req,
 	if (blk_rq_cpu_valid(next))
 		req->cpu = next->cpu;
 
+	/* owner-ship of bio passed from next to req */
+	next->bio = NULL;
 	__blk_put_request(q, next);
 	return 1;
 }
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index ee9c67d..626ee27 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -214,21 +214,10 @@ static int blk_fill_sghdr_rq(struct request_queue *q, struct request *rq,
 	return 0;
 }
 
-/*
- * unmap a request that was previously mapped to this sg_io_hdr. handles
- * both sg and non-sg sg_io_hdr.
- */
-static int blk_unmap_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr)
-{
-	blk_rq_unmap_user(rq->bio);
-	blk_put_request(rq);
-	return 0;
-}
-
 static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 				 struct bio *bio)
 {
-	int r, ret = 0;
+	int ret = 0;
 
 	/*
 	 * fill in all the output members
@@ -253,12 +242,10 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 			ret = -EFAULT;
 	}
 
-	rq->bio = bio;
-	r = blk_unmap_sghdr_rq(rq, hdr);
-	if (ret)
-		r = ret;
+	blk_rq_unmap_user(bio);
+	blk_put_request(rq);
 
-	return r;
+	return ret;
 }
 
 static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
-- 
1.6.2.1

Re: [PATCH] WARN_ON if blk_put_request leaks BIOs

[Jens Axboe]

On Thu, Mar 19 2009, Boaz Harrosh wrote:
> 
> 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.

Ho humm, not sure what to do about this. Honestly, in all the time that
I have been doing this, this would have found about zero bugs. And now
enforces a rule that ->bio must be cleared. Normal bio completion does
that automatically, so it's not a problem there, but it's still a new
rule just to satisfy this questionable debug mechanism.

But what the hell, it's simple enough. Kill the totally unrelated
scsi_ioctl.c change, and I'll toss it in for a spin.

-- 
Jens Axboe

Re: [PATCH] WARN_ON if blk_put_request leaks BIOs

[Boaz Harrosh]

Jens Axboe wrote:
> On Thu, Mar 19 2009, Boaz Harrosh wrote:
>> 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.
> 
> Ho humm, not sure what to do about this. Honestly, in all the time that
> I have been doing this, this would have found about zero bugs. And now
> enforces a rule that ->bio must be cleared. Normal bio completion does
> that automatically, so it's not a problem there, but it's still a new
> rule just to satisfy this questionable debug mechanism.
> 
> But what the hell, it's simple enough. Kill the totally unrelated
> scsi_ioctl.c change, and I'll toss it in for a spin.
> 

The scsi_ioctl.c is related. It sets req->bio back from some internal
held value just to carry it across a static function call, all the while
when the function is not needed, totally bogus in the error handling check
of an hard-coded 0, and a call sight that is bigger then the actual function.
So it was better to fix it then, go around the problem of the code putting
some private data on req->bio.

I thought the all patch is worth it just for that bit

Thanks
Boaz