Libguestfs - Jan 2018 - Re: [nbdkit PATCH 0/7] Initial implementation of FUA flag passthrough

Tested via:
term1$ qemu-nbd -k $PWD/sock -t -f raw -x foo junk --trace=nbd_\*
term2$ ./nbdkit -f -v -e bar nbd socket=$PWD/sock export=foo
term3$ qemu-io -t none -f raw nbd://localhost:10809/bar --trace=nbd_\*

and checking the traces to see that 'w 0 1' vs. 'w -f 0 1' was
able
to influence whether the FUA flag showed up at the server in term1.

Still to go: figure out how to expose the flags through the language
bindings

Patch 7 is RFC; Rich and I debated on IRC whether we want it (the
code in plugin_register() sounds like it tries hard to support
newer plugins with older nbdkit), or whether we should just drop
the patch and state that a user either manually supplies back-compat
variants (tedious boilerplate for the user to supply, especially
since newer nbdkit will never call the older functions) or just
document that newer plugins combined with old nbdkit is not
guaranteed to work.

Eric Blake (7):
  connections: Ignore FUA flag on read/flush
  protocol: Split flags from cmd field in requests
  plugins: Move FUA fallback to plugins
  plugins: Add callbacks for FUA semantics
  plugins: Implement FUA support
  nbd: Wire up FUA flag passthrough
  RFC: plugins: Add back-compat for new plugin with old nbdkit

 docs/nbdkit-plugin.pod  | 77 +++++++++++++++++++++++++++++++++++++--
 docs/nbdkit.pod         |  8 +++--
 include/nbdkit-plugin.h | 34 +++++++++++++++++-
 plugins/nbd/nbd.c       | 64 ++++++++++++++++++++-------------
 src/connections.c       | 46 +++++++++++++-----------
 src/internal.h          |  9 ++---
 src/plugins.c           | 96 +++++++++++++++++++++++++++++++++++++++----------
 src/protocol.h          | 10 +++---
 8 files changed, 266 insertions(+), 78 deletions(-)

-- 
2.14.3

The NBD spec says that we must tolerate a client sending
NBD_CMD_FLAG_FUA on any command (due to historical behavior
of at least qemu sending it on READ), but that it only has
to have defined semantics on commands that can cause write
actions.  It will be easier for future patches to support
plugins that can honor FUA semantics on write if we silently
ignore FUA on non-writes (the NBD spec says only WRITE,
WRITE_ZEROES, and TRIM have write semantics).

Note that validate_request already ensured that that we are
not calling a write command if conn->readonly; since only
write commands can leave flush_after_command set, we no longer
need to check conn->readonly in handle_request.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/connections.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index 111a810..a16c118 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2017 Red Hat Inc.
+ * Copyright (C) 2013-2018 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -871,9 +871,7 @@ handle_request (struct connection *conn,
   bool flush_after_command;

   /* Flush after command performed? */
-  flush_after_command = (flags & NBD_CMD_FLAG_FUA) != 0;
-  if (!conn->can_flush || conn->readonly)
-    flush_after_command = false;
+  flush_after_command = conn->can_flush && (flags &
NBD_CMD_FLAG_FUA);

   /* The plugin should call nbdkit_set_error() to request a particular
      error, otherwise we fallback to errno or EIO. */
@@ -881,6 +879,7 @@ handle_request (struct connection *conn,

   switch (cmd) {
   case NBD_CMD_READ:
+    flush_after_command = false;
     if (plugin_pread (conn, buf, count, offset) == -1)
       return get_error (conn);
     break;
@@ -891,6 +890,7 @@ handle_request (struct connection *conn,
     break;

   case NBD_CMD_FLUSH:
+    flush_after_command = false;
     if (plugin_flush (conn) == -1)
       return get_error (conn);
     break;
-- 
2.14.3

Since NBD is a big-endian protocol, the upstream spec was able to
repurpose a 32-bit field with flags starting at (1<<16) OR'd into
the command into two 16-bit fields (flags first, starting at 1<<0,
then the command) for ease of documentation.  Matching that split
in our code base will also make it easier to implement smarter FUA
flag support.  This addresses one of the TODO in the nbd plugin.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/nbd/nbd.c | 39 ++++++++++++++++++++-------------------
 src/connections.c | 12 ++++++------
 src/protocol.h    | 10 +++++-----
 3 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index 04147f1..c9727f7 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2017 Red Hat Inc.
+ * Copyright (C) 2017-2018 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -249,13 +249,14 @@ find_trans_by_cookie (struct handle *h, uint64_t cookie)

 /* Send a request, return 0 on success or -1 on write failure. */
 static int
-nbd_request_raw (struct handle *h, uint32_t type, uint64_t offset,
-                 uint32_t count, uint64_t cookie, const void *buf)
+nbd_request_raw (struct handle *h, uint16_t flags, uint16_t type,
+                 uint64_t offset, uint32_t count, uint64_t cookie,
+                 const void *buf)
 {
   struct request req = {
     .magic = htobe32 (NBD_REQUEST_MAGIC),
-    /* TODO nbdkit should have a way to pass flags, separate from cmd type */
-    .type = htobe32 (type),
+    .flags = htobe16 (flags),
+    .type = htobe16 (type),
     .handle = cookie, /* Opaque to server, so endianness doesn't matter */
     .offset = htobe64 (offset),
     .count = htobe32 (count),
@@ -275,8 +276,9 @@ nbd_request_raw (struct handle *h, uint32_t type, uint64_t
offset,
 /* Perform the request half of a transaction. On success, return the
    non-negative fd for reading the reply; on error return -1. */
 static int
-nbd_request_full (struct handle *h, uint32_t type, uint64_t offset,
-                  uint32_t count, const void *req_buf, void *rep_buf)
+nbd_request_full (struct handle *h, uint16_t flags, uint16_t type,
+                  uint64_t offset, uint32_t count, const void *req_buf,
+                  void *rep_buf)
 {
   int err;
   struct transaction *trans;
@@ -307,7 +309,7 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t
offset,
   fd = trans->u.fds[0];
   cookie = trans->u.cookie;
   nbd_unlock (h);
-  if (nbd_request_raw (h, type, offset, count, cookie, req_buf) == 0)
+  if (nbd_request_raw (h, flags, type, offset, count, cookie, req_buf) == 0)
     return fd;
   trans = find_trans_by_cookie (h, cookie);

@@ -326,9 +328,10 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t
offset,

 /* Shorthand for nbd_request_full when no extra buffers are involved. */
 static int
-nbd_request (struct handle *h, uint32_t type, uint64_t offset, uint32_t count)
+nbd_request (struct handle *h, uint16_t flags, uint16_t type, uint64_t offset,
+             uint32_t count)
 {
-  return nbd_request_full (h, type, offset, count, NULL, NULL);
+  return nbd_request_full (h, flags, type, offset, count, NULL, NULL);
 }

 /* Read a reply, and look up the fd corresponding to the transaction.
@@ -563,7 +566,7 @@ nbd_close (void *handle)
   struct handle *h = handle;

   if (!h->dead)
-    nbd_request_raw (h, NBD_CMD_DISC, 0, 0, 0, NULL);
+    nbd_request_raw (h, 0, NBD_CMD_DISC, 0, 0, 0, NULL);
   close (h->fd);
   if ((errno = pthread_join (h->reader, NULL)))
     nbdkit_debug ("failed to join reader thread: %m");
@@ -622,7 +625,7 @@ nbd_pread (void *handle, void *buf, uint32_t count, uint64_t
offset)

   /* TODO Auto-fragment this if the client has a larger max transfer
      limit than the server */
-  c = nbd_request_full (h, NBD_CMD_READ, offset, count, NULL, buf);
+  c = nbd_request_full (h, 0, NBD_CMD_READ, offset, count, NULL, buf);
   return c < 0 ? c : nbd_reply (h, c);
 }

@@ -635,7 +638,7 @@ nbd_pwrite (void *handle, const void *buf, uint32_t count,
uint64_t offset)

   /* TODO Auto-fragment this if the client has a larger max transfer
      limit than the server */
-  c = nbd_request_full (h, NBD_CMD_WRITE, offset, count, buf, NULL);
+  c = nbd_request_full (h, 0, NBD_CMD_WRITE, offset, count, buf, NULL);
   return c < 0 ? c : nbd_reply (h, c);
 }

@@ -644,7 +647,6 @@ static int
 nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
 {
   struct handle *h = handle;
-  uint32_t cmd = NBD_CMD_WRITE_ZEROES;
   int c;

   if (!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES)) {
@@ -653,9 +655,8 @@ nbd_zero (void *handle, uint32_t count, uint64_t offset, int
may_trim)
     return -1;
   }

-  if (!may_trim)
-    cmd |= NBD_CMD_FLAG_NO_HOLE;
-  c = nbd_request (h, cmd, offset, count);
+  c = nbd_request (h, may_trim ? 0 : NBD_CMD_FLAG_NO_HOLE,
+                   NBD_CMD_WRITE_ZEROES, offset, count);
   return c < 0 ? c : nbd_reply (h, c);
 }

@@ -666,7 +667,7 @@ nbd_trim (void *handle, uint32_t count, uint64_t offset)
   struct handle *h = handle;
   int c;

-  c = nbd_request (h, NBD_CMD_TRIM, offset, count);
+  c = nbd_request (h, 0, NBD_CMD_TRIM, offset, count);
   return c < 0 ? c : nbd_reply (h, c);
 }

@@ -677,7 +678,7 @@ nbd_flush (void *handle)
   struct handle *h = handle;
   int c;

-  c = nbd_request (h, NBD_CMD_FLUSH, 0, 0);
+  c = nbd_request (h, 0, NBD_CMD_FLUSH, 0, 0);
   return c < 0 ? c : nbd_reply (h, c);
 }

diff --git a/src/connections.c b/src/connections.c
index a16c118..24df4b6 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -760,7 +760,7 @@ valid_range (struct connection *conn, uint64_t offset,
uint32_t count)

 static bool
 validate_request (struct connection *conn,
-                  uint32_t cmd, uint32_t flags, uint64_t offset, uint32_t
count,
+                  uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t
count,
                   uint32_t *error)
 {
   /* Readonly connection? */
@@ -865,7 +865,7 @@ get_error (struct connection *conn)
  */
 static uint32_t
 handle_request (struct connection *conn,
-                uint32_t cmd, uint32_t flags, uint64_t offset, uint32_t count,
+                uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count,
                 void *buf)
 {
   bool flush_after_command;
@@ -979,7 +979,8 @@ recv_request_send_reply (struct connection *conn)
   int r;
   struct request request;
   struct reply reply;
-  uint32_t magic, cmd, flags, count, error = 0;
+  uint16_t cmd, flags;
+  uint32_t magic, count, error = 0;
   uint64_t offset;
   CLEANUP_FREE char *buf = NULL;

@@ -1005,9 +1006,8 @@ recv_request_send_reply (struct connection *conn)
       return set_status (conn, -1);
     }

-    cmd = be32toh (request.type);
-    flags = cmd & ~NBD_CMD_MASK_COMMAND;
-    cmd &= NBD_CMD_MASK_COMMAND;
+    flags = be16toh (request.flags);
+    cmd = be16toh (request.type);

     offset = be64toh (request.offset);
     count = be32toh (request.count);
diff --git a/src/protocol.h b/src/protocol.h
index 9d9dad9..aa458a0 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013 Red Hat Inc.
+ * Copyright (C) 2013-2018 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -112,7 +112,8 @@ struct new_handshake_finish {
 /* Request (client -> server). */
 struct request {
   uint32_t magic;               /* NBD_REQUEST_MAGIC. */
-  uint32_t type;                /* Request type. */
+  uint16_t flags;               /* Request flags. */
+  uint16_t type;                /* Request type. */
   uint64_t handle;              /* Opaque handle. */
   uint64_t offset;              /* Request offset. */
   uint32_t count;               /* Request length. */
@@ -134,9 +135,8 @@ struct reply {
 #define NBD_CMD_FLUSH             3
 #define NBD_CMD_TRIM              4
 #define NBD_CMD_WRITE_ZEROES      6
-#define NBD_CMD_MASK_COMMAND 0xffff
-#define NBD_CMD_FLAG_FUA     (1<<16)
-#define NBD_CMD_FLAG_NO_HOLE (2<<16)
+#define NBD_CMD_FLAG_FUA      (1<<0)
+#define NBD_CMD_FLAG_NO_HOLE  (1<<1)

 /* Error codes (previously errno).
  * See
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=ca4414804114fd0095b317785bc0b51862e62ebb
-- 
2.14.3

As long as the plugins couldn't directly handle Forced Unit Access
(FUA), it made sense to have a common fallback in handle_request().
But upcoming patches will allow plugins to handle FUA, at which
point the fallback is easier to implement on a per-command basis,
via an extra parameter to the plugins wrapper code.

We also add an additional validation to commands sent by the client:
if we did not advertise FUA flag support (because the plugin does
not support flush), then the client should not be requesting FUA;
this in turn means that we can assert that the flush callback is
valid if the plugins layer gets a fua flag.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/connections.c | 21 +++++++++------------
 src/internal.h    |  8 ++++----
 src/plugins.c     | 42 +++++++++++++++++++++++++++++++-----------
 3 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index 24df4b6..e4a0a82 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -814,6 +814,11 @@ validate_request (struct connection *conn,
     *error = EINVAL;
     return false;
   }
+  if (!conn->can_flush && (flags & NBD_CMD_FLAG_FUA)) {
+    nbdkit_error ("invalid request: FUA flag not supported");
+    *error = EINVAL;
+    return false;
+  }

   /* Refuse over-large read and write requests. */
   if ((cmd == NBD_CMD_WRITE || cmd == NBD_CMD_READ) &&
@@ -868,10 +873,7 @@ handle_request (struct connection *conn,
                 uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count,
                 void *buf)
 {
-  bool flush_after_command;
-
-  /* Flush after command performed? */
-  flush_after_command = conn->can_flush && (flags &
NBD_CMD_FLAG_FUA);
+  bool fua = conn->can_flush && (flags & NBD_CMD_FLAG_FUA);

   /* The plugin should call nbdkit_set_error() to request a particular
      error, otherwise we fallback to errno or EIO. */
@@ -879,30 +881,28 @@ handle_request (struct connection *conn,

   switch (cmd) {
   case NBD_CMD_READ:
-    flush_after_command = false;
     if (plugin_pread (conn, buf, count, offset) == -1)
       return get_error (conn);
     break;

   case NBD_CMD_WRITE:
-    if (plugin_pwrite (conn, buf, count, offset) == -1)
+    if (plugin_pwrite (conn, buf, count, offset, fua) == -1)
       return get_error (conn);
     break;

   case NBD_CMD_FLUSH:
-    flush_after_command = false;
     if (plugin_flush (conn) == -1)
       return get_error (conn);
     break;

   case NBD_CMD_TRIM:
-    if (plugin_trim (conn, count, offset) == -1)
+    if (plugin_trim (conn, count, offset, fua) == -1)
       return get_error (conn);
     break;

   case NBD_CMD_WRITE_ZEROES:
     if (plugin_zero (conn, count, offset,
-                     !(flags & NBD_CMD_FLAG_NO_HOLE)) == -1)
+                     !(flags & NBD_CMD_FLAG_NO_HOLE), fua) == -1)
       return get_error (conn);
     break;

@@ -910,9 +910,6 @@ handle_request (struct connection *conn,
     abort ();
   }

-  if (flush_after_command && plugin_flush (conn) == -1)
-    return get_error (conn);
-
   return 0;
 }

diff --git a/src/internal.h b/src/internal.h
index 73bc09e..3ab08d3 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2017 Red Hat Inc.
+ * Copyright (C) 2013-2018 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -164,10 +164,10 @@ extern int plugin_can_flush (struct connection *conn);
 extern int plugin_is_rotational (struct connection *conn);
 extern int plugin_can_trim (struct connection *conn);
 extern int plugin_pread (struct connection *conn, void *buf, uint32_t count,
uint64_t offset);
-extern int plugin_pwrite (struct connection *conn, void *buf, uint32_t count,
uint64_t offset);
+extern int plugin_pwrite (struct connection *conn, void *buf, uint32_t count,
uint64_t offset, int fua);
 extern int plugin_flush (struct connection *conn);
-extern int plugin_trim (struct connection *conn, uint32_t count, uint64_t
offset);
-extern int plugin_zero (struct connection *conn, uint32_t count, uint64_t
offset, int may_trim);
+extern int plugin_trim (struct connection *conn, uint32_t count, uint64_t
offset, int fua);
+extern int plugin_zero (struct connection *conn, uint32_t count, uint64_t
offset, int may_trim, int fua);

 /* sockets.c */
 extern int *bind_unix_socket (size_t *);
diff --git a/src/plugins.c b/src/plugins.c
index 9b5d2d5..61f0cc8 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013 Red Hat Inc.
+ * Copyright (C) 2013-2018 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -497,19 +497,26 @@ plugin_pread (struct connection *conn,

 int
 plugin_pwrite (struct connection *conn,
-               void *buf, uint32_t count, uint64_t offset)
+               void *buf, uint32_t count, uint64_t offset, int fua)
 {
+  int r;
   assert (dl);
   assert (connection_get_handle (conn));

-  debug ("pwrite count=%" PRIu32 " offset=%" PRIu64, count,
offset);
+  debug ("pwrite count=%" PRIu32 " offset=%" PRIu64 "
fua=%d", count, offset,
+         fua);

   if (plugin.pwrite != NULL)
-    return plugin.pwrite (connection_get_handle (conn), buf, count, offset);
+    r = plugin.pwrite (connection_get_handle (conn), buf, count, offset);
   else {
     errno = EROFS;
     return -1;
   }
+  if (r == 0 && fua) {
+    assert (plugin.flush);
+    r = plugin_flush (conn);
+  }
+  return r;
 }

 int
@@ -529,24 +536,31 @@ plugin_flush (struct connection *conn)
 }

 int
-plugin_trim (struct connection *conn, uint32_t count, uint64_t offset)
+plugin_trim (struct connection *conn, uint32_t count, uint64_t offset, int fua)
 {
+  int r;
   assert (dl);
   assert (connection_get_handle (conn));

-  debug ("trim count=%" PRIu32 " offset=%" PRIu64, count,
offset);
+  debug ("trim count=%" PRIu32 " offset=%" PRIu64 "
fua=%d", count, offset,
+         fua);

   if (plugin.trim != NULL)
-    return plugin.trim (connection_get_handle (conn), count, offset);
+    r = plugin.trim (connection_get_handle (conn), count, offset);
   else {
     errno = EINVAL;
     return -1;
   }
+  if (r == 0 && fua) {
+    assert (plugin.flush);
+    r = plugin_flush (conn);
+  }
+  return r;
 }

 int
 plugin_zero (struct connection *conn,
-             uint32_t count, uint64_t offset, int may_trim)
+             uint32_t count, uint64_t offset, int may_trim, int fua)
 {
   assert (dl);
   assert (connection_get_handle (conn));
@@ -555,8 +569,8 @@ plugin_zero (struct connection *conn,
   int result;
   int err = 0;

-  debug ("zero count=%" PRIu32 " offset=%" PRIu64 "
may_trim=%d",
-         count, offset, may_trim);
+  debug ("zero count=%" PRIu32 " offset=%" PRIu64 "
may_trim=%d fua=%d",
+         count, offset, may_trim, fua);

   if (!count)
     return 0;
@@ -569,7 +583,7 @@ plugin_zero (struct connection *conn,
         err = errno;
     }
     if (result == 0 || err != EOPNOTSUPP)
-      return result;
+      goto done;
   }

   assert (plugin.pwrite);
@@ -593,5 +607,11 @@ plugin_zero (struct connection *conn,
   err = errno;
   free (buf);
   errno = err;
+
+ done:
+  if (!result && fua) {
+    assert (plugin.flush);
+    result = plugin_flush (conn);
+  }
   return result;
 }
-- 
2.14.3

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 documentations for those
callbacks, although the actual implementation to take advantage of
the new callbacks will be in later patches.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-plugin.pod  | 77 +++++++++++++++++++++++++++++++++++++++++++++++--
 docs/nbdkit.pod         |  9 ++++--
 include/nbdkit-plugin.h |  8 ++++-
 3 files changed, 88 insertions(+), 6 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 9abf75f..cb936f7 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -359,7 +359,7 @@ If there is an error, C<.can_write> should call
C<nbdkit_error> with
 an error message and return C<-1>.

 This callback is not required.  If omitted, then we return true iff a
-C<.pwrite> callback has been defined.
+C<.pwrite> or C<.pwrite_fua> callback has been defined.

 =head2 C<.can_flush>

@@ -400,7 +400,28 @@ If there is an error, C<.can_trim> should call
C<nbdkit_error> with an
 error message and return C<-1>.

 This callback is not required.  If omitted, then we return true iff a
-C<.trim> callback has been defined.
+C<.trim> or C<.trim_fua> callback has been defined.
+
+=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 and trim
+requests.
+
+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.  If omitted, then we return true iff
+either the C<.pwrite_fua> callback has been defined, or if
C<.can_flush>
+returns true (in the latter case, FUA semantics are emulated by nbdkit
+calling C<.flush> before completing any write or trim operation with
+the FUA flag set).
+
+Note that if this defaults to true and C<.can_trim> also returns true,
+the plugin must provide either C<.flush> or C<.trim_fua> for
correct
+FUA semantics.

 =head2 C<.pread>

@@ -442,6 +463,21 @@ recovered from), C<.pwrite> should call
C<nbdkit_error> with an error
 message, and C<nbdkit_set_error> to record an appropriate error
 (unless C<errno> is sufficient), then return C<-1>.

+If the plugin can provide efficient Forced Unit Access (FUA) semantics,
+it should define C<.pwrite_fua> instead.
+
+=head2 C<.pwrite_fua>
+
+ int pwrite_fua (void *handle, const void *buf, uint32_t count, uint64_t
offset, int fua);
+
+This callback has the same requirements as C<.pwrite>, with the
+additional parameter C<fua> set to a non-zero value if the client
+wants FUA semantics (where the command must not return until the
+actions of the write have landed in persistent storage).  If the
+plugin cannot provide efficient FUA, but C<.can_flush> returns true
+and C<.can_fua> does not return false, then client requests for FUA
+semantics are emulated by nbdkit calling C<.flush>.
+
 =head2 C<.flush>

  int flush (void *handle);
@@ -455,6 +491,11 @@ If there is an error, C<.flush> should call
C<nbdkit_error> with an
 error message, and C<nbdkit_set_error> to record an appropriate error
 (unless C<errno> is sufficient), then return C<-1>.

+Note that C<.flush> can be called both by the client doing an explicit
+flush request, and by nbdkit when emulating Forced Unit Access (FUA)
+semantics after a write or trim where the plugin did not provide FUA
+callbacks (C<.pwrite_fua>, C<.zero_fua>, and C<.trim_fua>).
+
 =head2 C<.trim>

  int trim (void *handle, uint32_t count, uint64_t offset);
@@ -467,6 +508,21 @@ If there is an error, C<.trim> should call
C<nbdkit_error> with an
 error message, and C<nbdkit_set_error> to record an appropriate error
 (unless C<errno> is sufficient), then return C<-1>.

+If the plugin can provide efficient Forced Unit Access (FUA) semantics,
+it should define C<.trim_fua> instead.
+
+=head2 C<.trim_fua>
+
+ int trim_fua (void *handle, uint32_t count, uint64_t offset, int fua);
+
+This callback has the same requirements as C<.trim>, with the
+additional parameter C<fua> set to a non-zero value if the client
+wants FUA semantics (where the command must not return until the
+actions of the trim have landed in persistent storage).  If the plugin
+cannot provide efficient FUA, but C<.can_flush> returns true and
+C<.can_fua> does not return false, then client requests for FUA
+semantics are emulated by nbdkit calling C<.flush>.
+
 =head2 C<.zero>

  int zero (void *handle, uint32_t count, uint64_t offset, int may_trim);
@@ -488,6 +544,21 @@ If there is an error, C<.zero> should call
C<nbdkit_error> with an
 error message, and C<nbdkit_set_error> to record an appropriate error
 (unless C<errno> is sufficient), then return C<-1>.

+If the plugin can provide efficient Forced Unit Access (FUA) semantics,
+it should define C<.zero_fua> instead.
+
+=head2 C<.zero_fua>
+
+ int zero_fua (void *handle, uint32_t count, uint64_t offset, int may_trim, int
fua);
+
+This callback has the same requirements as C<.zero>, with the
+additional parameter C<fua> set to a non-zero value if the client
+wants FUA semantics (where the command must not return until the
+actions of the write have landed in persistent storage).  If the
+plugin cannot provide efficient FUA, but C<.can_flush> returns true
+and C<.can_fua> does not return false, then client requests for FUA
+semantics are emulated by nbdkit calling C<.flush>.
+
 =head1 THREADS

 Each nbdkit plugin must declare its thread safety model by defining
@@ -711,7 +782,7 @@ Pino Toscano

 =head1 COPYRIGHT

-Copyright (C) 2013-2017 Red Hat Inc.
+Copyright (C) 2013-2018 Red Hat Inc.

 =head1 LICENSE

diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index 1687ac9..2b868d9 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -785,7 +785,12 @@ information about that plugin, eg:
  [etc]

 Plugins which ship with nbdkit usually have the same version as the
-corresponding nbdkit binary.
+corresponding nbdkit binary.  The nbdkit binary will always be able
+to utilize plugins compiled against an older version of the header;
+however, there are cases where a newer plugin may not be fully
+supported by an older nbdkit binary (for example, a plugin that
+supplies C<.pwrite_fua> but not C<.pwrite> may not support writes
+when loaded by the older nbdkit).

 =head2 Detect if a plugin is installed

@@ -895,7 +900,7 @@ Pino Toscano

 =head1 COPYRIGHT

-Copyright (C) 2013-2017 Red Hat Inc.
+Copyright (C) 2013-2018 Red Hat Inc.

 =head1 LICENSE

diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 2ec3b15..d3b0050 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2017 Red Hat Inc.
+ * Copyright (C) 2013-2018 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -96,6 +96,12 @@ struct nbdkit_plugin {

   void (*dump_plugin) (void);

+  int (*can_fua) (void *handle);
+  int (*pwrite_fua) (void *handle, const void *buf, uint32_t count,
+                     uint64_t offset, int fua);
+  int (*zero_fua) (void *handle, uint32_t count, uint64_t offset, int may_trim,
+                   int fua);
+  int (*trim_fua) (void *handle, uint32_t count, uint64_t offset, int fua);
   /* int (*set_exportname) (void *handle, const char *exportname); */
 };

-- 
2.14.3

Implement the logic to actually use FUA callbacks defined by a
plugin.  This includes separating the tracking of advertising
flush vs. fua to clients, as it is feasible that a plugin might
support fua but not flush.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/connections.c | 15 ++++++++++++---
 src/internal.h    |  1 +
 src/plugins.c     | 54 ++++++++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index e4a0a82..2884f0e 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -79,6 +79,7 @@ struct connection {
   int is_rotational;
   int can_trim;
   int using_tls;
+  int can_fua;

   int sockin, sockout;
   connection_recv_function recv;
@@ -367,10 +368,18 @@ compute_eflags (struct connection *conn, uint16_t *flags)
   if (fl == -1)
     return -1;
   if (fl) {
-    eflags |= NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA;
+    eflags |= NBD_FLAG_SEND_FLUSH;
     conn->can_flush = 1;
   }

+  fl = plugin_can_fua (conn);
+  if (fl == -1)
+    return -1;
+  if (fl) {
+    eflags |= NBD_FLAG_SEND_FUA;
+    conn->can_fua = 1;
+  }
+
   fl = plugin_is_rotational (conn);
   if (fl == -1)
     return -1;
@@ -814,7 +823,7 @@ validate_request (struct connection *conn,
     *error = EINVAL;
     return false;
   }
-  if (!conn->can_flush && (flags & NBD_CMD_FLAG_FUA)) {
+  if (!conn->can_fua && (flags & NBD_CMD_FLAG_FUA)) {
     nbdkit_error ("invalid request: FUA flag not supported");
     *error = EINVAL;
     return false;
@@ -873,7 +882,7 @@ handle_request (struct connection *conn,
                 uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count,
                 void *buf)
 {
-  bool fua = conn->can_flush && (flags & NBD_CMD_FLAG_FUA);
+  bool fua = conn->can_fua && (flags & NBD_CMD_FLAG_FUA);

   /* The plugin should call nbdkit_set_error() to request a particular
      error, otherwise we fallback to errno or EIO. */
diff --git a/src/internal.h b/src/internal.h
index 3ab08d3..88a2eb8 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -163,6 +163,7 @@ extern int plugin_can_write (struct connection *conn);
 extern int plugin_can_flush (struct connection *conn);
 extern int plugin_is_rotational (struct connection *conn);
 extern int plugin_can_trim (struct connection *conn);
+extern int plugin_can_fua (struct connection *conn);
 extern int plugin_pread (struct connection *conn, void *buf, uint32_t count,
uint64_t offset);
 extern int plugin_pwrite (struct connection *conn, void *buf, uint32_t count,
uint64_t offset, int fua);
 extern int plugin_flush (struct connection *conn);
diff --git a/src/plugins.c b/src/plugins.c
index 61f0cc8..93cc535 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -271,6 +271,10 @@ plugin_dump_fields (void)
   HAS (flush);
   HAS (trim);
   HAS (zero);
+  HAS (can_fua);
+  HAS (pwrite_fua);
+  HAS (zero_fua);
+  HAS (trim_fua);
 #undef HAS

   /* Custom fields. */
@@ -437,7 +441,7 @@ plugin_can_write (struct connection *conn)
   if (plugin.can_write)
     return plugin.can_write (connection_get_handle (conn));
   else
-    return plugin.pwrite != NULL;
+    return plugin.pwrite || plugin.pwrite_fua;
 }

 int
@@ -454,6 +458,21 @@ plugin_can_flush (struct connection *conn)
     return plugin.flush != NULL;
 }

+int
+plugin_can_fua (struct connection *conn)
+{
+  assert (dl);
+  assert (connection_get_handle (conn));
+
+  debug ("can_fua");
+
+  if (plugin.can_fua)
+    return plugin.can_fua (connection_get_handle (conn));
+  if (plugin.pwrite_fua)
+    return 1;
+  return plugin_can_flush (conn);
+}
+
 int
 plugin_is_rotational (struct connection *conn)
 {
@@ -506,7 +525,12 @@ plugin_pwrite (struct connection *conn,
   debug ("pwrite count=%" PRIu32 " offset=%" PRIu64 "
fua=%d", count, offset,
          fua);

-  if (plugin.pwrite != NULL)
+  if (plugin.pwrite_fua) {
+    r = plugin.pwrite_fua (connection_get_handle (conn), buf, count, offset,
+                           fua);
+    fua = 0;
+  }
+  else if (plugin.pwrite != NULL)
     r = plugin.pwrite (connection_get_handle (conn), buf, count, offset);
   else {
     errno = EROFS;
@@ -545,7 +569,11 @@ plugin_trim (struct connection *conn, uint32_t count,
uint64_t offset, int fua)
   debug ("trim count=%" PRIu32 " offset=%" PRIu64 "
fua=%d", count, offset,
          fua);

-  if (plugin.trim != NULL)
+  if (plugin.trim_fua) {
+    r = plugin.trim_fua (connection_get_handle (conn), count, offset, fua);
+    fua = 0;
+  }
+  else if (plugin.trim)
     r = plugin.trim (connection_get_handle (conn), count, offset);
   else {
     errno = EINVAL;
@@ -574,19 +602,25 @@ plugin_zero (struct connection *conn,

   if (!count)
     return 0;
-  if (plugin.zero) {
+  if (plugin.zero || plugin.zero_fua) {
     errno = 0;
-    result = plugin.zero (connection_get_handle (conn), count, offset,
may_trim);
+    result = plugin.zero_fua
+      ? plugin.zero_fua (connection_get_handle (conn), count, offset, may_trim,
+                         fua)
+      : plugin.zero (connection_get_handle (conn), count, offset, may_trim);
     if (result == -1) {
       err = threadlocal_get_error ();
       if (!err && plugin_errno_is_preserved ())
         err = errno;
     }
-    if (result == 0 || err != EOPNOTSUPP)
+    if (result == 0 || err != EOPNOTSUPP) {
+      if (plugin.zero_fua)
+        fua = 0;
       goto done;
+    }
   }

-  assert (plugin.pwrite);
+  assert (plugin.pwrite || plugin.pwrite_fua);
   threadlocal_set_error (0);
   limit = count < MAX_REQUEST_SIZE ? count : MAX_REQUEST_SIZE;
   buf = calloc (limit, 1);
@@ -595,14 +629,18 @@ plugin_zero (struct connection *conn,
     return -1;
   }

+  /* If we have to emulate FUA, we can flush once at the end rather
+     than on each write iteration */
   while (count) {
-    result = plugin.pwrite (connection_get_handle (conn), buf, limit, offset);
+    result = plugin_pwrite (conn, buf, limit, offset, fua &&
plugin.pwrite_fua);
     if (result < 0)
       break;
     count -= limit;
     if (count < limit)
       limit = count;
   }
+  if (plugin.pwrite_fua)
+    fua = 0;

   err = errno;
   free (buf);
-- 
2.14.3

If the target server supports FUA, then we should pass the client
flag through rather than emulating things with a less-efficient
flush.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/nbd/nbd.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index c9727f7..09cefe5 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -616,6 +616,14 @@ nbd_can_trim (void *handle)
   return h->flags & NBD_FLAG_SEND_TRIM;
 }

+static int
+nbd_can_fua (void *handle)
+{
+  struct handle *h = handle;
+
+  return h->flags & NBD_FLAG_SEND_FUA;
+}
+
 /* Read data from the file. */
 static int
 nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset)
@@ -631,23 +639,26 @@ nbd_pread (void *handle, void *buf, uint32_t count,
uint64_t offset)

 /* Write data to the file. */
 static int
-nbd_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset)
+nbd_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
+            int fua)
 {
   struct handle *h = handle;
   int c;

   /* TODO Auto-fragment this if the client has a larger max transfer
      limit than the server */
-  c = nbd_request_full (h, 0, NBD_CMD_WRITE, offset, count, buf, NULL);
+  c = nbd_request_full (h, fua ? NBD_CMD_FLAG_FUA : 0,
+                        NBD_CMD_WRITE, offset, count, buf, NULL);
   return c < 0 ? c : nbd_reply (h, c);
 }

 /* Write zeroes to the file. */
 static int
-nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
+nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim, int fua)
 {
   struct handle *h = handle;
   int c;
+  int flags = 0;

   if (!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES)) {
     /* Trigger a fall back to regular writing */
@@ -655,19 +666,22 @@ nbd_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
     return -1;
   }

-  c = nbd_request (h, may_trim ? 0 : NBD_CMD_FLAG_NO_HOLE,
-                   NBD_CMD_WRITE_ZEROES, offset, count);
+  if (!may_trim)
+    flags |= NBD_CMD_FLAG_NO_HOLE;
+  if (fua)
+    flags |= NBD_CMD_FLAG_FUA;
+  c = nbd_request (h, flags, NBD_CMD_WRITE_ZEROES, offset, count);
   return c < 0 ? c : nbd_reply (h, c);
 }

 /* Trim a portion of the file. */
 static int
-nbd_trim (void *handle, uint32_t count, uint64_t offset)
+nbd_trim (void *handle, uint32_t count, uint64_t offset, int fua)
 {
   struct handle *h = handle;
   int c;

-  c = nbd_request (h, 0, NBD_CMD_TRIM, offset, count);
+  c = nbd_request (h, fua ? NBD_CMD_FLAG_FUA : 0, NBD_CMD_TRIM, offset, count);
   return c < 0 ? c : nbd_reply (h, c);
 }

@@ -697,11 +711,12 @@ static struct nbdkit_plugin plugin = {
   .can_flush          = nbd_can_flush,
   .is_rotational      = nbd_is_rotational,
   .can_trim           = nbd_can_trim,
+  .can_fua            = nbd_can_fua,
   .pread              = nbd_pread,
-  .pwrite             = nbd_pwrite,
-  .zero               = nbd_zero,
+  .pwrite_fua         = nbd_pwrite,
+  .zero_fua           = nbd_zero,
   .flush              = nbd_flush,
-  .trim               = nbd_trim,
+  .trim_fua           = nbd_trim,
   .errno_is_preserved = 1,
 };

-- 
2.14.3

New nbdkit already knows how to gracefully handle a plugin that
defines .pwrite_fua but not .pwrite, making it possible to write
a plugin that does not have to provide duplicate boilerplate
code.  But for maximum cross-version compatibility, we must make
sure that older nbdkit, which doesn't know about the existance
of the .pwrite_fua callback, can still perform writes when loading
such a newer plugin.  The trick is to make our registration
macro provide sane fallback definitions, marked such that gcc
won't warn if the fallbacks are not needed.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit.pod         | 11 +++++------
 include/nbdkit-plugin.h | 26 ++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index 2b868d9..a72c9b1 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -785,12 +785,11 @@ information about that plugin, eg:
  [etc]

 Plugins which ship with nbdkit usually have the same version as the
-corresponding nbdkit binary.  The nbdkit binary will always be able
-to utilize plugins compiled against an older version of the header;
-however, there are cases where a newer plugin may not be fully
-supported by an older nbdkit binary (for example, a plugin that
-supplies C<.pwrite_fua> but not C<.pwrite> may not support writes
-when loaded by the older nbdkit).
+corresponding nbdkit binary.  The nbdkit binary will always be able to
+utilize plugins compiled against an older version of the header; the
+converse direction of an older nbdkit driving a newer plugin is not
+guaranteed to work, although the design tries hard to preserve
+back-compatibility.

 =head2 Detect if a plugin is installed

diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index d3b0050..0c6e67e 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -117,6 +117,12 @@ extern char *nbdkit_absolute_path (const char *path);
 extern int64_t nbdkit_parse_size (const char *str);
 extern int nbdkit_read_password (const char *value, char **password);

+#ifdef __GNUC__
+#define NBDKIT_UNUSED __attribute__ (( __unused__ ))
+#else
+#define NBDKIT_UNUSED /* empty */
+#endif
+
 #ifdef __cplusplus
 #define NBDKIT_CXX_LANG_C extern "C"
 #else
@@ -124,6 +130,20 @@ extern int nbdkit_read_password (const char *value, char
**password);
 #endif

 #define NBDKIT_REGISTER_PLUGIN(plugin)                                  \
+  static int NBDKIT_UNUSED                                              \
+  nbdkit_pwrite_no_fua (void *handle, const void *buf, uint32_t count,  \
+                        uint64_t offset) {                              \
+    return (plugin).pwrite_fua (handle, buf, count, offset, 0);         \
+  }                                                                     \
+  static int NBDKIT_UNUSED                                              \
+  nbdkit_zero_no_fua (void *handle, uint32_t count, uint64_t offset,    \
+                      int may_trim) {                                   \
+    return (plugin).zero_fua (handle, count, offset, may_trim, 0);      \
+  }                                                                     \
+  static int NBDKIT_UNUSED                                              \
+  nbdkit_trim_no_fua (void *handle, uint32_t count, uint64_t offset) {  \
+    return (plugin).trim_fua (handle, count, offset, 0);                \
+  }                                                                     \
   NBDKIT_CXX_LANG_C                                                     \
   struct nbdkit_plugin *                                                \
   plugin_init (void)                                                    \
@@ -131,6 +151,12 @@ extern int nbdkit_read_password (const char *value, char
**password);
     (plugin)._struct_size = sizeof (plugin);                            \
     (plugin)._api_version = NBDKIT_API_VERSION;                         \
     (plugin)._thread_model = THREAD_MODEL;                              \
+    if ((plugin).pwrite_fua && !(plugin).pwrite)                       
\
+      (plugin).pwrite = nbdkit_pwrite_no_fua;                           \
+    if ((plugin).zero_fua && !(plugin).zero)                           
\
+      (plugin).zero = nbdkit_zero_no_fua;                               \
+    if ((plugin).trim_fua && !(plugin).trim)                           
\
+      (plugin).trim = nbdkit_trim_no_fua;                               \
     return &(plugin);                                                   \
   }

-- 
2.14.3

I had an idea about how we might do this and still keep a single
.pwrite method for plugins.  Maybe it's too complicated but here goes:


(1) Rename the pwrite method as pwrite_old1, and add new pwrite method
with either the FUA flag or general flags:

   int (*pread) (void *handle, void *buf, uint32_t count, uint64_t offset);
-  int (*pwrite) (void *handle, const void *buf, uint32_t count, uint64_t
offset);
+  int (*pwrite_old1) (void *handle, const void *buf, uint32_t count, uint64_t
offset);
   int (*flush) (void *handle);
   int (*trim) (void *handle, uint32_t count, uint64_t offset);
   int (*zero) (void *handle, uint32_t count, uint64_t offset, int may_trim);
 
   int errno_is_preserved;
 
   void (*dump_plugin) (void);
 
+  int (*pwrite) (void *handle, const void *buf, uint32_t count, uint64_t
offset, unsigned flags);


This is binary-compatible with old plugins, but not source compatible, so:


(2) Plugsin may optionally define NBDKIT_PLUGIN_LEVEL before including
<nbdkit-plugin.h>.  Code wishing to use the flags variant of pwrite
can do:

#define NBDKIT_PLUGIN_LEVEL 2
#include <nbdkit-plugin.h>

which would modify the definition of the struct (again), something
like:

#ifndef NBDKIT_PLUGIN_LEVEL
#define NBDKIT_PLUGIN_LEVEL 1
#endif

 ...
 
  int (*pread) (void *handle, void *buf, uint32_t count, uint64_t offset);
#if NBDKIT_PLUGIN_LEVEL <= 1
  int (*pwrite) (void *handle, const void *buf, uint32_t count, uint64_t
offset);
#else
  int (*pwrite_old1) (void *handle, const void *buf, uint32_t count, uint64_t
offset);
#endif
  int (*flush) (void *handle);
  int (*trim) (void *handle, uint32_t count, uint64_t offset);
  int (*zero) (void *handle, uint32_t count, uint64_t offset, int may_trim);

  int errno_is_preserved;

  void (*dump_plugin) (void);

#if NBDKIT_PLUGIN_LEVEL <= 1
  int (*never_used1) (void *handle, const void *buf, uint32_t count, uint64_t
offset, unsigned flags);
#else
  int (*pwrite) (void *handle, const void *buf, uint32_t count, uint64_t offset,
unsigned flags);
#endif


(3) Core nbdkit code always defines NBDKIT_PLUGIN_LEVEL == 2 so that
it always sees the new function, but it may need to call the old
function:

int
plugin_pwrite (...)
{
  if (plugin.pwrite != NULL)
    plugin.pwrite (handle, buf, count, offset, flags);
  else if (plugin.pwrite_old1 != NULL)
    plugin.pwrite_old1 (handle, buf, count, offset);
}


(4) Internal plugins which don't need the FUA flag can continue to use
the level 1 API, which means that we keep checking that the old API
still works.


What do you think?

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 01/16/2018 02:25 AM, Richard W.M. Jones wrote:
> I had an idea about how we might do this and still keep a single
> .pwrite method for plugins.  Maybe it's too complicated but here goes:
> 
> 
> This is binary-compatible with old plugins, but not source compatible, so:
> 
> 
> (2) Plugsin may optionally define NBDKIT_PLUGIN_LEVEL before including
> <nbdkit-plugin.h>.  Code wishing to use the flags variant of pwrite
> can do:
> 
> #define NBDKIT_PLUGIN_LEVEL 2
> #include <nbdkit-plugin.h>
> 
> which would modify the definition of the struct (again), something
> like:
> 
> #ifndef NBDKIT_PLUGIN_LEVEL
> #define NBDKIT_PLUGIN_LEVEL 1
> #endif
So the difference between your proposal and my patch 4 is that in your
proposal, a new plugin has to explicitly opt-in to the new ABI by
defining NBDKIT_PLUGIN_LEVEL 2, while in mine, a new plugin opts-in by
assigning to .pwrite_fua instead of .pwrite.  Yours is slightly cleaner
in making it obvious that there is only one (and not two) pwrite
callback per plugin, and either way, the plugin chooses which variant it
wants.
> (3) Core nbdkit code always defines NBDKIT_PLUGIN_LEVEL == 2 so that
> it always sees the new function, but it may need to call the old
> function:
> 
> int
> plugin_pwrite (...)
> {
>   if (plugin.pwrite != NULL)
>     plugin.pwrite (handle, buf, count, offset, flags);
>   else if (plugin.pwrite_old1 != NULL)
>     plugin.pwrite_old1 (handle, buf, count, offset);
> }
This part happens whether by your proposal or by mine (see patch 5) - a
new nbdkit is always able to call the correct one out of two callbacks
for both old and new plugins.

Both proposals are still able to install a shim for the old name when
the plugin uses only the new semantics; if I use your proposal, my patch
7 would be rewritten to only create the shims when a plugin requests
NBDKIT_PLUGIN_LEVEL 2 (whereas by my proposal, I had to define the shims
always).

Documentation may be a bit tricky under your proposal, but I'm not
seeing it as much different than mine.  Then there's the decision of
whether to go with a single flags parameter (closer to the wire
protocol, but the plugin has to decode bits; and hopefully we do proper
feature advertisement so that a client never passes a flag that the
plugin won't understand) or with multiple bool parameters (what my
proposal did for .pwrite_fua).
> 
> 
> (4) Internal plugins which don't need the FUA flag can continue to use
> the level 1 API, which means that we keep checking that the old API
> still works.
My proposal did that as well - most plugins still used the older
spelling without the fua flag.
> 
> 
> What do you think?
I don't see any fundamental differences in end behavior, so much as
which style we find nicer to document and expose to plugin writers.  I'm
happy to go with whichever of the two styles you prefer.

The other thing to consider is how we translate all of this to other
language bindings.  For languages that allow function overloading, I
suppose we want the declaration of 'pwrite' to take either function
signature, then the glue code that converts back to C knows based on the
function signature whether the plugin is written for old- or new-style
API; for languages without function overloading, they will have to
mirror some way of declaring which of the two APIs they are interested in.

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