Virtuozzo QEMU development (svt-core@virtuozzo.com)
 help / color / mirror / Atom feed
* [PATCH hci-8.0 0/5] nbd/server: accept a large NBD_CMD_CACHE #VSTOR-119829
@ 2026-08-31 15:25 Denis V. Lunev
  2026-08-31 15:25 ` [PATCH hci-8.0 1/5] iotests: add coverage for NBD transmission commands #VSTOR-119829 Denis V. Lunev
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-31 15:25 UTC (permalink / raw)
  To: svt-core; +Cc: andrey.drobyshev, den

Backport of the upstream series which lets the NBD server accept an
NBD_CMD_CACHE whose effect length is above the advertised maximum
payload size.

vz_backup_client prefetches through NBD_CMD_CACHE and is refused as
soon as it asks for more than 32 MiB, which is what VSTOR-119667 hit:

  failed to prefetch 28114944:33619968: 'nbd_cache: cache: command
  failed: Invalid argument'

33619968 is 32 MiB plus 64 KiB. The command carries no payload in
either direction, and the specification lets a client ask for an
effect length larger than the maximum payload size, which we already
allow for NBD_CMD_TRIM and NBD_CMD_WRITE_ZEROES. Patch 2 drops the
check, so a client may prefetch as much as it likes in one request.
The advertised maximum is unchanged and still bounds NBD_CMD_READ and
NBD_CMD_WRITE.

Patches 1, 3, 4 and 5 add tests/qemu-iotests/tests/nbd-commands, a
libnbd driven client which speaks the transmission phase itself. It
pins patch 2 and covers the reply modes, the command flags and the
sparse replies, none of which any test in the tree reached. The test
is skipped where the libnbd python bindings are missing, the same way
nbd-multiconn already is.

The series applies to the branch unchanged. The python iotests cannot
run on my host against this branch, since its python/qemu asyncio shim
predates Python 3.13 and nbd-multiconn fails there the same way, so the
new test was exercised upstream rather than here. The fix itself was
checked against qemu-nbd from this build: a 64 MiB NBD_CMD_CACHE is
refused with EINVAL before the change and accepted after it, and the
advertised maximum payload size stays at 32 MiB.

Upstream posting, reviewed by the block maintainer:
https://lore.kernel.org/qemu-devel/20260827161002.310688-1-den@openvz.org/

Signed-off-by: Denis V. Lunev <den@openvz.org>

Denis V. Lunev (5):
  iotests: add coverage for NBD transmission commands #VSTOR-119829
  nbd/server: accept NBD_CMD_CACHE above the maximum payload size
    #VSTOR-119829
  iotests/nbd-commands: exercise the simple and structured reply modes
    #VSTOR-119829
  iotests/nbd-commands: cover NBD_CMD_BLOCK_STATUS with a payload
    #VSTOR-119829
  iotests/nbd-commands: cover the command flags and sparse replies
    #VSTOR-119829

 nbd/server.c                              |   4 +-
 tests/qemu-iotests/tests/nbd-commands     | 244 ++++++++++++++++++++++
 tests/qemu-iotests/tests/nbd-commands.out |   5 +
 3 files changed, 250 insertions(+), 3 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/nbd-commands
 create mode 100644 tests/qemu-iotests/tests/nbd-commands.out

-- 
2.53.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH hci-8.0 1/5] iotests: add coverage for NBD transmission commands #VSTOR-119829
  2026-08-31 15:25 [PATCH hci-8.0 0/5] nbd/server: accept a large NBD_CMD_CACHE #VSTOR-119829 Denis V. Lunev
@ 2026-08-31 15:25 ` Denis V. Lunev
  2026-09-01 15:51   ` Andrey Drobyshev
  2026-08-31 15:25 ` [PATCH hci-8.0 2/5] nbd/server: accept NBD_CMD_CACHE above the maximum payload size #VSTOR-119829 Denis V. Lunev
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-31 15:25 UTC (permalink / raw)
  To: svt-core; +Cc: andrey.drobyshev, den

From: Denis V. Lunev <den@openvz.org>

NBD_CMD_CACHE has no coverage anywhere in the tree. Nothing ever
sends it: our own NBD client does not implement the command, and
neither qemu-io nor 'qemu-nbd --list' can issue one, so the only
clients reaching this server path are external ones.

Add a test driven by libnbd, gated the way nbd-multiconn already is,
and start it with the case the command exists for. The export is a
qcow2 image over a fully written backing file, so a prefetch has
visible work to do.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
Message-ID: <20260827161002.310688-2-den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/qemu-iotests/tests/nbd-commands     | 140 ++++++++++++++++++++++
 tests/qemu-iotests/tests/nbd-commands.out |   5 +
 2 files changed, 145 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-commands
 create mode 100644 tests/qemu-iotests/tests/nbd-commands.out

