[Qemu-devel] [PATCH 0/5] Promote x-nbd-server-add-bitmap to stable

[Qemu-devel] [PATCH 0/5] Promote x-nbd-server-add-bitmap to stable

[Eric Blake]

Or rather, move its functionality into nbd-server-add.  And as
a side effect, teach qemu-nbd how to export a persistent bitmap
without having to go through a qemu process and several QMP
commands.

Based-on: <20181221093529.23855-1-jsnow@redhat.com>
[0/11 bitmaps: remove x- prefix from QMP api]

Patch 5 was very lightly tested; I hope to have more time to play
with that patch alongside Andrey's pending [qemu-img info lists
bitmap directory entries], with an update to iotests to
demonstrate how it is possible to inspect dirty bitmaps by using
qemu-img to learn what is available then qemu-nbd to expose them.
It's still painful that there are no qemu-img commands for
creating a new persistent bitmap; that still has to be done via
QMP on a live guest.

Eric Blake (5):
  nbd: Merge nbd_export_set_name into nbd_export_new
  nbd: Allow bitmap export during QMP nbd-server-add
  nbd: Remove x-nbd-server-add-bitmap
  nbd: Merge nbd_export_bitmap into nbd_export_new
  qemu-nbd: Add --bitmap=NAME option

 qemu-nbd.texi              |   5 ++
 qapi/block.json            |  35 ++++-------
 include/block/nbd.h        |   7 +--
 blockdev-nbd.c             |  44 ++++++--------
 hmp.c                      |   6 +-
 nbd/server.c               | 121 ++++++++++++++++---------------------
 qemu-nbd.c                 |  19 ++++--
 tests/qemu-iotests/223     |  11 ++--
 tests/qemu-iotests/223.out |   2 -
 9 files changed, 109 insertions(+), 141 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/5] nbd: Merge nbd_export_set_name into nbd_export_new

[Eric Blake]

The existing NBD code had a weird split where nbd_export_new()
created an export but did not add it to the list of exported
names until a later nbd_export_set_name() came along and grabbed
a second reference on the object; later, nbd_export_close()
drops the second reference.  But since we never change the
name of an NBD export while it is exposed, it is easier to just
inline the process of setting the name as part of creating the
export.

Inline the contents of nbd_export_set_name() and
nbd_export_set_description() into the two points in an export
lifecycle where they matter, then adjust both callers to pass
the name up front.  Note that all callers pass a non-NULL name,
(passing NULL at creation was for old style servers, but we
removed support for that in commit 7f7dfe2a).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  3 +--
 blockdev-nbd.c      |  5 ++---
 nbd/server.c        | 45 ++++++++++++++++-----------------------------
 qemu-nbd.c          |  5 ++---
 4 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 65402d33964..2f9a2aeb73c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -295,6 +295,7 @@ typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
+                          const char *name, const char *description,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp);
@@ -306,8 +307,6 @@ void nbd_export_put(NBDExport *exp);
 BlockBackend *nbd_export_get_blockdev(NBDExport *exp);

 NBDExport *nbd_export_find(const char *name);
-void nbd_export_set_name(NBDExport *exp, const char *name);
-void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);

 void nbd_client_new(QIOChannelSocket *sioc,
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1d170c80b82..f5edbc27d88 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -174,14 +174,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         writable = false;
     }

-    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
+    exp = nbd_export_new(bs, 0, -1, name, NULL,
+                         writable ? 0 : NBD_FLAG_READ_ONLY,
                          NULL, false, on_eject_blk, errp);
     if (!exp) {
         return;
     }

-    nbd_export_set_name(exp, name);
-
     /* The list of named exports has a strong reference to this export now and
      * our only way of accessing it is through nbd_export_find(), so we can drop
      * the strong reference that is @exp. */
diff --git a/nbd/server.c b/nbd/server.c
index 7af0ddffb20..9cb305aa1bf 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1456,6 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
 }

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
+                          const char *name, const char *description,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp)
@@ -1471,6 +1472,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
      * that BDRV_O_INACTIVE is cleared and the image is ready for write
      * access since the export could be available before migration handover.
      */
+    assert(name);
     ctx = bdrv_get_aio_context(bs);
     aio_context_acquire(ctx);
     bdrv_invalidate_cache(bs, NULL);
@@ -1494,6 +1496,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     QTAILQ_INIT(&exp->clients);
     exp->blk = blk;
     exp->dev_offset = dev_offset;
+    exp->name = g_strdup(name);
+    exp->description = g_strdup(description);
     exp->nbdflags = nbdflags;
     exp->size = size < 0 ? blk_getlength(blk) : size;
     if (exp->size < 0) {
@@ -1513,10 +1517,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
         exp->eject_notifier.notify = nbd_eject_notifier;
         blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
     }
+    QTAILQ_INSERT_TAIL(&exports, exp, next);
+    nbd_export_get(exp);
     return exp;

 fail:
     blk_unref(blk);
+    g_free(exp->name);
+    g_free(exp->description);
     g_free(exp);
     return NULL;
 }
@@ -1533,33 +1541,6 @@ NBDExport *nbd_export_find(const char *name)
     return NULL;
 }

-void nbd_export_set_name(NBDExport *exp, const char *name)
-{
-    if (exp->name == name) {
-        return;
-    }
-
-    nbd_export_get(exp);
-    if (exp->name != NULL) {
-        g_free(exp->name);
-        exp->name = NULL;
-        QTAILQ_REMOVE(&exports, exp, next);
-        nbd_export_put(exp);
-    }
-    if (name != NULL) {
-        nbd_export_get(exp);
-        exp->name = g_strdup(name);
-        QTAILQ_INSERT_TAIL(&exports, exp, next);
-    }
-    nbd_export_put(exp);
-}
-
-void nbd_export_set_description(NBDExport *exp, const char *description)
-{
-    g_free(exp->description);
-    exp->description = g_strdup(description);
-}
-
 void nbd_export_close(NBDExport *exp)
 {
     NBDClient *client, *next;
@@ -1568,8 +1549,14 @@ void nbd_export_close(NBDExport *exp)
     QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
         client_close(client, true);
     }
-    nbd_export_set_name(exp, NULL);
-    nbd_export_set_description(exp, NULL);
+    if (exp->name) {
+        nbd_export_put(exp);
+        g_free(exp->name);
+        exp->name = NULL;
+        QTAILQ_REMOVE(&exports, exp, next);
+    }
+    g_free(exp->description);
+    exp->description = NULL;
     nbd_export_put(exp);
 }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2807e132396..696bd78a2e2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1015,10 +1015,9 @@ int main(int argc, char **argv)
         }
     }

-    exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed,
+    exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
+                         export_description, nbdflags, nbd_export_closed,
                          writethrough, NULL, &error_fatal);
-    nbd_export_set_name(exp, export_name);
-    nbd_export_set_description(exp, export_description);

     if (device) {
 #if HAVE_NBD_DEVICE
-- 
2.20.1

[Qemu-devel] [PATCH 2/5] nbd: Allow bitmap export during QMP nbd-server-add

[Eric Blake]

With the experimental x-nbd-server-add-bitmap command, there was
a window of time where a client could see the export but not the
associated dirty bitmap, which can cause a client that planned
on using the dirty bitmap to be forced to treat the entire image
as dirty as a safety fallback.  Furthermore, if the QMP client
successfully exports a disk but then fails to add the bitmap, it
has to take on the burden of removing the export.  Since we don't
allow changing a dirty bitmap once it is exported (whether to a
different bitmap, or removing advertisement of the bitmap), it is
nicer to make the bitmap tied to the export at the time the export
is created, and automatically failing to export if the bitmap is
not available.

Since there is working libvirt demo code that uses both the bitmap
export and the ability to specify an alternative name (rather than
exposing the private-use bitmap that libvirt created to merge in
several persistent bitmaps when doing a differential backup), the
two new parameters do not need to be marked experimental.  See
https://www.redhat.com/archives/libvir-list/2018-October/msg01254.html,
https://kvmforum2018.sched.com/event/FzuB/facilitating-incremental-backup-eric-blake-red-hat

This patch focuses on the user interface, and reduces (but does
not completely eliminate) the window where an NBD client can see
the export but not the dirty bitmap.  Later patches will add
further cleanups now that this interface is available.

Update test 223 to use the new interface.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block.json            | 12 +++++++++++-
 blockdev-nbd.c             | 27 ++++++++++++++++++++++++++-
 hmp.c                      |  6 ++++--
 tests/qemu-iotests/223     | 11 ++++-------
 tests/qemu-iotests/223.out |  2 --
 5 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 11f01f28efe..4b336303d21 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -246,6 +246,15 @@
 #
 # @writable: Whether clients should be able to write to the device via the
 #     NBD connection (default false).
+
+# @bitmap: Dirty bitmap to associate with the selected export. The export
+#          must be read-only, and the given bitmap is locked until the
+#          export is removed. (since 4.0)
+#
+# @bitmap-export-name: How the bitmap will be seen by nbd clients
+#                      (default @bitmap). The NBD client must use
+#                      NBD_OPT_SET_META_CONTEXT with "qemu:dirty-bitmap:NAME"
+#                      to access the exposed bitmap. (since 4.0)
 #
 # Returns: error if the server is not running, or export with the same name
 #          already exists.
@@ -253,7 +262,8 @@
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-add',
-  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
+  'data': {'device': 'str', '*name': 'str', '*writable': 'bool',
+           '*bitmap': 'str', '*bitmap-export-name': 'str' } }

 ##
 # @NbdServerRemoveMode:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index f5edbc27d88..cae9e802d48 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -140,7 +140,10 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
 }

 void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
