Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
On Mon, Jul 27, 2020 at 5:04 PM Eric Blake 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
> > ---
> > 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
>
Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
On Mon, Jul 27, 2020 at 5:04 PM Eric Blake 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
> > ---
> > 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
>
Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
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
---
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
Re: [PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
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
> ---
> 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
signature.asc
Description: OpenPGP digital signature
[PATCH 1/2] block: nbd: Fix convert qcow2 compressed to nbd
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
---
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
