[PATCH v2 for-7.1 0/3] qapi: nbd-export: select bitmap by node/name pair

[PATCH v2 for-7.1 0/3] qapi: nbd-export: select bitmap by node/name pair

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>

Hi all!

Here is small improvement for bitmap exporting interface.

v2: Sorry for the noise, me trying to find a email service, that don't
consider sending patch series by git-send-email as as spam :/

Vladimir Sementsov-Ogievskiy (3):
  qapi: rename BlockDirtyBitmapMergeSource to BlockDirtyBitmapOrStr
  qapi: nbd-export: allow select bitmaps by node/name pair
  iotests/223: check new possibility of exporting bitmaps by node/name

 block/monitor/bitmap-qmp-cmds.c        |  6 +--
 blockdev-nbd.c                         |  8 +++-
 include/block/block_int-global-state.h |  2 +-
 nbd/server.c                           | 63 +++++++++++++++++---------
 qapi/block-core.json                   |  6 +--
 qapi/block-export.json                 |  5 +-
 qemu-img.c                             |  8 ++--
 qemu-nbd.c                             | 11 ++++-
 tests/qemu-iotests/223                 | 16 +++++++
 tests/qemu-iotests/223.out             | 47 ++++++++++++++++++-
 10 files changed, 133 insertions(+), 39 deletions(-)

-- 
2.35.1

[PATCH v2 1/3] qapi: rename BlockDirtyBitmapMergeSource to BlockDirtyBitmapOrStr

[Vladimir Sementsov-Ogievskiy]

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

Rename the type to be reused. Old name is "what is it for". To be
natively reused for other needs, let's name it exactly "what is it".

Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
---
 block/monitor/bitmap-qmp-cmds.c        | 6 +++---
 include/block/block_int-global-state.h | 2 +-
 qapi/block-core.json                   | 6 +++---
 qemu-img.c                             | 8 ++++----
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 8e35616c2e..2b677c4a2f 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -257,12 +257,12 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
 }
 
 BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
-                                          BlockDirtyBitmapMergeSourceList *bms,
+                                          BlockDirtyBitmapOrStrList *bms,
                                           HBitmap **backup, Error **errp)
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *dst, *src, *anon;
-    BlockDirtyBitmapMergeSourceList *lst;
+    BlockDirtyBitmapOrStrList *lst;
 
     GLOBAL_STATE_CODE();
 
@@ -317,7 +317,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
 }
 
 void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
-                                  BlockDirtyBitmapMergeSourceList *bitmaps,
+                                  BlockDirtyBitmapOrStrList *bitmaps,
                                   Error **errp)
 {
     block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index 0f21b0570b..8b2e95f5ff 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -262,7 +262,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
                                            BlockDriverState **pbs,
                                            Error **errp);
 BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
-                                          BlockDirtyBitmapMergeSourceList *bms,
+                                          BlockDirtyBitmapOrStrList *bms,
                                           HBitmap **backup, Error **errp);
 BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
                                            bool release,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e89f2dfb5b..c0eacd66db 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2078,7 +2078,7 @@
             '*persistent': 'bool', '*disabled': 'bool' } }
 
 ##
-# @BlockDirtyBitmapMergeSource:
+# @BlockDirtyBitmapOrStr:
 #
 # @local: name of the bitmap, attached to the same node as target bitmap.
 #
@@ -2086,7 +2086,7 @@
 #
 # Since: 4.1
 ##
-{ 'alternate': 'BlockDirtyBitmapMergeSource',
+{ 'alternate': 'BlockDirtyBitmapOrStr',
   'data': { 'local': 'str',
             'external': 'BlockDirtyBitmap' } }
 
@@ -2105,7 +2105,7 @@
 ##
 { 'struct': 'BlockDirtyBitmapMerge',
   'data': { 'node': 'str', 'target': 'str',
-            'bitmaps': ['BlockDirtyBitmapMergeSource'] } }
+            'bitmaps': ['BlockDirtyBitmapOrStr'] } }
 
 ##
 # @block-dirty-bitmap-add:
