[libnbd PATCH 0/3] resend: fix nbdcopy with nbd-server
While benchmarking nbd-server vs. nbdkit on an underlying raw block device, I encountered multiple bugs. One directly in nbd-server (I'm proposing a patch there soon): block device size is advertised to NBD_OPT_INFO as 0, because it was relying on just stat() data rather than lseek(SEEK_END) the way it does for NBD_OPT_GO. Another two issues in libnbd: we were failing to handle errors in nbd_can_meta_context() correctly which resulted in nbdcopy refusing to work with nbd-server 3.25, plus the fact that we were failing nbd_can_meta_context() in the first place. But getting the testsuite to show the broken behavior was easier when I first patched nbdkit to have the option to behave more like nbd-server (patch for that just sent). Eric Blake (3): copy, info: Treat can_meta_context failures as unsupported api: Fix nbd_can_meta_context for server that lacks meta contexts copy: Add test for server without meta context support generator/states-newstyle-opt-meta-context.c | 2 ++ copy/copy-nbd-error.sh | 10 +++++++++- copy/nbd-ops.c | 2 +- info/map.c | 2 +- 4 files changed, 13 insertions(+), 3 deletions(-) -- 2.43.2
When nbdcopy was first written (commit 48d1e206 is in v1.7.2), nbd_can_meta_context() could not fail. Likewise for nbdinfo --map (earlier than commit 8f30c47 in v1.7.12). But more recently, v1.15.3 made it possible to return an error, and our return conversion of -1 to bool then behaves as if the meta context is available. More specifically, when talking to a source server that has structured replies but lacks meta context support (nbd-server 3.25), this causes nbdcopy to then fail with an odd error message (possibly displayed more than once, depending on how fast multi-threading tackles the problem): nbd://localhost:10811: nbd_block_status_64: did not negotiate any metadata contexts, either you did not call nbd_add_meta_context before connecting or the server does not support it: Operation not supported With this patch, nbdcopy now works on nbd-server as source; meanwhile, nbdinfo --map still gives an error, but it is now more legible: /home/eblake/libnbd/info/.libs/nbdinfo: --map: server does not support metadata context "base:allocation" An audit of other in-tree clients, such as nbddump, found that they were already correctly checking for failures. The next patch will fix the regression in nbd_can_meta_context() behavior for returning an error in the first place; but the later testsuite addition can be shuffled around in the patch series to show that this change in isolation makes nbdcopy more robust despite the API regression. Signed-off-by: Eric Blake <eblake@redhat.com> --- copy/nbd-ops.c | 2 +- info/map.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c index 71ee0a3b..fd42fd19 100644 --- a/copy/nbd-ops.c +++ b/copy/nbd-ops.c @@ -236,7 +236,7 @@ nbd_ops_can_extents (struct rw *rw) struct rw_nbd *rwn = (struct rw_nbd *)rw; if (rwn->handles.len > 0) - return nbd_can_meta_context (rwn->handles.ptr[0], "base:allocation"); + return nbd_can_meta_context (rwn->handles.ptr[0], "base:allocation") > 0; else return false; } diff --git a/info/map.c b/info/map.c index 399e0355..d17cdd06 100644 --- a/info/map.c +++ b/info/map.c @@ -62,7 +62,7 @@ do_map (void) } /* Did we get the requested map? */ - if (!nbd_can_meta_context (nbd, map)) { + if (nbd_can_meta_context (nbd, map) <= 0) { fprintf (stderr, "%s: --map: server does not support metadata context \"%s\"\n", progname, map); -- 2.43.2
Commit 55b09667 in v1.15.3 made it possible for nbd_can_meta_context() to return an error if it is called too early during handshaking, which is useful when doing manual control via nbd_set_opt_mode for integration testing. But it had the unfortunate side effect of treating ALL server errors as unknown state for meta contexts. This didn't matter for clients that never attempt meta context negotiation, or for servers that don't support structured replies (nbd-server 3.24); but for servers with structured replies but not meta contexts (nbd-server 3.25), it means we are now reporting an error about failure to negotiate rather than silently treating the query as false. And with this patch in place, the one-off hack to nbdkit mentioned in that commit (when a server fails after partial success, rather than immediately) still has the desired effect of an error in nbd_can_meta_context. The test for this fix is in a separate commit, to make it easier to rearrange the patch series to demonstrate that the issue at hand can also be worked around in client code. Fiexs: 55b09667 ("api: Fix nbd_can_meta_context if NBD_OPT_SET_META_CONTEXT fails", v1.15.3) Signed-off-by: Eric Blake <eblake@redhat.com> --- generator/states-newstyle-opt-meta-context.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 47e0fcd5..b62a7df0 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -281,6 +281,8 @@ NEWSTYLE.OPT_META_CONTEXT.CHECK_REPLY: SET_NEXT_STATE (%.NEGOTIATING); } else { + if (opt == NBD_OPT_SET_META_CONTEXT && h->meta_contexts.len == 0) + h->meta_valid = true; debug (h, "handshake: ignoring unexpected error from " "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply); SET_NEXT_STATE (%^OPT_GO.START); -- 2.43.2
On Wed, Feb 21, 2024 at 08:53:51AM -0600, Eric Blake wrote:
Commit 55b09667 in v1.15.3 made it possible for nbd_can_meta_context() to return an error if it is called too early during handshaking, which is useful when doing manual control via nbd_set_opt_mode for integration testing. But it had the unfortunate side effect of treating ALL server errors as unknown state for meta contexts. This didn't matter for clients that never attempt meta context negotiation, or for servers that don't support structured replies (nbd-server 3.24); but for servers with structured replies but not meta contexts (nbd-server 3.25), it means we are now reporting an error about failure to negotiate rather than silently treating the query as false. And with this patch in place, the one-off hack to nbdkit mentioned in that commit (when a server fails after partial success, rather than immediately) still has the desired effect of an error in nbd_can_meta_context.
The test for this fix is in a separate commit, to make it easier to rearrange the patch series to demonstrate that the issue at hand can also be worked around in client code.
Fiexs: 55b09667 ("api: Fix nbd_can_meta_context if NBD_OPT_SET_META_CONTEXT fails", v1.15.3)
s/Fiexs/Fixes/
Signed-off-by: Eric Blake <eblake@redhat.com> --- generator/states-newstyle-opt-meta-context.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 47e0fcd5..b62a7df0 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -281,6 +281,8 @@ NEWSTYLE.OPT_META_CONTEXT.CHECK_REPLY: SET_NEXT_STATE (%.NEGOTIATING); } else { + if (opt == NBD_OPT_SET_META_CONTEXT && h->meta_contexts.len == 0) + h->meta_valid = true;
I'm not sure I understand this. Shouldn't it be setting this to false? Rich.
debug (h, "handshake: ignoring unexpected error from " "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply); SET_NEXT_STATE (%^OPT_GO.START); -- 2.43.2 _______________________________________________ Libguestfs mailing list -- guestfs@lists.libguestfs.org To unsubscribe send an email to guestfs-leave@lists.libguestfs.org
-- 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
On Wed, Feb 21, 2024 at 07:08:05PM +0000, Richard W.M. Jones wrote:
On Wed, Feb 21, 2024 at 08:53:51AM -0600, Eric Blake wrote:
Commit 55b09667 in v1.15.3 made it possible for nbd_can_meta_context() to return an error if it is called too early during handshaking, which is useful when doing manual control via nbd_set_opt_mode for integration testing. But it had the unfortunate side effect of treating ALL server errors as unknown state for meta contexts. This didn't matter for clients that never attempt meta context negotiation, or for servers that don't support structured replies (nbd-server 3.24); but for servers with structured replies but not meta contexts (nbd-server 3.25), it means we are now reporting an error about failure to negotiate rather than silently treating the query as false. And with this patch in place, the one-off hack to nbdkit mentioned in that commit (when a server fails after partial success, rather than immediately) still has the desired effect of an error in nbd_can_meta_context.
The test for this fix is in a separate commit, to make it easier to rearrange the patch series to demonstrate that the issue at hand can also be worked around in client code.
Fiexs: 55b09667 ("api: Fix nbd_can_meta_context if NBD_OPT_SET_META_CONTEXT fails", v1.15.3)
s/Fiexs/Fixes/
D'oh; fixed locally.
Signed-off-by: Eric Blake <eblake@redhat.com> --- generator/states-newstyle-opt-meta-context.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 47e0fcd5..b62a7df0 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -281,6 +281,8 @@ NEWSTYLE.OPT_META_CONTEXT.CHECK_REPLY: SET_NEXT_STATE (%.NEGOTIATING); } else { + if (opt == NBD_OPT_SET_META_CONTEXT && h->meta_contexts.len == 0) + h->meta_valid = true;
I'm not sure I understand this. Shouldn't it be setting this to false?
The act of starting a context negotiation sets h->meta_valid = false with meta_contexts.len == 0. If the server replies with NBD_REP_ACK (no matter if there were 0, 1, or many intervening NBD_REP_META_CONTEXT), we then set h->meta_valid = true since the server reported success at the end of a list (that code was already in place at line 224). But this section of code is when the server replies with NBD_REP_ERR_*; here, we want to treat any immediate error (with no intervening NBD_REP_META_CONTEXT) as a successful empty list (we know that the server doesn't support any metacontexts), but any late error (with one or more intervening NBD_REP_META_CONTEXT) should leave things to the already-set meta_valid=false so that we aren't confusing ourselves on a partial list before a late error. Would it make more sense if I wrote it as: /* When ignoring error, treat attempt as success only if list is empty */ if (opt == NBD_OPT_SET_META_CONTEXT) h->meta_valid = h->meta_contexts.len == 0; -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
On Wed, Feb 21, 2024 at 02:22:36PM -0600, Eric Blake wrote:
On Wed, Feb 21, 2024 at 07:08:05PM +0000, Richard W.M. Jones wrote:
On Wed, Feb 21, 2024 at 08:53:51AM -0600, Eric Blake wrote:
Commit 55b09667 in v1.15.3 made it possible for nbd_can_meta_context() to return an error if it is called too early during handshaking, which is useful when doing manual control via nbd_set_opt_mode for integration testing. But it had the unfortunate side effect of treating ALL server errors as unknown state for meta contexts. This didn't matter for clients that never attempt meta context negotiation, or for servers that don't support structured replies (nbd-server 3.24); but for servers with structured replies but not meta contexts (nbd-server 3.25), it means we are now reporting an error about failure to negotiate rather than silently treating the query as false. And with this patch in place, the one-off hack to nbdkit mentioned in that commit (when a server fails after partial success, rather than immediately) still has the desired effect of an error in nbd_can_meta_context.
The test for this fix is in a separate commit, to make it easier to rearrange the patch series to demonstrate that the issue at hand can also be worked around in client code.
Fiexs: 55b09667 ("api: Fix nbd_can_meta_context if NBD_OPT_SET_META_CONTEXT fails", v1.15.3)
s/Fiexs/Fixes/
D'oh; fixed locally.
Signed-off-by: Eric Blake <eblake@redhat.com> --- generator/states-newstyle-opt-meta-context.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 47e0fcd5..b62a7df0 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -281,6 +281,8 @@ NEWSTYLE.OPT_META_CONTEXT.CHECK_REPLY: SET_NEXT_STATE (%.NEGOTIATING); } else { + if (opt == NBD_OPT_SET_META_CONTEXT && h->meta_contexts.len == 0) + h->meta_valid = true;
I'm not sure I understand this. Shouldn't it be setting this to false?
The act of starting a context negotiation sets h->meta_valid = false with meta_contexts.len == 0. If the server replies with NBD_REP_ACK (no matter if there were 0, 1, or many intervening NBD_REP_META_CONTEXT), we then set h->meta_valid = true since the server reported success at the end of a list (that code was already in place at line 224). But this section of code is when the server replies with NBD_REP_ERR_*; here, we want to treat any immediate error (with no intervening NBD_REP_META_CONTEXT) as a successful empty list (we know that the server doesn't support any metacontexts), but any late error (with one or more intervening NBD_REP_META_CONTEXT) should leave things to the already-set meta_valid=false so that we aren't confusing ourselves on a partial list before a late error.
Would it make more sense if I wrote it as:
/* When ignoring error, treat attempt as success only if list is empty */ if (opt == NBD_OPT_SET_META_CONTEXT) h->meta_valid = h->meta_contexts.len == 0;
That's nice, thanks! Reviewed-by: Richard W.M. Jones <rjones@redhat.com> -- 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
nbd-server 3.24 behaves like 'nbdkit --no-sr', and we were gracefully handling that. But nbd-server 3.25 added structured replies while still leaving meta contexts unsupported. This patch adds testsuite coverage for that case by relying on a new feature in upcoming nbdkit 1.38, independently of two previous patches (one to nbdcopy and one to nbd_can_meta_context), so that you can rearrange application of the patch series and see that either fix in isolation prevents nbdcopy from tripping over a spurious failure. Signed-off-by: Eric Blake <eblake@redhat.com> --- copy/copy-nbd-error.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/copy/copy-nbd-error.sh b/copy/copy-nbd-error.sh index df883274..a0e7676d 100755 --- a/copy/copy-nbd-error.sh +++ b/copy/copy-nbd-error.sh @@ -31,9 +31,17 @@ requires $NBDKIT --filter=nozero memory --version fail=0 +# Failure to negotiate block status should not be fatal, but merely downgrade +# to reading the entire image as if data +if $NBDKIT --help | grep 'no-meta-contexts'; then + echo "Testing early extents failures on source" + $VG nbdcopy -- [ $NBDKIT --exit-with-parent -v --no-meta-contexts \ + pattern 5M ] null: || fail=1 +fi + # Failure to get block status should not be fatal, but merely downgrade to # reading the entire image as if data -echo "Testing extents failures on source" +echo "Testing late extents failures on source" $VG nbdcopy -- [ $NBDKIT --exit-with-parent -v --filter=error pattern 5M \ error-extents-rate=1 ] null: || fail=1 -- 2.43.2
On Wed, Feb 21, 2024 at 08:53:52AM -0600, Eric Blake wrote:
nbd-server 3.24 behaves like 'nbdkit --no-sr', and we were gracefully handling that. But nbd-server 3.25 added structured replies while still leaving meta contexts unsupported. This patch adds testsuite coverage for that case by relying on a new feature in upcoming nbdkit 1.38, independently of two previous patches (one to nbdcopy and one to nbd_can_meta_context), so that you can rearrange application of the patch series and see that either fix in isolation prevents nbdcopy from tripping over a spurious failure.
Signed-off-by: Eric Blake <eblake@redhat.com> --- copy/copy-nbd-error.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/copy/copy-nbd-error.sh b/copy/copy-nbd-error.sh index df883274..a0e7676d 100755 --- a/copy/copy-nbd-error.sh +++ b/copy/copy-nbd-error.sh @@ -31,9 +31,17 @@ requires $NBDKIT --filter=nozero memory --version
fail=0
+# Failure to negotiate block status should not be fatal, but merely downgrade +# to reading the entire image as if data +if $NBDKIT --help | grep 'no-meta-contexts'; then + echo "Testing early extents failures on source" + $VG nbdcopy -- [ $NBDKIT --exit-with-parent -v --no-meta-contexts \ + pattern 5M ] null: || fail=1 +fi + # Failure to get block status should not be fatal, but merely downgrade to # reading the entire image as if data -echo "Testing extents failures on source" +echo "Testing late extents failures on source" $VG nbdcopy -- [ $NBDKIT --exit-with-parent -v --filter=error pattern 5M \ error-extents-rate=1 ] null: || fail=1
For 1 & 3: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Wed, Feb 21, 2024 at 07:08:50PM +0000, Richard W.M. Jones wrote:
On Wed, Feb 21, 2024 at 08:53:52AM -0600, Eric Blake wrote:
nbd-server 3.24 behaves like 'nbdkit --no-sr', and we were gracefully handling that. But nbd-server 3.25 added structured replies while still leaving meta contexts unsupported. This patch adds testsuite coverage for that case by relying on a new feature in upcoming nbdkit 1.38, independently of two previous patches (one to nbdcopy and one to nbd_can_meta_context), so that you can rearrange application of the patch series and see that either fix in isolation prevents nbdcopy from tripping over a spurious failure.
For 1 & 3:
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
With subsequent reviewed modifications to 2, and the followon patch to let nbdinfo show contents of nbd-server, I've now pushed patches 2b1f9dad..a0aa3c16. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
participants (2)
-
Eric Blake -
Richard W.M. Jones