Re: [Qemu-block] [PATCH v4 09/10] block/nbd-client: nbd reconnect
On 2/5/19 11:07 AM, Vladimir Sementsov-Ogievskiy wrote: >> Or, put another way, we KNOW we have (corner) cases where a mis-aligned >> image can currently cause the server to return BLOCK_STATUS replies that >> aren't aligned to the advertised minimumm block size. Attempting to >> read the last sector of an image then causes the client to see the >> misaligned reply and complain, which we are treating as fatal. > > Do we have fixes for it? Not yet - it's still on my queue of things to fix after I get libvirt incremental backup APIs in. Might make 4.0, might not (but not the end of the world; it's been known-broken since 3.0, so it's not a new regression). > >> But why >> not instead just fail that particular read, but still attempt a >> reconnect, in order to attempt further reads elsewhere in the image that >> do not trip up the server's misaligned reply? >> > > Hmm, for these cases, if we consider this errors not fatal, we don't need > even a reconnect.. Well, it all depends on whether the client is still in sync with the server. If either side has disobeyed the spec, and send too many/too few bytes compared to what the other side expects, you'll have magic number mismatches, where you really DO need a reconnect to get back in sync. > > If we want to consider protocol errors to be recoverable, we need reconnect > only > on wrong magic and may be some kind of inconsistent variable data lenghts.. > > And it may need addition option, like --strict=false An option on how hard to try may be reasonable. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4 09/10] block/nbd-client: nbd reconnect
16.01.2019 20:04, Eric Blake wrote:
> On 7/31/18 12:30 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Implement reconnect. To achieve this:
>>
>> 1. add new modes:
>> connecting-wait: means, that reconnecting is in progress, and there
>> were small number of reconnect attempts, so all requests are
>> waiting for the connection.
>> connecting-nowait: reconnecting is in progress, there were a lot of
>> attempts of reconnect, all requests will return errors.
>>
>> two old modes are used too:
>> connected: normal state
>> quit: exiting after fatal error or on close
>
> What makes an error fatal? Without reconnect, life is simple - if the
> server sends something we can't parse, we permanently turn the device
> into an error condition - because we have no way to get back in sync
> with the server for further commands. Your patch allows reconnect
> attempts where the connection is down (we failed to send to the server
> or failed to receive the server's reply), but why can we not ALSO
> attempt to reconnect after a parse error? A reconnect would let us get
> back in sync for attempting further commands. You're right that the
> current command should probably fail in that case (if the server sent us
> garbage for a specific request, it will probably do so again on a repeat
> of that request; which is different than when we don't even know what
> the server would have sent because of a disconnect).
My thought was that protocol error is fatal, as it shows that server is
buggy, and it seems unsafe to communicate to buggy server..
>
> Or, put another way, we KNOW we have (corner) cases where a mis-aligned
> image can currently cause the server to return BLOCK_STATUS replies that
> aren't aligned to the advertised minimumm block size. Attempting to
> read the last sector of an image then causes the client to see the
> misaligned reply and complain, which we are treating as fatal.
Do we have fixes for it?
> But why
> not instead just fail that particular read, but still attempt a
> reconnect, in order to attempt further reads elsewhere in the image that
> do not trip up the server's misaligned reply?
>
Hmm, for these cases, if we consider this errors not fatal, we don't need
even a reconnect..
If we want to consider protocol errors to be recoverable, we need reconnect only
on wrong magic and may be some kind of inconsistent variable data lenghts..
And it may need addition option, like --strict=false
>>
>> Possible transitions are:
>>
>> * -> quit
>> connecting-* -> connected
>> connecting-wait -> connecting-nowait (transition is done after
>>reconnect-delay seconds in connecting-wait mode)
>> connected -> connecting-wait
>>
>> 2. Implement reconnect in connection_co. So, in connecting-* mode,
>> connection_co, tries to reconnect unlimited times.
>>
>> 3. Retry nbd queries on channel error, if we are in connecting-wait
>> state.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/nbd-client.h | 4 +
>> block/nbd-client.c | 304
>> +++--
>> 2 files changed, 255 insertions(+), 53 deletions(-)
>>
>
>> @@ -781,16 +936,21 @@ static int nbd_co_request(BlockDriverState *bs,
>> NBDRequest *request,
>> } else {
>> assert(request->type != NBD_CMD_WRITE);
>> }
>> -ret = nbd_co_send_request(bs, request, write_qiov);
>> -if (ret < 0) {
>> -return ret;
>> -}
>>
>> -ret = nbd_co_receive_return_code(client, request->handle,
>> - _ret, _err);
>> -if (local_err) {
>> -error_report_err(local_err);
>> -}
>> +do {
>> +ret = nbd_co_send_request(bs, request, write_qiov);
>> +if (ret < 0) {
>> +continue;
>> +}
>> +
>> +ret = nbd_co_receive_return_code(client, request->handle,
>> + _ret, _err);
>> +if (local_err) {
>> +error_report_err(local_err);
>> +local_err = NULL;
>
> Conflicts with the conversion to use trace points.
>
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v4 09/10] block/nbd-client: nbd reconnect
On 7/31/18 12:30 PM, Vladimir Sementsov-Ogievskiy wrote:
> Implement reconnect. To achieve this:
>
> 1. add new modes:
>connecting-wait: means, that reconnecting is in progress, and there
> were small number of reconnect attempts, so all requests are
> waiting for the connection.
>connecting-nowait: reconnecting is in progress, there were a lot of
> attempts of reconnect, all requests will return errors.
>
>two old modes are used too:
>connected: normal state
>quit: exiting after fatal error or on close
What makes an error fatal? Without reconnect, life is simple - if the
server sends something we can't parse, we permanently turn the device
into an error condition - because we have no way to get back in sync
with the server for further commands. Your patch allows reconnect
attempts where the connection is down (we failed to send to the server
or failed to receive the server's reply), but why can we not ALSO
attempt to reconnect after a parse error? A reconnect would let us get
back in sync for attempting further commands. You're right that the
current command should probably fail in that case (if the server sent us
garbage for a specific request, it will probably do so again on a repeat
of that request; which is different than when we don't even know what
the server would have sent because of a disconnect).
Or, put another way, we KNOW we have (corner) cases where a mis-aligned
image can currently cause the server to return BLOCK_STATUS replies that
aren't aligned to the advertised minimumm block size. Attempting to
read the last sector of an image then causes the client to see the
misaligned reply and complain, which we are treating as fatal. But why
not instead just fail that particular read, but still attempt a
reconnect, in order to attempt further reads elsewhere in the image that
do not trip up the server's misaligned reply?
>
> Possible transitions are:
>
>* -> quit
>connecting-* -> connected
>connecting-wait -> connecting-nowait (transition is done after
> reconnect-delay seconds in connecting-wait mode)
>connected -> connecting-wait
>
> 2. Implement reconnect in connection_co. So, in connecting-* mode,
> connection_co, tries to reconnect unlimited times.
>
> 3. Retry nbd queries on channel error, if we are in connecting-wait
> state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/nbd-client.h | 4 +
> block/nbd-client.c | 304
> +++--
> 2 files changed, 255 insertions(+), 53 deletions(-)
>
> @@ -781,16 +936,21 @@ static int nbd_co_request(BlockDriverState *bs,
> NBDRequest *request,
> } else {
> assert(request->type != NBD_CMD_WRITE);
> }
> -ret = nbd_co_send_request(bs, request, write_qiov);
> -if (ret < 0) {
> -return ret;
> -}
>
> -ret = nbd_co_receive_return_code(client, request->handle,
> - _ret, _err);
> -if (local_err) {
> -error_report_err(local_err);
> -}
> +do {
> +ret = nbd_co_send_request(bs, request, write_qiov);
> +if (ret < 0) {
> +continue;
> +}
> +
> +ret = nbd_co_receive_return_code(client, request->handle,
> + _ret, _err);
> +if (local_err) {
> +error_report_err(local_err);
> +local_err = NULL;
Conflicts with the conversion to use trace points.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4 09/10] block/nbd-client: nbd reconnect
31.07.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
> Implement reconnect. To achieve this:
>
> 1. add new modes:
> connecting-wait: means, that reconnecting is in progress, and there
> were small number of reconnect attempts, so all requests are
> waiting for the connection.
> connecting-nowait: reconnecting is in progress, there were a lot of
> attempts of reconnect, all requests will return errors.
>
> two old modes are used too:
> connected: normal state
> quit: exiting after fatal error or on close
>
> Possible transitions are:
>
> * -> quit
> connecting-* -> connected
> connecting-wait -> connecting-nowait (transition is done after
>reconnect-delay seconds in connecting-wait mode)
> connected -> connecting-wait
>
> 2. Implement reconnect in connection_co. So, in connecting-* mode,
> connection_co, tries to reconnect unlimited times.
>
> 3. Retry nbd queries on channel error, if we are in connecting-wait
> state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/nbd-client.h | 4 +
> block/nbd-client.c | 304
> +++--
> 2 files changed, 255 insertions(+), 53 deletions(-)
>
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index ef8a6a9239..52e4ec66be 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -40,6 +40,10 @@ typedef struct NBDClientSession {
> Coroutine *connection_co;
> int in_flight;
> NBDClientState state;
> +bool receiving;
> +int connect_status;
> +Error *connect_err;
> +bool wait_in_flight;
>
> NBDClientRequest requests[MAX_NBD_REQUESTS];
> NBDReply reply;
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 41e6e6e702..b09907096d 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -34,10 +34,26 @@
> #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
> #define INDEX_TO_HANDLE(bs, index) ((index) ^ (uint64_t)(intptr_t)(bs))
[...]
> +static coroutine_fn void nbd_reconnect_attempt(NBDConnection *con)
> +{
> +NBDClientSession *s = nbd_get_client_session(con->bs);
> +Error *local_err = NULL;
> +
> +assert(nbd_client_connecting(s));
> +
> +/* Wait completion of all in-flight requests */
> +
> +qemu_co_mutex_lock(>send_mutex);
> +
> +while (s->in_flight > 0) {
> +qemu_co_mutex_unlock(>send_mutex);
> +nbd_recv_coroutines_wake_all(s);
> +s->wait_in_flight = true;
> +qemu_coroutine_yield();
> +s->wait_in_flight = false;
> +qemu_co_mutex_lock(>send_mutex);
> +}
> +
> +qemu_co_mutex_unlock(>send_mutex);
> +
> +/* Now we are sure, that nobody accessing the channel now and nobody
> + * will try to access the channel, until we set state to CONNECTED
> + */
> +
> +/* Finalize previous connection if any */
> +if (s->ioc) {
> +nbd_client_detach_aio_context(con->bs);
> +object_unref(OBJECT(s->sioc));
> +s->sioc = NULL;
> +object_unref(OBJECT(s->ioc));
> +s->ioc = NULL;
> +}
> +
> +s->connect_status = nbd_client_connect(con->bs, con->saddr,
> + con->export, con->tlscreds,
> + con->hostname,
> con->x_dirty_bitmap,
> + _err);
> +error_free(s->connect_err);
> +s->connect_err = NULL;
> +error_propagate(>connect_err, local_err);
> +local_err = NULL;
>
> -nbd_client_detach_aio_context(bs);
> -object_unref(OBJECT(client->sioc));
> -client->sioc = NULL;
> -object_unref(OBJECT(client->ioc));
> -client->ioc = NULL;
> +if (s->connect_status == -EINVAL) {
> +/* Protocol error or something like this, go to NBD_CLIENT_QUIT */
> +nbd_channel_error(s, s->connect_status);
> +return;
Unfortunately, nbd_client_connect returns -EINVAL for io errors instead
of -EIO. And it is not trivial to fix it. So, this if{} should be removed.
> +}
> +
> +if (s->connect_status < 0) {
> +/* failed attempt */
> +return;
> +}
> +
> +/* successfully connected */
> +s->state = NBD_CLIENT_CONNECTED;
> +qemu_co_queue_restart_all(>free_sema);
> +}
> +
--
Best regards,
Vladimir
[Qemu-block] [PATCH v4 09/10] block/nbd-client: nbd reconnect
Implement reconnect. To achieve this:
1. add new modes:
connecting-wait: means, that reconnecting is in progress, and there
were small number of reconnect attempts, so all requests are
waiting for the connection.
connecting-nowait: reconnecting is in progress, there were a lot of
attempts of reconnect, all requests will return errors.
two old modes are used too:
connected: normal state
quit: exiting after fatal error or on close
Possible transitions are:
* -> quit
connecting-* -> connected
connecting-wait -> connecting-nowait (transition is done after
reconnect-delay seconds in connecting-wait mode)
connected -> connecting-wait
2. Implement reconnect in connection_co. So, in connecting-* mode,
connection_co, tries to reconnect unlimited times.
3. Retry nbd queries on channel error, if we are in connecting-wait
state.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd-client.h | 4 +
block/nbd-client.c | 304 +++--
2 files changed, 255 insertions(+), 53 deletions(-)
diff --git a/block/nbd-client.h b/block/nbd-client.h
index ef8a6a9239..52e4ec66be 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -40,6 +40,10 @@ typedef struct NBDClientSession {
Coroutine *connection_co;
int in_flight;
NBDClientState state;
+bool receiving;
+int connect_status;
+Error *connect_err;
+bool wait_in_flight;
NBDClientRequest requests[MAX_NBD_REQUESTS];
NBDReply reply;
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 41e6e6e702..b09907096d 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -34,10 +34,26 @@
#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
#define INDEX_TO_HANDLE(bs, index) ((index) ^ (uint64_t)(intptr_t)(bs))
-/* @ret would be used for reconnect in future */
+static int nbd_client_connect(BlockDriverState *bs,
+ SocketAddress *saddr,
+ const char *export,
+ QCryptoTLSCreds *tlscreds,
+ const char *hostname,
+ const char *x_dirty_bitmap,
+ Error **errp);
+
static void nbd_channel_error(NBDClientSession *s, int ret)
{
-s->state = NBD_CLIENT_QUIT;
+if (ret == -EIO) {
+if (s->state == NBD_CLIENT_CONNECTED) {
+s->state = NBD_CLIENT_CONNECTING_WAIT;
+}
+} else {
+if (s->state == NBD_CLIENT_CONNECTED) {
+qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+s->state = NBD_CLIENT_QUIT;
+}
}
static void nbd_recv_coroutines_wake_all(NBDClientSession *s)
@@ -57,33 +73,151 @@ static void nbd_teardown_connection(BlockDriverState *bs)
{
NBDClientSession *client = nbd_get_client_session(bs);
-assert(client->ioc);
-
-/* finish any pending coroutines */
-qio_channel_shutdown(client->ioc,
- QIO_CHANNEL_SHUTDOWN_BOTH,
- NULL);
+if (client->state == NBD_CLIENT_CONNECTED) {
+/* finish any pending coroutines */
+assert(client->ioc);
+qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+client->state = NBD_CLIENT_QUIT;
BDRV_POLL_WHILE(bs, client->connection_co);
+}
+
+typedef struct NBDConnection {
+BlockDriverState *bs;
+SocketAddress *saddr;
+const char *export;
+QCryptoTLSCreds *tlscreds;
+const char *hostname;
+const char *x_dirty_bitmap;
+uint32_t reconnect_delay;
+} NBDConnection;
+
+static bool nbd_client_connecting(NBDClientSession *client)
+{
+return client->state == NBD_CLIENT_CONNECTING_WAIT ||
+ client->state == NBD_CLIENT_CONNECTING_NOWAIT;
+}
+
+static bool nbd_client_connecting_wait(NBDClientSession *client)
+{
+return client->state == NBD_CLIENT_CONNECTING_WAIT;
+}
+
+static coroutine_fn void nbd_reconnect_attempt(NBDConnection *con)
+{
+NBDClientSession *s = nbd_get_client_session(con->bs);
+Error *local_err = NULL;
+
+assert(nbd_client_connecting(s));
+
+/* Wait completion of all in-flight requests */
+
+qemu_co_mutex_lock(>send_mutex);
+
+while (s->in_flight > 0) {
+qemu_co_mutex_unlock(>send_mutex);
+nbd_recv_coroutines_wake_all(s);
+s->wait_in_flight = true;
+qemu_coroutine_yield();
+s->wait_in_flight = false;
+qemu_co_mutex_lock(>send_mutex);
+}
+
+qemu_co_mutex_unlock(>send_mutex);
+
+/* Now we are sure, that nobody accessing the channel now and nobody
+ * will try to access the channel, until we set state to CONNECTED
+ */
+
+/* Finalize previous connection if any */
+if (s->ioc) {
+nbd_client_detach_aio_context(con->bs);
+object_unref(OBJECT(s->sioc));
+s->sioc = NULL;
+object_unref(OBJECT(s->ioc));
+