diff --git a/tests/qemu-iotests/tests/nbd-commands b/tests/qemu-iotests/tests/nbd-commands
new file mode 100755
index 00000000000..4c1cd33db74
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-commands
@@ -0,0 +1,140 @@
+#!/usr/bin/env python3
+# group: rw auto quick
+#
+# Test NBD transmission commands against a qemu NBD export
+#
+# Copyright (C) 2026 Virtuozzo International GmbH
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+from types import ModuleType
+
+import iotests
+from iotests import qemu_img_create, qemu_img_map, qemu_io
+
+
+base = os.path.join(iotests.test_dir, 'base')
+top = os.path.join(iotests.test_dir, 'top')
+# Larger than the maximum payload size an export can advertise
+size = 64 * 1024 * 1024
+pattern = 0xa5
+nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock')
+nbd_uri = 'nbd+unix:///exp?socket=' + nbd_sock
+nbd: ModuleType
+
+DEPTH_LOCAL = 1
+DEPTH_BACKING = 2
+
+
+class TestNbdCommands(iotests.QMPTestCase):
+    def setUp(self):
+        qemu_img_create('-f', iotests.imgfmt, base, str(size))
+        qemu_io('-c', f'write -P {pattern} 0 {size}', base)
+        qemu_img_create('-f', iotests.imgfmt, '-b', base,
+                        '-F', iotests.imgfmt, top, str(size))
+
+        self.vm = iotests.VM()
+        self.vm.launch()
+        self.vm.cmd('blockdev-add', {
+            'driver': iotests.imgfmt,
+            'node-name': 'n',
+            'file': {'driver': 'file', 'filename': top},
+            'backing': {
+                'driver': iotests.imgfmt,
+                'node-name': 'base',
+                'file': {'driver': 'file', 'filename': base},
+            },
+        })
+        self.vm.cmd('nbd-server-start', {
+            'addr': {'type': 'unix', 'data': {'path': nbd_sock}}
+        })
+        self.vm.cmd('block-export-add', {
+            'type': 'nbd',
+            'id': 'exp',
+            'node-name': 'n',
+            'name': 'exp',
+            'writable': True,
+            'allocation-depth': True,
+        })
+
+        self.h = None
+        self.connect()
+
+    def tearDown(self):
+        self.disconnect()
+        self.vm.shutdown()
+        for f in (top, base, nbd_sock):
+            try:
+                os.remove(f)
+            except OSError:
+                pass
+
+    def connect(self, structured=True, extended=True):
+        self.disconnect()
+        h = nbd.NBD()
+        h.set_request_structured_replies(structured)
+        h.set_request_extended_headers(extended)
+        h.add_meta_context('base:allocation')
+        h.add_meta_context('qemu:allocation-depth')
+        # Let the server, not libnbd, reject the out of range requests below
+        h.set_strict_mode(h.get_strict_mode() &
+                          ~(nbd.STRICT_BOUNDS | nbd.STRICT_PAYLOAD))
+        h.connect_uri(nbd_uri)
+        self.assertEqual(h.get_structured_replies_negotiated(), structured)
+        self.assertEqual(h.get_extended_headers_negotiated(), extended)
+        self.h = h
+
+    def disconnect(self):
+        if self.h is not None:
+            self.h.shutdown()
+            self.h = None
+
+    def block_status(self, count=size):
+        """Map each meta context in the reply to its list of extents."""
+        reply = {}
+
+        def cb(meta, _offset, entries, _err):
+            reply.setdefault(meta, []).extend(zip(entries[0::2],
+                                                  entries[1::2]))
+
+        self.h.block_status(count, 0, cb)
+        return reply
+
+    def top_extents(self):
+        """Which parts of the top image are local, once qemu has let go."""
+        self.disconnect()
+        self.vm.shutdown()
+        return [(e['start'], e['length'], e['depth'])
+                for e in qemu_img_map(top)]
+
+    def test_cache_copies_on_read(self):
+        maximum = self.h.get_block_size(nbd.SIZE_MAXIMUM)
+        self.assertLess(maximum, size)
+        self.assertEqual(self.block_status()['qemu:allocation-depth'],
+                         [(size, DEPTH_BACKING)])
+
+        self.h.cache(maximum, 0)
+
+        self.assertEqual(self.top_extents(),
+                         [(0, maximum, 0), (maximum, size - maximum, 1)])
+        qemu_io('-c', f'read -P {pattern} 0 {size}', top)
+
+    def test_cache_past_end_of_export(self):
+        self.assertRaises(nbd.Error, self.h.cache, size + 1, 0)
+
+    def test_read_bound_by_max_payload(self):
+        maximum = self.h.get_block_size(nbd.SIZE_MAXIMUM)
+        self.assertRaises(nbd.Error, self.h.pread, maximum + 65536, 0)
+
+
+if __name__ == '__main__':
+    try:
+        # Easier to use libnbd than to try and set up parallel
+        # 'qemu-nbd --list' or 'qemu-io' processes, but not all systems
+        # have libnbd installed.
+        import nbd  # type: ignore
+
+        iotests.main(supported_fmts=['qcow2'])
+    except ImportError:
+        iotests.notrun('Python bindings to libnbd are not installed')
diff --git a/tests/qemu-iotests/tests/nbd-commands.out b/tests/qemu-iotests/tests/nbd-commands.out
new file mode 100644
index 00000000000..8d7e9967009
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-commands.out
@@ -0,0 +1,5 @@
+...
+----------------------------------------------------------------------
+Ran 3 tests
+
+OK
-- 
2.53.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH hci-8.0 2/5] nbd/server: accept NBD_CMD_CACHE above the maximum payload size #VSTOR-119829
  2026-08-31 15:25 [PATCH hci-8.0 0/5] nbd/server: accept a large NBD_CMD_CACHE #VSTOR-119829 Denis V. Lunev
  2026-08-31 15:25 ` [PATCH hci-8.0 1/5] iotests: add coverage for NBD transmission commands #VSTOR-119829 Denis V. Lunev
@ 2026-08-31 15:25 ` Denis V. Lunev
  2026-08-31 15:25 ` [PATCH hci-8.0 3/5] iotests/nbd-commands: exercise the simple and structured reply modes #VSTOR-119829 Denis V. Lunev
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-31 15:25 UTC (permalink / raw)
  To: svt-core; +Cc: andrey.drobyshev, den

From: Denis V. Lunev <den@openvz.org>

NBD_CMD_CACHE carries no payload in either direction. The request is
a header only, nbd_do_cmd_cache() passes a NULL qiov to
blk_co_preadv(), and the reply is a bare status. Still the server
rejects any effect length above NBD_MAX_BUFFER_SIZE with EINVAL,
which forces a client to split a large prefetch into 32 MiB pieces.

The specification does not ask for this. The constraint was renamed
from "maximum block size" to "maximum payload size" precisely to
separate payload length from effect length, and it says:

  For commands that do not require a payload in either direction
  (such as NBD_CMD_TRIM or NBD_CMD_WRITE_ZEROES), the client MAY
  request an effect length larger than the maximum payload size;
  the server SHOULD NOT disconnect, but MAY reply with an
  NBD_EOVERFLOW or NBD_EINVAL error if the oversize request would
  require too many server resources when compared to the same
  command with an effect length limited to the maximum payload
  size (such as an implementation of NBD_CMD_WRITE_ZEROES that
  utilizes a scratch buffer).

We already follow that for NBD_CMD_TRIM and NBD_CMD_WRITE_ZEROES,
which carry no length check at all, and our client assumes a server
supporting extended headers takes unlimited zero and trim lengths.
Handle NBD_CMD_CACHE in the same way.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: Eric Blake <eblake@redhat.com>
Message-ID: <20260827161002.310688-3-den@openvz.org>
---
 nbd/server.c                          | 4 +---
 tests/qemu-iotests/tests/nbd-commands | 5 ++---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index c3be1e3c5d0..f06b3321d66 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2716,7 +2716,6 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
         break;
 
     case NBD_CMD_CACHE:
-        check_length = true;
         break;
 
     case NBD_CMD_WRITE_ZEROES:
@@ -2750,7 +2749,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
         req->complete = true;
     }
     if (check_length && request->len > NBD_MAX_BUFFER_SIZE) {
-        /* READ, WRITE, CACHE */
+        /* READ, WRITE */
         error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
                    request->len, NBD_MAX_BUFFER_SIZE);
         return -EINVAL;
@@ -2906,7 +2905,6 @@ static coroutine_fn int nbd_do_cmd_cache(NBDClient *client, NBDRequest *request,
     NBDExport *exp = client->exp;
 
     assert(request->type == NBD_CMD_CACHE);
-    assert(request->len <= NBD_MAX_BUFFER_SIZE);
 
     ret = blk_co_preadv(exp->common.blk, request->from, request->len,
                         NULL, BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
diff --git a/tests/qemu-iotests/tests/nbd-commands b/tests/qemu-iotests/tests/nbd-commands
index 4c1cd33db74..cbfc47782ff 100755
--- a/tests/qemu-iotests/tests/nbd-commands
+++ b/tests/qemu-iotests/tests/nbd-commands
@@ -114,10 +114,9 @@ class TestNbdCommands(iotests.QMPTestCase):
         self.assertEqual(self.block_status()['qemu:allocation-depth'],
                          [(size, DEPTH_BACKING)])
 
-        self.h.cache(maximum, 0)
+        self.h.cache(size, 0)
 
-        self.assertEqual(self.top_extents(),
-                         [(0, maximum, 0), (maximum, size - maximum, 1)])
+        self.assertEqual(self.top_extents(), [(0, size, 0)])
         qemu_io('-c', f'read -P {pattern} 0 {size}', top)
 
     def test_cache_past_end_of_export(self):
-- 
2.53.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH hci-8.0 3/5] iotests/nbd-commands: exercise the simple and structured reply modes #VSTOR-119829
  2026-08-31 15:25 [PATCH hci-8.0 0/5] nbd/server: accept a large NBD_CMD_CACHE #VSTOR-119829 Denis V. Lunev
  2026-08-31 15:25 ` [PATCH hci-8.0 1/5] iotests: add coverage for NBD transmission commands #VSTOR-119829 Denis V. Lunev
  2026-08-31 15:25 ` [PATCH hci-8.0 2/5] nbd/server: accept NBD_CMD_CACHE above the maximum payload size #VSTOR-119829 Denis V. Lunev
@ 2026-08-31 15:25 ` Denis V. Lunev
  2026-08-31 15:25 ` [PATCH hci-8.0 4/5] iotests/nbd-commands: cover NBD_CMD_BLOCK_STATUS with a payload #VSTOR-119829 Denis V. Lunev
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-31 15:25 UTC (permalink / raw)
  To: svt-core; +Cc: andrey.drobyshev, den

From: Denis V. Lunev <den@openvz.org>

Every NBD request the iotests send arrives in NBD_MODE_EXTENDED, so
the server paths kept for older clients are never taken. Instrumenting
nbd_co_receive_request() over the whole auto group and every test that
touches NBD gives mode 4 for all of them, which leaves
nbd_co_send_simple_reply() and the compact header handling dead under
test. A regression there would only show against a third party client.

libnbd can negotiate down, so run the same command set three times,
once per mode, and assert the mode that was actually reached. Block
status is limited to the two modes that can negotiate a meta context.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
Message-ID: <20260827161002.310688-4-den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/qemu-iotests/tests/nbd-commands     | 29 +++++++++++++++++++++++
 tests/qemu-iotests/tests/nbd-commands.out |  4 ++--
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/tests/nbd-commands b/tests/qemu-iotests/tests/nbd-commands
index cbfc47782ff..3dfe9131900 100755
--- a/tests/qemu-iotests/tests/nbd-commands
+++ b/tests/qemu-iotests/tests/nbd-commands
@@ -119,6 +119,35 @@ class TestNbdCommands(iotests.QMPTestCase):
         self.assertEqual(self.top_extents(), [(0, size, 0)])
         qemu_io('-c', f'read -P {pattern} 0 {size}', top)
 
+    def check_commands(self, structured, extended):
+        self.connect(structured, extended)
+
+        self.assertEqual(self.h.pread(4096, 4096), bytes([pattern]) * 4096)
+
+        self.h.cache(size, 0)
+
+        self.h.pwrite(b'x' * 4096, 4096)
+        self.h.flush()
+        self.assertEqual(self.h.pread(4096, 4096), b'x' * 4096)
+
+        self.h.zero(4096, 8192)
+        self.assertEqual(self.h.pread(4096, 8192), bytes(4096))
+
+        self.h.trim(4096, 16384)
+
+        if structured:
+            self.assertEqual(self.block_status()['qemu:allocation-depth'],
+                             [(size, DEPTH_LOCAL)])
+
+    def test_commands_simple_replies(self):
+        self.check_commands(structured=False, extended=False)
+
+    def test_commands_structured_replies(self):
+        self.check_commands(structured=True, extended=False)
+
+    def test_commands_extended_headers(self):
+        self.check_commands(structured=True, extended=True)
+
     def test_cache_past_end_of_export(self):
         self.assertRaises(nbd.Error, self.h.cache, size + 1, 0)
 
diff --git a/tests/qemu-iotests/tests/nbd-commands.out b/tests/qemu-iotests/tests/nbd-commands.out
index 8d7e9967009..3f8a935a082 100644
--- a/tests/qemu-iotests/tests/nbd-commands.out
+++ b/tests/qemu-iotests/tests/nbd-commands.out
@@ -1,5 +1,5 @@
-...
+......
 ----------------------------------------------------------------------
-Ran 3 tests
+Ran 6 tests
 
 OK
-- 
2.53.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH hci-8.0 4/5] iotests/nbd-commands: cover NBD_CMD_BLOCK_STATUS with a payload #VSTOR-119829
  2026-08-31 15:25 [PATCH hci-8.0 0/5] nbd/server: accept a large NBD_CMD_CACHE #VSTOR-119829 Denis V. Lunev
                   ` (2 preceding siblings ...)
  2026-08-31 15:25 ` [PATCH hci-8.0 3/5] iotests/nbd-commands: exercise the simple and structured reply modes #VSTOR-119829 Denis V. Lunev
@ 2026-08-31 15:25 ` Denis V. Lunev
  2026-08-31 15:25 ` [PATCH hci-8.0 5/5] iotests/nbd-commands: cover the command flags and sparse replies #VSTOR-119829 Denis V. Lunev
  2026-09-01 15:50 ` [QEMU HCI-8.0 PATCH 6/5] VZ: iotests/nbd-commands: do not depend on cluster allocation order #VSTOR-119829 Andrey Drobyshev
  5 siblings, 0 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-31 15:25 UTC (permalink / raw)
  To: svt-core; +Cc: andrey.drobyshev, den

