[PATCH v4] libxl: add option for discard support to xl disk configuration

[PATCH v4] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

Handle new option discard=on|off for disk configuration. It is supposed
to disable discard support if file based backing storage was
intentionally created non-sparse to avoid fragmentation of the file.

The option is a boolean and intended for the backend driver. A new
boolean property "discard-enable" is written to the backend node. An
upcoming patch for qemu will make use of this property. The kernel
blkback driver may be updated as well to disable discard for phy based
backing storage.

v4:
rebase ontop of commit 6ec48cf4 (direct_io_safe)
rebase ontop of fixup commit for 6ec48cf4 (direct_io_safe)
add testcases to tools/libxl/check-xl-disk-parse
v3:
enable discard unconditionally by always writing discard-enable=1 to xenstore
fix typos in xl-disk-configuration.txt
update description in blkif.h, property should be ignored if unsupported
v2:
rename xenstore property from discard_enable to discard-enable
update description in xl-disk-configuration.txt
use libxl_defbool as type for discard_enable
update check-xl-disk-parse to use "<default>"
add LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE to libxl.h

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 docs/misc/xl-disk-configuration.txt | 15 +++++++
 tools/libxl/check-xl-disk-parse     | 80 +++++++++++++++++++++++++++++++++----
 tools/libxl/libxl.c                 |  2 +
 tools/libxl/libxl.h                 |  5 +++
 tools/libxl/libxl_types.idl         |  1 +
 tools/libxl/libxlu_disk.c           |  2 +
 tools/libxl/libxlu_disk_l.l         |  4 ++
 xen/include/public/io/blkif.h       | 10 +++++
 8 files changed, 112 insertions(+), 7 deletions(-)

diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
index 11fee9a..e2a56f3 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -217,6 +217,21 @@ If in the future the bug is fixed properly this option will then be
 silently ignored.
 
 
+discard=<boolean>
+---------------
+
+Description:           Instruct backend to advertise discard support to frontend
+Supported values:      on, off, 0, 1
+Mandatory:             No
+Default value:         on
+
+This option is an advisory setting for the backend driver, depending on the
+value, to advertise discard support (TRIM, UNMAP) to the frontend.  The real
+benefit of this option is to be able to force it off rather than on. It allows
+to disable "hole punching" for file based backends which were intentionally
+created non-sparse to avoid fragmentation of the file.
+
+
 ============================================
 DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES
 ============================================
diff --git a/tools/libxl/check-xl-disk-parse b/tools/libxl/check-xl-disk-parse
index 0698586..c564e01 100755
--- a/tools/libxl/check-xl-disk-parse
+++ b/tools/libxl/check-xl-disk-parse
@@ -62,7 +62,8 @@ disk: {
     "removable": 0,
     "readwrite": 1,
     "is_cdrom": 0,
-    "direct_io_safe": false
+    "direct_io_safe": false,
+    "discard_enable": "True"
 }
 
 END
@@ -84,7 +85,8 @@ disk: {
     "removable": 1,
     "readwrite": 0,
     "is_cdrom": 1,
-    "direct_io_safe": false
+    "direct_io_safe": false,
+    "discard_enable": "False"
 }
 
 END
@@ -107,7 +109,8 @@ disk: {
     "removable": 0,
     "readwrite": 1,
     "is_cdrom": 0,
-    "direct_io_safe": false
+    "direct_io_safe": false,
+    "discard_enable": "True"
 }
 
 EOF
@@ -125,7 +128,8 @@ disk: {
     "removable": 1,
     "readwrite": 0,
     "is_cdrom": 1,
-    "direct_io_safe": false
+    "direct_io_safe": false,
+    "discard_enable": "False"
 }
 
 EOF
@@ -147,7 +151,8 @@ disk: {
     "removable": 1,
     "readwrite": 0,
     "is_cdrom": 1,
-    "direct_io_safe": false
+    "direct_io_safe": false,
+    "discard_enable": "False"
 }
 
 EOF
