Re: [Libguestfs] [PATCH v4 24/24] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS
On Tue, Jun 27, 2023 at 10:42:20PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.06.23 16:56, Eric Blake wrote:
> > Allow a client to request a subset of negotiated meta contexts. For
> > example, a client may ask to use a single connection to learn about
> > both block status and dirty bitmaps, but where the dirty bitmap
> > queries only need to be performed on a subset of the disk; forcing the
> > server to compute that information on block status queries in the rest
> > of the disk is wasted effort (both at the server, and on the amount of
> > traffic sent over the wire to be parsed and ignored by the client).
> >
> > Qemu as an NBD client never requests to use more than one meta
> > context, so it has no need to use block status payloads. Testing this
> > instead requires support from libnbd, which CAN access multiple meta
> > contexts in parallel from a single NBD connection; an interop test
> > submitted to the libnbd project at the same time as this patch
> > demonstrates the feature working, as well as testing some corner cases
> > (for example, when the payload length is longer than the export
> > length), although other corner cases (like passing the same id
> > duplicated) requires a protocol fuzzer because libnbd is not wired up
> > to break the protocol that badly.
> >
> > This also includes tweaks to 'qemu-nbd --list' to show when a server
> > is advertising the capability, and to the testsuite to reflect the
> > addition to that output.
> >
> > Signed-off-by: Eric Blake
> > ---
>
> [..]
>
> > +static int
> > +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> > + Error **errp)
> > +{
> > +int payload_len = request->len;
> > +g_autofree char *buf = NULL;
> > +size_t count, i, nr_bitmaps;
> > +uint32_t id;
> > +
> > +assert(request->len <= NBD_MAX_BUFFER_SIZE);
> > +assert(client->contexts.exp == client->exp);
> > +nr_bitmaps = client->exp->nr_export_bitmaps;
> > +request->contexts = g_new0(NBDMetaContexts, 1);
> > +request->contexts->exp = client->exp;
> > +
> > +if (payload_len % sizeof(uint32_t) ||
> > +payload_len < sizeof(NBDBlockStatusPayload) ||
> > +payload_len > (sizeof(NBDBlockStatusPayload) +
> > + sizeof(id) * client->contexts.count)) {
> > +goto skip;
> > +}
> > @@ -2470,7 +2552,13 @@ static int coroutine_fn
> > nbd_co_receive_request(NBDRequestData *req, NBDRequest *
>
> [..]
>
> > @@ -2712,7 +2804,8 @@ static coroutine_fn int nbd_handle_request(NBDClient
> > *client,
> > "discard failed", errp);
> >
> > case NBD_CMD_BLOCK_STATUS:
> > -if (!request->len) {
> > +assert(request->contexts);
> > +if (!request->len && !(request->flags & NBD_CMD_FLAG_PAYLOAD_LEN))
> > {
>
> why not error-out for len=0 in case of payload?
Because nbd_co_block_status_payload_read() already rejected that case.
But an assertion would not hurt to make it obvious.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v4 24/24] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS
On 08.06.23 16:56, Eric Blake wrote:
Allow a client to request a subset of negotiated meta contexts. For
example, a client may ask to use a single connection to learn about
both block status and dirty bitmaps, but where the dirty bitmap
queries only need to be performed on a subset of the disk; forcing the
server to compute that information on block status queries in the rest
of the disk is wasted effort (both at the server, and on the amount of
traffic sent over the wire to be parsed and ignored by the client).
Qemu as an NBD client never requests to use more than one meta
context, so it has no need to use block status payloads. Testing this
instead requires support from libnbd, which CAN access multiple meta
contexts in parallel from a single NBD connection; an interop test
submitted to the libnbd project at the same time as this patch
demonstrates the feature working, as well as testing some corner cases
(for example, when the payload length is longer than the export
length), although other corner cases (like passing the same id
duplicated) requires a protocol fuzzer because libnbd is not wired up
to break the protocol that badly.
This also includes tweaks to 'qemu-nbd --list' to show when a server
is advertising the capability, and to the testsuite to reflect the
addition to that output.
Signed-off-by: Eric Blake
---
[..]
+static int
+nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
+ Error **errp)
+{
+int payload_len = request->len;
+g_autofree char *buf = NULL;
+size_t count, i, nr_bitmaps;
+uint32_t id;
+
+assert(request->len <= NBD_MAX_BUFFER_SIZE);
+assert(client->contexts.exp == client->exp);
+nr_bitmaps = client->exp->nr_export_bitmaps;
+request->contexts = g_new0(NBDMetaContexts, 1);
+request->contexts->exp = client->exp;
+
+if (payload_len % sizeof(uint32_t) ||
+payload_len < sizeof(NBDBlockStatusPayload) ||
+payload_len > (sizeof(NBDBlockStatusPayload) +
+ sizeof(id) * client->contexts.count)) {
+goto skip;
+}
+
+buf = g_malloc(payload_len);
+if (nbd_read(client->ioc, buf, payload_len,
+ "CMD_BLOCK_STATUS data", errp) < 0) {
+return -EIO;
+}
+trace_nbd_co_receive_request_payload_received(request->cookie,
+ payload_len);
+request->contexts->bitmaps = g_new0(bool, nr_bitmaps);
+count = (payload_len - sizeof(NBDBlockStatusPayload)) / sizeof(id);
+payload_len = 0;
+
+for (i = 0; i < count; i++) {
+id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) + sizeof(id) * i);
+if (id == NBD_META_ID_BASE_ALLOCATION) {
+if (request->contexts->base_allocation) {
+goto skip;
+}
+request->contexts->base_allocation = true;
+} else if (id == NBD_META_ID_ALLOCATION_DEPTH) {
+if (request->contexts->allocation_depth) {
+goto skip;
+}
+request->contexts->allocation_depth = true;
+} else {
maybe,
int ind = id - NBD_META_ID_DIRTY_BITMAP;
+if (id - NBD_META_ID_DIRTY_BITMAP > nr_bitmaps ||
= ?
+request->contexts->bitmaps[id - NBD_META_ID_DIRTY_BITMAP]) {
+goto skip;
+}
+request->contexts->bitmaps[id - NBD_META_ID_DIRTY_BITMAP] = true;
+}
+}
+
+request->len = ldq_be_p(buf);
+request->contexts->count = count;
+return 0;
+
+ skip:
+trace_nbd_co_receive_block_status_payload_compliance(request->from,
+ request->len);
+request->len = request->contexts->count = 0;
+return nbd_drop(client->ioc, payload_len, errp);
+}
+
/* nbd_co_receive_request
* Collect a client request. Return 0 if request looks valid, -EIO to drop
* connection right away, -EAGAIN to indicate we were interrupted and the
@@ -2470,7 +2552,13 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req, NBDRequest *
[..]
@@ -2712,7 +2804,8 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
"discard failed", errp);
case NBD_CMD_BLOCK_STATUS:
-if (!request->len) {
+assert(request->contexts);
+if (!request->len && !(request->flags & NBD_CMD_FLAG_PAYLOAD_LEN)) {
why not error-out for len=0 in case of payload?
return nbd_send_generic_reply(client, request, -EINVAL,
"need non-zero length", errp);
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 1d155fc2c66..cbca062 100644
--
Best regards,
Vladimir
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v4 24/24] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS
On Thu, Jun 08, 2023 at 08:56:53AM -0500, Eric Blake wrote:
> Allow a client to request a subset of negotiated meta contexts. For
> example, a client may ask to use a single connection to learn about
> both block status and dirty bitmaps, but where the dirty bitmap
> queries only need to be performed on a subset of the disk; forcing the
> server to compute that information on block status queries in the rest
> of the disk is wasted effort (both at the server, and on the amount of
> traffic sent over the wire to be parsed and ignored by the client).
>
> Qemu as an NBD client never requests to use more than one meta
> context, so it has no need to use block status payloads. Testing this
> instead requires support from libnbd, which CAN access multiple meta
> contexts in parallel from a single NBD connection; an interop test
> submitted to the libnbd project at the same time as this patch
> demonstrates the feature working, as well as testing some corner cases
> (for example, when the payload length is longer than the export
> length), although other corner cases (like passing the same id
> duplicated) requires a protocol fuzzer because libnbd is not wired up
> to break the protocol that badly.
>
> This also includes tweaks to 'qemu-nbd --list' to show when a server
> is advertising the capability, and to the testsuite to reflect the
> addition to that output.
>
> Signed-off-by: Eric Blake
> ---
> +++ b/nbd/server.c
> @@ -512,6 +512,9 @@ static int nbd_negotiate_handle_export_name(NBDClient
> *client, bool no_zeroes,
> if (client->mode >= NBD_MODE_STRUCTURED) {
> myflags |= NBD_FLAG_SEND_DF;
> }
> +if (client->mode >= NBD_MODE_EXTENDED && client->contexts.count) {
> +myflags |= NBD_FLAG_BLOCK_STAT_PAYLOAD;
> +}
This one's awkward. At the last minute, I changed what went into
upstream NBD to state that a client that successfully negotiates
extended headers MUST NOT use NBD_OPT_EXPORT_NAME, which means this
branch should never fire for a compliant client. I don't think it
hurts to leave this in, but it does point out that I am missing code
(either here or in 17/24) that at least logs when we detect a
non-compliant client trying to connect with the wrong command.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
