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

* [PATCH v5] libxl: add option for discard support to xl disk configuration
@ 2014-05-09 12:09 Olaf Hering
  2014-05-09 12:18 ` Olaf Hering
  2014-05-12 14:29 ` Ian Jackson
  0 siblings, 2 replies; 13+ messages in thread
From: Olaf Hering @ 2014-05-09 12:09 UTC (permalink / raw)
  To: xen-devel; +Cc: Olaf Hering, ian.jackson, ian.campbell, stefano.stabellini

Handle new boolean option discard/no-discard 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 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.

v5:
replace option discard=0|1 with boolean discard/no-discard
adjust wording in xl-disk-configuration.txt and blkif.h
call libxl_defbool_setdefault directly
adjust linewarapping in device_disk_add
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>
---

Please rerun bison/flex due to changes to .l files
Also note the change to the .idl file


 docs/misc/xl-disk-configuration.txt | 17 ++++++++
 tools/libxl/check-xl-disk-parse     | 80 +++++++++++++++++++++++++++++++++----
 tools/libxl/libxl.c                 |  3 ++
 tools/libxl/libxl.h                 |  5 +++
 tools/libxl/libxl_types.idl         |  1 +
 tools/libxl/libxlu_disk.c           |  1 +
 tools/libxl/libxlu_disk_l.l         |  2 +
 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..8421e8a 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -217,6 +217,23 @@ If in the future the bug is fixed properly this option will then be
 silently ignored.
 
 
+discard / no-discard
+---------------
+
+Description:           Request backend to advertise discard support to frontend
+Supported values:      discard
+                       no-discard
+Mandatory:             No
+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.
+
+
 ============================================
 DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES
 ============================================
diff --git a/tools/libxl/check-xl-disk-parse b/tools/libxl/check-xl-disk-parse
index 0698586..1bec4ca 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 vdev=hda target=/some/disk/image.raw
+one 0  discard 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 no-discard vdev=hda target=/some/disk/image.iso
+
 complete
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2a876b9..c3a9236 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2476,6 +2476,9 @@ 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 872a618..6cca2ef 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 81471c8..c2bee07 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..752a2c7 100644
--- a/tools/libxl/libxlu_disk.c
+++ b/tools/libxl/libxlu_disk.c
@@ -79,6 +79,7 @@ int xlu_disk_parse(XLU_Config *cfg,
         if (!disk->pdev_path || !strcmp(disk->pdev_path, ""))
             disk->format = LIBXL_DISK_FORMAT_EMPTY;
     }
+    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..1a5deb5 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -174,6 +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,?	{ 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 1e7cea9..6baf7fb 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. This optional property, set by the toolstack, requests that the
+ *      backend offer, or not offer, discard to the frontend.
+ *
  * discard-alignment
  *      Values:         <uint32_t>
  *      Default Value:  0

^ permalink raw reply related	[flat|nested] 13+ messages in thread