diff --git a/qemu-img.c b/qemu-img.c
index 5dffb3e616..f853efe6fb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1618,16 +1618,16 @@ static void do_dirty_bitmap_merge(const char *dst_node, const char *dst_name,
                                   const char *src_node, const char *src_name,
                                   Error **errp)
 {
-    BlockDirtyBitmapMergeSource *merge_src;
-    BlockDirtyBitmapMergeSourceList *list = NULL;
+    BlockDirtyBitmapOrStr *merge_src;
+    BlockDirtyBitmapOrStrList *list = NULL;
 
-    merge_src = g_new0(BlockDirtyBitmapMergeSource, 1);
+    merge_src = g_new0(BlockDirtyBitmapOrStr, 1);
     merge_src->type = QTYPE_QDICT;
     merge_src->u.external.node = g_strdup(src_node);
     merge_src->u.external.name = g_strdup(src_name);
     QAPI_LIST_PREPEND(list, merge_src);
     qmp_block_dirty_bitmap_merge(dst_node, dst_name, list, errp);
-    qapi_free_BlockDirtyBitmapMergeSourceList(list);
+    qapi_free_BlockDirtyBitmapOrStrList(list);
 }
 
 enum ImgConvertBlockStatus {
-- 
2.35.1

Re: [PATCH v2 1/3] qapi: rename BlockDirtyBitmapMergeSource to BlockDirtyBitmapOrStr

[Eric Blake]

On Tue, Mar 15, 2022 at 12:32:24AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Rename the type to be reused. Old name is "what is it for". To be
> natively reused for other needs, let's name it exactly "what is it".
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
> ---
>  block/monitor/bitmap-qmp-cmds.c        | 6 +++---
>  include/block/block_int-global-state.h | 2 +-
>  qapi/block-core.json                   | 6 +++---
>  qemu-img.c                             | 8 ++++----
>  4 files changed, 11 insertions(+), 11 deletions(-)

The type name does not affect QAPI interface stability, so this is safe.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v2 1/3] qapi: rename BlockDirtyBitmapMergeSource to BlockDirtyBitmapOrStr

[John Snow]

On Wed, Mar 16, 2022 at 5:18 PM Eric Blake <eblake@redhat.com> wrote:
>
> On Tue, Mar 15, 2022 at 12:32:24AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >
> > Rename the type to be reused. Old name is "what is it for". To be
> > natively reused for other needs, let's name it exactly "what is it".
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
> > ---
> >  block/monitor/bitmap-qmp-cmds.c        | 6 +++---
> >  include/block/block_int-global-state.h | 2 +-
> >  qapi/block-core.json                   | 6 +++---
> >  qemu-img.c                             | 8 ++++----
> >  4 files changed, 11 insertions(+), 11 deletions(-)
>
> The type name does not affect QAPI interface stability, so this is safe.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Sounds right.

Acked-by: John Snow <jsnow@redhat.com>

[PATCH v2 2/3] qapi: nbd-export: allow select bitmaps by node/name pair

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>

Hi all! Current logic of relying on search through backing chain is not
safe neither convenient.

Sometimes it leads to necessity of extra bitmap copying. Also, we are
going to add "snapshot-access" driver, to access some snapshot state
through NBD. And this driver is not formally a filter, and of course
it's not a COW format driver. So, searching through backing chain will
not work. Instead of widening the workaround of bitmap searching, let's
extend the interface so that user can select bitmap precisely.

