Libguestfs - May 2019 - [nbdkit PATCH] nozero: Add notrim mode

It may be useful to test whether the client's use of
NBD_CMD_FLAG_NO_HOLE makes a difference; do this by adding a mode to
--filter=nozero to force a non-trimming zero write.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 filters/nozero/nbdkit-nozero-filter.pod | 19 +++++++----
 filters/nozero/nozero.c                 | 45 ++++++++++++++++++++-----
 tests/test-nozero.sh                    | 27 +++++++++++++--
 3 files changed, 74 insertions(+), 17 deletions(-)

diff --git a/filters/nozero/nbdkit-nozero-filter.pod
b/filters/nozero/nbdkit-nozero-filter.pod
index 7e06570..8e694bb 100644
--- a/filters/nozero/nbdkit-nozero-filter.pod
+++ b/filters/nozero/nbdkit-nozero-filter.pod
@@ -18,14 +18,19 @@ testing client or server fallbacks.

 =over 4

-=item B<zeromode=none|emulate>
+=item B<zeromode=none|emulate|notrim>

 Optional, controls which mode the filter will use.  Mode B<none>
-(default) means that zero support is not advertised to the client;
-mode B<emulate> means that zero support is emulated by the filter
-using the plugin's C<pwrite> callback, regardless of whether the
-plugin itself implemented the C<zero> callback with a more efficient
-way to write zeroes.
+(default) means that zero support is not advertised to the
+client. Mode B<emulate> means that zero support is emulated by the
+filter using the plugin's C<pwrite> callback, regardless of whether
+the plugin itself implemented the C<zero> callback with a more
+efficient way to write zeros. Since nbdkit E<ge> 1.13.4, mode
+B<notrim> means that zero requests are forwarded on to the plugin,
+except that the plugin will never see the NBDKIT_MAY_TRIM flag, to
+determine if the client permitting trimming during zero operations
+makes a difference (it is an error to request this mode if the plugin
+does not support the C<zero> callback).

 =back

@@ -55,4 +60,4 @@ Eric Blake

 =head1 COPYRIGHT

-Copyright (C) 2018 Red Hat Inc.
+Copyright (C) 2018-2019 Red Hat Inc.
diff --git a/filters/nozero/nozero.c b/filters/nozero/nozero.c
index 3ec6346..6e0ffa9 100644
--- a/filters/nozero/nozero.c
+++ b/filters/nozero/nozero.c
@@ -47,8 +47,12 @@

 #define MAX_WRITE (64 * 1024 * 1024)

