[Qemu-devel] [PATCH v3 0/5] add blkdebug tests

[Qemu-devel] [PATCH v3 0/5] add blkdebug tests

[Eric Blake]

Based-on: Kevin Wolf's block-next branch:
http://repo.or.cz/qemu/kevin.git/shortlog/refs/heads/block-next

v2 of the series was here:
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg03495.html

since then, the first half was applied to 2.8; leaving just
this second half to enhance blkdebug for 2.9.  Rebase the patch
on top of Kevin's work to make blkdebug byte-based, and address
lots of good findings from Max.

Available as a tag at:
git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-v3

001/5:[0016] [FC] 'blkdebug: Sanity check block layer guarantees'
002/5:[0032] [FC] 'blkdebug: Add pass-through write_zero and discard support'
003/5:[down] 'blkdebug: Simplify override logic'
004/5:[0093] [FC] 'blkdebug: Add ability to override unmap geometries'
005/5:[0014] [FC] 'tests: Add coverage for recent block geometry fixes'

Eric Blake (5):
  blkdebug: Sanity check block layer guarantees
  blkdebug: Add pass-through write_zero and discard support
  blkdebug: Simplify override logic
  blkdebug: Add ability to override unmap geometries
  tests: Add coverage for recent block geometry fixes

 qapi/block-core.json       |  27 +++++-
 block/blkdebug.c           | 202 +++++++++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/173     | 115 ++++++++++++++++++++++++++
 tests/qemu-iotests/173.out |  49 +++++++++++
 tests/qemu-iotests/group   |   1 +
 5 files changed, 384 insertions(+), 10 deletions(-)
 create mode 100755 tests/qemu-iotests/173
 create mode 100644 tests/qemu-iotests/173.out

-- 
2.9.3

[Qemu-devel] [PATCH v3 1/5] blkdebug: Sanity check block layer guarantees

[Eric Blake]

Commits 04ed95f4 and 1a62d0ac updated the block layer to auto-fragment
any I/O to fit within device boundaries. Additionally, when using a
minimum alignment of 4k, we want to ensure the block layer does proper
read-modify-write rather than requesting I/O on a slice of a sector.
Let's enforce that the contract is obeyed when using blkdebug.  For
now, blkdebug only allows alignment overrides, and just inherits other
limits from whatever device it is wrapping, but a future patch will
further enhance things.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: rebase to byte-based interfaces
v2: new patch
---
 block/blkdebug.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index acccf85..37094a2 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -438,6 +438,13 @@ blkdebug_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     BDRVBlkdebugState *s = bs->opaque;
     BlkdebugRule *rule = NULL;

+    /* Sanity check block layer guarantees */
+    assert(QEMU_IS_ALIGNED(offset, bs->bl.request_alignment));
+    assert(QEMU_IS_ALIGNED(bytes, bs->bl.request_alignment));
+    if (bs->bl.max_transfer) {
+        assert(bytes <= bs->bl.max_transfer);
+    }
+
     QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
         uint64_t inject_offset = rule->options.inject.offset;

@@ -462,6 +469,13 @@ blkdebug_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     BDRVBlkdebugState *s = bs->opaque;
     BlkdebugRule *rule = NULL;

+    /* Sanity check block layer guarantees */
+    assert(QEMU_IS_ALIGNED(offset, bs->bl.request_alignment));
+    assert(QEMU_IS_ALIGNED(bytes, bs->bl.request_alignment));
+    if (bs->bl.max_transfer) {
+        assert(bytes <= bs->bl.max_transfer);
+    }
+
     QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
         uint64_t inject_offset = rule->options.inject.offset;

-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 1/5] blkdebug: Sanity check block layer guarantees

[Max Reitz]

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

On 02.12.2016 20:22, Eric Blake wrote:
> Commits 04ed95f4 and 1a62d0ac updated the block layer to auto-fragment
> any I/O to fit within device boundaries. Additionally, when using a
> minimum alignment of 4k, we want to ensure the block layer does proper
> read-modify-write rather than requesting I/O on a slice of a sector.
> Let's enforce that the contract is obeyed when using blkdebug.  For
> now, blkdebug only allows alignment overrides, and just inherits other
> limits from whatever device it is wrapping, but a future patch will
> further enhance things.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: rebase to byte-based interfaces
> v2: new patch
> ---
>  block/blkdebug.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v3 1/5] blkdebug: Sanity check block layer guarantees

[Kevin Wolf]

Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
> Commits 04ed95f4 and 1a62d0ac updated the block layer to auto-fragment
> any I/O to fit within device boundaries. Additionally, when using a
> minimum alignment of 4k, we want to ensure the block layer does proper
> read-modify-write rather than requesting I/O on a slice of a sector.
> Let's enforce that the contract is obeyed when using blkdebug.  For
> now, blkdebug only allows alignment overrides, and just inherits other
> limits from whatever device it is wrapping, but a future patch will
> further enhance things.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Eric Blake]

In order to test the effects of artificial geometry constraints
on operations like write zero or discard, we first need blkdebug
to manage these actions.  It also allows us to inject errors on
those operations, just like we can for read/write/flush.

We can also test the contract promised by the block layer; namely,
if a device has specified limits on alignment or maximum size,
then those limits must be obeyed (for now, the blkdebug driver
merely inherits limits from whatever it is wrapping, but the next
patch will further enhance it to allow specific limit overrides).

This patch intentionally refuses to service requests smaller than
the requested alignments; this is because an upcoming patch adds
a qemu-iotest to prove that the block layer is correctly handling
fragmentation, but the test only works if there is a way to tell
the difference at artificial alignment boundaries when blkdebug is
using a larger-than-default alignment.  If we let the blkdebug
layer always defer to the underlying layer, which potentially has
a smaller granularity, the iotest will be thwarted.

Tested by setting up an NBD server with export 'foo', then invoking:
$ ./qemu-io
qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo
qemu-io> d 0 15M
qemu-io> w -z 0 15M

Pre-patch, the server never sees the discard (it was silently
eaten by the block layer); post-patch it is passed across the
wire.  Likewise, pre-patch the write is always passed with
NBD_WRITE (with 15M of zeroes on the wire), while post-patch
it can utilize NBD_WRITE_ZEROES (for less traffic).

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: rebase to byte-based read/write, improve docs on why no
partial write zero passthrough
v2: new patch
---
 block/blkdebug.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 37094a2..aac8184 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -1,6 +1,7 @@
 /*
  * Block protocol for I/O error injection
  *
+ * Copyright (C) 2016 Red Hat, Inc.
  * Copyright (c) 2010 Kevin Wolf <kwolf@redhat.com>
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -382,6 +383,11 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }

+    bs->supported_write_flags = BDRV_REQ_FUA &
+        bs->file->bs->supported_write_flags;
+    bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
+        bs->file->bs->supported_zero_flags;
+
     /* Set request alignment */
     align = qemu_opt_get_size(opts, "align", 0);
     if (align < INT_MAX && is_power_of_2(align)) {
@@ -512,6 +518,79 @@ static int blkdebug_co_flush(BlockDriverState *bs)
 }


