[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] nbd/server: Suppress Broken pipe errors on abrupt disconn

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2] nbd/server: Suppress Broken pipe errors on abrupt disconnection
Date: Tue, 14 Sep 2021 18:21:58 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0

14.09.2021 17:52, Richard W.M. Jones wrote:
On Tue, Sep 14, 2021 at 05:40:59PM +0300, Vladimir Sementsov-Ogievskiy wrote:
13.09.2021 18:19, Richard W.M. Jones wrote:
$ rm -f /tmp/sock /tmp/pid
$ qemu-img create -f qcow2 /tmp/disk.qcow2 1M
$ qemu-nbd -t --format=qcow2 --socket=/tmp/sock --pid-file=/tmp/pid /tmp/disk.qcow2 
$ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()'
qemu-nbd: Disconnect client, due to: Failed to send reply: Unable to write to 
socket: Broken pipe
$ killall qemu-nbd

nbdsh is abruptly dropping the NBD connection here which is a valid
way to close the connection.  It seems unnecessary to print an error
in this case so this commit suppresses it.

Note that if you call the nbdsh h.shutdown() method then the message
was not printed:

$ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()' -c 'h.shutdown()'

My personal opinion, is that this warning doesn't hurt in general. I
think in production tools should gracefully shutdown any connection,
and abrupt shutdown is a sign of something wrong - i.e., worth

Shouldn't nbdsh do graceful shutdown by default?

On the client side the only difference is that nbd_shutdown sends
NBD_CMD_DISC to the server (versus simply closing the socket).

Qemu as NBD client always send NBD_CMD_DISC on close() (if io channel is alive).

 On the
server side when the server receives NBD_CMD_DISC it must complete any
in-flight requests, but there's no requirement for the server to
commit anything to disk.  IOW you can still lose data even though you
took the time to disconnect.

So I don't think there's any reason for libnbd to always gracefully

Hmm. Me go to NBD spec :)

I think, there is a reason:

"The client and the server MUST NOT initiate any form of disconnect other than in 
one of the above circumstances."

And the only possibility for client to initiate a hard disconnect listed above is 
"if it detects a violation by the other party of a mandatory condition within this 

So at least, nbdsh violates NBD protocol. May be spec should be updated to 
satisfy your needs.

shut down (especially in this case where there are no in-flight
requests), and anyway it would break ABI to make that change and slow
down the client in cases when there's nothing to clean up.

Which ABI will it break?

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
  nbd/server.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 3927f7789d..e0a43802b2 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2669,7 +2669,12 @@ static coroutine_fn void nbd_trip(void *opaque)
          ret = nbd_handle_request(client, &request, req->data, &local_err);
      if (ret < 0) {
-        error_prepend(&local_err, "Failed to send reply: ");
+        if (errno != EPIPE) {

Both nbd_handle_request() and nbd_send_generic_reply() declares that
they return -errno on failure in communication with client. I think,
you should use ret here: if (ret != -EPIPE). It's safer: who knows,
does errno really set on all error paths of called functions? If
not, we may see here errno of some another previous operation.

Should we set errno = 0 earlier in nbd_trip()?  I don't really know
how coroutines in qemu interact with thread-local variables though.


+            error_prepend(&local_err, "Failed to send reply: ");
+        } else {
+            error_free(local_err);
+            local_err = NULL;
+        }
          goto disconnect;

Best regards,

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]