-static char buffer[MAX_WRITE];
-static bool emulate;
+static const char buffer[MAX_WRITE];
+static enum ZeroMode {
+  NONE,
+  EMULATE,
+  NOTRIM,
+} zeromode;

 static int
 nozero_config (nbdkit_next_config *next, void *nxdata,
@@ -56,7 +60,9 @@ nozero_config (nbdkit_next_config *next, void *nxdata,
 {
   if (strcmp (key, "zeromode") == 0) {
     if (strcmp (value, "emulate") == 0)
-      emulate = true;
+      zeromode = EMULATE;
+    else if (strcmp (value, "notrim") == 0)
+      zeromode = NOTRIM;
     else if (strcmp (value, "none") != 0) {
       nbdkit_error ("unknown zeromode '%s'", value);
       return -1;
@@ -67,13 +73,31 @@ nozero_config (nbdkit_next_config *next, void *nxdata,
 }

 #define nozero_config_help \
-  "zeromode=<MODE>      Either 'none' (default) or
'emulate'.\n" \
+  "zeromode=<MODE>      Either 'none' (default),
'emulate', or 'notrim'.\n" \
+
+/* Check that desired mode is supported by plugin. */
+static int
+nozero_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle)
+{
+  int r;
+
+  if (zeromode == NOTRIM) {
+    r = next_ops->can_zero (nxdata);
+    if (r == -1)
+      return -1;
+    if (!r) {
+      nbdkit_error ("zeromode 'notrim' requires plugin zero
support");
+      return -1;
+    }
+  }
+  return 0;
+}

 /* Advertise desired WRITE_ZEROES mode. */
 static int
 nozero_can_zero (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle)
 {
-  return emulate;
+  return zeromode != NONE;
 }

 static int
@@ -81,11 +105,15 @@ nozero_zero (struct nbdkit_next_ops *next_ops, void
*nxdata,
              void *handle, uint32_t count, uint64_t offs, uint32_t flags,
              int *err)
 {
-  assert (emulate);
+  assert (zeromode != NONE);
+  flags &= ~NBDKIT_FLAG_MAY_TRIM;
+
+  if (zeromode == NOTRIM)
+    return next_ops->zero (nxdata, count, offs, flags, err);
+
   while (count) {
     uint32_t size = MIN (count, MAX_WRITE);
-    if (next_ops->pwrite (nxdata, buffer, size, offs,
-                          flags & ~NBDKIT_FLAG_MAY_TRIM, err) == -1)
+    if (next_ops->pwrite (nxdata, buffer, size, offs, flags, err) == -1)
       return -1;
     offs += size;
     count -= size;
@@ -99,6 +127,7 @@ static struct nbdkit_filter filter = {
   .version           = PACKAGE_VERSION,
   .config            = nozero_config,
   .config_help       = nozero_config_help,
+  .prepare           = nozero_prepare,
   .can_zero          = nozero_can_zero,
   .zero              = nozero_zero,
 };
diff --git a/tests/test-nozero.sh b/tests/test-nozero.sh
index 1282586..fc22420 100755
--- a/tests/test-nozero.sh
+++ b/tests/test-nozero.sh
@@ -39,12 +39,14 @@ sock3=`mktemp -u`
 sock4=`mktemp -u`
 sock5a=`mktemp -u`
 sock5b=`mktemp -u`
+sock6=`mktemp -u`
 files="nozero1.img nozero1.log $sock1 nozero1.pid
        nozero2.img nozero2.log $sock2 nozero2.pid
        nozero3.img nozero3.log $sock3 nozero3.pid
        nozero4.img nozero4.log $sock4 nozero4.pid
        nozero5.img nozero5a.log nozero5b.log $sock5a $sock5b
-       nozero5a.pid nozero5b.pid"
+       nozero5a.pid nozero5b.pid
+       nozero6.img nozero6.log $sock6 nozero6.pid"
 rm -f $files

 # Prep images, and check that qemu-io understands the actions we plan on
@@ -54,6 +56,7 @@ cp nozero1.img nozero2.img
 cp nozero1.img nozero3.img
 cp nozero1.img nozero4.img
 cp nozero1.img nozero5.img
+cp nozero1.img nozero6.img
 if ! qemu-io -f raw -d unmap -c 'w -z -u 0 1M' nozero1.img; then
     echo "$0: missing or broken qemu-io"
     rm nozero?.img
@@ -82,17 +85,20 @@ cleanup ()
     cat nozero5a.log || :
     echo "Log 5b file contents:"
     cat nozero5b.log || :
+    echo "Log 6 file contents:"
+    cat nozero6.log || :
     rm -f $files
 }
 cleanup_fn cleanup

-# Run four parallel nbdkit; to compare the logs and see what changes.
+# Run several parallel nbdkit; to compare the logs and see what changes.
 # 1: unfiltered, to check that qemu-io sends ZERO request and plugin trims
 # 2: log before filter with zeromode=none (default), to ensure no ZERO request
 # 3: log before filter with zeromode=emulate, to ensure ZERO from client
 # 4: log after filter with zeromode=emulate, to ensure no ZERO to plugin
 # 5a/b: both sides of nbd plugin: even though server side does not advertise
 # ZERO, the client side still exposes it, and just skips calling nbd's
.zero
+# 6: log after filter with zeromode=notrim, to ensure plugin does not trim
 start_nbdkit -P nozero1.pid -U $sock1 --filter=log \
        file logfile=nozero1.log nozero1.img
 start_nbdkit -P nozero2.pid -U $sock2 --filter=log --filter=nozero \
@@ -105,6 +111,8 @@ start_nbdkit -P nozero5a.pid -U $sock5a --filter=log
--filter=nozero \
        file logfile=nozero5a.log nozero5.img
 start_nbdkit -P nozero5b.pid -U $sock5b --filter=log \
        nbd logfile=nozero5b.log socket=$sock5a
+start_nbdkit -P nozero6.pid -U $sock6 --filter=nozero --filter=log \
+       file logfile=nozero6.log nozero6.img zeromode=notrim

 # Perform the zero write.
 qemu-io -f raw -c 'w -z -u 0 1M' "nbd+unix://?socket=$sock1"
@@ -112,6 +120,7 @@ qemu-io -f raw -c 'w -z -u 0 1M'
"nbd+unix://?socket=$sock2"
 qemu-io -f raw -c 'w -z -u 0 1M' "nbd+unix://?socket=$sock3"
 qemu-io -f raw -c 'w -z -u 0 1M' "nbd+unix://?socket=$sock4"
 qemu-io -f raw -c 'w -z -u 0 1M' "nbd+unix://?socket=$sock5b"
+qemu-io -f raw -c 'w -z -u 0 1M' "nbd+unix://?socket=$sock6"

 # Check for expected ZERO vs. WRITE results
 grep 'connection=1 Zero' nozero1.log
@@ -129,9 +138,23 @@ if grep 'connection=1 Zero' nozero5a.log; then
     echo "nbdkit should have converted zero into write before nbd
plugin"
     exit 1
 fi
+grep 'connection=1 Zero' nozero6.log

 # Sanity check on contents - all 5 files should read identically
 cmp nozero1.img nozero2.img
 cmp nozero2.img nozero3.img
 cmp nozero3.img nozero4.img
 cmp nozero4.img nozero5.img
+cmp nozero5.img nozero6.img
+
+# Sanity check on sparseness; only image 1 should be sparse
+if test "$(stat -c %b nozero1.img)" = "$(stat -c %b
nozero2.img)"; then
+    echo "nozero1.img was not trimmed"
+    exit 1
+fi
+for i in 3 4 5 6; do
+    if test "$(stat -c %b nozero2.img)" != "$(stat -c %b
nozero$i.img)"; then
+	echo "nozero$i.img was trimmed by mistake"
+	exit 1
+    fi
+done
-- 
2.20.1

On Thu, May 09, 2019 at 07:14:52PM -0500, Eric Blake
wrote:
> It may be useful to test whether the client's use of
> NBD_CMD_FLAG_NO_HOLE makes a difference; do this by adding a mode to
> --filter=nozero to force a non-trimming zero write.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  filters/nozero/nbdkit-nozero-filter.pod | 19 +++++++----
>  filters/nozero/nozero.c                 | 45 ++++++++++++++++++++-----
>  tests/test-nozero.sh                    | 27 +++++++++++++--
>  3 files changed, 74 insertions(+), 17 deletions(-)
> 
> diff --git a/filters/nozero/nbdkit-nozero-filter.pod
b/filters/nozero/nbdkit-nozero-filter.pod
> index 7e06570..8e694bb 100644
> --- a/filters/nozero/nbdkit-nozero-filter.pod
> +++ b/filters/nozero/nbdkit-nozero-filter.pod
> @@ -18,14 +18,19 @@ testing client or server fallbacks.
> 
>  =over 4
> 
> -=item B<zeromode=none|emulate>
> +=item B<zeromode=none|emulate|notrim>
> 
>  Optional, controls which mode the filter will use.  Mode B<none>
> -(default) means that zero support is not advertised to the client;
> -mode B<emulate> means that zero support is emulated by the filter
> -using the plugin's C<pwrite> callback, regardless of whether the
> -plugin itself implemented the C<zero> callback with a more efficient
> -way to write zeroes.
> +(default) means that zero support is not advertised to the
> +client. Mode B<emulate> means that zero support is emulated by the
> +filter using the plugin's C<pwrite> callback, regardless of
whether
> +the plugin itself implemented the C<zero> callback with a more
> +efficient way to write zeros. Since nbdkit E<ge> 1.13.4, mode
> +B<notrim> means that zero requests are forwarded on to the plugin,
> +except that the plugin will never see the NBDKIT_MAY_TRIM flag, to
> +determine if the client permitting trimming during zero operations
> +makes a difference (it is an error to request this mode if the plugin
> +does not support the C<zero> callback).
> 
>  =back
> 
> @@ -55,4 +60,4 @@ Eric Blake
> 
>  =head1 COPYRIGHT
> 
> -Copyright (C) 2018 Red Hat Inc.
> +Copyright (C) 2018-2019 Red Hat Inc.
> diff --git a/filters/nozero/nozero.c b/filters/nozero/nozero.c
> index 3ec6346..6e0ffa9 100644
> --- a/filters/nozero/nozero.c
> +++ b/filters/nozero/nozero.c
> @@ -47,8 +47,12 @@
> 
>  #define MAX_WRITE (64 * 1024 * 1024)
> 
> -static char buffer[MAX_WRITE];
> -static bool emulate;
> +static const char buffer[MAX_WRITE];
> +static enum ZeroMode {
> +  NONE,
> +  EMULATE,
> +  NOTRIM,
> +} zeromode;
> 
>  static int
>  nozero_config (nbdkit_next_config *next, void *nxdata,
> @@ -56,7 +60,9 @@ nozero_config (nbdkit_next_config *next, void *nxdata,
>  {
>    if (strcmp (key, "zeromode") == 0) {
>      if (strcmp (value, "emulate") == 0)
> -      emulate = true;
> +      zeromode = EMULATE;
> +    else if (strcmp (value, "notrim") == 0)
> +      zeromode = NOTRIM;
>      else if (strcmp (value, "none") != 0) {
>        nbdkit_error ("unknown zeromode '%s'", value);
>        return -1;
> @@ -67,13 +73,31 @@ nozero_config (nbdkit_next_config *next, void *nxdata,
>  }
> 
>  #define nozero_config_help \
> -  "zeromode=<MODE>      Either 'none' (default) or
'emulate'.\n" \
> +  "zeromode=<MODE>      Either 'none' (default),
'emulate', or 'notrim'.\n" \
> +
> +/* Check that desired mode is supported by plugin. */
> +static int
> +nozero_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void
*handle)
> +{
> +  int r;
> +
> +  if (zeromode == NOTRIM) {
> +    r = next_ops->can_zero (nxdata);
> +    if (r == -1)
> +      return -1;
> +    if (!r) {
> +      nbdkit_error ("zeromode 'notrim' requires plugin zero
support");
> +      return -1;
> +    }
> +  }
> +  return 0;
> +}
> 
>  /* Advertise desired WRITE_ZEROES mode. */
>  static int
>  nozero_can_zero (struct nbdkit_next_ops *next_ops, void *nxdata, void
*handle)
>  {
> -  return emulate;
> +  return zeromode != NONE;
>  }
> 
>  static int
> @@ -81,11 +105,15 @@ nozero_zero (struct nbdkit_next_ops *next_ops, void
*nxdata,
>               void *handle, uint32_t count, uint64_t offs, uint32_t flags,
>               int *err)
>  {
> -  assert (emulate);
> +  assert (zeromode != NONE);
> +  flags &= ~NBDKIT_FLAG_MAY_TRIM;
> +
> +  if (zeromode == NOTRIM)
> +    return next_ops->zero (nxdata, count, offs, flags, err);
> +
>    while (count) {
>      uint32_t size = MIN (count, MAX_WRITE);
> -    if (next_ops->pwrite (nxdata, buffer, size, offs,
> -                          flags & ~NBDKIT_FLAG_MAY_TRIM, err) == -1)
> +    if (next_ops->pwrite (nxdata, buffer, size, offs, flags, err) ==
-1)
>        return -1;
>      offs += size;
>      count -= size;
> @@ -99,6 +127,7 @@ static struct nbdkit_filter filter = {
>    .version           = PACKAGE_VERSION,
>    .config            = nozero_config,
>    .config_help       = nozero_config_help,
> +  .prepare           = nozero_prepare,
>    .can_zero          = nozero_can_zero,
>    .zero              = nozero_zero,
>  };
> diff --git a/tests/test-nozero.sh b/tests/test-nozero.sh
> index 1282586..fc22420 100755
> --- a/tests/test-nozero.sh
> +++ b/tests/test-nozero.sh
> @@ -39,12 +39,14 @@ sock3=`mktemp -u`
>  sock4=`mktemp -u`
>  sock5a=`mktemp -u`
>  sock5b=`mktemp -u`
> +sock6=`mktemp -u`
>  files="nozero1.img nozero1.log $sock1 nozero1.pid
>         nozero2.img nozero2.log $sock2 nozero2.pid
>         nozero3.img nozero3.log $sock3 nozero3.pid
>         nozero4.img nozero4.log $sock4 nozero4.pid
>         nozero5.img nozero5a.log nozero5b.log $sock5a $sock5b
> -       nozero5a.pid nozero5b.pid"
> +       nozero5a.pid nozero5b.pid
> +       nozero6.img nozero6.log $sock6 nozero6.pid"
>  rm -f $files
> 
>  # Prep images, and check that qemu-io understands the actions we plan on
> @@ -54,6 +56,7 @@ cp nozero1.img nozero2.img
>  cp nozero1.img nozero3.img
>  cp nozero1.img nozero4.img
>  cp nozero1.img nozero5.img
> +cp nozero1.img nozero6.img
>  if ! qemu-io -f raw -d unmap -c 'w -z -u 0 1M' nozero1.img; then
>      echo "$0: missing or broken qemu-io"
>      rm nozero?.img
> @@ -82,17 +85,20 @@ cleanup ()
>      cat nozero5a.log || :
>      echo "Log 5b file contents:"
>      cat nozero5b.log || :
> +    echo "Log 6 file contents:"
> +    cat nozero6.log || :
>      rm -f $files
>  }
>  cleanup_fn cleanup
> 
> -# Run four parallel nbdkit; to compare the logs and see what changes.
> +# Run several parallel nbdkit; to compare the logs and see what changes.
>  # 1: unfiltered, to check that qemu-io sends ZERO request and plugin trims
>  # 2: log before filter with zeromode=none (default), to ensure no ZERO
request
>  # 3: log before filter with zeromode=emulate, to ensure ZERO from client
>  # 4: log after filter with zeromode=emulate, to ensure no ZERO to plugin
>  # 5a/b: both sides of nbd plugin: even though server side does not
advertise
>  # ZERO, the client side still exposes it, and just skips calling nbd's
.zero
> +# 6: log after filter with zeromode=notrim, to ensure plugin does not trim
>  start_nbdkit -P nozero1.pid -U $sock1 --filter=log \
>         file logfile=nozero1.log nozero1.img
>  start_nbdkit -P nozero2.pid -U $sock2 --filter=log --filter=nozero \
> @@ -105,6 +111,8 @@ start_nbdkit -P nozero5a.pid -U $sock5a --filter=log
--filter=nozero \
>         file logfile=nozero5a.log nozero5.img
>  start_nbdkit -P nozero5b.pid -U $sock5b --filter=log \
>         nbd logfile=nozero5b.log socket=$sock5a
> +start_nbdkit -P nozero6.pid -U $sock6 --filter=nozero --filter=log \
> +       file logfile=nozero6.log nozero6.img zeromode=notrim
> 
>  # Perform the zero write.
>  qemu-io -f raw -c 'w -z -u 0 1M'
"nbd+unix://?socket=$sock1"
> @@ -112,6 +120,7 @@ qemu-io -f raw -c 'w -z -u 0 1M'
"nbd+unix://?socket=$sock2"
>  qemu-io -f raw -c 'w -z -u 0 1M'
"nbd+unix://?socket=$sock3"
>  qemu-io -f raw -c 'w -z -u 0 1M'
"nbd+unix://?socket=$sock4"
>  qemu-io -f raw -c 'w -z -u 0 1M'
"nbd+unix://?socket=$sock5b"
> +qemu-io -f raw -c 'w -z -u 0 1M'
"nbd+unix://?socket=$sock6"
> 
>  # Check for expected ZERO vs. WRITE results
>  grep 'connection=1 Zero' nozero1.log
> @@ -129,9 +138,23 @@ if grep 'connection=1 Zero' nozero5a.log; then
>      echo "nbdkit should have converted zero into write before nbd
plugin"
>      exit 1
>  fi
> +grep 'connection=1 Zero' nozero6.log
> 
>  # Sanity check on contents - all 5 files should read identically
>  cmp nozero1.img nozero2.img
>  cmp nozero2.img nozero3.img
>  cmp nozero3.img nozero4.img
>  cmp nozero4.img nozero5.img
> +cmp nozero5.img nozero6.img
> +
> +# Sanity check on sparseness; only image 1 should be sparse
> +if test "$(stat -c %b nozero1.img)" = "$(stat -c %b
nozero2.img)"; then
> +    echo "nozero1.img was not trimmed"
> +    exit 1
> +fi
> +for i in 3 4 5 6; do
> +    if test "$(stat -c %b nozero2.img)" != "$(stat -c %b
nozero$i.img)"; then
> +	echo "nozero$i.img was trimmed by mistake"
> +	exit 1
> +    fi
> +done
> -- 
> 2.20.1
ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW