[Qemu-devel] [PATCH 0/2] block: fix multiwrite_merge() overlapping requests

[Qemu-devel] [PATCH 0/2] block: fix multiwrite_merge() overlapping requests

[Stefan Hajnoczi]

This is a fix for https://bugs.launchpad.net/qemu/+bug/1343827.

Patch 1 fixes the bug.  Patch 2 adds a qemu-iotests test case to prevent regressions.

Stefan Hajnoczi (2):
  block: fix overlapping multiwrite requests
  qemu-iotests: add multiwrite test cases

 block.c                    |  6 +++
 tests/qemu-iotests/100     | 97 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/100.out | 54 ++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 158 insertions(+)
 create mode 100755 tests/qemu-iotests/100
 create mode 100644 tests/qemu-iotests/100.out

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] block: fix overlapping multiwrite requests

[Stefan Hajnoczi]

When request A is a strict subset of request B:

  AAAAAAAA
    BBBB

multiwrite_merge() merges them as follows:

  AABBBB

The tail of request A should have been included:

  AABBBBAA

This patch fixes data loss but this code path is probably rare.  Since
guests cannot assume ordering between in-flight requests, few
applications submit overlapping write requests.

Reported-by: Slava Pestov <sviatoslav.pestov@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block.c b/block.c
index 8cf519b..0a3ac43 100644
--- a/block.c
+++ b/block.c
@@ -4498,6 +4498,12 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
             // Add the second request
             qemu_iovec_concat(qiov, reqs[i].qiov, 0, reqs[i].qiov->size);
 
+            // Add tail of first request, if necessary
+            if (qiov->size < reqs[outidx].qiov->size) {
+                qemu_iovec_concat(qiov, reqs[outidx].qiov, qiov->size,
+                                  reqs[outidx].qiov->size - qiov->size);
+            }
+
             reqs[outidx].nb_sectors = qiov->size >> 9;
             reqs[outidx].qiov = qiov;
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/2] block: fix overlapping multiwrite requests

[Fam Zheng]

On Tue, 07/29 13:41, Stefan Hajnoczi wrote:
> When request A is a strict subset of request B:
> 
>   AAAAAAAA
>     BBBB

s/subset/superset/ ?

Fam

Re: [Qemu-devel] [PATCH 1/2] block: fix overlapping multiwrite requests

[Stefan Hajnoczi]

On Tue, Jul 29, 2014 at 1:46 PM, Fam Zheng <famz@redhat.com> wrote:
> On Tue, 07/29 13:41, Stefan Hajnoczi wrote:
>> When request A is a strict subset of request B:
>>
>>   AAAAAAAA
>>     BBBB
>
> s/subset/superset/ ?

Yes :)

[Qemu-devel] [PATCH 2/2] qemu-iotests: add multiwrite test cases

[Stefan Hajnoczi]

This test case covers the basic bdrv_aio_multiwrite() scenarios:
1. Single request
2. Sequential requests
3. Overlapping requests
4. Disjoint requests

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/100     | 97 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/100.out | 54 ++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 152 insertions(+)
 create mode 100755 tests/qemu-iotests/100
 create mode 100644 tests/qemu-iotests/100.out

diff --git a/tests/qemu-iotests/100 b/tests/qemu-iotests/100
new file mode 100755
index 0000000..636e0e0
--- /dev/null
+++ b/tests/qemu-iotests/100
@@ -0,0 +1,97 @@
+#!/bin/bash
+#
+# Test simple read/write using plain bdrv_read/bdrv_write
+#
+# Copyright (C) 2014 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/>.
+#
+
+# creator
+owner=stefanha@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+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 generic
+_supported_proto generic
+_supported_os Linux
+
+
+size=128M
+
+echo
+echo "== Single request =="
+_make_test_img $size
+$QEMU_IO -c "multiwrite 0 4k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== verify pattern =="
+$QEMU_IO -c "read -P 0xcd 0 4k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0 4k 4k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== Sequential requests =="
+_make_test_img $size
+$QEMU_IO -c "multiwrite 0 4k ; 4k 4k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== verify pattern =="
+$QEMU_IO -c "read -P 0xcd 0 4k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xce 4k 4k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0 8k 4k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== Overlapping requests =="
+_make_test_img $size
+$QEMU_IO -c "multiwrite 0 4k ; 1k 2k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== verify pattern =="
+# Order of overlapping in-flight requests is not guaranteed so we cannot verify
+# [1k, 3k) since it could have either pattern 0xcd or 0xce.
+$QEMU_IO -c "read -P 0xcd 0 1k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xcd 3k 1k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0 4k 4k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== Disjoint requests =="
+_make_test_img $size
+$QEMU_IO -c "multiwrite 0 4k ; 64k 4k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== verify pattern =="
+$QEMU_IO -c "read -P 0xcd 0 4k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0 4k 60k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xce 64k 4k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0 68k 4k" "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/100.out b/tests/qemu-iotests/100.out
new file mode 100644
index 0000000..2b9f741
--- /dev/null
+++ b/tests/qemu-iotests/100.out
@@ -0,0 +1,54 @@
+QA output created by 100
+
+== Single request ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify pattern ==
+read 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Sequential requests ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+wrote 8192/8192 bytes at offset 0
+8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify pattern ==
+read 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Overlapping requests ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+wrote 6144/6144 bytes at offset 0
+6 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify pattern ==
+read 1024/1024 bytes at offset 0
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024 bytes at offset 3072
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Disjoint requests ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+wrote 8192/8192 bytes at offset 0
+8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify pattern ==
+read 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 61440/61440 bytes at offset 4096
+60 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 65536
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 69632
+4 KiB, 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 6e67f61..8e2215e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -100,3 +100,4 @@
 091 rw auto quick
 092 rw auto quick
 095 rw auto quick
+100 rw auto quick
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/2] qemu-iotests: add multiwrite test cases

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On 07/29/2014 06:41 AM, Stefan Hajnoczi wrote:
> This test case covers the basic bdrv_aio_multiwrite() scenarios:
> 1. Single request
> 2. Sequential requests
> 3. Overlapping requests
> 4. Disjoint requests
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

> +echo
> +echo "== Overlapping requests =="
> +_make_test_img $size
> +$QEMU_IO -c "multiwrite 0 4k ; 1k 2k" "$TEST_IMG" | _filter_qemu_io
> +

This only tests superset overlap:

AAAA
 BB

Wouldn't it be good to also test head overlap, tail overlap, and subset
overlap, as in:

AAA
  BB

 AAA
BB

 AA
BBBB


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qemu-iotests: add multiwrite test cases

[Stefan Hajnoczi]

On Tue, Jul 29, 2014 at 4:11 PM, Eric Blake <eblake@redhat.com> wrote:
> On 07/29/2014 06:41 AM, Stefan Hajnoczi wrote:
>> This test case covers the basic bdrv_aio_multiwrite() scenarios:
>> 1. Single request
>> 2. Sequential requests
>> 3. Overlapping requests
>> 4. Disjoint requests
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>
>> +echo
>> +echo "== Overlapping requests =="
>> +_make_test_img $size
>> +$QEMU_IO -c "multiwrite 0 4k ; 1k 2k" "$TEST_IMG" | _filter_qemu_io
>> +
>
> This only tests superset overlap:
>
> AAAA
>  BB
>
> Wouldn't it be good to also test head overlap, tail overlap, and subset
> overlap, as in:
>
> AAA
>   BB
>
>  AAA
> BB
>
>  AA
> BBBB

Sure, I can add more test cases in v2.

Stefan

Re: [Qemu-devel] [PATCH 2/2] qemu-iotests: add multiwrite test cases

[Slava Pestov]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

Why are you guys merging requests in qemu at all? Just submit them to the
kernel and let the kernel do it.


On Wed, Jul 30, 2014 at 1:11 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Tue, Jul 29, 2014 at 4:11 PM, Eric Blake <eblake@redhat.com> wrote:
> > On 07/29/2014 06:41 AM, Stefan Hajnoczi wrote:
> >> This test case covers the basic bdrv_aio_multiwrite() scenarios:
> >> 1. Single request
> >> 2. Sequential requests
> >> 3. Overlapping requests
> >> 4. Disjoint requests
> >>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >
> >> +echo
> >> +echo "== Overlapping requests =="
> >> +_make_test_img $size
> >> +$QEMU_IO -c "multiwrite 0 4k ; 1k 2k" "$TEST_IMG" | _filter_qemu_io
> >> +
> >
> > This only tests superset overlap:
> >
> > AAAA
> >  BB
> >
> > Wouldn't it be good to also test head overlap, tail overlap, and subset
> > overlap, as in:
> >
> > AAA
> >   BB
> >
> >  AAA
> > BB
> >
> >  AA
> > BBBB
>
> Sure, I can add more test cases in v2.
>
> Stefan
>

[-- Attachment #2: Type: text/html, Size: 1784 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qemu-iotests: add multiwrite test cases

[Kevin Wolf]

Am 31.07.2014 um 20:19 hat Slava Pestov geschrieben:
> Why are you guys merging requests in qemu at all? Just submit them to the
> kernel and let the kernel do it.

Because the kernel generally isn't the next one seeing the requests. You
might be right for the special case of using only raw images with
cache=none,aio=native, where we would theoretically have a chance to
submit all requests in a batch with a single syscall and then let the
kernel merge them. It just isn't what everyone is running.

An important case is requests going to non-raw image format drivers,
where many small writes on sparse images are inefficient because they
cause a lot of unnecessary COW activity. Another case are backends that
don't even send the requests to the kernel, but use e.g. a network
protocol as their backend.

Kevin