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