Note, that checking for bitmap active status is not copied to the new
API, I don't see a reason for it, user should understand the risks. And
anyway, bitmap from other node is unrelated to this export being
read-only or read-write.

Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
---
 blockdev-nbd.c         |  8 +++++-
 nbd/server.c           | 63 +++++++++++++++++++++++++++---------------
 qapi/block-export.json |  5 +++-
 qemu-nbd.c             | 11 ++++++--
 4 files changed, 61 insertions(+), 26 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 9840d25a82..7f6531cba0 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -211,8 +211,14 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
     QAPI_CLONE_MEMBERS(BlockExportOptionsNbdBase, &export_opts->u.nbd,
                        qapi_NbdServerAddOptions_base(arg));
     if (arg->has_bitmap) {
+        BlockDirtyBitmapOrStr *el = g_new(BlockDirtyBitmapOrStr, 1);
+
+        *el = (BlockDirtyBitmapOrStr) {
+            .type = QTYPE_QSTRING,
+            .u.local = g_strdup(arg->bitmap),
+        };
         export_opts->u.nbd.has_bitmaps = true;
-        QAPI_LIST_PREPEND(export_opts->u.nbd.bitmaps, g_strdup(arg->bitmap));
+        QAPI_LIST_PREPEND(export_opts->u.nbd.bitmaps, el);
     }
 
     /*
diff --git a/nbd/server.c b/nbd/server.c
index 5da884c2fc..3956602c0a 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1643,7 +1643,7 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
     uint64_t perm, shared_perm;
     bool readonly = !exp_args->writable;
     bool shared = !exp_args->writable;
-    strList *bitmaps;
+    BlockDirtyBitmapOrStrList *bitmaps;
     size_t i;
     int ret;
 
@@ -1709,37 +1709,56 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
     }
     exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps);
     for (i = 0, bitmaps = arg->bitmaps; bitmaps;
-         i++, bitmaps = bitmaps->next) {
-        const char *bitmap = bitmaps->value;
+         i++, bitmaps = bitmaps->next)
+    {
+        const char *bitmap;
         BlockDriverState *bs = blk_bs(blk);
         BdrvDirtyBitmap *bm = NULL;
 
-        while (bs) {
-            bm = bdrv_find_dirty_bitmap(bs, bitmap);
-            if (bm != NULL) {
-                break;
+        switch (bitmaps->value->type) {
+        case QTYPE_QSTRING:
+            bitmap = bitmaps->value->u.local;
+            while (bs) {
+                bm = bdrv_find_dirty_bitmap(bs, bitmap);
+                if (bm != NULL) {
+                    break;
+                }
+
+                bs = bdrv_filter_or_cow_bs(bs);
             }
 
-            bs = bdrv_filter_or_cow_bs(bs);
-        }
+            if (bm == NULL) {
+                ret = -ENOENT;
+                error_setg(errp, "Bitmap '%s' is not found",
+                           bitmaps->value->u.local);
+                goto fail;
+            }
 
-        if (bm == NULL) {
-            ret = -ENOENT;
-            error_setg(errp, "Bitmap '%s' is not found", bitmap);
-            goto fail;
+            if (readonly && bdrv_is_writable(bs) &&
+                bdrv_dirty_bitmap_enabled(bm)) {
+                ret = -EINVAL;
+                error_setg(errp, "Enabled bitmap '%s' incompatible with "
+                           "readonly export", bitmap);
+                goto fail;
+            }
+            break;
+        case QTYPE_QDICT:
+            bitmap = bitmaps->value->u.external.name;
+            bm = block_dirty_bitmap_lookup(bitmaps->value->u.external.node,
+                                           bitmap, NULL, errp);
+            if (!bm) {
+                ret = -ENOENT;
+                goto fail;
+            }
+            break;
+        default:
+            abort();
         }
 
-        if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
-            ret = -EINVAL;
-            goto fail;
-        }
+        assert(bm);
 
-        if (readonly && bdrv_is_writable(bs) &&
-            bdrv_dirty_bitmap_enabled(bm)) {
+        if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
             ret = -EINVAL;
-            error_setg(errp,
-                       "Enabled bitmap '%s' incompatible with readonly export",
-                       bitmap);
             goto fail;
         }
 
diff --git a/qapi/block-export.json b/qapi/block-export.json
index f183522d0d..6afed472ff 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -6,6 +6,7 @@
 ##
 
 { 'include': 'sockets.json' }
+{ 'include': 'block-core.json' }
 
 ##
 # @NbdServerOptions:
@@ -89,6 +90,7 @@
 #           @device, so the NBD client can use NBD_OPT_SET_META_CONTEXT with
 #           the metadata context name "qemu:dirty-bitmap:BITMAP" to inspect
 #           each bitmap.
+#           Since 7.1 bitmap may be specified by node/name pair.
 #
 # @allocation-depth: Also export the allocation depth map for @device, so
 #                    the NBD client can use NBD_OPT_SET_META_CONTEXT with
@@ -99,7 +101,8 @@
 ##
 { 'struct': 'BlockExportOptionsNbd',
   'base': 'BlockExportOptionsNbdBase',
-  'data': { '*bitmaps': ['str'], '*allocation-depth': 'bool' } }
+  'data': { '*bitmaps': ['BlockDirtyBitmapOrStr'],
+            '*allocation-depth': 'bool' } }
 
 ##
 # @BlockExportOptionsVhostUserBlk:
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 713e7557a9..72138f703e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -567,7 +567,7 @@ int main(int argc, char **argv)
     QDict *options = NULL;
     const char *export_name = NULL; /* defaults to "" later for server mode */
     const char *export_description = NULL;