From: Denis V. Lunev <den@openvz.org>

With extended headers a client may name the meta contexts it wants in
a request payload. Our own client never sends one, so the server side
of it, nbd_co_block_status_payload_read(), is not exercised anywhere.

Export two meta contexts and ask libnbd for each of them in turn,
which is only answerable through that path.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
Message-ID: <20260827161002.310688-5-den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/qemu-iotests/tests/nbd-commands     | 14 ++++++++++++--
 tests/qemu-iotests/tests/nbd-commands.out |  4 ++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/tests/nbd-commands b/tests/qemu-iotests/tests/nbd-commands
index 3dfe9131900..09e811724b1 100755
--- a/tests/qemu-iotests/tests/nbd-commands
+++ b/tests/qemu-iotests/tests/nbd-commands
@@ -90,7 +90,7 @@ class TestNbdCommands(iotests.QMPTestCase):
             self.h.shutdown()
             self.h = None
 
-    def block_status(self, count=size):
+    def block_status(self, count=size, wanted=None):
         """Map each meta context in the reply to its list of extents."""
         reply = {}
 
@@ -98,7 +98,10 @@ class TestNbdCommands(iotests.QMPTestCase):
             reply.setdefault(meta, []).extend(zip(entries[0::2],
                                                   entries[1::2]))
 
