[Qemu-devel] [PATCH for-2.6 0/2] block: Fix blk_aio_write_zeroes()

[Qemu-devel] [PATCH for-2.6 0/2] block: Fix blk_aio_write_zeroes()

[Kevin Wolf]

Kevin Wolf (2):
  qemu-io: Support 'aio_write -z'
  block: Fix blk_aio_write_zeroes()

 block/block-backend.c      | 20 +++++++----
 qemu-io-cmds.c             | 64 +++++++++++++++++++++++++++---------
 tests/qemu-iotests/033     |  8 +++--
 tests/qemu-iotests/033.out | 82 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 148 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.6 1/2] qemu-io: Support 'aio_write -z'

[Kevin Wolf]

This allows testing blk_aio_write_zeroes().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 64 +++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 16 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 382faa8..51ae5a5 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1416,6 +1416,7 @@ struct aio_ctx {
     int vflag;
     int Cflag;
     int Pflag;
+    int zflag;
     BlockAcctCookie acct;
     int pattern;
     struct timeval t1;
@@ -1446,8 +1447,10 @@ static void aio_write_done(void *opaque, int ret)
     print_report("wrote", &t2, ctx->offset, ctx->qiov.size,
                  ctx->qiov.size, 1, ctx->Cflag);
 out:
-    qemu_io_free(ctx->buf);
-    qemu_iovec_destroy(&ctx->qiov);
+    if (!ctx->zflag) {
+        qemu_io_free(ctx->buf);
+        qemu_iovec_destroy(&ctx->qiov);
+    }
     g_free(ctx);
 }
 
@@ -1612,6 +1615,7 @@ static void aio_write_help(void)
 " -P, -- use different pattern to fill file\n"
 " -C, -- report statistics in a machine parsable format\n"
 " -q, -- quiet mode, do not show I/O statistics\n"
+" -z, -- write zeroes using blk_aio_write_zeroes\n"
 "\n");
 }
 
@@ -1622,7 +1626,7 @@ static const cmdinfo_t aio_write_cmd = {
     .cfunc      = aio_write_f,
     .argmin     = 2,
     .argmax     = -1,
-    .args       = "[-Cq] [-P pattern ] off len [len..]",
+    .args       = "[-Cqz] [-P pattern ] off len [len..]",
     .oneline    = "asynchronously writes a number of bytes",
     .help       = aio_write_help,
 };
@@ -1634,7 +1638,7 @@ static int aio_write_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, "CqP:")) != -1) {
+    while ((c = getopt(argc, argv, "CqP:z")) != -1) {
         switch (c) {
         case 'C':
             ctx->Cflag = 1;
@@ -1649,6 +1653,9 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
                 return 0;
             }
             break;
+        case 'z':
+            ctx->zflag = 1;
+            break;
         default:
             g_free(ctx);
             return qemuio_command_usage(&aio_write_cmd);
@@ -1660,6 +1667,18 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
         return qemuio_command_usage(&aio_write_cmd);
     }
 
+    if (ctx->zflag && optind != argc - 2) {
+        printf("-z supports only a single length parameter\n");
+        g_free(ctx);
+        return 0;
+    }
+
+    if (ctx->zflag && ctx->Pflag) {
+        printf("-z and -P cannot be specified at the same time\n");
+        g_free(ctx);
+        return 0;
+    }
+
     ctx->offset = cvtnum(argv[optind]);
     if (ctx->offset < 0) {
         print_cvtnum_err(ctx->offset, argv[optind]);
@@ -1676,19 +1695,32 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
         return 0;
     }
 
-    nr_iov = argc - optind;
-    ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov, pattern);
-    if (ctx->buf == NULL) {
-        block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_WRITE);
-        g_free(ctx);
-        return 0;
-    }
+    if (ctx->zflag) {
+        int64_t count = cvtnum(argv[optind]);
+        if (count < 0) {
+            print_cvtnum_err(count, argv[optind]);
+            return 0;
+        }
 
-    gettimeofday(&ctx->t1, NULL);
-    block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size,
-                     BLOCK_ACCT_WRITE);
-    blk_aio_writev(blk, ctx->offset >> 9, &ctx->qiov,
-                   ctx->qiov.size >> 9, aio_write_done, ctx);
+        ctx->qiov.size = count;
+        blk_aio_write_zeroes(blk, ctx->offset >> 9, count >> 9, 0,
+                             aio_write_done, ctx);
+    } else {
+        nr_iov = argc - optind;
+        ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov, pattern);
+        if (ctx->buf == NULL) {
+            block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_WRITE);
+            g_free(ctx);
+            return 0;
+        }
+
+        gettimeofday(&ctx->t1, NULL);
+        block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size,
+                         BLOCK_ACCT_WRITE);
+
+        blk_aio_writev(blk, ctx->offset >> 9, &ctx->qiov,
+                       ctx->qiov.size >> 9, aio_write_done, ctx);
+    }
     return 0;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.6 1/2] qemu-io: Support 'aio_write -z'

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 465 bytes --]

On 13.04.2016 13:06, Kevin Wolf wrote:
> This allows testing blk_aio_write_zeroes().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io-cmds.c | 64 +++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 48 insertions(+), 16 deletions(-)

Despite the continuing use of int instead of bool where the latter would
be appropriate and of "9" instead of "BDRV_SECTOR_BITS":

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.6 1/2] qemu-io: Support 'aio_write -z'

[Kevin Wolf]

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

Am 15.04.2016 um 01:14 hat Max Reitz geschrieben:
> On 13.04.2016 13:06, Kevin Wolf wrote:
> > This allows testing blk_aio_write_zeroes().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qemu-io-cmds.c | 64 +++++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 48 insertions(+), 16 deletions(-)
> 
> Despite the continuing use of int instead of bool where the latter would
> be appropriate and of "9" instead of "BDRV_SECTOR_BITS":

I considered whether to use bool, but having a few ints and then a bool
for flags in the same struct would be inconsistent. We can have a
cleanup patch on top that changes all instances.

Same thing really with the explicit 9. I didn't even notice it, but this
is just copied code and changing it in one instance only would be
inconsistent.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.6 1/2] qemu-io: Support 'aio_write -z'

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 610 bytes --]

On 13.04.2016 13:06, Kevin Wolf wrote:
> This allows testing blk_aio_write_zeroes().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io-cmds.c | 64 +++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 48 insertions(+), 16 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 382faa8..51ae5a5 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c

[...]

> +        ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov, pattern);

I just noticed this line has over 80 characters. R-b stands provided
this is fixed.

Max


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

[Qemu-devel] [PATCH for-2.6 2/2] block: Fix blk_aio_write_zeroes()

[Kevin Wolf]

Commit 57d6a428 broke blk_aio_write_zeroes() because in some write
functions in the call path don't have an explicit length argument but
reuse qiov->size instead. Which is great, except that write_zeroes
doesn't have a qiov, which this commit interprets as 0 bytes.
Consequently, blk_aio_write_zeroes() didn't effectively do anything.

This patch introduces an explicit acb->bytes in BlkAioEmAIOCB and uses
that instead of acb->rwco.size.

The synchronous version of the function is okay because it does pass a
qiov (with the right size and a NULL pointer as its base).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c      | 20 +++++++----
 tests/qemu-iotests/033     |  8 +++--
 tests/qemu-iotests/033.out | 82 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index d74f670..140c3f7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -852,6 +852,7 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
 typedef struct BlkAioEmAIOCB {
     BlockAIOCB common;
     BlkRwCo rwco;
+    int bytes;
     bool has_returned;
     QEMUBH* bh;
 } BlkAioEmAIOCB;
@@ -877,7 +878,7 @@ static void blk_aio_complete_bh(void *opaque)
     blk_aio_complete(opaque);
 }
 
-static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset,
+static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
                                 QEMUIOVector *qiov, CoroutineEntry co_entry,
                                 BdrvRequestFlags flags,
                                 BlockCompletionFunc *cb, void *opaque)
@@ -893,6 +894,7 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset,
         .flags  = flags,
         .ret    = NOT_DONE,
     };
+    acb->bytes = bytes;
     acb->bh = NULL;
     acb->has_returned = false;
 
@@ -913,7 +915,8 @@ static void blk_aio_read_entry(void *opaque)
     BlkAioEmAIOCB *acb = opaque;
     BlkRwCo *rwco = &acb->rwco;
 
-    rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, rwco->qiov->size,
+    assert(rwco->qiov->size == acb->bytes);
+    rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, acb->bytes,
                               rwco->qiov, rwco->flags);
     blk_aio_complete(acb);
 }
