Re: [PATCH] blkif: add indirect descriptors interface to public headers

[David Vrabel <david.vrabel@citrix.com>]

On 12/11/13 13:46, Paul Durrant wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Roger Pau Monne
>> Sent: 12 November 2013 10:37
>> To: xen-devel@lists.xenproject.org
>> Cc: Keir (Xen.org); Jan Beulich; Roger Pau Monne
>> Subject: [Xen-devel] [PATCH] blkif: add indirect descriptors interface to
>> public headers
>>
>> Indirect descriptors introduce a new block operation
>> (BLKIF_OP_INDIRECT) that passes grant references instead of segments
>> in the request. This grant references are filled with arrays of
>> blkif_request_segment_aligned, this way we can send more segments in a
>> request.
>>
>> This interface is already implemented in Linux >= 3.11.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> ---
>>  xen/include/public/io/blkif.h |   51
>> +++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 51 insertions(+), 0 deletions(-)
>>
>> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
>> index b9b9d98..84eb7fd 100644
>> --- a/xen/include/public/io/blkif.h
>> +++ b/xen/include/public/io/blkif.h
>> @@ -468,6 +468,30 @@
>>  #define BLKIF_OP_DISCARD           5
>>
>>  /*
>> + * Recognized if "feature-max-indirect-segments" in present in the backend
>> + * xenbus info. The "feature-max-indirect-segments" node contains the
>> maximum
>> + * number of segments allowed by the backend per request. If the node is
>> + * present, the frontend might use blkif_request_indirect structs in order to
>> + * issue requests with more than BLKIF_MAX_SEGMENTS_PER_REQUEST
>> (11). The
>> + * maximum number of indirect segments is fixed by the backend, but the
>> + * frontend can issue requests with any number of indirect segments as
>> long as
>> + * it's less than the number provided by the backend. The indirect_grefs
>> field
>> + * in blkif_request_indirect should be filled by the frontend with the
>> + * grant references of the pages that are holding the indirect segments.
>> + * This pages are filled with an array of blkif_request_segment_aligned
>> + * that hold the information about the segments. The number of indirect
>> + * pages to use is determined by the maximum number of segments
>> + * an indirect request contains. Every indirect page can contain a maximum
>> + * of 512 segments (PAGE_SIZE/sizeof(blkif_request_segment_aligned)),
>> + * so to calculate the number of indirect pages to use we have to do
>> + * ceil(indirect_segments/512).
>> + *
>> + * If a backend does not recognize BLKIF_OP_INDIRECT, it should *not*
>> + * create the "feature-max-indirect-segments" node!
>> + */
>> +#define BLKIF_OP_INDIRECT          6
>> +
>> +/*
>>   * Maximum scatter/gather segments per request.
>>   * This is carefully chosen so that sizeof(blkif_ring_t) <= PAGE_SIZE.
>>   * NB. This could be 12 if the ring indexes weren't stored in the same page.
>> @@ -475,6 +499,11 @@
>>  #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
>>
>>  /*
>> + * Maximum number of indirect pages to use per request.
>> + */
>> +#define BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST 8
>> +
>> +/*
>>   * NB. first_sect and last_sect in blkif_request_segment, as well as
>>   * sector_number in blkif_request, are always expressed in 512-byte units.
>>   * However they must be properly aligned to the real sector size of the
>> @@ -517,6 +546,28 @@ struct blkif_request_discard {
>>  };
>>  typedef struct blkif_request_discard blkif_request_discard_t;
>>
>> +struct blkif_request_indirect {
>> +    uint8_t        operation;    /* BLKIF_OP_INDIRECT                    */
>> +    uint8_t        indirect_op;  /* BLKIF_OP_{READ/WRITE}                */
>> +    uint16_t       nr_segments;  /* number of segments                   */
> 
> This is going to be a problem. What alignment boundary are you
> expecting the next field to start on? AFAIK 32-bit gcc will 4-byte align
> it, 32-bit MSVC will 8-byte align it.

The blkif request structure have always been a train wreck of ABI
incompatibilities.  The existing code in the backend for handling the
existing structures should handle this in a similar way.

>> +    uint64_t       id;           /* private guest value, echoed in resp  */
>> +    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
>> +    blkif_vdev_t   handle;       /* same as for read/write requests      */
>> +    grant_ref_t
>> indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
>> +#ifdef __i386__
>> +    uint64_t       pad;          /* Make it 64 byte aligned on i386      */
>> +#endif

This last field looks a bit odd though.  Why is it needed?

>> +};
>> +typedef struct blkif_request_indirect blkif_request_indirect_t;
>> +
>> +struct blkif_request_segment_aligned {
>> +    grant_ref_t gref;            /* reference to I/O buffer frame        */
>> +    /* @first_sect: first sector in frame to transfer (inclusive).   */
>> +    /* @last_sect: last sector in frame to transfer (inclusive).     */
>> +    uint8_t     first_sect, last_sect;

Missing uint8_t padding here?

>> +    uint16_t    _pad; /* padding to make it 8 bytes, so it's cache-aligned */
>> +};

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel