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 <ebl...@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefa...@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
signature.asc
Description: PGP signature
