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