[PATCH 8/8] iotests: Add coverage of recent NBD qio deadlock fix

Eric Blake posted 8 patches 1 week, 5 days ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, Eric Blake <eblake@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
There is a newer version of this series
[PATCH 8/8] iotests: Add coverage of recent NBD qio deadlock fix
Posted by Eric Blake 1 week, 5 days ago
Test that all images in a qcow2 chain using an NBD backing file can be
served by the same process.  Prior to the recent QIONetListener fixes,
this test would demonstrate deadlock.

The test borrows heavily from the original formula by "John Doe" in
the gitlab bug, but uses a Unix socket rather than TCP to avoid port
contention, and uses a full-blown QEMU rather than qemu-storage-daemon
since both programs were impacted.

[While preparing this patch by making the new test executable, I
noticed vvfat.out does not need execute permissions]

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3169
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/tests/nbd-in-qcow2-chain   | 84 +++++++++++++++++++
 .../qemu-iotests/tests/nbd-in-qcow2-chain.out | 56 +++++++++++++
 tests/qemu-iotests/tests/vvfat.out            |  0
 3 files changed, 140 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-in-qcow2-chain
 create mode 100644 tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
 mode change 100755 => 100644 tests/qemu-iotests/tests/vvfat.out

diff --git a/tests/qemu-iotests/tests/nbd-in-qcow2-chain b/tests/qemu-iotests/tests/nbd-in-qcow2-chain
new file mode 100755
index 00000000000..b89f74d4552
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-in-qcow2-chain
@@ -0,0 +1,84 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test of opening both server and client NBD in a qcow2 backing chain
+#
+# Copyright (C) Red Hat, Inc.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+# creator
+owner=eblake@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1    # failure is the default!
+
+_cleanup()
+{
+    _cleanup_qemu
+    _cleanup_test_img
+    rm -f "$SOCK_DIR/nbd"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+. ./common.nbd
+
+_supported_fmt qcow2  # Hardcoded to qcow2 command line and QMP below
+_supported_proto file
+
+size=100M
+
+echo
+echo "=== Preparing base image ==="
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+
+echo
+echo "=== Starting QEMU and exposing base image ==="
+
+_launch_qemu -machine q35
+h1=$QEMU_HANDLE
+_send_qemu_cmd $QEMU_HANDLE '{"execute": "qmp_capabilities"}' 'return'
+_send_qemu_cmd $QEMU_HANDLE '{"execute": "blockdev-add",
+  "arguments": {"node-name":"base", "driver":"qcow2",
+     "file":{"driver":"file", "filename":"'"$TEST_IMG.base"'"}}}' 'return'
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
+  "arguments": {"addr":{"type":"unix",
+    "data":{"path":"'"$SOCK_DIR/nbd"'"}}}}' 'return'
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments": {"device":"base","name":"base"}}' 'return'
+
+echo
+echo "=== Creating wrapper image ==="
+
+_make_test_img -F raw -b "nbd+unix:///base?socket=$SOCK_DIR/nbd" $size
+
+echo
+echo "=== Adding wrapper image ==="
+
+_send_qemu_cmd $QEMU_HANDLE '{"execute": "blockdev-add",
+  "arguments": {"node-name":"wrap", "driver":"qcow2",
+     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' 'return'
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments": {"device":"wrap","name":"wrap"}}' 'return'
+
+echo
+echo "=== Checking NBD server ==="
+
+$QEMU_NBD --list -k $SOCK_DIR/nbd
+
+echo
+echo "=== Cleaning up ==="
+
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' ''
+
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/nbd-in-qcow2-chain.out b/tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
new file mode 100644
index 00000000000..5f1d31ae2e0
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
@@ -0,0 +1,56 @@
+QA output created by nbd-in-qcow2-chain
+
+=== Preparing base image ===
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=104857600
+
+=== Starting QEMU and exposing base image ===
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "blockdev-add",
+  "arguments": {"node-name":"base", "driver":"IMGFMT",
+     "file":{"driver":"file", "filename":"TEST_DIR/t.IMGFMT.base"}}}
+{"return": {}}
+{"execute":"nbd-server-start",
+  "arguments": {"addr":{"type":"unix",
+    "data":{"path":"SOCK_DIR/nbd"}}}}
+{"return": {}}
+{"execute":"nbd-server-add",
+  "arguments": {"device":"base","name":"base"}}
+{"return": {}}
+
+=== Creating wrapper image ===
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 backing_file=nbd+unix:///base?socket=SOCK_DIR/nbd backing_fmt=raw
+
+=== Adding wrapper image ===
+{"execute": "blockdev-add",
+  "arguments": {"node-name":"wrap", "driver":"IMGFMT",
+     "file":{"driver":"file", "filename":"TEST_DIR/t.IMGFMT"}}}
+{"return": {}}
+{"execute":"nbd-server-add",
+  "arguments": {"device":"wrap","name":"wrap"}}
+{"return": {}}
+
+=== Checking NBD server ===
+exports available: 2
+ export: 'base'
+  size:  104857600
+  flags: 0x158f ( readonly flush fua df multi cache block-status-payload )
+  min block: 1
+  opt block: 4096
+  max block: 33554432
+  transaction size: 64-bit
+  available meta contexts: 1
+   base:allocation
+ export: 'wrap'
+  size:  104857600
+  flags: 0x158f ( readonly flush fua df multi cache block-status-payload )
+  min block: 1
+  opt block: 4096
+  max block: 33554432
+  transaction size: 64-bit
+  available meta contexts: 1
+   base:allocation
+
+=== Cleaning up ===
+{"execute":"quit"}
+*** done
diff --git a/tests/qemu-iotests/tests/vvfat.out b/tests/qemu-iotests/tests/vvfat.out
old mode 100755
new mode 100644
-- 
2.51.1
Re: [PATCH 8/8] iotests: Add coverage of recent NBD qio deadlock fix
Posted by Vladimir Sementsov-Ogievskiy 1 week, 4 days ago
On 03.11.25 23:10, Eric Blake wrote:
> Test that all images in a qcow2 chain using an NBD backing file can be
> served by the same process.  Prior to the recent QIONetListener fixes,
> this test would demonstrate deadlock.
> 
> The test borrows heavily from the original formula by "John Doe" in
> the gitlab bug, but uses a Unix socket rather than TCP to avoid port
> contention, and uses a full-blown QEMU rather than qemu-storage-daemon
> since both programs were impacted.
> 
> [While preparing this patch by making the new test executable, I
> noticed vvfat.out does not need execute permissions]
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3169
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/qemu-iotests/tests/nbd-in-qcow2-chain   | 84 +++++++++++++++++++
>   .../qemu-iotests/tests/nbd-in-qcow2-chain.out | 56 +++++++++++++
>   tests/qemu-iotests/tests/vvfat.out            |  0
>   3 files changed, 140 insertions(+)
>   create mode 100755 tests/qemu-iotests/tests/nbd-in-qcow2-chain
>   create mode 100644 tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
>   mode change 100755 => 100644 tests/qemu-iotests/tests/vvfat.out
> 
> diff --git a/tests/qemu-iotests/tests/nbd-in-qcow2-chain b/tests/qemu-iotests/tests/nbd-in-qcow2-chain
> new file mode 100755
> index 00000000000..b89f74d4552
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/nbd-in-qcow2-chain
> @@ -0,0 +1,84 @@
> +#!/usr/bin/env bash
> +# group: rw quick
> +#
> +# Test of opening both server and client NBD in a qcow2 backing chain
> +#
> +# Copyright (C) Red Hat, Inc.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +# creator
> +owner=eblake@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +status=1    # failure is the default!
> +
> +_cleanup()
> +{
> +    _cleanup_qemu
> +    _cleanup_test_img
> +    rm -f "$SOCK_DIR/nbd"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +cd ..
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +. ./common.nbd
> +
> +_supported_fmt qcow2  # Hardcoded to qcow2 command line and QMP below
> +_supported_proto file
> +
> +size=100M
> +
> +echo
> +echo "=== Preparing base image ==="
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img $size
> +
> +echo
> +echo "=== Starting QEMU and exposing base image ==="
> +
> +_launch_qemu -machine q35
> +h1=$QEMU_HANDLE
> +_send_qemu_cmd $QEMU_HANDLE '{"execute": "qmp_capabilities"}' 'return'
> +_send_qemu_cmd $QEMU_HANDLE '{"execute": "blockdev-add",
> +  "arguments": {"node-name":"base", "driver":"qcow2",
> +     "file":{"driver":"file", "filename":"'"$TEST_IMG.base"'"}}}' 'return'
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
> +  "arguments": {"addr":{"type":"unix",
> +    "data":{"path":"'"$SOCK_DIR/nbd"'"}}}}' 'return'
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> +  "arguments": {"device":"base","name":"base"}}' 'return'
> +
> +echo
> +echo "=== Creating wrapper image ==="
> +
> +_make_test_img -F raw -b "nbd+unix:///base?socket=$SOCK_DIR/nbd" $size
> +
> +echo
> +echo "=== Adding wrapper image ==="
> +
> +_send_qemu_cmd $QEMU_HANDLE '{"execute": "blockdev-add",
> +  "arguments": {"node-name":"wrap", "driver":"qcow2",
> +     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' 'return'

Hmm. Why don't you specify "backing": "base" here?

> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> +  "arguments": {"device":"wrap","name":"wrap"}}' 'return'
> +
> +echo
> +echo "=== Checking NBD server ==="
> +
> +$QEMU_NBD --list -k $SOCK_DIR/nbd
> +
> +echo
> +echo "=== Cleaning up ==="
> +
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' ''
> +
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/tests/nbd-in-qcow2-chain.out b/tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
> new file mode 100644
> index 00000000000..5f1d31ae2e0
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
> @@ -0,0 +1,56 @@
> +QA output created by nbd-in-qcow2-chain
> +
> +=== Preparing base image ===
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=104857600
> +
> +=== Starting QEMU and exposing base image ===
> +{"execute": "qmp_capabilities"}
> +{"return": {}}
> +{"execute": "blockdev-add",
> +  "arguments": {"node-name":"base", "driver":"IMGFMT",
> +     "file":{"driver":"file", "filename":"TEST_DIR/t.IMGFMT.base"}}}
> +{"return": {}}
> +{"execute":"nbd-server-start",
> +  "arguments": {"addr":{"type":"unix",
> +    "data":{"path":"SOCK_DIR/nbd"}}}}
> +{"return": {}}
> +{"execute":"nbd-server-add",
> +  "arguments": {"device":"base","name":"base"}}
> +{"return": {}}
> +
> +=== Creating wrapper image ===
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 backing_file=nbd+unix:///base?socket=SOCK_DIR/nbd backing_fmt=raw
> +
> +=== Adding wrapper image ===
> +{"execute": "blockdev-add",
> +  "arguments": {"node-name":"wrap", "driver":"IMGFMT",
> +     "file":{"driver":"file", "filename":"TEST_DIR/t.IMGFMT"}}}
> +{"return": {}}
> +{"execute":"nbd-server-add",
> +  "arguments": {"device":"wrap","name":"wrap"}}
> +{"return": {}}
> +
> +=== Checking NBD server ===
> +exports available: 2
> + export: 'base'
> +  size:  104857600
> +  flags: 0x158f ( readonly flush fua df multi cache block-status-payload )
> +  min block: 1
> +  opt block: 4096
> +  max block: 33554432
> +  transaction size: 64-bit
> +  available meta contexts: 1
> +   base:allocation
> + export: 'wrap'
> +  size:  104857600
> +  flags: 0x158f ( readonly flush fua df multi cache block-status-payload )
> +  min block: 1
> +  opt block: 4096
> +  max block: 33554432
> +  transaction size: 64-bit
> +  available meta contexts: 1
> +   base:allocation
> +
> +=== Cleaning up ===
> +{"execute":"quit"}
> +*** done
> diff --git a/tests/qemu-iotests/tests/vvfat.out b/tests/qemu-iotests/tests/vvfat.out
> old mode 100755
> new mode 100644


