Xen project Mailing List

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

To: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>

From: Olaf Hering <olaf@xxxxxxxxx>

Date: Fri, 9 May 2014 14:13:56 +0200

Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>, Ian Campbell <ian.campbell@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxx

Delivery-date: Fri, 09 May 2014 12:14:08 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

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> _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.