01.02.2022 14:40, Hanna Reitz wrote:
On 01.02.22 12:18, Vladimir Sementsov-Ogievskiy wrote:
28.01.2022 18:51, Hanna Reitz wrote:
s->ioc must always be attached to the NBD node's AioContext. If that
context changes, s->ioc must be attached to the new context.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1990835
Signed-off-by: Hanna Reitz <hre...@redhat.com>
---
This is an RFC because I believe there are some other things in the NBD
block driver that need attention on an AioContext change, too. Namely,
there are two timers (reconnect_delay_timer and open_timer) that are
also attached to the node's AioContext, and I'm afraid they need to be
handled, too. Probably pause them on detach, and resume them on attach,
but I'm not sure, which is why I'm posting this as an RFC to get some
comments from that from someone who knows this code better than me. :)
(Also, in a real v1, of course I'd want to add a regression test.)
---
block/nbd.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/block/nbd.c b/block/nbd.c
index 63dbfa807d..119a774c04 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -2036,6 +2036,25 @@ static void nbd_cancel_in_flight(BlockDriverState *bs)
nbd_co_establish_connection_cancel(s->conn);
}
+static void nbd_attach_aio_context(BlockDriverState *bs,
+ AioContext *new_context)
+{
+ BDRVNBDState *s = bs->opaque;
+
+ if (s->ioc) {
+ qio_channel_attach_aio_context(s->ioc, new_context);
+ }
+}
+
+static void nbd_detach_aio_context(BlockDriverState *bs)
+{
+ BDRVNBDState *s = bs->opaque;
+
+ if (s->ioc) {
+ qio_channel_detach_aio_context(s->ioc);
+ }
+}
+
static BlockDriver bdrv_nbd = {
.format_name = "nbd",
.protocol_name = "nbd",
@@ -2059,6 +2078,9 @@ static BlockDriver bdrv_nbd = {
.bdrv_dirname = nbd_dirname,
.strong_runtime_opts = nbd_strong_runtime_opts,
.bdrv_cancel_in_flight = nbd_cancel_in_flight,
+
+ .bdrv_attach_aio_context = nbd_attach_aio_context,
+ .bdrv_detach_aio_context = nbd_detach_aio_context,
};
static BlockDriver bdrv_nbd_tcp = {
@@ -2084,6 +2106,9 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_dirname = nbd_dirname,
.strong_runtime_opts = nbd_strong_runtime_opts,
.bdrv_cancel_in_flight = nbd_cancel_in_flight,
+
+ .bdrv_attach_aio_context = nbd_attach_aio_context,
+ .bdrv_detach_aio_context = nbd_detach_aio_context,
};
static BlockDriver bdrv_nbd_unix = {
@@ -2109,6 +2134,9 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_dirname = nbd_dirname,
.strong_runtime_opts = nbd_strong_runtime_opts,
.bdrv_cancel_in_flight = nbd_cancel_in_flight,
+
+ .bdrv_attach_aio_context = nbd_attach_aio_context,
+ .bdrv_detach_aio_context = nbd_detach_aio_context,
};
static void bdrv_nbd_init(void)
Hmm. I was so happy to remove these handlers together with connection-coroutine
:) . But you are right, seems I've removed too much :(.
open_timer exists only during bdrv_open() handler, so, I hope on attach/detach
it should not exist.
That’s… kind of surprising. It’s good for me here, but as far as I can see it
means that all of qemu blocks until the connection succeeds, right? That
doesn’t seem quite ideal...
Right. Still the intended usage was for command-line, so we can wait for
connection on Qemu start.
Using it in blockdev-add when vm is running is doubt-able. In v3 I had a patch
to make blockdev-add a coroutine qmp command to solve this problem. But it
raised a discussion and I decided that it's not a reason to block the whole
feature.
https://patchwork.kernel.org/project/qemu-devel/patch/20210906190654.183421-3-vsement...@virtuozzo.com/
Anyway, good for me. O:)
reconnect_delay_timer should exist only during IO request: it's created during
request if we don't have a connection. And request will not finish until timer
elapsed or connection established (timer should be removed in this case too).
So, again, when attaching / detaching the context we should be in a drained
sections, so no in-flight requests and no reconnect_delay_timer.
Got it. FWIW, other block drivers rely on this, too (e.g. null-aio with
latency-ns set creates a timer in every I/O request and settles the request
once the timer expires).
So, I think assertions that both timer pointers are NULL should be enough in
attach / detach handlers.
Great! I’ll cook up v1.
--
Best regards,
Vladimir
