* [PATCH 0/2] Fix convert to qcow2 compressed to NBD
@ 2020-07-26 15:25 Nir Soffer
2020-07-26 15:25 ` [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd Nir Soffer
2020-07-26 15:25 ` [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD Nir Soffer
0 siblings, 2 replies; 13+ messages in thread
From: Nir Soffer @ 2020-07-26 15:25 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: Kevin Wolf, Nir Soffer, Max Reitz
Fix qemu-img convert -O qcow2 -c to NBD URL and add missing test for this
usage.
This already works now, but unfortunately qemu-img fails when trying to
truncate the target image to the same size at the end of the operation.
Nir Soffer (2):
block: nbd: Fix convert qcow2 compressed to nbd
qemu-iotests: Test convert to qcow2 compressed to NBD
block/nbd.c | 27 +++++++++++++
tests/qemu-iotests/302 | 83 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/302.out | 27 +++++++++++++
tests/qemu-iotests/group | 1 +
4 files changed, 138 insertions(+)
create mode 100755 tests/qemu-iotests/302
create mode 100644 tests/qemu-iotests/302.out
--
2.25.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
2020-07-26 15:25 [PATCH 0/2] Fix convert to qcow2 compressed to NBD Nir Soffer
@ 2020-07-26 15:25 ` Nir Soffer
2020-07-27 8:58 ` Max Reitz
2020-07-27 14:04 ` Eric Blake
2020-07-26 15:25 ` [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD Nir Soffer
1 sibling, 2 replies; 13+ messages in thread
From: Nir Soffer @ 2020-07-26 15:25 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: Kevin Wolf, Nir Soffer, Max Reitz
When converting to qcow2 compressed format, the last step is a special
zero length compressed write, ending in call to bdrv_co_truncate(). This
call always fail for the nbd driver since it does not implement
bdrv_co_truncate().
For block devices, which have the same limits, the call succeeds since
file driver implements bdrv_co_truncate(). If the caller asked to
truncate to the same or smaller size with exact=false, the truncate
succeeds. Implement the same logic for nbd.
Example failing without this change:
In one shell starts qemu-nbd:
$ truncate -s 1g test.tar
$ qemu-nbd --socket=/tmp/nbd.sock --persistent --format=raw --offset 1536 test.tar
In another shell convert an image to qcow2 compressed via NBD:
$ echo "disk data" > disk.raw
$ truncate -s 1g disk.raw
$ qemu-img convert -f raw -O qcow2 -c disk1.raw nbd+unix:///?socket=/tmp/nbd.sock; echo $?
1
qemu-img failed, but the conversion was successful:
$ qemu-img info nbd+unix:///?socket=/tmp/nbd.sock
image: nbd+unix://?socket=/tmp/nbd.sock
file format: qcow2
virtual size: 1 GiB (1073741824 bytes)
...
$ qemu-img check nbd+unix:///?socket=/tmp/nbd.sock
No errors were found on the image.
1/16384 = 0.01% allocated, 100.00% fragmented, 100.00% compressed clusters
Image end offset: 393216
$ qemu-img compare disk.raw nbd+unix:///?socket=/tmp/nbd.sock
Images are identical.
Fixes: https://bugzilla.redhat.com/1860627
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
block/nbd.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/block/nbd.c b/block/nbd.c
index 65a4f56924..2154113af3 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1966,6 +1966,30 @@ static void nbd_close(BlockDriverState *bs)
nbd_clear_bdrvstate(s);
}
+/*
+ * NBD cannot truncate, but if the caller ask to truncate to the same size, or
+ * to a smaller size with extact=false, there is not reason to fail the
+ * operation.
+ */
+static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t offset,
+ bool exact, PreallocMode prealloc,
+ BdrvRequestFlags flags, Error **errp)
+{
+ BDRVNBDState *s = bs->opaque;
+
+ if (offset != s->info.size && exact) {
+ error_setg(errp, "Cannot resize NBD nodes");
+ return -ENOTSUP;
+ }
+
+ if (offset > s->info.size) {
+ error_setg(errp, "Cannot grow NBD nodes");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int64_t nbd_getlength(BlockDriverState *bs)
{
BDRVNBDState *s = bs->opaque;
@@ -2045,6 +2069,7 @@ static BlockDriver bdrv_nbd = {
.bdrv_co_flush_to_os = nbd_co_flush,
.bdrv_co_pdiscard = nbd_client_co_pdiscard,
.bdrv_refresh_limits = nbd_refresh_limits,
+ .bdrv_co_truncate = nbd_co_truncate,
.bdrv_getlength = nbd_getlength,
.bdrv_detach_aio_context = nbd_client_detach_aio_context,
.bdrv_attach_aio_context = nbd_client_attach_aio_context,
@@ -2072,6 +2097,7 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_co_flush_to_os = nbd_co_flush,
.bdrv_co_pdiscard = nbd_client_co_pdiscard,
.bdrv_refresh_limits = nbd_refresh_limits,
+ .bdrv_co_truncate = nbd_co_truncate,
.bdrv_getlength = nbd_getlength,
.bdrv_detach_aio_context = nbd_client_detach_aio_context,
.bdrv_attach_aio_context = nbd_client_attach_aio_context,
@@ -2099,6 +2125,7 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_co_flush_to_os = nbd_co_flush,
.bdrv_co_pdiscard = nbd_client_co_pdiscard,
.bdrv_refresh_limits = nbd_refresh_limits,
+ .bdrv_co_truncate = nbd_co_truncate,
.bdrv_getlength = nbd_getlength,
.bdrv_detach_aio_context = nbd_client_detach_aio_context,
.bdrv_attach_aio_context = nbd_client_attach_aio_context,
--
2.25.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD
2020-07-26 15:25 [PATCH 0/2] Fix convert to qcow2 compressed to NBD Nir Soffer
2020-07-26 15:25 ` [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd Nir Soffer
@ 2020-07-26 15:25 ` Nir Soffer
2020-07-27 10:04 ` Max Reitz
1 sibling, 1 reply; 13+ messages in thread
From: Nir Soffer @ 2020-07-26 15:25 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: Kevin Wolf, Nir Soffer, Max Reitz
Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
is writing compressed disk content to OVA archive.
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
tests/qemu-iotests/302 | 83 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/302.out | 27 +++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 111 insertions(+)
create mode 100755 tests/qemu-iotests/302
create mode 100644 tests/qemu-iotests/302.out
diff --git a/tests/qemu-iotests/302 b/tests/qemu-iotests/302
new file mode 100755
index 0000000000..cefde1f7cf
--- /dev/null
+++ b/tests/qemu-iotests/302
@@ -0,0 +1,83 @@
+#!/usr/bin/env python3
+#
+# Tests conveting qcow2 compressed to NBD
+#
+# Copyright (c) 2020 Nir Soffer <nirsof@gmail.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# owner=nirsof@gmail.com
+
+import json
+import iotests
+
+from iotests import (
+ file_path,
+ qemu_img,
+ qemu_img_create,
+ qemu_img_log,
+ qemu_img_pipe,
+ qemu_io,
+ qemu_nbd,
+)
+
+iotests.script_initialize(supported_fmts=["qcow2"])
+
+# Create source disk, format does not matter.
+src_disk = file_path("disk.img")
+qemu_img_create("-f", "raw", src_disk, "10m")
+qemu_io("-f", "raw", "-c", "write 1m 64K", src_disk)
+
+# The use case is writing qcow2 image directly into a tar file. Code to create
+# real tar file not included.
+#
+# offset content
+# -------------------------------
+# 0 first memebr header
+# 512 first member data
+# 1024 second memeber header
+# 1536 second member data
+
+tar_file = file_path("test.tar")
+out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", src_disk)
+measure = json.loads(out)
+qemu_img_create("-f", "raw", tar_file, str(measure["required"]))
+
+nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
+nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
+
+# Use raw format to allow creating qcow2 directy into tar file.
+qemu_nbd(
+ "--socket", nbd_sock,
+ "--persistent",
+ "--export-name", "exp",
+ "--format", "raw",
+ "--offset", "1536",
+ tar_file)
+
+iotests.log("=== Target image info ===")
+qemu_img_log("info", nbd_uri)
+
+# Write image into the tar file. In a real applicatio we would write a tar
+# entry after writing the image.
+qemu_img("convert", "-f", "raw", "-O", "qcow2", "-c", src_disk, nbd_uri)
+
+iotests.log("=== Converted image info ===")
+qemu_img_log("info", nbd_uri)
+
+iotests.log("=== Converted image check ===")
+qemu_img_log("check", nbd_uri)
+
+iotests.log("=== Comparing to source disk ===")
+qemu_img_log("compare", src_disk, nbd_uri)
diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
new file mode 100644
index 0000000000..babef3d574
--- /dev/null
+++ b/tests/qemu-iotests/302.out
@@ -0,0 +1,27 @@
+=== Target image info ===
+image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
+file format: raw
+virtual size: 446 KiB (457216 bytes)
+disk size: unavailable
+
+=== Converted image info ===
+image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
+file format: qcow2
+virtual size: 10 MiB (10485760 bytes)
+disk size: unavailable
+cluster_size: 65536
+Format specific information:
+ compat: 1.1
+ compression type: zlib
+ lazy refcounts: false
+ refcount bits: 16
+ corrupt: false
+
+=== Converted image check ===
+No errors were found on the image.
+1/160 = 0.62% allocated, 100.00% fragmented, 100.00% compressed clusters
+Image end offset: 393216
+
+=== Comparing to source disk ===
+Images are identical.
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1d0252e1f0..1e1cb27bc8 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -308,3 +308,4 @@
297 meta
299 auto quick
301 backing quick
+302 quick
--
2.25.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
2020-07-26 15:25 ` [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd Nir Soffer
@ 2020-07-27 8:58 ` Max Reitz
2020-07-27 14:04 ` Eric Blake
1 sibling, 0 replies; 13+ messages in thread
From: Max Reitz @ 2020-07-27 8:58 UTC (permalink / raw)
To: Nir Soffer, qemu-block, qemu-devel; +Cc: Kevin Wolf, Nir Soffer
[-- Attachment #1.1: Type: text/plain, Size: 3074 bytes --]
On 26.07.20 17:25, Nir Soffer wrote:
> When converting to qcow2 compressed format, the last step is a special
> zero length compressed write, ending in call to bdrv_co_truncate(). This
> call always fail for the nbd driver since it does not implement
> bdrv_co_truncate().
>
> For block devices, which have the same limits, the call succeeds since
> file driver implements bdrv_co_truncate(). If the caller asked to
> truncate to the same or smaller size with exact=false, the truncate
> succeeds. Implement the same logic for nbd.
>
> Example failing without this change:
>
> In one shell starts qemu-nbd:
>
> $ truncate -s 1g test.tar
> $ qemu-nbd --socket=/tmp/nbd.sock --persistent --format=raw --offset 1536 test.tar
>
> In another shell convert an image to qcow2 compressed via NBD:
>
> $ echo "disk data" > disk.raw
> $ truncate -s 1g disk.raw
> $ qemu-img convert -f raw -O qcow2 -c disk1.raw nbd+unix:///?socket=/tmp/nbd.sock; echo $?
> 1
>
> qemu-img failed, but the conversion was successful:
>
> $ qemu-img info nbd+unix:///?socket=/tmp/nbd.sock
> image: nbd+unix://?socket=/tmp/nbd.sock
> file format: qcow2
> virtual size: 1 GiB (1073741824 bytes)
> ...
>
> $ qemu-img check nbd+unix:///?socket=/tmp/nbd.sock
> No errors were found on the image.
> 1/16384 = 0.01% allocated, 100.00% fragmented, 100.00% compressed clusters
> Image end offset: 393216
>
> $ qemu-img compare disk.raw nbd+unix:///?socket=/tmp/nbd.sock
> Images are identical.
>
> Fixes: https://bugzilla.redhat.com/1860627
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
> block/nbd.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 65a4f56924..2154113af3 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1966,6 +1966,30 @@ static void nbd_close(BlockDriverState *bs)
> nbd_clear_bdrvstate(s);
> }
>
> +/*
> + * NBD cannot truncate, but if the caller ask to truncate to the same size, or
> + * to a smaller size with extact=false, there is not reason to fail the
> + * operation.
> + */
> +static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t offset,
> + bool exact, PreallocMode prealloc,
> + BdrvRequestFlags flags, Error **errp)
> +{
> + BDRVNBDState *s = bs->opaque;
> +
> + if (offset != s->info.size && exact) {
> + error_setg(errp, "Cannot resize NBD nodes");
> + return -ENOTSUP;
> + }
> +
> + if (offset > s->info.size) {
> + error_setg(errp, "Cannot grow NBD nodes");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Can we also check @prealloc, like iscsi.c does it?
(Yes, preallocation only makes sense for growing, but you can for
example truncate a 2 GB NBD node to 1 GB, and then grow it to 1.5 GB,
and for the latter operation it might make sense to give a preallocation
parameter. (Except block_resize doesn’t allow that, but, well.))
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD
2020-07-26 15:25 ` [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD Nir Soffer
@ 2020-07-27 10:04 ` Max Reitz
2020-07-27 14:14 ` Eric Blake
2020-07-27 14:44 ` Nir Soffer
0 siblings, 2 replies; 13+ messages in thread
From: Max Reitz @ 2020-07-27 10:04 UTC (permalink / raw)
To: Nir Soffer, qemu-block, qemu-devel; +Cc: Kevin Wolf, Nir Soffer
[-- Attachment #1.1: Type: text/plain, Size: 5326 bytes --]
On 26.07.20 17:25, Nir Soffer wrote:
> Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
> is writing compressed disk content to OVA archive.
>
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
> tests/qemu-iotests/302 | 83 ++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/302.out | 27 +++++++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 111 insertions(+)
> create mode 100755 tests/qemu-iotests/302
> create mode 100644 tests/qemu-iotests/302.out
>
> diff --git a/tests/qemu-iotests/302 b/tests/qemu-iotests/302
> new file mode 100755
> index 0000000000..cefde1f7cf
> --- /dev/null
> +++ b/tests/qemu-iotests/302
> @@ -0,0 +1,83 @@
> +#!/usr/bin/env python3
> +#
> +# Tests conveting qcow2 compressed to NBD
*converting
> +#
> +# Copyright (c) 2020 Nir Soffer <nirsof@gmail.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +# owner=nirsof@gmail.com
> +
> +import json
> +import iotests
> +
> +from iotests import (
> + file_path,
> + qemu_img,
> + qemu_img_create,
> + qemu_img_log,
> + qemu_img_pipe,
> + qemu_io,
> + qemu_nbd,
> +)
> +
> +iotests.script_initialize(supported_fmts=["qcow2"])
> +
> +# Create source disk, format does not matter.
> +src_disk = file_path("disk.img")
> +qemu_img_create("-f", "raw", src_disk, "10m")
If the format doesn’t matter, why not just use qcow2 and so put
iotests.imgfmt here? (And everywhere else where you now have -f raw.)
> +qemu_io("-f", "raw", "-c", "write 1m 64K", src_disk)
(Except I think qemu_io already has -f qcow2 in its arguments by
default, so specifying the format wouldn’t even be necessary here.)
> +# The use case is writing qcow2 image directly into a tar file. Code to create
> +# real tar file not included.
> +#
> +# offset content
> +# -------------------------------
> +# 0 first memebr header
*member
> +# 512 first member data
> +# 1024 second memeber header
*member
> +# 1536 second member data
> +
> +tar_file = file_path("test.tar")
> +out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", src_disk)
> +measure = json.loads(out)
> +qemu_img_create("-f", "raw", tar_file, str(measure["required"]))
Should this be measure["required"] + 1536?
> +
> +nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
> +nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
> +
> +# Use raw format to allow creating qcow2 directy into tar file.
> +qemu_nbd(
> + "--socket", nbd_sock,
> + "--persistent",
> + "--export-name", "exp",
> + "--format", "raw",
> + "--offset", "1536",
> + tar_file)
> +
> +iotests.log("=== Target image info ===")
> +qemu_img_log("info", nbd_uri)
> +
> +# Write image into the tar file. In a real applicatio we would write a tar
*application
> +# entry after writing the image.
> +qemu_img("convert", "-f", "raw", "-O", "qcow2", "-c", src_disk, nbd_uri)
> +
> +iotests.log("=== Converted image info ===")
> +qemu_img_log("info", nbd_uri)
> +
> +iotests.log("=== Converted image check ===")
> +qemu_img_log("check", nbd_uri)
> +
> +iotests.log("=== Comparing to source disk ===")
> +qemu_img_log("compare", src_disk, nbd_uri)
> diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
> new file mode 100644
> index 0000000000..babef3d574
> --- /dev/null
> +++ b/tests/qemu-iotests/302.out
> @@ -0,0 +1,27 @@
> +=== Target image info ===
> +image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
> +file format: raw
> +virtual size: 446 KiB (457216 bytes)
> +disk size: unavailable
> +
> +=== Converted image info ===
> +image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
> +file format: qcow2
> +virtual size: 10 MiB (10485760 bytes)
> +disk size: unavailable
> +cluster_size: 65536
> +Format specific information:
> + compat: 1.1
> + compression type: zlib
> + lazy refcounts: false
> + refcount bits: 16
> + corrupt: false
> +
> +=== Converted image check ===
> +No errors were found on the image.
> +1/160 = 0.62% allocated, 100.00% fragmented, 100.00% compressed clusters
> +Image end offset: 393216
I hope none of this is fs-dependant. (I don’t think it is, but who
knows. I suppose we’ll find out.)
Max
> +=== Comparing to source disk ===
> +Images are identical.
> +
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 1d0252e1f0..1e1cb27bc8 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -308,3 +308,4 @@
> 297 meta
> 299 auto quick
> 301 backing quick
> +302 quick
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
2020-07-26 15:25 ` [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd Nir Soffer
2020-07-27 8:58 ` Max Reitz
@ 2020-07-27 14:04 ` Eric Blake
2020-07-27 14:52 ` Nir Soffer
2020-07-27 15:12 ` Nir Soffer
1 sibling, 2 replies; 13+ messages in thread
From: Eric Blake @ 2020-07-27 14:04 UTC (permalink / raw)
To: Nir Soffer, qemu-block, qemu-devel; +Cc: Kevin Wolf, Nir Soffer, Max Reitz
On 7/26/20 10:25 AM, Nir Soffer wrote:
> When converting to qcow2 compressed format, the last step is a special
> zero length compressed write, ending in call to bdrv_co_truncate(). This
> call always fail for the nbd driver since it does not implement
fails
> bdrv_co_truncate().
Arguably, qemu-img should be taught to ignore the failure, since it is
not unique to the nbd driver. But I can live with your approach here.
>
> For block devices, which have the same limits, the call succeeds since
> file driver implements bdrv_co_truncate(). If the caller asked to
> truncate to the same or smaller size with exact=false, the truncate
> succeeds. Implement the same logic for nbd.
>
> Example failing without this change:
>
>
> Fixes: https://bugzilla.redhat.com/1860627
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
> block/nbd.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 65a4f56924..2154113af3 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1966,6 +1966,30 @@ static void nbd_close(BlockDriverState *bs)
> nbd_clear_bdrvstate(s);
> }
>
> +/*
> + * NBD cannot truncate, but if the caller ask to truncate to the same size, or
asks
> + * to a smaller size with extact=false, there is not reason to fail the
exact, no
> + * operation.
> + */
> +static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t offset,
> + bool exact, PreallocMode prealloc,
> + BdrvRequestFlags flags, Error **errp)
> +{
> + BDRVNBDState *s = bs->opaque;
> +
> + if (offset != s->info.size && exact) {
> + error_setg(errp, "Cannot resize NBD nodes");
> + return -ENOTSUP;
> + }
> +
> + if (offset > s->info.size) {
> + error_setg(errp, "Cannot grow NBD nodes");
> + return -EINVAL;
> + }
> +
> + return 0;
Looks reasonable. As Max said, I wonder if we want to reject particular
preallocation modes (looking at block/file-posix.c:raw_co_truncate), in
the case where the image was resized down and then back up (since
s->info.size is constant, but the BDS size is not if inexact resize
succeeds).
As you have a bugzilla entry, I think this is safe for -rc2; I'll be
touching up the typos and queuing it through my NBD tree later today.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD
2020-07-27 10:04 ` Max Reitz
@ 2020-07-27 14:14 ` Eric Blake
2020-07-27 14:35 ` Nir Soffer
2020-07-27 14:44 ` Nir Soffer
1 sibling, 1 reply; 13+ messages in thread
From: Eric Blake @ 2020-07-27 14:14 UTC (permalink / raw)
To: Max Reitz, Nir Soffer, qemu-block, qemu-devel; +Cc: Kevin Wolf, Nir Soffer
On 7/27/20 5:04 AM, Max Reitz wrote:
> On 26.07.20 17:25, Nir Soffer wrote:
>> Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
>> is writing compressed disk content to OVA archive.
>>
>> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
>> ---
>
>> +# The use case is writing qcow2 image directly into a tar file. Code to create
>> +# real tar file not included.
>> +#
>> +# offset content
>> +# -------------------------------
>> +# 0 first memebr header
>
> *member
>
>> +# 512 first member data
>> +# 1024 second memeber header
>
> *member
>
>> +# 1536 second member data
>> +
>> +tar_file = file_path("test.tar")
I guess it's okay that you don't create a real tar file here, but
listing the commands to create it (even as a comment) is better than
just saying "trust me". And it doesn't seem like that much more work -
it looks like the key to your test is that you created a tar file
containing two files, where the first file was less than 512 bytes and
the second file is your target destination that you will be rewriting.
>> +out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", src_disk)
>> +measure = json.loads(out)
>> +qemu_img_create("-f", "raw", tar_file, str(measure["required"]))
>
> Should this be measure["required"] + 1536?
The test works without it (because of compression), but yes, if you are
going to test writing into an offset, you should oversize your file by
that same offset.
>
>> +
>> +nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
>> +nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
>> +
>> +# Use raw format to allow creating qcow2 directy into tar file.
>> +qemu_nbd(
>> + "--socket", nbd_sock,
>> + "--persistent",
>> + "--export-name", "exp",
>> + "--format", "raw",
>> + "--offset", "1536",
>> + tar_file)
>> +
>> +iotests.log("=== Target image info ===")
>> +qemu_img_log("info", nbd_uri)
>> +
>> +# Write image into the tar file. In a real applicatio we would write a tar
>
> *application
>
>> +=== Converted image check ===
>> +No errors were found on the image.
>> +1/160 = 0.62% allocated, 100.00% fragmented, 100.00% compressed clusters
>> +Image end offset: 393216
>
> I hope none of this is fs-dependant. (I don’t think it is, but who
> knows. I suppose we’ll find out.)
Indeed - time to see what CI thinks of this.
At any rate, given the urgency of getting pull requests for -rc2 in
before slamming Peter tomorrow, I'll probably try to touch up the issues
Max pointed out and queue it today.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD
2020-07-27 14:14 ` Eric Blake
@ 2020-07-27 14:35 ` Nir Soffer
2020-07-27 14:41 ` Eric Blake
0 siblings, 1 reply; 13+ messages in thread
From: Nir Soffer @ 2020-07-27 14:35 UTC (permalink / raw)
To: Eric Blake; +Cc: QEMU Developers, Kevin Wolf, qemu-block, Nir Soffer, Max Reitz
On Mon, Jul 27, 2020 at 5:14 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 7/27/20 5:04 AM, Max Reitz wrote:
> > On 26.07.20 17:25, Nir Soffer wrote:
> >> Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
> >> is writing compressed disk content to OVA archive.
> >>
> >> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> >> ---
>
> >
> >> +# The use case is writing qcow2 image directly into a tar file. Code to create
> >> +# real tar file not included.
> >> +#
> >> +# offset content
> >> +# -------------------------------
> >> +# 0 first memebr header
> >
> > *member
Sorry for the typos, I need to setup automated spelling check :-)
> >
> >> +# 512 first member data
> >> +# 1024 second memeber header
> >
> > *member
> >
> >> +# 1536 second member data
> >> +
> >> +tar_file = file_path("test.tar")
>
> I guess it's okay that you don't create a real tar file here, but
> listing the commands to create it (even as a comment) is better than
> just saying "trust me". And it doesn't seem like that much more work -
> it looks like the key to your test is that you created a tar file
> containing two files, where the first file was less than 512 bytes and
> the second file is your target destination that you will be rewriting.
The real code is more complicated, something like:
offset = tar.fileobj.tell() + BLOCK_SIZE
with open(tar.name, "r+") as f:
f.truncate(offset + measure["required"])
convert_image(image, tar.name, offset)
check = check_image(tar.name, offset)
size = check["image-end-offset"]
member = tarfile.TarInfo(name)
member.size = size
tar.addfile(member)
tar_size = offset + round_up(size)
tar.fileobj.seek(tar_size)
with open(tar.name, "r+") as f:
f.truncate(tar_size)
I'm not sure it helps qemu developers working on these tests.
> >> +out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", src_disk)
> >> +measure = json.loads(out)
> >> +qemu_img_create("-f", "raw", tar_file, str(measure["required"]))
> >
> > Should this be measure["required"] + 1536?
>
> The test works without it (because of compression), but yes, if you are
> going to test writing into an offset, you should oversize your file by
> that same offset.
Right, in the real code using this I indeed use offset + required.
> >> +
> >> +nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
> >> +nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
> >> +
> >> +# Use raw format to allow creating qcow2 directy into tar file.
> >> +qemu_nbd(
> >> + "--socket", nbd_sock,
> >> + "--persistent",
> >> + "--export-name", "exp",
> >> + "--format", "raw",
> >> + "--offset", "1536",
> >> + tar_file)
> >> +
> >> +iotests.log("=== Target image info ===")
> >> +qemu_img_log("info", nbd_uri)
> >> +
> >> +# Write image into the tar file. In a real applicatio we would write a tar
> >
> > *application
> >
>
> >> +=== Converted image check ===
> >> +No errors were found on the image.
> >> +1/160 = 0.62% allocated, 100.00% fragmented, 100.00% compressed clusters
> >> +Image end offset: 393216
> >
> > I hope none of this is fs-dependant. (I don’t think it is, but who
> > knows. I suppose we’ll find out.)
>
> Indeed - time to see what CI thinks of this.
>
> At any rate, given the urgency of getting pull requests for -rc2 in
> before slamming Peter tomorrow, I'll probably try to touch up the issues
> Max pointed out and queue it today.
Thanks Max and Eric.
Should I post a fixed version later today?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD
2020-07-27 14:35 ` Nir Soffer
@ 2020-07-27 14:41 ` Eric Blake
2020-07-27 14:44 ` Nir Soffer
0 siblings, 1 reply; 13+ messages in thread
From: Eric Blake @ 2020-07-27 14:41 UTC (permalink / raw)
To: Nir Soffer; +Cc: QEMU Developers, Kevin Wolf, qemu-block, Nir Soffer, Max Reitz
On 7/27/20 9:35 AM, Nir Soffer wrote:
>> I guess it's okay that you don't create a real tar file here, but
>> listing the commands to create it (even as a comment) is better than
>> just saying "trust me". And it doesn't seem like that much more work -
>> it looks like the key to your test is that you created a tar file
>> containing two files, where the first file was less than 512 bytes and
>> the second file is your target destination that you will be rewriting.
>
> The real code is more complicated, something like:
>
> offset = tar.fileobj.tell() + BLOCK_SIZE
>
> with open(tar.name, "r+") as f:
> f.truncate(offset + measure["required"])
>
> convert_image(image, tar.name, offset)
>
> check = check_image(tar.name, offset)
> size = check["image-end-offset"]
>
> member = tarfile.TarInfo(name)
> member.size = size
> tar.addfile(member)
>
> tar_size = offset + round_up(size)
>
> tar.fileobj.seek(tar_size)
> with open(tar.name, "r+") as f:
> f.truncate(tar_size)
>
> I'm not sure it helps qemu developers working on these tests.
The closer the iotest is to reality, the more likely it will serve as a
good regression test. Cutting corners risks a test that passes in
isolation even when we've done something that breaks the overall process
in one of the corners you cut.
>>
>> At any rate, given the urgency of getting pull requests for -rc2 in
>> before slamming Peter tomorrow, I'll probably try to touch up the issues
>> Max pointed out and queue it today.
>
> Thanks Max and Eric.
>
> Should I post a fixed version later today?
A v2 would be helpful.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD
2020-07-27 14:41 ` Eric Blake
@ 2020-07-27 14:44 ` Nir Soffer
0 siblings, 0 replies; 13+ messages in thread
From: Nir Soffer @ 2020-07-27 14:44 UTC (permalink / raw)
To: Eric Blake; +Cc: QEMU Developers, Kevin Wolf, qemu-block, Nir Soffer, Max Reitz
On Mon, Jul 27, 2020 at 5:41 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 7/27/20 9:35 AM, Nir Soffer wrote:
>
> >> I guess it's okay that you don't create a real tar file here, but
> >> listing the commands to create it (even as a comment) is better than
> >> just saying "trust me". And it doesn't seem like that much more work -
> >> it looks like the key to your test is that you created a tar file
> >> containing two files, where the first file was less than 512 bytes and
> >> the second file is your target destination that you will be rewriting.
> >
> > The real code is more complicated, something like:
> >
> > offset = tar.fileobj.tell() + BLOCK_SIZE
> >
> > with open(tar.name, "r+") as f:
> > f.truncate(offset + measure["required"])
> >
> > convert_image(image, tar.name, offset)
> >
> > check = check_image(tar.name, offset)
> > size = check["image-end-offset"]
> >
> > member = tarfile.TarInfo(name)
> > member.size = size
> > tar.addfile(member)
> >
> > tar_size = offset + round_up(size)
> >
> > tar.fileobj.seek(tar_size)
> > with open(tar.name, "r+") as f:
> > f.truncate(tar_size)
> >
> > I'm not sure it helps qemu developers working on these tests.
>
> The closer the iotest is to reality, the more likely it will serve as a
> good regression test. Cutting corners risks a test that passes in
> isolation even when we've done something that breaks the overall process
> in one of the corners you cut.
I'll add this code then.
> >>
> >> At any rate, given the urgency of getting pull requests for -rc2 in
> >> before slamming Peter tomorrow, I'll probably try to touch up the issues
> >> Max pointed out and queue it today.
> >
> > Thanks Max and Eric.
> >
> > Should I post a fixed version later today?
>
> A v2 would be helpful.
Will post later today.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD
2020-07-27 10:04 ` Max Reitz
2020-07-27 14:14 ` Eric Blake
@ 2020-07-27 14:44 ` Nir Soffer
1 sibling, 0 replies; 13+ messages in thread
From: Nir Soffer @ 2020-07-27 14:44 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-block, Nir Soffer, QEMU Developers
On Mon, Jul 27, 2020 at 1:05 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 26.07.20 17:25, Nir Soffer wrote:
> > Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
> > is writing compressed disk content to OVA archive.
> >
> > Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> > ---
> > tests/qemu-iotests/302 | 83 ++++++++++++++++++++++++++++++++++++++
> > tests/qemu-iotests/302.out | 27 +++++++++++++
> > tests/qemu-iotests/group | 1 +
> > 3 files changed, 111 insertions(+)
> > create mode 100755 tests/qemu-iotests/302
> > create mode 100644 tests/qemu-iotests/302.out
> >
> > diff --git a/tests/qemu-iotests/302 b/tests/qemu-iotests/302
> > new file mode 100755
> > index 0000000000..cefde1f7cf
> > --- /dev/null
> > +++ b/tests/qemu-iotests/302
> > @@ -0,0 +1,83 @@
> > +#!/usr/bin/env python3
> > +#
> > +# Tests conveting qcow2 compressed to NBD
>
> *converting
>
> > +#
> > +# Copyright (c) 2020 Nir Soffer <nirsof@gmail.com>
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> > +#
> > +# owner=nirsof@gmail.com
> > +
> > +import json
> > +import iotests
> > +
> > +from iotests import (
> > + file_path,
> > + qemu_img,
> > + qemu_img_create,
> > + qemu_img_log,
> > + qemu_img_pipe,
> > + qemu_io,
> > + qemu_nbd,
> > +)
> > +
> > +iotests.script_initialize(supported_fmts=["qcow2"])
> > +
> > +# Create source disk, format does not matter.
> > +src_disk = file_path("disk.img")
> > +qemu_img_create("-f", "raw", src_disk, "10m")
>
> If the format doesn’t matter, why not just use qcow2 and so put
> iotests.imgfmt here? (And everywhere else where you now have -f raw.)
I tried to use the simplest setup that is less likely to break, but
thinking about
CI environments with strange storage, maybe using qcow2 source disk will be
more robust.
> > +qemu_io("-f", "raw", "-c", "write 1m 64K", src_disk)
>
> (Except I think qemu_io already has -f qcow2 in its arguments by
> default, so specifying the format wouldn’t even be necessary here.)
>
> > +# The use case is writing qcow2 image directly into a tar file. Code to create
> > +# real tar file not included.
> > +#
> > +# offset content
> > +# -------------------------------
> > +# 0 first memebr header
>
> *member
>
> > +# 512 first member data
> > +# 1024 second memeber header
>
> *member
>
> > +# 1536 second member data
> > +
> > +tar_file = file_path("test.tar")
> > +out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", src_disk)
> > +measure = json.loads(out)
> > +qemu_img_create("-f", "raw", tar_file, str(measure["required"]))
>
> Should this be measure["required"] + 1536?
>
> > +
> > +nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
> > +nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
> > +
> > +# Use raw format to allow creating qcow2 directy into tar file.
> > +qemu_nbd(
> > + "--socket", nbd_sock,
> > + "--persistent",
> > + "--export-name", "exp",
> > + "--format", "raw",
> > + "--offset", "1536",
> > + tar_file)
> > +
> > +iotests.log("=== Target image info ===")
> > +qemu_img_log("info", nbd_uri)
> > +
> > +# Write image into the tar file. In a real applicatio we would write a tar
>
> *application
>
> > +# entry after writing the image.
> > +qemu_img("convert", "-f", "raw", "-O", "qcow2", "-c", src_disk, nbd_uri)
> > +
> > +iotests.log("=== Converted image info ===")
> > +qemu_img_log("info", nbd_uri)
> > +
> > +iotests.log("=== Converted image check ===")
> > +qemu_img_log("check", nbd_uri)
> > +
> > +iotests.log("=== Comparing to source disk ===")
> > +qemu_img_log("compare", src_disk, nbd_uri)
> > diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
> > new file mode 100644
> > index 0000000000..babef3d574
> > --- /dev/null
> > +++ b/tests/qemu-iotests/302.out
> > @@ -0,0 +1,27 @@
> > +=== Target image info ===
> > +image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
> > +file format: raw
> > +virtual size: 446 KiB (457216 bytes)
> > +disk size: unavailable
> > +
> > +=== Converted image info ===
> > +image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
> > +file format: qcow2
> > +virtual size: 10 MiB (10485760 bytes)
> > +disk size: unavailable
> > +cluster_size: 65536
> > +Format specific information:
> > + compat: 1.1
> > + compression type: zlib
> > + lazy refcounts: false
> > + refcount bits: 16
> > + corrupt: false
> > +
> > +=== Converted image check ===
> > +No errors were found on the image.
> > +1/160 = 0.62% allocated, 100.00% fragmented, 100.00% compressed clusters
> > +Image end offset: 393216
>
> I hope none of this is fs-dependant. (I don’t think it is, but who
> knows. I suppose we’ll find out.)
I don't think the check would be affected, but maybe the compare step
can use the strict
option if we change to qcow2 source disk.
>
> Max
>
> > +=== Comparing to source disk ===
> > +Images are identical.
> > +
> > diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> > index 1d0252e1f0..1e1cb27bc8 100644
> > --- a/tests/qemu-iotests/group
> > +++ b/tests/qemu-iotests/group
> > @@ -308,3 +308,4 @@
> > 297 meta
> > 299 auto quick
> > 301 backing quick
> > +302 quick
> >
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
2020-07-27 14:04 ` Eric Blake
@ 2020-07-27 14:52 ` Nir Soffer
2020-07-27 15:12 ` Nir Soffer
1 sibling, 0 replies; 13+ messages in thread
From: Nir Soffer @ 2020-07-27 14:52 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, Max Reitz, qemu-block, Nir Soffer, QEMU Developers
On Mon, Jul 27, 2020 at 5:04 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 7/26/20 10:25 AM, Nir Soffer wrote:
> > When converting to qcow2 compressed format, the last step is a special
> > zero length compressed write, ending in call to bdrv_co_truncate(). This
> > call always fail for the nbd driver since it does not implement
>
> fails
>
> > bdrv_co_truncate().
>
> Arguably, qemu-img should be taught to ignore the failure, since it is
> not unique to the nbd driver. But I can live with your approach here.
I started with ignoring ENOTSUP in qcow2, but felt less safe about this
approach since the same issue may happen in other flows, and making nbd
driver behave like a block device looks like a safer change.
> > For block devices, which have the same limits, the call succeeds since
> > file driver implements bdrv_co_truncate(). If the caller asked to
> > truncate to the same or smaller size with exact=false, the truncate
> > succeeds. Implement the same logic for nbd.
> >
> > Example failing without this change:
> >
>
> >
> > Fixes: https://bugzilla.redhat.com/1860627
> > Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> > ---
> > block/nbd.c | 27 +++++++++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/block/nbd.c b/block/nbd.c
> > index 65a4f56924..2154113af3 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -1966,6 +1966,30 @@ static void nbd_close(BlockDriverState *bs)
> > nbd_clear_bdrvstate(s);
> > }
> >
> > +/*
> > + * NBD cannot truncate, but if the caller ask to truncate to the same size, or
>
> asks
>
> > + * to a smaller size with extact=false, there is not reason to fail the
>
> exact, no
>
> > + * operation.
> > + */
> > +static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t offset,
> > + bool exact, PreallocMode prealloc,
> > + BdrvRequestFlags flags, Error **errp)
> > +{
> > + BDRVNBDState *s = bs->opaque;
> > +
> > + if (offset != s->info.size && exact) {
> > + error_setg(errp, "Cannot resize NBD nodes");
> > + return -ENOTSUP;
> > + }
> > +
> > + if (offset > s->info.size) {
> > + error_setg(errp, "Cannot grow NBD nodes");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
>
> Looks reasonable. As Max said, I wonder if we want to reject particular
> preallocation modes (looking at block/file-posix.c:raw_co_truncate), in
> the case where the image was resized down and then back up (since
> s->info.size is constant, but the BDS size is not if inexact resize
> succeeds).
>
> As you have a bugzilla entry, I think this is safe for -rc2; I'll be
> touching up the typos and queuing it through my NBD tree later today.
I'll post v2 with the test fixes later today.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
2020-07-27 14:04 ` Eric Blake
2020-07-27 14:52 ` Nir Soffer
@ 2020-07-27 15:12 ` Nir Soffer
1 sibling, 0 replies; 13+ messages in thread
From: Nir Soffer @ 2020-07-27 15:12 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, Max Reitz, qemu-block, Nir Soffer, QEMU Developers
On Mon, Jul 27, 2020 at 5:04 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 7/26/20 10:25 AM, Nir Soffer wrote:
> > When converting to qcow2 compressed format, the last step is a special
> > zero length compressed write, ending in call to bdrv_co_truncate(). This
> > call always fail for the nbd driver since it does not implement
>
> fails
>
> > bdrv_co_truncate().
>
> Arguably, qemu-img should be taught to ignore the failure, since it is
> not unique to the nbd driver. But I can live with your approach here.
>
> >
> > For block devices, which have the same limits, the call succeeds since
> > file driver implements bdrv_co_truncate(). If the caller asked to
> > truncate to the same or smaller size with exact=false, the truncate
> > succeeds. Implement the same logic for nbd.
> >
> > Example failing without this change:
> >
>
> >
> > Fixes: https://bugzilla.redhat.com/1860627
> > Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> > ---
> > block/nbd.c | 27 +++++++++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/block/nbd.c b/block/nbd.c
> > index 65a4f56924..2154113af3 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -1966,6 +1966,30 @@ static void nbd_close(BlockDriverState *bs)
> > nbd_clear_bdrvstate(s);
> > }
> >
> > +/*
> > + * NBD cannot truncate, but if the caller ask to truncate to the same size, or
>
> asks
>
> > + * to a smaller size with extact=false, there is not reason to fail the
>
> exact, no
>
> > + * operation.
> > + */
> > +static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t offset,
> > + bool exact, PreallocMode prealloc,
> > + BdrvRequestFlags flags, Error **errp)
> > +{
> > + BDRVNBDState *s = bs->opaque;
> > +
> > + if (offset != s->info.size && exact) {
> > + error_setg(errp, "Cannot resize NBD nodes");
> > + return -ENOTSUP;
> > + }
> > +
> > + if (offset > s->info.size) {
> > + error_setg(errp, "Cannot grow NBD nodes");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
>
> Looks reasonable. As Max said, I wonder if we want to reject particular
> preallocation modes (looking at block/file-posix.c:raw_co_truncate), in
> the case where the image was resized down and then back up (since
> s->info.size is constant, but the BDS size is not if inexact resize
> succeeds).
Do we want to fail if someone specifies -o preallocation={falloc,full}?
I see we convert DRV_REQ_MAY_UNMAP to NBD_CMD_FLAG_NO_HOLE
so using -o preallocation=falloc,full should be correct. But the last
request zero
length write request does not do anything, so failing does not look useful.
> As you have a bugzilla entry, I think this is safe for -rc2; I'll be
> touching up the typos and queuing it through my NBD tree later today.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-07-27 15:15 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-26 15:25 [PATCH 0/2] Fix convert to qcow2 compressed to NBD Nir Soffer
2020-07-26 15:25 ` [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd Nir Soffer
2020-07-27 8:58 ` Max Reitz
2020-07-27 14:04 ` Eric Blake
2020-07-27 14:52 ` Nir Soffer
2020-07-27 15:12 ` Nir Soffer
2020-07-26 15:25 ` [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD Nir Soffer
2020-07-27 10:04 ` Max Reitz
2020-07-27 14:14 ` Eric Blake
2020-07-27 14:35 ` Nir Soffer
2020-07-27 14:41 ` Eric Blake
2020-07-27 14:44 ` Nir Soffer
2020-07-27 14:44 ` Nir Soffer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.