Libguestfs - Feb 2022 - [nbdkit PATCH] RFC: noextents: Add extentmode=data config parameter

While writing tests for an nbdcopy bug, I found myself wanting a way
to easily view an entire image as data, but without disabling extents
support altogether.  The existing extentlist filter can do this, but
requires a secondary file.

Still an RFC because I need testsuite coverage similar to
test-nozero.sh (eek - we don't have any direct test of the noextent
filter, but only indirect coverage through other tests).  Also, are
there any other extentmode=MODE values that might make sense?
---
 filters/noextents/nbdkit-noextents-filter.pod | 32 ++++++++++--
 filters/noextents/noextents.c                 | 50 ++++++++++++++++++-
 2 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/filters/noextents/nbdkit-noextents-filter.pod
b/filters/noextents/nbdkit-noextents-filter.pod
index 891b197d..aac2f097 100644
--- a/filters/noextents/nbdkit-noextents-filter.pod
+++ b/filters/noextents/nbdkit-noextents-filter.pod
@@ -4,7 +4,7 @@ nbdkit-noextents-filter - disable extents in the underlying
plugin

 =head1 SYNOPSIS

- nbdkit --filter=noextents plugin
+ nbdkit --filter=noextents plugin [plugin-args...] [extentmode=MODE]

 =head1 DESCRIPTION

@@ -23,9 +23,31 @@ performance (C<tmpfs> is known to be one such system).

 =head1 PARAMETERS

-There are no parameters specific to nbdkit-noextents-filter.  Any
-parameters are passed through to and processed by the underlying
-plugin in the normal way.
+The parameter C<extentmode> is optional, and controls which mode the
+filter will use.
+
+=over 4
+
+=item B<extentmode=mask>
+
+Extent support is not advertised to the client; clients should not
+query for extent information, and must assume the entire disk is
+allocated.
+
+This is the default if the C<extentmode> parameter is not specified.
+
+=item B<extentmode=data>
+
+(nbdkit E<ge> 1.30)
+
+Extent support is advertised, but extent requests from the client will
+be answered with a claim that the entire disk forms a single allocated
+data extent.
+
+=back
+
+All other parameters are passed through to and processed by the
+underlying plugin in the normal way.

 =head1 FILES

@@ -61,4 +83,4 @@ Richard W.M. Jones

 =head1 COPYRIGHT

-Copyright (C) 2019 Red Hat Inc.
+Copyright (C) 2019-2022 Red Hat Inc.
diff --git a/filters/noextents/noextents.c b/filters/noextents/noextents.c
index f3044809..36231a35 100644
--- a/filters/noextents/noextents.c
+++ b/filters/noextents/noextents.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2019 Red Hat Inc.
+ * Copyright (C) 2019-2022 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -32,19 +32,65 @@

 #include <config.h>

+#include <string.h>
+#include <assert.h>
+
 #include <nbdkit-filter.h>

+static enum ExtentMode {
+  MASK,
+  DATA,
+} extentmode;
+
+static int
+noextents_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
+                  const char *key, const char *value)
+{
+  if (strcmp (key, "extentmode") == 0) {
+    if (strcmp (value, "mask") == 0)
+      extentmode = MASK;
+    else if (strcmp (value, "data") == 0)
+      extentmode = DATA;
+    else {
+      nbdkit_error ("unknown extentmode '%s'", value);
+      return -1;
+    }
+    return 0;
+  }
+
+  return next (nxdata, key, value);
+}
+
+#define noextents_config_help \
+  "extentmode=<MODE>    One of 'mask' (default),
'data'.\n"
+
+/* Advertise desired extents support. */
 static int
 noextents_can_extents (nbdkit_next *next,
                        void *handle)
 {
-  return 0;
+  return extentmode == DATA;
+}
+
+/* Produce single data extent. */
+static int
+noextents_extents (nbdkit_next *next,
+                   void *handle, uint32_t count, uint64_t offset,
+                   uint32_t flags,
+                   struct nbdkit_extents *ret_extents,
+                   int *err)
+{
+  assert (extentmode == DATA);
+  return nbdkit_add_extent (ret_extents, offset, count, 0);
 }

 static struct nbdkit_filter filter = {
   .name              = "noextents",
   .longname          = "nbdkit noextents filter",
+  .config            = noextents_config,
+  .config_help       = noextents_config_help,
   .can_extents       = noextents_can_extents,
+  .extents           = noextents_extents,
 };

 NBDKIT_REGISTER_FILTER(filter)
-- 
2.34.1

On Fri, Feb 04, 2022 at 10:44:38AM -0600, Eric Blake
wrote:
> While writing tests for an nbdcopy bug, I found myself wanting a way
> to easily view an entire image as data, but without disabling extents
> support altogether.  The existing extentlist filter can do this, but
> requires a secondary file.
> 
> Still an RFC because I need testsuite coverage similar to
> test-nozero.sh (eek - we don't have any direct test of the noextent
> filter, but only indirect coverage through other tests).  Also, are
> there any other extentmode=MODE values that might make sense?
I probably would have added another filter, but this way makes sense too.

Rich.
> ---
>  filters/noextents/nbdkit-noextents-filter.pod | 32 ++++++++++--
>  filters/noextents/noextents.c                 | 50 ++++++++++++++++++-
>  2 files changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/filters/noextents/nbdkit-noextents-filter.pod
b/filters/noextents/nbdkit-noextents-filter.pod
> index 891b197d..aac2f097 100644
> --- a/filters/noextents/nbdkit-noextents-filter.pod
> +++ b/filters/noextents/nbdkit-noextents-filter.pod
> @@ -4,7 +4,7 @@ nbdkit-noextents-filter - disable extents in the underlying
plugin
> 
>  =head1 SYNOPSIS
> 
> - nbdkit --filter=noextents plugin
> + nbdkit --filter=noextents plugin [plugin-args...] [extentmode=MODE]
> 
>  =head1 DESCRIPTION
> 
> @@ -23,9 +23,31 @@ performance (C<tmpfs> is known to be one such
system).
> 
>  =head1 PARAMETERS
> 
> -There are no parameters specific to nbdkit-noextents-filter.  Any
> -parameters are passed through to and processed by the underlying
> -plugin in the normal way.
> +The parameter C<extentmode> is optional, and controls which mode the
> +filter will use.
> +
> +=over 4
> +
> +=item B<extentmode=mask>
> +
> +Extent support is not advertised to the client; clients should not
> +query for extent information, and must assume the entire disk is
> +allocated.
> +
> +This is the default if the C<extentmode> parameter is not specified.
> +
> +=item B<extentmode=data>
> +
> +(nbdkit E<ge> 1.30)
> +
> +Extent support is advertised, but extent requests from the client will
> +be answered with a claim that the entire disk forms a single allocated
> +data extent.
> +
> +=back
> +
> +All other parameters are passed through to and processed by the
> +underlying plugin in the normal way.
> 
>  =head1 FILES
> 
> @@ -61,4 +83,4 @@ Richard W.M. Jones
> 
>  =head1 COPYRIGHT
> 
> -Copyright (C) 2019 Red Hat Inc.
> +Copyright (C) 2019-2022 Red Hat Inc.
> diff --git a/filters/noextents/noextents.c b/filters/noextents/noextents.c
> index f3044809..36231a35 100644
> --- a/filters/noextents/noextents.c
> +++ b/filters/noextents/noextents.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2019 Red Hat Inc.
> + * Copyright (C) 2019-2022 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -32,19 +32,65 @@
> 
>  #include <config.h>
> 
> +#include <string.h>
> +#include <assert.h>
> +
>  #include <nbdkit-filter.h>
> 
> +static enum ExtentMode {
> +  MASK,
> +  DATA,
> +} extentmode;
> +
> +static int
> +noextents_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
> +                  const char *key, const char *value)
> +{
> +  if (strcmp (key, "extentmode") == 0) {
> +    if (strcmp (value, "mask") == 0)
> +      extentmode = MASK;
> +    else if (strcmp (value, "data") == 0)
> +      extentmode = DATA;
> +    else {
> +      nbdkit_error ("unknown extentmode '%s'", value);
> +      return -1;
> +    }
> +    return 0;
> +  }
> +
> +  return next (nxdata, key, value);
> +}
> +
> +#define noextents_config_help \
> +  "extentmode=<MODE>    One of 'mask' (default),
'data'.\n"
> +
> +/* Advertise desired extents support. */
>  static int
>  noextents_can_extents (nbdkit_next *next,
>                         void *handle)
>  {
> -  return 0;
> +  return extentmode == DATA;
> +}
> +
> +/* Produce single data extent. */
> +static int
> +noextents_extents (nbdkit_next *next,
> +                   void *handle, uint32_t count, uint64_t offset,
> +                   uint32_t flags,
> +                   struct nbdkit_extents *ret_extents,
> +                   int *err)
> +{
> +  assert (extentmode == DATA);
> +  return nbdkit_add_extent (ret_extents, offset, count, 0);
>  }
> 
>  static struct nbdkit_filter filter = {
>    .name              = "noextents",
>    .longname          = "nbdkit noextents filter",
> +  .config            = noextents_config,
> +  .config_help       = noextents_config_help,
>    .can_extents       = noextents_can_extents,
> +  .extents           = noextents_extents,
>  };
> 
>  NBDKIT_REGISTER_FILTER(filter)
> -- 
> 2.34.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 02/04/22 17:44, Eric Blake wrote:
> While writing tests for an nbdcopy bug, I found myself wanting a way
> to easily view an entire image as data, but without disabling extents
> support altogether.  The existing extentlist filter can do this, but
> requires a secondary file.
> 
> Still an RFC because I need testsuite coverage similar to
> test-nozero.sh (eek - we don't have any direct test of the noextent
> filter, but only indirect coverage through other tests).  Also, are
> there any other extentmode=MODE values that might make sense?
> ---
>  filters/noextents/nbdkit-noextents-filter.pod | 32 ++++++++++--
>  filters/noextents/noextents.c                 | 50 ++++++++++++++++++-
>  2 files changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/filters/noextents/nbdkit-noextents-filter.pod
b/filters/noextents/nbdkit-noextents-filter.pod
> index 891b197d..aac2f097 100644
> --- a/filters/noextents/nbdkit-noextents-filter.pod
> +++ b/filters/noextents/nbdkit-noextents-filter.pod
> @@ -4,7 +4,7 @@ nbdkit-noextents-filter - disable extents in the underlying
plugin
> 
>  =head1 SYNOPSIS
> 
> - nbdkit --filter=noextents plugin
> + nbdkit --filter=noextents plugin [plugin-args...] [extentmode=MODE]
> 
>  =head1 DESCRIPTION
> 
> @@ -23,9 +23,31 @@ performance (C<tmpfs> is known to be one such
system).
> 
>  =head1 PARAMETERS
> 
> -There are no parameters specific to nbdkit-noextents-filter.  Any
> -parameters are passed through to and processed by the underlying
> -plugin in the normal way.
> +The parameter C<extentmode> is optional, and controls which mode the
> +filter will use.
> +
> +=over 4
> +
> +=item B<extentmode=mask>
> +
> +Extent support is not advertised to the client; clients should not
> +query for extent information, and must assume the entire disk is
> +allocated.
> +
> +This is the default if the C<extentmode> parameter is not specified.
> +
> +=item B<extentmode=data>
> +
> +(nbdkit E<ge> 1.30)
> +
> +Extent support is advertised, but extent requests from the client will
> +be answered with a claim that the entire disk forms a single allocated
> +data extent.
> +
> +=back
> +
> +All other parameters are passed through to and processed by the
> +underlying plugin in the normal way.
Is this necessary to spell out? (Looked at a random other filter's
documentation, and didn't see such a statement, so I think it's the
default.)

(The same question applies to "plugin-args" in the synopsys, more or
less...)
> 
>  =head1 FILES
> 
> @@ -61,4 +83,4 @@ Richard W.M. Jones
> 
>  =head1 COPYRIGHT
> 
> -Copyright (C) 2019 Red Hat Inc.
> +Copyright (C) 2019-2022 Red Hat Inc.
> diff --git a/filters/noextents/noextents.c b/filters/noextents/noextents.c
> index f3044809..36231a35 100644
> --- a/filters/noextents/noextents.c
> +++ b/filters/noextents/noextents.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2019 Red Hat Inc.
> + * Copyright (C) 2019-2022 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -32,19 +32,65 @@
> 
>  #include <config.h>
> 
> +#include <string.h>
> +#include <assert.h>
> +
>  #include <nbdkit-filter.h>
> 
> +static enum ExtentMode {
> +  MASK,
> +  DATA,
> +} extentmode;
> +
> +static int
> +noextents_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
> +                  const char *key, const char *value)
> +{
> +  if (strcmp (key, "extentmode") == 0) {
> +    if (strcmp (value, "mask") == 0)
> +      extentmode = MASK;
> +    else if (strcmp (value, "data") == 0)
> +      extentmode = DATA;
> +    else {
> +      nbdkit_error ("unknown extentmode '%s'", value);
> +      return -1;
> +    }
> +    return 0;
> +  }
> +
> +  return next (nxdata, key, value);
> +}
> +
> +#define noextents_config_help \
> +  "extentmode=<MODE>    One of 'mask' (default),
'data'.\n"
> +
> +/* Advertise desired extents support. */
>  static int
>  noextents_can_extents (nbdkit_next *next,
>                         void *handle)
>  {
> -  return 0;
> +  return extentmode == DATA;
> +}
> +
> +/* Produce single data extent. */
> +static int
> +noextents_extents (nbdkit_next *next,
> +                   void *handle, uint32_t count, uint64_t offset,
> +                   uint32_t flags,
> +                   struct nbdkit_extents *ret_extents,
> +                   int *err)
> +{
> +  assert (extentmode == DATA);
> +  return nbdkit_add_extent (ret_extents, offset, count, 0);
>  }
I don't have the context in which this function may be called, in my
head, but this implementation seems to reflect precisely the requested
offset range back at the client, so a question arises:

- What if the client requests an offset range that (at least partially)
exceeds the size of the file? In that case, I think we should not report
that range as existent. For example, the chunked block status query in
virt-v2v asks for 2GB offset ranges, and it's expected that the returned
extent list will not exceed the file size.

(I understand that a server is permitted to cover a larger offset range
than requested in its reply, unless LIBNBD_CMD_FLAG_REQ_ONE is set;
however, this is different. Without LIBNBD_CMD_FLAG_REQ_ONE, the
response may well be permitted to exceed the requested range, but it's
still not permitted, AIUI, to report nonexistent data.)

In short, I think we should call get_size() (?) on the underlying plugin
(?), and truncate the requested range accordingly.
> 
>  static struct nbdkit_filter filter = {
>    .name              = "noextents",
>    .longname          = "nbdkit noextents filter",
> +  .config            = noextents_config,
> +  .config_help       = noextents_config_help,
>    .can_extents       = noextents_can_extents,
> +  .extents           = noextents_extents,
>  };
> 
>  NBDKIT_REGISTER_FILTER(filter)
> 
Thanks,
Laszlo