+static int coroutine_fn blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
+                                                  int64_t offset, int count,
+                                                  BdrvRequestFlags flags)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    BlkdebugRule *rule = NULL;
+    uint32_t align = MAX(bs->bl.request_alignment,
+                         bs->bl.pwrite_zeroes_alignment);
+
+    /* Only pass through requests that are larger than requested
+     * preferred alignment (so that we test the fallback to writes on
+     * unaligned portions), and check that the block layer never hands
+     * us anything crossing an alignment boundary.  */
+    if (count < align) {
+        return -ENOTSUP;
+    }
+    assert(QEMU_IS_ALIGNED(offset, align));
+    assert(QEMU_IS_ALIGNED(count, align));
+    if (bs->bl.max_pwrite_zeroes) {
+        assert(count <= bs->bl.max_pwrite_zeroes);
+    }
+
+    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
+        if (rule->options.inject.offset == -1) {
+            break;
+        }
+    }
+
+    if (rule && rule->options.inject.error) {
+        return inject_error(bs, rule);
+    }
+
+    return bdrv_co_pwrite_zeroes(bs->file, offset, count, flags);
+}
+
+
+static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
+                                             int64_t offset, int count)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    BlkdebugRule *rule = NULL;
+    uint32_t align = bs->bl.pdiscard_alignment;
+
+    /* Only pass through requests that are larger than requested
+     * minimum alignment, and ensure that unaligned requests do not
+     * cross optimum discard boundaries. */
+    if (count < bs->bl.request_alignment) {
+        return -ENOTSUP;
+    }
+    assert(QEMU_IS_ALIGNED(offset, bs->bl.request_alignment));
+    assert(QEMU_IS_ALIGNED(count, bs->bl.request_alignment));
+    if (align && count >= align) {
+        assert(QEMU_IS_ALIGNED(offset, align));
+        assert(QEMU_IS_ALIGNED(count, align));
+    }
+    if (bs->bl.max_pdiscard) {
+        assert(count <= bs->bl.max_pdiscard);
+    }
+
+    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
+        if (rule->options.inject.offset == -1) {
+            break;
+        }
+    }
+
+    if (rule && rule->options.inject.error) {
+        return inject_error(bs, rule);
+    }
+
+    return bdrv_co_pdiscard(bs->file->bs, offset, count);
+}
+
+
 static void blkdebug_close(BlockDriverState *bs)
 {
     BDRVBlkdebugState *s = bs->opaque;
@@ -763,6 +842,8 @@ static BlockDriver bdrv_blkdebug = {
     .bdrv_co_preadv         = blkdebug_co_preadv,
     .bdrv_co_pwritev        = blkdebug_co_pwritev,
     .bdrv_co_flush_to_disk  = blkdebug_co_flush,
+    .bdrv_co_pwrite_zeroes  = blkdebug_co_pwrite_zeroes,
+    .bdrv_co_pdiscard       = blkdebug_co_pdiscard,

     .bdrv_debug_event           = blkdebug_debug_event,
     .bdrv_debug_breakpoint      = blkdebug_debug_breakpoint,
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Max Reitz]

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

On 02.12.2016 20:22, Eric Blake wrote:
> In order to test the effects of artificial geometry constraints
> on operations like write zero or discard, we first need blkdebug
> to manage these actions.  It also allows us to inject errors on
> those operations, just like we can for read/write/flush.
> 
> We can also test the contract promised by the block layer; namely,
> if a device has specified limits on alignment or maximum size,
> then those limits must be obeyed (for now, the blkdebug driver
> merely inherits limits from whatever it is wrapping, but the next
> patch will further enhance it to allow specific limit overrides).
> 
> This patch intentionally refuses to service requests smaller than
> the requested alignments; this is because an upcoming patch adds
> a qemu-iotest to prove that the block layer is correctly handling
> fragmentation, but the test only works if there is a way to tell
> the difference at artificial alignment boundaries when blkdebug is
> using a larger-than-default alignment.  If we let the blkdebug
> layer always defer to the underlying layer, which potentially has
> a smaller granularity, the iotest will be thwarted.
> 
> Tested by setting up an NBD server with export 'foo', then invoking:
> $ ./qemu-io
> qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo
> qemu-io> d 0 15M
> qemu-io> w -z 0 15M
> 
> Pre-patch, the server never sees the discard (it was silently
> eaten by the block layer); post-patch it is passed across the
> wire.  Likewise, pre-patch the write is always passed with
> NBD_WRITE (with 15M of zeroes on the wire), while post-patch
> it can utilize NBD_WRITE_ZEROES (for less traffic).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: rebase to byte-based read/write, improve docs on why no
> partial write zero passthrough
> v2: new patch
> ---
>  block/blkdebug.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Eric Blake]

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

On 12/06/2016 04:00 PM, Max Reitz wrote:

>> Tested by setting up an NBD server with export 'foo', then invoking:
>> $ ./qemu-io
>> qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo

By the way, I'd LOVE to know if there is a way to write a qemu-io
command line that would do this connection automatically (so that I can
batch commands up front and benefit from the shell's history) rather
than having to issue an 'open' after the fact.  I tried various
incantations with --object and --image-opts, but got stumped.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Max Reitz]

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

On 06.12.2016 23:12, Eric Blake wrote:
> On 12/06/2016 04:00 PM, Max Reitz wrote:
> 
>>> Tested by setting up an NBD server with export 'foo', then invoking:
>>> $ ./qemu-io
>>> qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo
> 
> By the way, I'd LOVE to know if there is a way to write a qemu-io
> command line that would do this connection automatically (so that I can
> batch commands up front and benefit from the shell's history) rather
> than having to issue an 'open' after the fact.  I tried various
> incantations with --object and --image-opts, but got stumped.

Can't you just do qemu-io -c 'open'?

Max


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

Re: [Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Eric Blake]

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

On 12/06/2016 04:14 PM, Max Reitz wrote:
> On 06.12.2016 23:12, Eric Blake wrote:
>> On 12/06/2016 04:00 PM, Max Reitz wrote:
>>
>>>> Tested by setting up an NBD server with export 'foo', then invoking:
>>>> $ ./qemu-io
>>>> qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo
>>
>> By the way, I'd LOVE to know if there is a way to write a qemu-io
>> command line that would do this connection automatically (so that I can
>> batch commands up front and benefit from the shell's history) rather
>> than having to issue an 'open' after the fact.  I tried various
>> incantations with --object and --image-opts, but got stumped.
> 
> Can't you just do qemu-io -c 'open'?

I suppose that would get command-line history.  But I still want
interactive mode.  The moment you use -c, ALL commands get run
back-to-back without stopping, so I'd have to add additional -c
'read'/'write' commands up front. I like interactive mode (open
pre-connected, now let me explore the image at will).

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Max Reitz]

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

On 06.12.2016 23:18, Eric Blake wrote:
> On 12/06/2016 04:14 PM, Max Reitz wrote:
>> On 06.12.2016 23:12, Eric Blake wrote:
>>> On 12/06/2016 04:00 PM, Max Reitz wrote:
>>>
>>>>> Tested by setting up an NBD server with export 'foo', then invoking:
>>>>> $ ./qemu-io
>>>>> qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo
>>>
>>> By the way, I'd LOVE to know if there is a way to write a qemu-io
>>> command line that would do this connection automatically (so that I can
>>> batch commands up front and benefit from the shell's history) rather
>>> than having to issue an 'open' after the fact.  I tried various
>>> incantations with --object and --image-opts, but got stumped.
>>
>> Can't you just do qemu-io -c 'open'?
> 
> I suppose that would get command-line history.  But I still want
> interactive mode.  The moment you use -c, ALL commands get run
> back-to-back without stopping, so I'd have to add additional -c
> 'read'/'write' commands up front. I like interactive mode (open
> pre-connected, now let me explore the image at will).

Well, the usual --image-opts version would be:

./qemu-io --image-opts driver=blkdebug,image.driver=nbd,\
image.host=localhost,image.export=foo

Max


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

Re: [Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Eric Blake]

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

On 12/06/2016 04:23 PM, Max Reitz wrote:
> On 06.12.2016 23:18, Eric Blake wrote:
>> On 12/06/2016 04:14 PM, Max Reitz wrote:
>>> On 06.12.2016 23:12, Eric Blake wrote:
>>>> On 12/06/2016 04:00 PM, Max Reitz wrote:
>>>>
>>>>>> Tested by setting up an NBD server with export 'foo', then invoking:
>>>>>> $ ./qemu-io
>>>>>> qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo
>>>>
>>>> By the way, I'd LOVE to know if there is a way to write a qemu-io
>>>> command line that would do this connection automatically (so that I can
>>>> batch commands up front and benefit from the shell's history) rather
>>>> than having to issue an 'open' after the fact.  I tried various
>>>> incantations with --object and --image-opts, but got stumped.
>>>
>>> Can't you just do qemu-io -c 'open'?
>>
>> I suppose that would get command-line history.  But I still want
>> interactive mode.  The moment you use -c, ALL commands get run
>> back-to-back without stopping, so I'd have to add additional -c
>> 'read'/'write' commands up front. I like interactive mode (open
>> pre-connected, now let me explore the image at will).
> 
> Well, the usual --image-opts version would be:
> 
> ./qemu-io --image-opts driver=blkdebug,image.driver=nbd,\
> image.host=localhost,image.export=foo

Thanks, that appears to do the trick! I think I was getting confused by
trying 'file.driver' instead of 'image.driver', or maybe it was because
I was trying 'image.align' to set the blkdebug alignment where just
plain 'align' works once you are in --image-opts mode, or some such
problem on my end.  [Maybe I shouldn't be testing patches late at night,
either...]

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Kevin Wolf]

Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
> In order to test the effects of artificial geometry constraints
> on operations like write zero or discard, we first need blkdebug
> to manage these actions.  It also allows us to inject errors on
> those operations, just like we can for read/write/flush.
> 
> We can also test the contract promised by the block layer; namely,
> if a device has specified limits on alignment or maximum size,
> then those limits must be obeyed (for now, the blkdebug driver
> merely inherits limits from whatever it is wrapping, but the next
> patch will further enhance it to allow specific limit overrides).
> 
> This patch intentionally refuses to service requests smaller than
> the requested alignments; this is because an upcoming patch adds
> a qemu-iotest to prove that the block layer is correctly handling
> fragmentation, but the test only works if there is a way to tell
> the difference at artificial alignment boundaries when blkdebug is
> using a larger-than-default alignment.  If we let the blkdebug
> layer always defer to the underlying layer, which potentially has
> a smaller granularity, the iotest will be thwarted.
> 
> Tested by setting up an NBD server with export 'foo', then invoking:
> $ ./qemu-io
> qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo
> qemu-io> d 0 15M
> qemu-io> w -z 0 15M
> 
> Pre-patch, the server never sees the discard (it was silently
> eaten by the block layer); post-patch it is passed across the
> wire.  Likewise, pre-patch the write is always passed with
> NBD_WRITE (with 15M of zeroes on the wire), while post-patch
> it can utilize NBD_WRITE_ZEROES (for less traffic).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: rebase to byte-based read/write, improve docs on why no
> partial write zero passthrough
> v2: new patch
> ---
>  block/blkdebug.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 37094a2..aac8184 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -1,6 +1,7 @@
>  /*
>   * Block protocol for I/O error injection
>   *
> + * Copyright (C) 2016 Red Hat, Inc.
>   * Copyright (c) 2010 Kevin Wolf <kwolf@redhat.com>
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
> @@ -382,6 +383,11 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>          goto out;
>      }
> 
> +    bs->supported_write_flags = BDRV_REQ_FUA &
> +        bs->file->bs->supported_write_flags;
> +    bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
> +        bs->file->bs->supported_zero_flags;
> +
>      /* Set request alignment */
>      align = qemu_opt_get_size(opts, "align", 0);
>      if (align < INT_MAX && is_power_of_2(align)) {
> @@ -512,6 +518,79 @@ static int blkdebug_co_flush(BlockDriverState *bs)
>  }
> 
> 
> +static int coroutine_fn blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
> +                                                  int64_t offset, int count,
> +                                                  BdrvRequestFlags flags)
> +{
> +    BDRVBlkdebugState *s = bs->opaque;
> +    BlkdebugRule *rule = NULL;
> +    uint32_t align = MAX(bs->bl.request_alignment,
> +                         bs->bl.pwrite_zeroes_alignment);
> +
> +    /* Only pass through requests that are larger than requested
> +     * preferred alignment (so that we test the fallback to writes on
> +     * unaligned portions), and check that the block layer never hands
> +     * us anything crossing an alignment boundary.  */
> +    if (count < align) {
> +        return -ENOTSUP;
> +    }
> +    assert(QEMU_IS_ALIGNED(offset, align));
> +    assert(QEMU_IS_ALIGNED(count, align));
> +    if (bs->bl.max_pwrite_zeroes) {
> +        assert(count <= bs->bl.max_pwrite_zeroes);
> +    }
> +
> +    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
> +        if (rule->options.inject.offset == -1) {

We do have offset and bytes parameters in this function, so I guess we
should check overlaps like in the read/write functions instead of only
executing the rule if it doesn't specify an offset.

> +            break;
> +        }
> +    }
> +
> +    if (rule && rule->options.inject.error) {
> +        return inject_error(bs, rule);
> +    }
> +
> +    return bdrv_co_pwrite_zeroes(bs->file, offset, count, flags);
> +}
> +
> +

Why two newlines?

> +static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
> +                                             int64_t offset, int count)
> +{
> +    BDRVBlkdebugState *s = bs->opaque;
> +    BlkdebugRule *rule = NULL;
> +    uint32_t align = bs->bl.pdiscard_alignment;
> +
> +    /* Only pass through requests that are larger than requested
> +     * minimum alignment, and ensure that unaligned requests do not
> +     * cross optimum discard boundaries. */
> +    if (count < bs->bl.request_alignment) {
> +        return -ENOTSUP;
> +    }
> +    assert(QEMU_IS_ALIGNED(offset, bs->bl.request_alignment));
> +    assert(QEMU_IS_ALIGNED(count, bs->bl.request_alignment));
> +    if (align && count >= align) {
> +        assert(QEMU_IS_ALIGNED(offset, align));
> +        assert(QEMU_IS_ALIGNED(count, align));
> +    }
> +    if (bs->bl.max_pdiscard) {
> +        assert(count <= bs->bl.max_pdiscard);
> +    }
> +
> +    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
> +        if (rule->options.inject.offset == -1) {

Same thing as above.

> +            break;
> +        }
> +    }
> +
> +    if (rule && rule->options.inject.error) {
> +        return inject_error(bs, rule);
> +    }
> +
> +    return bdrv_co_pdiscard(bs->file->bs, offset, count);
> +}

Kevin

Re: [Qemu-devel] [PATCH v3 2/5] blkdebug: Add pass-through write_zero and discard support

[Eric Blake]

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

On 12/07/2016 07:55 AM, Kevin Wolf wrote:
> Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
>> In order to test the effects of artificial geometry constraints
>> on operations like write zero or discard, we first need blkdebug
>> to manage these actions.  It also allows us to inject errors on
>> those operations, just like we can for read/write/flush.
>>

>>
>>
>> +static int coroutine_fn blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
>> +                                                  int64_t offset, int count,
>> +                                                  BdrvRequestFlags flags)
>> +{
>> +    BDRVBlkdebugState *s = bs->opaque;
>> +    BlkdebugRule *rule = NULL;
>> +    uint32_t align = MAX(bs->bl.request_alignment,
>> +                         bs->bl.pwrite_zeroes_alignment);
>> +
>> +    /* Only pass through requests that are larger than requested
>> +     * preferred alignment (so that we test the fallback to writes on
>> +     * unaligned portions), and check that the block layer never hands
>> +     * us anything crossing an alignment boundary.  */
>> +    if (count < align) {
>> +        return -ENOTSUP;
>> +    }

This early exit bypasses the checks for error injection - but the block
layer will then fall back to write which will perform the check there.

>> +    assert(QEMU_IS_ALIGNED(offset, align));
>> +    assert(QEMU_IS_ALIGNED(count, align));
>> +    if (bs->bl.max_pwrite_zeroes) {
>> +        assert(count <= bs->bl.max_pwrite_zeroes);
>> +    }
>> +
>> +    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
>> +        if (rule->options.inject.offset == -1) {
> 
> We do have offset and bytes parameters in this function, so I guess we
> should check overlaps like in the read/write functions instead of only
> executing the rule if it doesn't specify an offset.

Oh, right. I copied and pasted from flush, when I should have copied
from read.

>> +    return bdrv_co_pwrite_zeroes(bs->file, offset, count, flags);
>> +}
>> +
>> +
> 
> Why two newlines?
> 

Habit; they aren't essential, so I'll trim to 1 for consistency with the
rest of the file.

>> +static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
>> +                                             int64_t offset, int count)
>> +{
>> +    BDRVBlkdebugState *s = bs->opaque;
>> +    BlkdebugRule *rule = NULL;
>> +    uint32_t align = bs->bl.pdiscard_alignment;
>> +
>> +    /* Only pass through requests that are larger than requested
>> +     * minimum alignment, and ensure that unaligned requests do not
>> +     * cross optimum discard boundaries. */
>> +    if (count < bs->bl.request_alignment) {
>> +        return -ENOTSUP;
>> +    }

Here, the early exit is a no-op; we never see the error injection
because there is no fallback path at the block layer.  But I guess
that's still okay - when we are discarding the unaligned portion of a
request, there really is no I/O and therefore no way to inject an error
representing failed I/O.

Looks like I get to spin a v4 for the error injection to do specific
range checking.

-- 
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: 604 bytes --]

[Qemu-devel] [PATCH v3 3/5] blkdebug: Simplify override logic

[Eric Blake]

Rather than store into a local variable, the copy to the struct
if the value is valid, then reporting errors otherwise, it is
simpler to just store into the struct and report errors if the
value is invalid.  This however requires that the struct store
a 64-bit number, rather than a narrower type.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: new patch
---
 block/blkdebug.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index aac8184..edc2b05 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -38,7 +38,7 @@
 typedef struct BDRVBlkdebugState {
     int state;
     int new_state;
-    int align;
+    uint64_t align;

     /* For blkdebug_refresh_filename() */
     char *config_file;
@@ -353,7 +353,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVBlkdebugState *s = bs->opaque;
     QemuOpts *opts;
     Error *local_err = NULL;
-    uint64_t align;
     int ret;

     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -389,11 +388,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
         bs->file->bs->supported_zero_flags;

     /* Set request alignment */
-    align = qemu_opt_get_size(opts, "align", 0);
-    if (align < INT_MAX && is_power_of_2(align)) {
-        s->align = align;
-    } else if (align) {
-        error_setg(errp, "Invalid alignment");
+    s->align = qemu_opt_get_size(opts, "align", 0);
+    if (s->align && (s->align >= INT_MAX || !is_power_of_2(s->align))) {
+        error_setg(errp, "Invalid alignment, align must be integer power of 2");
         ret = -EINVAL;
         goto fail_unref;
     }
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 3/5] blkdebug: Simplify override logic

[Max Reitz]

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

On 02.12.2016 20:22, Eric Blake wrote:
> Rather than store into a local variable, the copy to the struct
> if the value is valid, then reporting errors otherwise,

It is rather difficult to part this sentence starting from "the copy".

>                                                         it is
> simpler to just store into the struct and report errors if the
> value is invalid.  This however requires that the struct store
> a 64-bit number, rather than a narrower type.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: new patch
> ---
>  block/blkdebug.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Anyway, thanks! If you can explain to me how to parse the commit message
or make it easier to read:

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v3 3/5] blkdebug: Simplify override logic

[Eric Blake]

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

On 12/06/2016 04:10 PM, Max Reitz wrote:
> On 02.12.2016 20:22, Eric Blake wrote:
>> Rather than store into a local variable, the copy to the struct
>> if the value is valid, then reporting errors otherwise,
> 
> It is rather difficult to part this sentence starting from "the copy".

That's because I meant to type "then copy".  (What a difference an 'n'
makes - I hope it does't imply that the batteries i my wireless keyboard
are o the declie agai, or a sig of ay other more serious problem)

> 
>>                                                         it is
>> simpler to just store into the struct and report errors if the
>> value is invalid.  This however requires that the struct store
>> a 64-bit number, rather than a narrower type.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v3: new patch
>> ---
>>  block/blkdebug.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> Anyway, thanks! If you can explain to me how to parse the commit message
> or make it easier to read:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 3/5] blkdebug: Simplify override logic

