[PATCH v3 0/2] nbd/client-connection: Fix client returning a closed channel

[PATCH v3 0/2] nbd/client-connection: Fix client returning a closed channel

[Guillaume Vincke]

NBD client relies on lazy reconnection on error. There is a case where
nbd_co_establish_connection() will hand back a channel disconnected by
its peer. This leads to the request running on the dead channel. When
reconnect-delay is set to 0 (the default), this bug prevents the
nbd-connect thread from running on the first operation. This patch set
addresses this specific case.

Found while searching for a way to force client reconnection to simplify
the resize of idle nodes which will be the subject of another patch set.

v1: https://patchew.org/QEMU/20260925155346.357703-1-guillaume.vincke@ovhcloud.com/
v2: https://patchew.org/QEMU/20261016084136.233836-1-guillaume.vincke@ovhcloud.com/

v1 -> v2:
- patch 1: Pick up Eric Blake's formatting changes.
- patch 2: Start the connect thread after the NBD server so the test no
longer depends on the 1s connect retry, which made v1 fail in CI. Raise
the connect wait to 1s.

v2 -> v3:
- patch 2: Restrict the test to qcow2. The luks CI failure was the
missing key-secret and the parallels failure was the dirty-image repair
message. Neither was timing.

v3 does not use a more robust method than sleeping to wait for the NBD
client to reach the desired state. An option would be to add a QMP
unstable debug query reporting the NBD client state and whether the
connect thread has a channel ready. The test would poll this instead of
sleeping. Would that be an acceptable follow-up?

Guillaume Vincke (2):
  nbd/client-connection: Fix client returning a closed channel
  iotests: Add coverage for NBD connect thread start delay

 nbd/client-connection.c                       | 32 ++++++++++
 tests/qemu-iotests/tests/nbd-stale-connection | 60 +++++++++++++++++++
 .../tests/nbd-stale-connection.out            | 17 ++++++
 3 files changed, 109 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-stale-connection
 create mode 100644 tests/qemu-iotests/tests/nbd-stale-connection.out

-- 
2.43.0

[PATCH v3 1/2] nbd/client-connection: Fix client returning a closed channel

[Guillaume Vincke]

When a server closes the connection after a successful handshake but
before running any operations, the channel returned by
nbd_co_establish_connection() is the closed one and the connect thread
is not started.

When configured with reconnect-delay=0 (the default), this situation
generates at least two IO errors. The first one happens when trying to
issue an operation to the closed socket. The connect thread will be
started on the next request, which may happen long after, and will
create the second error as nbd_co_establish_connection() returns a NULL
channel.

Peeking the channel to detect FIN or RST avoids the first IO error and
does not delay the starting of the connect thread.

Signed-off-by: Guillaume Vincke <guillaume.vincke@ovhcloud.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/client-connection.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 6a4f080717..aec62934d1 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -274,6 +274,23 @@ void nbd_client_connection_release(NBDClientConnection *conn)
     }
 }
 
+/* Only true when the peer closed the channel. Does not detect if dead. */
+static bool nbd_channel_is_closed(QIOChannel *ioc)
+{
+    char data;
+    struct iovec iov = { .iov_base = &data, .iov_len = 1 };
+    ssize_t len = -1;
+
+    if (!qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
+        return false;
+    }
+
+    len = qio_channel_readv_full(
+        ioc, &iov, 1, NULL, NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, NULL);
+
+    return len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK);
+}
+
 /*
  * Get a new connection in context of @conn:
  *   if the thread is running, wait for completion
@@ -295,6 +312,7 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
                             bool blocking, Error **errp)
 {
     QemuThread thread;
+    QIOChannel *ioc;
 
     if (conn->do_negotiation) {
         assert(info);
@@ -308,6 +326,20 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
         assert(!conn->wait_co);
 
         if (!conn->running) {
+            if (conn->sioc) {
+                /* discard if the socket got closed by its peer */
+                ioc = QIO_CHANNEL(conn->sioc);
+                if (!qio_channel_set_blocking(ioc, false, NULL) ||
+                    nbd_channel_is_closed(ioc)) {
+
+                    qio_channel_close(ioc, NULL);
+                    object_unref(OBJECT(conn->sioc));
+                    conn->sioc = NULL;
+                    object_unref(OBJECT(conn->ioc));
+                    conn->ioc = NULL;
+                }
+            }
+
             if (conn->sioc) {
                 /* Previous attempt finally succeeded in background */
                 if (conn->do_negotiation) {
-- 
2.43.0

[PATCH v3 2/2] iotests: Add coverage for NBD connect thread start delay

[Guillaume Vincke]

Test the connect thread starting condition with reconnect-delay=0 when
the server disconnects after a successful handshake but before running
operations. Prior to the change to nbd_co_establish_connection(), this
test would demonstrate two I/O errors. The tests 264 and 277 covering
reconnection do not use reconnect-delay=0 so this case is not exercised.

Add a test that provokes multiple errors in order to put an NBD client
in the above-mentioned situation and shows that the first operation
starts the connect thread and leads to successful subsequent I/O.

Signed-off-by: Guillaume Vincke <guillaume.vincke@ovhcloud.com>
---
 tests/qemu-iotests/tests/nbd-stale-connection | 60 +++++++++++++++++++
 .../tests/nbd-stale-connection.out            | 17 ++++++
 2 files changed, 77 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-stale-connection
 create mode 100644 tests/qemu-iotests/tests/nbd-stale-connection.out

diff --git a/tests/qemu-iotests/tests/nbd-stale-connection b/tests/qemu-iotests/tests/nbd-stale-connection
new file mode 100755
index 0000000000..e361bccfa9
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-stale-connection
@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+# group: quick
+#
+# Test the NBD client stale channel discard
+#
+# Copyright (C) 2026 OVHcloud
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import time
+import iotests
+from iotests import qemu_nbd_popen, qemu_img_create, file_path, log
+
+
+# estimated duration required by the connect thread to reach a running server
+MAX_NBD_CONNECT_DURATION = 1
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
+disk = file_path('disk')
+qemu_img_create('-f', iotests.imgfmt, disk, '1M')
+
+with iotests.VM() as vm:
+
+    vm.launch()
+
+    with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk):
+        vm.cmd('blockdev-add', {
+            'driver': 'nbd',
+            'node-name': 'nbd0',
+            'server': {'type': 'unix', 'path': nbd_sock},
+        })
+        iotests.log('=== NBD Device connected ===')
+        log(vm.qmp('x-qemu-io', device='nbd0', command='write -P 0x5a 0 64k'))
+
+    iotests.log('=== NBD server down. Detect the break and start the connect '
+                'thread ===')
+    # client was in CONNECTED state a new operation updates to CONNECTING
+    log(vm.qmp('x-qemu-io', device='nbd0', command='write -P 0x5a 0 64k'))
+
+    with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk):
+        iotests.log('=== NBD server up. Wait for the connect thread to park '
+                    'the channel ===')
+        # in CONNECTING, the operation starts the connect thread
+        log(vm.qmp('x-qemu-io', device='nbd0', command='write -P 0x5a 0 64k'))
+        # wait for the connect thread to get a new channel and stop
+        time.sleep(MAX_NBD_CONNECT_DURATION)
+
+    iotests.log('=== NBD server down. The parked channel is closed ===')
+
+    with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk):
+        iotests.log('=== NBD server up. Restart the connect thread ===')
+        # operation will determine that the channel got closed and restart the
+        # connect thread
+        log(vm.qmp('x-qemu-io', device='nbd0', command='write -P 0x5a 0 64k'))
+        # Wait for it to succeed
+        iotests.log('=== Wait for the connect thread to park the new channel. '
+                    'Subsequent operations are successful ===')
+        time.sleep(MAX_NBD_CONNECT_DURATION)
+        log(vm.qmp('x-qemu-io', device='nbd0', command='write -P 0x5a 0 64k'))
diff --git a/tests/qemu-iotests/tests/nbd-stale-connection.out b/tests/qemu-iotests/tests/nbd-stale-connection.out
new file mode 100644
index 0000000000..065ba329e9
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-stale-connection.out
@@ -0,0 +1,17 @@
+Start NBD server
+=== NBD Device connected ===
+{"return": {}}
+Kill NBD server
+=== NBD server down. Detect the break and start the connect thread ===
+{"error": {"class": "GenericError", "desc": "write failed: Input/output error"}}
+Start NBD server
+=== NBD server up. Wait for the connect thread to park the channel ===
+{"error": {"class": "GenericError", "desc": "write failed: Input/output error"}}
+Kill NBD server
+=== NBD server down. The parked channel is closed ===
+Start NBD server
+=== NBD server up. Restart the connect thread ===
+{"error": {"class": "GenericError", "desc": "write failed: Input/output error"}}
+=== Wait for the connect thread to park the new channel. Subsequent operations are successful ===
+{"return": {}}
+Kill NBD server
-- 
2.43.0

