Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Fix regression in 136 on aio_read invalid

[Max Reitz <mreitz@redhat.com>]

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

On 12.05.2016 23:39, Eric Blake wrote:
> Commit 093ea232 removed the ability for aio_read and aio_write
> to artificially inflate the invalid statistics counters for
> block devices, since it no longer flags unaligned offset or
> length.  Add 'aio_read -i' and 'aio_write -i' to restore
> the ability, and update test 136 to use it.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-io-cmds.c         | 20 ++++++++++++++++----
>  tests/qemu-iotests/136 | 18 +++++++-----------
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 415be25..059b8ee 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1476,6 +1476,7 @@ static void aio_read_help(void)
>  " used to ensure all outstanding aio requests have been completed.\n"
>  " -C, -- report statistics in a machine parsable format\n"
>  " -P, -- use a pattern to verify read data\n"
> +" -i, -- treat request as invalid, for exercising stats\n"
>  " -v, -- dump buffer to standard output\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
>  "\n");
> @@ -1488,7 +1489,7 @@ static const cmdinfo_t aio_read_cmd = {
>      .cfunc      = aio_read_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-Cqv] [-P pattern] off len [len..]",
> +    .args       = "[-Ciqv] [-P pattern] off len [len..]",
>      .oneline    = "asynchronously reads a number of bytes",
>      .help       = aio_read_help,
>  };
> @@ -1499,7 +1500,7 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
>      struct aio_ctx *ctx = g_new0(struct aio_ctx, 1);
> 
>      ctx->blk = blk;
> -    while ((c = getopt(argc, argv, "CP:qv")) != -1) {
> +    while ((c = getopt(argc, argv, "CP:iqv")) != -1) {
>          switch (c) {
>          case 'C':
>              ctx->Cflag = true;
> @@ -1512,6 +1513,11 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
>                  return 0;
>              }
>              break;
> +        case 'i':
> +            printf("injecting invalid read request\n");
> +            block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_READ);
> +            g_free(ctx);
> +            return 0;
>          case 'q':
>              ctx->qflag = true;
>              break;
> @@ -1569,6 +1575,7 @@ static void aio_write_help(void)
>  " -P, -- use different pattern to fill file\n"
>  " -C, -- report statistics in a machine parsable format\n"
>  " -f, -- use Force Unit Access semantics\n"
> +" -i, -- treat request as invalid, for exercising stats\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
>  " -u, -- with -z, allow unmapping\n"
>  " -z, -- write zeroes using blk_aio_write_zeroes\n"
> @@ -1582,7 +1589,7 @@ static const cmdinfo_t aio_write_cmd = {
>      .cfunc      = aio_write_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-Cfquz] [-P pattern] off len [len..]",
> +    .args       = "[-Cfiquz] [-P pattern] off len [len..]",
>      .oneline    = "asynchronously writes a number of bytes",
>      .help       = aio_write_help,
>  };
> @@ -1595,7 +1602,7 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
>      int flags = 0;
> 
>      ctx->blk = blk;
> -    while ((c = getopt(argc, argv, "CfqP:uz")) != -1) {
> +    while ((c = getopt(argc, argv, "CfiqP:uz")) != -1) {
>          switch (c) {
>          case 'C':
>              ctx->Cflag = true;
> @@ -1616,6 +1623,11 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
>                  return 0;
>              }
>              break;
> +        case 'i':
> +            printf("injecting invalid write request\n");
> +            block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_WRITE);
> +            g_free(ctx);
> +            return 0;
>          case 'z':
>              ctx->zflag = true;
>              break;
> diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
> index e8c6937..5e92c4b 100644
> --- a/tests/qemu-iotests/136
> +++ b/tests/qemu-iotests/136
> @@ -226,18 +226,14 @@ sector = "%d"
> 
>          highest_offset = wr_ops * wr_size
> 
> -        # Two types of invalid operations: unaligned length and unaligned offset
> -        for i in range(invalid_rd_ops / 2):
> -            ops.append("aio_read 0 511")
> +        # Block layer abstracts away unaligned length and offset, so we
> +        # can't trigger an invalid op with any addresses; use qemu-io's
> +        # invalid injection feature instead

This comment makes sense when seeing this patch, but it doesn't make a
lot of sense in this file after the patch has been applied.

We needed the comment to explain how the following commands are invalid
requests, but after this patch it's obvious, so it would be completely
fine to scratch the comment altogether. Or make it something short and
simple like "Generate invalid requests".

This being a comment and it just being a bit weird instead of wrong, I'd
be fine with applying the patch as-is, though, if you'd rather not send
a v2. What would you like it to be? :-)

Max

> +        for i in range(invalid_rd_ops):
> +            ops.append("aio_read -i 0 512")
> 
> -        for i in range(invalid_rd_ops / 2, invalid_rd_ops):
> -            ops.append("aio_read 13 512")
> -
> -        for i in range(invalid_wr_ops / 2):
> -            ops.append("aio_write 0 511")
> -
> -        for i in range(invalid_wr_ops / 2, invalid_wr_ops):
> -            ops.append("aio_write 13 512")
> +        for i in range(invalid_wr_ops):
> +            ops.append("aio_write -i 0 512")
> 
>          for i in range(failed_rd_ops):
>              ops.append("aio_read %d 512" % bad_offset)
> 



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