[Kevin Wolf]

Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
> Rather than store into a local variable, the copy to the struct
> if the value is valid, then reporting errors otherwise, it is
> simpler to just store into the struct and report errors if the
> value is invalid.  This however requires that the struct store
> a 64-bit number, rather than a narrower type.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v3 4/5] blkdebug: Add ability to override unmap geometries

[Eric Blake]

Make it easier to simulate various unusual hardware setups (for
example, recent commits 3482b9b and b8d0a98 affect the Dell
Equallogic iSCSI with its 15M preferred and maximum unmap and
write zero sizing, or b2f95fe deals with the Linux loopback
block device having a max_transfer of 64k), by allowing blkdebug
to wrap any other device with further restrictions on various
alignments.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: improve legibility of bounds checking, improve docs
v2: new patch
---
 qapi/block-core.json | 27 ++++++++++++++-
 block/blkdebug.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 120 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c29bef7..26f3e9f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2068,6 +2068,29 @@
 # @align:           #optional required alignment for requests in bytes,
 #                   must be power of 2, or 0 for default
 #
+# @max-transfer:    #optional maximum size for I/O transfers in bytes,
+#                   must be multiple of the larger of @align and and 512
+#                   (but need not be a power of 2), or 0 for default
+#                   (since 2.9)
+#
+# @opt-write-zero:  #optional preferred alignment for write zero requests
+#                   in bytes, must be multiple of the larger of @align
+#                   and 512 (but need not be a power of 2), or 0 for
+#                   default (since 2.9)
+#
+# @max-write-zero:  #optional maximum size for write zero requests in bytes,
+#                   must be multiple of the larger of @align and 512 (but
+#                   need not be a power of 2), or 0 for default (since 2.9)
+#
+# @opt-discard:     #optional preferred alignment for discard requests
+#                   in bytes, must be multiple of the larger of @align
+#                   and 512 (but need not be a power of 2), or 0 for
+#                   default (since 2.9)
+#
+# @max-discard:     #optional maximum size for discard requests in bytes,
+#                   must be multiple of the larger of @align and 512 (but
+#                   need not be a power of 2), or 0 for default (since 2.9)
+#
 # @inject-error:    #optional array of error injection descriptions
 #
 # @set-state:       #optional array of state-change descriptions
@@ -2077,7 +2100,9 @@
 { 'struct': 'BlockdevOptionsBlkdebug',
   'data': { 'image': 'BlockdevRef',
             '*config': 'str',
-            '*align': 'int',
+            '*align': 'int', '*max-transfer': 'int32',
+            '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
+            '*opt-discard': 'int32', '*max-discard': 'int32',
             '*inject-error': ['BlkdebugInjectErrorOptions'],
             '*set-state': ['BlkdebugSetStateOptions'] } }

diff --git a/block/blkdebug.c b/block/blkdebug.c
index edc2b05..45f565d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -39,6 +39,11 @@ typedef struct BDRVBlkdebugState {
     int state;
     int new_state;
     uint64_t align;
+    uint64_t max_transfer;
+    uint64_t opt_write_zero;
+    uint64_t max_write_zero;
+    uint64_t opt_discard;
+    uint64_t max_discard;

     /* For blkdebug_refresh_filename() */
     char *config_file;
@@ -343,6 +348,31 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_SIZE,
             .help = "Required alignment in bytes",
         },
+        {
+            .name = "max-transfer",
+            .type = QEMU_OPT_SIZE,
+            .help = "Maximum transfer size in bytes",
+        },
+        {
+            .name = "opt-write-zero",
+            .type = QEMU_OPT_SIZE,
+            .help = "Optimum write zero alignment in bytes",
+        },
+        {
+            .name = "max-write-zero",
+            .type = QEMU_OPT_SIZE,
+            .help = "Maximum write zero size in bytes",
+        },
+        {
+            .name = "opt-discard",
+            .type = QEMU_OPT_SIZE,
+            .help = "Optimum discard alignment in bytes",
+        },
+        {
+            .name = "max-discard",
+            .type = QEMU_OPT_SIZE,
+            .help = "Maximum discard size in bytes",
+        },
         { /* end of list */ }
     },
 };
@@ -387,11 +417,57 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
         bs->file->bs->supported_zero_flags;

-    /* Set request alignment */
+    /* Set alignment overrides */
     s->align = qemu_opt_get_size(opts, "align", 0);
     if (s->align && (s->align >= INT_MAX || !is_power_of_2(s->align))) {
         error_setg(errp, "Invalid alignment, align must be integer power of 2");
-        ret = -EINVAL;
+        goto fail_unref;
+    }
+
+    s->max_transfer = qemu_opt_get_size(opts, "max-transfer", 0);
+    if (s->max_transfer &&
+        (s->max_transfer >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->max_transfer,
+                          MAX(s->align, BDRV_SECTOR_SIZE)))) {
+        error_setg(errp, "Invalid max-transfer, must be multiple of align");
+        goto fail_unref;
+    }
+
+    s->opt_write_zero = qemu_opt_get_size(opts, "opt-write-zero", 0);
+    if (s->opt_write_zero &&
+        (s->opt_write_zero >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->opt_write_zero,
+                          MAX(s->align, BDRV_SECTOR_SIZE)))) {
+        error_setg(errp, "Invalid opt-write-zero, not consistent with align");
+        goto fail_unref;
+    }
+
+    s->max_write_zero = qemu_opt_get_size(opts, "max-write-zero", 0);
+    if (s->max_write_zero &&
+        (s->max_write_zero >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->max_write_zero,
+                          MAX(s->opt_write_zero,
+                              MAX(s->align, BDRV_SECTOR_SIZE))))) {
+        error_setg(errp, "Invalid max-write-zero, not consistent with align");
+        goto fail_unref;
+    }
+
+    s->opt_discard = qemu_opt_get_size(opts, "opt-discard", 0);
+    if (s->opt_discard &&
+        (s->opt_discard >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->opt_discard,
+                          MAX(s->align, BDRV_SECTOR_SIZE)))) {
+        error_setg(errp, "Invalid opt-discard, not consistent with align");
+        goto fail_unref;
+    }
+
+    s->max_discard = qemu_opt_get_size(opts, "max-discard", 0);
+    if (s->max_discard &&
+        (s->max_discard >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->max_discard,
+                          MAX(s->opt_discard,
+                              MAX(s->align, BDRV_SECTOR_SIZE))))) {
+        error_setg(errp, "Invalid max-discard, not consistent with align");
         goto fail_unref;
     }

@@ -399,6 +475,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     goto out;

 fail_unref:
