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 <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

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(-)

-- 
2.53.0