Re: [Qemu-devel] [PATCH v3 28/29] qemu-iotests: Test pwritev RMW logic

[Kevin Wolf <kwolf@redhat.com>]

Am 18.01.2014 um 17:01 hat Max Reitz geschrieben:
> On 17.01.2014 15:15, Kevin Wolf wrote:
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  block.c                    |   7 ++
> >  block/blkdebug.c           |   8 ++
> >  include/block/block.h      |   8 ++
> >  tests/qemu-iotests/077     | 278 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/077.out | 202 ++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/group   |   1 +
> >  6 files changed, 504 insertions(+)
> >  create mode 100755 tests/qemu-iotests/077
> >  create mode 100644 tests/qemu-iotests/077.out
> >
> >diff --git a/block.c b/block.c
> >index 812b1b2..12af7fb 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -2961,10 +2961,13 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
> >      if (ret < 0) {
> >          /* Do nothing, write notifier decided to fail this request */
> >      } else if (flags & BDRV_REQ_ZERO_WRITE) {
> >+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_ZERO);
> >          ret = bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors, flags);
> >      } else {
> >+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV);
> >          ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
> >      }
> >+    BLKDBG_EVENT(bs, BLKDBG_PWRITEV_DONE);
> >      if (ret == 0 && !bs->enable_write_cache) {
> >          ret = bdrv_co_flush(bs);
> >@@ -3035,11 +3038,13 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
> >          };
> >          qemu_iovec_init_external(&head_qiov, &head_iov, 1);
> >+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_HEAD);
> >          ret = bdrv_aligned_preadv(bs, &req, offset & ~(align - 1), align,
> >                                    align, &head_qiov, 0);
> >          if (ret < 0) {
> >              goto fail;
> >          }
> >+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
> >          qemu_iovec_init(&local_qiov, qiov->niov + 2);
> >          qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1));
> >@@ -3067,11 +3072,13 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
> >          };
> >          qemu_iovec_init_external(&tail_qiov, &tail_iov, 1);
> >+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_TAIL);
> >          ret = bdrv_aligned_preadv(bs, &req, (offset + bytes) & ~(align - 1), align,
> >                                    align, &tail_qiov, 0);
> >          if (ret < 0) {
> >              goto fail;
> >          }
> >+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
> >          if (!use_local_qiov) {
> >              qemu_iovec_init(&local_qiov, qiov->niov + 1);
> >diff --git a/block/blkdebug.c b/block/blkdebug.c
> >index dc4ba46..6657671 100644
> >--- a/block/blkdebug.c
> >+++ b/block/blkdebug.c
> >@@ -186,6 +186,14 @@ static const char *event_names[BLKDBG_EVENT_MAX] = {
> >      [BLKDBG_FLUSH_TO_OS]                    = "flush_to_os",
> >      [BLKDBG_FLUSH_TO_DISK]                  = "flush_to_disk",
> >+
> >+    [BLKDBG_PWRITEV_RMW_HEAD]               = "pwritev_rmw.head",
> >+    [BLKDBG_PWRITEV_RMW_AFTER_HEAD]         = "pwritev_rmw.after_head",
> >+    [BLKDBG_PWRITEV_RMW_TAIL]               = "pwritev_rmw.tail",
> >+    [BLKDBG_PWRITEV_RMW_AFTER_TAIL]         = "pwritev_rmw.after_tail",
> >+    [BLKDBG_PWRITEV]                        = "pwritev",
> >+    [BLKDBG_PWRITEV_ZERO]                   = "pwritev_zero",
> >+    [BLKDBG_PWRITEV_DONE]                   = "pwritev_done",
> >  };
> >  static int get_event_by_name(const char *name, BlkDebugEvent *event)
> >diff --git a/include/block/block.h b/include/block/block.h
> >index 7e40ccc..127fc2f 100644
> >--- a/include/block/block.h
> >+++ b/include/block/block.h
> >@@ -518,6 +518,14 @@ typedef enum {
> >      BLKDBG_FLUSH_TO_OS,
> >      BLKDBG_FLUSH_TO_DISK,
> >+    BLKDBG_PWRITEV_RMW_HEAD,
> >+    BLKDBG_PWRITEV_RMW_AFTER_HEAD,
> >+    BLKDBG_PWRITEV_RMW_TAIL,
> >+    BLKDBG_PWRITEV_RMW_AFTER_TAIL,
> >+    BLKDBG_PWRITEV,
> >+    BLKDBG_PWRITEV_ZERO,
> >+    BLKDBG_PWRITEV_DONE,
> >+
> >      BLKDBG_EVENT_MAX,
> >  } BlkDebugEvent;
> >diff --git a/tests/qemu-iotests/077 b/tests/qemu-iotests/077
> >new file mode 100755
> >index 0000000..58bfc8f
> >--- /dev/null
> >+++ b/tests/qemu-iotests/077
> >@@ -0,0 +1,278 @@
> >+#!/bin/bash
> >+#
> >+# Test concurrent pread/pwrite
> >+#
> >+# 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=kwolf@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
> >+
> >+CLUSTER_SIZE=4k
> >+size=128M
> >+
> >+_make_test_img $size
> >+
> >+echo
> >+echo "== Some concurrent requests involving RMW =="
> >+
> >+function test_io()
> >+{
> >+echo "open -o file.align=4k blkdebug::$TEST_IMG"
> >+# A simple RMW request
> >+cat  <<EOF
> >+aio_write -P 10 0x200 0x200
> >+aio_flush
> >+EOF
> >+
> >+# Sequential RMW requests on the same physical sector
> >+off=0x1000
> >+for ev in "head" "after_head" "tail" "after_tail"; do
> >+cat  <<EOF
> >+break pwritev_rmw.$ev A
> >+aio_write -P 10 $((off + 0x200)) 0x200
> >+wait_break A
> >+aio_write -P 11 $((off + 0x400)) 0x200
> >+sleep 100
> >+resume A
> >+aio_flush
> >+EOF
> >+off=$((off + 0x1000))
> >+done
> >+
> >+# Chained dependencies
> >+cat  <<EOF
> >+break pwritev_rmw.after_tail A
> >+aio_write -P 10 0x5000 0x200
> >+wait_break A
> >+aio_write -P 11 0x5200 0x200
> >+aio_write -P 12 0x5400 0x200
> >+aio_write -P 13 0x5600 0x200
> >+aio_write -P 14 0x5800 0x200
> >+aio_write -P 15 0x5a00 0x200
> >+aio_write -P 16 0x5c00 0x200
> >+aio_write -P 17 0x5e00 0x200
> >+sleep 100
> >+resume A
> >+aio_flush
> >+EOF
> >+
> >+# Overlapping multiple requests
> >+cat  <<EOF
> >+break pwritev_rmw.after_tail A
> >+aio_write -P 10 0x6000 0x200
> >+wait_break A
> >+break pwritev_rmw.after_head B
> >+aio_write -P 10 0x7e00 0x200
> >+wait_break B
> >+aio_write -P 11 0x6800 0x1000
> >+resume A
> >+sleep 100
> >+resume B
> >+aio_flush
> >+EOF
> >+
> >+cat  <<EOF
> >+break pwritev_rmw.after_tail A
> >+aio_write -P 10 0x8000 0x200
> >+wait_break A
> >+break pwritev_rmw.after_head B
> >+aio_write -P 10 0x9e00 0x200
> >+wait_break B
> >+aio_write -P 11 0x8800 0x1000
> >+resume B
> >+sleep 100
> >+resume A
> >+aio_flush
> >+EOF
> >+
> >+cat  <<EOF
> >+break pwritev_rmw.after_tail A
> >+aio_write -P 10 0xa000 0x200
> >+wait_break A
> >+aio_write -P 11 0xa800 0x1000
> >+break pwritev_rmw.after_head B
> >+aio_write -P 10 0xbe00 0x200
> >+wait_break B
> >+resume A
> >+sleep 100
> >+resume B
> >+aio_flush
> >+EOF
> >+
> >+cat  <<EOF
> >+break pwritev_rmw.after_tail A
> >+aio_write -P 10 0xc000 0x200
> >+wait_break A
> >+aio_write -P 11 0xc800 0x1000
> >+break pwritev_rmw.after_head B
> >+aio_write -P 10 0xde00 0x200
> >+wait_break B
> >+resume B
> >+sleep 100
> >+resume A
> >+aio_flush
> >+EOF
> >+
> >+# Only RMW for the tail part
> >+cat  <<EOF
> >+break pwritev_rmw.after_tail A
> >+aio_write -P 10 0xe000 0x1800
> >+wait_break A
> >+aio_write -P 11 0xf000 0xc00
> >+sleep 100
> >+resume A
> >+aio_flush
> >+EOF
> >+
> >+cat  <<EOF
> >+break pwritev A
> >+aio_write -P 10 0x10000 0x800
> >+wait_break A
> >+break pwritev_rmw.after_tail B
> >+aio_write -P 11 0x10000 0x400
> >+break pwritev_done C
> >+resume A
> >+wait_break C
> >+resume C
> >+sleep 100
> >+wait_break B
> >+resume B
> >+aio_flush
> >+EOF
> >+
> >+cat  <<EOF
> >+break pwritev A
> >+aio_write -P 10 0x11000 0x800
> >+wait_break A
> >+aio_write -P 11 0x11000 0x1000
> >+sleep 100
> >+resume A
> >+aio_flush
> >+EOF
> >+}
> >+
> >+test_io | $QEMU_IO  | _filter_qemu_io | \
> >+    sed -e 's,[0-9/]* bytes at offset [0-9]*,XXX/XXX bytes at offset XXX,g' \
> >+        -e 's/^[0-9]* \(bytes\|KiB\)/XXX bytes/' \
> >+        -e '/Suspended/d'
> >+
> >+echo
> >+echo "== Verify image content =="
> >+
> >+function verify_io()
> >+{
> >+    # A simple RMW request
> >+    echo read -P 0       0 0x200
> >+    echo read -P 10  0x200 0x200
> >+    echo read -P 0   0x400 0xc00
> >+
> >+    # Sequential RMW requests on the same physical sector
> >+    echo read -P 0  0x1000 0x200
> >+    echo read -P 10 0x1200 0x200
> >+    echo read -P 11 0x1400 0x200
> >+    echo read -P 0  0x1600 0xa00
> >+
> >+    echo read -P 0  0x2000 0x200
> >+    echo read -P 10 0x2200 0x200
> >+    echo read -P 11 0x2400 0x200
> >+    echo read -P 0  0x2600 0xa00
> >+
> >+    echo read -P 0  0x3000 0x200
> >+    echo read -P 10 0x3200 0x200
> >+    echo read -P 11 0x3400 0x200
> >+    echo read -P 0  0x3600 0xa00
> >+
> >+    echo read -P 0  0x4000 0x200
> >+    echo read -P 10 0x4200 0x200
> >+    echo read -P 11 0x4400 0x200
> >+    echo read -P 0  0x4600 0xa00
> >+
> >+    # Chained dependencies
> >+    echo read -P 10 0x5000 0x200
> >+    echo read -P 11 0x5200 0x200
> >+    echo read -P 12 0x5400 0x200
> >+    echo read -P 13 0x5600 0x200
> >+    echo read -P 14 0x5800 0x200
> >+    echo read -P 15 0x5a00 0x200
> >+    echo read -P 16 0x5c00 0x200
> >+    echo read -P 17 0x5e00 0x200
> >+
> >+    # Overlapping multiple requests
> >+    echo read -P 10 0x6000 0x200
> >+    echo read -P  0 0x6200 0x600
> >+    echo read -P 11 0x6800 0x1000
> >+    echo read -P  0 0x7800 0x600
> >+    echo read -P 10 0x7e00 0x200
> >+
> >+    echo read -P 10 0x8000 0x200
> >+    echo read -P  0 0x8200 0x600
> >+    echo read -P 11 0x8800 0x1000
> >+    echo read -P  0 0x9800 0x600
> >+    echo read -P 10 0x9e00 0x200
> >+
> >+    echo read -P 10 0xa000 0x200
> >+    echo read -P  0 0xa200 0x600
> >+    echo read -P 11 0xa800 0x1000
> >+    echo read -P  0 0xb800 0x600
> >+    echo read -P 10 0xbe00 0x200
> >+
> >+    echo read -P 10 0xc000 0x200
> >+    echo read -P  0 0xc200 0x600
> >+    echo read -P 11 0xc800 0x1000
> >+    echo read -P  0 0xd800 0x600
> >+    echo read -P 10 0xde00 0x200
> >+
> >+    # Only RMW for the tail part
> >+    echo read -P 10 0xe000 0x200
> >+    echo read -P 11 0xf800 0x400
> 
> I'd propose:
> 
> read -P 10 0xe000 0x1000
> read -P 11 0xf000 0xc00
> 
> instead. That way, this would cover the overlapping section as well;
> but since this is covered by the following reads just as well:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Not sure why I only read 0x200 bytes from 0xe000, I think this should be
0x1000 indeed. However, we can't test the overlapping part because this
is AIO and the order of the requests is undefined. Both 10 and 11 are
correct results.

Kevin