Re: New TRIM/UNMAP tree published (2009-05-02)

[Jeff Garzik <jeff@garzik.org>]

James Bottomley wrote:
> On Sun, 2009-05-03 at 14:40 -0400, Jeff Garzik wrote:
>> Jeff Garzik wrote:
>>> (2) determine at init if queue (a) supports explicit DISCARD and/or (b) 
>>> supports DISCARD flag passed with READ or WRITE
>>
>> As an aside -- does any existing command set support case #b, above?
> 
> Not to my knowledge.
> 
> I think discard was modelled on barrier (which can be associated with
> data or stand on its own).

Yeah -- I am thinking we can reduce complexity if no real hardware 
supports "associated with data."


>> AFAICT, ATA, SCSI and NVMHCI all have a single, explicit hardware 
>> command to discard/deallocate unused sectors.
>>
>> Therefore, creating REQ_TYPE_DISCARD seems to eliminate any need for new 
>> hook ->prepare_discard().
> 
> Well, yes and no ... in the SCSI implementation of either UNMAP or WRITE
> SAME, we still need a data buffer to store either the extents or the
> actual same data.

Sure, UNMAP's extents would quite naturally travel with the 
REQ_TYPE_DISCARD struct request, and can be set up at struct request 
allocation time.  In all of ATA, SCSI and NVMHCI, they appear to want 
the extents.

Is WRITE SAME associated with this current DISCARD work, or is that just 
a similar example?  I'm unfamiliar with its issues...


>> This provides a 1:1 correspondence between hardware and struct request, 
>> most closely matching the setup of known hardware.
> 
> Agreed ... but we still have to allocate the adjunct buffer
> somewhere ....

Agreed.  I merely argue ->prep_rq_fn() would be a better place -- it can 
at least return a useful return value -- than ->prepare_discard(), which 
would be the natural disposition for a REQ_TYPE_DISCARD -- or 
->request_fn() itself, if a block driver so chooses.

The extents would be an argument to REQ_TYPE_DISCARD, and should be 
associated with struct request somehow, by struct request's creator. 
The extent info needs to be in some generic form, because all of 
ATA|SCSI|NVMHCI seem to want it.


[tangent...]

Does make you wonder if a ->init_rq_fn() would be helpful, one that 
could perform gfp_t allocations rather than GFP_ATOMIC?  The idea being 
to call ->init_rq_fn() almost immediately after creation of struct 
request, by the struct request creator.

I obviously have not thought in depth about this, but it does seem that 
init_rq_fn(), called earlier in struct request lifetime, could eliminate 
the need for ->prepare_flush, ->prepare_discard, and perhaps could be a 
better place for some of the ->prep_rq_fn logic.

The creator of struct request generally has more freedom to sleep, and 
it seems logical to give low-level drivers a "fill in LLD-specific info" 
hook BEFORE the request is ever added to a request_queue.

Who knows...

	Jeff