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 <[EMAIL PROTECTED]> >>>>>>>> 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. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html