-        self.h.block_status(count, 0, cb)
+        if wanted is None:
+            self.h.block_status(count, 0, cb)
+        else:
+            self.h.block_status_filter(count, 0, wanted, cb)
         return reply
 
     def top_extents(self):
@@ -148,6 +151,13 @@ class TestNbdCommands(iotests.QMPTestCase):
     def test_commands_extended_headers(self):
         self.check_commands(structured=True, extended=True)
 
+    def test_block_status_payload_filter(self):
+        self.assertEqual(sorted(self.block_status()),
+                         ['base:allocation', 'qemu:allocation-depth'])
+
+        for wanted in (['base:allocation'], ['qemu:allocation-depth']):
+            self.assertEqual(sorted(self.block_status(wanted=wanted)), wanted)
+
     def test_cache_past_end_of_export(self):
         self.assertRaises(nbd.Error, self.h.cache, size + 1, 0)
 
diff --git a/tests/qemu-iotests/tests/nbd-commands.out b/tests/qemu-iotests/tests/nbd-commands.out
index 3f8a935a082..2f7d3902f23 100644
--- a/tests/qemu-iotests/tests/nbd-commands.out
+++ b/tests/qemu-iotests/tests/nbd-commands.out
@@ -1,5 +1,5 @@
-......
+.......
 ----------------------------------------------------------------------