-                        bool has_writable, bool writable, Error **errp)
+                        bool has_writable, bool writable,
+                        bool has_bitmap, const char *bitmap,
+                        bool has_bitmap_export_name,
+                        const char *bitmap_export_name, Error **errp)
 {
     BlockDriverState *bs = NULL;
     BlockBackend *on_eject_blk;
@@ -160,6 +163,17 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         return;
     }

+    if (has_bitmap_export_name && !has_bitmap) {
+        error_setg(errp, "Choosing bitmap export name '%s' requires a bitmap",
+                   bitmap_export_name);
+        return;
+    }
+    if (has_bitmap && writable) {
+        error_setg(errp, "Cannot export bitmap '%s' on writable export",
+                   bitmap);
+        return;
+    }
+
     on_eject_blk = blk_by_name(device);

     bs = bdrv_lookup_bs(device, device, errp);
@@ -185,6 +199,17 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
      * our only way of accessing it is through nbd_export_find(), so we can drop
      * the strong reference that is @exp. */
     nbd_export_put(exp);
+
+    if (has_bitmap) {
+        Error *err = NULL;
+        nbd_export_bitmap(exp, bitmap,
+                          has_bitmap_export_name ? bitmap_export_name : bitmap,
+                          &err);
+        if (err) {
+            error_propagate(errp, err);
+            nbd_export_remove(exp, NBD_SERVER_REMOVE_MODE_HARD, NULL);
+        }
+    }
 }

 void qmp_nbd_server_remove(const char *name,
diff --git a/hmp.c b/hmp.c
index 80aa5ab504b..484a0000de1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2326,7 +2326,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         }

         qmp_nbd_server_add(info->value->device, false, NULL,
-                           true, writable, &local_err);
+                           true, writable, false, NULL, false, NULL,
+                           &local_err);

         if (local_err != NULL) {
             qmp_nbd_server_stop(NULL);
@@ -2347,7 +2348,8 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
     bool writable = qdict_get_try_bool(qdict, "writable", false);
     Error *local_err = NULL;

-    qmp_nbd_server_add(device, !!name, name, true, writable, &local_err);
+    qmp_nbd_server_add(device, !!name, name, true, writable,
+                       false, NULL, false, NULL, &local_err);
     hmp_handle_error(mon, &local_err);
 }

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 5513dc62159..7a92c3018e2 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -120,13 +120,10 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
     "data":{"path":"'"$TEST_DIR/nbd"'"}}}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
-  "arguments":{"device":"n"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
-  "arguments":{"name":"n", "bitmap":"b"}}' "return"
+  "arguments":{"device":"n", "bitmap":"b"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
-  "arguments":{"device":"n", "name":"n2"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
-  "arguments":{"name":"n2", "bitmap":"b2"}}' "return"
+  "arguments":{"device":"n", "name":"n2", "bitmap":"b2",
+    "bitmap-export-name":"b3"}}' "return"

 echo
 echo "=== Contrast normal status to large granularity dirty-bitmap ==="
@@ -147,7 +144,7 @@ echo

 IMG="driver=nbd,export=n2,server.type=unix,server.path=$TEST_DIR/nbd"
 $QEMU_IMG map --output=json --image-opts \
-  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b3" | _filter_qemu_img_map

 echo
 echo "=== End NBD server ==="
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 99ca172fbb8..0e467981bb8 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -31,8 +31,6 @@ wrote 2097152/2097152 bytes at offset 2097152
 {"return": {}}
 {"return": {}}
 {"return": {}}
-{"return": {}}
-{"return": {}}

 === Contrast normal status to large granularity dirty-bitmap ===

-- 
2.20.1

[Qemu-devel] [PATCH 3/5] nbd: Remove x-nbd-server-add-bitmap

[Eric Blake]

Now that nbd-server-add can do the same functionality, we no
longer need the experimental separate command.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block.json | 23 -----------------------
 blockdev-nbd.c  | 23 -----------------------
 2 files changed, 46 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 4b336303d21..980682e69ea 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -306,29 +306,6 @@
 { 'command': 'nbd-server-remove',
   'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }

-##
-# @x-nbd-server-add-bitmap:
-#
-# Expose a dirty bitmap associated with the selected export. The bitmap search
-# starts at the device attached to the export, and includes all backing files.
-# The exported bitmap is then locked until the NBD export is removed.
-#
-# @name: Export name.
-#
-# @bitmap: Bitmap name to search for.
-#
-# @bitmap-export-name: How the bitmap will be seen by nbd clients
-#                      (default @bitmap)
-#
-# Note: the client must use NBD_OPT_SET_META_CONTEXT with a query of
-# "qemu:dirty-bitmap:NAME" (where NAME matches @bitmap-export-name) to access
-# the exposed bitmap.
-#
-# Since: 3.0
-##
-  { 'command': 'x-nbd-server-add-bitmap',
-    'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} }
-
 ##
 # @nbd-server-stop:
 #
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index cae9e802d48..faecde3b6e1 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -243,26 +243,3 @@ void qmp_nbd_server_stop(Error **errp)
     nbd_server_free(nbd_server);
     nbd_server = NULL;
 }
-
-void qmp_x_nbd_server_add_bitmap(const char *name, const char *bitmap,
-                                 bool has_bitmap_export_name,
-                                 const char *bitmap_export_name,
-                                 Error **errp)
-{
-    NBDExport *exp;
-
-    if (!nbd_server) {
-        error_setg(errp, "NBD server not running");
-        return;
-    }
-
-    exp = nbd_export_find(name);
-    if (exp == NULL) {
-        error_setg(errp, "Export '%s' is not found", name);
-        return;
-    }
-
-    nbd_export_bitmap(exp, bitmap,
-                      has_bitmap_export_name ? bitmap_export_name : bitmap,
-                      errp);
-}
-- 
2.20.1

[Qemu-devel] [PATCH 4/5] nbd: Merge nbd_export_bitmap into nbd_export_new

[Eric Blake]

We only have one caller that wants to export a bitmap name,
which it does right after creation of the export. But there is
still a brief window of time where an NBD client could see the
export but not the dirty bitmap, which a robust client would
have to interpret as meaning the entire image should be treated
as dirty.  Better is to eliminate the window entirely, by
inlining nbd_export_bitmap() into nbd_export_new(), and refusing
to create the bitmap in the first place if the requested bitmap
can't be located.

This also moves the logic for determining the default bitmap
export name out of blockdev-nbd into nbd_export_new().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  4 +--
 blockdev-nbd.c      | 13 +-------
 nbd/server.c        | 76 +++++++++++++++++++++------------------------
 qemu-nbd.c          |  2 +-
 4 files changed, 38 insertions(+), 57 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 2f9a2aeb73c..e598305ecda 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -296,6 +296,7 @@ typedef struct NBDClient NBDClient;

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           const char *name, const char *description,
+                          const char *bitmap, const char *bitmap_name,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp);
@@ -319,9 +320,6 @@ void nbd_client_put(NBDClient *client);
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
                       Error **errp);

