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

[Olaf Hering <olaf@aepfle.de>]

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>