Re: [Fwd: [RFT] major libata update]

[Jens Axboe <axboe@suse.de>]

On Wed, May 17 2006, Jeff Garzik wrote:
> Jens Axboe wrote:
> >On Tue, May 16 2006, Jeff Garzik wrote:
> >>James Bottomley wrote:
> >>>On Tue, 2006-05-16 at 12:12 -0400, Jeff Garzik wrote:
> >>>>Its an API-which-only-libata-uses that we're discussing.  And because 
> >>>>its moving to the block layer, its also a 
> >>>>temporary-API-which-only-libata-uses.
> >>>OK ... this may be the root of the problem.  I really would like libata
> >>>to migrate to being block only ... especially as PATA looks to be trying
> >>>to follow you into the SCSI subsystem.  However, this has been the
> >>>statement for the past two years (at least), and really, few
> >>>enhancements have been made to block that you need to make good on this.
> >>>I think one of the things we'll try to find time to do at the storage
> >>>summit is to take a hard look at block to see exactly what has to be
> >>>added to make libata solely dependent upon it.
> >>100% agreed...
> >
> >Ditto! I'd be more than willing to implement some of these features (and
> >already started to, the per command timeout for instance), but I was
> >starting to write off libata moving to block as a silly pipe dream in
> >all honesty... But if momentum is picking up behind this move, then I'll
> >all for it.
> 
> Just gotta be patient.  Rome wasn't built in a day, and all that :)

:-)

> Like I mentioned in another message, the ideal world is that libata uses 
> an ATA disk driver and a SCSI MMC driver -- just like a modern SAS 
> controller (which likely supports SATA too) will use both an ATA disk 
> driver and a SCSI disk driver.
>
> Given this "ideal world", its IMO best that the "storage driver" 
> infrastructure lives in the block layer not SCSI layer.

Right

> >>The general list, off the top of my head:
> >>
> >>* objects: storage message, storage device, storage host, and the 
> >>requisite interconnections
> >
> >Storage message -> request. The rq-cmd-type branch of the block repo has
> >most/some of that done. For an explicit storage device + host, I have no
> >plans to expland on what we have.
> 
> Agreed that storage message == request.
> 
> storage device and storage host are key objects included in the 
> infrastructure libata uses SCSI for.  They fall naturally out of the 
> infrastructure that provides "device busy", "host busy", EH and EH 
> synchronization across multiple devices, etc.  Though these, SCSI also 
> provides infrastructure through which an LLDD may export a bus topology 
> to the user.

James/others already touched on that, and I agree it's a useful
abstraction. It's something that we can use for other drivers right now,
such as cciss.

> >>* queuecommand-style API
> >
> >That's a style issue, rather than a required item. You can roll that on
> >top of the current api by just doing a:
> >
> >int queuecommand_helper(request_queue_t *q, struct request *rq)
> >{
> >        /* issue request */
> >        ...
> >        return OK/DEFER/REJECT/WHATEVER
> >}
> >
> >blk_queuecommand_helper(request_queue_t *q, queue_command_fn *fn)
> >{
> >        struct request *rq;
> >        int ret;
> >
> >        do {
> >                rq = elv_next_request(q);
> >                if (!rq)
> >                        break;
> >
> >                ret = fn(q, rq);
> >                if (ret == OK)
> >                        continue;
> >
> >                /* handle replugging/killing/whatever */
> >        } while (1);
> >}
> >
> >if you really wanted.
> 
> That's not an optional piece.  Given the needed timeout / device / host 

I think we have a different opinion on what 'optional' is then - because
things can certainly work just fine the way they current do. And it's
faster, too.

> infrastructure, you inevitably wind up with the following code pattern:
> 
> 	infrastructure code
> 	send fully prepared request to hardware
> 	infrastructure code

But yes, you can make the code nicer for _some_ things with a
->queueone() type setup.

> At this point I should note that all of what I've been describing is
> an _optional addition_ to the block layer.  Its all helpers and a few
> new, optional structs.  This SHOULD NOT involve changing the core
> block layer at all.  Well, maybe struct request would like the
> addition of a timer.  But that's it, and such a mod is easy to do.

The timer is a given, we can't escape that. And the ->queueone() is
basically hashed out above, no infrastructure changes needed.
queuecommand_helper would be driver supplied, blk_queuecommand_helper()
would be a block layer helper. With better names of course, I truly do
suck at naming functions :-)

-- 
Jens Axboe