-void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
-                       const char *bitmap_export_name, Error **errp);
-
 /* nbd_read
  * Reads @size bytes from @ioc. Returns 0 on success.
  */
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index faecde3b6e1..d895ee4adf5 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -188,7 +188,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         writable = false;
     }

-    exp = nbd_export_new(bs, 0, -1, name, NULL,
+    exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap, bitmap_export_name,
                          writable ? 0 : NBD_FLAG_READ_ONLY,
                          NULL, false, on_eject_blk, errp);
     if (!exp) {
@@ -199,17 +199,6 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
      * our only way of accessing it is through nbd_export_find(), so we can drop
      * the strong reference that is @exp. */
     nbd_export_put(exp);
-
-    if (has_bitmap) {
-        Error *err = NULL;
-        nbd_export_bitmap(exp, bitmap,
-                          has_bitmap_export_name ? bitmap_export_name : bitmap,
-                          &err);
-        if (err) {
-            error_propagate(errp, err);
-            nbd_export_remove(exp, NBD_SERVER_REMOVE_MODE_HARD, NULL);
-        }
-    }
 }

 void qmp_nbd_server_remove(const char *name,
diff --git a/nbd/server.c b/nbd/server.c
index 9cb305aa1bf..556da68ac26 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1457,6 +1457,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           const char *name, const char *description,
+                          const char *bitmap, const char *bitmap_name,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp)
@@ -1507,6 +1508,40 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     }
     exp->size -= exp->size % BDRV_SECTOR_SIZE;

+    if (bitmap) {
+        BdrvDirtyBitmap *bm = NULL;
+        BlockDriverState *bs = blk_bs(blk);
+
+        while (true) {
+            bm = bdrv_find_dirty_bitmap(bs, bitmap);
+            if (bm != NULL || bs->backing == NULL) {
+                break;
+            }
+
+            bs = bs->backing->bs;
+        }
+
+        if (bm == NULL) {
+            error_setg(errp, "Bitmap '%s' is not found", bitmap);
+            goto fail;
+        }
+
+        if (bdrv_dirty_bitmap_enabled(bm)) {
+            error_setg(errp, "Bitmap '%s' is enabled", bitmap);
+            goto fail;
+        }
+
+        if (bdrv_dirty_bitmap_user_locked(bm)) {
+            error_setg(errp, "Bitmap '%s' is in use", bitmap);
+            goto fail;
+        }
+
+        bdrv_dirty_bitmap_set_qmp_locked(bm, true);
+        exp->export_bitmap = bm;
+        exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
+                                                     bitmap_name ?: bitmap);
+    }
+
     exp->close = close;
     exp->ctx = blk_get_aio_context(blk);
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
@@ -2417,44 +2452,3 @@ void nbd_client_new(QIOChannelSocket *sioc,
     co = qemu_coroutine_create(nbd_co_client_start, client);
     qemu_coroutine_enter(co);
 }
-
-void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
-                       const char *bitmap_export_name, Error **errp)
-{
-    BdrvDirtyBitmap *bm = NULL;
-    BlockDriverState *bs = blk_bs(exp->blk);
-
-    if (exp->export_bitmap) {
-        error_setg(errp, "Export bitmap is already set");
-        return;
-    }
-
-    while (true) {
-        bm = bdrv_find_dirty_bitmap(bs, bitmap);
-        if (bm != NULL || bs->backing == NULL) {
-            break;
-        }
-
-        bs = bs->backing->bs;
-    }
-
-    if (bm == NULL) {
-        error_setg(errp, "Bitmap '%s' is not found", bitmap);
-        return;
-    }
-
-    if (bdrv_dirty_bitmap_enabled(bm)) {
-        error_setg(errp, "Bitmap '%s' is enabled", bitmap);
-        return;
-    }
-
-    if (bdrv_dirty_bitmap_user_locked(bm)) {
-        error_setg(errp, "Bitmap '%s' is in use", bitmap);
-        return;
-    }
-
-    bdrv_dirty_bitmap_set_qmp_locked(bm, true);
-    exp->export_bitmap = bm;
-    exp->export_bitmap_context =
-            g_strdup_printf("qemu:dirty-bitmap:%s", bitmap_export_name);
-}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 696bd78a2e2..a6cc0f2553e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1015,7 +1015,7 @@ int main(int argc, char **argv)
         }
     }

