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
--
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