-Ran 6 tests
+Ran 7 tests
 
 OK
-- 
2.53.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH hci-8.0 5/5] iotests/nbd-commands: cover the command flags and sparse replies #VSTOR-119829
  2026-08-31 15:25 [PATCH hci-8.0 0/5] nbd/server: accept a large NBD_CMD_CACHE #VSTOR-119829 Denis V. Lunev
                   ` (3 preceding siblings ...)
  2026-08-31 15:25 ` [PATCH hci-8.0 4/5] iotests/nbd-commands: cover NBD_CMD_BLOCK_STATUS with a payload #VSTOR-119829 Denis V. Lunev
@ 2026-08-31 15:25 ` Denis V. Lunev
  2026-09-01 15:50 ` [QEMU HCI-8.0 PATCH 6/5] VZ: iotests/nbd-commands: do not depend on cluster allocation order #VSTOR-119829 Andrey Drobyshev
  5 siblings, 0 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-31 15:25 UTC (permalink / raw)
  To: svt-core; +Cc: andrey.drobyshev, den

From: Denis V. Lunev <den@openvz.org>

No test sends the NBD command flags, so the server paths behind them
go unexercised. Send them from the new client.

A structured read answers a hole with an offset and a length rather
than a cluster of zeroes. Lay out data, a hole and data again, and
read the three arrangements a hole can appear in, since a leading hole
and a hole between two extents take different turns through
nbd_co_send_sparse_read(). NBD_CMD_FLAG_DF asks for one chunk instead,
which is the same layout sent as real zeroes. Check the chunk
boundaries in both forms, and that the data still reads back.

NBD_CMD_FLAG_REQ_ONE caps the extent array at one entry, so the block
status reply covers only the first cluster of the three asked about.
That a reply may describe less than was requested is also how the
server keeps NBD_MAX_BLOCK_STATUS_EXTENTS from being exceeded on a
long fragmented range, and a client assuming full coverage believes
stale status.

NBD_CMD_FLAG_FAST_ZERO becomes BDRV_REQ_NO_FALLBACK. A cluster aligned
zero can be done by marking the cluster, while zeroing part of a
cluster over a backing file needs the read modify write the flag
forbids, so check both the success and the ENOTSUP.

NBD_CMD_FLAG_FUA is added to a write and a trim.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
Message-ID: <20260827161002.310688-6-den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/qemu-iotests/tests/nbd-commands     | 72 ++++++++++++++++++++++-
 tests/qemu-iotests/tests/nbd-commands.out |  4 +-
 2 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/tests/nbd-commands b/tests/qemu-iotests/tests/nbd-commands
index 09e811724b1..adfd4a495cb 100755
--- a/tests/qemu-iotests/tests/nbd-commands
+++ b/tests/qemu-iotests/tests/nbd-commands
@@ -25,6 +25,7 @@ nbd: ModuleType
 
 DEPTH_LOCAL = 1
 DEPTH_BACKING = 2
+CLUSTER = 65536
 
 
 class TestNbdCommands(iotests.QMPTestCase):
@@ -90,7 +91,7 @@ class TestNbdCommands(iotests.QMPTestCase):
             self.h.shutdown()
             self.h = None
 