-    strList *bitmaps = NULL;
+    BlockDirtyBitmapOrStrList *bitmaps = NULL;
     bool alloc_depth = false;
     const char *tlscredsid = NULL;
     const char *tlshostname = NULL;
@@ -687,7 +687,14 @@ int main(int argc, char **argv)
             alloc_depth = true;
             break;
         case 'B':
-            QAPI_LIST_PREPEND(bitmaps, g_strdup(optarg));
+            {
+                BlockDirtyBitmapOrStr *el = g_new(BlockDirtyBitmapOrStr, 1);
+                *el = (BlockDirtyBitmapOrStr) {
+                    .type = QTYPE_QSTRING,
+                    .u.local = g_strdup(optarg),
+                };
+                QAPI_LIST_PREPEND(bitmaps, el);
+            }
             break;
         case 'k':
             sockpath = optarg;
-- 
2.35.1

Re: [PATCH v2 2/3] qapi: nbd-export: allow select bitmaps by node/name pair

[Eric Blake]

On Tue, Mar 15, 2022 at 12:32:25AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
> 
> Hi all! Current logic of relying on search through backing chain is not
> safe neither convenient.
> 
> Sometimes it leads to necessity of extra bitmap copying. Also, we are
> going to add "snapshot-access" driver, to access some snapshot state
> through NBD. And this driver is not formally a filter, and of course
> it's not a COW format driver. So, searching through backing chain will
> not work. Instead of widening the workaround of bitmap searching, let's
> extend the interface so that user can select bitmap precisely.
> 
> Note, that checking for bitmap active status is not copied to the new
> API, I don't see a reason for it, user should understand the risks. And
> anyway, bitmap from other node is unrelated to this export being
> read-only or read-write.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
> ---
>  blockdev-nbd.c         |  8 +++++-
>  nbd/server.c           | 63 +++++++++++++++++++++++++++---------------
>  qapi/block-export.json |  5 +++-
>  qemu-nbd.c             | 11 ++++++--
>  4 files changed, 61 insertions(+), 26 deletions(-)
> 

> @@ -1709,37 +1709,56 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
>      }
>      exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps);
>      for (i = 0, bitmaps = arg->bitmaps; bitmaps;
> -         i++, bitmaps = bitmaps->next) {
> -        const char *bitmap = bitmaps->value;
> +         i++, bitmaps = bitmaps->next)
> +    {
> +        const char *bitmap;

I'm not sure if our prevailing style splits { to its own line on a
multi-line 'for'.  But this is a cosmetic question, not one of
correctness.

> +        case QTYPE_QDICT:
> +            bitmap = bitmaps->value->u.external.name;
> +            bm = block_dirty_bitmap_lookup(bitmaps->value->u.external.node,
> +                                           bitmap, NULL, errp);
> +            if (!bm) {
> +                ret = -ENOENT;
> +                goto fail;
> +            }
> +            break;
> +        default:
> +            abort();

Not sure if g_assert_not_reached() or __builtin_unreachable() would be
any better here.  I'm fine with the abort() for now.

> +++ b/qapi/block-export.json
> @@ -6,6 +6,7 @@
>  ##
>  
>  { 'include': 'sockets.json' }
> +{ 'include': 'block-core.json' }

Hmm.  Does this extra inclusion negatively impact qemu-storage-daemon,
since that is why we created block-export.json in the first place (to
minimize the stuff that qsd pulled in without needing all of
block-core.json)?  In other words, would it be better to move
BlockDirtyBitmapOrStr to this file?

Everything else looks okay with this patch.

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

Re: [PATCH v2 2/3] qapi: nbd-export: allow select bitmaps by node/name pair

[Vladimir Sementsov-Ogievskiy]

17.03.2022 00:28, Eric Blake wrote:
> On Tue, Mar 15, 2022 at 12:32:25AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
>>
>> Hi all! Current logic of relying on search through backing chain is not
>> safe neither convenient.
>>
>> Sometimes it leads to necessity of extra bitmap copying. Also, we are
>> going to add "snapshot-access" driver, to access some snapshot state
>> through NBD. And this driver is not formally a filter, and of course
>> it's not a COW format driver. So, searching through backing chain will
>> not work. Instead of widening the workaround of bitmap searching, let's
>> extend the interface so that user can select bitmap precisely.
>>
>> Note, that checking for bitmap active status is not copied to the new
>> API, I don't see a reason for it, user should understand the risks. And
>> anyway, bitmap from other node is unrelated to this export being
>> read-only or read-write.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
>> ---
>>   blockdev-nbd.c         |  8 +++++-
>>   nbd/server.c           | 63 +++++++++++++++++++++++++++---------------
>>   qapi/block-export.json |  5 +++-
>>   qemu-nbd.c             | 11 ++++++--
>>   4 files changed, 61 insertions(+), 26 deletions(-)
>>
> 
>> @@ -1709,37 +1709,56 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
>>       }
>>       exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps);
>>       for (i = 0, bitmaps = arg->bitmaps; bitmaps;
>> -         i++, bitmaps = bitmaps->next) {
>> -        const char *bitmap = bitmaps->value;
>> +         i++, bitmaps = bitmaps->next)
>> +    {
>> +        const char *bitmap;
> 
> I'm not sure if our prevailing style splits { to its own line on a
> multi-line 'for'.  But this is a cosmetic question, not one of
> correctness.
> 
>> +        case QTYPE_QDICT:
>> +            bitmap = bitmaps->value->u.external.name;
>> +            bm = block_dirty_bitmap_lookup(bitmaps->value->u.external.node,
>> +                                           bitmap, NULL, errp);
>> +            if (!bm) {
>> +                ret = -ENOENT;
>> +                goto fail;
>> +            }
>> +            break;
>> +        default:
>> +            abort();
> 
> Not sure if g_assert_not_reached() or __builtin_unreachable() would be
> any better here.  I'm fine with the abort() for now.
> 
>> +++ b/qapi/block-export.json
>> @@ -6,6 +6,7 @@
>>   ##
>>   
>>   { 'include': 'sockets.json' }
>> +{ 'include': 'block-core.json' }
> 
> Hmm.  Does this extra inclusion negatively impact qemu-storage-daemon,
> since that is why we created block-export.json in the first place (to
> minimize the stuff that qsd pulled in without needing all of
> block-core.json)?  In other words, would it be better to move
> BlockDirtyBitmapOrStr to this file?

And include block-export in block-core?

Another alternative is to move BlockDirtyBitmapOrStr to a separate file included from both block-export and block-core but that seems to be too much.

> 
> Everything else looks okay with this patch.
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/3] qapi: nbd-export: allow select bitmaps by node/name pair

[Eric Blake]

