* [PATCH] add trim command to blkback interface
@ 2010-12-06 11:28 Owen Smith
2010-12-06 11:53 ` Jan Beulich
2010-12-06 20:26 ` Jeremy Fitzhardinge
0 siblings, 2 replies; 6+ messages in thread
From: Owen Smith @ 2010-12-06 11:28 UTC (permalink / raw)
To: Xen Devel
[-- Attachment #1.1: Type: text/plain, Size: 13586 bytes --]
[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 {
[-- Attachment #1.2: Type: text/html, Size: 47504 bytes --]
[-- Attachment #2: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] add trim command to blkback interface
2010-12-06 11:28 [PATCH] add trim command to blkback interface Owen Smith
@ 2010-12-06 11:53 ` Jan Beulich
2010-12-06 18:35 ` Jeremy Fitzhardinge
2010-12-06 20:26 ` Jeremy Fitzhardinge
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2010-12-06 11:53 UTC (permalink / raw)
To: Owen Smith; +Cc: Xen Devel
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] add trim command to blkback interface
2010-12-06 11:53 ` Jan Beulich
@ 2010-12-06 18:35 ` Jeremy Fitzhardinge
2010-12-07 10:06 ` Paul Durrant
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2010-12-06 18:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen Devel, Owen Smith
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] add trim command to blkback interface
2010-12-06 11:28 [PATCH] add trim command to blkback interface Owen Smith
2010-12-06 11:53 ` Jan Beulich
@ 2010-12-06 20:26 ` Jeremy Fitzhardinge
1 sibling, 0 replies; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2010-12-06 20:26 UTC (permalink / raw)
To: Owen Smith; +Cc: Xen Devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] add trim command to blkback interface
2010-12-06 18:35 ` Jeremy Fitzhardinge
@ 2010-12-07 10:06 ` Paul Durrant
2010-12-07 18:12 ` Jeremy Fitzhardinge
0 siblings, 1 reply; 6+ messages in thread
From: Paul Durrant @ 2010-12-07 10:06 UTC (permalink / raw)
To: Jeremy Fitzhardinge, Jan Beulich; +Cc: Xen Devel, Owen Smith
> -----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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] add trim command to blkback interface
2010-12-07 10:06 ` Paul Durrant
@ 2010-12-07 18:12 ` Jeremy Fitzhardinge
0 siblings, 0 replies; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2010-12-07 18:12 UTC (permalink / raw)
To: Paul Durrant; +Cc: Xen Devel, Owen Smith, Jan Beulich
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-07 18:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-06 11:28 [PATCH] add trim command to blkback interface Owen Smith
2010-12-06 11:53 ` Jan Beulich
2010-12-06 18:35 ` Jeremy Fitzhardinge
2010-12-07 10:06 ` Paul Durrant
2010-12-07 18:12 ` Jeremy Fitzhardinge
2010-12-06 20:26 ` Jeremy Fitzhardinge
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.