-    def block_status(self, count=size, wanted=None):
+    def block_status(self, count=size, wanted=None, flags=0):
         """Map each meta context in the reply to its list of extents."""
         reply = {}
 
@@ -99,9 +100,9 @@ class TestNbdCommands(iotests.QMPTestCase):
                                                   entries[1::2]))
 
         if wanted is None:
-            self.h.block_status(count, 0, cb)
+            self.h.block_status(count, 0, cb, flags)
         else:
-            self.h.block_status_filter(count, 0, wanted, cb)
+            self.h.block_status_filter(count, 0, wanted, cb, flags)
         return reply
 
     def top_extents(self):
@@ -158,6 +159,71 @@ class TestNbdCommands(iotests.QMPTestCase):
         for wanted in (['base:allocation'], ['qemu:allocation-depth']):
             self.assertEqual(sorted(self.block_status(wanted=wanted)), wanted)
 
+    def read_chunks(self, count, offset, flags=0):
+        chunks = []
+
+        def cb(subbuf, off, status, _err):
+            chunks.append((off, len(subbuf), status))
+
+        self.h.pread_structured(count, offset, cb, flags)
+        return chunks
+
+    def make_sparse(self):
+        """Lay out data, a hole and data again, one cluster each."""
+        self.h.pwrite(b'z' * CLUSTER, 0)
+        self.h.zero(CLUSTER, CLUSTER)
+        self.h.pwrite(b'z' * CLUSTER, 2 * CLUSTER)
+
+    def test_read_sparse_chunks(self):
+        self.make_sparse()
+
+        first = (0, CLUSTER, nbd.READ_DATA)
+        hole = (CLUSTER, CLUSTER, nbd.READ_HOLE)
+        second = (2 * CLUSTER, CLUSTER, nbd.READ_DATA)
+
+        # A hole is an offset and a length, not a cluster of zeroes,
+        # wherever it falls in the reply
+        self.assertEqual(self.read_chunks(2 * CLUSTER, 0), [first, hole])
+        self.assertEqual(self.read_chunks(2 * CLUSTER, CLUSTER),
+                         [hole, second])
+        self.assertEqual(self.read_chunks(3 * CLUSTER, 0),
+                         [first, hole, second])
+
+        self.assertEqual(self.h.pread(CLUSTER, CLUSTER), bytes(CLUSTER))
+        self.assertEqual(self.h.pread(CLUSTER, 0), b'z' * CLUSTER)
+
+    def test_read_dont_fragment(self):
+        self.make_sparse()
+
+        self.assertEqual(self.read_chunks(3 * CLUSTER, 0, nbd.CMD_FLAG_DF),
+                         [(0, 3 * CLUSTER, nbd.READ_DATA)])
+
+    def test_block_status_req_one(self):
+        hole = nbd.STATE_HOLE | nbd.STATE_ZERO
+        self.make_sparse()
+
+        alloc = self.block_status(3 * CLUSTER)['base:allocation']
+        self.assertEqual(alloc, [(CLUSTER, 0), (CLUSTER, hole), (CLUSTER, 0)])
+
+        # One extent, so the reply covers less than was asked for and the
+        # client has to come back for the rest
+        alloc = self.block_status(3 * CLUSTER, flags=nbd.CMD_FLAG_REQ_ONE)
+        self.assertEqual(alloc['base:allocation'], [(CLUSTER, 0)])
+
+    def test_write_and_trim_fua(self):
+        self.h.pwrite(b'y' * 4096, 4096, nbd.CMD_FLAG_FUA)
+        self.assertEqual(self.h.pread(4096, 4096), b'y' * 4096)
+        self.h.trim(4096, 4096, nbd.CMD_FLAG_FUA)
+
+    def test_fast_zero(self):
+        self.h.zero(CLUSTER, CLUSTER, nbd.CMD_FLAG_FAST_ZERO)
+        self.assertEqual(self.h.pread(CLUSTER, CLUSTER), bytes(CLUSTER))
+
+        # Zeroing part of a cluster needs the fallback the flag forbids
+        with self.assertRaises(nbd.Error) as caught:
+            self.h.zero(4096, 4096, nbd.CMD_FLAG_FAST_ZERO)
+        self.assertEqual(caught.exception.errno, 'ENOTSUP')
+
     def test_cache_past_end_of_export(self):
         self.assertRaises(nbd.Error, self.h.cache, size + 1, 0)
 
diff --git a/tests/qemu-iotests/tests/nbd-commands.out b/tests/qemu-iotests/tests/nbd-commands.out
index 2f7d3902f23..281b69efeac 100644
--- a/tests/qemu-iotests/tests/nbd-commands.out
+++ b/tests/qemu-iotests/tests/nbd-commands.out
@@ -1,5 +1,5 @@
-.......
+............
 ----------------------------------------------------------------------
-Ran 7 tests
+Ran 12 tests
 
 OK
-- 
2.53.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [QEMU HCI-8.0 PATCH 6/5] VZ: iotests/nbd-commands: do not depend on cluster allocation order #VSTOR-119829
  2026-08-31 15:25 [PATCH hci-8.0 0/5] nbd/server: accept a large NBD_CMD_CACHE #VSTOR-119829 Denis V. Lunev
                   ` (4 preceding siblings ...)
  2026-08-31 15:25 ` [PATCH hci-8.0 5/5] iotests/nbd-commands: cover the command flags and sparse replies #VSTOR-119829 Denis V. Lunev
