Libguestfs - Jun 2019 - [libnbd] How close are we to declaring a stable API?

As the subject says, how close are we to being able to declare a
stable API for libnbd?

I believe these are the main topics:

* Do we need to have an extra thread for writing?  I'm unclear about
  whether b92392b717 (which allows the state machine to break during
  reply processing) means we definitely don't need threads.  I imagine
  that two threads doing simultaneous send(2) and recv(2) calls could
  still improve performance (eg. having two cores copying skbs from
  userspace to and from kernel).

* Should ‘nbd_shutdown’ take an extra parameter to indicate that it
  should be delayed until all commands in the queue are retired?

Is there anything else?

We could also consider doing a "soft stable API" release where we bump
the version up to 0.9.x, announce that we're going to make the API
stable soon, have a much higher bar for breaking the API, but don't
actually prevent API breaks in cases where it's necessary.

Rich.

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

On 6/29/19 5:25 AM, Richard W.M. Jones wrote:
> As the subject says, how close are we to being able to declare a
> stable API for libnbd?
> 
> I believe these are the main topics:
> 
> * Do we need to have an extra thread for writing?  I'm unclear about
>   whether b92392b717 (which allows the state machine to break during
>   reply processing) means we definitely don't need threads.  I imagine
>   that two threads doing simultaneous send(2) and recv(2) calls could
>   still improve performance (eg. having two cores copying skbs from
>   userspace to and from kernel).
It's hard to see how simultaneous send() and recv() with nonblocking
sockets will do any better across two threads than one. If it was
blocking, then it makes total sense, but since we have non-blocking I/O,
I'm not seeing that it will make any noticeable difference.  That said,
I do know that you were experimenting at one point about adding a way to
offload writing to a user-controlled thread, and maybe it's still worth
playing with that a bit more.
> 
> * Should ‘nbd_shutdown’ take an extra parameter to indicate that it
>   should be delayed until all commands in the queue are retired?
That may still be worthwhile to pursue.
> 
> Is there anything else?
Do we like the signature of all the callbacks? Right now, there is a
slight inconsistency in that the 'int *error' parameter is last for
block_status and notify callbacks, but comes before 'int status' for
pread_structured. It would be a simple API switch to pread_structured to
put it last there as well, but something we can't do after declaring
stability.
> 
> We could also consider doing a "soft stable API" release where we
bump
> the version up to 0.9.x, announce that we're going to make the API
> stable soon, have a much higher bar for breaking the API, but don't
> actually prevent API breaks in cases where it's necessary.
The notify APIs are in now, and I'm trying to squash a valgrind failure
where tests/errors sometimes fails under load (if the server can read
data fast enough that a large NBD_CMD_WRITE doesn't block after all).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 7/2/19 8:48 PM, Eric Blake wrote:
>>
>> Is there anything else?
> 
> Do we like the signature of all the callbacks? Right now, there is a
> slight inconsistency in that the 'int *error' parameter is last for
> block_status and notify callbacks, but comes before 'int status'
for
> pread_structured. It would be a simple API switch to pread_structured to
> put it last there as well, but something we can't do after declaring
> stability.
Oh, we may also want to experiment with block sizes prior to declaring
API stable, in order to better support servers that want us to obey
strict NBD_INFO_BLOCK_SIZE.  We may find that it gets easier if we h ave
additional parameters and/or APIs to let clients easily perform
read-modify-write or similar to widen requests out to the granularities
required by the server.

And I still wonder if we need an nbd_aio_notify_error for use when
poll() fails with POLLHUP/POLLERR/POLLNVAL, to at least advance the
state machine into an error state.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Jul 02, 2019 at 08:48:15PM -0500, Eric Blake
wrote:
> On 6/29/19 5:25 AM, Richard W.M. Jones wrote:
> > As the subject says, how close are we to being able to declare a
> > stable API for libnbd?
> > 
> > I believe these are the main topics:
> > 
> > * Do we need to have an extra thread for writing?  I'm unclear
about
> >   whether b92392b717 (which allows the state machine to break during
> >   reply processing) means we definitely don't need threads.  I
imagine
> >   that two threads doing simultaneous send(2) and recv(2) calls could
> >   still improve performance (eg. having two cores copying skbs from
> >   userspace to and from kernel).
> 
> It's hard to see how simultaneous send() and recv() with nonblocking
> sockets will do any better across two threads than one. If it was
> blocking, then it makes total sense, but since we have non-blocking I/O,
> I'm not seeing that it will make any noticeable difference.  That said,
> I do know that you were experimenting at one point about adding a way to
> offload writing to a user-controlled thread, and maybe it's still worth
> playing with that a bit more.
The POSIX send and recv APIs involve copying.  It does seem plausible
that two cores could perform better than one by doing the copies
between userspace and kernel space in parallel.  However it would
certainly depend on the specifics of how Linux works - for example
does it hold a lock per socket?

I had a patch to enable a concurrent writer thread:
https://www.redhat.com/archives/libguestfs/2019-June/msg00030.html
However it had multiple unresolved design problems:
https://www.redhat.com/archives/libguestfs/2019-June/msg00036.html

A better way to solve it might be to use an alternate set of APIs.
uring is one obvious choice.
> > 
> > * Should ‘nbd_shutdown’ take an extra parameter to indicate that it
> >   should be delayed until all commands in the queue are retired?
> 
> That may still be worthwhile to pursue.
> 
> > 
> > Is there anything else?
> 
> Do we like the signature of all the callbacks? Right now, there is a
> slight inconsistency in that the 'int *error' parameter is last for
> block_status and notify callbacks, but comes before 'int status'
for
> pread_structured. It would be a simple API switch to pread_structured to
> put it last there as well, but something we can't do after declaring
> stability.
Yes we ought to fix these kinds of things before making the API
stable.
> > 
> > We could also consider doing a "soft stable API" release
where we bump
> > the version up to 0.9.x, announce that we're going to make the API
> > stable soon, have a much higher bar for breaking the API, but
don't
> > actually prevent API breaks in cases where it's necessary.
> 
> The notify APIs are in now, and I'm trying to squash a valgrind failure
> where tests/errors sometimes fails under load (if the server can read
> data fast enough that a large NBD_CMD_WRITE doesn't block after all).
OK, thanks.

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

On Tue, Jul 02, 2019 at 08:48:15PM -0500, Eric Blake
wrote:
> On 6/29/19 5:25 AM, Richard W.M. Jones wrote:
> > As the subject says, how close are we to being able to declare a
> > stable API for libnbd?
> > 
> > I believe these are the main topics:
> > 
> > * Do we need to have an extra thread for writing?  I'm unclear
about
> >   whether b92392b717 (which allows the state machine to break during
> >   reply processing) means we definitely don't need threads.  I
imagine
> >   that two threads doing simultaneous send(2) and recv(2) calls could
> >   still improve performance (eg. having two cores copying skbs from
> >   userspace to and from kernel).
> 
> It's hard to see how simultaneous send() and recv() with nonblocking
> sockets will do any better across two threads than one. If it was
> blocking, then it makes total sense, but since we have non-blocking I/O,
> I'm not seeing that it will make any noticeable difference.  That said,
> I do know that you were experimenting at one point about adding a way to
> offload writing to a user-controlled thread, and maybe it's still worth
> playing with that a bit more.
Consider also what other work the threads are doing. With GNUTLS' API
design, the thread doing the recv()/send() is almost certainly going
to be doing the encrypt/decrypt of data, and that is likely to dominate
the utilization of the thread, even with harware accelerated crypto.
So if there's likely to be non-trivially size data packets both recvd
and sent oncurrently, separate threads will be a win with TLS.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|