[PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

Alberto Garcia posted 1 patch 4 years, 5 months ago
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191014081545.29694-1-berto@igalia.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>, Stefan Hajnoczi <stefanha@redhat.com>, Max Reitz <mreitz@redhat.com>
block/io.c                 |  6 +++++
tests/qemu-iotests/268     | 55 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/268.out |  7 +++++
tests/qemu-iotests/group   |  1 +
4 files changed, 69 insertions(+)
create mode 100755 tests/qemu-iotests/268
create mode 100644 tests/qemu-iotests/268.out
[PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK
Posted by Alberto Garcia 4 years, 5 months ago
The BDRV_REQ_NO_FALLBACK flag means that an operation should only be
performed if it can be offloaded or otherwise performed efficiently.

However a misaligned write request requires a RMW so we should return
an error and let the caller decide how to proceed.

This hits an assertion since commit c8bb23cbdb if the required
alignment is larger than the cluster size:

qemu-img create -f qcow2 -o cluster_size=2k img.qcow2 4G
qemu-io -c "open -o driver=qcow2,file.align=4k blkdebug::img.qcow2" \
        -c 'write 0 512'
qemu-io: block/io.c:1127: bdrv_driver_pwritev: Assertion `!(flags & BDRV_REQ_NO_FALLBACK)' failed.
Aborted

The reason is that when writing to an unallocated cluster we try to
skip the copy-on-write part and zeroize it using BDRV_REQ_NO_FALLBACK
instead, resulting in a write request that is too small (2KB cluster
size vs 4KB required alignment).

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
v2:
- Use QEMU_IS_ALIGNED and (offset | bytes) [Nir]
- Fix typo in comment in iotest
- Expand commit message

v1:
- Initial version

 block/io.c                 |  6 +++++
 tests/qemu-iotests/268     | 55 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/268.out |  7 +++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 69 insertions(+)
 create mode 100755 tests/qemu-iotests/268
 create mode 100644 tests/qemu-iotests/268.out

diff --git a/block/io.c b/block/io.c
index 4f9ee97c2b..18d842382a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2071,6 +2071,12 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
         return ret;
     }
 
+    /* If the request is misaligned then we can't make it efficient */
+    if (!QEMU_IS_ALIGNED(offset | bytes, align) &&
+        (flags & BDRV_REQ_NO_FALLBACK)) {
+        return -ENOTSUP;
+    }
+
     bdrv_inc_in_flight(bs);
     /*
      * Align write if necessary by performing a read-modify-write cycle.
diff --git a/tests/qemu-iotests/268 b/tests/qemu-iotests/268
new file mode 100755
index 0000000000..78c3f4db3a
--- /dev/null
+++ b/tests/qemu-iotests/268
@@ -0,0 +1,55 @@
+#!/usr/bin/env bash
+#
+# Test write request with required alignment larger than the cluster size
+#
+# Copyright (C) 2019 Igalia, S.L.
+# Author: Alberto Garcia <berto@igalia.com>
+#
+# 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/>.
+#
+
+# creator
+owner=berto@igalia.com
+
+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 qcow2
+_supported_proto file
+
+echo
+echo "== Required alignment larger than cluster size =="
+
+CLUSTER_SIZE=2k _make_test_img 1M
+# Since commit c8bb23cbdb writing to an unallocated cluster fills the
+# empty COW areas with bdrv_write_zeroes(flags=BDRV_REQ_NO_FALLBACK)
+$QEMU_IO -c "open -o driver=$IMGFMT,file.align=4k blkdebug::$TEST_IMG" \
+         -c "write 0 512" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/268.out b/tests/qemu-iotests/268.out
new file mode 100644
index 0000000000..2ed6c68529
--- /dev/null
+++ b/tests/qemu-iotests/268.out
@@ -0,0 +1,7 @@
+QA output created by 268
+
+== Required alignment larger than cluster size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5805a79d9e..4c861f7eed 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -278,3 +278,4 @@
 265 rw auto quick
 266 rw quick
 267 rw auto quick snapshot
+268 rw auto quick
-- 
2.20.1


Re: [PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK
Posted by no-reply@patchew.org 4 years, 5 months ago
Patchew URL: https://patchew.org/QEMU/20191014081545.29694-1-berto@igalia.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/qed-table.o
  CC      block/qed-cluster.o
  CC      block/qed-check.o
make: *** [Makefile:994: docs/interop/index.html] Error 2
make: *** Waiting for unfinished jobs....
  CC      block/vhdx.o
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=5e3c9ceb0d0e48f1b7fdaee32bfbd2c2', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-t_sgxpzt/src/docker-src.2019-10-14-04.39.53.16659:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=5e3c9ceb0d0e48f1b7fdaee32bfbd2c2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-t_sgxpzt/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    1m53.697s
user    0m7.411s


The full log is available at
http://patchew.org/logs/20191014081545.29694-1-berto@igalia.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK
Posted by Kevin Wolf 4 years, 5 months ago
Am 14.10.2019 um 10:15 hat Alberto Garcia geschrieben:
> The BDRV_REQ_NO_FALLBACK flag means that an operation should only be
> performed if it can be offloaded or otherwise performed efficiently.
> 
> However a misaligned write request requires a RMW so we should return
> an error and let the caller decide how to proceed.
> 
> This hits an assertion since commit c8bb23cbdb if the required
> alignment is larger than the cluster size:
> 
> qemu-img create -f qcow2 -o cluster_size=2k img.qcow2 4G
> qemu-io -c "open -o driver=qcow2,file.align=4k blkdebug::img.qcow2" \
>         -c 'write 0 512'
> qemu-io: block/io.c:1127: bdrv_driver_pwritev: Assertion `!(flags & BDRV_REQ_NO_FALLBACK)' failed.
> Aborted
> 
> The reason is that when writing to an unallocated cluster we try to
> skip the copy-on-write part and zeroize it using BDRV_REQ_NO_FALLBACK
> instead, resulting in a write request that is too small (2KB cluster
> size vs 4KB required alignment).
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Thanks, applied to the block branch.

Kevin

Re: [PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK
Posted by Alberto Garcia 4 years, 5 months ago
On Mon 14 Oct 2019 11:26:01 AM CEST, Kevin Wolf wrote:
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> Thanks, applied to the block branch.

I'm a bit late now, but a possible trivial optimization is to flip the
conditions, because checking the flag should be faster than checking the
alignment and it's going to be false in almost all cases:

    if ((flags & BDRV_REQ_NO_FALLBACK) &&
        !QEMU_IS_ALIGNED(offset | bytes, align)) {
        return -ENOTSUP;
    }

Feel free to edit the commit if you want.

Berto

Re: [PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK
Posted by Kevin Wolf 4 years, 5 months ago
Am 14.10.2019 um 13:22 hat Alberto Garcia geschrieben:
> On Mon 14 Oct 2019 11:26:01 AM CEST, Kevin Wolf wrote:
> >> Signed-off-by: Alberto Garcia <berto@igalia.com>
> >
> > Thanks, applied to the block branch.
> 
> I'm a bit late now, but a possible trivial optimization is to flip the
> conditions, because checking the flag should be faster than checking the
> alignment and it's going to be false in almost all cases:
> 
>     if ((flags & BDRV_REQ_NO_FALLBACK) &&
>         !QEMU_IS_ALIGNED(offset | bytes, align)) {
>         return -ENOTSUP;
>     }
> 
> Feel free to edit the commit if you want.

Ok, I'll update it.

Kevin