Richard W.M. Jones
2019-Mar-23 11:42 UTC
[Libguestfs] nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected
nbdkit (upstream 5a7a394c699) currently fails with qemu 2.12.0:
$ ./nbdkit memory size=64M --run 'qemu-img convert $nbd /var/tmp/out'
nbdkit: memory.2: error: invalid request: unknown command (7) ignored
qemu-img: Protocol error: simple reply when structured reply chunk was
expected
This was a bug in qemu which was fixed upstream quite a long time ago
by the commit I've attached at the end of this email.
Unfortunately RHEL 7.6 & 7.7 (qemu-kvm-rhev-2.12.0-25.el7) doesn't
contain the fix. I wonder if we should:
(a) Not worry about it - it's Red Hat's problem.
(b) Try and get it fixed in RHEL. I filed a BZ already but I guess it
won't be fixed any time soon: https://bugzilla.redhat.com/1692018
(c) Add a workaround in nbdkit.
I feel that we probably shouldn't be broken out of the box with RHEL
and CentOS 7.6 and 7.7, even though it's not an nbdkit (or qemu) bug.
Rich.
----------------------------------------------------------------------
89aa0d87634e2cb98517509dc8bdb876f26ecf8b is the first bad commit
commit 89aa0d87634e2cb98517509dc8bdb876f26ecf8b
Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Date: Fri Apr 27 17:20:01 2018 +0300
nbd/client: fix nbd_negotiate_simple_meta_context
Initialize received variable. Otherwise, is is possible for server to
answer without any contexts, but we will set context_id to something
random (received_id is not initialized too) and return 1, which is
wrong.
To solve it, just initialize received to false. Initialize received_id
too, just to make all possible checkers happy.
Bug was introduced in 78a33ab58782efdb206de14 "nbd: BLOCK_STATUS for
standard get_block_status function: client part" with the whole
function.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20180427142002.21930-2-vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
:040000 040000 9993feb118af1a9a59dbc8fe92015a324e93e557
14db90d621d6b7f1ee5ff97a0ca2cb92f6f2f7e9 M nbd
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
Eric Blake
2019-Mar-23 11:57 UTC
Re: [Libguestfs] nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected
On 3/23/19 6:42 AM, Richard W.M. Jones wrote:> nbdkit (upstream 5a7a394c699) currently fails with qemu 2.12.0: > > $ ./nbdkit memory size=64M --run 'qemu-img convert $nbd /var/tmp/out' > nbdkit: memory.2: error: invalid request: unknown command (7) ignored > qemu-img: Protocol error: simple reply when structured reply chunk was expected > > This was a bug in qemu which was fixed upstream quite a long time ago > by the commit I've attached at the end of this email. > > Unfortunately RHEL 7.6 & 7.7 (qemu-kvm-rhev-2.12.0-25.el7) doesn't > contain the fix. I wonder if we should: > > (a) Not worry about it - it's Red Hat's problem.Upstream qemu has moved on, so from that perspective, yes, it's Red Hat's problem (Red Hat must ensure that any versions of qemu and nbdkit that it ships and which must interoperate do so correctly, whether that be qemu from RHEL 7 and nbdkit from RHEL 8, or the other way around).> > (b) Try and get it fixed in RHEL. I filed a BZ already but I guess it > won't be fixed any time soon: https://bugzilla.redhat.com/1692018Several RHEL 7 bugs against NBD are pending - there's hope for RHEL 7.7, but you are also right that guessing timelines is hard.> > (c) Add a workaround in nbdkit. >And what workaround would that be? Looking at the qemu patch, the problem is that qemu asked for "base:allocation" and nbdkit replied with 0 contexts. The workaround would either be to reply with NBD_REP_ERR_UNSUP (the behavior before NBD_OPT_SET_META_CONTEXT was added), or to implement "base:allocation" (even if we implement the poor-man's version that always reports that the entire image is data, for all requests, without any feedback from the plugins). The former feels odd, but the latter seems oddly appealing as progress towards our end goal (kind of how our initial implementation of NBD_CMD_WRITE_ZEROES was always advertised even before we finished wiring up plug support).> I feel that we probably shouldn't be broken out of the box with RHEL > and CentOS 7.6 and 7.7, even though it's not an nbdkit (or qemu) bug.Correct that it is not nbdkit's bug; it was a bug in qemu (but one that upstream no longer worries about), and old enough of a qemu release that I don't the the NBD protocol needs to bother mentioning it as a special case.> > Rich. > > ---------------------------------------------------------------------- > 89aa0d87634e2cb98517509dc8bdb876f26ecf8b is the first bad commit > commit 89aa0d87634e2cb98517509dc8bdb876f26ecf8b > Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Date: Fri Apr 27 17:20:01 2018 +0300 > > nbd/client: fix nbd_negotiate_simple_meta_context > > Initialize received variable. Otherwise, is is possible for server to > answer without any contexts, but we will set context_id to something > random (received_id is not initialized too) and return 1, which is > wrong. > > To solve it, just initialize received to false. Initialize received_id > too, just to make all possible checkers happy. > > Bug was introduced in 78a33ab58782efdb206de14 "nbd: BLOCK_STATUS for > standard get_block_status function: client part" with the whole > function. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Message-Id: <20180427142002.21930-2-vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > CC: qemu-stable@nongnu.org > Signed-off-by: Eric Blake <eblake@redhat.com> > > :040000 040000 9993feb118af1a9a59dbc8fe92015a324e93e557 14db90d621d6b7f1ee5ff97a0ca2cb92f6f2f7e9 M nbd > >-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Mar-23 12:21 UTC
Re: [Libguestfs] nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected
On Sat, Mar 23, 2019 at 06:57:14AM -0500, Eric Blake wrote:> On 3/23/19 6:42 AM, Richard W.M. Jones wrote: > > (b) Try and get it fixed in RHEL. I filed a BZ already but I guess it > > won't be fixed any time soon: https://bugzilla.redhat.com/1692018 > > Several RHEL 7 bugs against NBD are pending - there's hope for RHEL 7.7, > but you are also right that guessing timelines is hard.Yes, my worry is that if we miss 7.7 then this bug will exist forever since as far as I know we (Red Hat) are not planning any further qemu improvements in 7.8 and above.> > (c) Add a workaround in nbdkit. > > > > And what workaround would that be? Looking at the qemu patch, the > problem is that qemu asked for "base:allocation" and nbdkit replied with > 0 contexts. The workaround would either be to reply with > NBD_REP_ERR_UNSUP (the behavior before NBD_OPT_SET_META_CONTEXT was > added), or to implement "base:allocation" (even if we implement the > poor-man's version that always reports that the entire image is data, > for all requests, without any feedback from the plugins). The former > feels odd, but the latter seems oddly appealing as progress towards our > end goal (kind of how our initial implementation of NBD_CMD_WRITE_ZEROES > was always advertised even before we finished wiring up plug support).A good point here is what happens with the block-status branch[1]. Let's see: $ ./nbdkit memory size=64M --run '/home/rjones/d/qemu/qemu-img convert $nbd /var/tmp/out' qemu-img: Payload too large nbdkit: memory.1: error: write reply: NBD_CMD_BLOCK_STATUS: Broken pipe Oh dear. I believe this is actually a bug in the block status code so let's see if I can narrow this down first ... Rich. [1] https://github.com/rwmjones/nbdkit/tree/block-status -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Eric Blake
2019-Mar-23 12:26 UTC
Re: [Libguestfs] nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected
[adding qemu] On 3/23/19 6:57 AM, Eric Blake wrote:> On 3/23/19 6:42 AM, Richard W.M. Jones wrote: >> nbdkit (upstream 5a7a394c699) currently fails with qemu 2.12.0: >> >> $ ./nbdkit memory size=64M --run 'qemu-img convert $nbd /var/tmp/out' >> nbdkit: memory.2: error: invalid request: unknown command (7) ignored >> qemu-img: Protocol error: simple reply when structured reply chunk was expected >> >> This was a bug in qemu which was fixed upstream quite a long time ago >> by the commit I've attached at the end of this email. >>>> ---------------------------------------------------------------------- >> 89aa0d87634e2cb98517509dc8bdb876f26ecf8b is the first bad commit >> commit 89aa0d87634e2cb98517509dc8bdb876f26ecf8b >> Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Date: Fri Apr 27 17:20:01 2018 +0300 >> >> nbd/client: fix nbd_negotiate_simple_meta_context >> >> Initialize received variable. Otherwise, is is possible for server to >> answer without any contexts, but we will set context_id to something >> random (received_id is not initialized too) and return 1, which is >> wrong. >>Ouch - upstream qemu still has a latent bug in this area. When qemu as server sends an error to NBD_CMD_BLOCK_STATUS, it uses a structured error reply, so we haven't tickled it before. But the NBD spec permits a simple error reply to any command except NBD_CMD_READ when structured replies are negotiated, and that's what nbdkit currently implements. If I add this hack patch to qemu's server (to intentionally fail NBD_CMD_BLOCK_STATUS with a simple error), then the qemu client _should_ be able to just report that block status failed and keep the connection alive, rather than its current behavior of hanging up. So we still need a patch for qemu 4.0 for the client to accept simple errors. :( With the hack applied, I got: $ ./qemu-io -f raw nbd://localhost:10809 --trace=nbd_\* -c map ... 17218@1553343857.048440:nbd_send_request Sending request to server: { .from = 0, .len = 1049088, .handle = 94796979915216, .flags = 0x8, .type = 7 (block status) } 17218@1553343857.048643:nbd_receive_simple_reply Got simple reply: { .error = 22 (EINVAL), handle = 94796979915216 } 17218@1553343857.048655:nbd_co_request_fail Request failed { .from = 0, .len = 1049088, .handle = 94796979915216, .flags = 0x8, .type = 7 (block status) } ret = -22, err: Protocol error: simple reply when structured reply chunk was expected Failed to get allocation status: Invalid argument diff --git i/nbd/server.c w/nbd/server.c index fd013a2817a..35f549abbf9 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -2148,16 +2148,16 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, * Returns 0 if connection is still live, -errno on failure to talk to client */ static coroutine_fn int nbd_send_generic_reply(NBDClient *client, - uint64_t handle, + NBDRequest *request, int ret, const char *error_msg, Error **errp) { - if (client->structured_reply && ret < 0) { - return nbd_co_send_structured_error(client, handle, -ret, error_msg, + if (client->structured_reply && ret < 0 && request->type =NBD_CMD_READ) { + return nbd_co_send_structured_error(client, request->handle, -ret, error_msg, errp); } else { - return nbd_co_send_simple_reply(client, handle, ret < 0 ? -ret : 0, + return nbd_co_send_simple_reply(client, request->handle, ret < 0 ? -ret : 0, NULL, 0, errp); } } @@ -2177,7 +2177,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request, if (request->flags & NBD_CMD_FLAG_FUA) { ret = blk_co_flush(exp->blk); if (ret < 0) { - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "flush failed", errp); } } @@ -2192,7 +2192,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request, ret = blk_pread(exp->blk, request->from + exp->dev_offset, data, request->len); if (ret < 0 || request->type == NBD_CMD_CACHE) { - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "reading from file failed", errp); } @@ -2234,7 +2234,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } ret = blk_pwrite(exp->blk, request->from + exp->dev_offset, data, request->len, flags); - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "writing to file failed", errp); case NBD_CMD_WRITE_ZEROES: @@ -2247,7 +2247,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset, request->len, flags); - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "writing to file failed", errp); case NBD_CMD_DISC: @@ -2256,7 +2256,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, case NBD_CMD_FLUSH: ret = blk_co_flush(exp->blk); - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "flush failed", errp); case NBD_CMD_TRIM: @@ -2265,12 +2265,14 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, if (ret == 0 && request->flags & NBD_CMD_FLAG_FUA) { ret = blk_co_flush(exp->blk); } - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "discard failed", errp); case NBD_CMD_BLOCK_STATUS: + return nbd_send_generic_reply(client, request, -EINVAL, + "no status for you today", errp); if (!request->len) { - return nbd_send_generic_reply(client, request->handle, -EINVAL, + return nbd_send_generic_reply(client, request, -EINVAL, "need non-zero length", errp); } if (client->export_meta.valid && @@ -2304,7 +2306,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, return ret; } else { - return nbd_send_generic_reply(client, request->handle, -EINVAL, + return nbd_send_generic_reply(client, request, -EINVAL, "CMD_BLOCK_STATUS not negotiated", errp); } @@ -2312,7 +2314,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, default: msg = g_strdup_printf("invalid request type (%" PRIu32 ") received", request->type); - ret = nbd_send_generic_reply(client, request->handle, -EINVAL, msg, + ret = nbd_send_generic_reply(client, request, -EINVAL, msg, errp); g_free(msg); return ret; @@ -2357,7 +2359,7 @@ static coroutine_fn void nbd_trip(void *opaque) Error *export_err = local_err; local_err = NULL; - ret = nbd_send_generic_reply(client, request.handle, -EINVAL, + ret = nbd_send_generic_reply(client, &request, -EINVAL, error_get_pretty(export_err), &local_err); error_free(export_err); } else { -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Mar-25 18:07 UTC
Re: [Libguestfs] nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected
[Removed public mailing list] On Mon, Mar 25, 2019 at 01:45:32PM -0400, John Snow wrote:> > > On 3/23/19 8:21 AM, Richard W.M. Jones wrote: > > On Sat, Mar 23, 2019 at 06:57:14AM -0500, Eric Blake wrote: > >> On 3/23/19 6:42 AM, Richard W.M. Jones wrote: > >>> (b) Try and get it fixed in RHEL. I filed a BZ already but I guess it > >>> won't be fixed any time soon: https://bugzilla.redhat.com/1692018 > >> > >> Several RHEL 7 bugs against NBD are pending - there's hope for RHEL 7.7, > >> but you are also right that guessing timelines is hard. > > > > Yes, my worry is that if we miss 7.7 then this bug will exist forever > > since as far as I know we (Red Hat) are not planning any further qemu > > improvements in 7.8 and above. > > > > Feel free to add some ACKs to the lengthy backports I've posted in 7.7 > recently ... > > 3.1.0 NBD Roundup is probably particularly of interest to you right now.Is this on rhvirt-patches? I can't see it ... Rich.> I'm working on 4.0.0 NBD roundup right now, too.-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Apparently Analagous Threads
- cross-project patches: Add NBD Fast Zero support
- Re: nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected
- [RFC PATCH 1/3] qemu: virtio-bypass should explicitly bind to a passthrough device
- [libnbd PATCH v2 3/5] states: Add nbd_pread_structured API
- [PATCH 2/3] qemu: Implement virtio-pstore device