Don't let blk_put_request leak BIOs
Subsystems (2)
cover Don't let blk_put_request leak BIOs
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
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.
--- block/blk-core.c | 17 +++++++++++++++++ block/blk-merge.c | 2 ++ block/scsi_ioctl.c | 21 ++++----------------- 3 files changed, 23 insertions(+), 17 deletions(-)
block/blk-core.c +17 -0
@@ -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
block/blk-merge.c +2 -0
@@ -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 ; }
block/scsi_ioctl.c +4 -17
@@ -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,
Signature · 1 line
1.6.2.1
PATCH 2/2 libosd: Don't let osd abuse block internals, now that it's fixed
In reply to Boaz Harroshblk_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
--- drivers/scsi/osd/osd_initiator.c | 17 +---------------- 1 files changed, 1 insertions(+), 16 deletions(-)
drivers/scsi/osd/osd_initiator.c +1 -16
@@ -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);
Signature · 1 line
1.6.2.1
Review
Quoted 13 lines · 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:(quoted diff) +0
-6- 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.
Quoted 4 lines
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!
Signature · 1 line
Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Boaz Harrosh
Hi Jens
You never commented on these patches. Please have a look?
Quoted 9 lines
Review
Quoted 9 lines
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:(quoted diff) +0
-6- 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)
Quoted 12 lines
Sorry, I think this is a horrible design.
blk_put_request()doesn't care about any data attachments, in fact (if possible) it should goBUG()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.Quoted 4 lines
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.
Quoted 5 lines
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
Quoted 36 lines · Boaz Harrosh
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Boaz Harrosh
Hi Jens
You never commented on these patches. Please have a look?
Quoted 9 lines
Review
Quoted 9 lines
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:(quoted diff) +0
-6- 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)Quoted 12 lines
Sorry, I think this is a horrible design.
blk_put_request()doesn't care about any data attachments, in fact (if possible) it should goBUG()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.Quoted 4 lines
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.
Quoted 5 lines
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!
Quoted 6 lines
Then the above code taken from today's
osd_initiatoris 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 fromblk_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.
Signature · 1 line
Jens Axboe
Quoted 37 lines · Jens Axboe
Quoted 36 lines · Boaz Harrosh
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Boaz Harrosh
Hi Jens
You never commented on these patches. Please have a look?
Quoted 9 lines
Review
Quoted 9 lines
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:(quoted diff) +0
-6- 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)Quoted 6 lines
Sorry, I think this is a horrible design.
blk_put_request()doesn't care about any data attachments, in fact (if possible) it should goBUG()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.Quoted 9 lines
Quoted 4 lines
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_initiatoris good? People where complaining that it is not good, because it is messing up with internal block structures.
Quoted 6 lines
Quoted 2 lines
What about just exporting an
blk_rq_abort(struct request *req)that is the same as PATCH 1/2. But is not called fromblk_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
Quoted 45 lines · Boaz Harrosh
Quoted 37 lines · Jens Axboe
Quoted 36 lines · Boaz Harrosh
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Boaz Harrosh
Hi Jens
You never commented on these patches. Please have a look?
Quoted 9 lines
Review
Quoted 9 lines
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:(quoted diff) +0
-6- 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)Quoted 6 lines
Sorry, I think this is a horrible design.
blk_put_request()doesn't care about any data attachments, in fact (if possible) it should goBUG()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.Quoted 9 lines
Quoted 4 lines
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_initiatoris good? People where complaining that it is not good, because it is messing up with internal block structures.Quoted 6 lines
Quoted 2 lines
What about just exporting an
blk_rq_abort(struct request *req)that is the same as PATCH 1/2. But is not called fromblk_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.
Quoted 5 lines
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 theend_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.
Signature · 1 line
Jens Axboe
Quoted 54 lines · Jens Axboe
Quoted 40 lines · Boaz Harrosh
Quoted 39 lines · Jens Axboe
Quoted 38 lines · Boaz Harrosh
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Boaz Harrosh
Hi Jens
You never commented on these patches. Please have a look?
Quoted 9 lines
Review
Quoted 9 lines
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:(quoted diff) +0
-6- 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)Quoted 6 lines
Sorry, I think this is a horrible design.
blk_put_request()doesn't care about any data attachments, in fact (if possible) it should goBUG()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.Quoted 9 lines
Quoted 4 lines
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_initiatoris 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 fromblk_put_request?Quoted 8 lines
Quoted 3 lines
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 theend_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.
Quoted 6 lines
Doing panic style cleanup at
blk_put_request()time is definitely not the right approach. And you can't just export ablk_rq_abort()type functionality, since you don't don't know what you are trying to abort. You assume thatbio_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
Quoted 62 lines · Boaz Harrosh
Quoted 54 lines · Jens Axboe
Quoted 40 lines · Boaz Harrosh
Quoted 39 lines · Jens Axboe
Quoted 38 lines · Boaz Harrosh
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Boaz Harrosh
Hi Jens
You never commented on these patches. Please have a look?
Quoted 9 lines
Review
Quoted 9 lines
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:(quoted diff) +0
-6- 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)Quoted 6 lines
Sorry, I think this is a horrible design.
blk_put_request()doesn't care about any data attachments, in fact (if possible) it should goBUG()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.Quoted 9 lines
Quoted 4 lines
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_initiatoris 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 fromblk_put_request?Quoted 8 lines
Quoted 3 lines
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 theend_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.
Quoted 6 lines
Doing panic style cleanup at
blk_put_request()time is definitely not the right approach. And you can't just export ablk_rq_abort()type functionality, since you don't don't know what you are trying to abort. You assume thatbio_endio()will always do the right thing, that may not be enough. Hence you need the caller to cleanup after themselves.
Quoted 2 lines
So current (above) code in
osd_initiator.cis 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?
Signature · 1 line
Jens Axboe
Quoted 6 lines · Jens Axboe
Quoted 2 lines
So current (above) code in
osd_initiator.cis 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 1/1 WARN_ON if blk_put_request leaks BIOs
In reply to Jens AxboePut 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.
--- block/blk-core.c | 3 +++ block/blk-merge.c | 2 ++ block/scsi_ioctl.c | 21 ++++----------------- 3 files changed, 9 insertions(+), 17 deletions(-)
block/blk-core.c +3 -0
@@ -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
block/blk-merge.c +2 -0
@@ -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 ; }
block/scsi_ioctl.c +4 -17
@@ -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,
Signature · 1 line
1.6.2.1
Re: [PATCH] WARN_ON if blk_put_request leaks BIOs
Quoted 8 lines · Boaz Harrosh
Put a
WARN_ONin__blk_put_requestif 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/whererequest->bio!= NULL ownership was not honored. And a small cleanup atsg_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.
Signature · 1 line
Jens Axboe
Re: [PATCH] WARN_ON if blk_put_request leaks BIOs
Quoted 9 lines · Jens Axboe
Quoted 7 lines · Boaz Harrosh
Put a
WARN_ONin__blk_put_requestif 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/whererequest->bio!= NULL ownership was not honored. And a small cleanup atsg_io()while at it.
Quoted 9 lines
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.cchange, 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