Re: [PATCH v3 2/2] iotests: Add coverage for NBD connect thread start delay

[Eric Blake]

On Thu, Nov 05, 2026 at 05:41:24PM +0100, Guillaume Vincke wrote:
> Test the connect thread starting condition with reconnect-delay=0 when
> the server disconnects after a successful handshake but before running
> operations. Prior to the change to nbd_co_establish_connection(), this
> test would demonstrate two I/O errors. The tests 264 and 277 covering
> reconnection do not use reconnect-delay=0 so this case is not exercised.
> 
> Add a test that provokes multiple errors in order to put an NBD client
> in the above-mentioned situation and shows that the first operation
> starts the connect thread and leads to successful subsequent I/O.
> 
> Signed-off-by: Guillaume Vincke <guillaume.vincke@ovhcloud.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v3 0/2] nbd/client-connection: Fix client returning a closed channel

[Eric Blake]

On Thu, Nov 05, 2026 at 05:41:22PM +0100, Guillaume Vincke wrote:
> NBD client relies on lazy reconnection on error. There is a case where
> nbd_co_establish_connection() will hand back a channel disconnected by
> its peer. This leads to the request running on the dead channel. When
> reconnect-delay is set to 0 (the default), this bug prevents the
> nbd-connect thread from running on the first operation. This patch set
> addresses this specific case.
> 

> 
> v3 does not use a more robust method than sleeping to wait for the NBD
> client to reach the desired state. An option would be to add a QMP
> unstable debug query reporting the NBD client state and whether the
> connect thread has a channel ready. The test would poll this instead of
> sleeping. Would that be an acceptable follow-up?

I think an unstable query for current NBD server state (is the server
up, and if so, which exports is it serving and what is the state of
all client(s) attached to each server) would indeed make sense.  That
will probably miss this release, but I still think this series is okay
for now.  Queueing it up as a bug fix, for my upcoming pull request
intended to make it into rc0.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v3 0/2] nbd/client-connection: Fix client returning a closed channel

[guillaume vincke]

On 11/6/26 20:45, Eric Blake wrote:
> On Thu, Nov 05, 2026 at 05:41:22PM +0100, Guillaume Vincke wrote:
>> NBD client relies on lazy reconnection on error. There is a case where
>> nbd_co_establish_connection() will hand back a channel disconnected by
>> its peer. This leads to the request running on the dead channel. When
>> reconnect-delay is set to 0 (the default), this bug prevents the
>> nbd-connect thread from running on the first operation. This patch set
>> addresses this specific case.
>>
> 
>>
>> v3 does not use a more robust method than sleeping to wait for the NBD
>> client to reach the desired state. An option would be to add a QMP
>> unstable debug query reporting the NBD client state and whether the
>> connect thread has a channel ready. The test would poll this instead of
>> sleeping. Would that be an acceptable follow-up?
> 
> I think an unstable query for current NBD server state (is the server
> up, and if so, which exports is it serving and what is the state of
> all client(s) attached to each server) would indeed make sense.  That
> will probably miss this release, but I still think this series is okay
> for now.  Queueing it up as a bug fix, for my upcoming pull request
> intended to make it into rc0.
> 

Thanks for the review and for queueing it. I sent a new patchset with
a QMP command at:
https://patchew.org/QEMU/20261109160609.1434289-1-guillaume.vincke@ovhcloud.com/
It does not access the server's internals but the client's, since the
test uses qemu-nbd that does not have QMP. Let me know if that still
fits your expectations.