@@ -166,7 +171,8 @@ disk: {
     "removable": 0,
     "readwrite": 1,
     "is_cdrom": 0,
-    "direct_io_safe": false
+    "direct_io_safe": false,
+    "discard_enable": "True"
 }
 
 EOF
@@ -187,7 +193,8 @@ disk: {
     "removable": 0,
     "readwrite": 1,
     "is_cdrom": 0,
-    "direct_io_safe": false
+    "direct_io_safe": false,
+    "discard_enable": "True"
 }
 
 EOF
@@ -196,4 +203,63 @@ EOF
 # http://www.drbd.org/users-guide-emb/s-xen-configure-domu.html
 one 0 drbd:app01,hda,w
 
+expected <<END
+disk: {
+    "backend_domid": 0,
+    "backend_domname": null,
+    "pdev_path": "/some/disk/image.raw",
+    "vdev": "hda",
+    "backend": "unknown",
+    "format": "raw",
+    "script": null,
+    "removable": 0,
+    "readwrite": 1,
+    "is_cdrom": 0,
+    "direct_io_safe": false,
+    "discard_enable": "True"
+}
+
+END
+one 0  discard=on  vdev=hda target=/some/disk/image.raw
+one 0  discard=1   vdev=hda target=/some/disk/image.raw
+
+expected <<END
+disk: {
+    "backend_domid": 0,
+    "backend_domname": null,
+    "pdev_path": "/some/disk/image.raw",
+    "vdev": "hda",
+    "backend": "unknown",
+    "format": "raw",
+    "script": null,
+    "removable": 0,
+    "readwrite": 1,
+    "is_cdrom": 0,
+    "direct_io_safe": false,
+    "discard_enable": "False"
+}
+
+END
+one 0  discard=off vdev=hda target=/some/disk/image.raw
+one 0  discard=0   vdev=hda target=/some/disk/image.raw
+
+expected <<END
+disk: {
+    "backend_domid": 0,
+    "backend_domname": null,
+    "pdev_path": "/some/disk/image.iso",
+    "vdev": "hda",
+    "backend": "unknown",
+    "format": "raw",
+    "script": null,
+    "removable": 1,
+    "readwrite": 0,
+    "is_cdrom": 1,
+    "direct_io_safe": false,
+    "discard_enable": "False"
+}
+
+END
+one 0  cdrom discard=off vdev=hda target=/some/disk/image.iso
+
 complete
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d59ce0c..2b7352c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2209,6 +2209,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
             flexarray_append(back, "direct-io-safe");
             flexarray_append(back, "1");
         }
+        flexarray_append_pair(back, "discard-enable",
+                              libxl_defbool_val(disk->discard_enable) ? "1" : "0");
 
         flexarray_append(front, "backend-id");
         flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 84f9c0e..c7aa817 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -102,6 +102,11 @@
 #define LIBXL_HAVE_DEVICE_DISK_DIRECT_IO_SAFE 1
 
 /*
+ * The libxl_device_disk has the discard_enable field.
+ */
+#define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 8944686..52f1aa9 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -417,6 +417,7 @@ libxl_device_disk = Struct("device_disk", [
     ("readwrite", integer),
     ("is_cdrom", integer),
     ("direct_io_safe", bool),
+    ("discard_enable", libxl_defbool),
     ])
 
 libxl_device_nic = Struct("device_nic", [
diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
index 18fe386..5827025 100644
--- a/tools/libxl/libxlu_disk.c
+++ b/tools/libxl/libxlu_disk.c
@@ -79,6 +79,8 @@ int xlu_disk_parse(XLU_Config *cfg,
         if (!disk->pdev_path || !strcmp(disk->pdev_path, ""))
             disk->format = LIBXL_DISK_FORMAT_EMPTY;
     }
+    if (libxl_defbool_is_default(disk->discard_enable))
+        libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);
 
     if (!disk->vdev) {
         xlu__disk_err(&dpc,0, "no vdev specified");
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index ba8577c..7fb378a 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -174,6 +174,10 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
 vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
 script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
 direct-io-safe,? { DPC->disk->direct_io_safe = 1; }
+discard=on,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
+discard=1,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
+discard=off,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
+discard=0,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
 
  /* the target magic parameter, eats the rest of the string */
 
diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 1e7cea9..27a88ba 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -197,6 +197,16 @@
  *
  *------------------------- Backend Device Properties -------------------------
  *
+ * discard-enable
+ *      Values:         0/1 (boolean)
+ *      Default Value:  1
+ *
+ *      This optional property, set by the toolstack, instructs the backend to
+ *      offer discard to the frontend. If the property is missing the backend
+ *      should offer discard if the backing storage actually supports it. The
+ *      backend driver should ignore the property if the property is set but
+ *      the backing storage does not support the feature.
+ *
  * discard-alignment
  *      Values:         <uint32_t>
  *      Default Value:  0

Re: [PATCH v4] libxl: add option for discard support to xl disk configuration

[Konrad Rzeszutek Wilk]

On Mon, May 05, 2014 at 04:04:21PM +0200, Olaf Hering wrote:
> Handle new option discard=on|off for disk configuration. It is supposed
> to disable discard support if file based backing storage was
> intentionally created non-sparse to avoid fragmentation of the file.
> 
> The option is a boolean and intended for the backend driver. A new
> boolean property "discard-enable" is written to the backend node. An
> upcoming patch for qemu will make use of this property. The kernel
> blkback driver may be updated as well to disable discard for phy based
> backing storage.
> 
> v4:
> rebase ontop of commit 6ec48cf4 (direct_io_safe)
> rebase ontop of fixup commit for 6ec48cf4 (direct_io_safe)
> add testcases to tools/libxl/check-xl-disk-parse
> v3:
> enable discard unconditionally by always writing discard-enable=1 to xenstore
> fix typos in xl-disk-configuration.txt
> update description in blkif.h, property should be ignored if unsupported
> v2:
> rename xenstore property from discard_enable to discard-enable
> update description in xl-disk-configuration.txt
> use libxl_defbool as type for discard_enable
> update check-xl-disk-parse to use "<default>"
> add LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE to libxl.h
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  docs/misc/xl-disk-configuration.txt | 15 +++++++
>  tools/libxl/check-xl-disk-parse     | 80 +++++++++++++++++++++++++++++++++----
>  tools/libxl/libxl.c                 |  2 +
>  tools/libxl/libxl.h                 |  5 +++
>  tools/libxl/libxl_types.idl         |  1 +
>  tools/libxl/libxlu_disk.c           |  2 +
>  tools/libxl/libxlu_disk_l.l         |  4 ++
>  xen/include/public/io/blkif.h       | 10 +++++
>  8 files changed, 112 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
> index 11fee9a..e2a56f3 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -217,6 +217,21 @@ If in the future the bug is fixed properly this option will then be
>  silently ignored.
>  
>  
> +discard=<boolean>
> +---------------
> +
> +Description:           Instruct backend to advertise discard support to frontend
> +Supported values:      on, off, 0, 1
> +Mandatory:             No
> +Default value:         on
> +
> +This option is an advisory setting for the backend driver, depending on the
> +value, to advertise discard support (TRIM, UNMAP) to the frontend.  The real
> +benefit of this option is to be able to force it off rather than on. It allows
> +to disable "hole punching" for file based backends which were intentionally
> +created non-sparse to avoid fragmentation of the file.
> +
> +
>  ============================================
>  DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES
>  ============================================
> diff --git a/tools/libxl/check-xl-disk-parse b/tools/libxl/check-xl-disk-parse
> index 0698586..c564e01 100755
> --- a/tools/libxl/check-xl-disk-parse
> +++ b/tools/libxl/check-xl-disk-parse
> @@ -62,7 +62,8 @@ disk: {
>      "removable": 0,
>      "readwrite": 1,
>      "is_cdrom": 0,
> -    "direct_io_safe": false
> +    "direct_io_safe": false,
> +    "discard_enable": "True"
>  }
>  
>  END
> @@ -84,7 +85,8 @@ disk: {
>      "removable": 1,
>      "readwrite": 0,
>      "is_cdrom": 1,
> -    "direct_io_safe": false
> +    "direct_io_safe": false,
> +    "discard_enable": "False"
>  }
>  
>  END
> @@ -107,7 +109,8 @@ disk: {
>      "removable": 0,
>      "readwrite": 1,
>      "is_cdrom": 0,
> -    "direct_io_safe": false
> +    "direct_io_safe": false,
> +    "discard_enable": "True"
>  }
>  
>  EOF
> @@ -125,7 +128,8 @@ disk: {
>      "removable": 1,
>      "readwrite": 0,
>      "is_cdrom": 1,
> -    "direct_io_safe": false
> +    "direct_io_safe": false,
> +    "discard_enable": "False"
>  }
>  
>  EOF
> @@ -147,7 +151,8 @@ disk: {
>      "removable": 1,
>      "readwrite": 0,
>      "is_cdrom": 1,
> -    "direct_io_safe": false
> +    "direct_io_safe": false,
> +    "discard_enable": "False"
>  }
>  
>  EOF
> @@ -166,7 +171,8 @@ disk: {
>      "removable": 0,
>      "readwrite": 1,
>      "is_cdrom": 0,
> -    "direct_io_safe": false
> +    "direct_io_safe": false,
> +    "discard_enable": "True"
>  }
>  
>  EOF
> @@ -187,7 +193,8 @@ disk: {
>      "removable": 0,
>      "readwrite": 1,
>      "is_cdrom": 0,
> -    "direct_io_safe": false
> +    "direct_io_safe": false,
> +    "discard_enable": "True"
>  }
>  
>  EOF
> @@ -196,4 +203,63 @@ EOF
>  # http://www.drbd.org/users-guide-emb/s-xen-configure-domu.html
>  one 0 drbd:app01,hda,w
>  
> +expected <<END
> +disk: {
> +    "backend_domid": 0,
> +    "backend_domname": null,
> +    "pdev_path": "/some/disk/image.raw",
> +    "vdev": "hda",
> +    "backend": "unknown",
> +    "format": "raw",
> +    "script": null,
> +    "removable": 0,
> +    "readwrite": 1,
> +    "is_cdrom": 0,
> +    "direct_io_safe": false,
> +    "discard_enable": "True"
> +}
> +
> +END
> +one 0  discard=on  vdev=hda target=/some/disk/image.raw
> +one 0  discard=1   vdev=hda target=/some/disk/image.raw
> +
> +expected <<END
> +disk: {
> +    "backend_domid": 0,
> +    "backend_domname": null,
> +    "pdev_path": "/some/disk/image.raw",
> +    "vdev": "hda",
> +    "backend": "unknown",
> +    "format": "raw",
> +    "script": null,
> +    "removable": 0,
> +    "readwrite": 1,
> +    "is_cdrom": 0,
> +    "direct_io_safe": false,
> +    "discard_enable": "False"
> +}
> +
> +END
> +one 0  discard=off vdev=hda target=/some/disk/image.raw
> +one 0  discard=0   vdev=hda target=/some/disk/image.raw
> +
> +expected <<END
> +disk: {
> +    "backend_domid": 0,
> +    "backend_domname": null,
> +    "pdev_path": "/some/disk/image.iso",
> +    "vdev": "hda",
> +    "backend": "unknown",
> +    "format": "raw",
> +    "script": null,
> +    "removable": 1,
> +    "readwrite": 0,
> +    "is_cdrom": 1,
> +    "direct_io_safe": false,
> +    "discard_enable": "False"
> +}
> +
> +END
> +one 0  cdrom discard=off vdev=hda target=/some/disk/image.iso
> +
>  complete
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d59ce0c..2b7352c 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2209,6 +2209,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
>              flexarray_append(back, "direct-io-safe");
>              flexarray_append(back, "1");
>          }
> +        flexarray_append_pair(back, "discard-enable",
> +                              libxl_defbool_val(disk->discard_enable) ? "1" : "0");
>  
>          flexarray_append(front, "backend-id");
>          flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 84f9c0e..c7aa817 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -102,6 +102,11 @@
>  #define LIBXL_HAVE_DEVICE_DISK_DIRECT_IO_SAFE 1
>  
>  /*
> + * The libxl_device_disk has the discard_enable field.
> + */
> +#define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 8944686..52f1aa9 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -417,6 +417,7 @@ libxl_device_disk = Struct("device_disk", [
>      ("readwrite", integer),
>      ("is_cdrom", integer),
>      ("direct_io_safe", bool),
> +    ("discard_enable", libxl_defbool),
>      ])
>  
>  libxl_device_nic = Struct("device_nic", [
> diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
> index 18fe386..5827025 100644
> --- a/tools/libxl/libxlu_disk.c
> +++ b/tools/libxl/libxlu_disk.c
> @@ -79,6 +79,8 @@ int xlu_disk_parse(XLU_Config *cfg,
>          if (!disk->pdev_path || !strcmp(disk->pdev_path, ""))
>              disk->format = LIBXL_DISK_FORMAT_EMPTY;
>      }
> +    if (libxl_defbool_is_default(disk->discard_enable))
> +        libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);
>  
>      if (!disk->vdev) {
>          xlu__disk_err(&dpc,0, "no vdev specified");
> diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
> index ba8577c..7fb378a 100644
> --- a/tools/libxl/libxlu_disk_l.l
> +++ b/tools/libxl/libxlu_disk_l.l
> @@ -174,6 +174,10 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
>  vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
>  script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
>  direct-io-safe,? { DPC->disk->direct_io_safe = 1; }
> +discard=on,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
> +discard=1,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
> +discard=off,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
> +discard=0,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }

I think this automatically generated?

>  
>   /* the target magic parameter, eats the rest of the string */
>  
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 1e7cea9..27a88ba 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -197,6 +197,16 @@
>   *
>   *------------------------- Backend Device Properties -------------------------
>   *
> + * discard-enable
> + *      Values:         0/1 (boolean)
> + *      Default Value:  1
> + *
> + *      This optional property, set by the toolstack, instructs the backend to
> + *      offer discard to the frontend. If the property is missing the backend
> + *      should offer discard if the backing storage actually supports it. The


> + *      backend driver should ignore the property if the property is set but
> + *      the backing storage does not support the feature.

? I think you want to say: "The backend driver should ignore the propery
if the backing storage does not expose this functionality."

> + *
>   * discard-alignment
>   *      Values:         <uint32_t>
>   *      Default Value:  0
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v4] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Mon, May 05, Konrad Rzeszutek Wilk wrote:

> > +++ b/tools/libxl/libxlu_disk_l.l
> > @@ -174,6 +174,10 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
> >  vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
> >  script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
> >  direct-io-safe,? { DPC->disk->direct_io_safe = 1; }
> > +discard=on,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
> > +discard=1,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
> > +discard=off,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
> > +discard=0,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
> 
> I think this automatically generated?

Yes, bison/flex should be rerun after applying this change.

> > + *      backend driver should ignore the property if the property is set but
> > + *      the backing storage does not support the feature.
> ? I think you want to say: "The backend driver should ignore the propery
> if the backing storage does not expose this functionality."

That would be ok as well, if your version is better english anyway.


Olaf

Re: [PATCH v4] libxl: add option for discard support to xl disk configuration

[Ian Jackson]

Olaf Hering writes ("[PATCH v4] libxl: add option for discard support to xl disk configuration"):
> Handle new option discard=on|off for disk configuration. It is supposed
> to disable discard support if file based backing storage was
> intentionally created non-sparse to avoid fragmentation of the file.
> 
> The option is a boolean and intended for the backend driver. A new
> boolean property "discard-enable" is written to the backend node. An
> upcoming patch for qemu will make use of this property. The kernel
> blkback driver may be updated as well to disable discard for phy based
> backing storage.

Thanks.  This is pretty good, but I have some minor comments and
questions:

> +++ b/docs/misc/xl-disk-configuration.txt
...
> +discard=<boolean>
> +---------------
> +
> +Description:           Instruct backend to advertise discard support to frontend

I think you mean "Request" rather than "Instruct".  Since the request
may or may not be honoured.

> +Supported values:      on, off, 0, 1
> +Mandatory:             No
> +Default value:         on

We have two other roughly boolean configuration parameters, and
neither of them use "thing=0" or "thing=on":
disk->is_cdrom is set by simply saying "cdrom".
disk->direct_io_safe is set by simply saying "direct-io-safe".

I think we should follow this pattern.

This new option needs a negative, though, since the default is on.  I
suggest:
  discard
  no-discard
?

> +This option is an advisory setting for the backend driver,
> +depending on the value, to advertise discard support (TRIM, UNMAP)
> +to the frontend.  The real benefit of this option is to be able to
> +force it off rather than on. It allows to disable "hole punching"
> +for file based backends which were intentionally created non-sparse
> +to avoid fragmentation of the file.

This reads rather oddly to a native speaker.  Let me rephrase for you:

   An advisory setting for the backend driver, specifying whether, to
   advertise discard support (TRIM, UNMAP) to the frontend.  The real
   benefit of this option is to be able to force it off rather than
   on.  It can be used to disable "hole punching" for file based
   backends which were intentionally created non-sparse to avoid
   fragmentation of the file.

