Re: [PATCH] add trim command to blkback interface

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 12/07/2010 02:06 AM, Paul Durrant wrote:
>> 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;

Spurious _t there.

> };
>
> typedef union blkif_request blkif_request_t;
>
> then the specialization is done immediately after determining the op code.

Sure.  (But drop all the typedefs.)

>> 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.

Yes, but if a kernel with this patch applied as posted doesn't compile,
it doesn't give much confidence in its testing.

    J