Today I have faced real data corrupt from our customer with the
situation very close to the one addressed in the patch
"qcow2: do not try to clear the dirty bit on a read-only node" and
that is interesting. I was really unsure that design is correct
but with today case I can say that correct thing was done.
The problem
-----------
qcow2_do_open() repairs an image carrying QCOW2_INCOMPAT_DIRTY, but only
for a node that is writable from the start:
if (!(flags & BDRV_O_CHECK) && bdrv_is_writable(bs) &&
(s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
A node opened read-only skips it, correctly, since it resolves nothing
and writes nothing. Nothing revisits the question when that same node
later becomes writable, and bdrv_reopen() is not an exotic way to get
there: commit_active_start() and commit_start() both reopen the base
read-write for the duration of the job, so an ordinary block-commit onto
a read-only backing file is enough.
With lazy refcounts the refcount blocks are not written again once the
dirty bit is set, so an image left behind by a killed QEMU has an
on-disk refcount block that accounts for the metadata clusters and
nothing else. Every data cluster reads as free. s->free_cluster_index
starts at 0, so the first allocation after such a reopen starts at the
front of the image and hands out clusters that L2 entries still point
at.
The result is aliasing: two guest offsets mapped onto one host cluster,
so the guest reads back data belonging to some other offset. Nothing
about this fails. No I/O error is reported, the corrupt bit stays clear,
and a clean close clears the dirty bit as well, so no later open will
repair the image either. Afterwards qemu-img check reports
ERROR cluster N refcount=1 reference=2
ERROR cluster N refcount=0 reference=1
ERROR OFLAG_COPIED data cluster: l2_entry=<host>|COPIED refcount=0
and the only runtime witness, if something eventually frees one of those
clusters, is a bare
qcow2_free_clusters failed: Invalid argument
on stderr, with the guest none the wiser.
How it looked in production
---------------------------
A VM was killed while its storage was unavailable, leaving a 100 GiB
volume dirty. It came back with a snapshot-revert overlay on top, so the
volume itself was now the read-only backing file, and the overlay was
committed into it two and a half hours later. 8082 host clusters ended
up referenced by two L2 entries each, roughly 8 GiB of guest data
cross-mapped. The guest filesystem began failing metadata verification
on buffers holding fragments of unrelated files.
Two properties of the damage are worth recording, because they are what
told us this was not a race:
- aliasing is exactly two-way, never three or more, which is a single
monotonic sweep of the allocator rather than a window hit repeatedly;
- it stops dead at the host cluster that was the image end at the
moment the volume was made writable. Everything allocated after that
point is intact.
qemu-img check -r all makes the metadata self-consistent again, and then
honestly reports no errors, but it cannot un-alias anything. The guest
data stays wrong.
Reproducer
----------
Under a second, no guest and no block job required:
qemu-img create -f qcow2 -o compat=1.1,lazy_refcounts=on base.qcow2 1G
qemu-io -f qcow2 -c "write -P 0xaa 0 100M" -c flush \
-c "sigraise 9" base.qcow2
qemu-io -r -f qcow2 base.qcow2 \
<<< $'reopen -w\nwrite -P 0xbb 900M 100M\nquit'
qemu-io -r -f qcow2 -c "read -v 0 16" base.qcow2
# 00000000: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb
The flush is load-bearing: it writes out the L2 cache but not the
refcount blocks, which is exactly the asymmetry the bug needs. Guest
fsyncs supply it in production, so a long-running VM is the ideal
victim. qemu-img commit of an overlay reaches the same state through
commit_active_start().
v1:
https://lore.kernel.org/qemu-devel/[email protected]/
v2:
https://lore.kernel.org/qemu-devel/[email protected]/
Notes for review
----------------
- The repair still runs under bdrv_drain_all(), so a block-commit onto
a large dirty base stalls guest I/O for the length of a full metadata
scan. Rejecting the reopen instead would be cheap, but block-commit
depends on it succeeding, so repairing in place is the only option.
- .bdrv_reopen_commit_post() runs after the transaction is committed
and cannot roll anything back, so a negative return value reports an
unusable node rather than rejecting the reopen. That is unusual, and
it is the only channel available: there is no failable hook once the
node holds BLK_PERM_WRITE.
Changes in v3
-------------
- patch 1: the same bug aborts QEMU, not only fails a reopen, when the
file node below is writable; the message says so and iotests 039
covers the read-write to read-only direction as well. (Andrey)
- patch 2: new, a reopen of a node whose driver is gone segfaults in
bdrv_reopen_queue_child() and asserts in bdrv_reopen_prepare(), which
is the crash a failed repair would reach through commit_clean();
bdrv_reopen_prepare() reports it instead, iotests 060 covers it.
(Andrey)
- patch 3: bdrv_reopen_multiple() documents that a failure means either
nothing changed or the reopen went through and left an unusable tree;
no caller changes. (Andrey)
- patch 3: an implementation which fails must set an error, asserted,
and the loop skips a node whose driver is already gone.
- patch 4: the pre-reopen flags are recorded as int old_flags rather
than a bool, with a bdrv_reopen_was_writable() accessor for drivers.
(Andrey)
- patch 5: the failure names the node and keeps the errno of the check.
(Andrey)
- patch 5: a failed repair drops the driver instead of calling
qcow2_signal_corruption(), which would write the corrupt bit into the
header for what may be a transient ENOSPC.
- patch 5: blockdev-reopen and block-commit document the failure and the
state it leaves. (Andrey)
- patch 5: iotests 040 covers a commit onto a dirty base through both
commit_start() and commit_active_start(), and the failed repair.
(Andrey)
Signed-off-by: Denis V. Lunev <[email protected]>
CC: Kevin Wolf <[email protected]>
CC: Hanna Reitz <[email protected]>
CC: Andrey Drobyshev <[email protected]>
Denis V. Lunev (5):
qcow2: do not clear the dirty bit when reopening a read-only node
block: reject a reopen of an unusable node instead of crashing
block: let bdrv_reopen_commit_post() report a failure
block: remember the flags a reopen starts from
qcow2: repair a dirty image when it becomes writable
block.c | 55 ++++++++++++--
block/qcow2.c | 33 ++++++++-
include/block/block-common.h | 1 +
include/block/block_int-common.h | 11 ++-
qapi/block-core.json | 16 ++++
tests/qemu-iotests/039 | 110 ++++++++++++++++++++++++++++
tests/qemu-iotests/039.out | 56 ++++++++++++++
tests/qemu-iotests/040 | 122 +++++++++++++++++++++++++++++++
tests/qemu-iotests/040.out | 4 +-
tests/qemu-iotests/060 | 30 ++++++++
tests/qemu-iotests/060.out | 13 ++++
11 files changed, 436 insertions(+), 15 deletions(-)
base-commit: fa19879df1658f96ac07365fca8835b7decd6995
--
2.53.0
