search for: nbd_aio_pread_callback

Previously discussed here: https://www.redhat.com/archives/libguestfs/2019-July/msg00213.html It turns out that deferring callbacks is a PITA. (It would be a bit easier if C has closures.) However by rewriting the example we can avoid the need to use the cookie at all and make it run a bit more efficiently, so let's do that instead. Rich.

...(sizeof *wrap); wrap->buf = malloc (sizeof (*wrap->buf)); caml_register_generational_global_root (wrap->buf); struct nbd_buffer *buf_buf = NBD_buffer_val (bufv); const void *buf = buf_buf->data; size_t count = buf_buf->len; if (in nbd_aio_pread) { wrap->callback = NULL; res = nbd_aio_pread_callback(h, buf, count, offset, NULL, free_cb, wrap, flags); } else /* in nbd_aio_pread_callback */ { wrap->callback = ...; res = nbd_aio_pread_callback(h, buf, count, offset, completion_cb, free_cb, wrap,...

There was a theoretic race in this example: If the server was very fast at handling commands then it's possible that in a call such as: cookie = nbd_aio_pread_callback (..., callback, ...); buffers[i].cookie = cookie; nbd_aio_pread_callback finished and calls the callback before returning. buffers[i].cookie would therefore not be set, but the callback() function was checking the list of buffers for the cookie. This would have caused an abort() in the existing...

...clients. Also, there may still be a possible race: > +/* This idle callback reads data from the source nbdkit until the ring > + * is full. > + */ > +static gboolean > +read_data (gpointer user_data) > +{ > + static uint64_t posn = 0; > + buffers[i].rcookie = > + nbd_aio_pread_callback (gssrc->nbd, buffers[i].data, > + BUFFER_SIZE, buffers[i].offset, > + finished_read, NULL, 0); It may be possible in rare situations that the libnbd state machine can send() the command AND see data ready to recv() (perhaps from a prev...

On Mon, Aug 12, 2019 at 11:00:01PM +0100, Richard W.M. Jones wrote: > On Mon, Aug 12, 2019 at 01:53:56PM -0500, Eric Blake wrote: > > We then have the design question of whether to make an OClosure type, > > where C has two functions nbd_aio_pread and nbd_aio_pread_callback for > > convenience, but where other languages have only a single nbd.aio_pread > > where the callback parameter is optional (the Closure type for > > pread_structured chunk and for block_status extent will still be > > mandatory; it is only the completion callback that is c...

...ns that the libnbd state machine can > send() the command AND see data ready to recv() (perhaps from a previous > command still in flight), where it ends up read()ing until blocking and > happens to get the server's reply to this command and fire off the > callback, all prior to the nbd_aio_pread_callback() returning. If that > ever happens, then buffers[i].rcookie will still be unset at the time > the callback fires... > >> +/* This callback is called from libnbd when any read command finishes. */ >> +static int >> +finished_read (void *vp, int64_t rcookie, int *error)...

...on-optional in libnbd C bindings seems reasonable enough. Another possibility is to generate n different function names (each adding one additional optarg) or even 2^n function names (one for each combination of which optional args are being provided); in that light, our existing nbd_aio_pread vs. nbd_aio_pread_callback could be viewed as two expansions with an optional completion Closure argument, where we generate 2 C bindings, but where other languages could have just a single entry point with an optional Closure argument. But that's obviously ideas for a future patch... > > Note this commit does n...

On 8/12/19 11:08 AM, Richard W.M. Jones wrote: > This adds a C-only semi-private function for freeing various types of > persistent data passed to libnbd. > > There are some similarities with nbd_add_close_callback which we > removed in commit 7f191b150b52ed50098976309a6af883d245fc56. > --- > +=head1 FREE CALLBACKS > + > +B<Note:> The API described in this

...nbd_aio_* > > (2) Any existing call to nbd_aio_* must have two extra NULL parameters > added before the final flags parameter. > > In non-C languages, only change (1) is required. It is still possible to compile for 0.9.6 and this patch simultaneously, by checking LIBNBD_HAVE_NBD_AIO_PREAD_CALLBACK as a witness of which API style to use (although it's also just as easy to bump minimum version requirements to 0.9.7, once we have a release including this and any other API changes being discussed...). > +++ b/docs/libnbd.pod > @@ -276,9 +276,8 @@ command has completed: > } >...

libnbd 0.1.7 was released today, which breaks compilation of nbdkit-nbd-plugin. We could work around it by using #ifdef LIBNBD_HAVE_XXX to learn about the renamed functions, but supporting older versions is not all that important when we don't yet have API stability. So patch 1 copes by just bumping the minimum version instead, except that we have yet another pending libnbd patch with an API

This applies on top of the OClosure v2 series posted a few minutes ago. Rich.

This is working now, and incorporates all of the changes in Eric's review, *except* that it still doesn't retire commands (although this seems to make no obvious difference, except possibly a performance and memory impact). Rich.

On Mon, Aug 12, 2019 at 01:53:56PM -0500, Eric Blake wrote: > We then have the design question of whether to make an OClosure type, > where C has two functions nbd_aio_pread and nbd_aio_pread_callback for > convenience, but where other languages have only a single nbd.aio_pread > where the callback parameter is optional (the Closure type for > pread_structured chunk and for block_status extent will still be > mandatory; it is only the completion callback that is currently causing &gt...

...--git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 48873d09..83a30583 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -673,7 +673,7 @@ nbdplug_pread (void *handle, void *buf, uint32_t count, uint64_t offset, assert (!flags); nbdplug_prepare (&s); nbdplug_register (h, &s, nbd_aio_pread_callback (h->nbd, buf, count, offset, - &s, nbdplug_notify, 0)); + nbdplug_notify, &s, 0)); return nbdplug_reply (h, &s); } @@ -689,7 +689,7 @@ nbdplug_pwrite (void *handle, const void *bu...

...51b1a03..aeecaee 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -276,9 +276,8 @@ command has completed: } For almost all high level synchronous calls (eg. C<nbd_pread>) there -are two low level asynchronous equivalents (eg. C<nbd_aio_pread> for -starting a command, and C<nbd_aio_pread_callback> for also registering -a callback to be invoked right before the command is complete). +is a low level asynchronous equivalents (eg. C<nbd_aio_pread> for +starting a command). =head2 glib2 integration @@ -600,8 +599,8 @@ will use your login name): =head1 CALLBACKS Some libnbd call...

The next patch will add a new 'nbd_aio_pread_callback' function for allowing the user more immediate access as each chunk of a structured reply read is received. But before we do that, let's refactor the command code. This includes a revert of 12843a1a, since the read callback will need both a buffer and the user's opaque object at the sam...

...llback. If any callback fails, and if no prior error was set, then the callback's failure becomes the failure reason for the overall read. Nothing actually passes a callback function yet, so for now this is no functional change; but this will make it possible for the next patch to add an 'nbd_aio_pread_callback' API. --- generator/generator | 4 +++ generator/states-reply-simple.c | 15 +++++++++- generator/states-reply-structured.c | 43 +++++++++++++++++++++++++++-- lib/internal.h | 4 ++- 4 files changed, 62 insertions(+), 4 deletions(-) diff --git a/gen...

...d_aio_get_fd@LIBNBD_1.0 U nbd_aio_in_flight@LIBNBD_1.0 U nbd_aio_is_ready@LIBNBD_1.0 U nbd_aio_notify_read@LIBNBD_1.0 U nbd_aio_notify_write@LIBNBD_1.0 U nbd_aio_peek_command_completed@LIBNBD_1.0 U nbd_aio_pread_callback@LIBNBD_1.0 U nbd_aio_pwrite_callback@LIBNBD_1.0 U nbd_close@LIBNBD_1.0 U nbd_create@LIBNBD_1.0 U nbd_get_debug@LIBNBD_1.0 U nbd_get_error@LIBNBD_1.0 Rich.

** NOT WORKING ** This patch shows how to integrate libnbd and the glib main loop. Posted mainly as a point of discussion as it doesn't quite work yet. Rich.

On Mon, Jun 17, 2019 at 07:07:56PM -0500, Eric Blake wrote: > diff --git a/generator/generator b/generator/generator > index 2614689..ce77f17 100755 > --- a/generator/generator > +++ b/generator/generator > @@ -1305,7 +1305,72 @@ Issue a read command to the NBD server for the range starting > at C<offset> and ending at C<offset> + C<count> - 1. NBD > can