Re: [PATCH v3 3/4] qemu-io: add -r option to register I/O buffer

[Hanna Czenczek]

On 01.02.23 16:27, Stefan Hajnoczi wrote:
> The blk_register_buf() API is an optimization hint that allows some
> block drivers to avoid I/O buffer housekeeping or bounce buffers.
>
> Add an -r option to register the I/O buffer so that qemu-io can be used
> to test the blk_register_buf() API. The next commit will add a test that
> uses the new option.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   qemu-io-cmds.c | 167 +++++++++++++++++++++++++++++++++----------------
>   1 file changed, 114 insertions(+), 53 deletions(-)
>
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index c0125d14c0..4b8dbef36d 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c

[...]

> @@ -347,17 +348,24 @@ static void *qemu_io_alloc(BlockBackend *blk, size_t len, 
> int pattern)
>       }
>       buf = blk_blockalign(blk, len);
>       memset(buf, pattern, len);
> +    if (register_buf) {
> +        blk_register_buf(blk, buf, len, &error_abort);
> +    }
>       if (qemuio_misalign) {
>           buf += MISALIGN_OFFSET;
>       }
>       return buf;
>   }
>   
> -static void qemu_io_free(void *p)
> +static void qemu_io_free(BlockBackend *blk, void *p, size_t len,
> +                         bool unregister_buf)
>   {
>       if (qemuio_misalign) {
>           p -= MISALIGN_OFFSET;
>       }
> +    if (unregister_buf) {
> +        blk_unregister_buf(blk, p, len);

If `qemuio_misalign` is set, we must also increase `len` by 
`MISALIGN_OFFSET`, I think, or it won’t match what’s been used in 
`qemu_io_alloc()`.

> +    }
>       qemu_vfree(p);
>   }

[...]

> @@ -414,6 +423,10 @@ static void *qemu_io_alloc_from_file(BlockBackend *blk, 
> size_t len,
>       fclose(f);
>       f = NULL;
>   
> +    if (register_buf) {
> +        blk_register_buf(blk, buf_origin, len, &error_abort);

`qemu_io_alloc()` registers the buffer before `MISALIGN_OFFSET` is/might 
be applied, and `qemu_io_free()` assumes this is the case (the buffer to 
be unregistered is passed after the offset has been subtracted again).  
Here, however, the offset has already been applied, so there’ll be a 
mismatch with `blk_unregister_buf()` when `qemu_io_free()` is called 
(and `qemuio_misalign` is set).

> +    }
> +
>       if (len > pattern_len) {
>           len -= pattern_len;
>           buf += pattern_len;

[...]

> @@ -750,6 +768,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>       int64_t total = 0;
>       int pattern = 0;
>       int64_t pattern_offset = 0, pattern_count = 0;
> +    BdrvRequestFlags flags = 0;
>   
>       while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {

I think we’ll need the "r" here.

>           switch (c) {

[...]

> @@ -1384,8 +1434,9 @@ static void aio_write_done(void *opaque, int ret)
>                    ctx->qiov.size, 1, ctx->Cflag);
>   out:
>       if (!ctx->zflag) {
> -        qemu_io_free(ctx->buf);
>           qemu_iovec_destroy(&ctx->qiov);
> +        qemu_io_free(ctx->blk, ctx->buf, ctx->qiov.size,
> +                     ctx->flags & BDRV_REQ_REGISTERED_BUF);

So far in this patch, you’ve always swapped the existing 
qemu_iovec_destroy(); qemu_io_free() combination to qemu_io_free(); 
qemu_iovec_destroy().  I think that is correct because 
qemu_iovec_destroy() overwrites the qiov by 0, so that accessing 
qiov.size will then read 0, regardless of what it was before.

Here, you’re swapping it the other way around, which means that 
`ctx->qiov.size` will read 0 when `qemu_io_free()` is called, which 
seems wrong.

>       }
>       g_free(ctx);
>   }

[...]

> @@ -1663,12 +1724,12 @@ static int aio_write_f(BlockBackend *blk, int argc, 
> char **argv)
>           }
>   
>           ctx->qiov.size = count;
> -        blk_aio_pwrite_zeroes(blk, ctx->offset, count, flags, aio_write_done,
> -                              ctx);
> +        blk_aio_pwrite_zeroes(blk, ctx->offset, count, ctx->flags,
> +                              aio_write_done, ctx);

write_f() emits an error when -r is used together with -z – why doesn’t 
this function, too?  (Or, alternatively, why does write_f()?  Maybe we 
want to check what happens when you call a zero-writing function with 
that flag.  Or we don’t.)

Hanna

>       } else {
>           nr_iov = argc - optind;
>           ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov,
> -                                pattern);
> +                                pattern, ctx->flags & BDRV_REQ_REGISTERED_BUF);
>           if (ctx->buf == NULL) {
>               block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_WRITE);
>               g_free(ctx);