[PATCH for-6.2 0/2] NBD 6.2-rc fixes

[PATCH for-6.2 0/2] NBD 6.2-rc fixes

[Eric Blake]

Back in September, Rich proposed a patch to silence an EPIPE message
from qemu-nbd when used with Unix sockets:
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg03359.html

But investigating that further, I found that we had a different
message with TCP sockets, and that we regressed in qemu 6.0 with
regards to the message we print due to the use of uninitialized
memory.  Fixing the uninitialized memory use happens to also silence
the message that Rich was seeing, but without needing to special-case
EPIPE.

I also noticed that even though commit 2800637a and friends made the
block layer support 64-bit zero/trim, we are still manually splitting
3G requests in the NBD driver.  Patch 2 fixes that, although I'm less
certain whether it counts as 6.2-rc material since it is merely a
minor performance tweak to a feature new to 6.2, rather than a
regression fix.

Eric Blake (2):
  nbd/server: Don't complain on certain client disconnects
  nbd/server: Simplify zero and trim

 nbd/server.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

-- 
2.33.1

[PATCH for-6.2 1/2] nbd/server: Don't complain on certain client disconnects

[Eric Blake]

When a client disconnects abruptly, but did not have any pending
requests (for example, when using nbdsh without calling h.shutdown),
we used to output the following message:

$ qemu-nbd -f raw file
$ nbdsh -u 'nbd://localhost:10809' -c 'h.trim(1,0)'
qemu-nbd: Disconnect client, due to: Failed to read request: Unexpected end-of-file before all bytes were read

Then in commit f148ae7, we refactored nbd_receive_request() to use
nbd_read_eof(); when this returns 0, we regressed into tracing
uninitialized memory (if tracing is enabled) and reporting a
less-specific:

qemu-nbd: Disconnect client, due to: Request handling failed in intermediate state

Note that with Unix sockets, we have yet another error message,
unchanged by the 6.0 regression:

$ qemu-nbd -k /tmp/sock -f raw file
$ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.trim(1,0)'
qemu-nbd: Disconnect client, due to: Failed to send reply: Unable to write to socket: Broken pipe

But in all cases, the error message goes away if the client performs a
soft shutdown by using NBD_CMD_DISC, rather than a hard shutdown by
abrupt disconnect:

$ nbdsh -u 'nbd://localhost:10809' -c 'h.trim(1,0)' -c 'h.shutdown()'

This patch fixes things to avoid uninitialized memory, and in general
avoids warning about a client that does a hard shutdown when not in
the middle of a packet.  A client that aborts mid-request, or which
does not read the full server's reply, can still result in warnings,
but those are indeed much more unusual situations.

CC: qemu-stable@nongnu.org
Fixes: f148ae7d36 (nbd/server: Quiesce coroutines on context switch)
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index d9164ee6d0da..85877f630533 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1418,6 +1418,9 @@ static int nbd_receive_request(NBDClient *client, NBDRequest *request,
     if (ret < 0) {
         return ret;
     }
+    if (ret == 0) {
+        return -EIO;
+    }

     /* Request
        [ 0 ..  3]   magic   (NBD_REQUEST_MAGIC)
@@ -2285,7 +2288,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
     assert(client->recv_coroutine == qemu_coroutine_self());
     ret = nbd_receive_request(client, request, errp);
     if (ret < 0) {
-        return  ret;
+        return ret;
     }

     trace_nbd_co_receive_request_decode_type(request->handle, request->type,
@@ -2662,7 +2665,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     }

     if (ret < 0) {
-        /* It wans't -EIO, so, according to nbd_co_receive_request()
+        /* It wasn't -EIO, so, according to nbd_co_receive_request()
          * semantics, we should return the error to the client. */
         Error *export_err = local_err;

-- 
2.33.1

Re: [PATCH for-6.2 1/2] nbd/server: Don't complain on certain client disconnects

[Vladimir Sementsov-Ogievskiy]

17.11.2021 20:02, Eric Blake wrote:
> When a client disconnects abruptly, but did not have any pending
> requests (for example, when using nbdsh without calling h.shutdown),
> we used to output the following message:
> 
> $ qemu-nbd -f raw file
> $ nbdsh -u 'nbd://localhost:10809' -c 'h.trim(1,0)'
> qemu-nbd: Disconnect client, due to: Failed to read request: Unexpected end-of-file before all bytes were read
> 
> Then in commit f148ae7, we refactored nbd_receive_request() to use
> nbd_read_eof(); when this returns 0, we regressed into tracing
> uninitialized memory (if tracing is enabled) and reporting a
> less-specific:
> 
> qemu-nbd: Disconnect client, due to: Request handling failed in intermediate state
> 
> Note that with Unix sockets, we have yet another error message,
> unchanged by the 6.0 regression:
> 
> $ qemu-nbd -k /tmp/sock -f raw file
> $ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.trim(1,0)'
> qemu-nbd: Disconnect client, due to: Failed to send reply: Unable to write to socket: Broken pipe
> 
> But in all cases, the error message goes away if the client performs a
> soft shutdown by using NBD_CMD_DISC, rather than a hard shutdown by
> abrupt disconnect:
> 
> $ nbdsh -u 'nbd://localhost:10809' -c 'h.trim(1,0)' -c 'h.shutdown()'
> 
> This patch fixes things to avoid uninitialized memory, and in general
> avoids warning about a client that does a hard shutdown when not in
> the middle of a packet.  A client that aborts mid-request, or which
> does not read the full server's reply, can still result in warnings,
> but those are indeed much more unusual situations.
> 
> CC: qemu-stable@nongnu.org
> Fixes: f148ae7d36 (nbd/server: Quiesce coroutines on context switch)
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   nbd/server.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index d9164ee6d0da..85877f630533 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1418,6 +1418,9 @@ static int nbd_receive_request(NBDClient *client, NBDRequest *request,
>       if (ret < 0) {
>           return ret;
>       }
> +    if (ret == 0) {
> +        return -EIO;
> +    }

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

I'd prefer not include following hunks to the patch, as they are unrelated.

> 
>       /* Request
>          [ 0 ..  3]   magic   (NBD_REQUEST_MAGIC)
> @@ -2285,7 +2288,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
>       assert(client->recv_coroutine == qemu_coroutine_self());
>       ret = nbd_receive_request(client, request, errp);
>       if (ret < 0) {
> -        return  ret;
> +        return ret;
>       }
> 
>       trace_nbd_co_receive_request_decode_type(request->handle, request->type,
> @@ -2662,7 +2665,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>       }
> 
>       if (ret < 0) {
> -        /* It wans't -EIO, so, according to nbd_co_receive_request()
> +        /* It wasn't -EIO, so, according to nbd_co_receive_request()
>            * semantics, we should return the error to the client. */
>           Error *export_err = local_err;
> 


-- 
Best regards,
Vladimir

Re: [PATCH for-6.2 1/2] nbd/server: Don't complain on certain client disconnects

[Eric Blake]

On Wed, Nov 17, 2021 at 08:57:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 17.11.2021 20:02, Eric Blake wrote:

> > This patch fixes things to avoid uninitialized memory, and in general
> > avoids warning about a client that does a hard shutdown when not in
> > the middle of a packet.  A client that aborts mid-request, or which
> > does not read the full server's reply, can still result in warnings,
> > but those are indeed much more unusual situations.
> > 
> > CC: qemu-stable@nongnu.org
> > Fixes: f148ae7d36 (nbd/server: Quiesce coroutines on context switch)
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >   nbd/server.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/nbd/server.c b/nbd/server.c
> > index d9164ee6d0da..85877f630533 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1418,6 +1418,9 @@ static int nbd_receive_request(NBDClient *client, NBDRequest *request,
> >       if (ret < 0) {
> >           return ret;
> >       }
> > +    if (ret == 0) {
> > +        return -EIO;
> > +    }
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> I'd prefer not include following hunks to the patch, as they are unrelated.
> 
> > 
> >       /* Request
> >          [ 0 ..  3]   magic   (NBD_REQUEST_MAGIC)
> > @@ -2285,7 +2288,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
> >       assert(client->recv_coroutine == qemu_coroutine_self());
> >       ret = nbd_receive_request(client, request, errp);
> >       if (ret < 0) {
> > -        return  ret;
> > +        return ret;
> >       }
> > 
> >       trace_nbd_co_receive_request_decode_type(request->handle, request->type,
> > @@ -2662,7 +2665,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> >       }
> > 
> >       if (ret < 0) {
> > -        /* It wans't -EIO, so, according to nbd_co_receive_request()
> > +        /* It wasn't -EIO, so, according to nbd_co_receive_request()

Yeah, they were typo fixes I noticed while investigating the code; I
should have either called them out in the commit message, or as you
say, split them into a separate trivial patch (in which case they
aren't urgent for 6.2).  I'm happy to do the latter, since I'm working
on more patches for qemu 7.0 to add a new NBD protocol extension
NBD_OPT_EXTENDED_HEADERS to expose your work on 64-bit write-zero
support end-to-end.

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

[PATCH for-6.2? 2/2] nbd/server: Simplify zero and trim

[Eric Blake]

Now that the block layer supports 64-bit operations, we no longer have
to self-fragment requests larger than 2G, reverting the workaround
added in 890cbccb08 (nbd: Fix large trim/zero requests).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 85877f630533..1b3945220bd3 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2509,16 +2509,8 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
         if (request->flags & NBD_CMD_FLAG_FAST_ZERO) {
             flags |= BDRV_REQ_NO_FALLBACK;
         }
-        ret = 0;
-        /* FIXME simplify this when blk_pwrite_zeroes switches to 64-bit */
-        while (ret >= 0 && request->len) {
-            int align = client->check_align ?: 1;
-            int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
-                                                        align));
-            ret = blk_pwrite_zeroes(exp->common.blk, request->from, len, flags);
-            request->len -= len;
-            request->from += len;
-        }
+        ret = blk_pwrite_zeroes(exp->common.blk, request->from, request->len,
+                                flags);
         return nbd_send_generic_reply(client, request->handle, ret,
                                       "writing to file failed", errp);

@@ -2532,16 +2524,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
                                       "flush failed", errp);

     case NBD_CMD_TRIM:
