| Message ID | 20230724154611.178858-2-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/blkio: fix opening virtio-blk drivers | expand |
On Mon, Jul 24, 2023 at 05:46:10PM +0200, Stefano Garzarella wrote: > libblkio 1.3.0 added support of "fd" property for virtio-blk-vhost-vdpa > driver. In QEMU, starting from commit cad2ccc395 ("block/blkio: use > qemu_open() to support fd passing for virtio-blk") we are using > `blkio_get_int(..., "fd")` to check if the "fd" property is supported > for all the virtio-blk-* driver. > > Unfortunately that property is also available for those driver that do > not support it, such as virtio-blk-vhost-user. Indeed now QEMU is > failing if used with virtio-blk-vhost-user in this way: > > -blockdev node-name=drive0,driver=virtio-blk-vhost-user,path=vhost-user-blk.sock,cache.direct=on: Could not open 'vhost-user-blk.sock': No such device or address > > So, `blkio_get_int()` is not enough to check whether the driver supports > the `fd` property or not. This is because the virito-blk common libblkio > driver only checks whether or not `fd` is set during `blkio_connect()` > and fails for those transports that do not support it (all except > vhost-vdpa for now). > > So for now let's also check that the driver is virtio-blk-vhost-vdpa, > since that's the only one that supports it. What happens when more virtio-blk-* libblkio drivers gain support for `fd`? I think we'll be back to the same problem because QEMU will be unable to distinguish between old and new libraries. How about retrying with `path` if opening with `fd` fails? > > Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk") > Reported-by: Qing Wang <qinwang@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > block/blkio.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/block/blkio.c b/block/blkio.c > index 1798648134..ca1149042a 100644 > --- a/block/blkio.c > +++ b/block/blkio.c > @@ -662,6 +662,7 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs, > QDict *options, int flags, Error **errp) > { > const char *path = qdict_get_try_str(options, "path"); > + const char *blkio_driver = bs->drv->protocol_name; > BDRVBlkioState *s = bs->opaque; > bool fd_supported = false; > int fd, ret; > @@ -676,7 +677,8 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs, > return -EINVAL; > } > > - if (blkio_get_int(s->blkio, "fd", &fd) == 0) { > + if (strcmp(blkio_driver, "virtio-blk-vhost-vdpa") == 0 && > + blkio_get_int(s->blkio, "fd", &fd) == 0) { > fd_supported = true; > } > > -- > 2.41.0 >
On Tue, Jul 25, 2023 at 04:00:38PM -0400, Stefan Hajnoczi wrote: >On Mon, Jul 24, 2023 at 05:46:10PM +0200, Stefano Garzarella wrote: >> libblkio 1.3.0 added support of "fd" property for virtio-blk-vhost-vdpa >> driver. In QEMU, starting from commit cad2ccc395 ("block/blkio: use >> qemu_open() to support fd passing for virtio-blk") we are using >> `blkio_get_int(..., "fd")` to check if the "fd" property is supported >> for all the virtio-blk-* driver. >> >> Unfortunately that property is also available for those driver that do >> not support it, such as virtio-blk-vhost-user. Indeed now QEMU is >> failing if used with virtio-blk-vhost-user in this way: >> >> -blockdev node-name=drive0,driver=virtio-blk-vhost-user,path=vhost-user-blk.sock,cache.direct=on: Could not open 'vhost-user-blk.sock': No such device or address >> >> So, `blkio_get_int()` is not enough to check whether the driver supports >> the `fd` property or not. This is because the virito-blk common libblkio >> driver only checks whether or not `fd` is set during `blkio_connect()` >> and fails for those transports that do not support it (all except >> vhost-vdpa for now). >> >> So for now let's also check that the driver is virtio-blk-vhost-vdpa, >> since that's the only one that supports it. > >What happens when more virtio-blk-* libblkio drivers gain support for >`fd`? I think we'll be back to the same problem because QEMU will be >unable to distinguish between old and new libraries. If we release a v1.3.1 version of libblkio with https://gitlab.com/libblkio/libblkio/-/merge_requests/208 we can set a minimum requirement in QEMU and use blkio_set_fd() here. > >How about retrying with `path` if opening with `fd` fails? IIUC the only way is to check if blkio_connect() will fail with -EINVAL, that can also be generated by other issues, then retry forcing `path`. Do you see other ways? The code wouldn't be great, but we could do it for now and then when we release a new version of libblkio, do the revert and use blkio_set_int(fd) to see if it's supported or not. I don't know if it is worth it, or if it is better to merge this, release libblkio v1.3.1 and force the minimum requirement. WDYT? Thanks, Stefano
On Wed, Jul 26, 2023 at 09:26:45AM +0200, Stefano Garzarella wrote: > On Tue, Jul 25, 2023 at 04:00:38PM -0400, Stefan Hajnoczi wrote: > > On Mon, Jul 24, 2023 at 05:46:10PM +0200, Stefano Garzarella wrote: > > > libblkio 1.3.0 added support of "fd" property for virtio-blk-vhost-vdpa > > > driver. In QEMU, starting from commit cad2ccc395 ("block/blkio: use > > > qemu_open() to support fd passing for virtio-blk") we are using > > > `blkio_get_int(..., "fd")` to check if the "fd" property is supported > > > for all the virtio-blk-* driver. > > > > > > Unfortunately that property is also available for those driver that do > > > not support it, such as virtio-blk-vhost-user. Indeed now QEMU is > > > failing if used with virtio-blk-vhost-user in this way: > > > > > > -blockdev node-name=drive0,driver=virtio-blk-vhost-user,path=vhost-user-blk.sock,cache.direct=on: Could not open 'vhost-user-blk.sock': No such device or address > > > > > > So, `blkio_get_int()` is not enough to check whether the driver supports > > > the `fd` property or not. This is because the virito-blk common libblkio > > > driver only checks whether or not `fd` is set during `blkio_connect()` > > > and fails for those transports that do not support it (all except > > > vhost-vdpa for now). > > > > > > So for now let's also check that the driver is virtio-blk-vhost-vdpa, > > > since that's the only one that supports it. > > > > What happens when more virtio-blk-* libblkio drivers gain support for > > `fd`? I think we'll be back to the same problem because QEMU will be > > unable to distinguish between old and new libraries. > > If we release a v1.3.1 version of libblkio with > https://gitlab.com/libblkio/libblkio/-/merge_requests/208 > we can set a minimum requirement in QEMU and use blkio_set_fd() here. > > > > > How about retrying with `path` if opening with `fd` fails? > > IIUC the only way is to check if blkio_connect() will fail with -EINVAL, > that can also be generated by other issues, then retry forcing `path`. > > Do you see other ways? No. Checking for -EINVAL and then retrying with `path` is what I had in mind. > The code wouldn't be great, but we could do it for now and then when > we release a new version of libblkio, do the revert and use > blkio_set_int(fd) to see if it's supported or not. > > I don't know if it is worth it, or if it is better to merge this, > release libblkio v1.3.1 and force the minimum requirement. > > WDYT? I prefer retrying on -EINVAL because even if libblkio 1.3.1 is released today, we don't have control over when it becomes available in distros. Stefan
On Wed, Jul 26, 2023 at 11:32:10AM -0400, Stefan Hajnoczi wrote: >On Wed, Jul 26, 2023 at 09:26:45AM +0200, Stefano Garzarella wrote: >> On Tue, Jul 25, 2023 at 04:00:38PM -0400, Stefan Hajnoczi wrote: >> > On Mon, Jul 24, 2023 at 05:46:10PM +0200, Stefano Garzarella wrote: >> > > libblkio 1.3.0 added support of "fd" property for virtio-blk-vhost-vdpa >> > > driver. In QEMU, starting from commit cad2ccc395 ("block/blkio: use >> > > qemu_open() to support fd passing for virtio-blk") we are using >> > > `blkio_get_int(..., "fd")` to check if the "fd" property is supported >> > > for all the virtio-blk-* driver. >> > > >> > > Unfortunately that property is also available for those driver that do >> > > not support it, such as virtio-blk-vhost-user. Indeed now QEMU is >> > > failing if used with virtio-blk-vhost-user in this way: >> > > >> > > -blockdev node-name=drive0,driver=virtio-blk-vhost-user,path=vhost-user-blk.sock,cache.direct=on: Could not open 'vhost-user-blk.sock': No such device or address >> > > >> > > So, `blkio_get_int()` is not enough to check whether the driver supports >> > > the `fd` property or not. This is because the virito-blk common libblkio >> > > driver only checks whether or not `fd` is set during `blkio_connect()` >> > > and fails for those transports that do not support it (all except >> > > vhost-vdpa for now). >> > > >> > > So for now let's also check that the driver is virtio-blk-vhost-vdpa, >> > > since that's the only one that supports it. >> > >> > What happens when more virtio-blk-* libblkio drivers gain support for >> > `fd`? I think we'll be back to the same problem because QEMU will be >> > unable to distinguish between old and new libraries. >> >> If we release a v1.3.1 version of libblkio with >> https://gitlab.com/libblkio/libblkio/-/merge_requests/208 >> we can set a minimum requirement in QEMU and use blkio_set_fd() here. >> >> > >> > How about retrying with `path` if opening with `fd` fails? >> >> IIUC the only way is to check if blkio_connect() will fail with -EINVAL, >> that can also be generated by other issues, then retry forcing `path`. >> >> Do you see other ways? > >No. Checking for -EINVAL and then retrying with `path` is what I had in >mind. > >> The code wouldn't be great, but we could do it for now and then when >> we release a new version of libblkio, do the revert and use >> blkio_set_int(fd) to see if it's supported or not. >> >> I don't know if it is worth it, or if it is better to merge this, >> release libblkio v1.3.1 and force the minimum requirement. >> >> WDYT? > >I prefer retrying on -EINVAL because even if libblkio 1.3.1 is released >today, we don't have control over when it becomes available in distros. Fair point! I'll send v2 using that approach! Thanks, Stefano
diff --git a/block/blkio.c b/block/blkio.c index 1798648134..ca1149042a 100644 --- a/block/blkio.c +++ b/block/blkio.c @@ -662,6 +662,7 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { const char *path = qdict_get_try_str(options, "path"); + const char *blkio_driver = bs->drv->protocol_name; BDRVBlkioState *s = bs->opaque; bool fd_supported = false; int fd, ret; @@ -676,7 +677,8 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs, return -EINVAL; } - if (blkio_get_int(s->blkio, "fd", &fd) == 0) { + if (strcmp(blkio_driver, "virtio-blk-vhost-vdpa") == 0 && + blkio_get_int(s->blkio, "fd", &fd) == 0) { fd_supported = true; }
libblkio 1.3.0 added support of "fd" property for virtio-blk-vhost-vdpa driver. In QEMU, starting from commit cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk") we are using `blkio_get_int(..., "fd")` to check if the "fd" property is supported for all the virtio-blk-* driver. Unfortunately that property is also available for those driver that do not support it, such as virtio-blk-vhost-user. Indeed now QEMU is failing if used with virtio-blk-vhost-user in this way: -blockdev node-name=drive0,driver=virtio-blk-vhost-user,path=vhost-user-blk.sock,cache.direct=on: Could not open 'vhost-user-blk.sock': No such device or address So, `blkio_get_int()` is not enough to check whether the driver supports the `fd` property or not. This is because the virito-blk common libblkio driver only checks whether or not `fd` is set during `blkio_connect()` and fails for those transports that do not support it (all except vhost-vdpa for now). So for now let's also check that the driver is virtio-blk-vhost-vdpa, since that's the only one that supports it. Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk") Reported-by: Qing Wang <qinwang@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- block/blkio.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)