[Qemu-devel] [PATCH] nbd-client: fix hang after server closes connection

[Qemu-devel] [PATCH] nbd-client: fix hang after server closes connection

[Stefan Hajnoczi]

Commit 72b6ffc76653214b69a94a7b1643ff80df134486 ("nbd-client: Fix
regression when server sends garbage") improved NBD client behavior when
the connection enters a broken state.

The following still does not behave as expected:

  $ qemu-nbd -p 1234 -x drive0 -f qcow2 test.qcow2
  $ qemu-system-x86_64 -M accel=kvm -m 1G \
        -drive if=virtio,id=drive0,file=nbd://localhost:1234/drive0,format=raw
  $ pkill qemu-nbd
  (qemu) quit
  ...hang...

QEMU should be able to quit even when the connection was previously
closed by the NBD server.  Currently the nbd_read_reply_entry()
coroutine terminates without letting in-flight requests know that the
connection is dead.

This patch flags the connection as dead so in-flight requests can
complete.

Reported-by: Longxiang Lyu <lolyu@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/nbd-client.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 422ecb4307..5a5fe02015 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -80,6 +80,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
             error_report_err(local_err);
         }
         if (ret <= 0) {
+            s->quit = true;
             break;
         }
 
@@ -107,9 +108,6 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
         qemu_coroutine_yield();
     }
 
-    if (ret < 0) {
-        s->quit = true;
-    }
     nbd_recv_coroutines_enter_all(s);
     s->read_reply_co = NULL;
 }
-- 
2.13.5

Re: [Qemu-devel] [Qemu-block] [PATCH] nbd-client: fix hang after server closes connection

[Stefan Hajnoczi]

On Mon, Aug 21, 2017 at 5:15 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>   (qemu) quit
>   ...hang...

By the way, the same issue is present in QEMU 2.9.  This is not a 2.10
regression.

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH] nbd-client: fix hang after server closes connection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On 08/21/2017 11:27 AM, Stefan Hajnoczi wrote:
> On Mon, Aug 21, 2017 at 5:15 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>   (qemu) quit
>>   ...hang...
> 
> By the way, the same issue is present in QEMU 2.9.  This is not a 2.10
> regression.

Most likely, though, it IS a regression introduced in 2.9 and not
present in 2.8.  I'll add this to my list of 2.11 NBD patches.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] nbd-client: fix hang after server closes connection

[Philippe Mathieu-Daudé]

On 08/21/2017 01:15 PM, Stefan Hajnoczi wrote:
> Commit 72b6ffc76653214b69a94a7b1643ff80df134486 ("nbd-client: Fix
> regression when server sends garbage") improved NBD client behavior when
> the connection enters a broken state.
> 
> The following still does not behave as expected:
> 
>    $ qemu-nbd -p 1234 -x drive0 -f qcow2 test.qcow2
>    $ qemu-system-x86_64 -M accel=kvm -m 1G \
>          -drive if=virtio,id=drive0,file=nbd://localhost:1234/drive0,format=raw
>    $ pkill qemu-nbd
>    (qemu) quit
>    ...hang...
> 
> QEMU should be able to quit even when the connection was previously
> closed by the NBD server.  Currently the nbd_read_reply_entry()
> coroutine terminates without letting in-flight requests know that the
> connection is dead.
> 
> This patch flags the connection as dead so in-flight requests can
> complete.
> 
> Reported-by: Longxiang Lyu <lolyu@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   block/nbd-client.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 422ecb4307..5a5fe02015 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -80,6 +80,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
>               error_report_err(local_err);
>           }
>           if (ret <= 0) {
> +            s->quit = true;
>               break;
>           }
>   
> @@ -107,9 +108,6 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
>           qemu_coroutine_yield();
>       }
>   
> -    if (ret < 0) {
> -        s->quit = true;
> -    }
>       nbd_recv_coroutines_enter_all(s);
>       s->read_reply_co = NULL;
>   }
> 

Re: [Qemu-devel] [Qemu-block] [PATCH] nbd-client: fix hang after server closes connection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On 08/21/2017 02:01 PM, Eric Blake wrote:
> On 08/21/2017 11:27 AM, Stefan Hajnoczi wrote:
>> On Mon, Aug 21, 2017 at 5:15 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>>   (qemu) quit
>>>   ...hang...
>>
>> By the way, the same issue is present in QEMU 2.9.  This is not a 2.10
>> regression.
> 
> Most likely, though, it IS a regression introduced in 2.9 and not
> present in 2.8.  I'll add this to my list of 2.11 NBD patches.

Since it looks like we're doing -rc4 and including other NBD patches for
that, I'm adding this to my 2.10 NBD queue, and will send a pull request
shortly...

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] nbd-client: fix hang after server closes connection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

On 08/23/2017 09:25 AM, Eric Blake wrote:
> On 08/21/2017 02:01 PM, Eric Blake wrote:
>> On 08/21/2017 11:27 AM, Stefan Hajnoczi wrote:
>>> On Mon, Aug 21, 2017 at 5:15 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>>>   (qemu) quit
>>>>   ...hang...
>>>
>>> By the way, the same issue is present in QEMU 2.9.  This is not a 2.10
>>> regression.
>>
>> Most likely, though, it IS a regression introduced in 2.9 and not
>> present in 2.8.  I'll add this to my list of 2.11 NBD patches.
> 
> Since it looks like we're doing -rc4 and including other NBD patches for
> that, I'm adding this to my 2.10 NBD queue,

On IRC, Stefan informed me that 'nbd-client: avoid spurious
qio_channel_yield() re-entry' obsoletes this one.

> and will send a pull request
> shortly...

This part is still true

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] nbd-client: fix hang after server closes connection

[Stefan Hajnoczi]

On Mon, Aug 21, 2017 at 05:15:39PM +0100, Stefan Hajnoczi wrote:
> Commit 72b6ffc76653214b69a94a7b1643ff80df134486 ("nbd-client: Fix
> regression when server sends garbage") improved NBD client behavior when
> the connection enters a broken state.
> 
> The following still does not behave as expected:
> 
>   $ qemu-nbd -p 1234 -x drive0 -f qcow2 test.qcow2
>   $ qemu-system-x86_64 -M accel=kvm -m 1G \
>         -drive if=virtio,id=drive0,file=nbd://localhost:1234/drive0,format=raw
>   $ pkill qemu-nbd
>   (qemu) quit
>   ...hang...
> 
> QEMU should be able to quit even when the connection was previously
> closed by the NBD server.  Currently the nbd_read_reply_entry()
> coroutine terminates without letting in-flight requests know that the
> connection is dead.
> 
> This patch flags the connection as dead so in-flight requests can
> complete.
> 
> Reported-by: Longxiang Lyu <lolyu@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/nbd-client.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

This patch is superceded by "[PATCH] nbd-client: avoid spurious
qio_channel_yield() re-entry".

Stefan