-- 
Best regards,
Vladimir
Re: [PATCH 8/8] iotests: Add coverage of recent NBD qio deadlock fix
Posted by Eric Blake 1 week, 3 days ago
On Tue, Nov 04, 2025 at 02:38:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 03.11.25 23:10, Eric Blake wrote:
> > Test that all images in a qcow2 chain using an NBD backing file can be
> > served by the same process.  Prior to the recent QIONetListener fixes,
> > this test would demonstrate deadlock.
> > 
> > The test borrows heavily from the original formula by "John Doe" in
> > the gitlab bug, but uses a Unix socket rather than TCP to avoid port
> > contention, and uses a full-blown QEMU rather than qemu-storage-daemon
> > since both programs were impacted.
> > 
> > [While preparing this patch by making the new test executable, I
> > noticed vvfat.out does not need execute permissions]
> > 
> > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3169
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >   tests/qemu-iotests/tests/nbd-in-qcow2-chain   | 84 +++++++++++++++++++
> >   .../qemu-iotests/tests/nbd-in-qcow2-chain.out | 56 +++++++++++++
> >   tests/qemu-iotests/tests/vvfat.out            |  0
> >   3 files changed, 140 insertions(+)
> >   create mode 100755 tests/qemu-iotests/tests/nbd-in-qcow2-chain
> >   create mode 100644 tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
> >   mode change 100755 => 100644 tests/qemu-iotests/tests/vvfat.out

Should I split out that file mode change to a separate cleanup patch?

> > 
> > diff --git a/tests/qemu-iotests/tests/nbd-in-qcow2-chain b/tests/qemu-iotests/tests/nbd-in-qcow2-chain
> > new file mode 100755
> > index 00000000000..b89f74d4552
> > --- /dev/null
> > +++ b/tests/qemu-iotests/tests/nbd-in-qcow2-chain

> > +echo
> > +echo "=== Creating wrapper image ==="
> > +
> > +_make_test_img -F raw -b "nbd+unix:///base?socket=$SOCK_DIR/nbd" $size
> > +
> > +echo
> > +echo "=== Adding wrapper image ==="
> > +
> > +_send_qemu_cmd $QEMU_HANDLE '{"execute": "blockdev-add",
> > +  "arguments": {"node-name":"wrap", "driver":"qcow2",
> > +     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' 'return'
> 
> Hmm. Why don't you specify "backing": "base" here?

Because the original bug report didn't either.  However, I can see the
wisdom in enhancing the test to cover multiple scenarios: both a
backing chain learned only by what is in the qcow2 file, and an
explicit backing chain where the NBD client is spelled out in the QMP
code.  I'll see if I can enhance that for v2.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org
Re: [PATCH 8/8] iotests: Add coverage of recent NBD qio deadlock fix
Posted by Kevin Wolf 1 week, 2 days ago
Am 05.11.2025 um 23:10 hat Eric Blake geschrieben:
> On Tue, Nov 04, 2025 at 02:38:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 03.11.25 23:10, Eric Blake wrote:
> > > Test that all images in a qcow2 chain using an NBD backing file can be
> > > served by the same process.  Prior to the recent QIONetListener fixes,
> > > this test would demonstrate deadlock.
> > > 
> > > The test borrows heavily from the original formula by "John Doe" in
> > > the gitlab bug, but uses a Unix socket rather than TCP to avoid port
> > > contention, and uses a full-blown QEMU rather than qemu-storage-daemon
> > > since both programs were impacted.
> > > 
> > > [While preparing this patch by making the new test executable, I
> > > noticed vvfat.out does not need execute permissions]
> > > 
> > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3169
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > >   tests/qemu-iotests/tests/nbd-in-qcow2-chain   | 84 +++++++++++++++++++
> > >   .../qemu-iotests/tests/nbd-in-qcow2-chain.out | 56 +++++++++++++
> > >   tests/qemu-iotests/tests/vvfat.out            |  0
> > >   3 files changed, 140 insertions(+)
> > >   create mode 100755 tests/qemu-iotests/tests/nbd-in-qcow2-chain
> > >   create mode 100644 tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
> > >   mode change 100755 => 100644 tests/qemu-iotests/tests/vvfat.out
> 
> Should I split out that file mode change to a separate cleanup patch?

It's an unrelated change, so while the patch to change only the file
mode may look funny, it's probably better to split it.

Kevin
Re: [PATCH 8/8] iotests: Add coverage of recent NBD qio deadlock fix
Posted by Vladimir Sementsov-Ogievskiy 1 week, 2 days ago
On 06.11.25 01:10, Eric Blake wrote:
> On Tue, Nov 04, 2025 at 02:38:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 03.11.25 23:10, Eric Blake wrote:
>>> Test that all images in a qcow2 chain using an NBD backing file can be
>>> served by the same process.  Prior to the recent QIONetListener fixes,
>>> this test would demonstrate deadlock.
>>>
>>> The test borrows heavily from the original formula by "John Doe" in
>>> the gitlab bug, but uses a Unix socket rather than TCP to avoid port
>>> contention, and uses a full-blown QEMU rather than qemu-storage-daemon
>>> since both programs were impacted.
>>>
>>> [While preparing this patch by making the new test executable, I
>>> noticed vvfat.out does not need execute permissions]
>>>
>>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3169
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>    tests/qemu-iotests/tests/nbd-in-qcow2-chain   | 84 +++++++++++++++++++
>>>    .../qemu-iotests/tests/nbd-in-qcow2-chain.out | 56 +++++++++++++
>>>    tests/qemu-iotests/tests/vvfat.out            |  0
>>>    3 files changed, 140 insertions(+)
>>>    create mode 100755 tests/qemu-iotests/tests/nbd-in-qcow2-chain
>>>    create mode 100644 tests/qemu-iotests/tests/nbd-in-qcow2-chain.out
>>>    mode change 100755 => 100644 tests/qemu-iotests/tests/vvfat.out
> 
> Should I split out that file mode change to a separate cleanup patch?

Probably. Personally, I don't care.

> 
>>>
>>> diff --git a/tests/qemu-iotests/tests/nbd-in-qcow2-chain b/tests/qemu-iotests/tests/nbd-in-qcow2-chain
>>> new file mode 100755
>>> index 00000000000..b89f74d4552
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/tests/nbd-in-qcow2-chain
> 
>>> +echo
>>> +echo "=== Creating wrapper image ==="
>>> +
>>> +_make_test_img -F raw -b "nbd+unix:///base?socket=$SOCK_DIR/nbd" $size
>>> +
>>> +echo
>>> +echo "=== Adding wrapper image ==="
>>> +
>>> +_send_qemu_cmd $QEMU_HANDLE '{"execute": "blockdev-add",
>>> +  "arguments": {"node-name":"wrap", "driver":"qcow2",
>>> +     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' 'return'
>>
>> Hmm. Why don't you specify "backing": "base" here?
> 
> Because the original bug report didn't either.

Ah, I missed the idea: we setup our qcow2 image, so that backing is a
nbd protocol uri. So "backing": "base" would be wrong

>  However, I can see the
> wisdom in enhancing the test to cover multiple scenarios: both a
> backing chain learned only by what is in the qcow2 file, and an
> explicit backing chain where the NBD client is spelled out in the QMP
> code.  I'll see if I can enhance that for v2.
> 
> 

Not sure, that it will add real value to the test.. It should be the same
block-graph finally. If you decide keep it as is (or with vvfat fix dropped):

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>


-- 
Best regards,
Vladimir