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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Tue, Feb 07, 2023 at 12:47:06PM +0100, Hanna Czenczek wrote:
> 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()`.

Good catch, thank you!

> 
> > +    }
> >       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.

Oops, thanks!

> 
> >           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.

Yes, you're right. I will reverse the order here.

> 
> >       }
> >       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.)

I added an explicit check in write_f() and forgot to add the same check
to aio_write_f(). Will fix.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]