qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 3/4] nbd/server: implement dirty bitmap export
Date: Wed, 21 Mar 2018 11:57:57 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Rather sparse on the details in the commit message; I had to read the patch to even learn what the new namespace is.

---
  include/block/nbd.h |   2 +
  nbd/server.c        | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++--
  2 files changed, 203 insertions(+), 6 deletions(-)


+++ 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
+
+#define NBD_MAX_BITMAP_EXTENTS (0x100000 / 8) /* 1 mb of extents data */
+#define MAX_EXTENT_LENGTH QEMU_ALIGN_DOWN(INT32_MAX, 512)

Worth comments on these two limits?

+
+#define NBD_STATE_DIRTY 1

Does this belong better in nbd.h, alongside NBD_STATE_HOLE? (And defining it as (1 << 0) might be nicer, too).

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_name;
  };
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> */

That's a LONG namespace name, and it does not lend itself well to other qemu extensions; so future qemu BLOCK_STATUS additions would require consuming yet another namespace and additional upstream NBD registration. Wouldn't it be better to have the namespace be 'qemu:' (which we register with upstream NBD just once), where we are then free to document leafnames to look like 'dirty-bitmap:name' or 'foo:bar'?

  } NBDExportMetaContexts;
struct NBDClient {
@@ -786,12 +796,32 @@ 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_name, len,
+                                 &meta->dirty_bitmap, errp);

So if I'm reading this right, a client can do _LIST_ 'qemu-dirty-bitmap:' (or 'qemu:dirty-bitmap:' if we choose a shorter initial namespace) and get back the exported bitmap name; or the user already knows the bitmap name and both _LIST_ and _SET_ 'qemu-dirty-bitmap:name' return the one supported bitmap for that NBD server.


  /* 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,

Worth a bool flag that the caller can inform this function whether to include FLAG_DONE?
+
+static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
+                              BdrvDirtyBitmap *bitmap, uint64_t offset,
+                              uint64_t length, uint32_t context_id,
+                              Error **errp)
+{
+    int ret;
+    unsigned nb_extents;
+    NBDExtent *extents = g_new(NBDExtent, NBD_MAX_BITMAP_EXTENTS);

Is this correct even when the client used NBD_CMD_FLAG_REQ_ONE?

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]