Re: [PATCH v4 7/7] nbd: Allow export of multiple bitmaps for one device

[Vladimir Sementsov-Ogievskiy]

10.10.2020 00:55, Eric Blake wrote:
With this, 'qemu-nbd -B b0 -B b1 -f qcow2 img.qcow2' can let you sniff
out multiple bitmaps from one server.  qemu-img as client can still
only read one bitmap per client connection, but other NBD clients
(hello libnbd) can now read multiple bitmaps in a single pass.
Signed-off-by: Eric Blake <ebl...@redhat.com>


You didn't update nbd_export_create failure patch, I suggest:

@@ -1533,6 +1537,7 @@ static int nbd_export_create(BlockExport *blk_exp, 
BlockExportOptions *exp_args,
     bool shared = !exp_args->writable;
     strList *bitmaps;
     int ret;
+    size_t i;
 
     assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
 
@@ -1632,11 +1637,15 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
             goto fail;
         }
 
-        bdrv_dirty_bitmap_set_busy(bm, true);
         exp->export_bitmaps[exp->nr_export_bitmaps++] = bm;
         assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
     }
 
+    /* Mark bitmaps busy in a separate loop, to not bother with roll-back */
+    for (i = 0; i < exp->nr_export_bitmaps; i++) {
+        bdrv_dirty_bitmap_set_busy(bm, true);
+    }
+
     exp->allocation_depth = arg->allocation_depth;
 
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
@@ -1646,6 +1655,7 @@ static int nbd_export_create(BlockExport *blk_exp, 
BlockExportOptions *exp_args,
     return 0;
 
 fail:
+    g_free(exp->export_bitmaps);
     g_free(exp->name);
     g_free(exp->description);
     return ret;

and with it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>


Also, would be good to add a comment:

@@ -29,6 +29,10 @@
 #define NBD_META_ID_BASE_ALLOCATION 0
 #define NBD_META_ID_ALLOCATION_DEPTH 1
 #define NBD_META_ID_DIRTY_BITMAP 2
+/*
+ * NBD_META_ID_DIRTY_BITMAP+i are reserved for dirty bitmaps, so keep
+ * NBD_META_ID_DIRTY_BITMAP the last one.
+ */
 
 /*
  * NBD_MAX_BLOCK_STATUS_EXTENTS: 1 MiB of extents data. An empirical



--
Best regards,
Vladimir