Stefano Garzarella
2023-Jan-16 14:46 UTC
[Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Mon, Jan 16, 2023 at 03:39:29PM +0100, Stefano Garzarella wrote:>On Wed, Jan 11, 2023 at 04:49:14PM +0000, Richard W.M. Jones wrote: >>On Wed, Jan 11, 2023 at 11:34:50AM -0500, Stefan Hajnoczi wrote: >>>On Sat, Jan 07, 2023 at 07:44:37PM +0000, Richard W.M. Jones wrote: >>>> >>>> This is upstream in nbdkit now: >>>> https://gitlab.com/nbdkit/nbdkit/-/tree/master/plugins/blkio >>>> >>>> Another question: >>>> >>>> (6) vhost-user + "read-only" property acts a bit strangely. After >>>> opening virtio-blk-vhost-user it throws an EROFS error if you try to >>>> "blkio_start" it. However if you set the "read-only" property to true >>>> then it's OK to start it, and subsequent read operations work too. >>>> >>>> I would expect that any device can be started without needing to set >>>> this property, and in general I don't understand why vhost-user is r/o >>>> since everything I read about it seems to indicate it's a general >>>> purpose writable protocol. >>> >>>I guess the vhost-user-blk server is read-only and that's why it's >>>refusing to blkio_start() unless readonly was set to true. >>> >>>If you're using qemu-storage-daemon, set the writable=on parameter on >>>the export. >> >>So it turned out to be the case. Alberto pointed out that unless you >>set writable=on explicitly, q-s-d makes the export read-only. This >>was the reverse of what I expected. But it is covered in the q-s-d >>man page. >> >>But about libblkio: The problem is basically you must know if the >>export is read-only or not before you connect. For a readonly >>vhost-user export this is the observed behaviour: >> >> blkio_create ==> OK >> blkio_get_bool ("read-only") ==> returns false >> blkio_connect ==> OK >> blkio_get_bool ("read-only") ==> returns false >> blkio_start ==> fails "Device is read-only" >> >>At least after connecting it seems we should be able to find out that >>the export is read-only. > >Yes, indeed IIUC we could take this information, but currently >"read-only" property for virtio-blk is not filled by querying the >device. > >At this point I wonder if for virtio-blk devices we should provide the >"read-only" property not writable (sorry for the confusion...) so that >it reflects whether the device is read-only or not. > >Otherwise we could also leave it writable, but surely after the >connect we would have to set it with the value exported from the >device and allow the user to change it before the start (I think it's >a bit different from what we do in io-uring, or maybe it is comparable >when we use "fd" instead of "path" in the io-uring driver).I just checked, and IIUC when we use the io-uring driver and use the "fd" property instead of "path" to specify the backend, the "read-only" property is ignored if set by the user and overwritten (if the fd was opened read-only). So I think for virtio-blk we should do the same. (I'll prepare a MR and maybe we can discuss there). Rich thanks for bringing it up! Stefano
Stefano Garzarella
2023-Jan-16 15:45 UTC
[Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Mon, Jan 16, 2023 at 3:46 PM Stefano Garzarella <sgarzare at redhat.com> wrote:> On Mon, Jan 16, 2023 at 03:39:29PM +0100, Stefano Garzarella wrote: > >On Wed, Jan 11, 2023 at 04:49:14PM +0000, Richard W.M. Jones wrote: > >>On Wed, Jan 11, 2023 at 11:34:50AM -0500, Stefan Hajnoczi wrote: > >>>On Sat, Jan 07, 2023 at 07:44:37PM +0000, Richard W.M. Jones wrote: > >>>> > >>>> This is upstream in nbdkit now: > >>>> https://gitlab.com/nbdkit/nbdkit/-/tree/master/plugins/blkio > >>>> > >>>> Another question: > >>>> > >>>> (6) vhost-user + "read-only" property acts a bit strangely. After > >>>> opening virtio-blk-vhost-user it throws an EROFS error if you try to > >>>> "blkio_start" it. However if you set the "read-only" property to true > >>>> then it's OK to start it, and subsequent read operations work too. > >>>> > >>>> I would expect that any device can be started without needing to set > >>>> this property, and in general I don't understand why vhost-user is r/o > >>>> since everything I read about it seems to indicate it's a general > >>>> purpose writable protocol. > >>> > >>>I guess the vhost-user-blk server is read-only and that's why it's > >>>refusing to blkio_start() unless readonly was set to true. > >>> > >>>If you're using qemu-storage-daemon, set the writable=on parameter on > >>>the export. > >> > >>So it turned out to be the case. Alberto pointed out that unless you > >>set writable=on explicitly, q-s-d makes the export read-only. This > >>was the reverse of what I expected. But it is covered in the q-s-d > >>man page. > >> > >>But about libblkio: The problem is basically you must know if the > >>export is read-only or not before you connect. For a readonly > >>vhost-user export this is the observed behaviour: > >> > >> blkio_create ==> OK > >> blkio_get_bool ("read-only") ==> returns false > >> blkio_connect ==> OK > >> blkio_get_bool ("read-only") ==> returns false > >> blkio_start ==> fails "Device is read-only" > >> > >>At least after connecting it seems we should be able to find out that > >>the export is read-only. > > > >Yes, indeed IIUC we could take this information, but currently > >"read-only" property for virtio-blk is not filled by querying the > >device. > > > >At this point I wonder if for virtio-blk devices we should provide the > >"read-only" property not writable (sorry for the confusion...) so that > >it reflects whether the device is read-only or not. > > > >Otherwise we could also leave it writable, but surely after the > >connect we would have to set it with the value exported from the > >device and allow the user to change it before the start (I think it's > >a bit different from what we do in io-uring, or maybe it is comparable > >when we use "fd" instead of "path" in the io-uring driver). > > I just checked, and IIUC when we use the io-uring driver and use the > "fd" property instead of "path" to specify the backend, the "read-only" > property is ignored if set by the user and overwritten (if the fd was > opened read-only). > > So I think for virtio-blk we should do the same. (I'll prepare a MR and > maybe we can discuss there).MR posted: https://gitlab.com/libblkio/libblkio/-/merge_requests/161 Thanks, Stefano