Libguestfs - Aug 2019 - [nbdkit PATCH 2/2] nozero: More efficient FUA handling

Another couple things I noticed that are worth improving, but aren't
strictly related to implementing fast zero support.

Eric Blake (2):
  server: Assert sane error responses
  nozero: More efficient FUA handling

 filters/nozero/nozero.c | 17 +++++++++++--
 server/filters.c        | 56 +++++++++++++++++++++++++++++++++--------
 server/protocol.c       | 32 +++++++++++++++++------
 3 files changed, 84 insertions(+), 21 deletions(-)

-- 
2.20.1

Our filters document that calling through next_ops should reliably set
*err on failure, and in turn that the filter must set *err if
returning -1.  Since plugins.c wrapper ensures that *err is always set
regardless of the plugin, the only place where *err could be left
unset is in a broken filter.  Our documentation also states that a
filter must never observe or return EOPNOTSUPP from a failed .zero,
since any fallback to .pwrite should have already happened (this
restriction will be changed in future patches when the new
NBD_CMD_FLAG_FAST_ZERO is added, but for now it is worth making sure
we obey).  Since filters are always in-tree, it is appropriate to
assert that none of our backends are forgetting to set *err correctly.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/filters.c  | 56 +++++++++++++++++++++++++++++++++++++----------
 server/protocol.c | 32 ++++++++++++++++++++-------
 2 files changed, 69 insertions(+), 19 deletions(-)

diff --git a/server/filters.c b/server/filters.c
index 3d9e1efa..14ca0cc6 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -347,8 +347,13 @@ next_pread (void *nxdata, void *buf, uint32_t count,
uint64_t offset,
             uint32_t flags, int *err)
 {
   struct b_conn *b_conn = nxdata;
-  return b_conn->b->pread (b_conn->b, b_conn->conn, buf, count,
offset, flags,
-                           err);
+  int r;
+
+  r = b_conn->b->pread (b_conn->b, b_conn->conn, buf, count,
offset, flags,
+                        err);
+  if (r == -1)
+    assert (*err);
+  return 1;
 }

 static int
@@ -356,15 +361,25 @@ next_pwrite (void *nxdata, const void *buf, uint32_t
count, uint64_t offset,
              uint32_t flags, int *err)
 {
   struct b_conn *b_conn = nxdata;
-  return b_conn->b->pwrite (b_conn->b, b_conn->conn, buf, count,
offset, flags,
-                            err);
+  int r;
+
+  r = b_conn->b->pwrite (b_conn->b, b_conn->conn, buf, count,
offset, flags,
+                         err);
+  if (r == -1)
+    assert (*err);
+  return r;
 }

 static int
 next_flush (void *nxdata, uint32_t flags, int *err)
 {
   struct b_conn *b_conn = nxdata;
-  return b_conn->b->flush (b_conn->b, b_conn->conn, flags, err);
+  int r;
+
+  r = b_conn->b->flush (b_conn->b, b_conn->conn, flags, err);
+  if (r == -1)
+    assert (*err);
+  return r;
 }

 static int
@@ -372,7 +387,12 @@ next_trim (void *nxdata, uint32_t count, uint64_t offset,
uint32_t flags,
            int *err)
 {
   struct b_conn *b_conn = nxdata;
-  return b_conn->b->trim (b_conn->b, b_conn->conn, count, offset,
flags, err);
+  int r;
+
+  r = b_conn->b->trim (b_conn->b, b_conn->conn, count, offset,
flags, err);
+  if (r == -1)
+    assert (*err);
+  return r;
 }

 static int
@@ -380,7 +400,12 @@ next_zero (void *nxdata, uint32_t count, uint64_t offset,
uint32_t flags,
            int *err)
 {
   struct b_conn *b_conn = nxdata;
-  return b_conn->b->zero (b_conn->b, b_conn->conn, count, offset,
flags, err);
+  int r;
+
+  r = b_conn->b->zero (b_conn->b, b_conn->conn, count, offset,
flags, err);
+  if (r == -1)
+    assert (*err && *err != ENOTSUP && *err != EOPNOTSUPP);
+  return r;
 }

 static int
@@ -388,8 +413,13 @@ next_extents (void *nxdata, uint32_t count, uint64_t
offset, uint32_t flags,
               struct nbdkit_extents *extents, int *err)
 {
   struct b_conn *b_conn = nxdata;
-  return b_conn->b->extents (b_conn->b, b_conn->conn, count,
offset, flags,
-                             extents, err);
+  int r;
+
+  r = b_conn->b->extents (b_conn->b, b_conn->conn, count, offset,
flags,
+                          extents, err);
+  if (r == -1)
+    assert (*err);
+  return r;
 }

 static int
@@ -397,8 +427,12 @@ next_cache (void *nxdata, uint32_t count, uint64_t offset,
             uint32_t flags, int *err)
 {
   struct b_conn *b_conn = nxdata;
-  return b_conn->b->cache (b_conn->b, b_conn->conn, count, offset,
flags,
-                           err);
+  int r;
+
+  r = b_conn->b->cache (b_conn->b, b_conn->conn, count, offset,
flags, err);
+  if (r == -1)
+    assert (*err);
+  return r;
 }

 static struct nbdkit_next_ops next_ops = {
diff --git a/server/protocol.c b/server/protocol.c
index 59676224..72f6f2a8 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -240,27 +240,35 @@ handle_request (struct connection *conn,

   switch (cmd) {
   case NBD_CMD_READ:
-    if (backend->pread (backend, conn, buf, count, offset, 0, &err) ==
-1)
+    if (backend->pread (backend, conn, buf, count, offset, 0, &err) ==
-1) {
+      assert (err);
       return err;
+    }
     break;

   case NBD_CMD_WRITE:
     if (fua)
       f |= NBDKIT_FLAG_FUA;
-    if (backend->pwrite (backend, conn, buf, count, offset, f, &err) ==
-1)
+    if (backend->pwrite (backend, conn, buf, count, offset, f, &err) ==
-1) {
+      assert (err);
       return err;
+    }
     break;

   case NBD_CMD_FLUSH:
-    if (backend->flush (backend, conn, 0, &err) == -1)
+    if (backend->flush (backend, conn, 0, &err) == -1) {
+      assert (err);
       return err;
+    }
     break;

   case NBD_CMD_TRIM:
     if (fua)
       f |= NBDKIT_FLAG_FUA;
-    if (backend->trim (backend, conn, count, offset, f, &err) == -1)
+    if (backend->trim (backend, conn, count, offset, f, &err) == -1) {
+      assert (err);
       return err;
+    }
     break;

   case NBD_CMD_CACHE:
@@ -271,13 +279,17 @@ handle_request (struct connection *conn,
       while (count) {
         limit = MIN (count, sizeof buf);
         if (backend->pread (backend, conn, buf, limit, offset, flags,
-                            &err) == -1)
+                            &err) == -1) {
+          assert (err);
           return err;
+        }
         count -= limit;
       }
     }
-    else if (backend->cache (backend, conn, count, offset, 0, &err) ==
-1)
+    else if (backend->cache (backend, conn, count, offset, 0, &err) ==
-1) {
+      assert (err);
       return err;
+    }
     break;

   case NBD_CMD_WRITE_ZEROES:
@@ -285,8 +297,10 @@ handle_request (struct connection *conn,
       f |= NBDKIT_FLAG_MAY_TRIM;
     if (fua)
       f |= NBDKIT_FLAG_FUA;
-    if (backend->zero (backend, conn, count, offset, f, &err) == -1)
+    if (backend->zero (backend, conn, count, offset, f, &err) == -1) {
+      assert (err && err != ENOTSUP && err != EOPNOTSUPP);
       return err;
+    }
     break;

   case NBD_CMD_BLOCK_STATUS:
@@ -299,8 +313,10 @@ handle_request (struct connection *conn,
       if (flags & NBD_CMD_FLAG_REQ_ONE)
         f |= NBDKIT_FLAG_REQ_ONE;
       if (backend->extents (backend, conn, count, offset, f,
-                            extents, &err) == -1)
+                            extents, &err) == -1) {
+        assert (err);
         return err;
+      }
     }
     else {
       int r;
-- 
2.20.1

The nozero filter can split a large request with FUA into several
smaller requests; optimize whether the FUA flag is passed on to the
next layer based on whether FUA is emulated with flush (only the last
write needs it) or is natively supported (every write needs it).
Missed in commit df0cc21d.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 filters/nozero/nozero.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/filters/nozero/nozero.c b/filters/nozero/nozero.c
index 035caae3..964cce9f 100644
--- a/filters/nozero/nozero.c
+++ b/filters/nozero/nozero.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2018 Red Hat Inc.
+ * Copyright (C) 2018-2019 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -102,12 +102,22 @@ nozero_zero (struct nbdkit_next_ops *next_ops, void
*nxdata,
              void *handle, uint32_t count, uint64_t offs, uint32_t flags,
              int *err)
 {
+  int writeflags = 0;
+  bool need_flush = false;
+
   assert (zeromode != NONE);
   flags &= ~NBDKIT_FLAG_MAY_TRIM;

   if (zeromode == NOTRIM)
     return next_ops->zero (nxdata, count, offs, flags, err);

+  if (flags & NBDKIT_FLAG_FUA) {
+    if (next_ops->can_fua (nxdata) == NBDKIT_FUA_EMULATE)
+      need_flush = true;
+    else
+      writeflags = NBDKIT_FLAG_FUA;
+  }
+
   while (count) {
     /* Always contains zeroes, but we can't use const or else gcc 9
      * will use .rodata instead of .bss and inflate the binary size.
@@ -115,7 +125,10 @@ nozero_zero (struct nbdkit_next_ops *next_ops, void
*nxdata,
     static /* const */ char buffer[MAX_WRITE];
     uint32_t size = MIN (count, MAX_WRITE);

-    if (next_ops->pwrite (nxdata, buffer, size, offs, flags, err) == -1)
+    if (size == count && need_flush)
+      writeflags = NBDKIT_FLAG_FUA;
+
+    if (next_ops->pwrite (nxdata, buffer, size, offs, writeflags, err) ==
-1)
       return -1;
     offs += size;
     count -= size;
-- 
2.20.1

ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org