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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Nov 12, 2013 at 01:46:19PM +0000, 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.
> 

Oh no. I thought that the Linux one had this set correctly, ah it did:

                                                                                  
struct blkif_request_indirect {                                                    
        uint8_t        indirect_op;                                                
        uint16_t       nr_segments;                                                
#ifdef CONFIG_X86_64                                                               
        uint32_t       _pad1;        /* offsetof(blkif_...,u.indirect.id) == 8 */
#endif                                                                             
        uint64_t       id;                                                         
        blkif_sector_t sector_number;                                              
        blkif_vdev_t   handle;                                                     
        uint16_t       _pad2;                                                      
        grant_ref_t    indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];       
#ifdef CONFIG_X86_64                                                               
        uint32_t      _pad3;         /* make it 64 byte aligned */                 
#else                                                                              
        uint64_t      _pad3;         /* make it 64 byte aligned */                 
#endif                                                                             
} __attribute__((__packed__));     

Wheew.
>   Paul
> 
> > +    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
> > +};
> > +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;
> > +    uint16_t    _pad; /* padding to make it 8 bytes, so it's cache-aligned */
> > +};
> > +
> >  struct blkif_response {
> >      uint64_t        id;              /* copied from request */
> >      uint8_t         operation;       /* copied from request */
> > --
> > 1.7.7.5 (Apple Git-26)
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel