Re: [RFC v2 1/8] blkio: add io_uring block driver using libblkio

[Kevin Wolf]

Am 07.04.2022 um 10:25 hat Kevin Wolf geschrieben:
> Am 07.04.2022 um 09:22 hat Stefan Hajnoczi geschrieben:
> > On Wed, Apr 06, 2022 at 07:32:04PM +0200, Kevin Wolf wrote:
> > > Am 05.04.2022 um 17:33 hat Stefan Hajnoczi geschrieben:
> > > > libblkio (https://gitlab.com/libblkio/libblkio/) is a library for
> > > > high-performance disk I/O. It currently supports io_uring with
> > > > additional drivers planned.
> > > > 
> > > > One of the reasons for developing libblkio is that other applications
> > > > besides QEMU can use it. This will be particularly useful for
> > > > vhost-user-blk which applications may wish to use for connecting to
> > > > qemu-storage-daemon.
> > > > 
> > > > libblkio also gives us an opportunity to develop in Rust behind a C API
> > > > that is easy to consume from QEMU.
> > > > 
> > > > This commit adds an io_uring BlockDriver to QEMU using libblkio. For now
> > > > I/O buffers are copied through bounce buffers if the libblkio driver
> > > > requires it. Later commits add an optimization for pre-registering guest
> > > > RAM to avoid bounce buffers. It will be easy to add other libblkio
> > > > drivers since they will share the majority of code.
> > > > 
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > 
> > > > +static BlockDriver bdrv_io_uring = {
> > > > +    .format_name                = "io_uring",
> > > > +    .protocol_name              = "io_uring",
> > > > +    .instance_size              = sizeof(BDRVBlkioState),
> > > > +    .bdrv_needs_filename        = true,
> > > > +    .bdrv_parse_filename        = blkio_parse_filename_io_uring,
> > > > +    .bdrv_file_open             = blkio_file_open,
> > > > +    .bdrv_close                 = blkio_close,
> > > > +    .bdrv_getlength             = blkio_getlength,
> > > > +    .has_variable_length        = true,
> > > 
> > > This one is a bad idea. It means that every request will call
> > > blkio_getlength() first, which looks up the "capacity" property in
> > > libblkio and then calls lseek() for the io_uring backend.
> > 
> > Thanks for pointing this out. I didn't think this through. More below on
> > what I was trying to do.
> > 
> > > For other backends like the vhost_user one (where I just copied your
> > > definition and then noticed this behaviour), it involve a message over
> > > the vhost socket, which is even worse.
> > 
> > (A vhost-user-blk driver could cache the capacity field and update it
> > when a Configuration Change Notification is received. There is no need
> > to send a vhost-user protocol message every time.)
> 
> In theory we could cache in libblkio, but then we would need a mechanism
> to invalidate the cache so we can support resizing an image (similar to
> what block_resize does in QEMU, except that it wouldn't set the new
> size from a parameter, but just get the new value from the backend).

Oh, sorry, I misread. VHOST_USER_SLAVE_CONFIG_CHANGE_MSG is probably
what you mean, so that the backend triggers the update. It exists in the
spec, but neither libvhost-user nor rust-vmm seem to support it
currently. We also don't set up the backchannel yet where this message
could even be passed.

So it's an option, but probably only for later because it involves
extending several places.

> I think it's simpler to leave caching to the application - and QEMU
> already does this automatically if we don't set .has_variable_length =
> true.

I still think the application shouldn't query the capacity more often
than necessary, so optimising it is probably not very important.

Kevin

signature.asc
Description: PGP signature