* [Qemu-devel] [PATCH v2 1/2] block/file-posix: Unaligned O_DIRECT block-status
2019-05-15 4:15 [Qemu-devel] [PATCH v2 0/2] block/file-posix: Fix unaligned O_DIRECT block status Max Reitz
@ 2019-05-15 4:15 ` Max Reitz
2019-05-15 4:15 ` [Qemu-devel] [PATCH v2 2/2] iotests: Test unaligned raw images with O_DIRECT Max Reitz
2019-05-20 13:25 ` [Qemu-devel] [PATCH v2 0/2] block/file-posix: Fix unaligned O_DIRECT block status Kevin Wolf
2 siblings, 0 replies; 4+ messages in thread
From: Max Reitz @ 2019-05-15 4:15 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-stable, qemu-devel, Max Reitz
Currently, qemu crashes whenever someone queries the block status of an
unaligned image tail of an O_DIRECT image:
$ 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.
This is because bdrv_co_block_status() checks that the result returned
by the driver's implementation is aligned to the request_alignment, but
file-posix can fail to do so, which is actually mentioned in a comment
there: "[...] possibly including a partial sector at EOF".
Fix this by rounding up those partial sectors.
There are two possible alternative fixes:
(1) We could refuse to open unaligned image files with O_DIRECT
altogether. That sounds reasonable until you realize that qcow2
does necessarily not fill up its metadata clusters, and that nobody
runs qemu-img create with O_DIRECT. Therefore, unpreallocated qcow2
files usually have an unaligned image tail.
(2) bdrv_co_block_status() could ignore unaligned tails. It actually
throws away everything past the EOF already, so that sounds
reasonable.
Unfortunately, the block layer knows file lengths only with a
granularity of BDRV_SECTOR_SIZE, so bdrv_co_block_status() usually
would have to guess whether its file length information is inexact
or whether the driver is broken.
Fixing what raw_co_block_status() returns is the safest thing to do.
There seems to be no other block driver that sets request_alignment and
does not make sure that it always returns aligned values.
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/file-posix.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index e09e15bbf8..d018429672 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2488,6 +2488,8 @@ static int coroutine_fn raw_co_block_status(BlockDriverState *bs,
off_t data = 0, hole = 0;
int ret;
+ assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
+
ret = fd_open(bs);
if (ret < 0) {
return ret;
@@ -2513,6 +2515,20 @@ static int coroutine_fn raw_co_block_status(BlockDriverState *bs,
/* On a data extent, compute bytes to the end of the extent,
* possibly including a partial sector at EOF. */
*pnum = MIN(bytes, hole - offset);
+
+ /*
+ * We are not allowed to return partial sectors, though, so
+ * round up if necessary.
+ */
+ if (!QEMU_IS_ALIGNED(*pnum, bs->bl.request_alignment)) {
+ int64_t file_length = raw_getlength(bs);
+ if (file_length > 0) {
+ /* Ignore errors, this is just a safeguard */
+ assert(hole == file_length);
+ }
+ *pnum = ROUND_UP(*pnum, bs->bl.request_alignment);
+ }
+
ret = BDRV_BLOCK_DATA;
} else {
/* On a hole, compute bytes to the beginning of the next extent. */
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Qemu-devel] [PATCH v2 2/2] iotests: Test unaligned raw images with O_DIRECT
2019-05-15 4:15 [Qemu-devel] [PATCH v2 0/2] block/file-posix: Fix unaligned O_DIRECT block status Max Reitz
2019-05-15 4:15 ` [Qemu-devel] [PATCH v2 1/2] block/file-posix: Unaligned O_DIRECT block-status Max Reitz
@ 2019-05-15 4:15 ` Max Reitz
2019-05-20 13:25 ` [Qemu-devel] [PATCH v2 0/2] block/file-posix: Fix unaligned O_DIRECT block status Kevin Wolf
2 siblings, 0 replies; 4+ messages in thread
From: Max Reitz @ 2019-05-15 4:15 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-stable, qemu-devel, Max Reitz
We already have 221 for accesses through the page cache, but it is
better to create a new file for O_DIRECT instead of integrating those
test cases into 221. This way, we can make use of
_supported_cache_modes (and _default_cache_mode) so the test is
automatically skipped on filesystems that do not support O_DIRECT.
As part of the split, add _supported_cache_modes to 221. With that, it
no longer fails when run with -c none or -c directsync.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/221 | 4 ++
tests/qemu-iotests/253 | 84 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/253.out | 14 +++++++
tests/qemu-iotests/group | 1 +
4 files changed, 103 insertions(+)
create mode 100755 tests/qemu-iotests/253
create mode 100644 tests/qemu-iotests/253.out
diff --git a/tests/qemu-iotests/221 b/tests/qemu-iotests/221
index 25dd47bcfe..0e9096fec7 100755
--- a/tests/qemu-iotests/221
+++ b/tests/qemu-iotests/221
@@ -1,6 +1,7 @@
#!/usr/bin/env bash
#
# Test qemu-img vs. unaligned images
+# (See also 253, which is the O_DIRECT version)
#
# Copyright (C) 2018-2019 Red Hat, Inc.
#
@@ -37,6 +38,9 @@ _supported_fmt raw
_supported_proto file
_supported_os Linux
+_default_cache_mode writeback
+_supported_cache_modes writeback writethrough unsafe
+
echo
echo "=== Check mapping of unaligned raw image ==="
echo
diff --git a/tests/qemu-iotests/253 b/tests/qemu-iotests/253
new file mode 100755
index 0000000000..d88d5afa45
--- /dev/null
+++ b/tests/qemu-iotests/253
@@ -0,0 +1,84 @@
+#!/usr/bin/env bash
+#
+# Test qemu-img vs. unaligned images; O_DIRECT version
+# (Originates from 221)
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt raw
+_supported_proto file
+_supported_os Linux
+
+_default_cache_mode none
+_supported_cache_modes none directsync
+
+echo
+echo "=== Check mapping of unaligned raw image ==="
+echo
+
+# We do not know how large a physical sector is, but it is certainly
+# going to be a factor of 1 MB
+size=$((1 * 1024 * 1024 - 1))
+
+# qemu-img create rounds size up to BDRV_SECTOR_SIZE
+_make_test_img $size
+$QEMU_IMG map --output=json --image-opts \
+ "driver=$IMGFMT,file.driver=file,file.filename=$TEST_IMG,cache.direct=on" \
+ | _filter_qemu_img_map
+
+# so we resize it and check again
+truncate --size=$size "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+ "driver=$IMGFMT,file.driver=file,file.filename=$TEST_IMG,cache.direct=on" \
+ | _filter_qemu_img_map
+
+# qemu-io with O_DIRECT always writes whole physical sectors. Again,
+# we do not know how large a physical sector is, so we just start
+# writing from a 64 kB boundary, which should always be aligned.
+offset=$((1 * 1024 * 1024 - 64 * 1024))
+$QEMU_IO -c "w $offset $((size - offset))" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG map --output=json --image-opts \
+ "driver=$IMGFMT,file.driver=file,file.filename=$TEST_IMG,cache.direct=on" \
+ | _filter_qemu_img_map
+
+# Resize it and check again -- contrary to 221, we may not get partial
+# sectors here, so there should be only two areas (one zero, one
+# data).
+truncate --size=$size "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+ "driver=$IMGFMT,file.driver=file,file.filename=$TEST_IMG,cache.direct=on" \
+ | _filter_qemu_img_map
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/253.out b/tests/qemu-iotests/253.out
new file mode 100644
index 0000000000..607c0baa0b
--- /dev/null
+++ b/tests/qemu-iotests/253.out
@@ -0,0 +1,14 @@
+QA output created by 253
+
+=== Check mapping of unaligned raw image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048575
+[{ "start": 0, "length": 1048576, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
+wrote 65535/65535 bytes at offset 983040
+63.999 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+[{ "start": 0, "length": 983040, "depth": 0, "zero": true, "data": false, "offset": OFFSET},
+{ "start": 983040, "length": 65536, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 983040, "depth": 0, "zero": true, "data": false, "offset": OFFSET},
+{ "start": 983040, "length": 65536, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 00e474ab0a..52b7c16e15 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -250,3 +250,4 @@
248 rw auto quick
249 rw auto quick
252 rw auto backing quick
+253 rw auto quick
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread