[libvirt PATCH 1/2] storage: add support for QCOW2 cluster_size option

Pavel Hrdina posted 2 patches 4 years, 9 months ago
There is a newer version of this series
[libvirt PATCH 1/2] storage: add support for QCOW2 cluster_size option
Posted by Pavel Hrdina 4 years, 9 months ago
The default value hard-coded in QEMU (64KiB) is not always the ideal.
Having a possibility to set the cluster_size by user may in specific
use-cases improve performance for QCOW2 images.

QEMU internally has some limits, the value has to be between 512B and
2048KiB and must by power of two, except when the image has Extended L2
Entries the minimal value has to be 16KiB.

Since qemu-img ensures the value is correct and the limit is not always
the same libvirt will not duplicate any of these checks as the error
message from qemu-img is good enough:

    Cluster size must be a power of two between 512 and 2048k

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/154

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 docs/formatstorage.html.in                      |  6 ++++++
 docs/schemas/storagecommon.rng                  |  7 +++++++
 docs/schemas/storagevol.rng                     |  3 +++
 src/conf/storage_conf.c                         | 12 ++++++++++++
 src/storage/storage_util.c                      |  5 +++++
 .../qcow2-clusterSize.argv                      |  6 ++++++
 tests/storagevolxml2argvtest.c                  |  4 ++++
 .../vol-qcow2-clusterSize.xml                   | 17 +++++++++++++++++
 .../vol-qcow2-clusterSize.xml                   | 17 +++++++++++++++++
 tests/storagevolxml2xmltest.c                   |  1 +
 10 files changed, 78 insertions(+)
 create mode 100644 tests/storagevolxml2argvdata/qcow2-clusterSize.argv
 create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
 create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-clusterSize.xml

diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index cac44503da..1cf65b0b18 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -791,6 +791,7 @@
   &lt;/encryption&gt;
   &lt;compat&gt;1.1&lt;/compat&gt;
   &lt;nocow/&gt;
+  &lt;clusterSize unit='KiB'&gt;64&lt;/clusterSize&gt;
   &lt;features&gt;
     &lt;lazy_refcounts/&gt;
   &lt;/features&gt;
@@ -867,6 +868,11 @@
         the file image is used in VM. To create non-raw file images, it
         requires QEMU version since 2.1. <span class="since">Since 1.2.7</span>
       </dd>
+      <dt><code>clusterSize</code></dt>
+      <dd> Changes the qcow2 cluster size where smaller size can improve image
+        file size whereas larger size can improve performance.
+        <span class="since">Since 7.4.0</span>
+      </dd>
       <dt><code>features</code></dt>
       <dd>Format-specific features. Only used for <code>qcow2</code> now.
         Valid sub-elements are:
diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
index e3d08a8410..9ebb27700d 100644
--- a/docs/schemas/storagecommon.rng
+++ b/docs/schemas/storagecommon.rng
@@ -110,6 +110,13 @@
       </data>
     </element>
   </define>
+
+  <define name="clusterSize">
+    <element name="clusterSize">
+      <ref name="scaledInteger"/>
+    </element>
+  </define>
+
   <define name="fileFormatFeatures">
     <element name="features">
       <interleave>
diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng
index 22ce5eaa8d..3e0f482007 100644
--- a/docs/schemas/storagevol.rng
+++ b/docs/schemas/storagevol.rng
@@ -124,6 +124,9 @@
             <empty/>
           </element>
         </optional>
+        <optional>
+          <ref name="clusterSize"/>
+        </optional>
         <optional>
           <ref name="fileFormatFeatures"/>
         </optional>
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index e481cac75c..0ecdb0969a 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1416,6 +1416,13 @@ virStorageVolDefParseXML(virStoragePoolDef *pool,
     if (virXPathNode("./target/nocow", ctxt))
         def->target.nocow = true;
 
+    if (virParseScaledValue("./target/clusterSize",
+                            "./target/clusterSize/@unit",
+                            ctxt, &def->target.clusterSize,
+                            1, ULLONG_MAX, false) < 0) {
+        return NULL;
+    }
+
     if (virXPathNode("./target/features", ctxt)) {
         if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0)
             return NULL;
@@ -1580,6 +1587,11 @@ virStorageVolTargetDefFormat(virStorageVolOptions *options,
 
     virBufferEscapeString(buf, "<compat>%s</compat>\n", def->compat);
 
+    if (def->clusterSize > 0) {
+        virBufferAsprintf(buf, "<clusterSize unit='B'>%llu</clusterSize>\n",
+                          def->clusterSize);
+    }
+
     if (def->features) {
         size_t i;
         bool empty = virBitmapIsAllClear(def->features);
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 2b0d08c65d..c8299852a3 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -664,6 +664,7 @@ struct _virStorageBackendQemuImgInfo {
     const char *path;
     unsigned long long size_arg;
     unsigned long long allocation;
+    unsigned long long clusterSize;
     bool encryption;
     bool preallocate;
     const char *compat;
@@ -780,6 +781,9 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDef *encinfo,
     else if (info->format == VIR_STORAGE_FILE_QCOW2)
         virBufferAddLit(&buf, "compat=0.10,");
 
+    if (info->clusterSize > 0)
+        virBufferAsprintf(&buf, "cluster_size=%llu,", info->clusterSize);
+
     if (info->features && info->format == VIR_STORAGE_FILE_QCOW2) {
         if (virBitmapIsBitSet(info->features,
                               VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS)) {
@@ -1130,6 +1134,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObj *pool,
         .compat = vol->target.compat,
         .features = vol->target.features,
         .nocow = vol->target.nocow,
+        .clusterSize = vol->target.clusterSize,
         .secretAlias = NULL,
     };
     virStorageEncryption *enc = vol->target.encryption;
diff --git a/tests/storagevolxml2argvdata/qcow2-clusterSize.argv b/tests/storagevolxml2argvdata/qcow2-clusterSize.argv
new file mode 100644
index 0000000000..94b1110ccc
--- /dev/null
+++ b/tests/storagevolxml2argvdata/qcow2-clusterSize.argv
@@ -0,0 +1,6 @@
+qemu-img \
+create \
+-f qcow2 \
+-o compat=0.10,cluster_size=131072 \
+/var/lib/libvirt/images/OtherDemo.img \
+5242880K
\ No newline at end of file
diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c
index 6058d2b689..9597509f00 100644
--- a/tests/storagevolxml2argvtest.c
+++ b/tests/storagevolxml2argvtest.c
@@ -238,6 +238,10 @@ mymain(void)
                  "pool-dir", "vol-qcow2-nocapacity-backing", NULL, NULL,
                  "qcow2-nocapacity", 0);
 
+    DO_TEST("pool-dir", "vol-qcow2-clusterSize",
+            NULL, NULL,
+            "qcow2-clusterSize", 0);
+
     DO_TEST("pool-dir", "vol-file-iso",
             NULL, NULL,
             "iso", 0);
diff --git a/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
new file mode 100644
index 0000000000..22534982a1
--- /dev/null
+++ b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
@@ -0,0 +1,17 @@
+<volume>
+  <name>OtherDemo.img</name>
+  <key>/var/lib/libvirt/images/OtherDemo.img</key>
+  <capacity unit="G">5</capacity>
+  <allocation>294912</allocation>
+  <target>
+    <path>/var/lib/libvirt/images/OtherDemo.img</path>
+    <format type='qcow2'/>
+    <permissions>
+      <mode>0644</mode>
+      <owner>0</owner>
+      <group>0</group>
+      <label>unconfined_u:object_r:virt_image_t:s0</label>
+    </permissions>
+    <clusterSize unit='KiB'>128</clusterSize>
+  </target>
+</volume>
diff --git a/tests/storagevolxml2xmlout/vol-qcow2-clusterSize.xml b/tests/storagevolxml2xmlout/vol-qcow2-clusterSize.xml
new file mode 100644
index 0000000000..393a492536
--- /dev/null
+++ b/tests/storagevolxml2xmlout/vol-qcow2-clusterSize.xml
@@ -0,0 +1,17 @@
+<volume type='file'>
+  <name>OtherDemo.img</name>
+  <key>/var/lib/libvirt/images/OtherDemo.img</key>
+  <capacity unit='bytes'>5368709120</capacity>
+  <allocation unit='bytes'>294912</allocation>
+  <target>
+    <path>/var/lib/libvirt/images/OtherDemo.img</path>
+    <format type='qcow2'/>
+    <permissions>
+      <mode>0644</mode>
+      <owner>0</owner>
+      <group>0</group>
+      <label>unconfined_u:object_r:virt_image_t:s0</label>
+    </permissions>
+    <clusterSize unit='B'>131072</clusterSize>
+  </target>
+</volume>
diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c
index ed24d98426..be38f50a51 100644
--- a/tests/storagevolxml2xmltest.c
+++ b/tests/storagevolxml2xmltest.c
@@ -88,6 +88,7 @@ mymain(void)
     DO_TEST("pool-dir", "vol-qcow2-nobacking");
     DO_TEST("pool-dir", "vol-qcow2-encryption");
     DO_TEST("pool-dir", "vol-qcow2-luks");
+    DO_TEST("pool-dir", "vol-qcow2-clusterSize");
     DO_TEST("pool-dir", "vol-luks");
     DO_TEST("pool-dir", "vol-luks-cipher");
     DO_TEST("pool-disk", "vol-partition");
-- 
2.31.1

Re: [libvirt PATCH 1/2] storage: add support for QCOW2 cluster_size option
Posted by Peter Krempa 4 years, 8 months ago
On Thu, May 13, 2021 at 13:23:04 +0200, Pavel Hrdina wrote:
> The default value hard-coded in QEMU (64KiB) is not always the ideal.
> Having a possibility to set the cluster_size by user may in specific
> use-cases improve performance for QCOW2 images.
> 
> QEMU internally has some limits, the value has to be between 512B and
> 2048KiB and must by power of two, except when the image has Extended L2
> Entries the minimal value has to be 16KiB.
> 
> Since qemu-img ensures the value is correct and the limit is not always
> the same libvirt will not duplicate any of these checks as the error
> message from qemu-img is good enough:
> 
>     Cluster size must be a power of two between 512 and 2048k
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/154
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---

[...]

> @@ -867,6 +868,11 @@
>          the file image is used in VM. To create non-raw file images, it
>          requires QEMU version since 2.1. <span class="since">Since 1.2.7</span>
>        </dd>
> +      <dt><code>clusterSize</code></dt>
> +      <dd> Changes the qcow2 cluster size where smaller size can improve image
> +        file size whereas larger size can improve performance.
> +        <span class="since">Since 7.4.0</span>

I think we should refrain from advice on how the cluster size can be
configured because it has way more nuance than this short sentence.

> +      </dd>
>        <dt><code>features</code></dt>
>        <dd>Format-specific features. Only used for <code>qcow2</code> now.
>          Valid sub-elements are:



> diff --git a/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
> new file mode 100644
> index 0000000000..22534982a1
> --- /dev/null
> +++ b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
> @@ -0,0 +1,17 @@
> +<volume>
> +  <name>OtherDemo.img</name>
> +  <key>/var/lib/libvirt/images/OtherDemo.img</key>
> +  <capacity unit="G">5</capacity>
> +  <allocation>294912</allocation>
> +  <target>
> +    <path>/var/lib/libvirt/images/OtherDemo.img</path>
> +    <format type='qcow2'/>

I wonder whether it wouldn't make more sense to stash cluster size under
the 'format' tag.

Does the LVM volume XML have <format> ?

> +    <permissions>
> +      <mode>0644</mode>
> +      <owner>0</owner>
> +      <group>0</group>
> +      <label>unconfined_u:object_r:virt_image_t:s0</label>
> +    </permissions>
> +    <clusterSize unit='KiB'>128</clusterSize>
> +  </target>
> +</volume>

Re: [libvirt PATCH 1/2] storage: add support for QCOW2 cluster_size option
Posted by Pavel Hrdina 4 years, 8 months ago
On Wed, May 19, 2021 at 02:49:14PM +0200, Peter Krempa wrote:
> On Thu, May 13, 2021 at 13:23:04 +0200, Pavel Hrdina wrote:
> > The default value hard-coded in QEMU (64KiB) is not always the ideal.
> > Having a possibility to set the cluster_size by user may in specific
> > use-cases improve performance for QCOW2 images.
> > 
> > QEMU internally has some limits, the value has to be between 512B and
> > 2048KiB and must by power of two, except when the image has Extended L2
> > Entries the minimal value has to be 16KiB.
> > 
> > Since qemu-img ensures the value is correct and the limit is not always
> > the same libvirt will not duplicate any of these checks as the error
> > message from qemu-img is good enough:
> > 
> >     Cluster size must be a power of two between 512 and 2048k
> > 
> > Resolves: https://gitlab.com/libvirt/libvirt/-/issues/154
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> 
> [...]
> 
> > @@ -867,6 +868,11 @@
> >          the file image is used in VM. To create non-raw file images, it
> >          requires QEMU version since 2.1. <span class="since">Since 1.2.7</span>
> >        </dd>
> > +      <dt><code>clusterSize</code></dt>
> > +      <dd> Changes the qcow2 cluster size where smaller size can improve image
> > +        file size whereas larger size can improve performance.
> > +        <span class="since">Since 7.4.0</span>
> 
> I think we should refrain from advice on how the cluster size can be
> configured because it has way more nuance than this short sentence.

Reasonable, I can drop it as I was just rephrasing what qemu has in
their documentation.

> > +      </dd>
> >        <dt><code>features</code></dt>
> >        <dd>Format-specific features. Only used for <code>qcow2</code> now.
> >          Valid sub-elements are:
> 
> 
> 
> > diff --git a/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
> > new file mode 100644
> > index 0000000000..22534982a1
> > --- /dev/null
> > +++ b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
> > @@ -0,0 +1,17 @@
> > +<volume>
> > +  <name>OtherDemo.img</name>
> > +  <key>/var/lib/libvirt/images/OtherDemo.img</key>
> > +  <capacity unit="G">5</capacity>
> > +  <allocation>294912</allocation>
> > +  <target>
> > +    <path>/var/lib/libvirt/images/OtherDemo.img</path>
> > +    <format type='qcow2'/>
> 
> I wonder whether it wouldn't make more sense to stash cluster size under
> the 'format' tag.
> 
> Does the LVM volume XML have <format> ?

No it doesn't have it. Only for pool XML. In addition currently it has
only path attribute.

For qcow2 we have most of the options represented outside the <format>
element. I'm not saying it's correct as I cannot decide which place
would be better, but following what we already do with other qcow2
options I figured we should do the same here.

It is definitely possible to have it in <format>.

Pavel

> > +    <permissions>
> > +      <mode>0644</mode>
> > +      <owner>0</owner>
> > +      <group>0</group>
> > +      <label>unconfined_u:object_r:virt_image_t:s0</label>
> > +    </permissions>
> > +    <clusterSize unit='KiB'>128</clusterSize>
> > +  </target>
> > +</volume>