search for: nbd_aio_foo

I noticed while reading the code that we have a documentation hole that may cause memory leaks for clients that are unaware, in relation to completion callbacks. The situation arises as follows: for all commands with a completion callback, I checked that the code has clean semantics: either nbd_aio_FOO() returns -1 and we never call the callback cleanup, or nbd_aio_FOO() returns a cookie and we call the callback cleanup when the command is retired. And since completion callbacks can only ever be passed to nbd_aio_FOO() functions, it is simple enough to document that when nbd_aio_FOO() fails...

...code that we have a documentation hole that > may cause memory leaks for clients that are unaware, in relation to > completion callbacks. > > The situation arises as follows: for all commands with a completion > callback, I checked that the code has clean semantics: either > nbd_aio_FOO() returns -1 and we never call the callback cleanup, or > nbd_aio_FOO() returns a cookie and we call the callback cleanup when the > command is retired.  And since completion callbacks can only ever be > passed to nbd_aio_FOO() functions, it is simple enough to document that > when...

...gt; commands are complete. It's also desirable when writing a server > validation program (such as for checking structured reads for > compliance) to be able to clean up the associated opaque data and have > a final chance to change the overall command status. > > Introduce new nbd_aio_FOO_notify functions for each command. Rewire > the existing nbd_aio_FOO to forward to the new command. (Perhaps the > generator could reduce some of the boilerplate duplication, if a later > patch wants to refactor this). I'm writing some code now using these new nbd_aio_<CMD>_not...

...les changed, 6 insertions(+), 6 deletions(-) > > ACK. > A bit of bike-shedding: In libc, we have qsort_r() which takes the function pointer before the opaque data. I'm trying to find other common frameworks that have common Closure conventions, to see if we should instead swap our nbd_aio_FOO functions to take user_data after the function pointers, instead of this switch to the nbd_add_close_callback parameter order. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

...mplete. It's also desirable when writing a server >> validation program (such as for checking structured reads for >> compliance) to be able to clean up the associated opaque data and have >> a final chance to change the overall command status. >> >> Introduce new nbd_aio_FOO_notify functions for each command. Rewire >> the existing nbd_aio_FOO to forward to the new command. (Perhaps the >> generator could reduce some of the boilerplate duplication, if a later >> patch wants to refactor this). > > I'm writing some code now using these new n...

...ing a number of changes: - Use symbolic constants instead of magic numbers/open-coded strings (well, the string for "base:allocation" was present before this libnbd bump) - Change callbacks to drop the valid_flag parameter - Add _is to nbd_read_only call - Drop the _callback suffix on nbd_aio_FOO calls - Add a struct for managing callback/user_data at once Signed-off-by: Eric Blake <eblake@redhat.com> --- Pushing, since we're releasing libnbd 0.9.8 today configure.ac | 4 +-- plugins/nbd/nbd.c | 85 +++++++++++++++++++++-------------------------- README | 2 +...

...> > ACK. > > > > A bit of bike-shedding: > > In libc, we have qsort_r() which takes the function pointer before the > opaque data. > > I'm trying to find other common frameworks that have common Closure > conventions, to see if we should instead swap our nbd_aio_FOO functions > to take user_data after the function pointers, instead of this switch to > the nbd_add_close_callback parameter order. I don't really mind except to say we should do it consistently one way or the other, and we should decide which way to do it fairly soon :-) Rich. -- Rich...

...- Use symbolic constants instead of magic numbers/open-coded strings > (well, the string for "base:allocation" was present before this > libnbd bump) > - Change callbacks to drop the valid_flag parameter > - Add _is to nbd_read_only call > - Drop the _callback suffix on nbd_aio_FOO calls > - Add a struct for managing callback/user_data at once Seems reasonable. > @@ -160,11 +160,12 @@ nbdplug_config (const char *key, const char *value) > if (strcasecmp (value, "require") == 0 || > strcasecmp (value, "required") == 0 || >...

The API changes from: int nbd_add_close_callback (struct nbd_handle *h, nbd_close_callback cb, void *user_data); to: int nbd_add_close_callback (struct nbd_handle *h, void *user_data, nbd_close_callback cb); The second way is consistent with how other callbacks work

...RC: instead of having nbd_aio_pread_callback which both creates the cookie and submits the command with callback, we could instead have: nbd_aio_pread(...) creates a command and returns a cookie but does NOT send it; then nbd_aio_submit(nbd, cookie, callback, opaque) which can be used to submit ANY nbd_aio_FOO command based on its cookie. That solves the race for the callback ever being fired before the user has had a chance to track the cookie, and makes it so that only one command has to register callback functions (rather than one counterpart per aio_FOO command). Or maybe even three parts: nbd_aio...

...nce. > Also, the standalone code was able to run read of command 1 in > parallel with write of command 2 via separate threads, whereas > libnbd's state machine is serializing everything (whether or not the > state machine spreads out the I/O to do writes from the thread calling > nbd_aio_FOO and reads from the reader thread, the two are never run at > once). Rich identified and fixed the culprit - libnbd was not setting TCP_NODELAY (disabling Nagle's algorithm) the way nbd-standalone.c did, which meant that any request that gets split over TCP windowing sizes waits to send the...

There are several races / deadlocks which I've thought about. Let's see if I can remember them all ... (1) This I experienced: nbd_aio_get_fd deadlocks if there are concurrent synchronous APIs going on. A typical case is where you set up the concurrent writer thread before connecting, and then call a synchronous connect function such as connect_tcp. The synchronous function grabs

On 7/17/19 8:00 AM, Richard W.M. Jones wrote: > --- > .gitignore | 1 + > README | 2 + > configure.ac | 9 + > examples/Makefile.am | 22 ++ > examples/glib-main-loop.c | 511 ++++++++++++++++++++++++++++++++++++++ > 5 files changed, 545 insertions(+) Looks good. > > + revents = g_source_query_unix_fd

...some serialized performance. Also, the standalone code was able to run read of command 1 in parallel with write of command 2 via separate threads, whereas libnbd's state machine is serializing everything (whether or not the state machine spreads out the I/O to do writes from the thread calling nbd_aio_FOO and reads from the reader thread, the two are never run at once). The trickiest part (for me) was the fact that since the state machine loop is in a separate thread from the initial requests, the loop is often blocked on just POLLIN for the state machine fd. My initial attempt tried to grab the t...

...hat structure to learn which commands are complete. It's also desirable when writing a server validation program (such as for checking structured reads for compliance) to be able to clean up the associated opaque data and have a final chance to change the overall command status. Introduce new nbd_aio_FOO_notify functions for each command. Rewire the existing nbd_aio_FOO to forward to the new command. (Perhaps the generator could reduce some of the boilerplate duplication, if a later patch wants to refactor this). --- docs/libnbd.pod | 22 +++- generator/generator | 278 ++++++++++++++++++++++...

...he code any faster or easier to maintain, but at least the added example shows one good use case for the new API. Eric Blake (6): api: Add nbd_aio_in_flight generator: Allow DEAD state actions to run generator: Allow Int64 in callbacks states: Prepare for aio notify callback api: Add new nbd_aio_FOO_notify functions examples: New example for strict read validations .gitignore | 1 + docs/libnbd.pod | 22 +- examples/Makefile.am | 14 + examples/batched-read-write.c | 17 +- exampl...

Patch 1 played with an early draft of Rich's Fedora 30 libnbd package: https://bugzilla.redhat.com/show_bug.cgi?id=1713767#c17 Note that comment 21 provides a newer package 0.1.1-1 with a different API; and that libnbd has more unreleased API changes in the pipeline (whether that will be called 0.2 or 0.1.2); so we'll have to tweak things based on what is actually available in distros.

...bd APIs to address this limitation. The trickiest part (for me) was the fact that since the state machine loop is in a separate thread from the initial requests, the loop is often blocked on just POLLIN for the state machine fd. My initial attempt tried to grab the transaction lock before calling nbd_aio_FOO to create the cookie, and generally worked where the server's reply would then wake up the reader. But that was even slower (due to a larger lock section), and also hit a problem where large NBD_CMD_WRITE would block on writes, which the poll() was not expecting (best seen when running tests/te...

libnbd-0.1.2-1 is now available in Fedora 29/30 updates-testing, although it was not compiled against libxml2 so it lacks uri support (I ended up testing patch 4 with a self-built libnbd). Diffs since v1 - rebase to master, bump from libnbd 0.1 to 0.1.2, add URI support, better timing results Still not done - patch 5 needs associated tests Eric Blake (5): nbd: Check for libnbd nbd:

libnbd-0.1.4-1 is now available in Fedora 29/30 updates testing. Diffs since v2 - rebase to master, bump from libnbd 0.1.2 to 0.1.3+, add tests to TLS usage which flushed out the need to turn relative pathnames into absolute, doc tweaks Now that the testsuite covers TLS and libnbd has been fixed to provide the things I found lacking when developing v2, I'm leaning towards pushing this on