@@ -923,8 +926,8 @@ static void blk_aio_write_entry(void *opaque)
     BlkAioEmAIOCB *acb = opaque;
     BlkRwCo *rwco = &acb->rwco;
 
-    rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset,
-                               rwco->qiov ? rwco->qiov->size : 0,
+    assert(!rwco->qiov || rwco->qiov->size == acb->bytes);
+    rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, acb->bytes,
                                rwco->qiov, rwco->flags);
     blk_aio_complete(acb);
 }
@@ -937,7 +940,8 @@ BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
         return blk_abort_aio_request(blk, cb, opaque, -EINVAL);
     }
 
-    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS, NULL,
+    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS,
+                        nb_sectors << BDRV_SECTOR_BITS, NULL,
                         blk_aio_write_entry, BDRV_REQ_ZERO_WRITE, cb, opaque);
 }
 
@@ -994,7 +998,8 @@ BlockAIOCB *blk_aio_readv(BlockBackend *blk, int64_t sector_num,
         return blk_abort_aio_request(blk, cb, opaque, -EINVAL);
     }
 
-    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS, iov,
+    assert(nb_sectors << BDRV_SECTOR_BITS == iov->size);
+    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS, iov->size, iov,
                         blk_aio_read_entry, 0, cb, opaque);
 }
 
@@ -1006,7 +1011,8 @@ BlockAIOCB *blk_aio_writev(BlockBackend *blk, int64_t sector_num,
         return blk_abort_aio_request(blk, cb, opaque, -EINVAL);
     }
 
-    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS, iov,
+    assert(nb_sectors << BDRV_SECTOR_BITS == iov->size);
+    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS, iov->size, iov,
                         blk_aio_write_entry, 0, cb, opaque);
 }
 
diff --git a/tests/qemu-iotests/033 b/tests/qemu-iotests/033
index a61d8ce..912373d 100755
--- a/tests/qemu-iotests/033
+++ b/tests/qemu-iotests/033
@@ -57,12 +57,13 @@ do_test()
 	} | $QEMU_IO
 }
 
+for write_zero_cmd in "write -z" "aio_write -z"; do
 for align in 512 4k; do
 	echo
 	echo "== preparing image =="
 	do_test $align "write -P 0xa 0x200 0x400" "$TEST_IMG" | _filter_qemu_io
 	do_test $align "write -P 0xa 0x20000 0x600" "$TEST_IMG" | _filter_qemu_io
-	do_test $align "write -z 0x400 0x20000" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "$write_zero_cmd 0x400 0x20000" "$TEST_IMG" | _filter_qemu_io
 
 	echo
 	echo "== verifying patterns (1) =="
@@ -73,7 +74,7 @@ for align in 512 4k; do
 	echo
 	echo "== rewriting zeroes =="
 	do_test $align "write -P 0xb 0x10000 0x10000" "$TEST_IMG" | _filter_qemu_io
-	do_test $align "write -z 0x10000 0x10000" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "$write_zero_cmd 0x10000 0x10000" "$TEST_IMG" | _filter_qemu_io
 
 	echo
 	echo "== verifying patterns (2) =="
@@ -82,7 +83,7 @@ for align in 512 4k; do
 	echo
 	echo "== rewriting unaligned zeroes =="
 	do_test $align "write -P 0xb 0x0 0x1000" "$TEST_IMG" | _filter_qemu_io
-	do_test $align "write -z 0x200 0x200" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "$write_zero_cmd 0x200 0x200" "$TEST_IMG" | _filter_qemu_io
 
 	echo
 	echo "== verifying patterns (3) =="
@@ -92,6 +93,7 @@ for align in 512 4k; do
 
 	echo
 done
+done
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/033.out b/tests/qemu-iotests/033.out
index c3d18aa..95929ef 100644
--- a/tests/qemu-iotests/033.out
+++ b/tests/qemu-iotests/033.out
@@ -82,4 +82,86 @@ read 512/512 bytes at offset 512
 read 3072/3072 bytes at offset 1024
 3 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+
+== preparing image ==
+wrote 1024/1024 bytes at offset 512
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1536/1536 bytes at offset 131072
+1.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (1) ==
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 132096
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting zeroes ==
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (2) ==
+read 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting unaligned zeroes ==
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (3) ==
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 3072/3072 bytes at offset 1024
+3 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
+== preparing image ==
+wrote 1024/1024 bytes at offset 512
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1536/1536 bytes at offset 131072
+1.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (1) ==
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 132096
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting zeroes ==
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (2) ==
+read 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting unaligned zeroes ==
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (3) ==
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 3072/3072 bytes at offset 1024
+3 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 *** done
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.6 2/2] block: Fix blk_aio_write_zeroes()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2038 bytes --]

On 13.04.2016 13:06, Kevin Wolf wrote:
> Commit 57d6a428 broke blk_aio_write_zeroes() because in some write
> functions in the call path don't have an explicit length argument but
> reuse qiov->size instead. Which is great, except that write_zeroes
> doesn't have a qiov, which this commit interprets as 0 bytes.
> Consequently, blk_aio_write_zeroes() didn't effectively do anything.

You mean it has written null data? :-)

Alternative version: So it has written zero bytes? Works as intended, then.

> 
> This patch introduces an explicit acb->bytes in BlkAioEmAIOCB and uses
> that instead of acb->rwco.size.
> 
> The synchronous version of the function is okay because it does pass a
> qiov (with the right size and a NULL pointer as its base).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c      | 20 +++++++----
>  tests/qemu-iotests/033     |  8 +++--
>  tests/qemu-iotests/033.out | 82 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+), 10 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index d74f670..140c3f7 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c

[...]

> @@ -937,7 +940,8 @@ BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
>          return blk_abort_aio_request(blk, cb, opaque, -EINVAL);
>      }
>  
> -    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS, NULL,
> +    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS,
> +                        nb_sectors << BDRV_SECTOR_BITS, NULL,
>                          blk_aio_write_entry, BDRV_REQ_ZERO_WRITE, cb, opaque);
>  }

Another thing about blk_aio_write_zeroes() that might need fixing is
that it completely ignores its flags parameter.

That can be done in a seperate patch, however (and is probably not
critical, as this currently just means that a potential REQ_MAY_UNMAP
from scsi-disk is ignored), so:

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.6 2/2] block: Fix blk_aio_write_zeroes()

[Kevin Wolf]

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

Am 15.04.2016 um 01:30 hat Max Reitz geschrieben:
> On 13.04.2016 13:06, Kevin Wolf wrote:
> > Commit 57d6a428 broke blk_aio_write_zeroes() because in some write
> > functions in the call path don't have an explicit length argument but
> > reuse qiov->size instead. Which is great, except that write_zeroes
> > doesn't have a qiov, which this commit interprets as 0 bytes.
> > Consequently, blk_aio_write_zeroes() didn't effectively do anything.
> 
> You mean it has written null data? :-)
> 
> Alternative version: So it has written zero bytes? Works as intended, then.

I consciously avoided the latter wording. ;-)

> > This patch introduces an explicit acb->bytes in BlkAioEmAIOCB and uses
> > that instead of acb->rwco.size.
> > 
> > The synchronous version of the function is okay because it does pass a
> > qiov (with the right size and a NULL pointer as its base).
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> > @@ -937,7 +940,8 @@ BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
> >          return blk_abort_aio_request(blk, cb, opaque, -EINVAL);
> >      }
> >  
> > -    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS, NULL,
> > +    return blk_aio_prwv(blk, sector_num << BDRV_SECTOR_BITS,
> > +                        nb_sectors << BDRV_SECTOR_BITS, NULL,
> >                          blk_aio_write_entry, BDRV_REQ_ZERO_WRITE, cb, opaque);
> >  }
> 
> Another thing about blk_aio_write_zeroes() that might need fixing is
> that it completely ignores its flags parameter.
> 
> That can be done in a seperate patch, however (and is probably not
> critical, as this currently just means that a potential REQ_MAY_UNMAP
> from scsi-disk is ignored), so:

Oh dear... Commit 57d6a428 was not my best patch ever, and apparently
Stefan wasn't suspicious enough to catch the bugs. Maybe someone should
re-review the whole thing...

"just" ignoring REQ_MAY_UNMAP is a regression in discard support, so no,
that's not what I would call harmless. Definitely needs fixing for 2.6.
And we probably need more qtests checking such things.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]