Owen Smith
2010-Dec-06 11:28 UTC
[Xen-devel] [PATCH] add trim command to blkback interface
[PATCH] add trim command to blkback interface
This patch adds the trim operation to the blkback ring protocol.
The ring protocol is extended by unioning the current read/write/write barrier
specific fields with fields specific for trim, and allowing further expansion
with additional union members, providing the structure size and alignment do not
change.
Blkback will respond to trim operations with a BLKIF_RSP_EOPNOTSUPP, to avoid
using the default response of writing an error log entry and returning
BLKIF_RSP_ERROR.
Trim commands are passed with sector_number as the sector index to begin trim
operations at, and nr_sectors as the number of sectors to be trimmed. These
sectors should be trimmed if the underlying block device supports trimming. More
information about trim commands:
http://t13.org/Documents/UploadedDocuments/docs2008/e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc
This patch is intended to fix an interface, not supply the implementation of
trim in the backend drivers.
Signed-off-by: Owen Smith <owen.smith@citrix.com>
diff --git a/drivers/xen/blkback/blkback.c b/drivers/xen/blkback/blkback.c
index 0bef445..bd09512 100644
--- a/drivers/xen/blkback/blkback.c
+++ b/drivers/xen/blkback/blkback.c
@@ -367,6 +367,11 @@ static int do_block_io_op(blkif_t *blkif)
blkif->st_wr_req++;
dispatch_rw_block_io(blkif,
&req, pending_req);
break;
+ case BLKIF_OP_TRIM:
+ make_response(blkif, req.id,
req.operation,
+
BLKIF_RSP_EOPNOTSUPP);
+ free_req(pending_req);
+ break;
default:
/* A good sign something is
wrong: sleep for a while to
* avoid excessive CPU
consumption by a bad guest. */
@@ -424,7 +429,7 @@ static void dispatch_rw_block_io(blkif_t *blkif,
}
preq.dev = req->handle;
- preq.sector_number = req->sector_number;
+ preq.sector_number = req->rw.sector_number;
preq.nr_sects = 0;
pending_req->blkif = blkif;
@@ -436,11 +441,11 @@ static void dispatch_rw_block_io(blkif_t *blkif,
for (i = 0; i < nseg; i++) {
uint32_t flags;
- seg[i].nsec = req->seg[i].last_sect -
- req->seg[i].first_sect + 1;
+ seg[i].nsec = req->rw.seg[i].last_sect -
+ req->rw.seg[i].first_sect + 1;
- if ((req->seg[i].last_sect >= (PAGE_SIZE
>> 9)) ||
- (req->seg[i].last_sect <
req->seg[i].first_sect))
+ if ((req->rw.seg[i].last_sect >= (PAGE_SIZE
>> 9)) ||
+ (req->rw.seg[i].last_sect <
req->rw.seg[i].first_sect))
goto fail_response;
preq.nr_sects += seg[i].nsec;
@@ -448,7 +453,7 @@ static void dispatch_rw_block_io(blkif_t *blkif,
if (operation != READ)
flags |= GNTMAP_readonly;
gnttab_set_map_op(&map[i], vaddr(pending_req,
i), flags,
-
req->seg[i].gref, blkif->domid);
+
req->rw.seg[i].gref, blkif->domid);
}
ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, map,
nseg);
@@ -466,11 +471,11 @@ static void dispatch_rw_block_io(blkif_t *blkif,
page_to_pfn(pending_page(pending_req, i)),
FOREIGN_FRAME(map[i].dev_bus_addr
>> PAGE_SHIFT));
seg[i].buf = map[i].dev_bus_addr |
- (req->seg[i].first_sect
<< 9);
+ (req->rw.seg[i].first_sect
<< 9);
blkback_pagemap_set(vaddr_pagenr(pending_req, i),
pending_page(pending_req, i),
blkif->domid, req->handle,
-
req->seg[i].gref);
+
req->rw.seg[i].gref);
pending_handle(pending_req, i) = map[i].handle;
}
diff --git a/include/xen/blkif.h b/include/xen/blkif.h
index 7172081..e727e5d 100644
--- a/include/xen/blkif.h
+++ b/include/xen/blkif.h
@@ -44,8 +44,18 @@ struct blkif_x86_32_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 seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+
+ union {
+ struct blkif_x86_32_request_rw {
+ blkif_sector_t sector_number;/*
start sector idx on disk (r/w only) */
+ struct blkif_request_segment
seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+ } rw;
+
+ struct blkif_x86_32_request_trim {
+ blkif_sector_t sector_number;
+ uint64_t nr_sectors;
+ } trim;
+ };
};
struct blkif_x86_32_response {
uint64_t id; /* copied from request */
@@ -62,8 +72,18 @@ struct blkif_x86_64_request {
uint8_t nr_segments; /* number of segments
*/
blkif_vdev_t handle; /* only for read/write requests
*/
uint64_t __attribute__((__aligned__(8))) id;
- blkif_sector_t sector_number;/* start sector idx on disk (r/w
only) */
- struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+
+ union {
+ struct blkif_x86_64_request_rw {
+ blkif_sector_t sector_number;/*
start sector idx on disk (r/w only) */
+ struct blkif_request_segment
seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+ } rw;
+
+ struct blkif_x86_64_request_trim {
+ blkif_sector_t sector_number;
+ uint64_t nr_sectors;
+ } trim;
+ };
};
struct blkif_x86_64_response {
uint64_t __attribute__((__aligned__(8))) id;
@@ -97,12 +117,24 @@ static void inline blkif_get_x86_32_req(struct
blkif_request *dst, struct blkif_
dst->nr_segments = src->nr_segments;
dst->handle = src->handle;
dst->id = src->id;
- dst->sector_number = src->sector_number;
- barrier();
- if (n > dst->nr_segments)
- n = dst->nr_segments;
- for (i = 0; i < n; i++)
- dst->seg[i] = src->seg[i];
+ switch (src->operation) {
+ case BLKIF_OP_READ:
+ case BLKIF_OP_WRITE:
+ case BLKIF_OP_WRITE_BARRIER:
+ dst->rw.sector_number =
src->rw.sector_number;
+ barrier();
+ if (n > dst->nr_segments)
+ n = dst->nr_segments;
+ for (i = 0; i < n; i++)
+ dst->rw.seg[i] =
src->rw.seg[i];
+ break;
+ case BLKIF_OP_TRIM:
+ dst->trim.sector_number =
src->trim.sector_number;
+ dst->trim.nr_sectors = src->trim.nr_sectors;
+ break;
+ default:
+ break;
+ }
}
static void inline blkif_get_x86_64_req(struct blkif_request *dst, struct
blkif_x86_64_request *src)
@@ -112,12 +144,24 @@ static void inline blkif_get_x86_64_req(struct
blkif_request *dst, struct blkif_
dst->nr_segments = src->nr_segments;
dst->handle = src->handle;
dst->id = src->id;
- dst->sector_number = src->sector_number;
- barrier();
- if (n > dst->nr_segments)
- n = dst->nr_segments;
- for (i = 0; i < n; i++)
- dst->seg[i] = src->seg[i];
+ switch (src->operation) {
+ case BLKIF_OP_READ:
+ case BLKIF_OP_WRITE:
+ case BLKIF_OP_WRITE_BARRIER:
+ dst->rw.sector_number =
src->rw.sector_number;
+ barrier();
+ if (n > dst->nr_segments)
+ n = dst->nr_segments;
+ for (i = 0; i < n; i++)
+ dst->rw.seg[i] =
src->rw.seg[i];
+ break;
+ case BLKIF_OP_TRIM:
+ dst->trim.sector_number =
src->trim.sector_number;
+ dst->trim.nr_sectors = src->trim.nr_sectors;
+ break;
+ default:
+ break;
+ }
}
#endif /* __XEN_BLKIF_H__ */
diff --git a/include/xen/interface/io/blkif.h b/include/xen/interface/io/blkif.h
index 68dd2b4..54bb598 100644
--- a/include/xen/interface/io/blkif.h
+++ b/include/xen/interface/io/blkif.h
@@ -43,6 +43,17 @@ typedef uint64_t blkif_sector_t;
* create the "feature-barrier" node!
*/
#define BLKIF_OP_WRITE_BARRIER 2
+/*
+ * Recognised only if "feature-trim" is present in backend xenbus
info.
+ * The "feature_trim" node contains a boolean indicating whether trim
+ * requests are likely to succeed or fail. Either way, a trim request
+ * may fail at any time with BLKIF_RSP_EOPNOTSUPP if it is unsupported by
+ * the underlying block-device hardware. The boolean simply indicates whether
+ * or not it is worthwhile for the frontend to attempt trim requests.
+ * If a backend does not recognise BLKIF_OP_TRIM, it should *not*
+ * create the "feature-trim" node!
+ */
+#define BLKIF_OP_TRIM 4
/*
* Maximum scatter/gather segments per request.
@@ -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;
+ };
};
struct blkif_response {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2010-Dec-06 11:53 UTC
Re: [Xen-devel] [PATCH] add trim command to blkback interface
>>> 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. 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@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Dec-06 18:35 UTC
Re: [Xen-devel] [PATCH] add trim command to blkback interface
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Dec-06 20:26 UTC
Re: [Xen-devel] [PATCH] add trim command to blkback interface
On 12/06/2010 03:28 AM, Owen Smith wrote:> > [PATCH] add trim command to blkback interface > > > > This patch adds the trim operation to the blkback ring protocol. > > The ring protocol is extended by unioning the current read/write/write > barrier specific fields with fields specific for trim, and allowing > further expansion with additional union members, providing the > structure size and alignment do not change. > > > > Blkback will respond to trim operations with a BLKIF_RSP_EOPNOTSUPP, > to avoid using the default response of writing an error log entry and > returning BLKIF_RSP_ERROR. > > > > Trim commands are passed with sector_number as the sector index to > begin trim operations at, and nr_sectors as the number of sectors to > be trimmed. These sectors should be trimmed if the underlying block > device supports trimming. More information about trim commands: > > http://t13.org/Documents/UploadedDocuments/docs2008/e07154r6-Data_Set_Management_Proposal_for_ATA-ACS2.doc >Aside from the comments in my reply to Jan''s mail, I can''t actually get an applyable patch out of this; its all mashed up with quoted-printable. Could you repost as a plain-text attachment if you can''t convince your mailer to send a plain-text email? Thanks, J> > > This patch is intended to fix an interface, not supply the > implementation of trim in the backend drivers. > > > > Signed-off-by: Owen Smith <owen.smith@citrix.com> > > > > > > diff --git a/drivers/xen/blkback/blkback.c b/drivers/xen/blkback/blkback.c > > index 0bef445..bd09512 100644 > > --- a/drivers/xen/blkback/blkback.c > > +++ b/drivers/xen/blkback/blkback.c > > @@ -367,6 +367,11 @@ static int do_block_io_op(blkif_t *blkif) > > blkif->st_wr_req++; > > > dispatch_rw_block_io(blkif, &req, pending_req); > > break; > > + case BLKIF_OP_TRIM: > > + make_response(blkif, > req.id, req.operation, > > + > BLKIF_RSP_EOPNOTSUPP); > > + free_req(pending_req); > > + break; > > default: > > /* A good sign > something is wrong: sleep for a while to > > * avoid excessive CPU > consumption by a bad guest. */ > > @@ -424,7 +429,7 @@ static void dispatch_rw_block_io(blkif_t *blkif, > > } > > preq.dev = req->handle; > > - preq.sector_number = req->sector_number; > > + preq.sector_number = req->rw.sector_number; > > preq.nr_sects = 0; > > pending_req->blkif = blkif; > > @@ -436,11 +441,11 @@ static void dispatch_rw_block_io(blkif_t *blkif, > > for (i = 0; i < nseg; i++) { > > uint32_t flags; > > - seg[i].nsec = req->seg[i].last_sect - > > - req->seg[i].first_sect + 1; > > + seg[i].nsec = req->rw.seg[i].last_sect - > > + > req->rw.seg[i].first_sect + 1; > > - if ((req->seg[i].last_sect >> (PAGE_SIZE >> 9)) || > > - (req->seg[i].last_sect < > req->seg[i].first_sect)) > > + if ((req->rw.seg[i].last_sect >> (PAGE_SIZE >> 9)) || > > + (req->rw.seg[i].last_sect < > req->rw.seg[i].first_sect)) > > goto fail_response; > > preq.nr_sects += seg[i].nsec; > > @@ -448,7 +453,7 @@ static void dispatch_rw_block_io(blkif_t *blkif, > > if (operation != READ) > > flags |= GNTMAP_readonly; > > gnttab_set_map_op(&map[i], > vaddr(pending_req, i), flags, > > - > req->seg[i].gref, blkif->domid); > > + > req->rw.seg[i].gref, blkif->domid); > > } > > ret > HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, map, nseg); > > @@ -466,11 +471,11 @@ static void dispatch_rw_block_io(blkif_t *blkif, > > > page_to_pfn(pending_page(pending_req, i)), > > > FOREIGN_FRAME(map[i].dev_bus_addr >> PAGE_SHIFT)); > > seg[i].buf = map[i].dev_bus_addr | > > - (req->seg[i].first_sect > << 9); > > + > (req->rw.seg[i].first_sect << 9); > > > blkback_pagemap_set(vaddr_pagenr(pending_req, i), > > > pending_page(pending_req, i), > > > blkif->domid, req->handle, > > - > req->seg[i].gref); > > + > req->rw.seg[i].gref); > > pending_handle(pending_req, i) > map[i].handle; > > } > > diff --git a/include/xen/blkif.h b/include/xen/blkif.h > > index 7172081..e727e5d 100644 > > --- a/include/xen/blkif.h > > +++ b/include/xen/blkif.h > > @@ -44,8 +44,18 @@ struct blkif_x86_32_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 > seg[BLKIF_MAX_SEGMENTS_PER_REQUEST]; > > + > > + union { > > + struct blkif_x86_32_request_rw { > > + blkif_sector_t > sector_number;/* start sector idx on disk (r/w only) */ > > + struct > blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST]; > > + } rw; > > + > > + struct blkif_x86_32_request_trim { > > + blkif_sector_t > sector_number; > > + uint64_t nr_sectors; > > + } trim; > > + }; > > }; > > struct blkif_x86_32_response { > > uint64_t id; /* copied from request */ > > @@ -62,8 +72,18 @@ struct blkif_x86_64_request { > > uint8_t nr_segments; /* number of > segments */ > > blkif_vdev_t handle; /* only for read/write > requests */ > > uint64_t __attribute__((__aligned__(8))) id; > > - blkif_sector_t sector_number;/* start sector idx on > disk (r/w only) */ > > - struct blkif_request_segment > seg[BLKIF_MAX_SEGMENTS_PER_REQUEST]; > > + > > + union { > > + struct blkif_x86_64_request_rw { > > + blkif_sector_t > sector_number;/* start sector idx on disk (r/w only) */ > > + struct > blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST]; > > + } rw; > > + > > + struct blkif_x86_64_request_trim { > > + blkif_sector_t > sector_number; > > + uint64_t nr_sectors; > > + } trim; > > + }; > > }; > > struct blkif_x86_64_response { > > uint64_t __attribute__((__aligned__(8))) id; > > @@ -97,12 +117,24 @@ static void inline blkif_get_x86_32_req(struct > blkif_request *dst, struct blkif_ > > dst->nr_segments = src->nr_segments; > > dst->handle = src->handle; > > dst->id = src->id; > > - dst->sector_number = src->sector_number; > > - barrier(); > > - if (n > dst->nr_segments) > > - n = dst->nr_segments; > > - for (i = 0; i < n; i++) > > - dst->seg[i] = src->seg[i]; > > + switch (src->operation) { > > + case BLKIF_OP_READ: > > + case BLKIF_OP_WRITE: > > + case BLKIF_OP_WRITE_BARRIER: > > + dst->rw.sector_number > src->rw.sector_number; > > + barrier(); > > + if (n > dst->nr_segments) > > + n = dst->nr_segments; > > + for (i = 0; i < n; i++) > > + dst->rw.seg[i] > src->rw.seg[i]; > > + break; > > + case BLKIF_OP_TRIM: > > + dst->trim.sector_number > src->trim.sector_number; > > + dst->trim.nr_sectors = src->trim.nr_sectors; > > + break; > > + default: > > + break; > > + } > > } > > static void inline blkif_get_x86_64_req(struct blkif_request *dst, > struct blkif_x86_64_request *src) > > @@ -112,12 +144,24 @@ static void inline blkif_get_x86_64_req(struct > blkif_request *dst, struct blkif_ > > dst->nr_segments = src->nr_segments; > > dst->handle = src->handle; > > dst->id = src->id; > > - dst->sector_number = src->sector_number; > > - barrier(); > > - if (n > dst->nr_segments) > > - n = dst->nr_segments; > > - for (i = 0; i < n; i++) > > - dst->seg[i] = src->seg[i]; > > + switch (src->operation) { > > + case BLKIF_OP_READ: > > + case BLKIF_OP_WRITE: > > + case BLKIF_OP_WRITE_BARRIER: > > + dst->rw.sector_number > src->rw.sector_number; > > + barrier(); > > + if (n > dst->nr_segments) > > + n = dst->nr_segments; > > + for (i = 0; i < n; i++) > > + dst->rw.seg[i] > src->rw.seg[i]; > > + break; > > + case BLKIF_OP_TRIM: > > + dst->trim.sector_number > src->trim.sector_number; > > + dst->trim.nr_sectors = src->trim.nr_sectors; > > + break; > > + default: > > + break; > > + } > > } > > #endif /* __XEN_BLKIF_H__ */ > > diff --git a/include/xen/interface/io/blkif.h > b/include/xen/interface/io/blkif.h > > index 68dd2b4..54bb598 100644 > > --- a/include/xen/interface/io/blkif.h > > +++ b/include/xen/interface/io/blkif.h > > @@ -43,6 +43,17 @@ typedef uint64_t blkif_sector_t; > > * create the "feature-barrier" node! > > */ > > #define BLKIF_OP_WRITE_BARRIER 2 > > +/* > > + * Recognised only if "feature-trim" is present in backend xenbus info. > > + * The "feature_trim" node contains a boolean indicating whether trim > > + * requests are likely to succeed or fail. Either way, a trim request > > + * may fail at any time with BLKIF_RSP_EOPNOTSUPP if it is unsupported by > > + * the underlying block-device hardware. The boolean simply indicates > whether > > + * or not it is worthwhile for the frontend to attempt trim requests. > > + * If a backend does not recognise BLKIF_OP_TRIM, it should *not* > > + * create the "feature-trim" node! > > + */ > > +#define BLKIF_OP_TRIM 4 > > /* > > * Maximum scatter/gather segments per request. > > @@ -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; > > + }; > > }; > > struct blkif_response { > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Paul Durrant
2010-Dec-07 10:06 UTC
RE: [Xen-devel] [PATCH] add trim command to blkback interface
> -----Original Message----- > From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel- > bounces@lists.xensource.com] On Behalf Of Jeremy Fitzhardinge > Sent: 06 December 2010 18:36 > To: Jan Beulich > Cc: Xen Devel; Owen Smith > Subject: Re: [Xen-devel] [PATCH] add trim command to blkback > interface > > 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. >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; }; typedef union blkif_request blkif_request_t; then the specialization is done immediately after determining the op code.> 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. Paul _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Dec-07 18:12 UTC
Re: [Xen-devel] [PATCH] add trim command to blkback interface
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@lists.xensource.com http://lists.xensource.com/xen-devel