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

[Eric Blake]

On Thu, Jan 26, 2023 at 03:14:00PM -0500, 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.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-io-cmds.c | 149 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 103 insertions(+), 46 deletions(-)
> 

>  static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> -                                     const char *file_name)
> +                                     const char *file_name, bool 
> register_buf)
>  {
>      char *buf, *buf_origin;
>      FILE *f = fopen(file_name, "r");
> @@ -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);
> +    }

Why is this conditionally true...

> +
>      if (len > pattern_len) {
>          len -= pattern_len;
>          buf += pattern_len;
> @@ -431,7 +444,7 @@ static void *qemu_io_alloc_from_file(BlockBackend *blk, 
> size_t len,
>      return buf_origin;
>  
>  error:
> -    qemu_io_free(buf_origin);
> +    qemu_io_free(blk, buf_origin, len, false);

...but cleanup unconditionally false?

> @@ -719,6 +732,7 @@ static void read_help(void)
>  " -p, -- ignored for backwards compatibility\n"
>  " -P, -- use a pattern to verify read data\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
> +" -r, -- register I/O buffer\n"
>  " -s, -- start offset for pattern verification (only with -P)\n"
>  " -v, -- dump buffer to standard output\n"

Here we have kept more or less alphabetical...

> @@ -909,6 +932,7 @@ static void readv_help(void)
>  " -P, -- use a pattern to verify read data\n"
>  " -v, -- dump buffer to standard output\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
> +" -r, -- register I/O buffer\n"

...here we have not.  While it's pre-existing, would it be worth
touching up while you're here?

> @@ -1455,6 +1503,7 @@ static void aio_read_help(void)
>  " -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"
> +" -r, -- register I/O buffer\n"

Another pre-existing spot.

Overall makes sense to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org