@ 2026-09-01 15:50 ` Andrey Drobyshev
  5 siblings, 0 replies; 8+ messages in thread
From: Andrey Drobyshev @ 2026-09-01 15:50 UTC (permalink / raw)
  To: svt-core; +Cc: andrey.drobyshev, den

test_cache_copies_on_read compares qemu-img map output verbatim, but
map only merges neighbours that are contiguous in the image file.  Our
copy-on-read runs its 16M chunks concurrently since ce4a1338cf7
("block/io: copy-on-read: use aio task pool"), so the two halves of
the 32M NBD_CMD_CACHE race for cluster allocation and flakily land in
the file in swapped order:

  OK:   {start: 0,   length: 32M, depth: 0, offset:   327680}

  FAIL: {start: 0,   length: 16M, depth: 0, offset: 17104896}
        {start: 16M, length: 16M, depth: 0, offset:   327680}

Same data, same depth, only the host offsets differ.  Merge adjacent
extents of equal depth in top_extents() so that the test compares the
guest layout, not the file layout.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/qemu-iotests/tests/nbd-commands | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/tests/nbd-commands b/tests/qemu-iotests/tests/nbd-commands
index 4c1cd33db74..9a4a0d1ecd8 100755
--- a/tests/qemu-iotests/tests/nbd-commands
+++ b/tests/qemu-iotests/tests/nbd-commands
@@ -105,8 +105,17 @@ class TestNbdCommands(iotests.QMPTestCase):
         """Which parts of the top image are local, once qemu has let go."""
         self.disconnect()
         self.vm.shutdown()
-        return [(e['start'], e['length'], e['depth'])
-                for e in qemu_img_map(top)]
+        # Concurrent copy-on-read tasks may allocate clusters out of
+        # order, and qemu-img map does not merge neighbours that are
+        # discontiguous in the image file.  Only the guest layout
+        # matters here, so merge adjacent extents of equal depth.
+        extents = []
+        for e in qemu_img_map(top):
+            if extents and extents[-1][2] == e['depth']:
+                extents[-1][1] += e['length']
+            else:
+                extents.append([e['start'], e['length'], e['depth']])
+        return [tuple(e) for e in extents]
 
     def test_cache_copies_on_read(self):
         maximum = self.h.get_block_size(nbd.SIZE_MAXIMUM)
-- 
2.47.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH hci-8.0 1/5] iotests: add coverage for NBD transmission commands #VSTOR-119829
  2026-08-31 15:25 ` [PATCH hci-8.0 1/5] iotests: add coverage for NBD transmission commands #VSTOR-119829 Denis V. Lunev
@ 2026-09-01 15:51   ` Andrey Drobyshev
  0 siblings, 0 replies; 8+ messages in thread
From: Andrey Drobyshev @ 2026-09-01 15:51 UTC (permalink / raw)
  To: Denis V. Lunev, svt-core

On 8/31/26 6:25 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> NBD_CMD_CACHE has no coverage anywhere in the tree. Nothing ever
> sends it: our own NBD client does not implement the command, and
> neither qemu-io nor 'qemu-nbd --list' can issue one, so the only
> clients reaching this server path are external ones.
> 
> Add a test driven by libnbd, gated the way nbd-multiconn already is,
> and start it with the case the command exists for. The export is a
> qcow2 image over a fully written backing file, so a prefetch has
> visible work to do.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> Message-ID: <20260827161002.310688-2-den@openvz.org>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  tests/qemu-iotests/tests/nbd-commands     | 140 ++++++++++++++++++++++
>  tests/qemu-iotests/tests/nbd-commands.out |   5 +
>  2 files changed, 145 insertions(+)
>  create mode 100755 tests/qemu-iotests/tests/nbd-commands
>  create mode 100644 tests/qemu-iotests/tests/nbd-commands.out
> 
> diff --git a/tests/qemu-iotests/tests/nbd-commands b/tests/qemu-iotests/tests/nbd-commands
> new file mode 100755
> index 00000000000..4c1cd33db74
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/nbd-commands
> @@ -0,0 +1,140 @@
> +#!/usr/bin/env python3
> +# group: rw auto quick
> +#
> +# Test NBD transmission commands against a qemu NBD export
> +#
> +# Copyright (C) 2026 Virtuozzo International GmbH
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import os
> +from types import ModuleType
> +
> +import iotests
> +from iotests import qemu_img_create, qemu_img_map, qemu_io
> +
> +
> +base = os.path.join(iotests.test_dir, 'base')
> +top = os.path.join(iotests.test_dir, 'top')
> +# Larger than the maximum payload size an export can advertise
> +size = 64 * 1024 * 1024
> +pattern = 0xa5
> +nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock')
> +nbd_uri = 'nbd+unix:///exp?socket=' + nbd_sock
> +nbd: ModuleType
> +
> +DEPTH_LOCAL = 1
> +DEPTH_BACKING = 2
> +
> +
> +class TestNbdCommands(iotests.QMPTestCase):
> +    def setUp(self):
> +        qemu_img_create('-f', iotests.imgfmt, base, str(size))
> +        qemu_io('-c', f'write -P {pattern} 0 {size}', base)
> +        qemu_img_create('-f', iotests.imgfmt, '-b', base,
> +                        '-F', iotests.imgfmt, top, str(size))
> +
> +        self.vm = iotests.VM()
> +        self.vm.launch()
> +        self.vm.cmd('blockdev-add', {
> +            'driver': iotests.imgfmt,
> +            'node-name': 'n',
> +            'file': {'driver': 'file', 'filename': top},
> +            'backing': {
> +                'driver': iotests.imgfmt,
> +                'node-name': 'base',
> +                'file': {'driver': 'file', 'filename': base},
> +            },
> +        })
> +        self.vm.cmd('nbd-server-start', {
> +            'addr': {'type': 'unix', 'data': {'path': nbd_sock}}
> +        })
> +        self.vm.cmd('block-export-add', {
> +            'type': 'nbd',
> +            'id': 'exp',
> +            'node-name': 'n',
> +            'name': 'exp',
> +            'writable': True,
> +            'allocation-depth': True,
> +        })
> +
> +        self.h = None
> +        self.connect()
> +
> +    def tearDown(self):
> +        self.disconnect()
> +        self.vm.shutdown()
> +        for f in (top, base, nbd_sock):
> +            try:
> +                os.remove(f)
> +            except OSError:
> +                pass
> +
> +    def connect(self, structured=True, extended=True):
> +        self.disconnect()
> +        h = nbd.NBD()
> +        h.set_request_structured_replies(structured)
> +        h.set_request_extended_headers(extended)
> +        h.add_meta_context('base:allocation')
> +        h.add_meta_context('qemu:allocation-depth')
> +        # Let the server, not libnbd, reject the out of range requests below
> +        h.set_strict_mode(h.get_strict_mode() &
> +                          ~(nbd.STRICT_BOUNDS | nbd.STRICT_PAYLOAD))
> +        h.connect_uri(nbd_uri)
> +        self.assertEqual(h.get_structured_replies_negotiated(), structured)
> +        self.assertEqual(h.get_extended_headers_negotiated(), extended)
> +        self.h = h
> +
> +    def disconnect(self):
> +        if self.h is not None:
> +            self.h.shutdown()
> +            self.h = None
> +
> +    def block_status(self, count=size):
> +        """Map each meta context in the reply to its list of extents."""
> +        reply = {}
> +
> +        def cb(meta, _offset, entries, _err):
> +            reply.setdefault(meta, []).extend(zip(entries[0::2],
> +                                                  entries[1::2]))
> +
> +        self.h.block_status(count, 0, cb)
> +        return reply
> +
> +    def top_extents(self):
> +        """Which parts of the top image are local, once qemu has let go."""
> +        self.disconnect()
> +        self.vm.shutdown()
> +        return [(e['start'], e['length'], e['depth'])
> +                for e in qemu_img_map(top)]
This test is broken in our downstream because of commit ce4a1338cf7
("block/io: copy-on-read: use aio task pool").  The test fails flakily.
Namely we get on qemu_img_map():

OK run:
  {start: 0,        length: 33554432, depth: 0, offset:   327680}
  {start: 33554432, length: 33554432, depth: 1, offset: 33882112}

FAIL run:
  {start: 0,        length: 16777216, depth: 0, offset: 17104896}
  {start: 16777216, length: 16777216, depth: 0, offset:   327680}
  {start: 33554432, length: 33554432, depth: 1, offset: 33882112}

I.e. it's same data, but cluster allocation order is broken.  I'll send
a follow up fix.

Andrey

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-09-01 15:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-31 15:25 [PATCH hci-8.0 0/5] nbd/server: accept a large NBD_CMD_CACHE #VSTOR-119829 Denis V. Lunev
2026-08-31 15:25 ` [PATCH hci-8.0 1/5] iotests: add coverage for NBD transmission commands #VSTOR-119829 Denis V. Lunev
2026-09-01 15:51   ` Andrey Drobyshev
2026-08-31 15:25 ` [PATCH hci-8.0 2/5] nbd/server: accept NBD_CMD_CACHE above the maximum payload size #VSTOR-119829 Denis V. Lunev
2026-08-31 15:25 ` [PATCH hci-8.0 3/5] iotests/nbd-commands: exercise the simple and structured reply modes #VSTOR-119829 Denis V. Lunev
2026-08-31 15:25 ` [PATCH hci-8.0 4/5] iotests/nbd-commands: cover NBD_CMD_BLOCK_STATUS with a payload #VSTOR-119829 Denis V. Lunev
2026-08-31 15:25 ` [PATCH hci-8.0 5/5] iotests/nbd-commands: cover the command flags and sparse replies #VSTOR-119829 Denis V. Lunev
2026-09-01 15:50 ` [QEMU HCI-8.0 PATCH 6/5] VZ: iotests/nbd-commands: do not depend on cluster allocation order #VSTOR-119829 Andrey Drobyshev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox