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 <hreitz@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)
-- 
2.34.1

On Fri, Jan 28, 2022 at 04:51:30PM +0100, 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.

Eww.  Good catch; and looks like it is not the first time where we've
run into issues (a quick grep for context through git history of nbd/
finds commit e6cada923).

> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1990835
> Signed-off-by: Hanna Reitz <hreitz@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

As Vladimir is more familiar with the timers needed for reconnecting
an NBD connection, I'm hoping he will chime in.  But yes, your worry
about context changes sounds reasonable.

> 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,

Looks straightforward, but as you say, fleshing out what else may need
similar treatment could make the "real" v1 more interesting.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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 <hreitz@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.

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.

So, I think assertions that both timer pointers are NULL should be enough in attach / detach handlers.

-- 
Best regards,
Vladimir

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 <hreitz@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...

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.

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 <hreitz@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-vsementsov@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

On 01.02.22 12: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 <hreitz@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...
>
> 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).

Looks like the timer isn’t removed when the connection is 
reestablished.  When I add an `assert(!s->reconnect_delay_timer)` to 
`nbd_attach_aio_context()` (on top of this patch), then I get:

$ ./qemu-nbd \
     --fork \
     --pid-file=/tmp/nbd.pid \
     --socket=/tmp/nbd.sock \
     -f raw \
     null-co://

$ (echo '{"execute": "qmp_capabilities"}';
sleep 1;
kill $(cat /tmp/nbd.pid);
./qemu-nbd \
     --fork \
     --pid-file=/tmp/nbd.pid \
     --socket=/tmp/nbd.sock \
     -f raw \
     null-co://;
echo '{"execute": "human-monitor-command",
        "arguments": {"command-line": "qemu-io nbd \"write 0 64k\""}}';
echo '{"execute": "x-blockdev-set-iothread",
        "arguments": {"node-name": "nbd", "iothread": "iothr0"}}') \
| ./qemu-system-x86_64 \
     -qmp stdio \
     -blockdev '{
         "node-name": "nbd",
         "driver": "nbd",
         "reconnect-delay": 1,
         "server": {
             "type": "unix",
             "path": "/tmp/nbd.sock"
         } }' \
     -object iothread,id=iothr0
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 6}, 
"package": "v6.2.0-1288-ge3116c38f7-dirty"}, "capabilities": ["oob"]}}
{"return": {}}
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 00.00 sec (170.326 MiB/sec and 2725.2189 ops/sec)
{"return": ""}
qemu-system-x86_64: ../block/nbd.c:2044: nbd_attach_aio_context: 
Assertion `!s->reconnect_delay_timer' failed.
Aborted (core dumped)


(The above kills the NBD server and immediately starts it, so that the 
following write request will have to reconnect, and immediately 
succeed.  The failed assertion when changing the AioContext shows that 
the timer is still there after successfully reconnecting.)

Not sure whether that’s a problem in normal operation.  On master, 
there’s no failure, of course, the only problem is that 
`reconnect_delay_timer_cb()` will probably be run in the old context.  
If in the new context we then have a concurrent reconnection attempt, 
perhaps the `reconnect_delay_timer_del()` might interfere with 
`reconnect_delay_timer_init()`, such that the former frees the timer 
(and sets it to NULL), and then the `timer_mod()` call in the latter 
function accesses NULL.  But that’d be extremely difficult to test, 
because that’s a very small time window...

I can definitely see the following problem with this RFC patch applied, 
though I don’t quite understand it:

./qemu-nbd \
     --fork \
     --pid-file=/tmp/nbd.pid \
     --socket=/tmp/nbd.sock \
     -f raw \
     null-co://
(echo '{"execute": "qmp_capabilities"}';
sleep 1;
kill $(cat /tmp/nbd.pid);
./qemu-nbd \
     --fork \
     --pid-file=/tmp/nbd.pid \
     --socket=/tmp/nbd.sock \
     -f raw \
     null-co://;
echo '{"execute": "human-monitor-command",
        "arguments": {"command-line": "qemu-io nbd \"write 0 64k\""}}';
echo '{"execute": "x-blockdev-set-iothread",
        "arguments": {"node-name": "nbd", "iothread": "iothr0"}}';
sleep 2;
kill $(cat /tmp/nbd.pid);
./qemu-nbd \
     --fork \
     --pid-file=/tmp/nbd.pid \
     --socket=/tmp/nbd.sock \
     -f raw \
     null-co://;
echo '{"execute": "human-monitor-command",
        "arguments": {"command-line": "qemu-io nbd \"write 0 64k\""}}';
echo '{"execute": "quit"}') \
| ./qemu-system-x86_64 \
     -qmp stdio \
     -blockdev '{
         "node-name": "nbd",
         "driver": "nbd",
         "reconnect-delay": 1,
         "server": {
             "type": "unix",
             "path": "/tmp/nbd.sock"
         } }' \
     -object iothread,id=iothr0
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 6}, 
"package": "v6.2.0-1129-g731bf9ede7"}, "capabilities": ["oob"]}}
{"return": {}}
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 00.00 sec (191.279 MiB/sec and 3060.4719 ops/sec)
{"return": ""}
{"return": {}}
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 00.00 sec (159.672 MiB/sec and 2554.7483 ops/sec)
{"return": ""}
{"return": {}}
{"timestamp": {"seconds": 1643731721, "microseconds": 22290}, "event": 
"SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
qemu-system-x86_64: ../util/qemu-timer.c:115: timerlist_free: Assertion 
`!timerlist_has_timers(timer_list)' failed.
Aborted (core dumped)


I.e.:
1. Kill/restart the NBD server, as above, so that the reconnect on write 
succeeds immediately
2. Move the NBD server to a different AioContext
3. Wait two seconds, so that the reconnect timer expires
4. Repeat step 1, which will install a new reconnect timer
5. Have qemu quit before that new timer instance can expire

I have tried stripping this down to just a single timer instance, but 
didn’t succeed.  I always needed one instance expire in the original 
context, and then start another one in the new context.

01.02.2022 19:14, Hanna Reitz wrote:
>>> 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).
> 
> Looks like the timer isn’t removed when the connection is reestablished.

Oops. And that's wrong.. If connection lost again when old timer is still not fired, assertion in reconnect_delay_timer_init() will fail.

I think we just need reconnect_delay_timer_del() call at the end of nbd_reconnect_attempt() function (after nbd_co_do_establish_connection() call).


-- 
Best regards,
Vladimir