| Message ID | 20190320142825.24565-1-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen-block: only advertize discard to the frontend when it is enabled... | expand |
On Wed, Mar 20, 2019 at 02:28:25PM +0000, Paul Durrant wrote: > ...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_driver_create() correctly parses this and uses it to set typo: It's xen_block_drive_create (s/driver/drive/), otherwise my IDE can't find it :-(. > 'discard' to 'unamp' for the file_layer, but fails to do the same for the Looks like s/unamp/unmap/ > 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 That doesn't match the code I'm reading, before the patch apply. if (discard_granularity > 0) feature-discard=1 Nothing seems to set discard_granularity, so it keeps it's default to -1, so .... wait, discard_granularity is unsigned :-( The description is fine then. > 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 <paul.durrant@citrix.com> With the two typos fixed (which I can try to remember to do on commit): Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
> -----Original Message----- > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > Sent: 21 March 2019 11:42 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; qemu-block@nongnu.org; qemu-devel@nongnu.org; Stefano Stabellini > <sstabellini@kernel.org>; Kevin Wolf <kwolf@redhat.com>; Max Reitz <mreitz@redhat.com> > Subject: Re: [PATCH] xen-block: only advertize discard to the frontend when it is enabled... > > On Wed, Mar 20, 2019 at 02:28:25PM +0000, Paul Durrant wrote: > > ...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_driver_create() correctly parses this and uses it to set > > typo: It's xen_block_drive_create (s/driver/drive/), otherwise my IDE > can't find it :-(. Sorry, my fingers are so used to typing 'driver' > > > 'discard' to 'unamp' for the file_layer, but fails to do the same for the > > Looks like s/unamp/unmap/ Yes. > > > 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 > > That doesn't match the code I'm reading, before the patch apply. > if (discard_granularity > 0) feature-discard=1 > Nothing seems to set discard_granularity, so it keeps it's default to > -1, so .... wait, discard_granularity is unsigned :-( > > The description is fine then. Indeed. > > > 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 <paul.durrant@citrix.com> > > With the two typos fixed (which I can try to remember to do on commit): > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Ok, thanks. Paul > > Thanks, > > -- > Anthony PERARD
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index 70fc2455e8..a848849f48 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);
...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_driver_create() correctly parses this and uses it to set 'discard' to 'unamp' 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 <paul.durrant@citrix.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Anthony Perard <anthony.perard@citrix.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> --- hw/block/xen-block.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)