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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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:

> +++ b/docs/misc/xl-disk-configuration.txt
...
> +discard=<boolean>
> +---------------
> +
> +Description:           Instruct backend to advertise discard support to 
> frontend

I think you mean "Request" rather than "Instruct".  Since the request
may or may not be honoured.

> +Supported values:      on, off, 0, 1
> +Mandatory:             No
> +Default value:         on

We have two other roughly boolean configuration parameters, and
neither of them use "thing=0" or "thing=on":
disk->is_cdrom is set by simply saying "cdrom".
disk->direct_io_safe is set by simply saying "direct-io-safe".

I think we should follow this pattern.

This new option needs a negative, though, since the default is on.  I
suggest:
  discard
  no-discard
?

> +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.

This reads rather oddly to a native speaker.  Let me rephrase for you:

   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.

> +++ b/tools/libxl/libxl.c
...
> +        flexarray_append_pair(back, "discard-enable",
> +                              libxl_defbool_val(disk->discard_enable) ? "1" 
> : "0");

This line needs to be wrapped.


> +++ b/tools/libxl/libxl.h
...
> +    if (libxl_defbool_is_default(disk->discard_enable))
> +        libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);

You can call setdefault unconditionally.

> + * 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 backe\
nd
> + *      should offer discard if the backing storage actually supports it. T\
he
> + *      backend driver should ignore the property if the property is set bu\
t
> + *      the backing storage does not support the feature.

I think it would be helpful to wrap this a bit tighter.  You can see
the wrap damage.

I agree with Konrad's comments about the last sentence.

I think here you want to say "request" again rather than "instruct".
Also, you need to make it go both ways.  I would suggest

   This optional property, set by the toolstack, requests that the
   backend offer, or not offer, discard to the frontend.

Thanks,
Ian.

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