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: "Owen Smith" <owen.smith@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] add trim command to blkback interface
From: "Jan Beulich" <JBeulich@xxxxxxxxxx>
Date: Mon, 06 Dec 2010 11:53:25 +0000
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Mon, 06 Dec 2010 03:54:34 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <DECC4DEE86A8074C9ABE8DFD330CB6E38D07A16D3D@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>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
>>> 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.

Isn't the whole patch also incomplete as it doesn't touch
blkfront at all (and hence will presumably cause build
errors)?

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?

Jan


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