> +++ b/tools/libxl/libxl.c
...
> +        flexarray_append_pair(back, "discard-enable",
> +                              libxl_defbool_val(disk->discard_enable) ? "1" : "0");

This line needs to be wrapped.


> +++ b/tools/libxl/libxl.h
...
> +    if (libxl_defbool_is_default(disk->discard_enable))
> +        libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);

You can call setdefault unconditionally.

> + * discard-enable
> + *      Values:         0/1 (boolean)
> + *      Default Value:  1
> + *
> + *      This optional property, set by the toolstack, instructs the backend\
 to
> + *      offer discard to the frontend. If the property is missing the backe\
nd
> + *      should offer discard if the backing storage actually supports it. T\
he
> + *      backend driver should ignore the property if the property is set bu\
t
> + *      the backing storage does not support the feature.

I think it would be helpful to wrap this a bit tighter.  You can see
the wrap damage.

I agree with Konrad's comments about the last sentence.

I think here you want to say "request" again rather than "instruct".
Also, you need to make it go both ways.  I would suggest

   This optional property, set by the toolstack, requests that the
   backend offer, or not offer, discard to the frontend.

Thanks,
Ian.

Re: [PATCH v4] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Thu, May 08, Ian Jackson wrote:

> Olaf Hering writes ("[PATCH v4] libxl: add option for discard support to xl disk configuration"):
> > Handle new option discard=on|off for disk configuration. It is supposed
> > to disable discard support if file based backing storage was
> > intentionally created non-sparse to avoid fragmentation of the file.
> > 
> > The option is a boolean and intended for the backend driver. A new
> > boolean property "discard-enable" is written to the backend node. An
> > upcoming patch for qemu will make use of this property. The kernel
> > blkback driver may be updated as well to disable discard for phy based
> > backing storage.
> 
> Thanks.  This is pretty good, but I have some minor comments and
> questions:

Thanks for the review. I sent v5, which contains this change compared to
v4.

Olaf

 docs/misc/xl-disk-configuration.txt | 22 ++++++++++++----------
 tools/libxl/check-xl-disk-parse     |  6 +++---
 tools/libxl/libxl.c                 |  3 ++-
 tools/libxl/libxlu_disk.c           |  3 +--
 tools/libxl/libxlu_disk_l.l         |  6 ++----
 xen/include/public/io/blkif.h       | 10 +++++-----
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
index e2a56f3..8421e8a 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -217,19 +217,21 @@ If in the future the bug is fixed properly this option will then be
 silently ignored.
 
 
