[Libguestfs] [nbdkit PATCH v3 11/15] plugins: Expose new FUA callbacks

[Eric Blake]

On 03/08/2018 05:03 PM, Eric Blake wrote:
> The NBD protocol supports Forced Unit Access (FUA) as a more efficient
> way to wait for just one write to land in persistent storage, rather
> than all outstanding writes at the time of a flush; modeled after
> the kernel's block I/O flag of the same name.  While we can emulate
> the proper semantics with a full-blown flush, there are some plugins
> that can properly pass the FUA flag on to the end storage and thereby
> avoid some overhead.
> 
> This patch introduces new callbacks, and updates the documentation
> to the new API, while ensuring that plugins compiled to the old API
> still work.  The new API adds .can_fua, then adds a flags parameter
> to all five data callbacks, even though only three of them will use
> a flag at the moment.  A plugin client has to opt in to both the
> version 2 API and provide .can_fua with a return of NBDKIT_FUA_NATIVE
> before nbdkit will pass the NBDKIT_FLAG_FUA to the plugin.
> 

> 
> +=head2 C<.can_fua>
> +
> + int can_fua (void *handle);
> +
> +This is called during the option negotiation phase to find out if the
> +plugin supports the Forced Unit Access (FUA) flag on write, zero, and
> +trim requests.  If this returns C<NBDKIT_FUA_NONE>, FUA support is not
> +advertised to the guest; if this returns C<NBDKIT_FUA_EMULATE>, the
> +C<.flush> callback must work (even if C<.can_flush> returns false),
> +and FUA support is emulated by calling C<.flush> after any write
> +operation; if this returns C<NBDKIT_FUA_NATIVE>, then the C<.pwrite>,
> +C<.zero>, and C<.trim> callbacks (if implemented) must handle the flag
> +C<NBDKIT_FLAG_FUA>, by not returning until that action has landed in
> +persistent storage.
> +
> +If there is an error, C<.can_fua> should call C<nbdkit_error> with an
> +error message and return C<-1>.
> +
> +This callback is not required unless a plugin wants to specifically
> +handle FUA requests.  If omitted, nbdkit checks C<.can_flush>, and
> +behaves as if this function returns C<NBDKIT_FUA_NONE> or
> +C<NBDKIT_FUA_EMULATE> as appropriate.

That documentation doesn't quite match the code, although I prefer the
documentation...

> +++ b/src/plugins.c

> @@ -377,14 +383,20 @@ plugin_can_fua (struct backend *b, struct connection *conn)
>    struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
>    int r;
> 
> +  assert (connection_get_handle (conn, 0));
> +
>    debug ("can_fua");
> 
> -  /* TODO - wire FUA flag support into plugins. Until then, this copies
> -   * can_flush, since that's how we emulate FUA. */
> +  if (p->plugin.can_fua) {
> +    r = p->plugin.can_fua (connection_get_handle (conn, 0));
> +    if (r > NBDKIT_FUA_EMULATE && p->plugin._api_version == 1)
> +      r = NBDKIT_FUA_EMULATE;
> +    return r;
> +  }
>    r = plugin_can_flush (b, conn);
>    if (r == -1)
>      return -1;
> -  if (r == 0 || !p->plugin.flush)
> +  if (r == 0 || !(p->plugin.flush || p->plugin._flush_old))
>      return NBDKIT_FUA_NONE;
>    return NBDKIT_FUA_EMULATE;

According to the documentation, the result of .can_flush should have no
impact on the return; we want to return NBDKIT_FUA_EMULATE if .flush
exists, even when can_flush returns false.  I'll be pushing this as
followup:

diff --git i/src/plugins.c w/src/plugins.c
index ff4facf..8ee58fb 100644
--- i/src/plugins.c
+++ w/src/plugins.c
@@ -386,18 +386,18 @@ plugin_can_fua (struct backend *b, struct
connection *conn)

   debug ("can_fua");

+  /* The plugin MUST provide .can_fua with a return of NBDKIT_FUA_NATIVE
+     before we will pass FUA on to the plugin. */
   if (p->plugin.can_fua) {
     r = p->plugin.can_fua (connection_get_handle (conn, 0));
     if (r > NBDKIT_FUA_EMULATE && p->plugin._api_version == 1)
       r = NBDKIT_FUA_EMULATE;
     return r;
   }
-  r = plugin_can_flush (b, conn);
-  if (r == -1)
-    return -1;
-  if (r == 0 || !(p->plugin.flush || p->plugin._flush_old))
-    return NBDKIT_FUA_NONE;
-  return NBDKIT_FUA_EMULATE;
+  /* We intend to call .flush even if .can_flush returns false */
+  if (p->plugin.flush || p->plugin._flush_old)
+    return NBDKIT_FUA_EMULATE;
+  return NBDKIT_FUA_NONE;
 }

 /* Grab the appropriate error value.


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

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