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