[Qemu-devel] [PATCH for-3.1? 0/3] NBD dirty bitmap cleanups

[Qemu-devel] [PATCH for-3.1? 0/3] NBD dirty bitmap cleanups

[Eric Blake]

I'm working on a larger series for 4.0 that will add
'qemu-nbd --list' to show all possible information about a
server's exports, to make it easier to diagnose if the right
exports and bitmaps are present.  But in the process, I found
a few bugs that may be worth fixing in 3.1.  By itself, this
series does not warrant -rc4 (particularly since all of the
problems exist in the 3.0 release, so this is not a regression
fix), but if we have -rc4 for other reasons, these would make
incremental backup support a bit more reliable.

Eric Blake (3):
  nbd/server: Advertise all contexts in response to bare LIST
  nbd/client: Make x-dirty-bitmap more reliable
  nbd/client: Send NBD_CMD_DISC if open fails after connect

 block/nbd-client.c | 21 ++++++++++++++++++++-
 nbd/server.c       |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

-- 
2.17.2

[Qemu-devel] [PATCH 1/3] nbd/server: Advertise all contexts in response to bare LIST

[Eric Blake]

The NBD spec, and even our code comment, says that if the client
asks for NBD_OPT_LIST_META_CONTEXT with 0 queries, then we should
reply with (a possibly-compressed representation of) ALL contexts
that we are willing to let them try.  But commit 3d068aff forgot
to advertise qemu:dirty-bitmap:FOO.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nbd/server.c b/nbd/server.c
index dc04513de70..7af0ddffb20 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -978,6 +978,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
     if (client->opt == NBD_OPT_LIST_META_CONTEXT && !nb_queries) {
         /* enable all known contexts */
         meta->base_allocation = true;
+        meta->bitmap = !!meta->exp->export_bitmap;
     } else {
         for (i = 0; i < nb_queries; ++i) {
             ret = nbd_negotiate_meta_query(client, meta, errp);
-- 
2.17.2

[Qemu-devel] [PATCH 2/3] nbd/client: Make x-dirty-bitmap more reliable

[Eric Blake]

The implementation of x-dirty-bitmap in qemu 3.0 silently
falls back to treating the server as not supporting
NBD_CMD_BLOCK_STATUS if a requested meta_context name was not
negotiated, which in turn means treating the _entire_ image as
data. Since our hack relied on using 'qemu-img map' to view
which portions of the image were dirty by seeing what the
redirected bdrv_block_status() treats as holes, this means
that our fallback treats the entire image as clean.  Better
would have been to treat the entire image as dirty, or to fail
to connect because the user's request for a specific context
could not be honored. This patch goes with the latter.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 76e9ca3abeb..e6e27dafa6a 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -992,6 +992,11 @@ int nbd_client_init(BlockDriverState *bs,
         logout("Failed to negotiate with the NBD server\n");
         return ret;
     }
+    if (x_dirty_bitmap && !client->info.base_allocation) {
+        error_setg(errp, "requested x-dirty-bitmap %s not found",
+                   x_dirty_bitmap);
+        return -EINVAL;
+    }
     if (client->info.flags & NBD_FLAG_READ_ONLY) {
         ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
         if (ret < 0) {
-- 
2.17.2

[Qemu-devel] [PATCH 3/3] nbd/client: Send NBD_CMD_DISC if open fails after connect

[Eric Blake]

If nbd_client_init() fails after we are already connected,
then the server will spam logs with:

Disconnect client, due to: Unexpected end-of-file before all bytes were read

unless we gracefully disconnect before closing the connection.

Ways to trigger this:

$ opts=driver=nbd,export=foo,server.type=inet,server.host=localhost,server.port=10809
$  qemu-img map --output=json --image-opts $opts,read-only=off
$  qemu-img map --output=json --image-opts $opts,x-dirty-bitmap=nosuch:

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index e6e27dafa6a..fc5b7eda8ee 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -995,12 +995,13 @@ int nbd_client_init(BlockDriverState *bs,
     if (x_dirty_bitmap && !client->info.base_allocation) {
         error_setg(errp, "requested x-dirty-bitmap %s not found",
                    x_dirty_bitmap);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto fail;
     }
     if (client->info.flags & NBD_FLAG_READ_ONLY) {
         ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
         if (ret < 0) {
-            return ret;
+            goto fail;
         }
     }
     if (client->info.flags & NBD_FLAG_SEND_FUA) {
@@ -1029,4 +1030,17 @@ int nbd_client_init(BlockDriverState *bs,

     logout("Established connection with NBD server\n");
     return 0;
+
+ fail:
+    /*
+     * We have connected, but must fail for other reasons. The
+     * connection is still blocking; send NBD_CMD_DISC as a courtesy
+     * to the server.
+     */
+    {
+        NBDRequest request = { .type = NBD_CMD_DISC };
+
+        nbd_send_request(client->ioc ?: QIO_CHANNEL(sioc), &request);
+        return ret;
+    }
 }
-- 
2.17.2

Re: [Qemu-devel] [PATCH 1/3] nbd/server: Advertise all contexts in response to bare LIST

[Vladimir Sementsov-Ogievskiy]

30.11.2018 5:32, Eric Blake wrote:
> The NBD spec, and even our code comment, says that if the client
> asks for NBD_OPT_LIST_META_CONTEXT with 0 queries, then we should
> reply with (a possibly-compressed representation of) ALL contexts
> that we are willing to let them try.  But commit 3d068aff forgot
> to advertise qemu:dirty-bitmap:FOO.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   nbd/server.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index dc04513de70..7af0ddffb20 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -978,6 +978,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
>       if (client->opt == NBD_OPT_LIST_META_CONTEXT && !nb_queries) {
>           /* enable all known contexts */
>           meta->base_allocation = true;
> +        meta->bitmap = !!meta->exp->export_bitmap;
>       } else {
>           for (i = 0; i < nb_queries; ++i) {
>               ret = nbd_negotiate_meta_query(client, meta, errp);
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/3] nbd/client: Make x-dirty-bitmap more reliable

[Vladimir Sementsov-Ogievskiy]

30.11.2018 5:32, Eric Blake wrote:
> The implementation of x-dirty-bitmap in qemu 3.0 silently
> falls back to treating the server as not supporting
> NBD_CMD_BLOCK_STATUS if a requested meta_context name was not
> negotiated, which in turn means treating the _entire_ image as
> data. Since our hack relied on using 'qemu-img map' to view
> which portions of the image were dirty by seeing what the
> redirected bdrv_block_status() treats as holes, this means
> that our fallback treats the entire image as clean.  Better
> would have been to treat the entire image as dirty, or to fail
> to connect because the user's request for a specific context
> could not be honored. This patch goes with the latter.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/nbd-client.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 76e9ca3abeb..e6e27dafa6a 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -992,6 +992,11 @@ int nbd_client_init(BlockDriverState *bs,
>           logout("Failed to negotiate with the NBD server\n");
>           return ret;
>       }
> +    if (x_dirty_bitmap && !client->info.base_allocation) {
> +        error_setg(errp, "requested x-dirty-bitmap %s not found",
> +                   x_dirty_bitmap);
> +        return -EINVAL;
> +    }
>       if (client->info.flags & NBD_FLAG_READ_ONLY) {
>           ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
>           if (ret < 0) {
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/3] nbd/client: Send NBD_CMD_DISC if open fails after connect

[Vladimir Sementsov-Ogievskiy]

30.11.2018 5:32, Eric Blake wrote:
> If nbd_client_init() fails after we are already connected,
> then the server will spam logs with:
> 
> Disconnect client, due to: Unexpected end-of-file before all bytes were read
> 
> unless we gracefully disconnect before closing the connection.
> 
> Ways to trigger this:
> 
> $ opts=driver=nbd,export=foo,server.type=inet,server.host=localhost,server.port=10809
> $  qemu-img map --output=json --image-opts $opts,read-only=off
> $  qemu-img map --output=json --image-opts $opts,x-dirty-bitmap=nosuch:
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/nbd-client.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index e6e27dafa6a..fc5b7eda8ee 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -995,12 +995,13 @@ int nbd_client_init(BlockDriverState *bs,
>       if (x_dirty_bitmap && !client->info.base_allocation) {
>           error_setg(errp, "requested x-dirty-bitmap %s not found",
>                      x_dirty_bitmap);
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto fail;
>       }
>       if (client->info.flags & NBD_FLAG_READ_ONLY) {
>           ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
>           if (ret < 0) {
> -            return ret;
> +            goto fail;
>           }
>       }
>       if (client->info.flags & NBD_FLAG_SEND_FUA) {
> @@ -1029,4 +1030,17 @@ int nbd_client_init(BlockDriverState *bs,
> 
>       logout("Established connection with NBD server\n");
>       return 0;
> +
> + fail:
> +    /*
> +     * We have connected, but must fail for other reasons. The
> +     * connection is still blocking; send NBD_CMD_DISC as a courtesy
> +     * to the server.
> +     */
> +    {
> +        NBDRequest request = { .type = NBD_CMD_DISC };
> +
> +        nbd_send_request(client->ioc ?: QIO_CHANNEL(sioc), &request);
> +        return ret;
> +    }
>   }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-3.1? 0/3] NBD dirty bitmap cleanups

[Eric Blake]

On 11/29/18 8:32 PM, Eric Blake wrote:
> I'm working on a larger series for 4.0 that will add
> 'qemu-nbd --list' to show all possible information about a
> server's exports, to make it easier to diagnose if the right
> exports and bitmaps are present.  But in the process, I found
> a few bugs that may be worth fixing in 3.1.  By itself, this
> series does not warrant -rc4 (particularly since all of the
> problems exist in the 3.0 release, so this is not a regression
> fix), but if we have -rc4 for other reasons, these would make
> incremental backup support a bit more reliable.

And CVEs might be the reason for an -rc4. I'm queuing this on my NBD 
tree, ready to send a Pull Request by early Monday if we indeed end up 
with an -rc4.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-3.1? 0/3] NBD dirty bitmap cleanups

[Peter Maydell]

On Fri, 30 Nov 2018 at 15:36, Eric Blake <eblake@redhat.com> wrote:
>
> On 11/29/18 8:32 PM, Eric Blake wrote:
> > I'm working on a larger series for 4.0 that will add
> > 'qemu-nbd --list' to show all possible information about a
> > server's exports, to make it easier to diagnose if the right
> > exports and bitmaps are present.  But in the process, I found
> > a few bugs that may be worth fixing in 3.1.  By itself, this
> > series does not warrant -rc4 (particularly since all of the
> > problems exist in the 3.0 release, so this is not a regression
> > fix), but if we have -rc4 for other reasons, these would make
> > incremental backup support a bit more reliable.
>
> And CVEs might be the reason for an -rc4. I'm queuing this on my NBD
> tree, ready to send a Pull Request by early Monday if we indeed end up
> with an -rc4.

...which we will, so would you like to send that pullreq ?

thanks
-- PMM