Re: [PATCH V2 1/3] xen-blkfront: add BLKIF_OP_TRIM and backend type flags

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 08/22/2011 02:36 AM, Li Dongyang wrote:
> On Sat, Aug 20, 2011 at 8:38 AM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>> On 08/18/2011 02:34 AM, Li Dongyang wrote:
>>> This adds the BLKIF_OP_TRIM for blkfront and blkback, also 2 flags telling
>>> us the type of the backend, used in blkback to determine what to do when we
>>> see a trim request.
>>> Part of the patch is just taken from Owen Smith, Thanks
>>>
>>> Signed-off-by: Owen Smith <owen.smith@citrix.com>
>>> Signed-off-by: Li Dongyang <lidongyang@novell.com>
>>> ---
>>>  include/xen/interface/io/blkif.h |   21 +++++++++++++++++++++
>>>  1 files changed, 21 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/xen/interface/io/blkif.h b/include/xen/interface/io/blkif.h
>>> index 3d5d6db..b92cf23 100644
>>> --- a/include/xen/interface/io/blkif.h
>>> +++ b/include/xen/interface/io/blkif.h
>>> @@ -57,6 +57,19 @@ typedef uint64_t blkif_sector_t;
>>>   * "feature-flush-cache" node!
>>>   */
>>>  #define BLKIF_OP_FLUSH_DISKCACHE   3
>>> +
>>> +/*
>>> + * Recognised only if "feature-trim" is present in backend xenbus info.
>>> + * The "feature-trim" node contains a boolean indicating whether barrier
>>> + * requests are likely to succeed or fail. Either way, a trim request
>> Barrier requests?
> hm, I wonder the same, seems it's a copy & paste mistake,
> the BLKIF_OP_TRIM part is taken from Owen's patch back in Jan 2011:
> http://lists.xensource.com/archives/html/xen-devel/2011-01/msg01059.html
>>> + * may fail at any time with BLKIF_RSP_EOPNOTSUPP if it is unsupported by
>>> + * the underlying block-device hardware. The boolean simply indicates whether
>>> + * or not it is worthwhile for the frontend to attempt trim requests.
>>> + * If a backend does not recognise BLKIF_OP_TRIM, it should *not*
>>> + * create the "feature-trim" node!
>> Is all this necessary?  What happens if guests just send OP_TRIM
>> requests, and if the host doesn't understand them then it will fails
>> them with EOPNOTSUPP?  Is a TRIM request ever anything more than a hint
>> to the backend that certain blocks are no longer needed?
> that won't happen: we only mark the queue in the guest has TRIM if
> blkback tells blkfront
> via xenstore. if we don't init the queue with TRIM in guest, if guest
> send OP_TRIM,
> it gonna fail with ENONOTSUPP in the guest's block layer, see
> blkdev_issue_discard.
> and yes, trim is just a hint, the basic idea is forward the hint to
> phy dev if it has trim, or
> punch a hole to reduce disk usage if the backend is a file.
> and this comment is taken from Owen, I think he could give sth here.

Right.  So if this is just a hint, and there's no correctness
implications for the frontend if the backend doesn't support trim, then
the frontend should just start trying to use trim and then suppress them
if they come back as failed.

I guess if the frontend needs other information from the backend (about
trim chunk size?) then this is moot, but this kind of feature
negotiation via xenbus seems pretty flaky and it would be nice to avoid
repeating the mistake.

    J