...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(-)

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);
 
-- 
2.20.1.2.gb21ebb6

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,

-- 
Anthony PERARD

> -----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