Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Daniel P . Berrangé]

On Sat, Nov 17, 2018 at 08:19:10PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > When sending a NBD_CMD_DISC message there is no reply expected,
> > however, the nbd_read_eof() coroutine is still waiting for a reply.
> > In a plain NBD connection this doesn't matter as it will just get an
> > EOF, however, on a TLS connection it will get an interrupted TLS data
> > packet. The nbd_read_eof() will then print an error message on the
> > console due to missing reply to NBD_CMD_DISC.
> > 
> > This can be seen with qemu-img
> > 
> >    $ qemu-img info \
> >        --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
> >        --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
> >    qemu-img: Cannot read from TLS channel: Input/output error
> >    image: nbd://127.0.0.1:9000
> >    file format: nbd
> >    virtual size: 10M (10485760 bytes)
> >    disk size: unavailable
> > 
> > Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
> > get the coroutine to stop waiting for a reply and thus supress the error
> > message.
> 
> Actually, it's not quite enough - once you actually start performing I/O,
> enough coroutines are kicked off that the error still happens:

Hmm, does the client not send requests synchronously  ? I expected that
any other I/O operations would have already received their reply by the
time we sent the DISC command.

> 
> $  qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
> --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\
>  driver=nbd,host=localhost,port=10809,tls-creds=tls0
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec)
> wrote 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec)
> Cannot read from TLS channel: Input/output error
> 
> Squashing this in on top of your patch helps, though:
> 
> diff --git i/block/nbd-client.c w/block/nbd-client.c
> index 5f63e4b8f15..e7916c78996 100644
> --- i/block/nbd-client.c
> +++ w/block/nbd-client.c
> @@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void
> *opaque)
>          assert(s->reply.handle == 0);
>          ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
>          if (local_err) {
> -            error_report_err(local_err);
> +            /* If we are already quitting, either another error has
> +             * already been reported, or we requested NBD_CMD_DISC and
> +             * don't need to report anything further.  */
> +            if (!s->quit) {
> +                error_report_err(local_err);
> +            } else {
> +                error_free(local_err);
> +            }
>          }
>          if (ret <= 0) {
>              break;
> 
> But I want to do more testing to make sure I'm not missing out on reporting
> an actual error if I add that.

Yes, I'd be a little concerned about missing reporting of an error from
a command other than NBD_CMD_DISC if we did this.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|