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

[Jens Axboe <jens.axboe@oracle.com>]

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