+    ret = -EINVAL;
     bdrv_unref_child(bs, bs->file);
 out:
     if (ret < 0) {
@@ -814,6 +891,21 @@ static void blkdebug_refresh_limits(BlockDriverState *bs, Error **errp)
     if (s->align) {
         bs->bl.request_alignment = s->align;
     }
+    if (s->max_transfer) {
+        bs->bl.max_transfer = s->max_transfer;
+    }
+    if (s->opt_write_zero) {
+        bs->bl.pwrite_zeroes_alignment = s->opt_write_zero;
+    }
+    if (s->max_write_zero) {
+        bs->bl.max_pwrite_zeroes = s->max_write_zero;
+    }
+    if (s->opt_discard) {
+        bs->bl.pdiscard_alignment = s->opt_discard;
+    }
+    if (s->max_discard) {
+        bs->bl.max_pdiscard = s->max_discard;
+    }
 }

 static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 4/5] blkdebug: Add ability to override unmap geometries

[Max Reitz]

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

On 02.12.2016 20:22, Eric Blake wrote:
> Make it easier to simulate various unusual hardware setups (for
> example, recent commits 3482b9b and b8d0a98 affect the Dell
> Equallogic iSCSI with its 15M preferred and maximum unmap and
> write zero sizing, or b2f95fe deals with the Linux loopback
> block device having a max_transfer of 64k), by allowing blkdebug
> to wrap any other device with further restrictions on various
> alignments.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: improve legibility of bounds checking, improve docs
> v2: new patch
> ---
>  qapi/block-core.json | 27 ++++++++++++++-
>  block/blkdebug.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 120 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v3 4/5] blkdebug: Add ability to override unmap geometries

[Kevin Wolf]

Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
> Make it easier to simulate various unusual hardware setups (for
> example, recent commits 3482b9b and b8d0a98 affect the Dell
> Equallogic iSCSI with its 15M preferred and maximum unmap and
> write zero sizing, or b2f95fe deals with the Linux loopback
> block device having a max_transfer of 64k), by allowing blkdebug
> to wrap any other device with further restrictions on various
> alignments.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: improve legibility of bounds checking, improve docs
> v2: new patch
> ---
>  qapi/block-core.json | 27 ++++++++++++++-
>  block/blkdebug.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 120 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c29bef7..26f3e9f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2068,6 +2068,29 @@
>  # @align:           #optional required alignment for requests in bytes,
>  #                   must be power of 2, or 0 for default
>  #
> +# @max-transfer:    #optional maximum size for I/O transfers in bytes,
> +#                   must be multiple of the larger of @align and and 512

s/and and/and/

> +#                   (but need not be a power of 2), or 0 for default
> +#                   (since 2.9)

What is the reason for the 512 bytes restriction? Not really a problem,
though, allowing more values later is easier than restricting them.

The rest looks good.

Kevin

Re: [Qemu-devel] [PATCH v3 4/5] blkdebug: Add ability to override unmap geometries

[Eric Blake]

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

On 12/07/2016 08:35 AM, Kevin Wolf wrote:

>> +++ b/qapi/block-core.json
>> @@ -2068,6 +2068,29 @@
>>  # @align:           #optional required alignment for requests in bytes,
>>  #                   must be power of 2, or 0 for default
>>  #
>> +# @max-transfer:    #optional maximum size for I/O transfers in bytes,
>> +#                   must be multiple of the larger of @align and and 512
> 
> s/and and/and/
> 
>> +#                   (but need not be a power of 2), or 0 for default
>> +#                   (since 2.9)
> 
> What is the reason for the 512 bytes restriction?

Probably because I wrote the patch prior to your work to make blkdebug
byte-based, and then forgot to touch it up on rebase.

> Not really a problem,
> though, allowing more values later is easier than restricting them.

I'll see if I can drop the restriction, or at least improve the text,
for v4.

> 
> The rest looks good.
> 
> Kevin
> 

-- 
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: 604 bytes --]

[Qemu-devel] [PATCH v3 5/5] tests: Add coverage for recent block geometry fixes

[Eric Blake]

Use blkdebug's new geometry constraints to emulate setups that
have caused recent regression fixes: write zeroes asserting
when running through a loopback block device with max-transfer
smaller than cluster size, and discard rounding away portions
of requests not aligned to preferred boundaries.  Also, add
coverage that the block layer is honoring max transfer limits.

For now, a single iotest performs all actions, with the idea
that we can add future blkdebug constraint test cases in the
same file; but it can be split into multiple iotests if we find
reason to run one portion of the test in more setups than what
are possible in the other.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: make comments tied more to test at hand, rather than the
particular hardware that led to the earlier patches being tested
v2: new patch
---
 tests/qemu-iotests/173     | 115 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/173.out |  49 +++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 165 insertions(+)
 create mode 100755 tests/qemu-iotests/173
 create mode 100644 tests/qemu-iotests/173.out

diff --git a/tests/qemu-iotests/173 b/tests/qemu-iotests/173
new file mode 100755
index 0000000..fd421fc
--- /dev/null
+++ b/tests/qemu-iotests/173
@@ -0,0 +1,115 @@
+#!/bin/bash
+#
+# Test corner cases with unusual block geometries
+#
+# Copyright (C) 2016 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=eblake@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+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
+
+CLUSTER_SIZE=1M
+size=128M
+options=driver=blkdebug,image.driver=qcow2
+
+echo
+echo "== setting up files =="
+
+_make_test_img $size
+$QEMU_IO -c "write -P 11 0 $size" "$TEST_IMG" | _filter_qemu_io
+mv "$TEST_IMG" "$TEST_IMG.base"
+_make_test_img -b "$TEST_IMG.base"
+$QEMU_IO -c "write -P 22 0 $size" "$TEST_IMG" | _filter_qemu_io
+
+# Limited to 64k max-transfer
+echo
+echo "== constrained alignment and max-transfer =="
+limits=align=4k,max-transfer=64k
+$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
+         -c "write -P 33 1000 128k" -c "read -P 33 1000 128k" | _filter_qemu_io
+
+echo
+echo "== write zero with constrained max-transfer =="
+limits=align=512,max-transfer=64k,opt-write-zero=$CLUSTER_SIZE
+$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
+         -c "write -z 8003584 2093056" | _filter_qemu_io
+
+# non-power-of-2 write-zero/discard alignments
+echo
+echo "== non-power-of-2 write zeroes =="
+
+limits=align=512,opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
+$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
+         -c "write -z 32M 32M" | _filter_qemu_io
+
+echo
+echo "== non-power-of-2 discard =="
+
+limits=align=512,opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
+$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
+         -c "discard 80000001 30M" | _filter_qemu_io
+
+echo
+echo "== verify image content =="
+
+function verify_io()
+{
+    if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" |
+	    grep "compat: 0.10" > /dev/null); then
+        # For v2 images, discarded clusters are read from the backing file
+        discarded=11
+    else
+        # Discarded clusters are zeroed for v3 or later
+        discarded=0
+    fi
+
+    echo read -P 22 0 1000
+    echo read -P 33 1000 128k
+    echo read -P 22 132072 7871512
+    echo read -P 0 8003584 2093056
+    echo read -P 22 10096640 23457792
+    echo read -P 0 32M 32M
+    echo read -P 22 64M 13M
+    echo read -P $discarded 77M 29M
+    echo read -P 22 106M 22M
+}
+
+verify_io | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
+
+_check_test_img
+
+# success, all done
+echo "*** done"
+status=0
diff --git a/tests/qemu-iotests/173.out b/tests/qemu-iotests/173.out
new file mode 100644
index 0000000..3665c3d
--- /dev/null
+++ b/tests/qemu-iotests/173.out
@@ -0,0 +1,49 @@
+QA output created by 173
+
+== setting up files ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+wrote 134217728/134217728 bytes at offset 0
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
+wrote 134217728/134217728 bytes at offset 0
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== constrained alignment and max-transfer ==
+wrote 131072/131072 bytes at offset 1000
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 1000
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== write zero with constrained max-transfer ==
+wrote 2093056/2093056 bytes at offset 8003584
+1.996 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== non-power-of-2 write zeroes ==
+wrote 33554432/33554432 bytes at offset 33554432
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== non-power-of-2 discard ==
+discard 31457280/31457280 bytes at offset 80000001
+30 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify image content ==
+read 1000/1000 bytes at offset 0
+1000 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 1000
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 7871512/7871512 bytes at offset 132072
+7.507 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 2093056/2093056 bytes at offset 8003584
+1.996 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 23457792/23457792 bytes at offset 10096640
+22.371 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 33554432/33554432 bytes at offset 33554432
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 13631488/13631488 bytes at offset 67108864
+13 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 30408704/30408704 bytes at offset 80740352
+29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 23068672/23068672 bytes at offset 111149056
+22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 866c1a0..0453e6c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -165,3 +165,4 @@
 170 rw auto quick
 171 rw auto quick
 172 auto
+173 rw auto quick
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 5/5] tests: Add coverage for recent block geometry fixes

[Max Reitz]

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

On 02.12.2016 20:22, Eric Blake wrote:
> Use blkdebug's new geometry constraints to emulate setups that
> have caused recent regression fixes: write zeroes asserting
> when running through a loopback block device with max-transfer
> smaller than cluster size, and discard rounding away portions
> of requests not aligned to preferred boundaries.  Also, add
> coverage that the block layer is honoring max transfer limits.
> 
> For now, a single iotest performs all actions, with the idea
> that we can add future blkdebug constraint test cases in the
> same file; but it can be split into multiple iotests if we find
> reason to run one portion of the test in more setups than what
> are possible in the other.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: make comments tied more to test at hand, rather than the
> particular hardware that led to the earlier patches being tested
> v2: new patch
> ---
>  tests/qemu-iotests/173     | 115 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/173.out |  49 +++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 165 insertions(+)
>  create mode 100755 tests/qemu-iotests/173
>  create mode 100644 tests/qemu-iotests/173.out

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v3 5/5] tests: Add coverage for recent block geometry fixes

[Kevin Wolf]

Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
> Use blkdebug's new geometry constraints to emulate setups that
> have caused recent regression fixes: write zeroes asserting
> when running through a loopback block device with max-transfer
> smaller than cluster size, and discard rounding away portions
> of requests not aligned to preferred boundaries.  Also, add
> coverage that the block layer is honoring max transfer limits.
> 
> For now, a single iotest performs all actions, with the idea
> that we can add future blkdebug constraint test cases in the
> same file; but it can be split into multiple iotests if we find
> reason to run one portion of the test in more setups than what
> are possible in the other.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: make comments tied more to test at hand, rather than the
> particular hardware that led to the earlier patches being tested
> v2: new patch
> ---
>  tests/qemu-iotests/173     | 115 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/173.out |  49 +++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 165 insertions(+)
>  create mode 100755 tests/qemu-iotests/173
>  create mode 100644 tests/qemu-iotests/173.out
> 
> diff --git a/tests/qemu-iotests/173 b/tests/qemu-iotests/173
> new file mode 100755
> index 0000000..fd421fc
> --- /dev/null
> +++ b/tests/qemu-iotests/173
> @@ -0,0 +1,115 @@
> +#!/bin/bash
> +#
> +# Test corner cases with unusual block geometries
> +#
> +# Copyright (C) 2016 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=eblake@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +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
> +
> +CLUSTER_SIZE=1M
> +size=128M
> +options=driver=blkdebug,image.driver=qcow2
> +
> +echo
> +echo "== setting up files =="
> +
> +_make_test_img $size
> +$QEMU_IO -c "write -P 11 0 $size" "$TEST_IMG" | _filter_qemu_io
> +mv "$TEST_IMG" "$TEST_IMG.base"

I know that you declared "_supported_proto file", but if you don't use
mv after creating the image, it might actually work with other
protocols.

Most other tests use something like this:

    TEST_IMG="$TEST_IMG.base" _make_test_img $size

And for the qemu-io invocation you can just use the right filename.

> +_make_test_img -b "$TEST_IMG.base"
> +$QEMU_IO -c "write -P 22 0 $size" "$TEST_IMG" | _filter_qemu_io
> +
> +# Limited to 64k max-transfer
> +echo
> +echo "== constrained alignment and max-transfer =="
> +limits=align=4k,max-transfer=64k
> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
> +         -c "write -P 33 1000 128k" -c "read -P 33 1000 128k" | _filter_qemu_io
> +
> +echo
> +echo "== write zero with constrained max-transfer =="
> +limits=align=512,max-transfer=64k,opt-write-zero=$CLUSTER_SIZE
> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
> +         -c "write -z 8003584 2093056" | _filter_qemu_io
> +
> +# non-power-of-2 write-zero/discard alignments
> +echo
> +echo "== non-power-of-2 write zeroes =="

"non-power-of-2 write zeroes _limits_". The request offset/size is a
power of two.

> +limits=align=512,opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
> +         -c "write -z 32M 32M" | _filter_qemu_io
> +
> +echo
> +echo "== non-power-of-2 discard =="

Here limits, too.

> +limits=align=512,opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
> +         -c "discard 80000001 30M" | _filter_qemu_io
> +
> +echo
> +echo "== verify image content =="
> +
> +function verify_io()
> +{
> +    if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" |
> +	    grep "compat: 0.10" > /dev/null); then
> +        # For v2 images, discarded clusters are read from the backing file
> +        discarded=11
> +    else
> +        # Discarded clusters are zeroed for v3 or later
> +        discarded=0
> +    fi
> +
> +    echo read -P 22 0 1000
> +    echo read -P 33 1000 128k
> +    echo read -P 22 132072 7871512
> +    echo read -P 0 8003584 2093056
> +    echo read -P 22 10096640 23457792
> +    echo read -P 0 32M 32M
> +    echo read -P 22 64M 13M
> +    echo read -P $discarded 77M 29M

Hm, why is this exactly 77M?

The original request starts at 80000001, 77M is 80740352. We have a
discard limit of 15M, but that is only used for splitting the request
(and wouldn't match 77M anyway). We still pass the partial requests at
the head and the tail to the driver, and what it enforces is align, i.e.
512. The next 512 byte boundary from 80000001 would be 80000512.

I'm almost sure that the patch is correct and I'm just missing a piece,
but what is it?

> +    echo read -P 22 106M 22M
> +}
> +
> +verify_io | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
> +
> +_check_test_img
> +
> +# success, all done
> +echo "*** done"
> +status=0

Kevin

Re: [Qemu-devel] [PATCH v3 5/5] tests: Add coverage for recent block geometry fixes

[Eric Blake]

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

On 12/07/2016 10:16 AM, Kevin Wolf wrote:
> Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
>> Use blkdebug's new geometry constraints to emulate setups that
>> have caused recent regression fixes: write zeroes asserting
>> when running through a loopback block device with max-transfer
>> smaller than cluster size, and discard rounding away portions
>> of requests not aligned to preferred boundaries.  Also, add
>> coverage that the block layer is honoring max transfer limits.
>>
>> For now, a single iotest performs all actions, with the idea
>> that we can add future blkdebug constraint test cases in the
>> same file; but it can be split into multiple iotests if we find
>> reason to run one portion of the test in more setups than what
>> are possible in the other.
>>

>> +
>> +_supported_fmt qcow2
>> +_supported_proto file
>> +
>> +CLUSTER_SIZE=1M
>> +size=128M
>> +options=driver=blkdebug,image.driver=qcow2
>> +
>> +echo
>> +echo "== setting up files =="
>> +
>> +_make_test_img $size
>> +$QEMU_IO -c "write -P 11 0 $size" "$TEST_IMG" | _filter_qemu_io
>> +mv "$TEST_IMG" "$TEST_IMG.base"
> 
> I know that you declared "_supported_proto file", but if you don't use
> mv after creating the image, it might actually work with other
> protocols.
> 
> Most other tests use something like this:
> 
>     TEST_IMG="$TEST_IMG.base" _make_test_img $size
> 
> And for the qemu-io invocation you can just use the right filename.

Thanks.  I obviously copied-and-pasted from an earlier test, rather than
a later one, so I'll make the tweaks in v4.


>> +# non-power-of-2 write-zero/discard alignments
>> +echo
>> +echo "== non-power-of-2 write zeroes =="
> 
> "non-power-of-2 write zeroes _limits_". The request offset/size is a
> power of two.

Indeed.

> 
>> +limits=align=512,opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
>> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
>> +         -c "write -z 32M 32M" | _filter_qemu_io
>> +
>> +echo
>> +echo "== non-power-of-2 discard =="
> 
> Here limits, too.
> 
>> +limits=align=512,opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
>> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
>> +         -c "discard 80000001 30M" | _filter_qemu_io
>> +
>> +echo
>> +echo "== verify image content =="
>> +
>> +function verify_io()
>> +{
>> +    if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" |
>> +	    grep "compat: 0.10" > /dev/null); then
>> +        # For v2 images, discarded clusters are read from the backing file
>> +        discarded=11
>> +    else
>> +        # Discarded clusters are zeroed for v3 or later
>> +        discarded=0
>> +    fi
>> +
>> +    echo read -P 22 0 1000
>> +    echo read -P 33 1000 128k
>> +    echo read -P 22 132072 7871512
>> +    echo read -P 0 8003584 2093056
>> +    echo read -P 22 10096640 23457792
>> +    echo read -P 0 32M 32M
>> +    echo read -P 22 64M 13M
>> +    echo read -P $discarded 77M 29M
> 
> Hm, why is this exactly 77M?
> 
> The original request starts at 80000001, 77M is 80740352. We have a
> discard limit of 15M, but that is only used for splitting the request
> (and wouldn't match 77M anyway). We still pass the partial requests at
> the head and the tail to the driver, and what it enforces is align, i.e.
> 512. The next 512 byte boundary from 80000001 would be 80000512.
> 
> I'm almost sure that the patch is correct and I'm just missing a piece,
> but what is it?

We are using a qcow2 image with 1M clusters.  Discarding visible through
qcow2 is therefore at 1M boundaries, regardless of whether we can
discard at finer granularity elsewhere.

Stepping through the request, we have:

qemu-io: discard 30M at 80000001, passed to blkdebug
  blkdebug: discard 511 bytes at 80000001, -ENOTSUP (smaller than
blkdebug's 512 align)
  blkdebug: discard 14371328 bytes at 80000512, passed to qcow2
    qcow2: discard 739840 bytes at 80000512, -ENOTSUP (smaller than
qcow2's 1M align)
    qcow2: discard 13M bytes at 77M, succeeds
  blkdebug: discard 15M bytes at 90M, passed to qcow2
    qcow2: discard 15M bytes at 90M, succeeds
  blkdebug: discard 1356800 bytes at 105M, passed to qcow2
    qcow2: discard 1M at 105M, succeeds
    qcow2: discard 308224 bytes at 106M, -ENOTSUP (smaller than qcow2's
1M align)
  blkdebug: discard 1 byte at 111457280, -ENOTSUP (smaller than
blkdebug's 512 align)


-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 5/5] tests: Add coverage for recent block geometry fixes

[Eric Blake]

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

On 12/07/2016 10:34 AM, Eric Blake wrote:
> On 12/07/2016 10:16 AM, Kevin Wolf wrote:
>> Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
>>> Use blkdebug's new geometry constraints to emulate setups that
>>> have caused recent regression fixes: write zeroes asserting
>>> when running through a loopback block device with max-transfer
>>> smaller than cluster size, and discard rounding away portions
>>> of requests not aligned to preferred boundaries.  Also, add
>>> coverage that the block layer is honoring max transfer limits.
>>>

>>> +
>>> +_supported_fmt qcow2
>>> +_supported_proto file
>>> +
>>> +CLUSTER_SIZE=1M
>>> +size=128M
>>> +options=driver=blkdebug,image.driver=qcow2
>>> +
>>> +echo
>>> +echo "== setting up files =="
>>> +
>>> +_make_test_img $size
>>> +$QEMU_IO -c "write -P 11 0 $size" "$TEST_IMG" | _filter_qemu_io
>>> +mv "$TEST_IMG" "$TEST_IMG.base"
>>
>> I know that you declared "_supported_proto file", but if you don't use
>> mv after creating the image, it might actually work with other
>> protocols.
>>
>> Most other tests use something like this:
>>
>>     TEST_IMG="$TEST_IMG.base" _make_test_img $size
>>
>> And for the qemu-io invocation you can just use the right filename.
> 
> Thanks.  I obviously copied-and-pasted from an earlier test, rather than
> a later one, so I'll make the tweaks in v4.

Even with the tweaks, I still wasn't able to get things like:

./check -qcow2 -nfs 173

to work.  I'm probably missing something trivial, but if someone wants
to improve the test as a follow-up, they can be my guest.  I'll go ahead
and post v4 with the cleaner creation of the backing file, but leave the
_supported_proto file line for now.

-- 
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: 604 bytes --]