-discard=<boolean>
+discard / no-discard
 ---------------
 
-Description:           Instruct backend to advertise discard support to frontend
-Supported values:      on, off, 0, 1
+Description:           Request backend to advertise discard support to frontend
+Supported values:      discard
+                       no-discard
 Mandatory:             No
-Default value:         on
-
-This option is an advisory setting for the backend driver, depending on the
-value, to advertise discard support (TRIM, UNMAP) to the frontend.  The real
-benefit of this option is to be able to force it off rather than on. It allows
-to disable "hole punching" for file based backends which were intentionally
-created non-sparse to avoid fragmentation of the file.
+Default value:         discard
+
+An advisory setting for the backend driver, specifying whether, to
+advertise discard support (TRIM, UNMAP) to the frontend.  The real
+benefit of this option is to be able to force it off rather than on.  It
+can be used to disable "hole punching" for file based backends which
+were intentionally created non-sparse to avoid fragmentation of the
+file.
 
 
 ============================================
diff --git a/tools/libxl/check-xl-disk-parse b/tools/libxl/check-xl-disk-parse
index c564e01..1bec4ca 100755
--- a/tools/libxl/check-xl-disk-parse
+++ b/tools/libxl/check-xl-disk-parse
@@ -240,8 +240,8 @@ disk: {
 }
 
 END
-one 0  discard=off vdev=hda target=/some/disk/image.raw
-one 0  discard=0   vdev=hda target=/some/disk/image.raw
+one 0  discard vdev=hda target=/some/disk/image.raw
+one 0  discard vdev=hda target=/some/disk/image.raw
 
 expected <<END
 disk: {
@@ -260,6 +260,6 @@ disk: {
 }
 
 END
-one 0  cdrom discard=off vdev=hda target=/some/disk/image.iso
+one 0  cdrom no-discard vdev=hda target=/some/disk/image.iso
 
 complete
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1b76a25..c3a9236 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2477,7 +2477,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
             flexarray_append(back, "1");
         }
         flexarray_append_pair(back, "discard-enable",
-                              libxl_defbool_val(disk->discard_enable) ? "1" : "0");
+                              libxl_defbool_val(disk->discard_enable) ?
+                              "1" : "0");
 
         flexarray_append(front, "backend-id");
         flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
index 5827025..752a2c7 100644
--- a/tools/libxl/libxlu_disk.c
+++ b/tools/libxl/libxlu_disk.c
@@ -79,8 +79,7 @@ int xlu_disk_parse(XLU_Config *cfg,
         if (!disk->pdev_path || !strcmp(disk->pdev_path, ""))
             disk->format = LIBXL_DISK_FORMAT_EMPTY;
     }
-    if (libxl_defbool_is_default(disk->discard_enable))
-        libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);
+    libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);
 
     if (!disk->vdev) {
         xlu__disk_err(&dpc,0, "no vdev specified");
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index 7fb378a..1a5deb5 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -174,10 +174,8 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
 vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
 script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
 direct-io-safe,? { DPC->disk->direct_io_safe = 1; }
-discard=on,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
-discard=1,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
-discard=off,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
-discard=0,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
+discard,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
+no-discard,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
 
  /* the target magic parameter, eats the rest of the string */
 
diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 27a88ba..6baf7fb 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -201,11 +201,11 @@
  *      Values:         0/1 (boolean)
  *      Default Value:  1
  *
- *      This optional property, set by the toolstack, instructs the backend to
- *      offer discard to the frontend. If the property is missing the backend
- *      should offer discard if the backing storage actually supports it. The
- *      backend driver should ignore the property if the property is set but
- *      the backing storage does not support the feature.
+ *      This optional property, set by the toolstack, instructs the backend
+ *      to offer discard to the frontend. If the property is missing the
+ *      backend should offer discard if the backing storage actually supports
+ *      it. This optional property, set by the toolstack, requests that the
+ *      backend offer, or not offer, discard to the frontend.
  *
  * discard-alignment
  *      Values:         <uint32_t>