[Libguestfs] [libnbd PATCH] api: Allow completion callbacks to auto-retire

[Eric Blake]

On 7/23/19 3:30 PM, Eric Blake wrote:
> When using the nbd_aio_FOO_callback commands, there is nothing further
> to be learned about the command by calling nbd_aio_command_completed()
> compared to what the callback already had access to.  There are still
> scenarios where manually retiring the command after the fact is useful
> (whether the return was 0 to keep the status unchanged, or -1 to alter
> the retirement status to *error), but by allowing a return value of 1,
> we can reduce the number of API calls required.  We can also
> auto-retire the lone NBD_CMD_DISC request, reducing the amount of
> special casing in nbd_aio_[peek_]command_completed.
> 
> Note that although this is not an API change, it does change ABI, but
> that's okay as we have not declared a stable interface yet.
> 
> Update a couple of the tests and examples to give this coverage:
> examples/glib-main-loop no longer piles up unretired commands, and
> tests/server-death shows that even a command stranded by server death
> can still be auto-retired.
> ---
> 
> This probably will conflict with Rich's work to improve callbacks to
> make it easier to free data on the last use of a callback.

Here's the counterpart patch for nbdkit to utilize this; however, my
timing runs of libnbd/examples/threaded-reads-and-writes did not show
any noticeable change, so this particular optimization is in the noise
compared to any real bottlenecks remaining.  Still, the reduced lines of
code makes me like the idea:

diff --git i/plugins/nbd/nbd.c w/plugins/nbd/nbd.c
index 83a30583..ccd773ba 100644
--- i/plugins/nbd/nbd.c
+++ w/plugins/nbd/nbd.c
@@ -57,7 +57,6 @@

 /* The per-transaction details */
 struct transaction {
-  int64_t cookie;
   sem_t sem;
   uint32_t early_err;
   uint32_t err;
@@ -360,14 +359,12 @@ nbdplug_notify (void *opaque, int64_t cookie, int
*error)

   nbdkit_debug ("cookie %" PRId64 " completed state machine, status %d",
                 cookie, *error);
-  assert (trans->cookie == 0 || trans->cookie == cookie);
-  trans->cookie = cookie;
   trans->err = *error;
   if (sem_post (&trans->sem)) {
     nbdkit_error ("failed to post semaphore: %m");
     abort ();
   }
-  return 0;
+  return 1;
 }

 /* Register a cookie and kick the I/O thread. */
@@ -386,8 +383,6 @@ nbdplug_register (struct handle *h, struct
transaction *trans, int64_t cookie)

   if (write (h->fds[1], &c, 1) != 1 && errno != EAGAIN)
     nbdkit_debug ("failed to kick reader thread: %m");
-  assert (trans->cookie == 0 || trans->cookie == cookie);
-  trans->cookie = cookie;
 }

 /* Perform the reply half of a transaction. */
@@ -405,13 +400,8 @@ nbdplug_reply (struct handle *h, struct transaction
*trans)
       nbdkit_debug ("failed to wait on semaphore: %m");
       err = EIO;
     }
-    else {
-      assert (trans->cookie > 0);
-      err = nbd_aio_command_completed (h->nbd, trans->cookie);
-      assert (err != 0);
-      assert (err == 1 ? trans->err == 0 : trans->err == nbd_get_errno ());
+    else
       err = trans->err;
-    }
   }
   if (sem_destroy (&trans->sem))
     abort ();

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190723/87eacd93/attachment.sig>