[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/4] qemu-io: add -r option to register I/O buffer
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 3/4] qemu-io: add -r option to register I/O buffer |
Date: |
Mon, 30 Jan 2023 15:55:20 -0500 |
On Fri, Jan 27, 2023 at 05:24:30PM -0600, Eric Blake wrote:
> 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?
It looks suspicious but it's correct: goto error only happens before the
blk_register_buf() call. Therefore we must *not* unregister the buffer
at this point.
I'll add a comment to make this clear.
>
> > @@ -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.
Sure, I'll order the help output alphabetically in the next revision.
Stefan
signature.asc
Description: PGP signature