Re: [PATCH libvirt 5/8] storage: add preallocation element

Christophe Fergeau <cfergeau@xxxxxxxxxx> · Wed, 16 May 2012 12:11:24 +0200

On Wed, May 16, 2012 at 12:55:12AM +0200, Marc-André Lureau wrote:
> Allow to specify preallocation mode for QCOW2 images.
> If not specified or not available, it's ignored.
> 
> This change only modify the schema, doc, parsing and tests.
> ---
>  docs/formatstorage.html.in               |    6 ++++++
>  docs/schemas/storagevol.rng              |   18 ++++++++++++++++++
>  src/conf/storage_conf.c                  |   26 ++++++++++++++++++++++++++
>  src/conf/storage_conf.h                  |    1 +
>  src/util/storage_file.c                  |    4 ++++
>  src/util/storage_file.h                  |   10 ++++++++++
>  tests/storagevolxml2xmlin/vol-qcow2.xml  |    1 +
>  tests/storagevolxml2xmlout/vol-qcow2.xml |    1 +
>  8 files changed, 67 insertions(+)
> 
> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> index d0e4319..8fd2908 100644
> --- a/docs/formatstorage.html.in
> +++ b/docs/formatstorage.html.in
> @@ -252,6 +252,12 @@
>          1,152,921,504,606,846,976 bytes).  <span class="since">Since
>          0.4.1, multi-character <code>unit</code> since
>          0.9.11</span></dd>
> +      <dt><code>preallocation</code></dt>
> +      <dd>An image with preallocated metadata is initially larger but
> +        can improve performance when the image needs to grow. This is
> +        supported by the QCOW2 image format.
> +        Attribute <code>mode</code> value can be 'off' or 'metadata'.
> +        <span class="since">Since 0.9.13</span></dd>

Sorry for not catching this earlier, but I'd also add a
&lt;preallocation mode='metadata'/&gt; line to the

    <pre>
      &lt;volume&gt;
      &lt;name&gt;sparse.img&lt;/name&gt;
      &lt;key&gt;/var/lib/xen/images/sparse.img&lt;/key&gt;
      &lt;allocation&gt;0&lt;/allocation&gt;
      &lt;capacity unit="T"&gt;1&lt;/capacity&gt;
      ...</pre>
block above.

> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
> index 530071e..63abe65 100644
> --- a/src/util/storage_file.c
> +++ b/src/util/storage_file.c
> @@ -47,6 +47,10 @@ VIR_ENUM_IMPL(virStorageFileFormat,
>                "cloop", "cow", "dmg", "iso",
>                "qcow", "qcow2", "qed", "vmdk", "vpc")
>  
> +VIR_ENUM_IMPL(virStoragePreallocationMode,
> +              VIR_STORAGE_PREALLOCATION_LAST,
> +              "default", "off", "metadata")
> +
>  enum lv_endian {
>      LV_LITTLE_ENDIAN = 1, /* 1234 */
>      LV_BIG_ENDIAN         /* 4321 */
> diff --git a/src/util/storage_file.h b/src/util/storage_file.h
> index 13d0e87..dfc8719 100644
> --- a/src/util/storage_file.h
> +++ b/src/util/storage_file.h
> @@ -46,6 +46,16 @@ enum virStorageFileFormat {
>  
>  VIR_ENUM_DECL(virStorageFileFormat);
>  
> +enum virStoragePreallocationMode {
> +    VIR_STORAGE_PREALLOCATION_NONE,

Nit: I'd call this DEFAULT to go with the string name, but feel free to
keep things this way.

ACK (though a quick review from someone more familiar with libvirt looking
at the XML changes and the error code that are used wouldn't hurt ;)

Christophe