Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary (rewrite)
On 05/08/2014 06:02 PM, John Ferlan wrote:
A post commit id 'e3d66229' review (and followup):
http://www.redhat.com/archives/libvir-list/2014-May/msg00268.html
noted some issues with the code, so I have adjusted the code
accordingly. The difference between this and the commit prior
to the change (commit id 'f3be5f0c') will just be the check for
qcow2/qed using a non 512 block aligned size will result in a
round up of size. If the size is within the last 512 bytes to
ULLONG_MAX, then just set it there rather than erroring out.
This patch essentially reverts commit e3d66229.
Reverting it explicitly using 'git revert' and basing this patch on that would
make it smaller and maybe easier to backport to the -maint branches.
If you choose to do an explicit revert, please include the bugzilla link in
the commit message.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ff8a2d..8771cae 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9413,7 +9413,6 @@ qemuDomainBlockResize(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1, idx;
-unsigned long long size_up;
char *device = NULL;
virDomainDiskDefPtr disk = NULL;
@@ -9434,12 +9433,6 @@ qemuDomainBlockResize(virDomainPtr dom,
return -1;
}
size *= 1024;
-size_up = size;
-} else {
-/* For 'qcow2' and 'qed', qemu resize blocks expects values
- * on sector boundary, so round our value up to prepare
- */
-size_up = VIR_ROUND_UP(size, 512);
}
if (!(vm = qemuDomObjFromDomain(dom)))
@@ -9466,19 +9459,16 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
-/* qcow2 and qed must be sized appropriately, so be sure our value
- * is sized appropriately and will fit
+/* qcow2 and qed must be sized on 512 byte blocks/sectors,
+ * so adjust size if necessary to round up (if possible).
*/
-if (size != size_up
-(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
- disk-src.format == VIR_STORAGE_FILE_QED)) {
-if (size_up ULLONG_MAX) {
-virReportError(VIR_ERR_OVERFLOW,
- _(size must be less than %llu KiB),
- ULLONG_MAX / 1024);
-goto endjob;
-}
-size = size_up;
+if ((disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED)
+(size % 512)) {
The (size % 512) condition is not necessary - VIR_ROUND_UP won't change the
value if the number is divisible by 512 if it does not overflow.
+if ((ULLONG_MAX - size) 512)
+size = ULLONG_MAX;
ULLONG_MAX is not divisible by 512 thus I don't think it's any better than
what the user entered.
+else
+size = VIR_ROUND_UP(size, 512);
}
if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
ACK with the ULLONG_MAX assignment removed - whether you do 'git revert' or not.
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary (rewrite)
On 05/12/2014 07:11 AM, Ján Tomko wrote:
On 05/08/2014 06:02 PM, John Ferlan wrote:
A post commit id 'e3d66229' review (and followup):
http://www.redhat.com/archives/libvir-list/2014-May/msg00268.html
noted some issues with the code, so I have adjusted the code
accordingly. The difference between this and the commit prior
to the change (commit id 'f3be5f0c') will just be the check for
qcow2/qed using a non 512 block aligned size will result in a
round up of size. If the size is within the last 512 bytes to
ULLONG_MAX, then just set it there rather than erroring out.
This patch essentially reverts commit e3d66229.
Reverting it explicitly using 'git revert' and basing this patch on that would
make it smaller and maybe easier to backport to the -maint branches.
If you choose to do an explicit revert, please include the bugzilla link in
the commit message.
So with the revert and the changes below this essentially turns into:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f0e82e9..63a9e77 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9459,6 +9459,13 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
+/* qcow2 and qed must be sized on 512 byte blocks/sectors,
+ * so adjust size if necessary to round up.
+ */
+if (disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+disk-src.format == VIR_STORAGE_FILE_QED)
+size = VIR_ROUND_UP(size, 512);
+
if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
disk-info.alias) 0)
goto endjob;
The whole overflow issue for VIR_ROUND_UP and VIR_DIV_UP is something
that could/would/should be handled separately...
John
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ff8a2d..8771cae 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9413,7 +9413,6 @@ qemuDomainBlockResize(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1, idx;
-unsigned long long size_up;
char *device = NULL;
virDomainDiskDefPtr disk = NULL;
@@ -9434,12 +9433,6 @@ qemuDomainBlockResize(virDomainPtr dom,
return -1;
}
size *= 1024;
-size_up = size;
-} else {
-/* For 'qcow2' and 'qed', qemu resize blocks expects values
- * on sector boundary, so round our value up to prepare
- */
-size_up = VIR_ROUND_UP(size, 512);
}
if (!(vm = qemuDomObjFromDomain(dom)))
@@ -9466,19 +9459,16 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
-/* qcow2 and qed must be sized appropriately, so be sure our value
- * is sized appropriately and will fit
+/* qcow2 and qed must be sized on 512 byte blocks/sectors,
+ * so adjust size if necessary to round up (if possible).
*/
-if (size != size_up
-(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
- disk-src.format == VIR_STORAGE_FILE_QED)) {
-if (size_up ULLONG_MAX) {
-virReportError(VIR_ERR_OVERFLOW,
- _(size must be less than %llu KiB),
- ULLONG_MAX / 1024);
-goto endjob;
-}
-size = size_up;
+if ((disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED)
+(size % 512)) {
The (size % 512) condition is not necessary - VIR_ROUND_UP won't change the
value if the number is divisible by 512 if it does not overflow.
+if ((ULLONG_MAX - size) 512)
+size = ULLONG_MAX;
ULLONG_MAX is not divisible by 512 thus I don't think it's any better than
what the user entered.
+else
+size = VIR_ROUND_UP(size, 512);
}
if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
ACK with the ULLONG_MAX assignment removed - whether you do 'git revert' or
not.
Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary
On 05/07/2014 09:05 PM, John Ferlan wrote:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4bb4819..3e407d7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9421,6 +9421,7 @@ qemuDomainBlockResize(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1, idx;
+unsigned long long size_up;
char *device = NULL;
virDomainDiskDefPtr disk = NULL;
@@ -9467,6 +9474,21 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
+/* qcow2 and qed must be sized appropriately, so be sure our value
+ * is sized appropriately and will fit
+ */
+if (size != size_up
+(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED)) {
+if (size_up ULLONG_MAX) {
This is always false.
An OVERLy cautious check - I cannot remember what I was thinking about a
month ago... I think this was the check can VIR_ROUND_UP provide an
incorrect value. I can send a follow-up patch to remove those lines if
that's desired.
VIR_ROUND_UP can still overflow if the size was specified in bytes and is
larger than ULLONG_MAX-511. This is unlikely to happen in the real world, but
it would be nice to check for it.
+virReportError(VIR_ERR_OVERFLOW,
+ _(size must be less than %llu KiB),
+ ULLONG_MAX / 1024);
+goto endjob;
+}
+size = size_up;
Just a nitpick: rounding it up unconditionally here would get rid of the
temporary variable and have no effect on values specified without the BYTES
flag.
Only qcow2 and qed have this issue regarding needing to be on a 512 byte
boundary. Since this is a generic routine I was limiting the rounding to
the two types from the bz rather than taking a chance that some generic
round up would cause some other issue. Or am I misinterpreting your
comment?
I meant unconditional on whether the size was specified in bytes or not,
something like:
if (qcow2 or qed) {
size = VIR_ROUND_UP(size, 512);
}
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary (rewrite)
A post commit id 'e3d66229' review (and followup):
http://www.redhat.com/archives/libvir-list/2014-May/msg00268.html
noted some issues with the code, so I have adjusted the code
accordingly. The difference between this and the commit prior
to the change (commit id 'f3be5f0c') will just be the check for
qcow2/qed using a non 512 block aligned size will result in a
round up of size. If the size is within the last 512 bytes to
ULLONG_MAX, then just set it there rather than erroring out.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ff8a2d..8771cae 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9413,7 +9413,6 @@ qemuDomainBlockResize(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1, idx;
-unsigned long long size_up;
char *device = NULL;
virDomainDiskDefPtr disk = NULL;
@@ -9434,12 +9433,6 @@ qemuDomainBlockResize(virDomainPtr dom,
return -1;
}
size *= 1024;
-size_up = size;
-} else {
-/* For 'qcow2' and 'qed', qemu resize blocks expects values
- * on sector boundary, so round our value up to prepare
- */
-size_up = VIR_ROUND_UP(size, 512);
}
if (!(vm = qemuDomObjFromDomain(dom)))
@@ -9466,19 +9459,16 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
-/* qcow2 and qed must be sized appropriately, so be sure our value
- * is sized appropriately and will fit
+/* qcow2 and qed must be sized on 512 byte blocks/sectors,
+ * so adjust size if necessary to round up (if possible).
*/
-if (size != size_up
-(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
- disk-src.format == VIR_STORAGE_FILE_QED)) {
-if (size_up ULLONG_MAX) {
-virReportError(VIR_ERR_OVERFLOW,
- _(size must be less than %llu KiB),
- ULLONG_MAX / 1024);
-goto endjob;
-}
-size = size_up;
+if ((disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED)
+(size % 512)) {
+if ((ULLONG_MAX - size) 512)
+size = ULLONG_MAX;
+else
+size = VIR_ROUND_UP(size, 512);
}
if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
--
1.9.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary
On 04/08/2014 12:26 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1002813
If qemuDomainBlockResize() is passed a size not on a KiB boundary - that
is passed a size based in bytes (VIR_DOMAIN_BLOCK_RESIZE_BYTES), then
depending on the source format (qcow2 or qed), the value passed must
be on a sector (or 512 byte) boundary. Since other libvirt code quietly
adjusts the capacity values, then do so here as well - of course ensuring
that adjustment still fits.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 22 ++
1 file changed, 22 insertions(+)
Although discussion was taken off this list - the changes here were
ACK'd and pushed today...
Essentially the following API's will round up the value as well:
virStorageBackendCreateQcowCreate()
virStorageBackendLogicalCreateVol()
virStorageBackendCreateQemuImgCmd()
For libvirt created volumes, virStorageBackendCreateQemuImgCmd() or
virStorageBackendCreateQcowCreate() is called - both will take the
capacity value and VIR_DIV_UP using 1024. For the vol-resize path (e.g.
non running vm case), virStorageBackendFilesystemResizeQemuImg() will
use ROUND_UP on 512 byte value because it knows (and comments) that
qemu-img will fail to resize on non sector boundaries.
Additionally, it was noted that using K and KiB would produce 1024
based results, it's libvirt's allowance of KB for sizes that results
in the nuance. Being strict KB shouldn't be used for storage, but rather
than penalize for not knowing the difference between KiB and KB the code
just assumes KiB should have been used.
John
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4bb4819..3e407d7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9421,6 +9421,7 @@ qemuDomainBlockResize(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1, idx;
+unsigned long long size_up;
char *device = NULL;
virDomainDiskDefPtr disk = NULL;
@@ -9441,6 +9442,12 @@ qemuDomainBlockResize(virDomainPtr dom,
return -1;
}
size *= 1024;
+size_up = size;
+} else {
+/* For 'qcow2' and 'qed', qemu resize blocks expects values
+ * on sector boundary, so round our value up to prepare
+ */
+size_up = VIR_ROUND_UP(size, 512);
}
if (!(vm = qemuDomObjFromDomain(dom)))
@@ -9467,6 +9474,21 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
+/* qcow2 and qed must be sized appropriately, so be sure our value
+ * is sized appropriately and will fit
+ */
+if (size != size_up
+(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED)) {
+if (size_up ULLONG_MAX) {
+virReportError(VIR_ERR_OVERFLOW,
+ _(size must be less than %llu KiB),
+ ULLONG_MAX / 1024);
+goto endjob;
+}
+size = size_up;
+}
+
if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
disk-info.alias) 0)
goto endjob;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary
On 05/07/2014 01:58 PM, John Ferlan wrote:
On 04/08/2014 12:26 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1002813
If qemuDomainBlockResize() is passed a size not on a KiB boundary - that
is passed a size based in bytes (VIR_DOMAIN_BLOCK_RESIZE_BYTES), then
depending on the source format (qcow2 or qed), the value passed must
be on a sector (or 512 byte) boundary. Since other libvirt code quietly
adjusts the capacity values, then do so here as well - of course ensuring
that adjustment still fits.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 22 ++
1 file changed, 22 insertions(+)
Although discussion was taken off this list - the changes here were
ACK'd and pushed today...
I think ACKs should be on-list.
Essentially the following API's will round up the value as well:
virStorageBackendCreateQcowCreate()
virStorageBackendLogicalCreateVol()
virStorageBackendCreateQemuImgCmd()
For libvirt created volumes, virStorageBackendCreateQemuImgCmd() or
virStorageBackendCreateQcowCreate() is called - both will take the
capacity value and VIR_DIV_UP using 1024. For the vol-resize path (e.g.
non running vm case), virStorageBackendFilesystemResizeQemuImg() will
use ROUND_UP on 512 byte value because it knows (and comments) that
qemu-img will fail to resize on non sector boundaries.
Additionally, it was noted that using K and KiB would produce 1024
based results, it's libvirt's allowance of KB for sizes that results
in the nuance. Being strict KB shouldn't be used for storage, but rather
than penalize for not knowing the difference between KiB and KB the code
just assumes KiB should have been used.
John
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4bb4819..3e407d7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9421,6 +9421,7 @@ qemuDomainBlockResize(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1, idx;
+unsigned long long size_up;
char *device = NULL;
virDomainDiskDefPtr disk = NULL;
@@ -9467,6 +9474,21 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
+/* qcow2 and qed must be sized appropriately, so be sure our value
+ * is sized appropriately and will fit
+ */
+if (size != size_up
+(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED)) {
+if (size_up ULLONG_MAX) {
This is always false.
+virReportError(VIR_ERR_OVERFLOW,
+ _(size must be less than %llu KiB),
+ ULLONG_MAX / 1024);
+goto endjob;
+}
+size = size_up;
Just a nitpick: rounding it up unconditionally here would get rid of the
temporary variable and have no effect on values specified without the BYTES
flag.
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary
On 05/07/2014 10:34 AM, Ján Tomko wrote:
On 05/07/2014 01:58 PM, John Ferlan wrote:
On 04/08/2014 12:26 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1002813
If qemuDomainBlockResize() is passed a size not on a KiB boundary - that
is passed a size based in bytes (VIR_DOMAIN_BLOCK_RESIZE_BYTES), then
depending on the source format (qcow2 or qed), the value passed must
be on a sector (or 512 byte) boundary. Since other libvirt code quietly
adjusts the capacity values, then do so here as well - of course ensuring
that adjustment still fits.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 22 ++
1 file changed, 22 insertions(+)
Although discussion was taken off this list - the changes here were
ACK'd and pushed today...
I think ACKs should be on-list.
Understood - Eric's CC/query to qemu-devel on his initial response got
no response other than my followup... So a more direct question was
asked to (I assume) someone more involved in the blockdev layer. We got
a direct response from that, but nothing else in general from qemu. So,
after a couple more weeks of receiving no feedback - I queried whether
the change was OK. Eric's response indicated to go ahead with this patch
- all these are attached to this response if interested. While the 3
letters weren't used, I took the response as implicitly being OK with
the change.
Essentially the following API's will round up the value as well:
virStorageBackendCreateQcowCreate()
virStorageBackendLogicalCreateVol()
virStorageBackendCreateQemuImgCmd()
For libvirt created volumes, virStorageBackendCreateQemuImgCmd() or
virStorageBackendCreateQcowCreate() is called - both will take the
capacity value and VIR_DIV_UP using 1024. For the vol-resize path (e.g.
non running vm case), virStorageBackendFilesystemResizeQemuImg() will
use ROUND_UP on 512 byte value because it knows (and comments) that
qemu-img will fail to resize on non sector boundaries.
Additionally, it was noted that using K and KiB would produce 1024
based results, it's libvirt's allowance of KB for sizes that results
in the nuance. Being strict KB shouldn't be used for storage, but rather
than penalize for not knowing the difference between KiB and KB the code
just assumes KiB should have been used.
John
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4bb4819..3e407d7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9421,6 +9421,7 @@ qemuDomainBlockResize(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1, idx;
+unsigned long long size_up;
char *device = NULL;
virDomainDiskDefPtr disk = NULL;
@@ -9467,6 +9474,21 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
+/* qcow2 and qed must be sized appropriately, so be sure our value
+ * is sized appropriately and will fit
+ */
+if (size != size_up
+(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED)) {
+if (size_up ULLONG_MAX) {
This is always false.
An OVERLy cautious check - I cannot remember what I was thinking about a
month ago... I think this was the check can VIR_ROUND_UP provide an
incorrect value. I can send a follow-up patch to remove those lines if
that's desired.
+virReportError(VIR_ERR_OVERFLOW,
+ _(size must be less than %llu KiB),
+ ULLONG_MAX / 1024);
+goto endjob;
+}
+size = size_up;
Just a nitpick: rounding it up unconditionally here would get rid of the
temporary variable and have no effect on values specified without the BYTES
flag.
Only qcow2 and qed have this issue regarding needing to be on a 512 byte
boundary. Since this is a generic routine I was limiting the rounding to
the two types from the bz rather than taking a chance that some generic
round up would cause some other issue. Or am I misinterpreting your
comment?
John
---BeginMessage---
Am 11.04.2014 um 16:09 hat Eric Blake geschrieben:
[adding Kevin for a qemu perspective]
On 04/11/2014 07:23 AM, John Ferlan wrote:
On 04/08/2014 12:45 PM, Eric Blake wrote:
On 04/08/2014 10:26 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1002813
$ qemu-img create -f qcow2 img 12345
Formatting 'img', fmt=qcow2 size=12345 encryption=off cluster_size=65536
lazy_refcounts=off
$ qemu-img info img
image: img
file format: qcow2
virtual size: 12K (12288 bytes)
disk size: 196K
cluster_size: 65536
Format specific information:
compat: 1.1
lazy refcounts: false
Wait a second - qemu-img rounded DOWN. That's wrong - it allocated less
bytes than I requested. I think we need to first figure out what's
going on with the qemu side, on whether qemu should be supporting
[libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary
https://bugzilla.redhat.com/show_bug.cgi?id=1002813
If qemuDomainBlockResize() is passed a size not on a KiB boundary - that
is passed a size based in bytes (VIR_DOMAIN_BLOCK_RESIZE_BYTES), then
depending on the source format (qcow2 or qed), the value passed must
be on a sector (or 512 byte) boundary. Since other libvirt code quietly
adjusts the capacity values, then do so here as well - of course ensuring
that adjustment still fits.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4bb4819..3e407d7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9421,6 +9421,7 @@ qemuDomainBlockResize(virDomainPtr dom,
virDomainObjPtr vm;
qemuDomainObjPrivatePtr priv;
int ret = -1, idx;
+unsigned long long size_up;
char *device = NULL;
virDomainDiskDefPtr disk = NULL;
@@ -9441,6 +9442,12 @@ qemuDomainBlockResize(virDomainPtr dom,
return -1;
}
size *= 1024;
+size_up = size;
+} else {
+/* For 'qcow2' and 'qed', qemu resize blocks expects values
+ * on sector boundary, so round our value up to prepare
+ */
+size_up = VIR_ROUND_UP(size, 512);
}
if (!(vm = qemuDomObjFromDomain(dom)))
@@ -9467,6 +9474,21 @@ qemuDomainBlockResize(virDomainPtr dom,
}
disk = vm-def-disks[idx];
+/* qcow2 and qed must be sized appropriately, so be sure our value
+ * is sized appropriately and will fit
+ */
+if (size != size_up
+(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED)) {
+if (size_up ULLONG_MAX) {
+virReportError(VIR_ERR_OVERFLOW,
+ _(size must be less than %llu KiB),
+ ULLONG_MAX / 1024);
+goto endjob;
+}
+size = size_up;
+}
+
if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
disk-info.alias) 0)
goto endjob;
--
1.9.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary
On 04/08/2014 10:26 AM, John Ferlan wrote: https://bugzilla.redhat.com/show_bug.cgi?id=1002813 If qemuDomainBlockResize() is passed a size not on a KiB boundary - that is passed a size based in bytes (VIR_DOMAIN_BLOCK_RESIZE_BYTES), then depending on the source format (qcow2 or qed), the value passed must be on a sector (or 512 byte) boundary. Since other libvirt code quietly adjusts the capacity values, then do so here as well - of course ensuring that adjustment still fits. qed may require aligned multiples for size, but I thought that qcow2 can support an unaligned size (uncommon, but not technically impossible) - after all, the 'size' field in the qcow2 header (bytes 24-31) is an 8-byte value in bytes, not a count of sectors. Maybe we should try the user's size, and only then fall back to a rounded up alignment if the unaligned size fails. Hmm, now that I've experimented a bit: $ qemu-img create -f qcow2 img 12345 Formatting 'img', fmt=qcow2 size=12345 encryption=off cluster_size=65536 lazy_refcounts=off $ qemu-img info img image: img file format: qcow2 virtual size: 12K (12288 bytes) disk size: 196K cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false Wait a second - qemu-img rounded DOWN. That's wrong - it allocated less bytes than I requested. I think we need to first figure out what's going on with the qemu side, on whether qemu should be supporting unaligned requestes, before trying to paper around it in libvirt. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary
On 04/08/2014 12:45 PM, Eric Blake wrote:
On 04/08/2014 10:26 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1002813
If qemuDomainBlockResize() is passed a size not on a KiB boundary - that
is passed a size based in bytes (VIR_DOMAIN_BLOCK_RESIZE_BYTES), then
depending on the source format (qcow2 or qed), the value passed must
be on a sector (or 512 byte) boundary. Since other libvirt code quietly
adjusts the capacity values, then do so here as well - of course ensuring
that adjustment still fits.
qed may require aligned multiples for size, but I thought that qcow2 can
support an unaligned size (uncommon, but not technically impossible) -
after all, the 'size' field in the qcow2 header (bytes 24-31) is an
8-byte value in bytes, not a count of sectors. Maybe we should try the
user's size, and only then fall back to a rounded up alignment if the
unaligned size fails.
Hmm, now that I've experimented a bit:
$ qemu-img create -f qcow2 img 12345
Formatting 'img', fmt=qcow2 size=12345 encryption=off cluster_size=65536
lazy_refcounts=off
$ qemu-img info img
image: img
file format: qcow2
virtual size: 12K (12288 bytes)
disk size: 196K
cluster_size: 65536
Format specific information:
compat: 1.1
lazy refcounts: false
Wait a second - qemu-img rounded DOWN. That's wrong - it allocated less
bytes than I requested. I think we need to first figure out what's
going on with the qemu side, on whether qemu should be supporting
unaligned requestes, before trying to paper around it in libvirt.
According to the bug report qemu requires multiples of sector size for
the size value. Furthermore there's other places in the libvirt code
where we adjust a provided value - virStorageBackendCreateQemuImgCmd()
for example.
So regardless of what qemu-img does with 12345 - we've already set the
precedent of rounding up on the size (cscope VIR_DIV_UP). I'm OK with
truncating or even returning an error, but I was following what Dan said
in the case (comment 4):
Whenever QEMU has granularity constraints, libvirt ought to be rounding
up the user value to nearest acceptable boundary for QEMU. We do this in
many other places, so just need to figure out where todo it for this case.
I did test volumes created as 5K on/for a guest and then attempt to
change to 10K. Adding the volumes is no problem (I used qemu-img -f
{qcow2|qed} disk-5k-{qcow2|qed}). Once attached to the running guest, a
blockresize using 10K failed before my change and succeeded afterwards.
I checked inside the guest /proc/partitions to see that the sizes
changed. Prior to the change the volumes were 5120 bytes and after 10240.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