-    exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
+    exp = nbd_export_new(bs, dev_offset, fd_size, export_name, NULL, NULL,
                          export_description, nbdflags, nbd_export_closed,
                          writethrough, NULL, &error_fatal);

-- 
2.20.1

[Qemu-devel] [PATCH 5/5] qemu-nbd: Add --bitmap=NAME option

[Eric Blake]

Having to fire up qemu, then use QMP commands for nbd-server-start
and nbd-server-add, just to expose a persistent dirty bitmap, is
rather tedious.  Make it possible to expose a dirty bitmap using
just qemu-nbd (of course, for now this only works when qemu-nbd is
visiting a BDS formatted as qcow2).

For now, I play it safe and only allow a bitmap to be exposed on
a read-only image.  We may relax it in the future.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-nbd.texi |  5 +++++
 qemu-nbd.c    | 16 ++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9a84e81eed9..1bf9d2bd8bf 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -45,6 +45,11 @@ auto-detecting
 Export the disk as read-only
 @item -P, --partition=@var{num}
 Only expose partition @var{num}
+@item -B, --bitmap=@var{name}
+If @var{filename} has a qcow2 persistent bitmap @var{name}, expose
+that bitmap via the ``qemu:dirty-bitmap:@var{name}'' context
+accessible through NBD_OPT_SET_META_CONTEXT.  Requires the use of
+@option{-r}.
 @item -s, --snapshot
 Use @var{filename} as an external snapshot, create a temporary
 file with backing_file=@var{filename}, redirect the write to
diff --git a/qemu-nbd.c b/qemu-nbd.c
index a6cc0f2553e..9781efe7a25 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -95,6 +95,7 @@ static void usage(const char *name)
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
 "  -P, --partition=NUM       only expose partition NUM\n"
+"  -B, --bitmap=NAME         expose a persistent dirty bitmap\n"
 "\n"
 "General purpose options:\n"
 "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
@@ -509,7 +510,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:";
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -519,6 +520,7 @@ int main(int argc, char **argv)
         { "offset", required_argument, NULL, 'o' },
         { "read-only", no_argument, NULL, 'r' },
         { "partition", required_argument, NULL, 'P' },
+        { "bitmap", required_argument, NULL, 'B' },
         { "connect", required_argument, NULL, 'c' },
         { "disconnect", no_argument, NULL, 'd' },
         { "snapshot", no_argument, NULL, 's' },
@@ -558,6 +560,7 @@ int main(int argc, char **argv)
     QDict *options = NULL;
     const char *export_name = ""; /* Default export name */
     const char *export_description = NULL;
+    const char *bitmap = NULL;
     const char *tlscredsid = NULL;
     bool imageOpts = false;
     bool writethrough = true;
@@ -695,6 +698,9 @@ int main(int argc, char **argv)
                 exit(EXIT_FAILURE);
             }
             break;
+        case 'B':
+            bitmap = optarg;
+            break;
         case 'k':
             sockpath = optarg;
             if (sockpath[0] != '/') {
@@ -822,6 +828,12 @@ int main(int argc, char **argv)
         }
     }

+    if (bitmap && !(nbdflags & NBD_FLAG_READ_ONLY)) {
+        error_report("Exporting bitmap '%s' requires a readonly export",
+                     bitmap);
+        exit(EXIT_FAILURE);
+    }
+
 #if !HAVE_NBD_DEVICE
     if (disconnect || device) {
         error_report("Kernel /dev/nbdN support not available");
@@ -1015,7 +1027,7 @@ int main(int argc, char **argv)
         }
     }

