lkml.org 
[lkml]   [2009]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: New TRIM/UNMAP tree published (2009-05-02)
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






\
 
 \ /
  Last update: 2009-05-03 21:57    [W:0.782 / U:0.492 seconds]
©2003-2011 Jasper Spaans