-        ret = 0;
-        /* FIXME simplify this when blk_co_pdiscard switches to 64-bit */
-        while (ret >= 0 && request->len) {
-            int align = client->check_align ?: 1;
-            int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
-                                                        align));
-            ret = blk_co_pdiscard(exp->common.blk, request->from, len);
-            request->len -= len;
-            request->from += len;
-        }
+        ret = blk_co_pdiscard(exp->common.blk, request->from, request->len);
         if (ret >= 0 && request->flags & NBD_CMD_FLAG_FUA) {
             ret = blk_co_flush(exp->common.blk);
         }
-- 
2.33.1

Re: [PATCH for-6.2? 2/2] nbd/server: Simplify zero and trim

[Vladimir Sementsov-Ogievskiy]

17.11.2021 20:02, Eric Blake wrote:
> Now that the block layer supports 64-bit operations, we no longer have
> to self-fragment requests larger than 2G, reverting the workaround
> added in 890cbccb08 (nbd: Fix large trim/zero requests).
> 
> Signed-off-by: Eric Blake<eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH for-6.2? 2/2] nbd/server: Simplify zero and trim

[Eric Blake]

On Wed, Nov 17, 2021 at 09:04:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 17.11.2021 20:02, Eric Blake wrote:
> > Now that the block layer supports 64-bit operations, we no longer have
> > to self-fragment requests larger than 2G, reverting the workaround
> > added in 890cbccb08 (nbd: Fix large trim/zero requests).
> > 
> > Signed-off-by: Eric Blake<eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Any preferences on whether this should be in 6.2, or deferred to 7.0?

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

Re: [PATCH for-6.2? 2/2] nbd/server: Simplify zero and trim

[Vladimir Sementsov-Ogievskiy]

17.11.2021 23:49, Eric Blake wrote:
> On Wed, Nov 17, 2021 at 09:04:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 17.11.2021 20:02, Eric Blake wrote:
>>> Now that the block layer supports 64-bit operations, we no longer have
>>> to self-fragment requests larger than 2G, reverting the workaround
>>> added in 890cbccb08 (nbd: Fix large trim/zero requests).
>>>
>>> Signed-off-by: Eric Blake<eblake@redhat.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Any preferences on whether this should be in 6.2, or deferred to 7.0?
> 

In my opinion it's good for 6.2, if we are not very strict on "only real bug fixes in hard freeze".

The commit is obviously safe: if it break something, it means that feature we already included into 6.2 is broken anyway :)

-- 
Best regards,
Vladimir