[Qemu-devel] [PATCH v2 0/2] block/file-posix: Fix unaligned O_DIRECT block status

Max Reitz posted 2 patches 4 years, 10 months ago
Test asan passed
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190515041541.12367-1-mreitz@redhat.com
Maintainers: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>
block/file-posix.c         | 16 ++++++++
tests/qemu-iotests/221     |  4 ++
tests/qemu-iotests/253     | 84 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/253.out | 14 +++++++
tests/qemu-iotests/group   |  1 +
5 files changed, 119 insertions(+)
create mode 100755 tests/qemu-iotests/253
create mode 100644 tests/qemu-iotests/253.out
[Qemu-devel] [PATCH v2 0/2] block/file-posix: Fix unaligned O_DIRECT block status
Posted by Max Reitz 4 years, 10 months ago
The user-visible problem:
$ echo > foo
$ qemu-img map --image-opts driver=file,filename=foo,cache.direct=on
Offset          Length          Mapped to       File
qemu-img: block/io.c:2093: bdrv_co_block_status: Assertion `*pnum &&
QEMU_IS_ALIGNED(*pnum, align) && align > offset - aligned_offset'
failed.

The internal problem: file-posix truncates status requests to the EOF.
If the EOF is not aligned at the request_alignment,
bdrv_co_block_status() won't like that.

See patch 1 for a deeper discussion (including two possible alternatives
how we could address the problem).
(As I note there, I’ve looked through all block drivers, and I didn’t
find any other which could have the same problem.  gluster uses the same
block-status code, but it doesn’t set a request_alignment.  NBD
force-aligns the server response in nbd_parse_blockstatus_payload().
qcow2... Should be fine as long as no crypto driver has a block limit
exceeding the qcow2 cluster size.  And so on.)

Patch 2 adds a test.  After writing that test, I noticed that we already
had one: 109 fails with -c none before patch 1.  Er, well, at least the
new test is more succinct and has the correct default cache mode, so it
will actually do the test if you run ./check without enforcing any cache
on a filesystem that supports O_DIRECT.


v2:
- Shortened the assert() in patch 1 [Eric]


git backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[0003] [FC] 'block/file-posix: Unaligned O_DIRECT block-status'
002/2:[----] [--] 'iotests: Test unaligned raw images with O_DIRECT'


Max Reitz (2):
  block/file-posix: Unaligned O_DIRECT block-status
  iotests: Test unaligned raw images with O_DIRECT

 block/file-posix.c         | 16 ++++++++
 tests/qemu-iotests/221     |  4 ++
 tests/qemu-iotests/253     | 84 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/253.out | 14 +++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 119 insertions(+)
 create mode 100755 tests/qemu-iotests/253
 create mode 100644 tests/qemu-iotests/253.out

-- 
2.21.0


Re: [Qemu-devel] [PATCH v2 0/2] block/file-posix: Fix unaligned O_DIRECT block status
Posted by Kevin Wolf 4 years, 10 months ago
Am 15.05.2019 um 06:15 hat Max Reitz geschrieben:
> The user-visible problem:
> $ echo > foo
> $ qemu-img map --image-opts driver=file,filename=foo,cache.direct=on
> Offset          Length          Mapped to       File
> qemu-img: block/io.c:2093: bdrv_co_block_status: Assertion `*pnum &&
> QEMU_IS_ALIGNED(*pnum, align) && align > offset - aligned_offset'
> failed.
> 
> The internal problem: file-posix truncates status requests to the EOF.
> If the EOF is not aligned at the request_alignment,
> bdrv_co_block_status() won't like that.
> 
> See patch 1 for a deeper discussion (including two possible alternatives
> how we could address the problem).
> (As I note there, I’ve looked through all block drivers, and I didn’t
> find any other which could have the same problem.  gluster uses the same
> block-status code, but it doesn’t set a request_alignment.  NBD
> force-aligns the server response in nbd_parse_blockstatus_payload().
> qcow2... Should be fine as long as no crypto driver has a block limit
> exceeding the qcow2 cluster size.  And so on.)
> 
> Patch 2 adds a test.  After writing that test, I noticed that we already
> had one: 109 fails with -c none before patch 1.  Er, well, at least the
> new test is more succinct and has the correct default cache mode, so it
> will actually do the test if you run ./check without enforcing any cache
> on a filesystem that supports O_DIRECT.

Thanks, applied to the block branch.

Kevin