[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 4/5] nbd: Add new qemu:allocation-depth metacontext
|
From: |
Vladimir Sementsov-Ogievskiy |
|
Subject: |
Re: [PATCH v2 4/5] nbd: Add new qemu:allocation-depth metacontext |
|
Date: |
Thu, 8 Oct 2020 16:04:16 +0300 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.2 |
08.10.2020 00:54, Eric Blake wrote:
On 10/7/20 8:40 AM, Vladimir Sementsov-Ogievskiy wrote:
30.09.2020 15:11, Eric Blake wrote:
'qemu-img map' provides a way to determine which extents of an image
come from the top layer vs. inherited from a backing chain. This is
useful information worth exposing over NBD. There is a proposal to
add a QMP command block-dirty-bitmap-populate which can create a dirty
bitmap that reflects allocation information, at which point
qemu:dirty-bitmap:NAME can expose that information via the creation of
a temporary bitmap, but we can shorten the effort by adding a new
qemu:allocation-depth context that does the same thing without an
intermediate bitmap (this patch does not eliminate the need for that
proposal, as it will have other uses as well).
For this patch, I just encoded a tri-state value (unallocated, from
this layer, from any of the backing layers); an obvious extension
would be to provide the actual depth in bits 31-4 while keeping bits
1-0 as a tri-state (leaving bits 3-2 unused, for ease of reading depth
from a hex number). But this extension would require
bdrv_is_allocated_above to return a depth number.
Note that this patch does not actually enable any way to request a
server to enable this context; that will come in the next patch.
+In the allocation depth context, bits 0 and 1 form a tri-state value:
+
+ bits 0-1 clear: NBD_STATE_DEPTH_UNALLOC, means the extent is
unallocated
+ bit 0 set: NBD_STATE_DEPTH_LOCAL, the extent is allocated in this
image
Maybe, s/this/the top level/, as, what is "this" for NBD client?
Sure.
@@ -864,12 +867,21 @@ static bool nbd_meta_qemu_query(NBDClient
*client, NBDExportMetaContexts *meta,
if (!*query) {
We get here for "qemu:".
if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
+ meta->allocation_depth = meta->exp->alloc_context;
meta->bitmap = !!meta->exp->export_bitmap;
}
trace_nbd_negotiate_meta_query_parse("empty");
return true;
}
+ if (nbd_strshift(&query, "allocation-depth")) {
We get here for "qemu:allocation-depth", and as you pointed out,
"qemu:allocation-depth-extended".
+ trace_nbd_negotiate_meta_query_parse("allocation-depth");
+ if (nbd_meta_empty_or_pattern(client, "", query)) {
How much it differs from "if (!*query) {", I don't see...
The nbd_meta_empty_or_pattern returns false for
"qemu:allocation-depth-extended"; it only returns true for
"qemu:allocation-depth". But, as you pointed out,
+ meta->allocation_depth = meta->exp->alloc_context;
+ }
+ return true;
+ }
why not just
if (!strcmp(query, "allocation-depth")) {
meta->allocation_depth = meta->exp->alloc_context;
return true;
}
?
that does seem shorter.
With your code you also parse something like
"allocation-depth-extended".. Is it on purpose?
The string is parsed, but does not affect meta->XXX, which means nothing
gets advertised to the client. The trace messages might differ, but
behavior is correct.
+
if (nbd_strshift(&query, "dirty-bitmap:")) {
trace_nbd_negotiate_meta_query_parse("dirty-bitmap:");
if (!meta->exp->export_bitmap) {
Also, "trace_nbd_negotiate_meta_query_skip("not dirty-bitmap"); " at
function end should
now be something like trace_nbd_negotiate_meta_query_skip("unknown
context in qemu: namespace");
Good idea.
+/* Get allocation depth from the exported device and send it to the
client */
+static int nbd_co_send_block_alloc(NBDClient *client, uint64_t handle,
+ BlockDriverState *bs, uint64_t
offset,
+ uint32_t length, bool dont_fragment,
+ bool last, uint32_t context_id,
+ Error **errp)
+{
+ int ret;
+ unsigned int nb_extents = dont_fragment ? 1 :
NBD_MAX_BLOCK_STATUS_EXTENTS;
+ g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
+
+ ret = blockalloc_to_extents(bs, offset, length, ea);
+ if (ret < 0) {
+ return nbd_co_send_structured_error(
+ client, handle, -ret, "can't get block status", errp);
+ }
+
+ return nbd_co_send_extents(client, handle, ea, last, context_id,
errp);
+}
The function is a copy of nbd_co_send_block_status()..
Probably, we can reuse nbd_co_send_block_status(), and just call
blockstatus_to_extents()
or blockalloc_to_extents() depending on context_id parameter.
Nice idea to reduce the duplication.
Still, I'm OK with it as is.
it was only about the duplicated function
So is that Reviewed-by:, or do I need to post v3 with my tweaks in
response to your comments?
Honestly it wasn't.. So, I found some things to discuss, and it's possible that
I've looked through with less attention. Hm. So, with v3 or without, I've have
to lookthrough it again..
My suggested diff:
diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index 56efec7aee..a0d91ff08b 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -36,7 +36,8 @@ the image, with a single context named:
In the allocation depth context, bits 0 and 1 form a tri-state value:
bits 0-1 clear: NBD_STATE_DEPTH_UNALLOC, means the extent is unallocated
- bit 0 set: NBD_STATE_DEPTH_LOCAL, the extent is allocated in this image
+ bit 0 set: NBD_STATE_DEPTH_LOCAL, the extent is allocated in the top
+ level image
bit 1 set: NBD_STATE_DEPTH_BACKING, the extent is inherited from a
backing layer
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 06208bc250..2d7165960d 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -262,7 +262,7 @@ enum {
/* Extent flags for qemu:allocation-depth in NBD_REPLY_TYPE_BLOCK_STATUS */
#define NBD_STATE_DEPTH_UNALLOC (0 << 0)
#define NBD_STATE_DEPTH_LOCAL (1 << 0)
-#define NBD_STATE_DEPTH_BACKING (2 << 0)
+#define NBD_STATE_DEPTH_BACKING (1 << 1)
#define NBD_STATE_DEPTH_MASK (0x3)
static inline bool nbd_reply_type_is_error(int type)
diff --git a/nbd/server.c b/nbd/server.c
index 830b21000b..3761ac180f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -809,7 +809,7 @@ static bool nbd_meta_empty_or_pattern(NBDClient *client,
const char *pattern,
{
if (!*query) {
trace_nbd_negotiate_meta_query_parse("empty");
- return !*pattern || client->opt == NBD_OPT_LIST_META_CONTEXT;
+ return client->opt == NBD_OPT_LIST_META_CONTEXT;
}
if (strcmp(query, pattern) == 0) {
trace_nbd_negotiate_meta_query_parse(pattern);
@@ -874,11 +874,9 @@ static bool nbd_meta_qemu_query(NBDClient *client,
NBDExportMetaContexts *meta,
return true;
}
- if (nbd_strshift(&query, "allocation-depth")) {
+ if (!strcmp(query, "allocation-depth")) {
trace_nbd_negotiate_meta_query_parse("allocation-depth");
- if (nbd_meta_empty_or_pattern(client, "", query)) {
- meta->allocation_depth = meta->exp->alloc_context;
- }
+ meta->allocation_depth = meta->exp->alloc_context;
return true;
}
@@ -896,7 +894,7 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
return true;
}
- trace_nbd_negotiate_meta_query_skip("not dirty-bitmap");
+ trace_nbd_negotiate_meta_query_skip("unknown context in qemu: namespace");
return true;
}
@@ -2045,7 +2043,11 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
return nbd_co_send_iov(client, iov, 2, errp);
}
-/* Get block status from the exported device and send it to the client */
+/*
+ * Get block status from the exported device and send it to the client,
+ * handling base:allocation or qemu:allocation-depth meta-context, based
+ * on @context_id value.
+ */
static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
BlockDriverState *bs, uint64_t offset,
uint32_t length, bool dont_fragment,
@@ -2056,27 +2058,12 @@ static int nbd_co_send_block_status(NBDClient *client,
uint64_t handle,
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
- ret = blockstatus_to_extents(bs, offset, length, ea);
- if (ret < 0) {
- return nbd_co_send_structured_error(
- client, handle, -ret, "can't get block status", errp);
+ if (context_id == NBD_META_ID_BASE_ALLOCATION) {
+ ret = blockstatus_to_extents(bs, offset, length, ea);
+ } else {
+ assert(context_id == NBD_META_ID_ALLOCATION_DEPTH);
+ ret = blockalloc_to_extents(bs, offset, length, ea);
}
-
- return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
-}
-
-/* Get allocation depth from the exported device and send it to the client */
-static int nbd_co_send_block_alloc(NBDClient *client, uint64_t handle,
- BlockDriverState *bs, uint64_t offset,
- uint32_t length, bool dont_fragment,
- bool last, uint32_t context_id,
- Error **errp)
-{
- int ret;
- unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
- g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
-
- ret = blockalloc_to_extents(bs, offset, length, ea);
if (ret < 0) {
return nbd_co_send_structured_error(
client, handle, -ret, "can't get block status", errp);
@@ -2433,13 +2420,13 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
}
if (client->export_meta.allocation_depth) {
- ret = nbd_co_send_block_alloc(client, request->handle,
- blk_bs(exp->common.blk),
- request->from, request->len,
- dont_fragment,
- !--contexts_remaining,
- NBD_META_ID_ALLOCATION_DEPTH,
- errp);
+ ret = nbd_co_send_block_status(client, request->handle,
+ blk_bs(exp->common.blk),
+ request->from, request->len,
+ dont_fragment,
+ !--contexts_remaining,
+ NBD_META_ID_ALLOCATION_DEPTH,
+ errp);
if (ret < 0) {
return ret;
}
with it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir