RE: [Xen-devel] [PATCH] add trim command to blkback interface

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Jeremy Fitzhardinge
> Sent: 06 December 2010 18:36
> To: Jan Beulich
> Cc: Xen Devel; Owen Smith
> Subject: Re: [Xen-devel] [PATCH] add trim command to blkback
> interface
> 
> On 12/06/2010 03:53 AM, Jan Beulich wrote:
> >>>> On 06.12.10 at 12:28, Owen Smith <owen.smith@xxxxxxxxxx> wrote:
> >> @@ -56,13 +67,23 @@ struct blkif_request {
> >>                uint8_t        nr_segments;  /* number of segments
> >>        */
> >>                blkif_vdev_t   handle;       /* only for
> read/write requests
> >>        */
> >>                uint64_t       id;           /* private guest
> value, echoed in resp  */
> >> -              blkif_sector_t sector_number;/* start sector idx
> on disk (r/w only)  */
> >> -              struct blkif_request_segment {
> >> -                              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;
> >> -              } seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> >> +
> >> +             union {
> >> +                             struct blkif_request_rw {
> >> +                                             blkif_sector_t
> sector_number;/* start sector idx on disk (r/w only)  */
> >> +                                             struct
> blkif_request_segment {
> >> +
> 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;
> >> +                                             }
> seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> >> +                             } rw;
> >> +
> >> +                             struct blkif_request_trim {
> >> +                             blkif_sector_t sector_number;
> >> +                                     uint64_t       nr_sectors;
> >> +                             } trim;
> >> +             };
> > Wouldn't the whole patch be quite a bit smaller if you kept
> > sector_number outside the union? If using anonymous
> > structs/unions is okay here (which I don't think it is), there
> > would also not have been a need to name the struct
> > blkif_request_rw instance, thus eliminating the need to
> > touch code just to add the new intermediate field name.
> 
> I don't think its so bad to have the name changes here, since if
> different operations take different argument formats, then its nice
> to
> explicitly name which operation args you're referring to.  The fact
> that
> the two existing arguments happen to have sector_number as their
> first
> parameter doesn't mean the third will, so moving it into the union
> makes
> sense.
> 

My feeling is that, for clarity, we should have something like this (and I 
haven't compiled this so there may be typos):

struct blkif_rw_request {
    uint8_t        operation;    /* BLKIF_OP_READ/WRITE                  */
    uint8_t        nr_segments;  /* number of segments                   */
    blkif_vdev_t   handle;       /* device handle                        */
    uint64_t       id;           /* private guest value, echoed in resp  */
    blkif_sector_t sector_number;/* start sector idx on disk             */
    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
};

struct blkif_trim_request {
    uint8_t        operation;    /* BLKIF_OP_TRIM                        */
    blkif_vdev_t   handle;       /* device handle                        */
    uint64_t       id;           /* private guest value, echoed in resp  */
    blkif_sector_t sector_number;/* start sector idx on disk             */
    uint64_t       nr_sectors;   /* number of sectors to trim            */ 
};

union blkif_request {
    uint8_t                     operation; /* BLKIF_OP_???               */
    struct blkif_rw_request     rw;
    struct blkif_trim_request_t trim;
};

typedef union blkif_request blkif_request_t;

then the specialization is done immediately after determining the op code.

> However, I'd prefer to see a separate patch do the rearrangement
> without
> adding any other functionality, and then a second patch adding trip
> support to this.
> 
> > Isn't the whole patch also incomplete as it doesn't touch
> > blkfront at all (and hence will presumably cause build
> > errors)?
> 
> Yes.  How tested is this?
> 

I believe Owen has tested this patch against a Windows frontend (which actually 
issues trims), and proven it does no harm against an existing linux frontend.

  Paul

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