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

[Li Dongyang <jerry87905@xxxxxxxxx>]

On Sat, Aug 20, 2011 at 8:38 AM, Jeremy Fitzhardinge <jeremy@xxxxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: Li Dongyang <lidongyang@xxxxxxxxxx>
>> ---
>>  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.
>
>> + */
>> +#define BLKIF_OP_TRIM            5
>> +
>>  /*
>>   * Maximum scatter/gather segments per request.
>>   * This is carefully chosen so that sizeof(struct blkif_ring) <= PAGE_SIZE.
>> @@ -74,6 +87,11 @@ struct blkif_request_rw {
>>       } seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>>  };
>>
>> +struct blkif_request_trim {
>> +     blkif_sector_t sector_number;
>> +     uint64_t nr_sectors;
>> +};
>> +
>>  struct blkif_request {
>>       uint8_t        operation;    /* BLKIF_OP_???                         */
>>       uint8_t        nr_segments;  /* number of segments                   */
>> @@ -81,6 +99,7 @@ struct blkif_request {
>>       uint64_t       id;           /* private guest value, echoed in resp  */
>>       union {
>>               struct blkif_request_rw rw;
>> +             struct blkif_request_trim trim;
>>       } u;
>>  };
>>
>> @@ -109,6 +128,8 @@ DEFINE_RING_TYPES(blkif, struct blkif_request, struct 
>> blkif_response);
>>  #define VDISK_CDROM        0x1
>>  #define VDISK_REMOVABLE    0x2
>>  #define VDISK_READONLY     0x4
>> +#define VDISK_FILE_BACKEND 0x8
>> +#define VDISK_PHY_BACKEND  0x10
>
> What are these for?  Why does a frontend care about these?
they are used for the backend driver to decide what to do when we get
a OP_TRIM, if it's phy then forward the trim,
or punch a hole in the file, Jan also mentioned this is not good,
gonna find another place for the flags, thanks
>
>    J
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel