[Qemu-devel] [PATCH v2 0/3] NBD export bitmaps

[Qemu-devel] [PATCH v2 0/3] NBD export bitmaps

[Vladimir Sementsov-Ogievskiy]

Hi all.

This is a proposal and realization of new NBD meta context:
qemu. (I hope to send corresponding proposal to NBD protocol soon)

New possible queries will look like:
qemu:dirty-bitmap:<export-bitmap-name>

Mapping from export-bitmap-name to BdrvDirtyBitmap is done through qmp
command nbd-server-add-bitmap. For now, only one bitmap export is
allowed per NBD export, however it may be easily improved if needed 
(we don't have such cases for now)

Client and testing.
I wrote client code for Virtuozzo, but it turned out to be unused,
actually it's used only for tests. We don't have cases, where we need
to import dirty bitmap through qemu nbd-client. All this done for
exporting dirty bitmaps to the third tool. So, I think, it is not worth
refactoring, rebasing and merging client part upstream, if there are no
real usage cases.

v2:
01 from v1 is dropped: actually, we don't need generic namespace
parsing for now (especially, after moving to qemu: namespace, which has
the same length as base:), lets postpone it.

01: Improve comment wording (Eric), add Eric's r-b
02: improve commit message
    move NBD_STATE_DIRTY to header
    add comment on NBD_MAX_BITMAP_EXTENTS
    remove MAX_EXTENT_LENGTH and instead update add_extents() which
      uses it
    use export_bitmap_context instead of export_bitmap_name to reduce
      operations on it
    move from qemu-dirty-bitmap to qemu:dirty-bitmap
    other way to parse namespace name
    handle FLAG_DF
03: Improve specification of new qmp command (Eric)

Vladimir Sementsov-Ogievskiy (3):
  nbd/server: add nbd_meta_single_query helper
  nbd/server: implement dirty bitmap export
  qapi: new qmp command nbd-server-add-bitmap

 qapi/block.json     |  23 +++++
 include/block/nbd.h |   6 ++
 blockdev-nbd.c      |  23 +++++
 nbd/server.c        | 274 ++++++++++++++++++++++++++++++++++++++++++++++------
 4 files changed, 297 insertions(+), 29 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH v2 1/3] nbd/server: add nbd_meta_single_query helper

[Vladimir Sementsov-Ogievskiy]

The helper will be reused for bitmaps namespace.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index cea158913b..c169c11b3e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -733,44 +733,58 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
     return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? -EIO : 0;
 }
 
-/* nbd_meta_base_query
- *
- * Handle query to 'base' namespace. For now, only base:allocation context is
- * available in it.  'len' is the amount of text remaining to be read from
- * the current name, after the 'base:' portion has been stripped.
+/* Read @len bytes, and set @match to true if they match @pattern, or if @len
+ * is 0 and the client is performing _LIST_. @match is never set to false.
  *
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
-                               uint32_t len, Error **errp)
+static int nbd_meta_single_query(NBDClient *client, const char *pattern,
+                                 uint32_t len, bool *match, Error **errp)
 {
     int ret;
-    char query[sizeof("allocation") - 1];
-    size_t alen = strlen("allocation");
+    char *query;
 
     if (len == 0) {
         if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
-            meta->base_allocation = true;
+            *match = true;
         }
         return 1;
     }
 
-    if (len != alen) {
+    if (len != strlen(pattern)) {
         return nbd_opt_skip(client, len, errp);
     }
 
+    query = g_malloc(len);
     ret = nbd_opt_read(client, query, len, errp);
     if (ret <= 0) {
+        g_free(query);
         return ret;
     }
 
-    if (strncmp(query, "allocation", alen) == 0) {
-        meta->base_allocation = true;
+    if (strncmp(query, pattern, len) == 0) {
+        *match = true;
     }
+    g_free(query);
 
     return 1;
 }
 
+/* nbd_meta_base_query
+ *
+ * Handle query to 'base' namespace. For now, only base:allocation context is
+ * available in it.  'len' is the amount of text remaining to be read from
+ * the current name, after the 'base:' portion has been stripped.
+ *
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */
+static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
+                               uint32_t len, Error **errp)
+{
+    return nbd_meta_single_query(client, "allocation", len,
+                                 &meta->base_allocation, errp);
+}
+
 /* nbd_negotiate_meta_query
  *
  * Parse namespace name and call corresponding function to parse body of the
-- 
2.11.1

[Qemu-devel] [PATCH v2 2/3] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

Handle new NBD meta namespace: "qemu", and corresponding queries:
"qemu:dirty-bitmap:<export bitmap name>".

With new metadata context negotiated, BLOCK_STATUS query will reply
with dirty-bitmap data, converted to extents. New public function
nbd_export_bitmap selects bitmap to export. For now, only one bitmap
may be exported.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/nbd.h |   6 ++
 nbd/server.c        | 234 ++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 224 insertions(+), 16 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index fcdcd54502..a653d0fba7 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -234,6 +234,10 @@ enum {
 #define NBD_STATE_HOLE (1 << 0)
 #define NBD_STATE_ZERO (1 << 1)
 
+/* Flags for extents (NBDExtent.flags) of NBD_REPLY_TYPE_BLOCK_STATUS,
+ * for qemu:dirty-bitmap:* meta contexts */
+#define NBD_STATE_DIRTY (1 << 0)
+
 static inline bool nbd_reply_type_is_error(int type)
 {
     return type & (1 << 15);
@@ -315,6 +319,8 @@ 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/nbd/server.c b/nbd/server.c
index c169c11b3e..4682b99354 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -23,6 +23,12 @@
 #include "nbd-internal.h"
 
 #define NBD_META_ID_BASE_ALLOCATION 0
+#define NBD_META_ID_DIRTY_BITMAP 1
+
+/* NBD_MAX_BITMAP_EXTENTS: 1 mb of extents data. An empirical constant. If need
+ * to increase, note that NBD protocol defines 32 mb as a limit, after which the
+ * reply may be considered as a denial of service attack. */
+#define NBD_MAX_BITMAP_EXTENTS (0x100000 / 8)
 
 static int system_errno_to_nbd_errno(int err)
 {
@@ -80,6 +86,9 @@ struct NBDExport {
 
     BlockBackend *eject_notifier_blk;
     Notifier eject_notifier;
+
+    BdrvDirtyBitmap *export_bitmap;
+    char *export_bitmap_context;
 };
 
 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -92,6 +101,7 @@ typedef struct NBDExportMetaContexts {
     bool valid; /* means that negotiation of the option finished without
                    errors */
     bool base_allocation; /* export base:allocation context (block status) */
+    bool dirty_bitmap; /* export qemu:dirty-bitmap:<export bitmap name> */
 } NBDExportMetaContexts;
 
 struct NBDClient {
@@ -785,6 +795,26 @@ static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
                                  &meta->base_allocation, errp);
 }
 
+/* nbd_meta_bitmap_query
+ *
+ * Handle query to 'qemu-dirty-bitmap' namespace.
+ * 'len' is the amount of text remaining to be read from the current name, after
+ * the 'qemu-dirty-bitmap:' portion has been stripped.
+ *
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */
+static int nbd_meta_bitmap_query(NBDClient *client, NBDExportMetaContexts *meta,
+                                 uint32_t len, Error **errp)
+{
+    if (!client->exp->export_bitmap) {
+        return nbd_opt_skip(client, len, errp);
+    }
+
+    return nbd_meta_single_query(
+            client, client->exp->export_bitmap_context + strlen("qemu:"), len,
+            &meta->dirty_bitmap, errp);
+}
+
 /* nbd_negotiate_meta_query
  *
  * Parse namespace name and call corresponding function to parse body of the
@@ -801,8 +831,10 @@ static int nbd_negotiate_meta_query(NBDClient *client,
                                     NBDExportMetaContexts *meta, Error **errp)
 {
     int ret;
-    char query[sizeof("base:") - 1];
-    size_t baselen = strlen("base:");
+    size_t ns_len = 5;
+    char ns[5]; /* Both 'qemu' and 'base' namespaces have length = 5 including a
+                   colon. If it's needed to introduce a namespace of the other
+                   length, this should be certainly refactored. */
     uint32_t len;
 
     ret = nbd_opt_read(client, &len, sizeof(len), errp);
@@ -811,22 +843,23 @@ static int nbd_negotiate_meta_query(NBDClient *client,
     }
     cpu_to_be32s(&len);
 
-    /* The only supported namespace for now is 'base'. So query should start
-     * with 'base:'. Otherwise, we can ignore it and skip the remainder. */
-    if (len < baselen) {
+    if (len < ns_len) {
         return nbd_opt_skip(client, len, errp);
     }
 
-    len -= baselen;
-    ret = nbd_opt_read(client, query, baselen, errp);
+    len -= ns_len;
+    ret = nbd_opt_read(client, ns, ns_len, errp);
     if (ret <= 0) {
         return ret;
     }
-    if (strncmp(query, "base:", baselen) != 0) {
-        return nbd_opt_skip(client, len, errp);
+
+    if (!strncmp(ns, "base:", ns_len)) {
+        return nbd_meta_base_query(client, meta, len, errp);
+    } else if (!strncmp(ns, "qemu:", ns_len)) {
+        return nbd_meta_bitmap_query(client, meta, len, errp);
     }
 
-    return nbd_meta_base_query(client, meta, len, errp);
+    return nbd_opt_skip(client, len, errp);
 }
 
 /* nbd_negotiate_meta_queries
@@ -894,6 +927,16 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
         }
     }
 
+    if (meta->dirty_bitmap) {
+        ret = nbd_negotiate_send_meta_context(client,
+                                              exp->export_bitmap_context,
+                                              NBD_META_ID_DIRTY_BITMAP,
+                                              errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
     ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
     if (ret == 0) {
         meta->valid = true;
@@ -1522,6 +1565,11 @@ void nbd_export_put(NBDExport *exp)
             exp->blk = NULL;
         }
 
+        if (exp->export_bitmap) {
+            bdrv_dirty_bitmap_set_qmp_locked(exp->export_bitmap, false);
+            g_free(exp->export_bitmap_context);
+        }
+
         g_free(exp);
     }
 }
@@ -1763,6 +1811,9 @@ static int blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,
 }
 
 /* nbd_co_send_extents
+ *
+ * NBD_REPLY_FLAG_DONE is not set, don't forget to send it.
+ *
  * @extents should be in big-endian */
 static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
                                NBDExtent *extents, unsigned nb_extents,
@@ -1775,7 +1826,7 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
         {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
     };
 
-    set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_BLOCK_STATUS,
+    set_be_chunk(&chunk.h, 0, NBD_REPLY_TYPE_BLOCK_STATUS,
                  handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
     stl_be_p(&chunk.context_id, context_id);
 
@@ -1800,6 +1851,94 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
     return nbd_co_send_extents(client, handle, &extent, 1, context_id, errp);
 }
 
+/* Set several extents, describing region of given @length with given @flags.
+ * Do not set more than @nb_extents, return number of set extents.
+ */
+static unsigned add_extents(NBDExtent *extents, unsigned nb_extents,
+                            uint64_t length, uint32_t flags)
+{
+    unsigned i = 0;
+    uint32_t max_extent = QEMU_ALIGN_DOWN(INT32_MAX, 512);
+    /* TODO: should be somehow in agreement with blocksize of OPT_GO,
+     * and this should be documented in NBD proto. */
+
+    uint32_t max_extent_be = cpu_to_be32(max_extent);
+    uint32_t flags_be = cpu_to_be32(flags);
+
+    for (i = 0; i < nb_extents && length > max_extent;
+         i++, length -= max_extent)
+    {
+        extents[i].length = max_extent_be;
+        extents[i].flags = flags_be;
+    }
+
+    if (length > 0 && i < nb_extents) {
+        extents[i].length = cpu_to_be32(length);
+        extents[i].flags = flags_be;
+        i++;
+    }
+
+    return i;
+}
+
+static unsigned bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
+                                  uint64_t length, NBDExtent *extents,
+                                  unsigned nb_extents)
+{
+    uint64_t begin = offset, end;
+    uint64_t overall_end = offset + length;
+    unsigned i = 0;
+    BdrvDirtyBitmapIter *it;
+    bool dirty;
+
+    bdrv_dirty_bitmap_lock(bitmap);
+
+    it = bdrv_dirty_iter_new(bitmap);
+    dirty = bdrv_get_dirty_locked(NULL, bitmap, offset);
+
+    while (begin < overall_end && i < nb_extents) {
+        if (dirty) {
+            end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
+        } else {
+            bdrv_set_dirty_iter(it, begin);
+            end = bdrv_dirty_iter_next(it);
+        }
+        if (end == -1) {
+            end = overall_end;
+        }
+
+        i += add_extents(extents + i, nb_extents - i, end - begin,
+                         dirty ? NBD_STATE_DIRTY : 0);
+        begin = end;
+        dirty = !dirty;
+    }
+
+    bdrv_dirty_iter_free(it);
+
+    bdrv_dirty_bitmap_unlock(bitmap);
+
+    return i;
+}
+
+static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
+                              BdrvDirtyBitmap *bitmap, uint64_t offset,
+                              uint64_t length, bool dont_fragment,
+                              uint32_t context_id, Error **errp)
+{
+    int ret;
+    unsigned nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS;
+    NBDExtent *extents = g_new(NBDExtent, nb_extents);
+
+    nb_extents = bitmap_to_extents(bitmap, offset, length, extents, nb_extents);
+
+    ret = nbd_co_send_extents(client, handle, extents, nb_extents, context_id,
+                              errp);
+
+    g_free(extents);
+
+    return ret;
+}
+
 /* nbd_co_receive_request
  * Collect a client request. Return 0 if request looks valid, -EIO to drop
  * connection right away, and any other negative value to report an error to
@@ -2013,11 +2152,33 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
                                       "discard failed", errp);
 
     case NBD_CMD_BLOCK_STATUS:
-        if (client->export_meta.valid && client->export_meta.base_allocation) {
-            return nbd_co_send_block_status(client, request->handle,
-                                            blk_bs(exp->blk), request->from,
-                                            request->len,
-                                            NBD_META_ID_BASE_ALLOCATION, errp);
+        if (client->export_meta.valid &&
+            (client->export_meta.base_allocation ||
+             client->export_meta.dirty_bitmap))
+        {
+            if (client->export_meta.base_allocation) {
+                ret = nbd_co_send_block_status(client, request->handle,
+                                               blk_bs(exp->blk), request->from,
+                                               request->len,
+                                               NBD_META_ID_BASE_ALLOCATION,
+                                               errp);
+                if (ret < 0) {
+                    return ret;
+                }
+            }
+
+            if (client->export_meta.dirty_bitmap) {
+                ret = nbd_co_send_bitmap(client, request->handle,
+                                         client->exp->export_bitmap,
+                                         request->from, request->len,
+                                         request->flags & NBD_CMD_FLAG_DF,
+                                         NBD_META_ID_DIRTY_BITMAP, errp);
+                if (ret < 0) {
+                    return ret;
+                }
+            }
+
+            return nbd_co_send_structured_done(client, request->handle, errp);
         } else {
             return nbd_send_generic_reply(client, request->handle, -EINVAL,
                                           "CMD_BLOCK_STATUS not negotiated",
@@ -2169,3 +2330,44 @@ void nbd_client_new(NBDExport *exp,
     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_qmp_locked(bm)) {
+        error_setg(errp, "Bitmap '%s' is locked", 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);
+}
-- 
2.11.1

[Qemu-devel] [PATCH v2 3/3] qapi: new qmp command nbd-server-add-bitmap

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json | 23 +++++++++++++++++++++++
 blockdev-nbd.c  | 23 +++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/qapi/block.json b/qapi/block.json
index c694524002..cc0e607b5b 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -269,6 +269,29 @@
   'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
 
 ##
+# @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: 2.13
+##
+  { 'command': 'nbd-server-add-bitmap',
+    'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} }
+
+##
 # @nbd-server-stop:
 #
 # Stop QEMU's embedded NBD server, and unregister all devices previously
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 65a84739ed..6b0c50732c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -220,3 +220,26 @@ void qmp_nbd_server_stop(Error **errp)
     nbd_server_free(nbd_server);
     nbd_server = NULL;
 }
+
+void qmp_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.11.1

Re: [Qemu-devel] [PATCH v2 2/3] nbd/server: implement dirty bitmap export

[Eric Blake]

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

On 04/13/2018 01:14 PM, Vladimir Sementsov-Ogievskiy wrote:
> Handle new NBD meta namespace: "qemu", and corresponding queries:
> "qemu:dirty-bitmap:<export bitmap name>".
> 
> With new metadata context negotiated, BLOCK_STATUS query will reply
> with dirty-bitmap data, converted to extents. New public function
> nbd_export_bitmap selects bitmap to export. For now, only one bitmap
> may be exported.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/nbd.h |   6 ++
>  nbd/server.c        | 234 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 224 insertions(+), 16 deletions(-)
> 

>  
> +/* nbd_meta_bitmap_query
> + *
> + * Handle query to 'qemu-dirty-bitmap' namespace.

Stale comment

> + * 'len' is the amount of text remaining to be read from the current name, after
> + * the 'qemu-dirty-bitmap:' portion has been stripped.

and again

> + *
> + * Return -errno on I/O error, 0 if option was completely handled by
> + * sending a reply about inconsistent lengths, or 1 on success. */
> +static int nbd_meta_bitmap_query(NBDClient *client, NBDExportMetaContexts *meta,
> +                                 uint32_t len, Error **errp)
> +{
> +    if (!client->exp->export_bitmap) {
> +        return nbd_opt_skip(client, len, errp);
> +    }
> +
> +    return nbd_meta_single_query(
> +            client, client->exp->export_bitmap_context + strlen("qemu:"), len,
> +            &meta->dirty_bitmap, errp);

Wait, so client->exp->export_bitmap_context is stored WITH the "qemu:"
prefix as part of the name?

During _LIST, you are allowing a query of "qemu:" to list all
qemu-specific contexts (namely, qemu:dirty-bitmap:<export-name>); but
should we also make it work that if you do "qemu:dirty-bitmap:", you get
a list of just dirty-bitmap contexts even if "qemu:" has added other
contexts in the meantime?

> @@ -1800,6 +1851,94 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
>      return nbd_co_send_extents(client, handle, &extent, 1, context_id, errp);
>  }
>  
> +/* Set several extents, describing region of given @length with given @flags.
> + * Do not set more than @nb_extents, return number of set extents.
> + */
> +static unsigned add_extents(NBDExtent *extents, unsigned nb_extents,
> +                            uint64_t length, uint32_t flags)
> +{
> +    unsigned i = 0;
> +    uint32_t max_extent = QEMU_ALIGN_DOWN(INT32_MAX, 512);
> +    /* TODO: should be somehow in agreement with blocksize of OPT_GO,
> +     * and this should be documented in NBD proto. */

nbd.git commit 218c446 documented that descriptor lengths MUST be an
integer multiple of minimum block size; as for maximum, I see no reason
why BLOCK_STATUS can't report larger extents than what NBD_CMD_READ can
do.  Do we need further doc tweaks to nbd.git?

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


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

Re: [Qemu-devel] [PATCH v2 2/3] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

18.04.2018 17:05, Eric Blake wrote:
> On 04/13/2018 01:14 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Handle new NBD meta namespace: "qemu", and corresponding queries:
>> "qemu:dirty-bitmap:<export bitmap name>".
>>
>> With new metadata context negotiated, BLOCK_STATUS query will reply
>> with dirty-bitmap data, converted to extents. New public function
>> nbd_export_bitmap selects bitmap to export. For now, only one bitmap
>> may be exported.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/nbd.h |   6 ++
>>   nbd/server.c        | 234 ++++++++++++++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 224 insertions(+), 16 deletions(-)
>>
>>   
>> +/* nbd_meta_bitmap_query
>> + *
>> + * Handle query to 'qemu-dirty-bitmap' namespace.
> Stale comment
>
>> + * 'len' is the amount of text remaining to be read from the current name, after
>> + * the 'qemu-dirty-bitmap:' portion has been stripped.
> and again
>
>> + *
>> + * Return -errno on I/O error, 0 if option was completely handled by
>> + * sending a reply about inconsistent lengths, or 1 on success. */
>> +static int nbd_meta_bitmap_query(NBDClient *client, NBDExportMetaContexts *meta,
>> +                                 uint32_t len, Error **errp)
>> +{
>> +    if (!client->exp->export_bitmap) {
>> +        return nbd_opt_skip(client, len, errp);
>> +    }
>> +
>> +    return nbd_meta_single_query(
>> +            client, client->exp->export_bitmap_context + strlen("qemu:"), len,
>> +            &meta->dirty_bitmap, errp);
> Wait, so client->exp->export_bitmap_context is stored WITH the "qemu:"
> prefix as part of the name?
>
> During _LIST, you are allowing a query of "qemu:" to list all
> qemu-specific contexts (namely, qemu:dirty-bitmap:<export-name>); but
> should we also make it work that if you do "qemu:dirty-bitmap:", you get
> a list of just dirty-bitmap contexts even if "qemu:" has added other
> contexts in the meantime?

reasonable.

>
>> @@ -1800,6 +1851,94 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
>>       return nbd_co_send_extents(client, handle, &extent, 1, context_id, errp);
>>   }
>>   
>> +/* Set several extents, describing region of given @length with given @flags.
>> + * Do not set more than @nb_extents, return number of set extents.
>> + */
>> +static unsigned add_extents(NBDExtent *extents, unsigned nb_extents,
>> +                            uint64_t length, uint32_t flags)
>> +{
>> +    unsigned i = 0;
>> +    uint32_t max_extent = QEMU_ALIGN_DOWN(INT32_MAX, 512);
>> +    /* TODO: should be somehow in agreement with blocksize of OPT_GO,
>> +     * and this should be documented in NBD proto. */
> nbd.git commit 218c446 documented that descriptor lengths MUST be an
> integer multiple of minimum block size; as for maximum, I see no reason
> why BLOCK_STATUS can't report larger extents than what NBD_CMD_READ can
> do.  Do we need further doc tweaks to nbd.git?
>

ok, missed it.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/3] NBD export bitmaps

[Eric Blake]

On 04/13/2018 01:13 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all.
> 
> This is a proposal and realization of new NBD meta context:
> qemu. (I hope to send corresponding proposal to NBD protocol soon)
> 
> New possible queries will look like:
> qemu:dirty-bitmap:<export-bitmap-name>
> 

> 
>   qapi/block.json     |  23 +++++
>   include/block/nbd.h |   6 ++
>   blockdev-nbd.c      |  23 +++++
>   nbd/server.c        | 274 ++++++++++++++++++++++++++++++++++++++++++++++------
>   4 files changed, 297 insertions(+), 29 deletions(-)

We need something in docs/interop/ that documents the qemu: namespace 
within NBD, so that the upstream NBD spec can point to our document as a 
reference for other clients to be aware of what semantics we are exporting.

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