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: Jan Beulich <JBeulich@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] add trim command to blkback interface
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Mon, 06 Dec 2010 10:35:39 -0800
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Owen Smith <owen.smith@xxxxxxxxxx>
Delivery-date: Mon, 06 Dec 2010 10:39:54 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4CFCDCC5020000780002617A@xxxxxxxxxxxxxxxxxx>
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>
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/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.

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

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