[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 13/14] nbd/server: Prepare for per-request filtering of BL
|
From: |
Vladimir Sementsov-Ogievskiy |
|
Subject: |
Re: [PATCH v3 13/14] nbd/server: Prepare for per-request filtering of BLOCK_STATUS |
|
Date: |
Thu, 1 Jun 2023 12:57:44 +0300 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 |
On 15.05.23 22:53, Eric Blake wrote:
The next commit will add support for the new addition of
NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can
request that the server only return a subset of negotiated contexts,
rather than all contexts. To make that task easier, this patch
populates the list of contexts to return on a per-command basis (for
now, identical to the full set of negotiated contexts).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/block/nbd.h | 15 ++++++
nbd/server.c | 108 +++++++++++++++++++++++---------------------
2 files changed, 72 insertions(+), 51 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 865bb4ee2e1..6696d61bd59 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -60,6 +60,20 @@ typedef enum NBDHeaderStyle {
NBD_HEADER_EXTENDED, /* NBD_OPT_EXTENDED_HEADERS negotiated */
} NBDHeaderStyle;
+/*
+ * NBDMetaContexts represents a list of meta contexts in use, as
+ * selected by NBD_OPT_SET_META_CONTEXT. Also used for
+ * NBD_OPT_LIST_META_CONTEXT, and payload filtering in
+ * NBD_CMD_BLOCK_STATUS.
+ */
+typedef struct NBDMetaContexts {
+ size_t count; /* number of negotiated contexts */
+ bool base_allocation; /* export base:allocation context (block status) */
+ bool allocation_depth; /* export qemu:allocation-depth */
+ size_t nr_bitmaps; /* Length of bitmaps array */
+ bool *bitmaps; /* export qemu:dirty-bitmap:<export bitmap name> */
+} NBDMetaContexts;
+
/*
* Note: NBDRequest is _NOT_ the same as the network representation of an NBD
* request!
@@ -70,6 +84,7 @@ typedef struct NBDRequest {
uint64_t len; /* Effect length; 32 bit limit without extended headers */
uint16_t flags; /* NBD_CMD_FLAG_* */
uint16_t type; /* NBD_CMD_* */
+ NBDMetaContexts contexts; /* Used by NBD_CMD_BLOCK_STATUS */
} NBDRequest;
typedef struct NBDSimpleReply {
diff --git a/nbd/server.c b/nbd/server.c
index 6475a76c1f0..db550c82cd2 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -105,20 +105,6 @@ struct NBDExport {
static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
-/* NBDExportMetaContexts represents a list of contexts to be exported,
- * as selected by NBD_OPT_SET_META_CONTEXT. Also used for
- * NBD_OPT_LIST_META_CONTEXT. */
-typedef struct NBDExportMetaContexts {
- NBDExport *exp;
- size_t count; /* number of negotiated contexts */
- bool base_allocation; /* export base:allocation context (block status) */
- bool allocation_depth; /* export qemu:allocation-depth */
- bool *bitmaps; /*
- * export qemu:dirty-bitmap:<export bitmap name>,
- * sized by exp->nr_export_bitmaps
- */
-} NBDExportMetaContexts;
-
struct NBDClient {
int refcount;
void (*close_fn)(NBDClient *client, bool negotiated);
@@ -144,7 +130,8 @@ struct NBDClient {
uint32_t check_align; /* If non-zero, check for aligned client requests */
NBDHeaderStyle header_style;
- NBDExportMetaContexts export_meta;
+ NBDExport *context_exp; /* export of last OPT_SET_META_CONTEXT */
+ NBDMetaContexts contexts; /* Negotiated meta contexts */
uint32_t opt; /* Current option being negotiated */
uint32_t optlen; /* remaining length of data in ioc for the option being
@@ -457,8 +444,8 @@ static int nbd_negotiate_handle_list(NBDClient *client,
Error **errp)
static void nbd_check_meta_export(NBDClient *client)
{
- if (client->exp != client->export_meta.exp) {
- client->export_meta.count = 0;
+ if (client->exp != client->context_exp) {
+ client->contexts.count = 0;
}
}
@@ -852,7 +839,7 @@ static bool nbd_strshift(const char **str, const char
*prefix)
* Handle queries to 'base' namespace. For now, only the base:allocation
* context is available. Return true if @query has been handled.
*/
-static bool nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
+static bool nbd_meta_base_query(NBDClient *client, NBDMetaContexts *meta,
const char *query)
{
if (!nbd_strshift(&query, "base:")) {
@@ -872,8 +859,8 @@ static bool nbd_meta_base_query(NBDClient *client,
NBDExportMetaContexts *meta,
* and qemu:allocation-depth contexts are available. Return true if @query
* has been handled.
*/
-static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
- const char *query)
+static bool nbd_meta_qemu_query(NBDClient *client, NBDExport *exp,
+ NBDMetaContexts *meta, const char *query)
{
size_t i;
@@ -884,9 +871,9 @@ static bool nbd_meta_qemu_query(NBDClient *client,
NBDExportMetaContexts *meta,
if (!*query) {
if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
- meta->allocation_depth = meta->exp->allocation_depth;
- if (meta->exp->nr_export_bitmaps) {
- memset(meta->bitmaps, 1, meta->exp->nr_export_bitmaps);
+ meta->allocation_depth = exp->allocation_depth;
+ if (meta->nr_bitmaps) {
+ memset(meta->bitmaps, 1, meta->nr_bitmaps);
}
}
trace_nbd_negotiate_meta_query_parse("empty");
@@ -895,7 +882,7 @@ static bool nbd_meta_qemu_query(NBDClient *client,
NBDExportMetaContexts *meta,
if (strcmp(query, "allocation-depth") == 0) {
trace_nbd_negotiate_meta_query_parse("allocation-depth");
- meta->allocation_depth = meta->exp->allocation_depth;
+ meta->allocation_depth = exp->allocation_depth;
return true;
}
@@ -903,17 +890,17 @@ static bool nbd_meta_qemu_query(NBDClient *client,
NBDExportMetaContexts *meta,
trace_nbd_negotiate_meta_query_parse("dirty-bitmap:");
if (!*query) {
if (client->opt == NBD_OPT_LIST_META_CONTEXT &&
- meta->exp->nr_export_bitmaps) {
- memset(meta->bitmaps, 1, meta->exp->nr_export_bitmaps);
+ exp->nr_export_bitmaps) {
+ memset(meta->bitmaps, 1, exp->nr_export_bitmaps);
sometimes you change s/meta->exp->nr_export_bitmaps/meta->nr_bitmaps/ and sometimes
s/meta->exp->nr_export_bitmaps/exp->nr_export_bitmaps/
}
trace_nbd_negotiate_meta_query_parse("empty");
return true;
}
- for (i = 0; i < meta->exp->nr_export_bitmaps; i++) {
+ for (i = 0; i < meta->nr_bitmaps; i++) {
const char *bm_name;
- bm_name = bdrv_dirty_bitmap_name(meta->exp->export_bitmaps[i]);
+ bm_name = bdrv_dirty_bitmap_name(exp->export_bitmaps[i]);
I don't like the change: we iterate through exp->export_bitmaps. So seems more
correct to use exp->nr_export_bitmaps as a limit.
I understand why you want to duplicated nr_export_bitmaps in NBDMetaContexts, but
the result doesn't seem better than just use exp->nr_export_bitmaps.
Probably, if we want to duplicate nr_export_bitmaps, we should duplicate
export_bitmaps as well. But this leads actually to keeping meta->exp as is.
Hm, may be just do
size_t nr_bitmaps = meta->nr_bitmaps;
assert(nr_bitmaps == exp->nr_export_bitmaps)
at top of the function? and use nr_bitmaps everywhere.
But this again make me doubt that meta.nr_bitmaps is good idea: we have to pass
exp together and check meta.nr_bitmaps == exp->nr_export_bitmaps every time.
So, I understand, that
bool *bitmaps; /*
* export qemu:dirty-bitmap:<export bitmap name>,
* sized by exp->nr_export_bitmaps
*/
sounds like not good design - we have an array, sized by field of another object.
But, we have a link that that object (meta->exp).
But with your way we fall into bad design too: we have a duplicated field, and
have to control that they not diverse. And we iterate things in exp, using
field of meta.
I looked through, seems in all places where we use nr_bitmaps, we also have
access to nr_export_bitmaps. So, maybe, drop this nr_bitmaps?
And why not to keep meta.exp? With it, meta is self-sufficient object, which
fully describe some selected contexts.
For per-request filtering, meta.exp just must always point to current export.
And anyway would be good to split refactoring of meta structure (if we need it)
and new logic of per-command filtering.
if (strcmp(bm_name, query) == 0) {
meta->bitmaps[i] = true;
trace_nbd_negotiate_meta_query_parse(query);
@@ -937,8 +924,8 @@ static bool nbd_meta_qemu_query(NBDClient *client,
NBDExportMetaContexts *meta,
*
* 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_negotiate_meta_query(NBDClient *client,
- NBDExportMetaContexts *meta, Error **errp)
+static int nbd_negotiate_meta_query(NBDClient *client, NBDExport *exp,
+ NBDMetaContexts *meta, Error **errp)
{
int ret;
g_autofree char *query = NULL;
@@ -965,7 +952,7 @@ static int nbd_negotiate_meta_query(NBDClient *client,
if (nbd_meta_base_query(client, meta, query)) {
return 1;
}
- if (nbd_meta_qemu_query(client, meta, query)) {
+ if (nbd_meta_qemu_query(client, exp, meta, query)) {
return 1;
}
@@ -977,14 +964,15 @@ static int nbd_negotiate_meta_query(NBDClient *client,
* Handle NBD_OPT_LIST_META_CONTEXT and NBD_OPT_SET_META_CONTEXT
*
* Return -errno on I/O error, or 0 if option was completely handled. */
-static int nbd_negotiate_meta_queries(NBDClient *client,
- NBDExportMetaContexts *meta, Error
**errp)
+static int nbd_negotiate_meta_queries(NBDClient *client, Error **errp)
{
int ret;
g_autofree char *export_name = NULL;
/* Mark unused to work around https://bugs.llvm.org/show_bug.cgi?id=3888
*/
g_autofree G_GNUC_UNUSED bool *bitmaps = NULL;
- NBDExportMetaContexts local_meta = {0};
+ NBDMetaContexts local_meta = {0};
+ NBDMetaContexts *meta;
+ NBDExport *exp;
uint32_t nb_queries;
size_t i;
size_t count = 0;
@@ -1000,6 +988,9 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
/* Only change the caller's meta on SET. */
meta = &local_meta;
+ } else {
+ meta = &client->contexts;
+ client->context_exp = NULL;
}
g_free(meta->bitmaps);
@@ -1010,14 +1001,15 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
return ret;
}
- meta->exp = nbd_export_find(export_name);
- if (meta->exp == NULL) {
+ exp = nbd_export_find(export_name);
+ if (exp == NULL) {
g_autofree char *sane_name = nbd_sanitize_name(export_name);
return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
"export '%s' not present", sane_name);
}
- meta->bitmaps = g_new0(bool, meta->exp->nr_export_bitmaps);
+ meta->nr_bitmaps = exp->nr_export_bitmaps;
+ meta->bitmaps = g_new0(bool, exp->nr_export_bitmaps);
if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
bitmaps = meta->bitmaps;
}
@@ -1033,13 +1025,13 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
if (client->opt == NBD_OPT_LIST_META_CONTEXT && !nb_queries) {
/* enable all known contexts */
meta->base_allocation = true;
- meta->allocation_depth = meta->exp->allocation_depth;
- if (meta->exp->nr_export_bitmaps) {
- memset(meta->bitmaps, 1, meta->exp->nr_export_bitmaps);
+ meta->allocation_depth = exp->allocation_depth;
+ if (exp->nr_export_bitmaps) {
+ memset(meta->bitmaps, 1, meta->nr_bitmaps);
}
} else {
for (i = 0; i < nb_queries; ++i) {
- ret = nbd_negotiate_meta_query(client, meta, errp);
+ ret = nbd_negotiate_meta_query(client, exp, meta, errp);
if (ret <= 0) {
return ret;
}
@@ -1066,7 +1058,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
count++;
}
- for (i = 0; i < meta->exp->nr_export_bitmaps; i++) {
+ for (i = 0; i < meta->nr_bitmaps; i++) {
const char *bm_name;
g_autofree char *context = NULL;
@@ -1074,7 +1066,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
continue;
}
- bm_name = bdrv_dirty_bitmap_name(meta->exp->export_bitmaps[i]);
+ bm_name = bdrv_dirty_bitmap_name(exp->export_bitmaps[i]);
context = g_strdup_printf("qemu:dirty-bitmap:%s", bm_name);
ret = nbd_negotiate_send_meta_context(client, context,
@@ -1089,6 +1081,9 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
if (ret == 0) {
meta->count = count;
+ if (client->opt == NBD_OPT_SET_META_CONTEXT) {
+ client->context_exp = exp;
+ }
}
return ret;
@@ -1282,8 +1277,7 @@ static int nbd_negotiate_options(NBDClient *client, Error
**errp)
case NBD_OPT_LIST_META_CONTEXT:
case NBD_OPT_SET_META_CONTEXT:
- ret = nbd_negotiate_meta_queries(client, &client->export_meta,
- errp);
+ ret = nbd_negotiate_meta_queries(client, errp);
break;
case NBD_OPT_EXTENDED_HEADERS:
@@ -1514,7 +1508,7 @@ void nbd_client_put(NBDClient *client)
QTAILQ_REMOVE(&client->exp->clients, client, next);
blk_exp_unref(&client->exp->common);
}
- g_free(client->export_meta.bitmaps);
+ g_free(client->contexts.bitmaps);
g_free(client);
}
}
@@ -2489,6 +2483,8 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req, NBDRequest *
return -ENOMEM;
}
}
+ } else if (request->type == NBD_CMD_BLOCK_STATUS) {
+ request->contexts = client->contexts;
}
if (payload_len) {
@@ -2715,11 +2711,11 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
}
assert(client->header_style >= NBD_HEADER_EXTENDED ||
request->len <= UINT32_MAX);
- if (client->export_meta.count) {
+ if (request->contexts.count) {
bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
- int contexts_remaining = client->export_meta.count;
+ int contexts_remaining = request->contexts.count;
- if (client->export_meta.base_allocation) {
+ if (request->contexts.base_allocation) {
ret = nbd_co_send_block_status(client, request,
exp->common.blk,
request->from,
@@ -2732,7 +2728,7 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
}
}
- if (client->export_meta.allocation_depth) {
+ if (request->contexts.allocation_depth) {
ret = nbd_co_send_block_status(client, request,
exp->common.blk,
request->from, request->len,
@@ -2745,8 +2741,10 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
}
}
+ assert(request->contexts.nr_bitmaps ==
+ client->exp->nr_export_bitmaps);
for (i = 0; i < client->exp->nr_export_bitmaps; i++) {
- if (!client->export_meta.bitmaps[i]) {
+ if (!request->contexts.bitmaps[i]) {
continue;
}
ret = nbd_co_send_bitmap(client, request,
@@ -2762,6 +2760,10 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
assert(!contexts_remaining);
return 0;
+ } else if (client->contexts.count) {
+ return nbd_send_generic_reply(client, request, -EINVAL,
+ "CMD_BLOCK_STATUS payload not valid",
+ errp);
} else {
return nbd_send_generic_reply(client, request, -EINVAL,
"CMD_BLOCK_STATUS not negotiated",
@@ -2840,6 +2842,10 @@ static coroutine_fn void nbd_trip(void *opaque)
} else {
ret = nbd_handle_request(client, &request, req->data, &local_err);
}
+ if (request.type == NBD_CMD_BLOCK_STATUS &&
+ request.contexts.bitmaps != client->contexts.bitmaps) {
+ g_free(request.contexts.bitmaps);
+ }
if (ret < 0) {
error_prepend(&local_err, "Failed to send reply: ");
goto disconnect;
--
Best regards,
Vladimir
| [Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v3 13/14] nbd/server: Prepare for per-request filtering of BLOCK_STATUS,
Vladimir Sementsov-Ogievskiy <=