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

Stefan Hajnoczi posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170821161539.11066-1-stefanha@redhat.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
block/nbd-client.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
[Qemu-devel] [PATCH] nbd-client: fix hang after server closes connection
Posted by Stefan Hajnoczi 6 years, 7 months ago
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
Posted by Stefan Hajnoczi 6 years, 7 months ago
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
Posted by Eric Blake 6 years, 7 months ago
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

Re: [Qemu-devel] [Qemu-block] [PATCH] nbd-client: fix hang after server closes connection
Posted by Eric Blake 6 years, 7 months ago
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

Re: [Qemu-devel] [Qemu-block] [PATCH] nbd-client: fix hang after server closes connection
Posted by Eric Blake 6 years, 7 months ago
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

Re: [Qemu-devel] [PATCH] nbd-client: fix hang after server closes connection
Posted by Philippe Mathieu-Daudé 6 years, 7 months ago
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] [PATCH] nbd-client: fix hang after server closes connection
Posted by Stefan Hajnoczi 6 years, 7 months ago
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