|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4] libxl: add option for discard support to xl disk configuration
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@xxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |