Libguestfs - Jan 2023 - [PATCH nbdkit] New plugin: blkio [incomplete]

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).

Thanks,
Stefano

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