Eric Blake
2019-Jun-04 17:12 UTC
[Libguestfs] [libnbd PATCH 0/2] Better handling of failed block_status callback
Rather than moving the connection to DEAD, we can just ignore further contexts to the existing command handle, and fail the overall command with the same errno as the failed callback. Eric Blake (2): states: Track cmd->error as errno, not wire value api: Recover from block status callback failure generator/generator | 5 ++- generator/states-reply-simple.c | 2 +- generator/states-reply-structured.c | 61 ++++++++++++++------------- interop/dirty-bitmap.c | 65 +++++++++++++++++++++++------ lib/aio.c | 1 - lib/internal.h | 2 +- 6 files changed, 91 insertions(+), 45 deletions(-) -- 2.20.1
Eric Blake
2019-Jun-04 17:12 UTC
[Libguestfs] [libnbd PATCH 1/2] states: Track cmd->error as errno, not wire value
We want to allow the block_status callback to be able to report a full range of errno values, rather than the limited set of NBD_E* defined in the protocol. To do so, we need to convert from wire error values back into local errno values at a sooner point in the state machine. This also changes a structured reply that includes more than one error designation (theoretically possible at least for structured reads that uses NBD_REPLY_TYPE_ERROR_OFFSET more than once to NBD_CMD_READ - even if no existing NBD server actually does that) to preserve the first error encountered, as that is often more interesting than any follow-on errors. --- generator/states-reply-simple.c | 2 +- generator/states-reply-structured.c | 4 +++- lib/aio.c | 1 - lib/internal.h | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index e170482..7e5340c 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -45,7 +45,7 @@ return 0; } - cmd->error = error; + cmd->error = nbd_internal_errno_of_nbd_error (error); if (cmd->error == 0 && cmd->type == NBD_CMD_READ) { h->rbuf = cmd->data; h->rlen = cmd->count; diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index c835713..c7ba892 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -179,7 +179,9 @@ } assert (cmd); /* guaranteed by CHECK */ - cmd->error = error; + /* Preserve first error encountered */ + if (cmd->error == 0) + cmd->error = nbd_internal_errno_of_nbd_error (error); if (flags & NBD_REPLY_FLAG_DONE) SET_NEXT_STATE (%^FINISH_COMMAND); diff --git a/lib/aio.c b/lib/aio.c index a129af2..8361854 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -165,7 +165,6 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, return 1; /* The command failed, set an error indication and return an error. */ - error = nbd_internal_errno_of_nbd_error (error); set_error (error, "%s: command failed: %s", nbd_internal_name_of_nbd_cmd (type), strerror (error)); return -1; diff --git a/lib/internal.h b/lib/internal.h index c1a57ac..45fc6df 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -223,7 +223,7 @@ struct command_in_flight { uint32_t count; void *data; /* Buffer for read/write, opaque for block status */ extent_fn extent_fn; - uint32_t error; + uint32_t error; /* Local errno value */ }; /* crypto.c */ -- 2.20.1
Eric Blake
2019-Jun-04 17:12 UTC
[Libguestfs] [libnbd PATCH 2/2] api: Recover from block status callback failure
If the user's extent_fn callback fails, we want to fail the overall block status command with the same errno, but not give up on the server. Do this by skipping any further context chunks from the server, and relying on the previous patch's ability to preserve the first error encountered. Update the interop test with qemu's dirty bitmap context to provoke callback failure, as well as prove that the connection is still valid. I did not go as far as documenting EPROTO as our default errno if the callback returns -1 with errno unchanged. --- generator/generator | 5 ++- generator/states-reply-structured.c | 57 +++++++++++++------------ interop/dirty-bitmap.c | 65 +++++++++++++++++++++++------ 3 files changed, 86 insertions(+), 41 deletions(-) diff --git a/generator/generator b/generator/generator index 91545ce..1261d54 100755 --- a/generator/generator +++ b/generator/generator @@ -1414,7 +1414,10 @@ supported by the server (see C<nbd_can_meta_context>) this call returns information about extents by calling back to the extent function. The callback cannot call C<nbd_*> APIs on the same handle since it holds the handle lock and will -cause a deadlock. +cause a deadlock. If the callback returns C<-1>, any remaining +contexts will be ignored and the overall block status command +will fail with the same value of C<errno> left after the +failing callback. The extent function is called once per type of metadata available. The C<metacontext> parameter is a string diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index c7ba892..5791360 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -357,34 +357,37 @@ assert (h->bs_entries); assert (length >= 12); - /* Need to byte-swap the entries returned, but apart from that we - * don't validate them. - */ - for (i = 0; i < length/4; ++i) - h->bs_entries[i] = be32toh (h->bs_entries[i]); - - /* Look up the context ID. */ - context_id = h->bs_entries[0]; - for (meta_context = h->meta_contexts; - meta_context; - meta_context = meta_context->next) - if (context_id == meta_context->context_id) - break; - - if (meta_context) { - /* Call the caller's extent function. */ - if (cmd->extent_fn (cmd->data, meta_context->name, cmd->offset, - &h->bs_entries[1], (length-4) / 4) == -1) { - SET_NEXT_STATE (%.DEAD); /* XXX We should be able to recover. */ - if (errno == 0) errno = EPROTO; - set_error (errno, "extent function failed"); - return -1; - } - } - else + if (cmd->error) /* Emit a debug message, but ignore it. */ - debug (h, "server sent unexpected meta context ID %" PRIu32, - context_id); + debug (h, "ignoring meta context ID %" PRIu32 " after callback failure", + be32toh (h->bs_entries[0])); + else { + /* Need to byte-swap the entries returned, but apart from that we + * don't validate them. + */ + for (i = 0; i < length/4; ++i) + h->bs_entries[i] = be32toh (h->bs_entries[i]); + + /* Look up the context ID. */ + context_id = h->bs_entries[0]; + for (meta_context = h->meta_contexts; + meta_context; + meta_context = meta_context->next) + if (context_id == meta_context->context_id) + break; + + if (meta_context) { + /* Call the caller's extent function. */ + errno = 0; + if (cmd->extent_fn (cmd->data, meta_context->name, cmd->offset, + &h->bs_entries[1], (length-4) / 4) == -1) + cmd->error = errno ? errno : EPROTO; + } + else + /* Emit a debug message, but ignore it. */ + debug (h, "server sent unexpected meta context ID %" PRIu32, + context_id); + } if (flags & NBD_REPLY_FLAG_DONE) SET_NEXT_STATE (%^FINISH_COMMAND); diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c index 8d34173..59be0c8 100644 --- a/interop/dirty-bitmap.c +++ b/interop/dirty-bitmap.c @@ -23,6 +23,8 @@ #include <string.h> #include <unistd.h> #include <assert.h> +#include <stdbool.h> +#include <errno.h> #include <libnbd.h> @@ -30,21 +32,31 @@ static const char *unixsocket; static const char *bitmap; static const char *base_allocation = "base:allocation"; -static int calls; /* Track which contexts passed through callback */ +struct data { + bool req_one; /* input: true if req_one was passed to request */ + int count; /* input: count of expected remaining calls */ + bool fail; /* input: true to return failure */ + bool seen_base; /* output: true if base:allocation encountered */ + bool seen_dirty; /* output: true if qemu:dirty-bitmap encountered */ +}; static int -cb (void *data, const char *metacontext, uint64_t offset, +cb (void *opaque, const char *metacontext, uint64_t offset, uint32_t *entries, size_t len) { - /* Hack: data is non-NULL if request included REQ_ONE flag */ + struct data *data = opaque; + assert (offset == 0); + assert (data->count-- > 0); + if (strcmp (metacontext, base_allocation) == 0) { - calls += 0x1; - assert (len == (data ? 2 : 8)); + assert (!data->seen_base); + data->seen_base = true; + assert (len == (data->req_one ? 2 : 8)); /* Data block offset 0 size 128k */ assert (entries[0] == 131072); assert (entries[1] == 0); - if (!data) { + if (!data->req_one) { /* hole|zero offset 128k size 384k */ assert (entries[2] == 393216); assert (entries[3] == 3); /* allocated zero offset 512k size 64k */ @@ -54,11 +66,12 @@ cb (void *data, const char *metacontext, uint64_t offset, } } else if (strcmp (metacontext, bitmap) == 0) { - calls += 0x10; - assert (len == (data ? 2 : 10)); + assert (!data->seen_dirty); + data->seen_dirty = true; + assert (len == (data->req_one ? 2 : 10)); assert (entries[0] == 65536); assert (entries[1] == 0); - if (!data) { + if (!data->req_one) { /* dirty block offset 64K size 64K */ assert (entries[2] == 65536); assert (entries[3] == 1); assert (entries[4] == 393216); assert (entries[5] == 0); @@ -72,6 +85,10 @@ cb (void *data, const char *metacontext, uint64_t offset, exit (EXIT_FAILURE); } + if (data->fail) { + errno = EPROTO; /* Something NBD servers can't send */ + return -1; + } return 0; } @@ -80,6 +97,8 @@ main (int argc, char *argv[]) { struct nbd_handle *nbd; int64_t exportsize; + struct data data; + char c; if (argc != 3) { fprintf (stderr, "%s unixsocket bitmap\n", argv[0]); @@ -108,17 +127,37 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - if (nbd_block_status (nbd, exportsize, 0, NULL, cb, 0) == -1) { + data = (struct data) { .count = 2, }; + if (nbd_block_status (nbd, exportsize, 0, &data, cb, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } - assert (calls == 0x11); - if (nbd_block_status (nbd, exportsize, 0, &exportsize, cb, + assert (data.seen_base && data.seen_dirty); + + data = (struct data) { .req_one = true, .count = 2, }; + if (nbd_block_status (nbd, exportsize, 0, &data, cb, LIBNBD_CMD_FLAG_REQ_ONE) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } - assert (calls == 0x22); + assert (data.seen_base && data.seen_dirty); + + /* Trigger a failed callback, to prove connection stays up. No way + * to know which context will respond first, but we can check that + * the other one did not get visited. + */ + data = (struct data) { .count = 1, .fail = true, }; + if (nbd_block_status (nbd, exportsize, 0, &data, cb, 0) != -1) { + fprintf (stderr, "unexpected block status success\n"); + exit (EXIT_FAILURE); + } + assert (nbd_get_errno () == EPROTO && nbd_aio_is_ready (nbd)); + assert (data.seen_base ^ data.seen_dirty); + + if (nbd_pread (nbd, &c, 1, 0, 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if (nbd_shutdown (nbd) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); -- 2.20.1
Richard W.M. Jones
2019-Jun-05 12:29 UTC
Re: [Libguestfs] [libnbd PATCH 2/2] api: Recover from block status callback failure
Series looks good, ACK, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Possibly Parallel Threads
- Re: [PATCH libnbd v2 2/4] generator: Callback returns int instead of void.
- [libnbd PATCH 2/2] api: Recover from block status callback failure
- [PATCH libnbd v2 0/4] api: Implement concurrent writer.
- [libnbd PATCH] block-status: Make callback usage consistent with pread_structured
- [PATCH libnbd] api: Get rid of nbd_connection.