* [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating
@ 2021-01-28 20:14 Roman Kagan
2021-01-28 20:14 ` [PATCH 1/3] block/nbd: only detach existing iochannel from aio_context Roman Kagan
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Roman Kagan @ 2021-01-28 20:14 UTC (permalink / raw
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block, Max Reitz
During the final phase of migration the NBD reconnection logic may
encounter situations it doesn't expect during regular operation.
This series addresses some of them that make qemu crash. They are
reproducible when a vm with a secondary drive attached via nbd with
non-zero "reconnect-delay" runs a stress load (fio with big queue depth)
in the guest on that drive and is migrated (e.g. to a file), while the
nbd server is SIGKILL-ed and restarted every second.
See the individual patches for specific crash conditions and more
detailed explanations.
Roman Kagan (3):
block/nbd: only detach existing iochannel from aio_context
block/nbd: only enter connection coroutine if it's present
nbd: make nbd_read* return -EIO on error
include/block/nbd.h | 7 ++++---
block/nbd.c | 25 +++++++++++++++++--------
2 files changed, 21 insertions(+), 11 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] block/nbd: only detach existing iochannel from aio_context
2021-01-28 20:14 [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Roman Kagan
@ 2021-01-28 20:14 ` Roman Kagan
2021-01-29 5:37 ` Vladimir Sementsov-Ogievskiy
2021-01-28 20:14 ` [PATCH 2/3] block/nbd: only enter connection coroutine if it's present Roman Kagan
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Roman Kagan @ 2021-01-28 20:14 UTC (permalink / raw
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block, Max Reitz
When the reconnect in NBD client is in progress, the iochannel used for
NBD connection doesn't exist. Therefore an attempt to detach it from
the aio_context of the parent BlockDriverState results in a NULL pointer
dereference.
The problem is triggerable, in particular, when an outgoing migration is
about to finish, and stopping the dataplane tries to move the
BlockDriverState from the iothread aio_context to the main loop. If the
NBD connection is lost before this point, and the NBD client has entered
the reconnect procedure, QEMU crashes:
at /build/qemu-gYtjVn/qemu-5.0.1/io/channel.c:452
at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6151
new_context=new_context@entry=0x562a260c9580,
ignore=ignore@entry=0x7feeadc9b780)
at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6230
(bs=bs@entry=0x562a268d6a00, ctx=0x562a260c9580,
ignore_child=<optimized out>, errp=<optimized out>)
at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6332
new_context=0x562a260c9580,
update_root_node=update_root_node@entry=true, errp=errp@entry=0x0)
at /build/qemu-gYtjVn/qemu-5.0.1/block/block-backend.c:1989
new_context=<optimized out>, errp=errp@entry=0x0)
at /build/qemu-gYtjVn/qemu-5.0.1/block/block-backend.c:2010
out>)
at /build/qemu-gYtjVn/qemu-5.0.1/hw/block/dataplane/virtio-blk.c:292
at /build/qemu-gYtjVn/qemu-5.0.1/hw/virtio/virtio-bus.c:245
running=0, state=<optimized out>)
at /build/qemu-gYtjVn/qemu-5.0.1/hw/virtio/virtio.c:3220
state=state@entry=RUN_STATE_FINISH_MIGRATE)
at /build/qemu-gYtjVn/qemu-5.0.1/softmmu/vl.c:1275
send_stop=<optimized out>)
at /build/qemu-gYtjVn/qemu-5.0.1/cpus.c:1032
at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:2914
at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:3275
at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:3439
at /build/qemu-gYtjVn/qemu-5.0.1/util/qemu-thread-posix.c:519
at pthread_create.c:333
oldval=0x562a2452b138, oldlenp=0x0, newval=0x562a2452c5e0
<__func__.28102>, newlen=0)
at ../sysdeps/unix/sysv/linux/sysctl.c:30
Fix it by checking that the iochannel is non-null before trying to
detach it from the aio_context. If it is null, no detaching is needed,
and it will get reattached in the proper aio_context once the connection
is reestablished.
Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
---
block/nbd.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/block/nbd.c b/block/nbd.c
index 42e10c7c93..bcd6641e90 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -235,7 +235,14 @@ static void nbd_client_detach_aio_context(BlockDriverState *bs)
/* Timer is deleted in nbd_client_co_drain_begin() */
assert(!s->reconnect_delay_timer);
- qio_channel_detach_aio_context(QIO_CHANNEL(s->ioc));
+ /*
+ * If reconnect is in progress we may have no ->ioc. It will be
+ * re-instantiated in the proper aio context once the connection is
+ * reestablished.
+ */
+ if (s->ioc) {
+ qio_channel_detach_aio_context(QIO_CHANNEL(s->ioc));
+ }
}
static void nbd_client_attach_aio_context_bh(void *opaque)
--
2.29.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] block/nbd: only enter connection coroutine if it's present
2021-01-28 20:14 [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Roman Kagan
2021-01-28 20:14 ` [PATCH 1/3] block/nbd: only detach existing iochannel from aio_context Roman Kagan
@ 2021-01-28 20:14 ` Roman Kagan
2021-01-29 5:40 ` Vladimir Sementsov-Ogievskiy
2021-01-28 20:14 ` [PATCH 3/3] nbd: make nbd_read* return -EIO on error Roman Kagan
2021-01-29 5:51 ` [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Vladimir Sementsov-Ogievskiy
3 siblings, 1 reply; 11+ messages in thread
From: Roman Kagan @ 2021-01-28 20:14 UTC (permalink / raw
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block, Max Reitz
When an NBD block driver state is moved from one aio_context to another
(e.g. when doing a drain in a migration thread),
nbd_client_attach_aio_context_bh is executed that enters the connection
coroutine.
However, the assumption that ->connection_co is always present here
appears incorrect: the connection may have encountered an error other
than -EIO in the underlying transport, and thus may have decided to quit
rather than keep trying to reconnect, and therefore it may have
terminated the connection coroutine. As a result an attempt to reassign
the client in this state (NBD_CLIENT_QUIT) to a different aio_context
leads to a null pointer dereference:
at /build/qemu-6MF7tq/qemu-5.0.1/util/qemu-coroutine.c:109
opaque=0x561805ed4c00) at /build/qemu-6MF7tq/qemu-5.0.1/block/nbd.c:164
at /build/qemu-6MF7tq/qemu-5.0.1/util/aio-wait.c:55
at /build/qemu-6MF7tq/qemu-5.0.1/util/async.c:136
at /build/qemu-6MF7tq/qemu-5.0.1/util/async.c:164
blocking=blocking@entry=true)
at /build/qemu-6MF7tq/qemu-5.0.1/util/aio-posix.c:650
cb=<optimized out>, opaque=<optimized out>)
at /build/qemu-6MF7tq/qemu-5.0.1/util/aio-wait.c:71
bs=0x561805ed4c00) at /build/qemu-6MF7tq/qemu-5.0.1/block.c:6172
new_context=new_context@entry=0x5618056c7580,
ignore=ignore@entry=0x7f60e1e63780)
at /build/qemu-6MF7tq/qemu-5.0.1/block.c:6237
bs=bs@entry=0x561805ed4c00, ctx=0x5618056c7580,
ignore_child=<optimized out>, errp=<optimized out>)
at /build/qemu-6MF7tq/qemu-5.0.1/block.c:6332
new_context=0x5618056c7580, update_root_node=update_root_node@entry=true,
errp=errp@entry=0x0)
at /build/qemu-6MF7tq/qemu-5.0.1/block/block-backend.c:1989
new_context=<optimized out>, errp=errp@entry=0x0)
at /build/qemu-6MF7tq/qemu-5.0.1/block/block-backend.c:2010
at /build/qemu-6MF7tq/qemu-5.0.1/hw/block/dataplane/virtio-blk.c:292
at /build/qemu-6MF7tq/qemu-5.0.1/hw/virtio/virtio-bus.c:245
running=0, state=<optimized out>)
at /build/qemu-6MF7tq/qemu-5.0.1/hw/virtio/virtio.c:3220
state=state@entry=RUN_STATE_FINISH_MIGRATE)
at /build/qemu-6MF7tq/qemu-5.0.1/softmmu/vl.c:1275
send_stop=<optimized out>) at /build/qemu-6MF7tq/qemu-5.0.1/cpus.c:1032
at /build/qemu-6MF7tq/qemu-5.0.1/migration/migration.c:2914
at /build/qemu-6MF7tq/qemu-5.0.1/migration/migration.c:3275
at /build/qemu-6MF7tq/qemu-5.0.1/migration/migration.c:3439
at /build/qemu-6MF7tq/qemu-5.0.1/util/qemu-thread-posix.c:519
from /lib/x86_64-linux-gnu/libpthread.so.0
Fix it by checking that the connection coroutine is non-null before
trying to enter it. If it is null, no entering is needed, as the
connection is probably going down anyway.
Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
---
block/nbd.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index bcd6641e90..b3cbbeb4b0 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -250,13 +250,15 @@ static void nbd_client_attach_aio_context_bh(void *opaque)
BlockDriverState *bs = opaque;
BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
- /*
- * The node is still drained, so we know the coroutine has yielded in
- * nbd_read_eof(), the only place where bs->in_flight can reach 0, or it is
- * entered for the first time. Both places are safe for entering the
- * coroutine.
- */
- qemu_aio_coroutine_enter(bs->aio_context, s->connection_co);
+ if (s->connection_co) {
+ /*
+ * The node is still drained, so we know the coroutine has yielded in
+ * nbd_read_eof(), the only place where bs->in_flight can reach 0, or
+ * it is entered for the first time. Both places are safe for entering
+ * the coroutine.
+ */
+ qemu_aio_coroutine_enter(bs->aio_context, s->connection_co);
+ }
bdrv_dec_in_flight(bs);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] nbd: make nbd_read* return -EIO on error
2021-01-28 20:14 [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Roman Kagan
2021-01-28 20:14 ` [PATCH 1/3] block/nbd: only detach existing iochannel from aio_context Roman Kagan
2021-01-28 20:14 ` [PATCH 2/3] block/nbd: only enter connection coroutine if it's present Roman Kagan
@ 2021-01-28 20:14 ` Roman Kagan
2021-01-29 5:48 ` Vladimir Sementsov-Ogievskiy
2021-01-29 5:51 ` [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Vladimir Sementsov-Ogievskiy
3 siblings, 1 reply; 11+ messages in thread
From: Roman Kagan @ 2021-01-28 20:14 UTC (permalink / raw
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block, Max Reitz
NBD reconnect logic considers the error code from the functions that
read NBD messages to tell if reconnect should be attempted or not: it is
attempted on -EIO, otherwise the client transitions to NBD_CLIENT_QUIT
state (see nbd_channel_error). This error code is propagated from the
primitives like nbd_read.
The problem, however, is that nbd_read itself turns every error into -1
rather than -EIO. As a result, if the NBD server happens to die while
sending the message, the client in QEMU receives less data than it
expects, considers it as a fatal error, and wouldn't attempt
reestablishing the connection.
Fix it by turning every negative return from qio_channel_read_all into
-EIO returned from nbd_read. Apparently that was the original behavior,
but got broken later. Also adjust nbd_readXX to follow.
Fixes: e6798f06a6 ("nbd: generalize usage of nbd_read")
Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
---
include/block/nbd.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4a52a43ef5..5f34d23bb0 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -364,7 +364,7 @@ static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
if (desc) {
error_prepend(errp, "Failed to read %s: ", desc);
}
- return -1;
+ return ret;
}
return 0;
@@ -375,8 +375,9 @@ static inline int nbd_read##bits(QIOChannel *ioc, \
uint##bits##_t *val, \
const char *desc, Error **errp) \
{ \
- if (nbd_read(ioc, val, sizeof(*val), desc, errp) < 0) { \
- return -1; \
+ int ret = nbd_read(ioc, val, sizeof(*val), desc, errp); \
+ if (ret < 0) { \
+ return ret; \
} \
*val = be##bits##_to_cpu(*val); \
return 0; \
--
2.29.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] block/nbd: only detach existing iochannel from aio_context
2021-01-28 20:14 ` [PATCH 1/3] block/nbd: only detach existing iochannel from aio_context Roman Kagan
@ 2021-01-29 5:37 ` Vladimir Sementsov-Ogievskiy
2021-01-29 7:03 ` Roman Kagan
0 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-29 5:37 UTC (permalink / raw
To: Roman Kagan, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
28.01.2021 23:14, Roman Kagan wrote:
> When the reconnect in NBD client is in progress, the iochannel used for
> NBD connection doesn't exist. Therefore an attempt to detach it from
> the aio_context of the parent BlockDriverState results in a NULL pointer
> dereference.
>
> The problem is triggerable, in particular, when an outgoing migration is
> about to finish, and stopping the dataplane tries to move the
> BlockDriverState from the iothread aio_context to the main loop. If the
> NBD connection is lost before this point, and the NBD client has entered
> the reconnect procedure, QEMU crashes:
>
> at /build/qemu-gYtjVn/qemu-5.0.1/io/channel.c:452
> at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6151
> new_context=new_context@entry=0x562a260c9580,
> ignore=ignore@entry=0x7feeadc9b780)
> at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6230
> (bs=bs@entry=0x562a268d6a00, ctx=0x562a260c9580,
> ignore_child=<optimized out>, errp=<optimized out>)
> at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6332
> new_context=0x562a260c9580,
> update_root_node=update_root_node@entry=true, errp=errp@entry=0x0)
> at /build/qemu-gYtjVn/qemu-5.0.1/block/block-backend.c:1989
> new_context=<optimized out>, errp=errp@entry=0x0)
> at /build/qemu-gYtjVn/qemu-5.0.1/block/block-backend.c:2010
> out>)
> at /build/qemu-gYtjVn/qemu-5.0.1/hw/block/dataplane/virtio-blk.c:292
> at /build/qemu-gYtjVn/qemu-5.0.1/hw/virtio/virtio-bus.c:245
> running=0, state=<optimized out>)
> at /build/qemu-gYtjVn/qemu-5.0.1/hw/virtio/virtio.c:3220
> state=state@entry=RUN_STATE_FINISH_MIGRATE)
> at /build/qemu-gYtjVn/qemu-5.0.1/softmmu/vl.c:1275
> send_stop=<optimized out>)
> at /build/qemu-gYtjVn/qemu-5.0.1/cpus.c:1032
> at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:2914
> at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:3275
> at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:3439
> at /build/qemu-gYtjVn/qemu-5.0.1/util/qemu-thread-posix.c:519
> at pthread_create.c:333
> oldval=0x562a2452b138, oldlenp=0x0, newval=0x562a2452c5e0
> <__func__.28102>, newlen=0)
> at ../sysdeps/unix/sysv/linux/sysctl.c:30
function names are somehow lost :(
>
> Fix it by checking that the iochannel is non-null before trying to
> detach it from the aio_context. If it is null, no detaching is needed,
> and it will get reattached in the proper aio_context once the connection
> is reestablished.
>
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
Thanks!
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/nbd.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 42e10c7c93..bcd6641e90 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -235,7 +235,14 @@ static void nbd_client_detach_aio_context(BlockDriverState *bs)
>
> /* Timer is deleted in nbd_client_co_drain_begin() */
> assert(!s->reconnect_delay_timer);
> - qio_channel_detach_aio_context(QIO_CHANNEL(s->ioc));
> + /*
> + * If reconnect is in progress we may have no ->ioc. It will be
> + * re-instantiated in the proper aio context once the connection is
> + * reestablished.
> + */
> + if (s->ioc) {
> + qio_channel_detach_aio_context(QIO_CHANNEL(s->ioc));
> + }
> }
>
> static void nbd_client_attach_aio_context_bh(void *opaque)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] block/nbd: only enter connection coroutine if it's present
2021-01-28 20:14 ` [PATCH 2/3] block/nbd: only enter connection coroutine if it's present Roman Kagan
@ 2021-01-29 5:40 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-29 5:40 UTC (permalink / raw
To: Roman Kagan, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
28.01.2021 23:14, Roman Kagan wrote:
> When an NBD block driver state is moved from one aio_context to another
> (e.g. when doing a drain in a migration thread),
> nbd_client_attach_aio_context_bh is executed that enters the connection
> coroutine.
>
> However, the assumption that ->connection_co is always present here
> appears incorrect: the connection may have encountered an error other
> than -EIO in the underlying transport, and thus may have decided to quit
> rather than keep trying to reconnect, and therefore it may have
> terminated the connection coroutine. As a result an attempt to reassign
> the client in this state (NBD_CLIENT_QUIT) to a different aio_context
> leads to a null pointer dereference:
>
> at /build/qemu-6MF7tq/qemu-5.0.1/util/qemu-coroutine.c:109
> opaque=0x561805ed4c00) at /build/qemu-6MF7tq/qemu-5.0.1/block/nbd.c:164
> at /build/qemu-6MF7tq/qemu-5.0.1/util/aio-wait.c:55
> at /build/qemu-6MF7tq/qemu-5.0.1/util/async.c:136
> at /build/qemu-6MF7tq/qemu-5.0.1/util/async.c:164
> blocking=blocking@entry=true)
> at /build/qemu-6MF7tq/qemu-5.0.1/util/aio-posix.c:650
> cb=<optimized out>, opaque=<optimized out>)
> at /build/qemu-6MF7tq/qemu-5.0.1/util/aio-wait.c:71
> bs=0x561805ed4c00) at /build/qemu-6MF7tq/qemu-5.0.1/block.c:6172
> new_context=new_context@entry=0x5618056c7580,
> ignore=ignore@entry=0x7f60e1e63780)
> at /build/qemu-6MF7tq/qemu-5.0.1/block.c:6237
> bs=bs@entry=0x561805ed4c00, ctx=0x5618056c7580,
> ignore_child=<optimized out>, errp=<optimized out>)
> at /build/qemu-6MF7tq/qemu-5.0.1/block.c:6332
> new_context=0x5618056c7580, update_root_node=update_root_node@entry=true,
> errp=errp@entry=0x0)
> at /build/qemu-6MF7tq/qemu-5.0.1/block/block-backend.c:1989
> new_context=<optimized out>, errp=errp@entry=0x0)
> at /build/qemu-6MF7tq/qemu-5.0.1/block/block-backend.c:2010
> at /build/qemu-6MF7tq/qemu-5.0.1/hw/block/dataplane/virtio-blk.c:292
> at /build/qemu-6MF7tq/qemu-5.0.1/hw/virtio/virtio-bus.c:245
> running=0, state=<optimized out>)
> at /build/qemu-6MF7tq/qemu-5.0.1/hw/virtio/virtio.c:3220
> state=state@entry=RUN_STATE_FINISH_MIGRATE)
> at /build/qemu-6MF7tq/qemu-5.0.1/softmmu/vl.c:1275
> send_stop=<optimized out>) at /build/qemu-6MF7tq/qemu-5.0.1/cpus.c:1032
> at /build/qemu-6MF7tq/qemu-5.0.1/migration/migration.c:2914
> at /build/qemu-6MF7tq/qemu-5.0.1/migration/migration.c:3275
> at /build/qemu-6MF7tq/qemu-5.0.1/migration/migration.c:3439
> at /build/qemu-6MF7tq/qemu-5.0.1/util/qemu-thread-posix.c:519
> from /lib/x86_64-linux-gnu/libpthread.so.0
>
> Fix it by checking that the connection coroutine is non-null before
> trying to enter it. If it is null, no entering is needed, as the
> connection is probably going down anyway.
>
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/nbd.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index bcd6641e90..b3cbbeb4b0 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -250,13 +250,15 @@ static void nbd_client_attach_aio_context_bh(void *opaque)
> BlockDriverState *bs = opaque;
> BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
>
> - /*
> - * The node is still drained, so we know the coroutine has yielded in
> - * nbd_read_eof(), the only place where bs->in_flight can reach 0, or it is
> - * entered for the first time. Both places are safe for entering the
> - * coroutine.
> - */
> - qemu_aio_coroutine_enter(bs->aio_context, s->connection_co);
> + if (s->connection_co) {
> + /*
> + * The node is still drained, so we know the coroutine has yielded in
> + * nbd_read_eof(), the only place where bs->in_flight can reach 0, or
> + * it is entered for the first time. Both places are safe for entering
> + * the coroutine.
> + */
> + qemu_aio_coroutine_enter(bs->aio_context, s->connection_co);
> + }
> bdrv_dec_in_flight(bs);
> }
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] nbd: make nbd_read* return -EIO on error
2021-01-28 20:14 ` [PATCH 3/3] nbd: make nbd_read* return -EIO on error Roman Kagan
@ 2021-01-29 5:48 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-29 5:48 UTC (permalink / raw
To: Roman Kagan, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
28.01.2021 23:14, Roman Kagan wrote:
> NBD reconnect logic considers the error code from the functions that
> read NBD messages to tell if reconnect should be attempted or not: it is
> attempted on -EIO, otherwise the client transitions to NBD_CLIENT_QUIT
> state (see nbd_channel_error). This error code is propagated from the
> primitives like nbd_read.
>
> The problem, however, is that nbd_read itself turns every error into -1
> rather than -EIO. As a result, if the NBD server happens to die while
> sending the message, the client in QEMU receives less data than it
> expects, considers it as a fatal error, and wouldn't attempt
> reestablishing the connection.
>
> Fix it by turning every negative return from qio_channel_read_all into
> -EIO returned from nbd_read. Apparently that was the original behavior,
> but got broken later. Also adjust nbd_readXX to follow.
>
> Fixes: e6798f06a6 ("nbd: generalize usage of nbd_read")
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Really looks like a bug in e6798f06a6: it changes error code from -EIO to -1 without any reasoning.
> ---
> include/block/nbd.h | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4a52a43ef5..5f34d23bb0 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -364,7 +364,7 @@ static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
> if (desc) {
> error_prepend(errp, "Failed to read %s: ", desc);
> }
> - return -1;
> + return ret;
> }
>
> return 0;
> @@ -375,8 +375,9 @@ static inline int nbd_read##bits(QIOChannel *ioc, \
> uint##bits##_t *val, \
> const char *desc, Error **errp) \
> { \
> - if (nbd_read(ioc, val, sizeof(*val), desc, errp) < 0) { \
> - return -1; \
> + int ret = nbd_read(ioc, val, sizeof(*val), desc, errp); \
> + if (ret < 0) { \
> + return ret; \
> } \
> *val = be##bits##_to_cpu(*val); \
> return 0; \
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating
2021-01-28 20:14 [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Roman Kagan
` (2 preceding siblings ...)
2021-01-28 20:14 ` [PATCH 3/3] nbd: make nbd_read* return -EIO on error Roman Kagan
@ 2021-01-29 5:51 ` Vladimir Sementsov-Ogievskiy
2021-01-29 7:35 ` Roman Kagan
3 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-29 5:51 UTC (permalink / raw
To: Roman Kagan, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
28.01.2021 23:14, Roman Kagan wrote:
> During the final phase of migration the NBD reconnection logic may
> encounter situations it doesn't expect during regular operation.
>
> This series addresses some of them that make qemu crash. They are
> reproducible when a vm with a secondary drive attached via nbd with
> non-zero "reconnect-delay" runs a stress load (fio with big queue depth)
> in the guest on that drive and is migrated (e.g. to a file), while the
> nbd server is SIGKILL-ed and restarted every second.
>
> See the individual patches for specific crash conditions and more
> detailed explanations.
>
> Roman Kagan (3):
> block/nbd: only detach existing iochannel from aio_context
> block/nbd: only enter connection coroutine if it's present
> nbd: make nbd_read* return -EIO on error
>
> include/block/nbd.h | 7 ++++---
> block/nbd.c | 25 +++++++++++++++++--------
> 2 files changed, 21 insertions(+), 11 deletions(-)
>
Thanks a lot for fixing!
Do you have some reproducer scripts? Could you post them or may be add an iotest?
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] block/nbd: only detach existing iochannel from aio_context
2021-01-29 5:37 ` Vladimir Sementsov-Ogievskiy
@ 2021-01-29 7:03 ` Roman Kagan
0 siblings, 0 replies; 11+ messages in thread
From: Roman Kagan @ 2021-01-29 7:03 UTC (permalink / raw
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, qemu-devel, qemu-block, Max Reitz
On Fri, Jan 29, 2021 at 08:37:13AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 28.01.2021 23:14, Roman Kagan wrote:
> > When the reconnect in NBD client is in progress, the iochannel used for
> > NBD connection doesn't exist. Therefore an attempt to detach it from
> > the aio_context of the parent BlockDriverState results in a NULL pointer
> > dereference.
> >
> > The problem is triggerable, in particular, when an outgoing migration is
> > about to finish, and stopping the dataplane tries to move the
> > BlockDriverState from the iothread aio_context to the main loop. If the
> > NBD connection is lost before this point, and the NBD client has entered
> > the reconnect procedure, QEMU crashes:
> >
> > at /build/qemu-gYtjVn/qemu-5.0.1/io/channel.c:452
> > at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6151
> > new_context=new_context@entry=0x562a260c9580,
> > ignore=ignore@entry=0x7feeadc9b780)
> > at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6230
> > (bs=bs@entry=0x562a268d6a00, ctx=0x562a260c9580,
> > ignore_child=<optimized out>, errp=<optimized out>)
> > at /build/qemu-gYtjVn/qemu-5.0.1/block.c:6332
> > new_context=0x562a260c9580,
> > update_root_node=update_root_node@entry=true, errp=errp@entry=0x0)
> > at /build/qemu-gYtjVn/qemu-5.0.1/block/block-backend.c:1989
> > new_context=<optimized out>, errp=errp@entry=0x0)
> > at /build/qemu-gYtjVn/qemu-5.0.1/block/block-backend.c:2010
> > out>)
> > at /build/qemu-gYtjVn/qemu-5.0.1/hw/block/dataplane/virtio-blk.c:292
> > at /build/qemu-gYtjVn/qemu-5.0.1/hw/virtio/virtio-bus.c:245
> > running=0, state=<optimized out>)
> > at /build/qemu-gYtjVn/qemu-5.0.1/hw/virtio/virtio.c:3220
> > state=state@entry=RUN_STATE_FINISH_MIGRATE)
> > at /build/qemu-gYtjVn/qemu-5.0.1/softmmu/vl.c:1275
> > send_stop=<optimized out>)
> > at /build/qemu-gYtjVn/qemu-5.0.1/cpus.c:1032
> > at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:2914
> > at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:3275
> > at /build/qemu-gYtjVn/qemu-5.0.1/migration/migration.c:3439
> > at /build/qemu-gYtjVn/qemu-5.0.1/util/qemu-thread-posix.c:519
> > at pthread_create.c:333
> > oldval=0x562a2452b138, oldlenp=0x0, newval=0x562a2452c5e0
> > <__func__.28102>, newlen=0)
> > at ../sysdeps/unix/sysv/linux/sysctl.c:30
>
> function names are somehow lost :(
Oops. Backtraces in gdb have frame numbers prefixed with a hash which
got interpreted as comments by git commit; I should have offset the
backtrace when pasting.
Will respin with fixed log messages, thanks.
Roman.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating
2021-01-29 5:51 ` [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Vladimir Sementsov-Ogievskiy
@ 2021-01-29 7:35 ` Roman Kagan
2021-01-29 8:19 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 11+ messages in thread
From: Roman Kagan @ 2021-01-29 7:35 UTC (permalink / raw
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, qemu-devel, qemu-block, Max Reitz
On Fri, Jan 29, 2021 at 08:51:39AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 28.01.2021 23:14, Roman Kagan wrote:
> > During the final phase of migration the NBD reconnection logic may
> > encounter situations it doesn't expect during regular operation.
> >
> > This series addresses some of them that make qemu crash. They are
> > reproducible when a vm with a secondary drive attached via nbd with
> > non-zero "reconnect-delay" runs a stress load (fio with big queue depth)
> > in the guest on that drive and is migrated (e.g. to a file), while the
> > nbd server is SIGKILL-ed and restarted every second.
> >
> > See the individual patches for specific crash conditions and more
> > detailed explanations.
> >
> > Roman Kagan (3):
> > block/nbd: only detach existing iochannel from aio_context
> > block/nbd: only enter connection coroutine if it's present
> > nbd: make nbd_read* return -EIO on error
> >
> > include/block/nbd.h | 7 ++++---
> > block/nbd.c | 25 +++++++++++++++++--------
> > 2 files changed, 21 insertions(+), 11 deletions(-)
> >
>
> Thanks a lot for fixing!
>
> Do you have some reproducer scripts? Could you post them or may be add
> an iotest?
I don't have it scripted, just ad hoc command lines. I'll look into
making up a test. Can you perhaps suggest what existing test to base
on?
Thanks,
Roman.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating
2021-01-29 7:35 ` Roman Kagan
@ 2021-01-29 8:19 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-29 8:19 UTC (permalink / raw
To: Roman Kagan, qemu-devel, Eric Blake, Max Reitz, qemu-block,
Kevin Wolf
29.01.2021 10:35, Roman Kagan wrote:
> On Fri, Jan 29, 2021 at 08:51:39AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 28.01.2021 23:14, Roman Kagan wrote:
>>> During the final phase of migration the NBD reconnection logic may
>>> encounter situations it doesn't expect during regular operation.
>>>
>>> This series addresses some of them that make qemu crash. They are
>>> reproducible when a vm with a secondary drive attached via nbd with
>>> non-zero "reconnect-delay" runs a stress load (fio with big queue depth)
>>> in the guest on that drive and is migrated (e.g. to a file), while the
>>> nbd server is SIGKILL-ed and restarted every second.
>>>
>>> See the individual patches for specific crash conditions and more
>>> detailed explanations.
>>>
>>> Roman Kagan (3):
>>> block/nbd: only detach existing iochannel from aio_context
>>> block/nbd: only enter connection coroutine if it's present
>>> nbd: make nbd_read* return -EIO on error
>>>
>>> include/block/nbd.h | 7 ++++---
>>> block/nbd.c | 25 +++++++++++++++++--------
>>> 2 files changed, 21 insertions(+), 11 deletions(-)
>>>
>>
>> Thanks a lot for fixing!
>>
>> Do you have some reproducer scripts? Could you post them or may be add
>> an iotest?
>
> I don't have it scripted, just ad hoc command lines. I'll look into
> making up a test. Can you perhaps suggest what existing test to base
> on?
>
For now reconnect feature is covered only by two tests tests/qemu-iotests/264 and tests/qemu-iotests/277.
Also note, that since "f203080bbd iotests: rewrite check into python" you should add new iotests with human-readable file names into tests/qemu-iotests/tests subdirectory. Also you don't need to update tests/qemu-iotests/group file (it's absent now), test groups are defined in tests themselves in a comment, like "# group: rw quick".
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-01-29 8:20 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-28 20:14 [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Roman Kagan
2021-01-28 20:14 ` [PATCH 1/3] block/nbd: only detach existing iochannel from aio_context Roman Kagan
2021-01-29 5:37 ` Vladimir Sementsov-Ogievskiy
2021-01-29 7:03 ` Roman Kagan
2021-01-28 20:14 ` [PATCH 2/3] block/nbd: only enter connection coroutine if it's present Roman Kagan
2021-01-29 5:40 ` Vladimir Sementsov-Ogievskiy
2021-01-28 20:14 ` [PATCH 3/3] nbd: make nbd_read* return -EIO on error Roman Kagan
2021-01-29 5:48 ` Vladimir Sementsov-Ogievskiy
2021-01-29 5:51 ` [PATCH 0/3] block/nbd: fix crashers in reconnect while migrating Vladimir Sementsov-Ogievskiy
2021-01-29 7:35 ` Roman Kagan
2021-01-29 8:19 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.