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

[Laszlo Ersek]

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