Re: [PATCH] add trim command to blkback interface

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 12/06/2010 03:53 AM, Jan Beulich wrote:
>>>> On 06.12.10 at 12:28, Owen Smith <owen.smith@citrix.com> 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.

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?

> Finally, shouldn't the patch be split (or at least accompanied
> by a second patch) to modify the master io/blkif.h (in
> -unstable) rather than the (edited) copy in the pv-ops Linux
> tree only?

Yep.  That's as close as we've got to a definitive definition of the ABI.

    J