WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] add trim command to blkback interface
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Tue, 07 Dec 2010 10:12:30 -0800
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Owen Smith <owen.smith@xxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>
Delivery-date: Tue, 07 Dec 2010 10:16:32 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <291EDFCB1E9E224A99088639C47620228CF938A588@xxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <DECC4DEE86A8074C9ABE8DFD330CB6E38D07A16D3D@xxxxxxxxxxxxxxxxxxxxxxxxx> <4CFCDCC5020000780002617A@xxxxxxxxxxxxxxxxxx> <4CFD2CFB.10200@xxxxxxxx> <291EDFCB1E9E224A99088639C47620228CF938A588@xxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101103 Fedora/1.0-0.33.b2pre.fc14 Lightning/1.0b3pre Thunderbird/3.1.6
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

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