From 15f084505a8556b70134e9081176e90a1c178b7b Mon Sep 17 00:00:00 2001 From: Paul Durrant Date: Wed, 20 Mar 2019 14:28:25 +0000 Subject: [PATCH 1/2] xen-block: only advertize discard to the frontend when it is enabled... ...and properly enable it when synthesizing a drive. The Xen toolstack sets 'discard-enable' to '1' in xenstore when it wants to enable discard on a specified image. The code in xen_block_drive_create() correctly parses this and uses it to set 'discard' to 'unmap' for the file_layer, but fails to do the same for the driver_layer (which effectively disables it). Meanwhile the code in xen_block_realize() advertizes discard support to the frontend in the default case (because conf->discard_granularity defaults to -1), even when the underlying image may not handle it. This patch adds the missing option to the driver_layer in xen_block_driver_create() and checks whether BDRV_O_UNMAP is actually set on the block device before advertizing discard to the frontend. In the case that discard is supported it also makes sure that the granularity is set to the physical block size. Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD Message-Id: <20190320142825.24565-1-paul.durrant@citrix.com> Signed-off-by: Anthony PERARD --- hw/block/xen-block.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index 9c722b9b95..475a67845d 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -232,8 +232,14 @@ static void xen_block_realize(XenDevice *xendev, Error **errp) blk_set_dev_ops(conf->blk, &xen_block_dev_ops, blockdev); blk_set_guest_block_size(conf->blk, conf->logical_block_size); - if (conf->discard_granularity > 0) { + if (conf->discard_granularity == -1) { + conf->discard_granularity = conf->physical_block_size; + } + + if (blk_get_flags(conf->blk) & BDRV_O_UNMAP) { xen_device_backend_printf(xendev, "feature-discard", "%u", 1); + xen_device_backend_printf(xendev, "discard-granularity", "%u", + conf->discard_granularity); } xen_device_backend_printf(xendev, "feature-flush-cache", "%u", 1); @@ -755,6 +761,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id, drive->id = g_strdup(id); file_layer = qdict_new(); + driver_layer = qdict_new(); qdict_put_str(file_layer, "driver", "file"); qdict_put_str(file_layer, "filename", filename); @@ -782,6 +789,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id, if (!qemu_strtoul(discard_enable, NULL, 2, &value) && !!value) { qdict_put_str(file_layer, "discard", "unmap"); + qdict_put_str(driver_layer, "discard", "unmap"); } } @@ -791,8 +799,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id, */ qdict_put_str(file_layer, "locking", "off"); - driver_layer = qdict_new(); - qdict_put_str(driver_layer, "driver", driver); g_free(driver); From 2bcd05cf24a7de34e7e265247c010977e43f40bc Mon Sep 17 00:00:00 2001 From: Paul Durrant Date: Mon, 1 Apr 2019 13:17:19 +0100 Subject: [PATCH 2/2] xen-block: scale sector based quantities correctly The Xen blkif protocol requires that sector based quantities should be interpreted strictly as multiples of 512 bytes. Specifically: "first_sect and last_sect in blkif_request_segment, as well as sector_number in blkif_request, are always expressed in 512-byte units." Commit fcab2b464e06 "xen: add header and build dataplane/xen-block.c" incorrectly modified behaviour to use the block device logical_block_size property as the scale, instead of correctly shifting values by the hardcoded BDRV_SECTOR_BITS (and hence scaling them to 512 byte units). This patch undoes that change and restores compliance with the spec. Furthermore, this patch also restores the original xen_disk behaviour of advertizing a hardcoded 'sector-size' value of 512 in xenstore and scaling 'sectors' accordingly. The realize() method is also modified to fail if logical_block_size is set to anything other than 512. Signed-off-by: Paul Durrant Reviewed-by: Anthony PERARD Message-Id: <20190401121719.27208-1-paul.durrant@citrix.com> Signed-off-by: Anthony PERARD --- hw/block/dataplane/xen-block.c | 28 +++++++++++++--------------- hw/block/xen-block.c | 10 ++++++++-- hw/block/xen_blkif.h | 2 ++ 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c index f1523c5b45..bb8f1186e4 100644 --- a/hw/block/dataplane/xen-block.c +++ b/hw/block/dataplane/xen-block.c @@ -49,7 +49,6 @@ struct XenBlockDataPlane { unsigned int *ring_ref; unsigned int nr_ring_ref; void *sring; - int64_t file_blk; int protocol; blkif_back_rings_t rings; int more_work; @@ -168,7 +167,7 @@ static int xen_block_parse_request(XenBlockRequest *request) goto err; } - request->start = request->req.sector_number * dataplane->file_blk; + request->start = request->req.sector_number * XEN_BLKIF_SECTOR_SIZE; for (i = 0; i < request->req.nr_segments; i++) { if (i == BLKIF_MAX_SEGMENTS_PER_REQUEST) { error_report("error: nr_segments too big"); @@ -178,14 +177,14 @@ static int xen_block_parse_request(XenBlockRequest *request) error_report("error: first > last sector"); goto err; } - if (request->req.seg[i].last_sect * dataplane->file_blk >= + if (request->req.seg[i].last_sect * XEN_BLKIF_SECTOR_SIZE >= XC_PAGE_SIZE) { error_report("error: page crossing"); goto err; } len = (request->req.seg[i].last_sect - - request->req.seg[i].first_sect + 1) * dataplane->file_blk; + request->req.seg[i].first_sect + 1) * XEN_BLKIF_SECTOR_SIZE; request->size += len; } if (request->start + request->size > blk_getlength(dataplane->blk)) { @@ -205,7 +204,6 @@ static int xen_block_copy_request(XenBlockRequest *request) XenDevice *xendev = dataplane->xendev; XenDeviceGrantCopySegment segs[BLKIF_MAX_SEGMENTS_PER_REQUEST]; int i, count; - int64_t file_blk = dataplane->file_blk; bool to_domain = (request->req.operation == BLKIF_OP_READ); void *virt = request->buf; Error *local_err = NULL; @@ -220,16 +218,17 @@ static int xen_block_copy_request(XenBlockRequest *request) if (to_domain) { segs[i].dest.foreign.ref = request->req.seg[i].gref; segs[i].dest.foreign.offset = request->req.seg[i].first_sect * - file_blk; + XEN_BLKIF_SECTOR_SIZE; segs[i].source.virt = virt; } else { segs[i].source.foreign.ref = request->req.seg[i].gref; segs[i].source.foreign.offset = request->req.seg[i].first_sect * - file_blk; + XEN_BLKIF_SECTOR_SIZE; segs[i].dest.virt = virt; } segs[i].len = (request->req.seg[i].last_sect - - request->req.seg[i].first_sect + 1) * file_blk; + request->req.seg[i].first_sect + 1) * + XEN_BLKIF_SECTOR_SIZE; virt += segs[i].len; } @@ -331,22 +330,22 @@ static bool xen_block_split_discard(XenBlockRequest *request, XenBlockDataPlane *dataplane = request->dataplane; int64_t byte_offset; int byte_chunk; - uint64_t byte_remaining, limit; + uint64_t byte_remaining; uint64_t sec_start = sector_number; uint64_t sec_count = nr_sectors; /* Wrap around, or overflowing byte limit? */ if (sec_start + sec_count < sec_count || - sec_start + sec_count > INT64_MAX / dataplane->file_blk) { + sec_start + sec_count > INT64_MAX / XEN_BLKIF_SECTOR_SIZE) { return false; } - limit = BDRV_REQUEST_MAX_SECTORS * dataplane->file_blk; - byte_offset = sec_start * dataplane->file_blk; - byte_remaining = sec_count * dataplane->file_blk; + byte_offset = sec_start * XEN_BLKIF_SECTOR_SIZE; + byte_remaining = sec_count * XEN_BLKIF_SECTOR_SIZE; do { - byte_chunk = byte_remaining > limit ? limit : byte_remaining; + byte_chunk = byte_remaining > BDRV_REQUEST_MAX_BYTES ? + BDRV_REQUEST_MAX_BYTES : byte_remaining; request->aio_inflight++; blk_aio_pdiscard(dataplane->blk, byte_offset, byte_chunk, xen_block_complete_aio, request); @@ -632,7 +631,6 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice *xendev, XenBlockDataPlane *dataplane = g_new0(XenBlockDataPlane, 1); dataplane->xendev = xendev; - dataplane->file_blk = conf->logical_block_size; dataplane->blk = conf->blk; QLIST_INIT(&dataplane->inflight); diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index 475a67845d..ef635be4c2 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -149,7 +149,7 @@ static void xen_block_set_size(XenBlockDevice *blockdev) const char *type = object_get_typename(OBJECT(blockdev)); XenBlockVdev *vdev = &blockdev->props.vdev; BlockConf *conf = &blockdev->props.conf; - int64_t sectors = blk_getlength(conf->blk) / conf->logical_block_size; + int64_t sectors = blk_getlength(conf->blk) / XEN_BLKIF_SECTOR_SIZE; XenDevice *xendev = XEN_DEVICE(blockdev); trace_xen_block_size(type, vdev->disk, vdev->partition, sectors); @@ -223,6 +223,12 @@ static void xen_block_realize(XenDevice *xendev, Error **errp) blkconf_blocksizes(conf); + if (conf->logical_block_size != XEN_BLKIF_SECTOR_SIZE) { + error_setg(errp, "logical_block_size != %u not supported", + XEN_BLKIF_SECTOR_SIZE); + return; + } + if (conf->logical_block_size > conf->physical_block_size) { error_setg( errp, "logical_block_size > physical_block_size not supported"); @@ -253,7 +259,7 @@ static void xen_block_realize(XenDevice *xendev, Error **errp) blockdev->device_type); xen_device_backend_printf(xendev, "sector-size", "%u", - conf->logical_block_size); + XEN_BLKIF_SECTOR_SIZE); xen_block_set_size(blockdev); diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h index 3e6e1ea365..a353693ea0 100644 --- a/hw/block/xen_blkif.h +++ b/hw/block/xen_blkif.h @@ -143,4 +143,6 @@ static inline void blkif_get_x86_64_req(blkif_request_t *dst, } } +#define XEN_BLKIF_SECTOR_SIZE 512 + #endif /* XEN_BLKIF_H */