diff mbox series

[v5,06/12] nbd: Update qapi to support exporting multiple bitmaps

Message ID 20201023183652.478921-7-eblake@redhat.com (mailing list archive)
State New, archived
Headers show
Series Exposing backing-chain allocation over NBD | expand

Commit Message

Eric Blake Oct. 23, 2020, 6:36 p.m. UTC 
Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to
5.2, we can still tweak the interface.  Allowing 'bitmaps':['str'] is
nicer than 'bitmap':'str'.  This wires up the qapi and qemu-nbd
changes to permit passing multiple bitmaps as distinct metadata
contexts that the NBD client may request, but the actual support for
more than one will require a further patch to the server.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/system/deprecated.rst |  4 +++-
 qapi/block-export.json     | 18 ++++++++++++------
 blockdev-nbd.c             | 13 +++++++++++++
 nbd/server.c               | 19 +++++++++++++------
 qemu-nbd.c                 | 10 +++++-----
 5 files changed, 46 insertions(+), 18 deletions(-)

Comments

Peter Krempa Oct. 26, 2020, 10:50 a.m. UTC | #1 
On Fri, Oct 23, 2020 at 13:36:46 -0500, Eric Blake wrote:
> Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to
> 5.2, we can still tweak the interface.  Allowing 'bitmaps':['str'] is
> nicer than 'bitmap':'str'.  This wires up the qapi and qemu-nbd
> changes to permit passing multiple bitmaps as distinct metadata
> contexts that the NBD client may request, but the actual support for
> more than one will require a further patch to the server.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/system/deprecated.rst |  4 +++-
>  qapi/block-export.json     | 18 ++++++++++++------
>  blockdev-nbd.c             | 13 +++++++++++++
>  nbd/server.c               | 19 +++++++++++++------
>  qemu-nbd.c                 | 10 +++++-----
>  5 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 905628f3a0cb..d6cd027ac740 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -268,7 +268,9 @@ the 'wait' field, which is only applicable to sockets in server mode
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> 
>  Use the more generic commands ``block-export-add`` and ``block-export-del``
> -instead.
> +instead.  As part of this deprecation, it is now preferred to export a
> +list of dirty bitmaps via ``bitmaps``, rather than a single bitmap via
> +``bitmap``.
> 
>  Human Monitor Protocol (HMP) commands
>  -------------------------------------
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 893d5cde5dfe..c7c749d61097 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -74,10 +74,10 @@
>  # @description: Free-form description of the export, up to 4096 bytes.
>  #               (Since 5.0)
>  #
> -# @bitmap: Also export the dirty bitmap reachable from @device, so the
> -#          NBD client can use NBD_OPT_SET_META_CONTEXT with the
> -#          metadata context name "qemu:dirty-bitmap:NAME" to inspect the
> -#          bitmap. (since 4.0)
> +# @bitmaps: Also export each of the named dirty bitmaps reachable from
> +#           @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 5.2)

Given unsynchronised release cycles between qemu and management apps
it's not cool to deprecate an interface without having at least one
release where the replacement interface is already stable.

This means that any project wanting to stay up to date will either have
to use deprecated interfaces for at least one release and develop the
replacement only when there's a stable interface or hope that they don't
have to change the interfaces too often.

This specifically impacts libvirt as we have validators which notify us
that a deprecated interface is used and we want to stay in sync, so that
there's one less group of users to worry about at the point qemu will
want to delete the interface.

>  # @allocation-depth: Also export the allocation depth map for @device, so
>  #                    the NBD client can use NBD_OPT_SET_META_CONTEXT with
> @@ -88,7 +88,7 @@
>  ##
>  { 'struct': 'BlockExportOptionsNbd',
>    'data': { '*name': 'str', '*description': 'str',
> -            '*bitmap': 'str', '*allocation-depth': 'bool' } }
> +            '*bitmaps': ['str'], '*allocation-depth': 'bool' } }

This adds 'bitmaps' also to nbd-server-add, which should not happen. You
probably want to stop using 'base' for 'NbdServerAddOptions' and just
duplicate everything else.

>  # @NbdServerAddOptions:
> @@ -100,12 +100,18 @@
>  # @writable: Whether clients should be able to write to the device via the
>  #            NBD connection (default false).
>  #
> +# @bitmap: Also export a single dirty bitmap reachable from @device, so the
> +#          NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata
> +#          context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap
> +#          (since 4.0).  Mutually exclusive with @bitmaps, and newer
> +#          clients should use that instead.

This doesn't make sense, nbd-server-add never had @bitmaps. Also adding
features to a deprecated interface doesn't IMO make sense if you want to
motivate users switch to thne new one.

> +#
>  # Since: 5.0
>  ##
>  { 'struct': 'NbdServerAddOptions',
>    'base': 'BlockExportOptionsNbd',
>    'data': { 'device': 'str',
> -            '*writable': 'bool' } }
> +            '*writable': 'bool', '*bitmap': 'str' } }
> 
>  ##
>  # @nbd-server-add:
Eric Blake Oct. 26, 2020, 1:06 p.m. UTC | #2 
On 10/26/20 5:50 AM, Peter Krempa wrote:
> On Fri, Oct 23, 2020 at 13:36:46 -0500, Eric Blake wrote:
>> Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to
>> 5.2, we can still tweak the interface.  Allowing 'bitmaps':['str'] is
>> nicer than 'bitmap':'str'.  This wires up the qapi and qemu-nbd
>> changes to permit passing multiple bitmaps as distinct metadata
>> contexts that the NBD client may request, but the actual support for
>> more than one will require a further patch to the server.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---

>> +++ b/qapi/block-export.json
>> @@ -74,10 +74,10 @@
>>  # @description: Free-form description of the export, up to 4096 bytes.
>>  #               (Since 5.0)
>>  #
>> -# @bitmap: Also export the dirty bitmap reachable from @device, so the
>> -#          NBD client can use NBD_OPT_SET_META_CONTEXT with the
>> -#          metadata context name "qemu:dirty-bitmap:NAME" to inspect the
>> -#          bitmap. (since 4.0)
>> +# @bitmaps: Also export each of the named dirty bitmaps reachable from
>> +#           @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 5.2)
> 
> Given unsynchronised release cycles between qemu and management apps
> it's not cool to deprecate an interface without having at least one
> release where the replacement interface is already stable.
> 

That's why I'm trying as hard as possible to get the block-export-add
interface perfect in its 5.2 release; if we agree that allowing qemu to
expose more than one bitmap is beneficial (and I argue that it is), then
the new interface MUST support that from the get-go, and not something
where we release it with 5.2 having 'bitmap' and 6.0 adding 'bitmaps'.

> This means that any project wanting to stay up to date will either have
> to use deprecated interfaces for at least one release and develop the
> replacement only when there's a stable interface or hope that they don't
> have to change the interfaces too often.
> 
> This specifically impacts libvirt as we have validators which notify us
> that a deprecated interface is used and we want to stay in sync, so that
> there's one less group of users to worry about at the point qemu will
> want to delete the interface.

The deprecated interface is nbd-server-add; for _that_ interface, the
'bitmap' parameter will continue to work until nbd-server-add is removed
in 6.1 (so you have all of 5.2 and 6.0 to switch from nbd-server-add to
block-export-add).

What this patch does, then, is alter the deprecation from merely
changing the command from nbd-server-add to block-export-add with all
parameter names remaining the same, to instead changing both the command
name and the 'bitmap'=>'bitmaps' parameter.  But I agree that libvirt
wants to do an all-or-none conversion: there is no reason for libvirt to
use block-export-add until 5.2 is actually released, at which point we
have locked in the new interface; and this patch is a demonstration that
we are still debating about a tweak to that interface before it becomes
locked in.

> 
>>  # @allocation-depth: Also export the allocation depth map for @device, so
>>  #                    the NBD client can use NBD_OPT_SET_META_CONTEXT with
>> @@ -88,7 +88,7 @@
>>  ##
>>  { 'struct': 'BlockExportOptionsNbd',
>>    'data': { '*name': 'str', '*description': 'str',
>> -            '*bitmap': 'str', '*allocation-depth': 'bool' } }
>> +            '*bitmaps': ['str'], '*allocation-depth': 'bool' } }
> 
> This adds 'bitmaps' also to nbd-server-add, which should not happen. You
> probably want to stop using 'base' for 'NbdServerAddOptions' and just
> duplicate everything else.

I can respin this to NOT add 'bitmaps' to the legacy 'nbd-server-add',
if you think that would be better.  It is more complex in the QAPI code,
but not too much more difficulty in the glue code; and the glue code all
goes away in 6.1 when the deprecation cycle ends.

> 
>>  # @NbdServerAddOptions:
>> @@ -100,12 +100,18 @@
>>  # @writable: Whether clients should be able to write to the device via the
>>  #            NBD connection (default false).
>>  #
>> +# @bitmap: Also export a single dirty bitmap reachable from @device, so the
>> +#          NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata
>> +#          context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap
>> +#          (since 4.0).  Mutually exclusive with @bitmaps, and newer
>> +#          clients should use that instead.
> 
> This doesn't make sense, nbd-server-add never had @bitmaps. Also adding
> features to a deprecated interface doesn't IMO make sense if you want to
> motivate users switch to thne new one.

Fair enough. v6 coming up later today.
diff mbox series

Patch

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 905628f3a0cb..d6cd027ac740 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -268,7 +268,9 @@  the 'wait' field, which is only applicable to sockets in server mode
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''''

 Use the more generic commands ``block-export-add`` and ``block-export-del``
-instead.
+instead.  As part of this deprecation, it is now preferred to export a
+list of dirty bitmaps via ``bitmaps``, rather than a single bitmap via
+``bitmap``.

 Human Monitor Protocol (HMP) commands
 -------------------------------------
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 893d5cde5dfe..c7c749d61097 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -74,10 +74,10 @@ 
 # @description: Free-form description of the export, up to 4096 bytes.
 #               (Since 5.0)
 #
-# @bitmap: Also export the dirty bitmap reachable from @device, so the
-#          NBD client can use NBD_OPT_SET_META_CONTEXT with the
-#          metadata context name "qemu:dirty-bitmap:NAME" to inspect the
-#          bitmap. (since 4.0)
+# @bitmaps: Also export each of the named dirty bitmaps reachable from
+#           @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 5.2)
 #
 # @allocation-depth: Also export the allocation depth map for @device, so
 #                    the NBD client can use NBD_OPT_SET_META_CONTEXT with
@@ -88,7 +88,7 @@ 
 ##
 { 'struct': 'BlockExportOptionsNbd',
   'data': { '*name': 'str', '*description': 'str',
-            '*bitmap': 'str', '*allocation-depth': 'bool' } }
+            '*bitmaps': ['str'], '*allocation-depth': 'bool' } }

 ##
 # @NbdServerAddOptions:
@@ -100,12 +100,18 @@ 
 # @writable: Whether clients should be able to write to the device via the
 #            NBD connection (default false).
 #
+# @bitmap: Also export a single dirty bitmap reachable from @device, so the
+#          NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata
+#          context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap
+#          (since 4.0).  Mutually exclusive with @bitmaps, and newer
+#          clients should use that instead.
+#
 # Since: 5.0
 ##
 { 'struct': 'NbdServerAddOptions',
   'base': 'BlockExportOptionsNbd',
   'data': { 'device': 'str',
-            '*writable': 'bool' } }
+            '*writable': 'bool', '*bitmap': 'str' } }

 ##
 # @nbd-server-add:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index cee9134b12eb..cfd46223bf4d 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -192,6 +192,19 @@  void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
         return;
     }

+    /*
+     * New code should use the list 'bitmaps'; but until this code is
+     * gone, we must support the older single 'bitmap'.  Use only one.
+     */
+    if (arg->has_bitmap) {
+        if (arg->has_bitmaps) {
+            error_setg(errp, "Can't mix 'bitmap' and 'bitmaps'");
+            return;
+        }
+        arg->has_bitmaps = true;
+        QAPI_LIST_ADD(arg->bitmaps, g_strdup(arg->bitmap));
+    }
+
     /*
      * block-export-add would default to the node-name, but we may have to use
      * the device name as a default here for compatibility.
diff --git a/nbd/server.c b/nbd/server.c
index 30cfe0eee467..884ffa00f1bd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1495,6 +1495,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;
     int ret;

     assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
@@ -1556,12 +1557,18 @@  static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
     }
     exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);

-    if (arg->bitmap) {
+    /* XXX Allow more than one bitmap */
+    if (arg->bitmaps && arg->bitmaps->next) {
+        error_setg(errp, "multiple bitmaps per export not supported yet");
+        return -EOPNOTSUPP;
+    }
+    for (bitmaps = arg->bitmaps; bitmaps; bitmaps = bitmaps->next) {
+        const char *bitmap = bitmaps->value;
         BlockDriverState *bs = blk_bs(blk);
         BdrvDirtyBitmap *bm = NULL;

         while (bs) {
-            bm = bdrv_find_dirty_bitmap(bs, arg->bitmap);
+            bm = bdrv_find_dirty_bitmap(bs, bitmap);
             if (bm != NULL) {
                 break;
             }
@@ -1571,7 +1578,7 @@  static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,

         if (bm == NULL) {
             ret = -ENOENT;
-            error_setg(errp, "Bitmap '%s' is not found", arg->bitmap);
+            error_setg(errp, "Bitmap '%s' is not found", bitmap);
             goto fail;
         }

@@ -1585,15 +1592,15 @@  static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
             ret = -EINVAL;
             error_setg(errp,
                        "Enabled bitmap '%s' incompatible with readonly export",
-                       arg->bitmap);
+                       bitmap);
             goto fail;
         }

         bdrv_dirty_bitmap_set_busy(bm, true);
         exp->export_bitmap = bm;
-        assert(strlen(arg->bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
+        assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
         exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
-                                                     arg->bitmap);
+                                                     bitmap);
         assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
     }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 847fde435a7f..5473821216f7 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -572,7 +572,7 @@  int main(int argc, char **argv)
     const char *export_name = NULL; /* defaults to "" later for server mode */
     const char *export_description = NULL;
     bool alloc_depth = false;
-    const char *bitmap = NULL;
+    strList *bitmaps = NULL;
     const char *tlscredsid = NULL;
     bool imageOpts = false;
     bool writethrough = true;
@@ -701,7 +701,7 @@  int main(int argc, char **argv)
             alloc_depth = true;
             break;
         case 'B':
-            bitmap = optarg;
+            QAPI_LIST_ADD(bitmaps, g_strdup(optarg));
             break;
         case 'k':
             sockpath = optarg;
@@ -798,7 +798,7 @@  int main(int argc, char **argv)
         }
         if (export_name || export_description || dev_offset ||
             device || disconnect || fmt || sn_id_or_name || alloc_depth ||
-            bitmap || seen_aio || seen_discard || seen_cache) {
+            bitmaps || seen_aio || seen_discard || seen_cache) {
             error_report("List mode is incompatible with per-device settings");
             exit(EXIT_FAILURE);
         }
@@ -1082,8 +1082,8 @@  int main(int argc, char **argv)
             .name                 = g_strdup(export_name),
             .has_description      = !!export_description,
             .description          = g_strdup(export_description),
-            .has_bitmap           = !!bitmap,
-            .bitmap               = g_strdup(bitmap),
+            .has_bitmaps          = !!bitmaps,
+            .bitmaps              = bitmaps,
             .has_allocation_depth = alloc_depth,
             .allocation_depth     = alloc_depth,
         },