Re: [PATCH hci-8.0 5/5] qcow2: repair a dirty image when it becomes writable #VSTOR-140073

["Denis V. Lunev" <den@virtuozzo.com>]

pls answer to mainstream first properly, I'll send there and here

Den

On 9/16/26 18:26, Andrey Drobyshev wrote:
> Other commits appear to be fine, but this commit has several issues:
>
>   - Major: a failed repair makes a readable image unreadable.
> state->bs->drv = NULL at block/qcow2.c:2251 blocks reads as well as
> writes. The base of a running chain, or the active layer in the
> stream_start() case, answers every guest read with ENOMEDIUM from then
> on, for an image whose L1/L2 metadata is intact and was serving reads a
> moment earlier. Test 039's own expected output shows it: read failed: No
> medium found. Only allocation has to be stopped. A per-node gate in
> BDRVQcow2State that fails pwritev/pwrite_zeroes/pdiscard/truncate with
> -EIO (and skips qcow2_mark_clean() in qcow2_inactivate() so the dirty
> bit survives close) keeps the guest running and the job simply fails.
> The current approach also makes query-named-block-nodes/query-block fail
> wholesale while the node exists, and in the commit case the node cannot
> be deleted until the VM is restarted.
>
>   - Major: use-after-free through the cache-clean coroutine. With
> bs->drv == NULL, bdrv_close() (block.c:5259) never calls qcow2_close(),
> so cache_clean_timer_del_and_wait() never runs, and bdrv_close() then
> frees bs->opaque. The coroutine at block/qcow2.c:895 is still sleeping
> on s->cache_clean_timer_wake (600 s default on Linux) and wakes on freed
> memory. Pre-existing for qcow2_signal_corruption(fatal), but this patch
> adds a second path and is the right place to stop the coroutine before
> dropping the driver. The same skipped close also leaks the L1 table,
> refcount table, both caches, crypto and bitmap state.
>
>   - Major: the check runs under bdrv_drain_all_begin().
> bdrv_reopen_queue() drains every node (block.c:4593), and bdrv_check()
> polls from the main loop. A full refcount scan plus rebuild of a large
> image stalls all guest I/O on every disk and the monitor for its whole
> duration, as a side effect of
> block-commit/block-stream/change-backing-file. The QAPI text discloses
> it, but the cover letter should quantify it and argue why this beats
> failing the command.
>
>   - Medium (downstream only): the new condition ignores s->keep_dirty,
> unlike qcow2_do_open() at block/qcow2.c:2025. A node opened with
> __vz_keep-dirty=on, then reopened read-write, gets checked and its dirty
> bit cleared, breaking that option's contract. Add !s->keep_dirty in the
> hci branch.
>
>   - Low: after the failure the node keeps BDRV_O_RDWR, BLK_PERM_WRITE on
> bs->file and the write lock, so the qemu-img check -r the hint
> recommends is impossible until the node is gone, which for a commit base
> means VM shutdown. Say so in the hint or the docs.
>
>   - Low: test 040 asserts exact error strings owned by other code ("Node
> 'base' is busy: node is used as backing hd of 'mid'", "Block device base
> is ejected"), which makes it brittle.
>
> On 9/16/26 6:24 PM, Denis V. Lunev wrote:
>> From: Denis V. Lunev <den@openvz.org>
>>
>> A dirty image must be repaired before anything allocates a cluster in
>> it. qcow2_do_open() does that, but only for a node that is writable
>> from the start. A node opened read-only skips it, and nothing revisits
>> the question once that node becomes writable, which block-commit does
>> routinely: commit_active_start() and commit_start() reopen the base
>> read-write for the duration of the job.
>>
>> With lazy refcounts the on-disk refcount block then still accounts for
>> the metadata clusters only, so the allocator restarts at the front of
>> the image and hands out clusters that L2 entries point at. Two guest
>> offsets end up sharing one host cluster. Nothing fails, the corrupt bit
>> stays clear, and a clean close clears the dirty bit, so no later open
>> repairs the image either. The bit also stays set for the whole writable
>> session, so a node which is merely writable says nothing.
>>
>> Refusing the reopen instead is simpler and keeps it atomic, but it
>> leaves nowhere to go: the base belongs to a chain the VM has open, so
>> the qemu-img check -r such an error would ask for cannot take the write
>> lock it needs. The repair does the trick in most cases anyway.
>>
>> Do the repair in qcow2_reopen_commit_post(), the earliest point where
>> the node is writable. An inactive node is skipped: bdrv_activate() calls
>> qcow2_do_open() again through qcow2_co_invalidate_cache().
>>
>> commit_post cannot reject the reopen, so a failed repair takes the
>> driver away from the node instead, which is what stops writes from
>> aliasing live clusters. qcow2_signal_corruption() does that as well,
>> but it also sends BLOCK_IMAGE_CORRUPTED and sets the corrupt bit,
>> which qcow2_do_open() honours by refusing every later read-write open.
>> The image is dirty and unrepaired, not corrupt, and qemu-img check -r
>> still fixes it, so neither belongs here. Return the error and skip
>> the bitmaps.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Hanna Reitz <hreitz@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> Cc: qemu-stable@nongnu.org
>> ---
>>  block/qcow2.c              |  21 +++++++
>>  qapi/block-core.json       |  19 ++++++
>>  tests/qemu-iotests/039     |  60 ++++++++++++++++++
>>  tests/qemu-iotests/039.out |  36 +++++++++++
>>  tests/qemu-iotests/040     | 123 +++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/040.out |   4 +-
>>  6 files changed, 261 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index ddad39a3084..be346f0339b 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2235,8 +2235,29 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
>>  
>>  static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp)
>>  {
>> +    ERRP_GUARD();
>> +    BDRVQcow2State *s = state->bs->opaque;
>> +
>>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>>  
>> +    if (!bdrv_reopen_was_writable(state) && bdrv_is_writable(state->bs) &&
>> +        (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
>> +        BdrvCheckResult result = {0};
>> +        int ret;
>> +
>> +        ret = bdrv_check(state->bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
>> +        if (ret < 0 || result.check_errors || !state->bs->drv) {
>> +            ret = ret < 0 ? ret : -EIO;
>> +            /* No write may reach an image whose refcounts are unaccounted */
>> +            state->bs->drv = NULL;
>> +            error_setg_errno(errp, -ret, "Could not repair dirty image '%s'",
>> +                             bdrv_get_device_or_node_name(state->bs));
>> +            error_append_hint(errp, "The image is left dirty and this node "
>> +                              "holds it open until the node is removed\n");
>> +            return ret;
>> +        }
>> +    }
>> +
>>      if (state->flags & BDRV_O_RDWR) {
>>          Error *local_err = NULL;
>>  
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index c036a3a8858..6fb8c31210a 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1843,6 +1843,9 @@
>>  # r/w -> r/o, if needed).  The new backing file string is written into
>>  # the image file metadata, and the QEMU internal strings are updated.
>>  #
>> +# A dirty qcow2 image is repaired during that reopen, which blocks
>> +# other requests and can fail the command.
>> +#
>>  # @image-node-name: The name of the block driver state node of the
>>  #     image to modify.  The "device" argument is used to verify
>>  #     "image-node-name" is in the chain described by "device".
>> @@ -1882,6 +1885,9 @@
>>  # size to match the size of the smaller top, you can safely truncate
>>  # it yourself once the commit operation successfully completes.
>>  #
>> +# The base is opened read-write.  A dirty qcow2 base is repaired
>> +# first, which blocks other requests and can fail the command.
>> +#
>>  # @job-id: identifier for the newly-created block job.  If omitted,
>>  #     the device name will be used.  (Since 2.7)
>>  #
>> @@ -2919,6 +2925,9 @@
>>  # On successful completion the image file is updated to drop the
>>  # backing file and the `BLOCK_JOB_COMPLETED` event is emitted.
>>  #
>> +# The top image is opened read-write.  A dirty qcow2 image is repaired
>> +# first, which blocks other requests and can fail the command.
>> +#
>>  # In case @device is a filter node, `block-stream` modifies the first
>>  # non-filter overlay node below it to point to the new backing node
>>  # instead of modifying @device itself.
>> @@ -5070,6 +5079,16 @@
>>  # transaction, so if one of them fails then the whole transaction is
>>  # cancelled.
>>  #
>> +# An error is also returned when a device cannot be used once it has
>> +# been reopened.  Such a reopen is not undone, so an error does not
>> +# always mean that nothing has changed.  A node the driver gave up on
>> +# serves nothing at all: it keeps its image open, makes
>> +# `query-named-block-nodes` fail for as long as it is in the graph,
>> +# and `blockdev-del` removes it only once nothing refers to it.
>> +#
>> +# Reopening a dirty qcow2 image read-write repairs it first, which
>> +# blocks other requests and can fail the command.
>> +#
>>  # The command receives a list of block devices to reopen.  For each
>>  # one of them, the top-level @node-name option (from `BlockdevOptions`)
>>  # must be specified and is used to select the block device to be
>> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
>> index 3d0c073d65d..3f37c36ca86 100755
>> --- a/tests/qemu-iotests/039
>> +++ b/tests/qemu-iotests/039
>> @@ -33,6 +33,7 @@ status=1	# failure is the default!
>>  _cleanup()
>>  {
>>  	_cleanup_test_img
>> +	rm -f "$TEST_DIR/blkdebug.conf"
>>  }
>>  trap "_cleanup; exit \$status" 0 1 2 3 15
>>  
>> @@ -176,6 +177,65 @@ $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io
>>  # The dirty bit must not be set
>>  _qcow2_dump_header | grep incompatible_features
>>  
>> +echo
>> +echo "== Reopening a dirty image read/write should repair it =="
>> +
>> +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
>> +
>> +_NO_VALGRIND \
>> +$QEMU_IO -c "write -P 0x5a 0 512" \
>> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>> +    | _filter_qemu_io
>> +
>> +# The dirty bit must be set
>> +_qcow2_dump_header | grep incompatible_features
>> +
>> +# Without the repair this write would alias the cluster at offset 0
>> +$QEMU_IO -r -c "reopen -w" \
>> +            -c "write -P 0xb1 1M 512" \
>> +            -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
>> +
>> +_check_test_img
>> +
>> +echo
>> +echo "== A read/write reopen must not check the image =="
>> +
>> +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
>> +
>> +_NO_VALGRIND \
>> +$QEMU_IO -c "write -P 0x5a 0 512" \
>> +         -c "reopen -o l2-cache-size=1M" \
>> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>> +    | _filter_qemu_io
>> +
>> +# The dirty bit must still be set, it belongs to the running session
>> +_qcow2_dump_header | grep incompatible_features
>> +
>> +echo
>> +echo "== A failed repair must fail the reopen =="
>> +
>> +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
>> +
>> +_NO_VALGRIND \
>> +$QEMU_IO -c "write -P 0x5a 0 512" \
>> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>> +    | _filter_qemu_io
>> +
>> +cat > "$TEST_DIR/blkdebug.conf" <<EOF
>> +[inject-error]
>> +event = "none"
>> +iotype = "write"
>> +errno = "5"
>> +EOF
>> +
>> +# The repair cannot write, so the reopen itself must report the failure
>> +$QEMU_IO -r -c "reopen -w" -c "read -P 0x5a 0 512" \
>> +    "blkdebug:$TEST_DIR/blkdebug.conf:$TEST_IMG" 2>&1 \
>> +    | _filter_testdir | _filter_qemu_io | _filter_generated_node_ids
>> +
>> +# The corrupt bit needs a write of its own, so the image is only left dirty
>> +_qcow2_dump_header | grep incompatible_features
>> +
>>  echo
>>  echo "== Creating an image file with lazy_refcounts=off =="
>>  
>> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
>> index ce8ee577216..cc6ca3ab95b 100644
>> --- a/tests/qemu-iotests/039.out
>> +++ b/tests/qemu-iotests/039.out
>> @@ -79,6 +79,42 @@ wrote 512/512 bytes at offset 0
>>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>  incompatible_features     []
>>  
>> +== Reopening a dirty image read/write should repair it ==
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>> +wrote 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>> +incompatible_features     [0]
>> +ERROR cluster 5 refcount=0 reference=1
>> +Rebuilding refcount structure
>> +Repairing cluster 1 refcount=1 reference=0
>> +Repairing cluster 2 refcount=1 reference=0
>> +wrote 512/512 bytes at offset 1048576
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +read 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +No errors were found on the image.
>> +
>> +== A read/write reopen must not check the image ==
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>> +wrote 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>> +incompatible_features     [0]
>> +
>> +== A failed repair must fail the reopen ==
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>> +wrote 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>> +ERROR cluster 5 refcount=0 reference=1
>> +Rebuilding refcount structure
>> +qemu-io: ERROR writing refblock: Input/output error
>> +qemu-io: Could not repair dirty image 'NODE_NAME': Input/output error
>> +The image is left dirty and this node holds it open until the node is removed
>> +read failed: No medium found
>> +incompatible_features     [0]
>> +
>>  == Creating an image file with lazy_refcounts=off ==
>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>  wrote 512/512 bytes at offset 0
>> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
>> index 5c18e413ec9..b25c76183a1 100755
>> --- a/tests/qemu-iotests/040
>> +++ b/tests/qemu-iotests/040
>> @@ -951,6 +951,129 @@ class TestCommitWithOverriddenBacking(iotests.QMPTestCase):
>>          self.vm.qmp('block-job-complete', device='commit')
>>          self.vm.event_wait('BLOCK_JOB_COMPLETED')
>>  
>> +QCOW2_INCOMPAT_FEATURES_OFFSET = 72
>> +QCOW2_INCOMPAT_DIRTY = 1 << 0
>> +
>> +image_size = 4 * 1024 * 1024
>> +dirty_base = os.path.join(iotests.test_dir, 'dirty-base.img')
>> +mid = os.path.join(iotests.test_dir, 'dirty-mid.img')
>> +top = os.path.join(iotests.test_dir, 'dirty-top.img')
>> +
>> +
>> +class TestCommitDirtyBase(iotests.QMPTestCase):
>> +    def setUp(self) -> None:
>> +        if iotests.imgfmt != 'qcow2':
>> +            self.case_skip('the dirty bit is a qcow2 feature')
>> +        iotests.qemu_img_create('-f', iotests.imgfmt, '-o',
>> +                                'compat=1.1,lazy_refcounts=on', dirty_base,
>> +                                str(image_size))
>> +        # Killing the process leaves the refcounts of the written cluster stale
>> +        iotests.qemu_io_popen('-t', 'writethrough',
>> +                              '-c', 'write -P 0x5a 0 512',
>> +                              '-c', 'sigraise 9', dirty_base).communicate()
>> +        iotests.qemu_img_create('-f', iotests.imgfmt, '-b', dirty_base,
>> +                                '-F', iotests.imgfmt, mid)
>> +        iotests.qemu_img_create('-f', iotests.imgfmt, '-b', mid,
>> +                                '-F', iotests.imgfmt, top)
>> +        # The commit has to allocate for this, which is where the stale
>> +        # refcounts hand out the cluster holding the data written above
>> +        qemu_io('-c', 'write -P 0xb1 1M 512', mid)
>> +
>> +        self.vm = iotests.VM()
>> +        self.vm.launch()
>> +        self.vm.cmd('blockdev-add', driver='file', filename=dirty_base,
>> +                    node_name='base-file')
>> +
>> +        self.assertTrue(self.is_dirty())
>> +
>> +    def tearDown(self) -> None:
>> +        if self.vm.is_running():
>> +            self.vm.shutdown()
>> +        for image in (dirty_base, mid, top):
>> +            os.remove(image)
>> +
>> +    def add_chain(self, base_file: str) -> None:
>> +        self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file=base_file,
>> +                    node_name='base', read_only=True)
>> +        self.vm.cmd('blockdev-add', driver='file', filename=mid,
>> +                    node_name='mid-file')
>> +        self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file='mid-file',
>> +                    node_name='mid', backing='base')
>> +        self.vm.cmd('blockdev-add', driver='file', filename=top,
>> +                    node_name='top-file')
>> +        self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file='top-file',
>> +                    node_name='top', backing='mid')
>> +
>> +    def check_base(self) -> None:
>> +        result = iotests.qemu_img_check(dirty_base)
>> +        self.assertEqual(result['check-errors'], 0)
>> +        self.assertEqual(result.get('corruptions', 0), 0)
>> +        # Without the repair the commit would have aliased this cluster
>> +        qemu_io('-c', 'read -P 0x5a 0 512', '-c', 'read -P 0xb1 1M 512',
>> +                dirty_base)
>> +
>> +    def is_dirty(self) -> bool:
>> +        with open(dirty_base, 'rb') as img:
>> +            img.seek(QCOW2_INCOMPAT_FEATURES_OFFSET)
>> +            features = struct.unpack('>Q', img.read(8))[0]
>> +        return bool(features & QCOW2_INCOMPAT_DIRTY)
>> +
>> +    def test_commit_repairs_base(self) -> None:
>> +        self.add_chain('base-file')
>> +
>> +        self.vm.cmd('block-commit', job_id='job0', device='top',
>> +                    top_node='mid', base_node='base')
>> +        self.wait_until_completed(drive='job0')
>> +
>> +        self.vm.shutdown()
>> +        self.assertFalse(self.is_dirty())
>> +        self.check_base()
>> +
>> +    def test_active_commit_repairs_base(self) -> None:
>> +        self.add_chain('base-file')
>> +
>> +        # Without top-node the whole chain commits, through
>> +        # commit_active_start() rather than commit_start()
>> +        self.vm.cmd('block-commit', job_id='job0', device='top',
>> +                    base_node='base')
>> +        self.complete_and_wait(drive='job0')
>> +
>> +        self.vm.shutdown()
>> +        self.assertFalse(self.is_dirty())
>> +        self.check_base()
>> +
>> +    def test_failed_repair_fails_the_commit(self) -> None:
>> +        self.vm.cmd('blockdev-add', driver='blkdebug', image='base-file',
>> +                    node_name='base-blkdebug',
>> +                    inject_error=[{'event': 'none', 'iotype': 'write',
>> +                                   'errno': 5}])
>> +        self.add_chain('base-blkdebug')
>> +
>> +        result = self.vm.qmp('block-commit', job_id='job0', device='top',
>> +                             top_node='mid', base_node='base')
>> +        self.assert_qmp(result, 'error/class', 'GenericError')
>> +        self.assertIn("Could not repair dirty image 'base'",
>> +                      result['error']['desc'])
>> +
>> +        # The base is left in the graph, and nothing can be queried while
>> +        # it is there
>> +        result = self.vm.qmp('query-named-block-nodes', flat=True)
>> +        self.assert_qmp(result, 'error/desc', 'Block device base is ejected')
>> +
>> +        # It only goes away once nothing refers to it
>> +        result = self.vm.qmp('blockdev-del', node_name='base')
>> +        self.assert_qmp(result, 'error/desc',
>> +                        "Node 'base' is busy: node is used as backing hd of "
>> +                        "'mid'")
>> +
>> +        # Marking the image corrupt needs a write of its own, which fails too
>> +        self.assertTrue(self.is_dirty())
>> +
>> +        # Nothing can use the base any more, and that is what is reported
>> +        result = self.vm.qmp('block-commit', job_id='job1', device='top',
>> +                             top_node='mid', base_node='base')
>> +        self.assert_qmp(result, 'error/desc', 'Device has no medium')
>> +
>>  if __name__ == '__main__':
>>      iotests.main(supported_fmts=['qcow2', 'qed'],
>>                   supported_protocols=['file'])
>> diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
>> index 1bb1dc5f0ea..f3cbf73a01d 100644
>> --- a/tests/qemu-iotests/040.out
>> +++ b/tests/qemu-iotests/040.out
>> @@ -1,5 +1,5 @@
>> -.................................................................
>> +....................................................................
>>  ----------------------------------------------------------------------
>> -Ran 65 tests
>> +Ran 68 tests
>>  
>>  OK