| Message ID | 20230511091527.46620-2-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/blkio: support 'fd' option for virtio-blk-vhost-vdpa driver | expand |
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. 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. As far as I can tell, versions 7.2.0 and 8.0.0 include this device but won't accept fds. Jonathon > > block/blkio.c | 53 ++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 44 insertions(+), 9 deletions(-) > > diff --git a/block/blkio.c b/block/blkio.c > index 0cdc99a729..6a6f20f923 100644 > --- a/block/blkio.c > +++ b/block/blkio.c > @@ -672,25 +672,60 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs, > { > const char *path = qdict_get_try_str(options, "path"); > BDRVBlkioState *s = bs->opaque; > - int ret; > + bool fd_supported = false; > + int fd, ret; > > if (!path) { > error_setg(errp, "missing 'path' option"); > return -EINVAL; > } > > - ret = blkio_set_str(s->blkio, "path", path); > - qdict_del(options, "path"); > - if (ret < 0) { > - error_setg_errno(errp, -ret, "failed to set path: %s", > - blkio_get_error_msg()); > - return ret; > - } > - > if (!(flags & BDRV_O_NOCACHE)) { > error_setg(errp, "cache.direct=off is not supported"); > return -EINVAL; > } > + > + if (blkio_get_int(s->blkio, "fd", &fd) == 0) { > + fd_supported = true; > + } > + > + /* > + * If the libblkio 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. > + */ > + if (fd_supported) { > + int open_flags; > + > + if (flags & BDRV_O_RDWR) { > + open_flags = O_RDWR; > + } else { > + open_flags = O_RDONLY; > + } > + > + fd = qemu_open(path, open_flags, errp); > + if (fd < 0) { > + return -EINVAL; > + } > + > + ret = blkio_set_int(s->blkio, "fd", fd); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "failed to set fd: %s", > + blkio_get_error_msg()); > + qemu_close(fd); > + return ret; > + } > + } else { > + ret = blkio_set_str(s->blkio, "path", path); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "failed to set path: %s", > + blkio_get_error_msg()); > + return ret; > + } > + } > + > + qdict_del(options, "path"); > + > return 0; > } >
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? >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? Thanks, Stefano
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. > >> 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? Thanks, Jonathon
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? Thanks, Stefano
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... Stefan
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. Thanks, Stefano
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
Stefan Hajnoczi <stefanha@redhat.com> writes: > 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: [...] >> > 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... Let me try to answer this without first reading the entire thread. QMP introspection lets you find out things like whether a command is there, or whether an an argument is there, and what its type is. Suffices most of the time. However, behavior can certainly change while the introspection data remains the same. When a management application needs to know about the change, we better expose the change in introspection somehow. The "obvious" way to do that would be some arbitrary change that *is* visible in introspection. Meh. The modern way is to add a suitable "feature". docs/devel/qapi-code-gen.rst: Features -------- Syntax:: FEATURES = [ FEATURE, ... ] FEATURE = STRING | { 'name': STRING, '*if': COND } Sometimes, the behaviour of QEMU changes compatibly, but without a change in the QMP syntax (usually by allowing values or operations that previously resulted in an error). QMP clients may still need to know whether the extension is available. For this purpose, a list of features can be specified for definitions, enumeration values, and struct members. Each feature list member can either be ``{ 'name': STRING, '*if': COND }``, or STRING, which is shorthand for ``{ 'name': STRING }``. The optional 'if' member specifies a conditional. See `Configuring the schema`_ below for more on this. Example:: { 'struct': 'TestType', 'data': { 'number': 'int' }, 'features': [ 'allow-negative-numbers' ] } The feature strings are exposed to clients in introspection, as explained in section `Client JSON Protocol introspection`_. Intended use is to have each feature string signal that this build of QEMU shows a certain behaviour. For a real example, see commit c6bdc312f30 (qapi: Add '@allow-write-only-overlay' feature for 'blockdev-snapshot'). Does this answer your question?
On 5/25/23 1:30 PM, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > >> 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: > > [...] > >>>> 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... > > Let me try to answer this without first reading the entire thread. > > QMP introspection lets you find out things like whether a command is > there, or whether an an argument is there, and what its type is. > Suffices most of the time. > > However, behavior can certainly change while the introspection data > remains the same. When a management application needs to know about the > change, we better expose the change in introspection somehow. > > The "obvious" way to do that would be some arbitrary change that *is* > visible in introspection. Meh. > > The modern way is to add a suitable "feature". > docs/devel/qapi-code-gen.rst: > > Features > -------- > > Syntax:: > > FEATURES = [ FEATURE, ... ] > FEATURE = STRING > | { 'name': STRING, '*if': COND } > > Sometimes, the behaviour of QEMU changes compatibly, but without a > change in the QMP syntax (usually by allowing values or operations > that previously resulted in an error). QMP clients may still need to > know whether the extension is available. > > For this purpose, a list of features can be specified for definitions, > enumeration values, and struct members. Each feature list member can > either be ``{ 'name': STRING, '*if': COND }``, or STRING, which is > shorthand for ``{ 'name': STRING }``. > > The optional 'if' member specifies a conditional. See `Configuring > the schema`_ below for more on this. > > Example:: > > { 'struct': 'TestType', > 'data': { 'number': 'int' }, > 'features': [ 'allow-negative-numbers' ] } > > The feature strings are exposed to clients in introspection, as > explained in section `Client JSON Protocol introspection`_. > > Intended use is to have each feature string signal that this build of > QEMU shows a certain behaviour. > > For a real example, see commit c6bdc312f30 (qapi: Add > '@allow-write-only-overlay' feature for 'blockdev-snapshot'). > > Does this answer your question? > That sounds perfect for libvirt's needs. Jonathon
On Thu, May 25, 2023 at 08:30:03PM +0200, Markus Armbruster wrote: >Stefan Hajnoczi <stefanha@redhat.com> writes: > >> 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: > >[...] > >>> > 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... > >Let me try to answer this without first reading the entire thread. > >QMP introspection lets you find out things like whether a command is >there, or whether an an argument is there, and what its type is. >Suffices most of the time. > >However, behavior can certainly change while the introspection data >remains the same. When a management application needs to know about the >change, we better expose the change in introspection somehow. > >The "obvious" way to do that would be some arbitrary change that *is* >visible in introspection. Meh. > >The modern way is to add a suitable "feature". >docs/devel/qapi-code-gen.rst: > > Features > -------- > > Syntax:: > > FEATURES = [ FEATURE, ... ] > FEATURE = STRING > | { 'name': STRING, '*if': COND } > > Sometimes, the behaviour of QEMU changes compatibly, but without a > change in the QMP syntax (usually by allowing values or operations > that previously resulted in an error). QMP clients may still need to > know whether the extension is available. > > For this purpose, a list of features can be specified for definitions, > enumeration values, and struct members. Each feature list member can > either be ``{ 'name': STRING, '*if': COND }``, or STRING, which is > shorthand for ``{ 'name': STRING }``. > > The optional 'if' member specifies a conditional. See `Configuring > the schema`_ below for more on this. > > Example:: > > { 'struct': 'TestType', > 'data': { 'number': 'int' }, > 'features': [ 'allow-negative-numbers' ] } > > The feature strings are exposed to clients in introspection, as > explained in section `Client JSON Protocol introspection`_. > > Intended use is to have each feature string signal that this build of > QEMU shows a certain behaviour. > >For a real example, see commit c6bdc312f30 (qapi: Add >'@allow-write-only-overlay' feature for 'blockdev-snapshot'). > >Does this answer your question? > Yep, this is exactly what we needed. Thank you very much! I'll send v4 adding `features` to the `path` member, and I'll check the version library in meson.build to enable the feature only when the vhost-vdpa driver in libblkio really supports it, so libvirt can be sure that if the feature is there, the fd passing will work. Stefano
diff --git a/block/blkio.c b/block/blkio.c index 0cdc99a729..6a6f20f923 100644 --- a/block/blkio.c +++ b/block/blkio.c @@ -672,25 +672,60 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs, { const char *path = qdict_get_try_str(options, "path"); BDRVBlkioState *s = bs->opaque; - int ret; + bool fd_supported = false; + int fd, ret; if (!path) { error_setg(errp, "missing 'path' option"); return -EINVAL; } - ret = blkio_set_str(s->blkio, "path", path); - qdict_del(options, "path"); - if (ret < 0) { - error_setg_errno(errp, -ret, "failed to set path: %s", - blkio_get_error_msg()); - return ret; - } - if (!(flags & BDRV_O_NOCACHE)) { error_setg(errp, "cache.direct=off is not supported"); return -EINVAL; } + + if (blkio_get_int(s->blkio, "fd", &fd) == 0) { + fd_supported = true; + } + + /* + * If the libblkio 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. + */ + if (fd_supported) { + int open_flags; + + if (flags & BDRV_O_RDWR) { + open_flags = O_RDWR; + } else { + open_flags = O_RDONLY; + } + + fd = qemu_open(path, open_flags, errp); + if (fd < 0) { + return -EINVAL; + } + + ret = blkio_set_int(s->blkio, "fd", fd); + if (ret < 0) { + error_setg_errno(errp, -ret, "failed to set fd: %s", + blkio_get_error_msg()); + qemu_close(fd); + return ret; + } + } else { + ret = blkio_set_str(s->blkio, "path", path); + if (ret < 0) { + error_setg_errno(errp, -ret, "failed to set path: %s", + blkio_get_error_msg()); + return ret; + } + } + + qdict_del(options, "path"); + return 0; }
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] block/blkio.c | 53 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 9 deletions(-)