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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-01-28 at 19:24 +0100, 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.
> 
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> ---
>  docs/misc/xl-disk-configuration.txt | 13 +++++++++++++
>  tools/libxl/check-xl-disk-parse     | 21 ++++++++++++++-------
>  tools/libxl/libxl.c                 |  2 ++
>  tools/libxl/libxl_types.idl         |  1 +
>  tools/libxl/libxlu_disk.c           |  1 +
>  tools/libxl/libxlu_disk_i.h         |  2 +-
>  tools/libxl/libxlu_disk_l.l         |  8 ++++++++
>  xen/include/public/io/blkif.h       |  8 ++++++++
>  8 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/misc/xl-disk-configuration.txt 
> b/docs/misc/xl-disk-configuration.txt
> index 5bd456d..4f81394 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -178,6 +178,19 @@ 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

I think this default should be "on if, available for that backend type".

What happens if the backed does not support discard?

> +This option instructs the backend driver, depending of the value, to 
> advertise
> +discard support (TRIM, UNMAP) to the frontend. It allows to disable "hole
> +punching" for file based backends which were intentionally created 
> non-sparse.
> +
> +
>  
>  ============================================
>  DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES

[...]
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 649ce50..b58b198 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -415,6 +415,7 @@ libxl_device_disk = Struct("device_disk", [
>      ("removable", integer),
>      ("readwrite", integer),
>      ("is_cdrom", integer),
> +    ("discard_enable", integer),

I have a feeling this should be a libxl_defbool, to allow for the
possibility of "libxl does what is best/lets the backend decide".

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2845ca4..3633a7d 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2196,6 +2196,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(back, "discard_enable");
> +        flexarray_append(back, libxl__sprintf(gc, "%d", 
> (disk->discard_enable) ? 1 : 0));


And if this were a defbool then you'd want to use libxl_defbool_is_default: i.e.
        if (!libxl_defbool_is_default(disk->discard_enable))
                flexarray_append(back, ..., libxl_defbool_val(...) ? "1" : "0"))

(note the lack of libxl_sprintf here too).

>  
>          flexarray_append(front, "backend-id");
>          flexarray_append(front, libxl__sprintf(gc, "%d", 
> disk->backend_domid));

>      ])
>  
>  libxl_device_nic = Struct("device_nic", [
> diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
> index 18fe386..ee82a8d 100644
> --- a/tools/libxl/libxlu_disk.c
> +++ b/tools/libxl/libxlu_disk.c
> @@ -58,6 +58,7 @@ int xlu_disk_parse(XLU_Config *cfg,
>      dpc.disk = disk;
>  
>      disk->readwrite = 1;
> +    disk->discard_enable = 1; /* Doing it twice?! */

Why?

> diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
> index 7c4e7f1..2afd5e7 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,? { DPC->disk->discard_enable = 1; DPC->discard_set = 1; }
> +discard=1,?  { DPC->disk->discard_enable = 1; DPC->discard_set = 1; }
> +discard=off,?        { DPC->disk->discard_enable = 0; DPC->discard_set = 1; }
> +discard=0,?  { DPC->disk->discard_enable = 0; DPC->discard_set = 1; }
>  
>   /* the target magic parameter, eats the rest of the string */
>  
> @@ -244,6 +248,10 @@ phy:/.*          { DPC->had_depr_prefix=1; DEPRECATE(0); 
> }
>          xlu__disk_err(DPC,yytext,"too many positional parameters");
>          return 0; /* don't print any more errors */
>      }
> +    if (!DPC->discard_set) {
> +        DPC->discard_set = 1;
> +     DPC->disk->discard_enable = 1;

Indentation is messed up here.

> +    }
>  }
>  
>  . {
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 542f123..0121e19 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -175,6 +175,14 @@
>   *
>   *------------------------- Backend Device Properties 
> -------------------------
>   *
> + * discard_enable

All of the existing properties use - not _.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel