On 9/30/26 18:49, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain
> it was sent from.
> Do not click links or open attachments unless it is an email you expected to
> receive.
> A guest running a container workload came up with 164 files of an image
> layer hollowed out, 45 MiB in all, reading back as zeroes where it had
> written data and been told the writes were on disk. The damage was
> scattered in guest offsets but contiguous in image file offsets: a
> qcow2 over something that zeroed a range of the file.
>
> The storage underneath records every change to the file's length. Over
> a tenth of a second the file grew thirteen times in unaligned steps,
> one per guest write past its end, then once by 82 MiB. A preallocation
> sets the length to the triggering write's end rounded up plus 128 MiB,
> so 82 cannot come from the length the file then had; it can only come
> from a zeroing that started where the burst began, 49 MiB lower, which
> is exactly the range that came back as zeroes. The fallocate ran over
> thirteen acknowledged writes.
>
> So the filter worked from a length the file no longer had. There are
> two ways to get there. Neither is proven, the record cannot decide
> between them, and both are fixed here.
>
> The first is that the filter meant to let those writes in. It raises
> data_end before issuing the preallocation, so that writes below it
> pass straight through into the range the fallocate is about to zero,
> trusting the block layer to order them after it. That trust is
> misplaced, and it needs no race to go wrong: two things meet. The
> file-posix widening that patch 1 removes parks the preallocation
> despite BDRV_REQ_NO_WAIT, and bdrv_find_conflicting_request() skips
> any overlapping request whose waiting_for is set without looking at
> what it points at, so a parked serialising request excludes nothing.
>
> The second is that the length was simply stale. The filter's lengths
> are plain fields, read, decided on and written back with nothing in
> between, and the guest that broke was a virtio-blk with its queues on
> two iothreads. A lost update on the reserved length gives a start 49
> MiB low with no ordering subtlety at all.
>
> For the first, the straight fix is in bdrv_find_conflicting_request(),
> and it regresses iotests/077, whose "Overlapping multiple requests"
> case then verifies wrong data. Patch 1 takes away the other half
> instead: the XFS bug the workaround guards was fixed by 249bd9087a52
> in v5.5, three days before the workaround went in, and predates every
> stable series still maintained. Without the widening NO_WAIT works as
> designed, so it is the one patch here I would not drop.
>
> For the second, the same state turns out to lose data three ways:
>
> - a write into the range the filter records as zero leaves the mark
> behind it, so a later write zeroes over that range is skipped and
> the guest reads the old data back;
>
> - two writers extending the file both pass the compare on data_end and
> the smaller end lands last, so closing the image truncates the file
> below acknowledged data;
>
> - two writers both preallocate over the same range, the older
> fallocate lands last, and the first writer's data is zeroed. Fixing
> this also stops the filter leaning on the ordering above.
>
> Two of the three have tests in iotests/298. The data_end race has
> none: there is no hook between the compare and the store to hang one
> on.
>
> Six patches are tooling, because none of this could be tested: every
> qemu-io request ran in the main loop, blkdebug could neither wake nor
> wait for a request on another AioContext, and nothing could hold a
> request before the block layer tracks it, which is the window the
> filter opens. None changes the output of an existing command.
>
> Signed-off-by: Denis V. Lunev <[email protected]>
> CC: Kevin Wolf <[email protected]>
> CC: Hanna Reitz <[email protected]>
> CC: Vladimir Sementsov-Ogievskiy <[email protected]>
> CC: Andrey Drobyshev <[email protected]>
>
> Andrey Drobyshev (1):
> iotests/298: add testcase for async writes with preallocation filter
>
> Denis V. Lunev (12):
> block/file-posix: drop the XFS workaround for post-EOF fallocate
> block/preallocate: keep zero_start above written data
> block/preallocate: collect the state assignments in one place
> block/preallocate: serialise the filter state
> block/preallocate: hold back writes into a preallocation in flight
> tests/unit: prefix the iothread test helper
> qemu-io: accept --object iothread
> qemu-io: add -t to run an aio request in a named iothread
> qemu-io: add -g to write past the end of the image
> block/blkdebug: make a breakpoint work across AioContexts
> block: add blkdebug events for a write before it is tracked
> iotests/298: cover the preallocation races
>
> block/blkdebug.c | 3 +-
> block/file-posix.c | 34 --------
> block/io.c | 3 +
> block/preallocate.c | 139 ++++++++++++++++++++++++------
> meson.build | 2 +-
> qapi/block-core.json | 10 ++-
> qemu-io-cmds.c | 116 ++++++++++++++++++++++---
> qemu-io.c | 41 ++++++++-
> tests/qemu-iotests/298 | 113 ++++++++++++++++++++++++
> tests/qemu-iotests/298.out | 4 +-
> tests/unit/iothread.c | 28 +++---
> tests/unit/iothread.h | 8 +-
> tests/unit/test-aio-multithread.c | 8 +-
> tests/unit/test-bdrv-drain.c | 32 +++----
> tests/unit/test-block-iothread.c | 28 +++---
> 15 files changed, 435 insertions(+), 134 deletions(-)
>
ping
