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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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