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

[PATCH 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.

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                       | 35 +++++++++++
 tests/qemu-iotests/tests/nbd-stale-connection | 62 +++++++++++++++++++
 .../tests/nbd-stale-connection.out            | 17 +++++
 3 files changed, 114 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-stale-connection
 create mode 100644 tests/qemu-iotests/tests/nbd-stale-connection.out


base-commit: 3876503faff51ce3a132dbbd0ab42c10f2269319
-- 
2.43.0

[PATCH 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>
---
 nbd/client-connection.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 6a4f080717..878219fede 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -274,6 +274,26 @@ 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);
+
+    if (len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK)) {
+        return true;
+    }
+    return false;
+}
+
 /*
  * Get a new connection in context of @conn:
  *   if the thread is running, wait for completion
@@ -295,6 +315,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 +329,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

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

[Eric Blake]

On Fri, Sep 25, 2026 at 05:53:45PM +0200, Guillaume Vincke wrote:
> 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>
> ---
>  nbd/client-connection.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
> index 6a4f080717..878219fede 100644
> --- a/nbd/client-connection.c
> +++ b/nbd/client-connection.c
> @@ -274,6 +274,26 @@ void nbd_client_connection_release(NBDClientConnection *conn)
>      }
>  }
>  
> +/* only true when the peer closed the channel. Does not detect if dead */

For consistent style, I would use

/* 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);
> +
> +    if (len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK)) {
> +        return true;
> +    }
> +    return false;

The last group could be shortened to:

   return len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK);

but that is not mandatory.

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

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

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

[guillaume vincke]

On 9/25/26 23:00, Eric Blake wrote:
> On Fri, Sep 25, 2026 at 05:53:45PM +0200, Guillaume Vincke wrote:
>> 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>
>> ---
>>   nbd/client-connection.c | 35 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>
>> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
>> index 6a4f080717..878219fede 100644
>> --- a/nbd/client-connection.c
>> +++ b/nbd/client-connection.c
>> @@ -274,6 +274,26 @@ void nbd_client_connection_release(NBDClientConnection *conn)
>>       }
>>   }
>>   
>> +/* only true when the peer closed the channel. Does not detect if dead */
> 
> For consistent style, I would use
> 
> /* 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);
>> +
>> +    if (len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK)) {
>> +        return true;
>> +    }
>> +    return false;
> 
> The last group could be shortened to:
> 
>     return len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK);

Thanks, I will make sure to include those changes in v2 once patch 2/2
is reviewed, to avoid a new submission for these alone.

> 
> but that is not mandatory.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

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

[Eric Blake]

On Mon, Sep 28, 2026 at 10:23:49AM +0200, guillaume vincke wrote:
> > > +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);
> > > +
> > > +    if (len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK)) {
> > > +        return true;
> > > +    }
> > > +    return false;
> > 
> > The last group could be shortened to:
> > 
> >     return len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK);
> 
> Thanks, I will make sure to include those changes in v2 once patch 2/2
> is reviewed, to avoid a new submission for these alone.

The changes turned out small enough, and I want to get a bundle of
pending patches merged, so I want ahead and made the minor changes and
included this on my NBD queue.

> 
> > 
> > but that is not mandatory.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > 
> 

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

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

[guillaume vincke]

On 10/6/26 21:13, Eric Blake wrote:
> On Mon, Sep 28, 2026 at 10:23:49AM +0200, guillaume vincke wrote:
>>>> +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);
>>>> +
>>>> +    if (len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK)) {
>>>> +        return true;
>>>> +    }
>>>> +    return false;
>>>
>>> The last group could be shortened to:
>>>
>>>      return len == 0 || (len < 0 && len != QIO_CHANNEL_ERR_BLOCK);
>>
>> Thanks, I will make sure to include those changes in v2 once patch 2/2
>> is reviewed, to avoid a new submission for these alone.
> 
> The changes turned out small enough, and I want to get a bundle of
> pending patches merged, so I want ahead and made the minor changes and
> included this on my NBD queue.

That's perfect. Thank you for the edits.

> 
>>
>>>
>>> but that is not mandatory.
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>
> 

[PATCH 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 | 62 +++++++++++++++++++
 .../tests/nbd-stale-connection.out            | 17 +++++
 2 files changed, 79 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..6f6fb6ef1b
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-stale-connection
@@ -0,0 +1,62 @@
+#!/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_HANDSHAKE_DURATION = 0.5
+# sleep duration of the connection retry in nbd client
+CONNECT_RETRY_DELAY = 1
+
+iotests.script_initialize()
+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'))
+    # in CONNECTING, the operation starts the connect thread
+    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 ===')
+        # wait for the connect thread to get a new channel and stop
+        time.sleep(CONNECT_RETRY_DELAY + MAX_NBD_HANDSHAKE_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_HANDSHAKE_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..50f80e2305
--- /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"}}
+{"error": {"class": "GenericError", "desc": "write failed: Input/output error"}}
+Start NBD server
+=== NBD server up. Wait for the connect thread to park the channel ===
+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 2/2] iotests: Add coverage for NBD connect thread start delay

[Eric Blake]

On Fri, Sep 25, 2026 at 05:53:46PM +0200, 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 2/2] iotests: Add coverage for NBD connect thread start delay

[Eric Blake]

On Mon, Oct 05, 2026 at 01:44:10PM -0500, Eric Blake wrote:
> On Fri, Sep 25, 2026 at 05:53:46PM +0200, 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>

Also, I checked that this new test exposes the problem if applied
independently from the patch in 1/2.

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

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