-    exp = nbd_export_new(bs, dev_offset, fd_size, export_name, NULL, NULL,
+    exp = nbd_export_new(bs, dev_offset, fd_size, export_name, bitmap, NULL,
                          export_description, nbdflags, nbd_export_closed,
                          writethrough, NULL, &error_fatal);

-- 
2.20.1

Re: [Qemu-devel] [PATCH 5/5] qemu-nbd: Add --bitmap=NAME option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On 1/8/19 10:14 PM, Eric Blake wrote:
> Having to fire up qemu, then use QMP commands for nbd-server-start
> and nbd-server-add, just to expose a persistent dirty bitmap, is
> rather tedious.  Make it possible to expose a dirty bitmap using
> just qemu-nbd (of course, for now this only works when qemu-nbd is
> visiting a BDS formatted as qcow2).
> 
> For now, I play it safe and only allow a bitmap to be exposed on
> a read-only image.  We may relax it in the future.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-nbd.texi |  5 +++++
>  qemu-nbd.c    | 16 ++++++++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 

> @@ -1015,7 +1027,7 @@ int main(int argc, char **argv)
>          }
>      }
> 
> -    exp = nbd_export_new(bs, dev_offset, fd_size, export_name, NULL, NULL,
> +    exp = nbd_export_new(bs, dev_offset, fd_size, export_name, bitmap, NULL,
>                           export_description, nbdflags, nbd_export_closed,
>                           writethrough, NULL, &error_fatal);

The cover letter DID warn that this was only lightly tested.  For it to
even work, you have to s/bitmap, NULL/NULL, bitmap/ or else swap the
meaning of the --bitmap and --description command line options.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/5] nbd: Allow bitmap export during QMP nbd-server-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3112 bytes --]

On 1/8/19 10:14 PM, Eric Blake wrote:
> With the experimental x-nbd-server-add-bitmap command, there was
> a window of time where a client could see the export but not the
> associated dirty bitmap, which can cause a client that planned
> on using the dirty bitmap to be forced to treat the entire image
> as dirty as a safety fallback.  Furthermore, if the QMP client
> successfully exports a disk but then fails to add the bitmap, it
> has to take on the burden of removing the export.  Since we don't
> allow changing a dirty bitmap once it is exported (whether to a
> different bitmap, or removing advertisement of the bitmap), it is
> nicer to make the bitmap tied to the export at the time the export
> is created, and automatically failing to export if the bitmap is
> not available.
> 
> Since there is working libvirt demo code that uses both the bitmap
> export and the ability to specify an alternative name (rather than
> exposing the private-use bitmap that libvirt created to merge in
> several persistent bitmaps when doing a differential backup), the
> two new parameters do not need to be marked experimental.  See
> https://www.redhat.com/archives/libvir-list/2018-October/msg01254.html,
> https://kvmforum2018.sched.com/event/FzuB/facilitating-incremental-backup-eric-blake-red-hat

...

> +#
> +# @bitmap-export-name: How the bitmap will be seen by nbd clients
> +#                      (default @bitmap). The NBD client must use
> +#                      NBD_OPT_SET_META_CONTEXT with "qemu:dirty-bitmap:NAME"
> +#                      to access the exposed bitmap. (since 4.0)
>  #

Actually, on re-reading my libvirt code, it turns out that I had
differential backups working without the use of bitmap-export-name,
because I always created a temporary bitmap (with the name I planned to
expose, namely "vda" if the bitmap belonged to the device that libvirt
calls "vda") and merged in the persistent bitmaps into the temporary
bitmap.  Of course, right now we only support one backup NBD job at a
time (because we don't support parallel NBD servers), but thinking ahead
to when we may support that, it becomes obvious that libvirt can't pick
the same name for two separate temporary bitmaps being used by two
parallel backup jobs.  On the other hand, Nir requested that libvirt
backup job XMLs support some sort of name or UUID, and it is just as
easy to ensure that the temporary bitmap is given the name of the backup
job (which will be unique) rather than the name of the device being
backed up.

And my proposed patches for 'qemu-nbd --list' make it easy to
definitively query what names are actually being exported.

So at this point, I'm leaning towards NOT exposing a way to remap the
exported bitmap name.  We can always add it later if it proves useful,
but I'd rather not be burdened with being stuck with it since libvirt
hasn't used it so far.  I'll spin v2 of this series along those lines.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]