Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, May 24, 2023 at 11:05:38AM +0200, Stefano Garzarella wrote:
> Gentle ping :-)
> 
> On Wed, May 17, 2023 at 4:30 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Wed, May 17, 2023 at 09:19:26AM +0200, Stefano Garzarella wrote:
> > > CCing Markus for some advice.
> > >
> > > On Tue, May 16, 2023 at 11:04:21AM -0500, Jonathon Jongsma wrote:
> > > > On 5/15/23 5:10 AM, Stefano Garzarella wrote:
> > > > > On Thu, May 11, 2023 at 11:03:22AM -0500, Jonathon Jongsma wrote:
> > > > > > On 5/11/23 4:15 AM, Stefano Garzarella wrote:
> > > > > > > The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the new
> > > > > > > 'fd' property. Let's expose this to the user, so the management layer
> > > > > > > can pass the file descriptor of an already opened vhost-vdpa character
> > > > > > > device. This is useful especially when the device can only be accessed
> > > > > > > with certain privileges.
> > > > > > >
> > > > > > > If the libblkio virtio-blk driver supports fd passing, let's always
> > > > > > > use qemu_open() to open the `path`, so we can handle fd passing
> > > > > > > from the management layer through the "/dev/fdset/N" special path.
> > > > > > >
> > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Notes:
> > > > > > >     v3:
> > > > > > >     - use qemu_open() on `path` to simplify libvirt code [Jonathon]
> > > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > The one drawback now is that it doesn't seem possible for
> > > > > > libvirt to introspect whether or not qemu supports passing an fd
> > > > > > to the driver or not.
> > > > >
> > > > > Yep, this was because the libblkio library did not support this new way.
> > > > >
> > > > > > When I was writing my initial patch (before I realized that it
> > > > > > was missing fd-passing), I just checked for the existence of the
> > > > > > virtio-blk-vhost-vdpa device. But we actually need to know both
> > > > > > that this device exists and supports fd passing.
> > > > >
> > > > > Yep, this was one of the advantages of using the new `fd` parameter.
> > > > > Can't libvirt handle the later failure?
> > > >
> > > > Not very well. libvirt tries to provide useful errors to the user. So
> > > > for example if the qemu executable doesn't support a device, we would
> > > > want to provide an error indicating that the device is not supported
> > > > rather than a possibly-inscrutable qemu error.
> > > >
> > > > For example, in this scenario, we would want an error such as:
> > > >
> > > > error: unsupported configuration: vhostvdpa disk is not supported with
> > > > this QEMU binary
> > > >
> > > > Instead of:
> > > >
> > > > error: internal error: qemu unexpectedly closed the monitor:
> > > > 2023-05-16T15:17:36.666129Z qemu-system-x86_64: -blockdev {"driver":"virtio-blk-vhost-vdpa","path":"/dev/fdset/0","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}:
> > > > blkio_connect failed: Failed to connect to vDPA device: Input/output
> > > > error
> > > >
> > > > And we can only do that if we can determine that the binary has the
> > > > proper support for fds.
> > >
> > > I see the problem, thanks for explaining this!
> > >
> > > >
> > > > >
> > > > > > As far as I can tell, versions 7.2.0 and 8.0.0 include this
> > > > > > device but won't accept fds.
> > > > >
> > > > > Right.
> > > > >
> > > > > How do you suggest to proceed?
> > > >
> > > > I need some way to determine that the particular qemu binary can accept
> > > > a /dev/fdset/ path for vdpa block devices. libvirt uses a variety of
> > > > methods to determine capabilities for a given qemu binary, including
> > > > querying the qmp schema, commands, object types, specific device/object
> > > > properties, etc. For example, right now I can determine (via querying
> > > > the qmp schema) whether virtio-blk-vhost-vdpa is a valid type for the
> > > > blockdev-add command by querying the qmp schema. I need something more
> > > > than that but I'm not sure how to do it without introducing a separate
> > > > 'fd' parameter. Any ideas?
> > >
> > > The only thing I can think of is to make a mix between v2 and v3. I mean add
> > > both the new `fd` parameter, and support qemu_open() on `path`.
> > >
> > > That way libvirt (or other users) can check that fd passing is supported and
> > > use `fd` or fdset with `path`.
> > >
> > > Obviously I would have liked to implement only one of the two methods, but
> > > if this helps, maybe it makes sense to support both.
> > >
> > > What do you think?
> >
> > Markus: Is a preferred way to make this new path handling behavior
> > introspectable? I vaguely remember a way for QMP clients to query
> > strings that describe QMP behavior that's not otherwise
> > introspectable...
> 
> If there is no other way for introspection with QMP, I think adding
> `fd` and supporting `qemu_open()` on `path` is the easiest.

It doesn't hurt to go ahead with that approach. If Markus prefers a
different approach, he'll let us know.

To be clear: the presence of 'fd' indicates that 'path' now supports
qemu_open() semantics and libvirt will still only use 'path'?

Stefan

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