8:53 a.m.
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
8:53 a.m.
New subject: [libnbd PATCH 1/3] copy, info: Treat can_meta_context failures as unsupported
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(a)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
8:53 a.m.
New subject: [libnbd PATCH 2/3] api: Fix nbd_can_meta_context for server that lacks meta contexts
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(a)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
1:08 p.m.
New subject: [libnbd PATCH 2/3] api: Fix nbd_can_meta_context for server that lacks meta contexts
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(a)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(a)lists.libguestfs.org
To unsubscribe send an email to guestfs-leave(a)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
2:22 p.m.
New subject: [libnbd PATCH 2/3] api: Fix nbd_can_meta_context for server that lacks meta contexts
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(a)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
2:52 p.m.
New subject: [libnbd PATCH 2/3] api: Fix nbd_can_meta_context for server that lacks meta contexts
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(a)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(a)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
8:53 a.m.
New subject: [libnbd PATCH 3/3] copy: Add test for server without meta context support
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(a)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
1:08 p.m.
New subject: [libnbd PATCH 3/3] copy: Add test for server without meta context support
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(a)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(a)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
4:12 p.m.
New subject: [libnbd PATCH 3/3] copy: Add test for server without meta context support
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(a)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
276
days inactive
276
days old
8 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones