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

[Fabio Fantoni <fabio.fantoni@m2r.biz>]

Il 29/04/2014 12:35, Olaf Hering ha scritto:
> 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.
>
> 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>
> ---
>   docs/misc/xl-disk-configuration.txt | 15 +++++++++++++++
>   tools/libxl/check-xl-disk-parse     | 21 ++++++++++++++-------
>   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, 53 insertions(+), 7 deletions(-)

Thanks for add discard (trim) support on libxl, from a quick look to 
this patch, however, seems missing adding the parameter to qemu disks 
that use qdisk (in libxl_dm.c: -drive ...,discard=on).

Thanks for any reply and sorry for my bad english.

> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
> index cf22397..b4a9376 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -178,6 +178,21 @@ information to be interpreted by the executable program <script>,
>   These scripts are normally called "block-<script>".
>   
>   
> +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 797277c..7ec7c83 100755
> --- a/tools/libxl/check-xl-disk-parse
> +++ b/tools/libxl/check-xl-disk-parse
> @@ -61,7 +61,8 @@ disk: {
>       "script": null,
>       "removable": 0,
>       "readwrite": 1,
> -    "is_cdrom": 0
> +    "is_cdrom": 0,
> +    "discard_enable": "True"
>   }
>   
>   END
> @@ -82,7 +83,8 @@ disk: {
>       "script": null,
>       "removable": 1,
>       "readwrite": 0,
> -    "is_cdrom": 1
> +    "is_cdrom": 1,
> +    "discard_enable": "False"
>   }
>   
>   END
> @@ -104,7 +106,8 @@ disk: {
>       "script": null,
>       "removable": 0,
>       "readwrite": 1,
> -    "is_cdrom": 0
> +    "is_cdrom": 0,
> +    "discard_enable": "True"
>   }
>   
>   EOF
> @@ -121,7 +124,8 @@ disk: {
>       "script": null,
>       "removable": 1,
>       "readwrite": 0,
> -    "is_cdrom": 1
> +    "is_cdrom": 1,
> +    "discard_enable": "False"
>   }
>   
>   EOF
> @@ -142,7 +146,8 @@ disk: {
>       "script": null,
>       "removable": 1,
>       "readwrite": 0,
> -    "is_cdrom": 1
> +    "is_cdrom": 1,
> +    "discard_enable": "False"
>   }
>   
>   EOF
> @@ -160,7 +165,8 @@ disk: {
>       "script": "block-iscsi",
>       "removable": 0,
>       "readwrite": 1,
> -    "is_cdrom": 0
> +    "is_cdrom": 0,
> +    "discard_enable": "True"
>   }
>   
>   EOF
> @@ -180,7 +186,8 @@ disk: {
>       "script": "block-drbd",
>       "removable": 0,
>       "readwrite": 1,
> -    "is_cdrom": 0
> +    "is_cdrom": 0,
> +    "discard_enable": "True"
>   }
>   
>   EOF
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 30b0b06..236a3f7 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2205,6 +2205,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
>           flexarray_append(back, disk->readwrite ? "w" : "r");
>           flexarray_append(back, "device-type");
>           flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
> +        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 b2c3015..ea94895 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -95,6 +95,11 @@
>   #define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 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 0f7bbf8..80d00a7 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -416,6 +416,7 @@ libxl_device_disk = Struct("device_disk", [
>       ("removable", integer),
>       ("readwrite", integer),
>       ("is_cdrom", integer),
> +    ("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 7c4e7f1..2585bee 100644
> --- a/tools/libxl/libxlu_disk_l.l
> +++ b/tools/libxl/libxlu_disk_l.l
> @@ -173,6 +173,10 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
>   
>   vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
>   script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
> +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); }
>   
>    /* 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 c3e169c..032c804 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -175,6 +175,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.
> + *
>    * discard-alignment
>    *      Values:         <uint32_t>
>    *      Default Value:  0
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel