[libvirt PATCH 0/2] storage_driver: add support for cluster_size QCOW2 option
Pavel Hrdina (2): storage: add support for QCOW2 cluster_size option storage_file: add support to probe cluster_size from QCOW2 images 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 | 8 +++ src/storage_file/storage_file_probe.c | 55 +++++++++++++------ .../qcow2-clusterSize.argv | 6 ++ tests/storagevolxml2argvtest.c | 4 ++ .../vol-qcow2-clusterSize.xml | 17 ++++++ .../vol-qcow2-clusterSize.xml | 17 ++++++ tests/storagevolxml2xmltest.c | 1 + 11 files changed, 120 insertions(+), 16 deletions(-) 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 -- 2.31.1
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 @@ </encryption> <compat>1.1</compat> <nocow/> + <clusterSize unit='KiB'>64</clusterSize> <features> <lazy_refcounts/> </features> @@ -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
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>
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>
From QEMU docs/interop/qcow2.txt :
Byte 20 - 23: cluster_bits Number of bits that are used for addressing an offset within a cluster (1 << cluster_bits is the cluster size). With this patch libvirt will be able to report the current cluster_size for all existing storage volumes managed by storage driver. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/storage/storage_util.c | 3 ++ src/storage_file/storage_file_probe.c | 55 +++++++++++++++++++-------- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index c8299852a3..a7c9355bf9 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3483,6 +3483,9 @@ storageBackendProbeTarget(virStorageSource *target, if (meta->capacity) target->capacity = meta->capacity; + if (meta->clusterSize > 0) + target->clusterSize = meta->clusterSize; + if (encryption && meta->encryption) { if (meta->encryption->payload_offset != -1) target->capacity -= meta->encryption->payload_offset * 512; diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index afe64da02e..423597049f 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -94,6 +94,9 @@ struct FileTypeInfo { /* Store a COW base image path (possibly relative), * or NULL if there is no COW base image, to RES; * return BACKING_STORE_* */ + int clusterBitsOffset; /* Byte offset from start of file where we find + * number of cluster bits, -1 to skip. */ + int clusterBitsSize; /* Number of bytes for cluster bits. */ const struct FileEncryptionInfo *cryptInfo; /* Encryption info */ int (*getBackingStore)(char **res, int *format, const char *buf, size_t buf_size); @@ -116,7 +119,8 @@ qedGetBackingStore(char **, int *, const char *, size_t); #define QCOWX_HDR_VERSION (4) #define QCOWX_HDR_BACKING_FILE_OFFSET (QCOWX_HDR_VERSION+4) #define QCOWX_HDR_BACKING_FILE_SIZE (QCOWX_HDR_BACKING_FILE_OFFSET+8) -#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_BACKING_FILE_SIZE+4+4) +#define QCOWX_HDR_CLUSTER_BITS_OFFSET (QCOWX_HDR_BACKING_FILE_SIZE+4) +#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_CLUSTER_BITS_OFFSET+4) #define QCOW1_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8+1+1+2) #define QCOW2_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8) @@ -238,18 +242,18 @@ static struct FileEncryptionInfo const qcow2EncryptionInfo[] = { static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, -1, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_RAW] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, + -1, 0, {0}, 0, 0, 0, -1, 0, luksEncryptionInfo, NULL, NULL }, [VIR_STORAGE_FILE_DIR] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, -1, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_BOCHS] = { /*"Bochs Virtual HD Image", */ /* Untested */ 0, NULL, LV_LITTLE_ENDIAN, 64, 4, {0x20000}, - 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL, NULL + 32+16+16+4+4+4+4+4, 8, 1, -1, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_CLOOP] = { /* #!/bin/sh @@ -258,7 +262,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { */ /* Untested */ 0, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, - -1, 0, 0, NULL, NULL, NULL + -1, 0, 0, -1, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_DMG] = { /* XXX QEMU says there's no magic for dmg, @@ -266,43 +270,44 @@ static struct FileTypeInfo const fileTypeInfo[] = { * would have to match) but then disables that check. */ 0, NULL, 0, -1, 0, {0}, - -1, 0, 0, NULL, NULL, NULL + -1, 0, 0, -1, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", LV_LITTLE_ENDIAN, -2, 0, {0}, - -1, 0, 0, NULL, NULL, NULL + -1, 0, 0, -1, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VPC] = { 0, "conectix", LV_BIG_ENDIAN, 12, 4, {0x10000}, - 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL, NULL + 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, 0, NULL, NULL, NULL }, /* TODO: add getBackingStore function */ [VIR_STORAGE_FILE_VDI] = { 64, "\x7f\x10\xda\xbe", LV_LITTLE_ENDIAN, 68, 4, {0x00010001}, - 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL, NULL}, + 64 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, 0, NULL, NULL, NULL}, /* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, -1, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, -1, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", LV_LITTLE_ENDIAN, -2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 0, - PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL }, + PLOOP_SIZE_MULTIPLIER, -1, 0, NULL, NULL, NULL }, /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { 0, "OOOM", LV_BIG_ENDIAN, 4, 4, {2}, - 4+4+1024+4, 8, 1, NULL, cowGetBackingStore, NULL + 4+4+1024+4, 8, 1, -1, 0, NULL, cowGetBackingStore, NULL }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", LV_BIG_ENDIAN, 4, 4, {1}, QCOWX_HDR_IMAGE_SIZE, 8, 1, + -1, 0, qcow1EncryptionInfo, qcowXGetBackingStore, NULL }, @@ -310,6 +315,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { 0, "QFI", LV_BIG_ENDIAN, 4, 4, {2, 3}, QCOWX_HDR_IMAGE_SIZE, 8, 1, + QCOWX_HDR_CLUSTER_BITS_OFFSET, 4, qcow2EncryptionInfo, qcowXGetBackingStore, qcow2GetFeatures @@ -318,12 +324,12 @@ static struct FileTypeInfo const fileTypeInfo[] = { /* https://wiki.qemu.org/Features/QED */ 0, "QED", LV_LITTLE_ENDIAN, -2, 0, {0}, - QED_HDR_IMAGE_SIZE, 8, 1, NULL, qedGetBackingStore, NULL + QED_HDR_IMAGE_SIZE, 8, 1, -1, 0, NULL, qedGetBackingStore, NULL }, [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", LV_LITTLE_ENDIAN, 4, 4, {1, 2, 3}, - 4+4+4, 8, 512, NULL, vmdk4GetBackingStore, NULL + 4+4+4, 8, 512, -1, 0, NULL, vmdk4GetBackingStore, NULL }, }; G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST); @@ -890,6 +896,23 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier; } + if (fileTypeInfo[meta->format].clusterBitsOffset != -1) { + int clusterBits = 0; + + if ((fileTypeInfo[meta->format].clusterBitsOffset + 4) > len) + return 0; + + if (fileTypeInfo[meta->format].endian == LV_LITTLE_ENDIAN) + clusterBits = virReadBufInt32LE(buf + + fileTypeInfo[meta->format].clusterBitsOffset); + else + clusterBits = virReadBufInt32BE(buf + + fileTypeInfo[meta->format].clusterBitsOffset); + + if (clusterBits > 0) + meta->clusterSize = 1 << clusterBits; + } + VIR_FREE(meta->backingStoreRaw); if (fileTypeInfo[meta->format].getBackingStore != NULL) { int store = fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw, -- 2.31.1
On Thu, May 13, 2021 at 13:23:05 +0200, Pavel Hrdina wrote:
From QEMU docs/interop/qcow2.txt :
Byte 20 - 23: cluster_bits Number of bits that are used for addressing an offset within a cluster (1 << cluster_bits is the cluster size).
With this patch libvirt will be able to report the current cluster_size for all existing storage volumes managed by storage driver.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/storage/storage_util.c | 3 ++ src/storage_file/storage_file_probe.c | 55 +++++++++++++++++++-------- 2 files changed, 42 insertions(+), 16 deletions(-)
[...]
diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index afe64da02e..423597049f 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -94,6 +94,9 @@ struct FileTypeInfo { /* Store a COW base image path (possibly relative), * or NULL if there is no COW base image, to RES; * return BACKING_STORE_* */ + int clusterBitsOffset; /* Byte offset from start of file where we find + * number of cluster bits, -1 to skip. */ + int clusterBitsSize; /* Number of bytes for cluster bits. */
This isn't actually used [1].
const struct FileEncryptionInfo *cryptInfo; /* Encryption info */ int (*getBackingStore)(char **res, int *format, const char *buf, size_t buf_size);
Given how the value is stored in the header it seems that it won't really be usable for any other format. I think we should parse it via a callback rather than the generic offset/size parser. Specifically since the cluster size is stored as number of bits.
@@ -116,7 +119,8 @@ qedGetBackingStore(char **, int *, const char *, size_t); #define QCOWX_HDR_VERSION (4) #define QCOWX_HDR_BACKING_FILE_OFFSET (QCOWX_HDR_VERSION+4) #define QCOWX_HDR_BACKING_FILE_SIZE (QCOWX_HDR_BACKING_FILE_OFFSET+8) -#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_BACKING_FILE_SIZE+4+4) +#define QCOWX_HDR_CLUSTER_BITS_OFFSET (QCOWX_HDR_BACKING_FILE_SIZE+4) +#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_CLUSTER_BITS_OFFSET+4)
#define QCOW1_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8+1+1+2) #define QCOW2_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8)
G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST); @@ -890,6 +896,23 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier; }
+ if (fileTypeInfo[meta->format].clusterBitsOffset != -1) { + int clusterBits = 0; + + if ((fileTypeInfo[meta->format].clusterBitsOffset + 4) > len)
[1] ... you hardcode the value.
+ return 0; + + if (fileTypeInfo[meta->format].endian == LV_LITTLE_ENDIAN) + clusterBits = virReadBufInt32LE(buf + + fileTypeInfo[meta->format].clusterBitsOffset); + else + clusterBits = virReadBufInt32BE(buf + + fileTypeInfo[meta->format].clusterBitsOffset); + + if (clusterBits > 0) + meta->clusterSize = 1 << clusterBits; + } + VIR_FREE(meta->backingStoreRaw); if (fileTypeInfo[meta->format].getBackingStore != NULL) { int store = fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw, -- 2.31.1
On Wed, May 19, 2021 at 02:57:18PM +0200, Peter Krempa wrote:
On Thu, May 13, 2021 at 13:23:05 +0200, Pavel Hrdina wrote:
From QEMU docs/interop/qcow2.txt :
Byte 20 - 23: cluster_bits Number of bits that are used for addressing an offset within a cluster (1 << cluster_bits is the cluster size).
With this patch libvirt will be able to report the current cluster_size for all existing storage volumes managed by storage driver.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/storage/storage_util.c | 3 ++ src/storage_file/storage_file_probe.c | 55 +++++++++++++++++++-------- 2 files changed, 42 insertions(+), 16 deletions(-)
[...]
diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index afe64da02e..423597049f 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -94,6 +94,9 @@ struct FileTypeInfo { /* Store a COW base image path (possibly relative), * or NULL if there is no COW base image, to RES; * return BACKING_STORE_* */ + int clusterBitsOffset; /* Byte offset from start of file where we find + * number of cluster bits, -1 to skip. */ + int clusterBitsSize; /* Number of bytes for cluster bits. */
This isn't actually used [1].
Good point, I'll drop it.
const struct FileEncryptionInfo *cryptInfo; /* Encryption info */ int (*getBackingStore)(char **res, int *format, const char *buf, size_t buf_size);
Given how the value is stored in the header it seems that it won't really be usable for any other format. I think we should parse it via a callback rather than the generic offset/size parser.
Specifically since the cluster size is stored as number of bits.
That will be way better then mangling with the crazy struct. I'll change it for v2. Thanks for the review, I'll make the changes and once we figure out the placement of cluster_size I'll post v2. Pavel
On Thu, May 13, 2021 at 7:23 PM Pavel Hrdina <phrdina@redhat.com> wrote:
Pavel Hrdina (2): storage: add support for QCOW2 cluster_size option storage_file: add support to probe cluster_size from QCOW2 images
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 | 8 +++ src/storage_file/storage_file_probe.c | 55 +++++++++++++------ .../qcow2-clusterSize.argv | 6 ++ tests/storagevolxml2argvtest.c | 4 ++ .../vol-qcow2-clusterSize.xml | 17 ++++++ .../vol-qcow2-clusterSize.xml | 17 ++++++ tests/storagevolxml2xmltest.c | 1 + 11 files changed, 120 insertions(+), 16 deletions(-) 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
Works for me when patched with libvirt v7.3.0-184-gdf28ba289c ➜ ~ cat /tmp/cluster.xml <volume type='file'> <name>cluster.qcow2</name> <key>/var/lib/libvirt/images/cluster.qcow2</key> <capacity unit='bytes'>5368709</capacity> <physical unit='bytes'>1271660544</physical> <target> <path>/var/lib/libvirt/images/hhan-pc.qcow2</path> <format type='qcow2'/> <permissions> <mode>0600</mode> <owner>0</owner> <group>0</group> <label>system_u:object_r:virt_image_t:s0</label> </permissions> <timestamps> <atime>1621221448.682657229</atime> <mtime>1618908525.055934734</mtime> <ctime>1618908525.253934734</ctime> <btime>0</btime> </timestamps> <compat>1.1</compat> <clusterSize>1024</clusterSize> <features> <lazy_refcounts/> </features> </target> </volume>
➜ ~ virt-xml-validate /tmp/cluster.xml /tmp/cluster.xml validates ➜ ~ virsh vol-create images /tmp/cluster.xml Vol cluster.qcow2 created from /tmp/cluster.xml ➜ ~ qemu-img info /var/lib/libvirt/images/cluster.qcow2 image: /var/lib/libvirt/images/cluster.qcow2 file format: qcow2 virtual size: 5.12 MiB (5368832 bytes) disk size: 4 KiB cluster_size: 1024 Format specific information: compat: 1.1 compression type: zlib lazy refcounts: true refcount bits: 16 corrupt: false extended l2: false ➜ ~ virsh vol-dumpxml --pool images cluster.qcow2 <volume type='file'> <name>cluster.qcow2</name> <key>/var/lib/libvirt/images/cluster.qcow2</key> <capacity unit='bytes'>5368832</capacity> <allocation unit='bytes'>4096</allocation> <physical unit='bytes'>3400</physical> <target> <path>/var/lib/libvirt/images/cluster.qcow2</path> <format type='qcow2'/> <permissions> <mode>0600</mode> <owner>0</owner> <group>0</group> <label>system_u:object_r:virt_image_t:s0</label> </permissions> <timestamps> <atime>1621224886.854657229</atime> <mtime>1621224875.045657229</mtime> <ctime>1621224875.049657229</ctime> <btime>0</btime> </timestamps> <compat>1.1</compat> <clusterSize unit='B'>1024</clusterSize> <features> <lazy_refcounts/> </features> </target> </volume>
-- 2.31.1
-- Reviewed-by: Han Han <hhan@redhat.com> Tested-by: Han Han <hhan@redhat.com>
participants (3)
-
Han Han -
Pavel Hrdina -
Peter Krempa