On Mon, Mar 21, 2022 at 02:50:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > +++ b/qapi/block-export.json
> > > @@ -6,6 +6,7 @@
> > >   ##
> > >   { 'include': 'sockets.json' }
> > > +{ 'include': 'block-core.json' }
> > 
> > Hmm.  Does this extra inclusion negatively impact qemu-storage-daemon,
> > since that is why we created block-export.json in the first place (to
> > minimize the stuff that qsd pulled in without needing all of
> > block-core.json)?  In other words, would it be better to move
> > BlockDirtyBitmapOrStr to this file?
> 
> And include block-export in block-core?

Right now, we have:

qapi/block-core.json "Block core (VM unrelated)" - includes
{common,crypto,job,sockets}.json

qapi/block-export.json "Block device exports" - includes sockets.json

qapi/block.json "Additional block stuff (VM related)" - includes block-core.json

Kevin, you forked off qapi/block-export.json.  What do you propose here?

> 
> Another alternative is to move BlockDirtyBitmapOrStr to a separate file included from both block-export and block-core but that seems to be too much.

Indeed, that feels like a step too far; we already have confusion on
which file to stick new stuff in, and adding another file won't help
that.

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

Re: [PATCH v2 2/3] qapi: nbd-export: allow select bitmaps by node/name pair

[Vladimir Sementsov-Ogievskiy]

17.03.2022 00:28, Eric Blake wrote:
>> +++ b/qapi/block-export.json
>> @@ -6,6 +6,7 @@
>>   ##
>>   
>>   { 'include': 'sockets.json' }
>> +{ 'include': 'block-core.json' }
> Hmm.  Does this extra inclusion negatively impact qemu-storage-daemon,
> since that is why we created block-export.json in the first place (to
> minimize the stuff that qsd pulled in without needing all of
> block-core.json)?  In other words, would it be better to move
> BlockDirtyBitmapOrStr to this file?

Actually, looking at storage-daemon/qapi/qapi-schema.json I see block-cores.json.

That's block.json which is not mentioned in storage-daemon/qapi/qapi-schema.json.

So, I think it's OK to keep simple include for now.

-- 
Best regards,
Vladimir

Re: [PATCH v2 2/3] qapi: nbd-export: allow select bitmaps by node/name pair

[Eric Blake]

On Fri, Apr 08, 2022 at 11:27:42PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 17.03.2022 00:28, Eric Blake wrote:
> > > +++ b/qapi/block-export.json
> > > @@ -6,6 +6,7 @@
> > >   ##
> > >   { 'include': 'sockets.json' }
> > > +{ 'include': 'block-core.json' }
> > Hmm.  Does this extra inclusion negatively impact qemu-storage-daemon,
> > since that is why we created block-export.json in the first place (to
> > minimize the stuff that qsd pulled in without needing all of
> > block-core.json)?  In other words, would it be better to move
> > BlockDirtyBitmapOrStr to this file?
> 
> Actually, looking at storage-daemon/qapi/qapi-schema.json I see block-cores.json.
> 
> That's block.json which is not mentioned in storage-daemon/qapi/qapi-schema.json.
> 
> So, I think it's OK to keep simple include for now.

We're early enough in the 7.1 cycle that if someone proposes a reason
why this would need to change, then we can adjust it.

So for now, I'm adding

Reviewed-by: Eric Blake <eblake@redhat.com>

and queuing this series through my NBD tree.

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

[PATCH v2 3/3] iotests/223: check new possibility of exporting bitmaps by node/name

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>

Add simple test that new interface introduced in previous commit works.

Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
---
 tests/qemu-iotests/223     | 16 +++++++++++++
 tests/qemu-iotests/223.out | 47 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index da87f2f4a2..0bbb283010 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -120,6 +120,11 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
+  "arguments":{"driver":"null-co", "node-name":"null",
+    "size": 4194304}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-add",
+  "arguments":{"node":"null", "name":"b3"}}' "return"
 
 for attempt in normal iothread; do
 
@@ -155,6 +160,9 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2", "writable":true,
   "description":"some text", "bitmap":"b2"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-export-add",
+  "arguments":{"type": "nbd", "node-name":"n", "id":"n3", "name": "n3",
+  "bitmaps":[{"node":"null","name":"b3"}]}}' "return"
 $QEMU_NBD_PROG -L -k "$SOCK_DIR/nbd"
 
 echo
@@ -178,6 +186,14 @@ IMG="driver=nbd,export=n2,server.type=unix,server.path=$SOCK_DIR/nbd"
 $QEMU_IMG map --output=json --image-opts \
   "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
 
+echo
+echo "=== Check bitmap taken from another node ==="
+echo
+
+IMG="driver=nbd,export=n3,server.type=unix,server.path=$SOCK_DIR/nbd"
+$QEMU_IMG map --output=json --image-opts \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b3" | _filter_qemu_img_map
+
 echo
 echo "=== End qemu NBD server ==="
 echo
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index e58ea5abbd..0647941531 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -33,6 +33,13 @@ wrote 2097152/2097152 bytes at offset 2097152
 {"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b"}}
 {"return": {}}
+{"execute":"blockdev-add",
+  "arguments":{"driver":"null-co", "node-name":"null",
+    "size": 4194304}}
+{"return": {}}
+{"execute":"block-dirty-bitmap-add",
+  "arguments":{"node":"null", "name":"b3"}}
+{"return": {}}
 
 === Set up NBD with normal access ===
 
@@ -69,7 +76,11 @@ exports available: 0
   "arguments":{"device":"n", "name":"n2", "writable":true,
   "description":"some text", "bitmap":"b2"}}
 {"return": {}}
-exports available: 2
+{"execute":"block-export-add",
+  "arguments":{"type": "nbd", "node-name":"n", "id":"n3", "name": "n3",
+  "bitmaps":[{"node":"null","name":"b3"}]}}
+{"return": {}}
+exports available: 3
  export: 'n'
   size:  4194304
   flags: 0x58f ( readonly flush fua df multi cache )
@@ -89,6 +100,15 @@ exports available: 2
   available meta contexts: 2
    base:allocation
    qemu:dirty-bitmap:b2
+ export: 'n3'
+  size:  4194304
+  flags: 0x58f ( readonly flush fua df multi cache )
+  min block: 1
+  opt block: 4096
+  max block: 33554432
+  available meta contexts: 2
+   base:allocation
+   qemu:dirty-bitmap:b3
 
 === Contrast normal status to large granularity dirty-bitmap ===
 
@@ -114,6 +134,10 @@ read 2097152/2097152 bytes at offset 2097152
 { "start": 1024, "length": 2096128, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 2097152, "depth": 0, "present": false, "zero": false, "data": false}]
 
+=== Check bitmap taken from another node ===
+
+[{ "start": 0, "length": 4194304, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
+
 === End qemu NBD server ===
 
 {"execute":"nbd-server-remove",
@@ -128,6 +152,7 @@ read 2097152/2097152 bytes at offset 2097152
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "n2"}}
 {"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
 {"execute":"nbd-server-stop"}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "n3"}}
 {"return": {}}
 {"execute":"nbd-server-stop"}
 {"error": {"class": "GenericError", "desc": "NBD server not running"}}
@@ -170,7 +195,11 @@ exports available: 0
   "arguments":{"device":"n", "name":"n2", "writable":true,
   "description":"some text", "bitmap":"b2"}}
 {"return": {}}
-exports available: 2
+{"execute":"block-export-add",
+  "arguments":{"type": "nbd", "node-name":"n", "id":"n3", "name": "n3",
+  "bitmaps":[{"node":"null","name":"b3"}]}}
+{"return": {}}
+exports available: 3
  export: 'n'
   size:  4194304
   flags: 0x58f ( readonly flush fua df multi cache )
@@ -190,6 +219,15 @@ exports available: 2
   available meta contexts: 2
    base:allocation
    qemu:dirty-bitmap:b2
+ export: 'n3'
+  size:  4194304
+  flags: 0x58f ( readonly flush fua df multi cache )
+  min block: 1
+  opt block: 4096
+  max block: 33554432
+  available meta contexts: 2
+   base:allocation
+   qemu:dirty-bitmap:b3
 
 === Contrast normal status to large granularity dirty-bitmap ===
 
@@ -215,6 +253,10 @@ read 2097152/2097152 bytes at offset 2097152
 { "start": 1024, "length": 2096128, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 2097152, "depth": 0, "present": false, "zero": false, "data": false}]
 
+=== Check bitmap taken from another node ===
+
+[{ "start": 0, "length": 4194304, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
+
 === End qemu NBD server ===
 
 {"execute":"nbd-server-remove",
@@ -229,6 +271,7 @@ read 2097152/2097152 bytes at offset 2097152
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "n2"}}
 {"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
 {"execute":"nbd-server-stop"}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "n3"}}
 {"return": {}}
 {"execute":"nbd-server-stop"}
 {"error": {"class": "GenericError", "desc": "NBD server not running"}}
-- 
2.35.1

Re: [PATCH v2 3/3] iotests/223: check new possibility of exporting bitmaps by node/name

[Eric Blake]

On Tue, Mar 15, 2022 at 12:32:26AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
> 
> Add simple test that new interface introduced in previous commit works.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
> ---
>  tests/qemu-iotests/223     | 16 +++++++++++++
>  tests/qemu-iotests/223.out | 47 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 61 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v2 for-7.1 0/3] qapi: nbd-export: select bitmap by node/name pair

[Vladimir Sementsov-Ogievskiy]

15.03.2022 00:32, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
> 
> Hi all!
> 
> Here is small improvement for bitmap exporting interface.
> 
> v2: Sorry for the noise, me trying to find a email service, that don't
> consider sending patch series by git-send-email as as spam :/

Aha, and @mail.ru works a lot better. So, that's a candidate for v2 of my "[PATCH] MAINTAINERS: change Vladimir's email address".

This series itself is OK for reviewing, but email in s-o-b mark may change in v3.

> 
> Vladimir Sementsov-Ogievskiy (3):
>    qapi: rename BlockDirtyBitmapMergeSource to BlockDirtyBitmapOrStr
>    qapi: nbd-export: allow select bitmaps by node/name pair
>    iotests/223: check new possibility of exporting bitmaps by node/name
> 
>   block/monitor/bitmap-qmp-cmds.c        |  6 +--
>   blockdev-nbd.c                         |  8 +++-
>   include/block/block_int-global-state.h |  2 +-
>   nbd/server.c                           | 63 +++++++++++++++++---------
>   qapi/block-core.json                   |  6 +--
>   qapi/block-export.json                 |  5 +-
>   qemu-img.c                             |  8 ++--
>   qemu-nbd.c                             | 11 ++++-
>   tests/qemu-iotests/223                 | 16 +++++++
>   tests/qemu-iotests/223.out             | 47 ++++++++++++++++++-
>   10 files changed, 133 insertions(+), 39 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 for-7.1 0/3] qapi: nbd-export: select bitmap by node/name pair

[Eric Blake]

On Tue, Mar 15, 2022 at 12:45:04AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 15.03.2022 00:32, Vladimir Sementsov-Ogievskiy wrote:
> > From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>
> > 
> > Hi all!
> > 
> > Here is small improvement for bitmap exporting interface.
> > 
> > v2: Sorry for the noise, me trying to find a email service, that don't
> > consider sending patch series by git-send-email as as spam :/
> 
> Aha, and @mail.ru works a lot better. So, that's a candidate for v2 of my "[PATCH] MAINTAINERS: change Vladimir's email address".
> 
> This series itself is OK for reviewing, but email in s-o-b mark may change in v3.

Vladimir, do I have your permission to alter your S-o-b marks from
@ya.ru to @mail.ru (since we have established that you have sent mail
from both addresses, but selected @mail.ru in MAINTAINERS)?  That's
the only thing that's preventing me from queuing this series as-is.

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