Re: [PATCH 4/4] bidi support: bidirectional request

[Benny Halevy <bhalevy@panasas.com>]

Jens Axboe wrote:
> On Mon, Apr 30 2007, Douglas Gilbert wrote:
>> Jens Axboe wrote:
>>> On Mon, Apr 30 2007, Benny Halevy wrote:
>>>> Jens Axboe wrote:
>>>>> On Sun, Apr 29 2007, James Bottomley wrote:
>>>>>> On Sun, 2007-04-29 at 18:48 +0300, Boaz Harrosh wrote:
>>>>>>> FUJITA Tomonori wrote:
>>>>>>>> From: Boaz Harrosh <bharrosh@panasas.com>
>>>>>>>> Subject: [PATCH 4/4] bidi support: bidirectional request
>>>>>>>> Date: Sun, 15 Apr 2007 20:33:28 +0300
>>>>>>>>
>>>>>>>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>>>>>>>> index 645d24b..16a02ee 100644
>>>>>>>>> --- a/include/linux/blkdev.h
>>>>>>>>> +++ b/include/linux/blkdev.h
>>>>>>>>> @@ -322,6 +322,7 @@ struct request {
>>>>>>>>>      void *end_io_data;
>>>>>>>>>
>>>>>>>>>      struct request_io_part uni;
>>>>>>>>> +    struct request_io_part bidi_read;
>>>>>>>>>  };
>>>>>>>> Would be more straightforward to have:
>>>>>>>>
>>>>>>>> struct request_io_part in;
>>>>>>>> struct request_io_part out;
>>>>>>>>
>>>>>>> Yes I wish I could do that. For bidi supporting drivers this is the most logical.
>>>>>>> But for the 99.9% of uni-directional drivers, calling rq_uni(), and being some what on
>>>>>>> the hotish paths, this means we will need a pointer to a uni request_io_part.
>>>>>>> This is bad because:
>>>>>>> 1st- There is no defined stage in a request life where to definitely set that pointer,
>>>>>>>      specially in the preparation stages.
>>>>>>> 2nd- hacks like scsi_error.c/scsi_send_eh_cmnd() will not work at all. Now this is a
>>>>>>>      very bad spot already, and I have a short term fix for it in the SCSI-bidi patches
>>>>>>>      (not sent yet) but a more long term solution is needed. Once such hacks are
>>>>>>>      cleaned up we can do what you say. This is exactly why I use the access functions
>>>>>>>      rq_uni/rq_io/rq_in/rq_out and not open code access.
>>>>>> I'm still not really convinced about this approach.  The primary job of
>>>>>> the block layer is to manage and merge READ and WRITE requests.  It
>>>>>> serves a beautiful secondary function of queueing for arbitrary requests
>>>>>> it doesn't understand (REQ_TYPE_BLOCK_PC or REQ_TYPE_SPECIAL ... or
>>>>>> indeed any non REQ_TYPE_FS).
>>>>>>
>>>>>> bidirectional requests fall into the latter category (there's nothing
>>>>>> really we can do to merge them ... they're just transported by the block
>>>>>> layer).  The only unusual feature is that they carry two bios.  I think
>>>>>> the drivers that actually support bidirectional will be a rarity, so it
>>>>>> might even be advisable to add it to the queue capability (refuse
>>>>>> bidirectional requests at the top rather than perturbing all the drivers
>>>>>> to process them).
>>>>>>
>>>>>> So, what about REQ_TYPE_BIDIRECTIONAL rather than REQ_BIDI?  That will
>>>>>> remove it from the standard path and put it on the special command type
>>>>>> path where we can process it specially.  Additionally, if you take this
>>>>>> approach, you can probably simply chain the second bio through
>>>>>> req->special as an additional request in the stream.  The only thing
>>>>>> that would then need modification would be the dequeue of the block
>>>>>> driver (it would have to dequeue both requests and prepare them) and
>>>>>> that needs to be done only for drivers handling bidirectional requests.
>>>>> I agree, I'm really not crazy about shuffling the entire request setup
>>>>> around just for something as exotic as bidirection commands. How about
>>>>> just keeping it simple - have a second request linked off the first one
>>>>> for the second data phase? So keep it completely seperate, not just
>>>>> overload ->special for 2nd bio list.
>>>>>
>>>>> So basically just add a struct request pointer, so you can do rq =
>>>>> rq->next_rq or something for the next data phase. I bet this would be a
>>>>> LOT less invasive as well, and we can get by with a few helpers to
>>>>> support it.
>>>>>
>>>>> And it should definitely be a request type.
>>>>>
>>>> I'm a bit confused since what you both suggest is very similar to what we've
>>>> proposed back in October 2006 and the impression we got was that it will be
>>>> better to support bidirectional block requests natively (yet to be honest,
>>>> James, you wanted a linked request all along).
>>> It still has to be implemented natively at the block layer, just
>>> differently like described above. So instead of messing all over the
>>> block layer adding rq_uni() stuff, just add that struct request pointer
>>> to the request structure for the 2nd data phase. You can relatively easy
>>> then modify the block layer helpers to support mapping and setup of such
>>> requests.
>>>
>>>> Before we go on that route again, how do you see the support for bidi
>>>> at the scsi mid-layer done?  Again, we prefer to support that officially
>>>> using two struct scsi_cmnd_buff instances in struct scsi_cmnd and not as
>>>> a one-off feature, using special-purpose state and logic (e.g. a linked
>>>> struct scsi_cmd for the bidi_read sg list).
>>> The SCSI part is up to James, that can be done as either inside a single
>>> scsi command, or as linked scsi commands as well. I don't care too much
>>> about that bit, just the block layer parts :-). And the proposed block
>>> layer design can be used both ways by the scsi layer.
>> Linked SCSI commands have been obsolete since SPC-4 rev 6
>> (18 July 2006) after proposal 06-259r1 was accepted. That
>> proposal starts: "The reasons for linked commands have been
>> overtaken by time and events." I haven't see anyone mourning
>> their demise on the t10 reflector.
> 
> This has nothing to do with linked commands as defined in the SCSI spec.
> 
>> Mapping two requests to one bidi SCSI command might make error
>> handling more of a challenge.
> 
> Then go the other way, a command for each. Not a big deal.
> 

Let's take a stab at it then and see how it goes.