Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Mon, Jan 16, 2023 at 02:35:41PM -0600, Eric Blake wrote: > On Wed, Jan 04, 2023 at 10:51:50PM +, Richard W.M. Jones wrote: > > Here's an updated version which supports read, write, flush, > > write-zeroes, discard and FUA: > > > > https://gitlab.com/rwmjones/nbdkit/-/commits/2023-libblkio/ > ... > > > PS: Eric: One thing we lack in the NBD space is a standards > > conformance test [-suite]. > > To some extent, interoperability between libnbd and nbdkit provides a > lot of that testing, but I agree that it is somewhat ad hoc. I've > added features to libnbd over the years to try and trigger more corner > cases of client behavior to see how servers respond, but that's more > in the handshaking side of things than in the I/O side after > handshaking is complete. What specific tests do you have in mind here? I think there are two overlapping but slightly different things we don't have: (1) A set of sanity checks for a server, which would check things like: After writing, can the same data be read back? Does the zeroing operation actually write zeroes? (2) An actual conformance suite which would be something like the Java TCK. It would attempt to check every corner of the NBD standard, being incredibly picky about everything. You could argue that (1) is just a subset of (2). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 04, 2023 at 10:51:50PM +, Richard W.M. Jones wrote: > Here's an updated version which supports read, write, flush, > write-zeroes, discard and FUA: > > https://gitlab.com/rwmjones/nbdkit/-/commits/2023-libblkio/ ... > PS: Eric: One thing we lack in the NBD space is a standards > conformance test [-suite]. To some extent, interoperability between libnbd and nbdkit provides a lot of that testing, but I agree that it is somewhat ad hoc. I've added features to libnbd over the years to try and trigger more corner cases of client behavior to see how servers respond, but that's more in the handshaking side of things than in the I/O side after handshaking is complete. What specific tests do you have in mind here? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Mon, Jan 16, 2023 at 04:45:28PM +0100, Stefano Garzarella wrote:
> On Mon, Jan 16, 2023 at 3:46 PM Stefano Garzarella
> wrote:
> > On Mon, Jan 16, 2023 at 03:39:29PM +0100, Stefano Garzarella wrote:
> > >On Wed, Jan 11, 2023 at 04:49:14PM +, 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 +, 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
Great, thanks for doing this. I will review the MR.
Stefan
signature.asc
Description: PGP signature
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Mon, Jan 16, 2023 at 3:46 PM Stefano Garzarella wrote:
> On Mon, Jan 16, 2023 at 03:39:29PM +0100, Stefano Garzarella wrote:
> >On Wed, Jan 11, 2023 at 04:49:14PM +, 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 +, 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [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 +, 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 +, 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 11, 2023 at 04:49:14PM +, 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 +, 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 11, 2023 at 10:52:01AM -0500, Stefan Hajnoczi wrote: > On Wed, Jan 04, 2023 at 06:14:34PM +, Richard W.M. Jones wrote: > > (1) There is no way to know which properties are readable, writable, > > and those which need to be set before or after blkio_connect (see > > is_preconnect_property in the plugin). It should be possible to > > introspect this information. Also might be nice to be able read a > > list of all available properties. > > libblkio doesn't support property introspection. The documentation > covers when each property can be read/written (e.g. read/write before > started and read-only after started). > > Why is introspection needed? (Answered more fully here: https://gitlab.com/libblkio/libblkio/-/issues/56 ) > > (2) It would be nice if libblkio had a way to enable debugging and > > call the caller back for debug messages. We could then redirect the > > callbacks into the nbdkit debug API (nbdkit_debug()) where they would > > eventually end up on stderr or syslog. > > > > However don't send debug messages to stderr, or at least allow that > > behaviour to be overridden. > > libblkio has no debug messages. What stderr output did you get? So in fact I didn't see any debug. The question was just speculation in the end. However Alberto filed this: https://gitlab.com/libblkio/libblkio/-/issues/57 [...] > > (4b) It's unclear how completions work. If I set min=max=1, will it > >return after the whole operation has completed? Do I need to > >call it again? What about if the request is very large, can it > >get split? > > > > Reading the example > > https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c > > it appears that requests cannot ever be split? > > min=max=1 blocks until exactly 1 completion occurs. BTW I checked the > documentation to see if min_completions/max_completions are covered and > I'm not sure how to improve the documentation? > > There is no need to call it again if you got the completion you were > waiting for. I guess the issue was whether a request (command?) might be split. For example if I requested reading far more than the device could handle in one go, would the command be rejected or would it be split into smaller requests, and hence into several completions (maybe?). I think the answer is that the request would be rejected, based on: > The caller must honor the constraints given by the "max-segments", > "max-segment-len", and "max-transfer" properties. Requests are not > split. OK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 11, 2023 at 11:34:50AM -0500, Stefan Hajnoczi wrote:
> On Sat, Jan 07, 2023 at 07:44:37PM +, 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.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Sat, Jan 07, 2023 at 07:44:37PM +, 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. Stefan signature.asc Description: PGP signature ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 04, 2023 at 09:43:57PM +, Richard W.M. Jones wrote: > On Wed, Jan 04, 2023 at 06:14:34PM +, Richard W.M. Jones wrote: > > (3) It seems like some drivers require pre-allocated memory regions, > > and since some do that means we might as well implement this. It > > also seems like some drivers require file-backed pre-allocated > > memory regions, and so we might as well implement that too. > > > > However what is not clear: does memfd_create(2) always allocate > > sufficiently aligned memory regions, such that we never need to bother > > reading the mem-region-alignment property? > > > > I notice that the example: > > https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c > > just passes on this and calls blkio_alloc_mem_region(). Is that the > > safest and easiest thing to do which will always work? > > So this seems to be the reverse of what I thought. > > In nbdkit plugins ideally we'd like to avoid using bounce buffers if > possible. Also NBD requests can be up to (IIRC) 32M, although we can > hint to the client to limit them to some smaller number. > > If I call blkio_alloc_mem_region() unconditionally then it seems as if > I always need to use this as a bounce buffer. Plus, is 32M too large > for a memory region allocated this way? The example allocates 128K. Allocating 32 MB with blkio_alloc_mem_region() is fine. > > It seems like for drivers which _don't_ require pre-allocated memory > regions then we should avoid calling blkio_alloc_mem_region which > would avoid bounce buffers. > > Some guidance about this would be appreciated. The most efficient approach is to use blkio_alloc_mem_region() in the core of your code so that no memory copies are necessary. If that's not possible (e.g. the network code cannot easily use the blkio buffer for NBD payloads), then a memory copy will be necessary. Stefan signature.asc Description: PGP signature ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 04, 2023 at 07:01:24PM +, Richard W.M. Jones wrote: > (5) "The application is responsible for thread-safety. No thread > synchronization is necessary when a queue is only used from a single > thread. Proper synchronization is required when sharing a queue > between multiple threads." > > Does this apply across multiple struct blkio handles? ie. Is there > now, or could there be in future, some global state which would be > corrupted by parallel calls across multiple handles? > > This matters because we could use NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS > instead of NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS > (https://libguestfs.org/nbdkit-plugin.3.html#Threads). There is no global state. If there is global state in the future then the user would be aware of it (e.g. they would have to explicitly create "shared" blkio instances). Stefan signature.asc Description: PGP signature ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 04, 2023 at 06:14:34PM +, Richard W.M. Jones wrote:
> This is an incomplete outline implementation for a libblkio plugin for
> nbdkit. At the moment it only supports reading the same ("capacity")
> of the device, and not even reading or writing. I have some questions
> about the libblkio API before I can complete the plugin (see below).
>
> The idea here is that by connecting libblkio to NBD we can use the
> existing set of scripting tools to script access to devices. For
> example you could use Python to read or modify a device:
>
> --
> $ nbdsh -c - <<'EOF'
>
> from subprocess import *
>
> # Run nbdkit-blkio-plugin.
> h.connect_systemd_socket_activation ("nbdkit" "blkio",
> "virtio-blk-vhost-user",
> "path=unix.sock")
>
> print("device size=%d", h.get_size())
>
> # Dump the boot sector.
> bootsect = h.pread(512, 0)
> p = Popen("hexdump -C", shell=True, stdin=PIPE)
> p.stdin.write(bootsect)
> EOF
> --
>
> So my questions and comments about libblkio:
Hi Rich,
This is cool! I've answered below:
>
> (1) There is no way to know which properties are readable, writable,
> and those which need to be set before or after blkio_connect (see
> is_preconnect_property in the plugin). It should be possible to
> introspect this information. Also might be nice to be able read a
> list of all available properties.
libblkio doesn't support property introspection. The documentation
covers when each property can be read/written (e.g. read/write before
started and read-only after started).
Why is introspection needed?
> (2) It would be nice if libblkio had a way to enable debugging and
> call the caller back for debug messages. We could then redirect the
> callbacks into the nbdkit debug API (nbdkit_debug()) where they would
> eventually end up on stderr or syslog.
>
> However don't send debug messages to stderr, or at least allow that
> behaviour to be overridden.
libblkio has no debug messages. What stderr output did you get?
Errors are returned via blkio_get_error_msg().
> (3) It seems like some drivers require pre-allocated memory regions,
> and since some do that means we might as well implement this. It
> also seems like some drivers require file-backed pre-allocated
> memory regions, and so we might as well implement that too.
>
> However what is not clear: does memfd_create(2) always allocate
> sufficiently aligned memory regions, such that we never need to bother
> reading the mem-region-alignment property?
>
> I notice that the example:
> https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c
> just passes on this and calls blkio_alloc_mem_region(). Is that the
> safest and easiest thing to do which will always work?
Yes, blkio_alloc_mem_region() is the safest and easiest way to allocate
correctly-aligned memory.
Regarding memfd_create(2), I don't believe the memory has any alignment
until it's mmapped and then it will be page-aligned. Normally page
alignment is sufficient for "buf-alignment" and "mem-region-alignment",
but in theory a loop could be used to increase the size of the memfd to
achieve any required alignment. That's why blkio_alloc_mem_region() is
exists as an easy API that most people will use instead of allocating
memory regions on their own.
>
> (4) As a first pass, I only want to bother implementing blocking mode.
> It'll be slow, but it doesn't matter for this application. Also I've
> chosen nbdkit's NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS so nbdkit
> will serialise all requests (again, only for a very simple first pass).
>
> Looking at: https://libblkio.gitlab.io/libblkio/blkio.html#performing-i-o
> seems simple enough but:
>
> (4a) What is the queue number? Always 0? Is it affected by num-entries?
Yes, the first queue is 0. "num-queues" sets the number of queues.
"num-entries" is a different property that some drivers have for sizing
rings.
> (4b) It's unclear how completions work. If I set min=max=1, will it
>return after the whole operation has completed? Do I need to
>call it again? What about if the request is very large, can it
>get split?
>
> Reading the example
> https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c
> it appears that requests cannot ever be split?
min=max=1 blocks until exactly 1 completion occurs. BTW I checked the
documentation to see if min_completions/max_completions are covered and
I'm not sure how to improve the documentation?
There is no need to call it again if you got the completion you were
waiting for.
The caller must honor the constraints given by the "max-segments",
"max-segment-len", and "max-transfer" properties. Requests are not
split.
Stefan
signature.asc
Description: PGP signature
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Sun, Jan 08, 2023 at 08:35:35PM +, Alberto Faria wrote: > With qemu-storage-daemon, you must set writable=on on the > vhost-user-blk export for the device to be writable. Ah, this was the thing I was missing, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
> On Wed, Jan 04, 2023 at 06:14:34PM +, Richard W.M. Jones wrote: > > (3) It seems like some drivers require pre-allocated memory regions, > > and since some do that means we might as well implement this. It > > also seems like some drivers require file-backed pre-allocated > > memory regions, and so we might as well implement that too. > > > > However what is not clear: does memfd_create(2) always allocate > > sufficiently aligned memory regions, such that we never need to bother > > reading the mem-region-alignment property? > > > > I notice that the example: > > https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c > > just passes on this and calls blkio_alloc_mem_region(). Is that the > > safest and easiest thing to do which will always work? > > So this seems to be the reverse of what I thought. > > In nbdkit plugins ideally we'd like to avoid using bounce buffers if > possible. Also NBD requests can be up to (IIRC) 32M, although we can > hint to the client to limit them to some smaller number. Since you're currently serializing requests, you may be able to manually break up big requests into smaller requests that don't exceed max-transfer, and then wait for the right number of completions with blkioq_do_io(). > If I call blkio_alloc_mem_region() unconditionally then it seems as if > I always need to use this as a bounce buffer. Plus, is 32M too large > for a memory region allocated this way? The example allocates 128K. There is no general limit to the size of memory regions. 32M should always be fine. In particular, using different parts of a single big memory region for many concurrent requests is a reasonable use case. > It seems like for drivers which _don't_ require pre-allocated memory > regions then we should avoid calling blkio_alloc_mem_region which > would avoid bounce buffers. > > Some guidance about this would be appreciated. A possible approach is to make nbdkit always allocate page-aligned, file-backed buffers, and attempt to blkio_map_mem_region() them at init time in the libblkio plugin. Note that mapping memory regions even for drivers that don't strictly need them can still improve performance. If this fails and the driver requires mapped memory regions, then one could fall back to bounce buffering with buffers allocated by blkio_alloc_mem_region(). But this probably won't happen in practice, so just failing in this case is probably fine. Alberto ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
> This is an incomplete outline implementation for a libblkio plugin for
> nbdkit. At the moment it only supports reading the same ("capacity")
> of the device, and not even reading or writing. I have some questions
> about the libblkio API before I can complete the plugin (see below).
>
> The idea here is that by connecting libblkio to NBD we can use the
> existing set of scripting tools to script access to devices. For
> example you could use Python to read or modify a device:
>
> --
> $ nbdsh -c - <<'EOF'
>
> from subprocess import *
>
> # Run nbdkit-blkio-plugin.
> h.connect_systemd_socket_activation ("nbdkit" "blkio",
> "virtio-blk-vhost-user",
> "path=unix.sock")
>
> print("device size=%d", h.get_size())
>
> # Dump the boot sector.
> bootsect = h.pread(512, 0)
> p = Popen("hexdump -C", shell=True, stdin=PIPE)
> p.stdin.write(bootsect)
> EOF
> --
>
> So my questions and comments about libblkio:
>
> (1) There is no way to know which properties are readable, writable,
> and those which need to be set before or after blkio_connect (see
> is_preconnect_property in the plugin). It should be possible to
> introspect this information. Also might be nice to be able read a
> list of all available properties.
>
> (2) It would be nice if libblkio had a way to enable debugging and
> call the caller back for debug messages. We could then redirect the
> callbacks into the nbdkit debug API (nbdkit_debug()) where they would
> eventually end up on stderr or syslog.
Thanks, I created a couple issues for these features.
> However don't send debug messages to stderr, or at least allow that
> behaviour to be overridden.
>
> (3) It seems like some drivers require pre-allocated memory regions,
> and since some do that means we might as well implement this. It
> also seems like some drivers require file-backed pre-allocated
> memory regions, and so we might as well implement that too.
>
> However what is not clear: does memfd_create(2) always allocate
> sufficiently aligned memory regions, such that we never need to bother
> reading the mem-region-alignment property?
mem-region-alignment is currently allowed to exceed the page size, but
I'm not sure if it ever could in practice. (Maybe it could due to
IOMMU alignment restrictions?)
> I notice that the example:
> https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c
> just passes on this and calls blkio_alloc_mem_region(). Is that the
> safest and easiest thing to do which will always work?
Yes.
> (4) As a first pass, I only want to bother implementing blocking mode.
> It'll be slow, but it doesn't matter for this application. Also I've
> chosen nbdkit's NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS so nbdkit
> will serialise all requests (again, only for a very simple first pass).
>
> Looking at: https://libblkio.gitlab.io/libblkio/blkio.html#performing-i-o
> seems simple enough but:
>
> (4a) What is the queue number? Always 0? Is it affected by num-entries?
With n = num-queues, (non-poll) queues have indices 0 through n-1.
num-queues is 1 by default.
num-entries is the size of each of those queues for io_uring drivers,
and doesn't affect their index.
> (4b) It's unclear how completions work. If I set min=max=1, will it
>return after the whole operation has completed? Do I need to
>call it again? What about if the request is very large, can it
>get split?
blkioq_do_io() will only return after at least min completions have
been copied out into the user's completion array. (Unless it fails, in
which case it returns an error and no completions are copied
out/consumed.)
With min=max=1, blkioq_do_io() will wait until at least one completion
is available, copy it out, and return 1. It will never return 0.
> Reading the example
> https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c
> it appears that requests cannot ever be split?
Requests are never split (by libblkio). If read/write requests are
larger than max-transfer, they fail.
Alberto
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
> 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. read-only has POSIX file open-like semantics: if the device is itself read only but the read-only property is false, blkio_start() fails. With qemu-storage-daemon, you must set writable=on on the vhost-user-blk export for the device to be writable. Alberto ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
> (5) "The application is responsible for thread-safety. No thread > synchronization is necessary when a queue is only used from a single > thread. Proper synchronization is required when sharing a queue > between multiple threads." > > Does this apply across multiple struct blkio handles? ie. Is there > now, or could there be in future, some global state which would be > corrupted by parallel calls across multiple handles? There will never be any need to synchronize accesses to separate blkio instances. We will clarify this in the docs. Alberto > This matters because we could use NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS > instead of NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS > (https://libguestfs.org/nbdkit-plugin.3.html#Threads). ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
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. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
Here's an updated version which supports read, write, flush, write-zeroes, discard and FUA: https://gitlab.com/rwmjones/nbdkit/-/commits/2023-libblkio/ I was able to access a local file using the io_uring driver using guestfish. You can try it like this: $ ./nbdkit -fv blkio io_uring path=/var/tmp/fedora-36.img \ --run ' guestfish --format=raw -a $uri -i ' I haven't tested it exhaustively, but it seems to work OK. Obviously it will be a bit slow. It doesn't implement pre-allocated buffers, so advice on how and when to do that would be welcome. Rich. PS: Eric: One thing we lack in the NBD space is a standards conformance test [-suite]. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 04, 2023 at 06:14:34PM +, Richard W.M. Jones wrote: > (3) It seems like some drivers require pre-allocated memory regions, > and since some do that means we might as well implement this. It > also seems like some drivers require file-backed pre-allocated > memory regions, and so we might as well implement that too. > > However what is not clear: does memfd_create(2) always allocate > sufficiently aligned memory regions, such that we never need to bother > reading the mem-region-alignment property? > > I notice that the example: > https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c > just passes on this and calls blkio_alloc_mem_region(). Is that the > safest and easiest thing to do which will always work? So this seems to be the reverse of what I thought. In nbdkit plugins ideally we'd like to avoid using bounce buffers if possible. Also NBD requests can be up to (IIRC) 32M, although we can hint to the client to limit them to some smaller number. If I call blkio_alloc_mem_region() unconditionally then it seems as if I always need to use this as a bounce buffer. Plus, is 32M too large for a memory region allocated this way? The example allocates 128K. It seems like for drivers which _don't_ require pre-allocated memory regions then we should avoid calling blkio_alloc_mem_region which would avoid bounce buffers. Some guidance about this would be appreciated. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
(5) "The application is responsible for thread-safety. No thread synchronization is necessary when a queue is only used from a single thread. Proper synchronization is required when sharing a queue between multiple threads." Does this apply across multiple struct blkio handles? ie. Is there now, or could there be in future, some global state which would be corrupted by parallel calls across multiple handles? This matters because we could use NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS instead of NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS (https://libguestfs.org/nbdkit-plugin.3.html#Threads). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
On Wed, Jan 04, 2023 at 06:14:34PM +, Richard W.M. Jones wrote:
> This is an incomplete outline implementation for a libblkio plugin for
> nbdkit. At the moment it only supports reading the same ("capacity")
s/same/size/
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
