Re: [PATCH V3 1/3] xen-blkback: add BLKIF_OP_TRIM and backend types

[Li Dongyang <lidongyang@novell.com>]

On Wed, Aug 24, 2011 at 6:26 PM, Jan Beulich <JBeulich@novell.com> wrote:
>>>> On 24.08.11 at 11:23, Li Dongyang <lidongyang@novell.com> wrote:
>> This adds the BLKIF_OP_TRIM for blkfront and blkback, also 2 enums telling
>> us the type of the backend, used in blkback to determine what to do when we
>> see a trim request.
>> The BLKIF_OP_TRIM part is just taken from Owen Smith, Thanks
>>
>> Signed-off-by: Owen Smith <owen.smith@citrix.com>
>> Signed-off-by: Li Dongyang <lidongyang@novell.com>
>> ---
>>  drivers/block/xen-blkback/common.h |   10 +++++++++-
>>  include/xen/interface/io/blkif.h   |   19 +++++++++++++++++++
>>  2 files changed, 28 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/common.h
>> b/drivers/block/xen-blkback/common.h
>> index 9e40b28..146d325 100644
>> --- a/drivers/block/xen-blkback/common.h
>> +++ b/drivers/block/xen-blkback/common.h
>> @@ -113,6 +113,11 @@ enum blkif_protocol {
>>       BLKIF_PROTOCOL_X86_64 = 3,
>>  };
>>
>> +enum blkif_backend_type {
>> +     BLKIF_BACKEND_PHY  = 1,
>> +     BLKIF_BACKEND_FILE = 2,
>> +};
>> +
>>  struct xen_vbd {
>>       /* What the domain refers to this vbd as. */
>>       blkif_vdev_t            handle;
>> @@ -138,6 +143,7 @@ struct xen_blkif {
>>       unsigned int            irq;
>>       /* Comms information. */
>>       enum blkif_protocol     blk_protocol;
>> +     enum blkif_backend_type blk_backend_type;
>>       union blkif_back_rings  blk_rings;
>>       struct vm_struct        *blk_ring_area;
>>       /* The VBD attached to this interface. */
>> @@ -159,8 +165,10 @@ struct xen_blkif {
>>       int                     st_wr_req;
>>       int                     st_oo_req;
>>       int                     st_f_req;
>> +     int                     st_tr_req;
>>       int                     st_rd_sect;
>>       int                     st_wr_sect;
>> +     int                     st_tr_sect;
>
> Just to repeat - I don't think this piece of statistic is very useful, the
> more that you use "int" here while ...
>
>>
>>       wait_queue_head_t       waiting_to_free;
>>
>> @@ -182,7 +190,7 @@ struct xen_blkif {
>>
>>  struct phys_req {
>>       unsigned short          dev;
>> -     unsigned short          nr_sects;
>> +     blkif_sector_t          nr_sects;
>
> ... you specifically widen the field to 64 bits here.
>
sounds reasonable, gonna kill the stat stuff
> Also, all of the changes to this header look somewhat misplaced, they
> should rather be part of the backend patch.
>
> Jan
>
>>       struct block_device     *bdev;
>>       blkif_sector_t          sector_number;
>>  };
>> diff --git a/include/xen/interface/io/blkif.h
>> b/include/xen/interface/io/blkif.h
>> index 3d5d6db..43762dd 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 trim
>> + * requests are likely to succeed or fail. Either way, a trim request
>> + * 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!
>> + */
>> +#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;
>>  };
>>
>
>
>
>