[Libguestfs] [PATCH nbdkit 6/6] XXX UNFINISHED New filter: nbdkit-block-size-constraint-filter
Eric Blake
eblake at redhat.com
Wed Feb 16 19:15:23 UTC 2022
On Wed, Feb 16, 2022 at 04:20:41PM +0000, Richard W.M. Jones wrote:
> This filter can add block size constraints to plugins which don't
> already support them. It can also enforce an error policy for badly
> behaved clients which do not obey the block size constraints.
>
> UNFINISHED: The error policy is not implemented.
> ---
> docs/nbdkit-plugin.pod | 4 +-
> .../nbdkit-block-size-constraint-filter.pod | 122 +++++++++++
> configure.ac | 2 +
> filters/block-size-constraint/Makefile.am | 70 ++++++
> tests/Makefile.am | 4 +
> .../block-size-constraint.c | 206 ++++++++++++++++++
> tests/test-block-size-constraint-filter.sh | 117 ++++++++++
> 7 files changed, 524 insertions(+), 1 deletion(-)
Our existing filter is named blocksize; using 'block-size-*' in this
one is inviting confusion, so we're probably better with 'blocksize-'.
Maybe 'blocksize-policy' is a better name than 'blocksize-constraint'?
We could even cram the functionality into the existing blocksize
filter, although it gets harder to argue whether it is easier to use
one filter to control multiple knobs (both what we pass down to the
plugin, and what we advertise to the client), than it is to have two
filters, so I like the idea of keeping the two separate.
Thinking about combining the two, we are probably best recommending:
nbdkit --filter=blocksize-policy --filter=blocksize plugin \
block-size-error-policy=allow ...
which says what to advertise, then under the hood rewrites it to the
actual needs of the plugin. The other way around:
nbdkit --filter=blocksize --filter=blocksize-policy plugin ...
does not make much sense (by the time the policy filter sees anything,
it has already been rewritten from the client into something that will
comply).
>
> diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
> index 15573f0a..427ca428 100644
> --- a/docs/nbdkit-plugin.pod
> +++ b/docs/nbdkit-plugin.pod
> @@ -890,7 +890,9 @@ that all client requests will obey the constraints. A client could
> still ignore the constraints. nbdkit passes all requests through to
> the plugin, because what the plugin does depends on the plugin's
> policy. It might decide to serve the requests correctly anyway, or
> -reject them with an error.
> +reject them with an error. Plugins can avoid this complexity by using
> +L<nbdkit-block-size-constraint-filter(1)> which allows both
> +setting/adjusting the constraints, and selecting an error policy.
>
> The minimum block size must be E<ge> 1. The maximum block size must
> be E<le> 0xffff_ffff. minimum E<le> preferred E<le> maximum.
> diff --git a/filters/block-size-constraint/nbdkit-block-size-constraint-filter.pod b/filters/block-size-constraint/nbdkit-block-size-constraint-filter.pod
> new file mode 100644
> index 00000000..9205e503
> --- /dev/null
> +++ b/filters/block-size-constraint/nbdkit-block-size-constraint-filter.pod
> @@ -0,0 +1,122 @@
> +=head1 NAME
> +
> +nbdkit-block-size-constraint-filter - set minimum, preferred and
> +maximum block size, and apply error policy
> +
> +=head1 SYNOPSIS
> +
> + nbdkit --filter=block-size-constraint PLUGIN
> + [block-size-error-policy=allow|error]
> + [block-size-minimum=N]
> + [block-size-preferred=N]
> + [block-size-maximum=N]
Do we want orthogonal policies for size vs. alignment (for example,
permitting smaller than minsize requests, but insisting that a request
cannot cross an alignment boundary)? Or are we okay with just one
policy (size and alignments are always enforced in parallel).
> +
> +=head1 DESCRIPTION
> +
> +C<nbdkit-block-size-constraint-filter> is an L<nbdkit(1)> filter that
> +can add block size constraints to plugins which don't already support
> +then. It can also enforce an error policy for badly behaved clients
> +which do not obey the block size constraints.
> +
> +For more information about block size constraints, see section
> +"Block size constraints" in
> +L<https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md>.
> +
> +The simplest usage is to place this filter on top of any plugin which
> +does not advertise block size constraints, and set the
> +C<block-size-minimum>, C<block-size-preferred> and
> +C<block-size-maximum> parameters with the desired constraints. For
> +example:
> +
> + nbdkit --filter=block-size-constraint memory 1G \
> + block-size-preferred=32K
> +
> +would adjust L<nbdkit-memory-plugin(1)> so that clients should
> +prefer 32K requests. You can query the NBD server advertised constraints
> +using L<nbdinfo(1)>:
> +
> + $ nbdinfo nbd://localhost
> + [...]
> + block_size_minimum: 1
> + block_size_preferred: 32768
> + block_size_maximum: 4294967295
> +
> +The second part of this filter is adjusting the error policy when
> +badly behaved clients do not obey the minimum or maximum request size.
> +Normally nbdkit permits these requests, leaving it up to the plugin
> +whether it rejects the request with an error or tries to process the
> +request (eg. trying to split an over-large request). With this filter
> +you can use C<block-size-error-policy=error> to reject these requests
> +in the filter with EIO error. The plugin will not see them.
> +
> +A related filter is L<nbdkit-blocksize-filter(1)>. That filter can
> +split and combine requests for plugins that cannot handle requests
> +under or over a particular size. The two filters may be used together
> +- this filter advertising what the plugin can support to well-behaved
> +clients, and the other filter coping with clients that are either not
> +well-behaved or not capable of adjusting request size.
This is where documenting the best order of the two filters would go.
> +
> +=head1 PARAMETERS
> +
> +=over 4
> +
> +=item B<block-size-error-policy=allow>
> +
> +=item B<block-size-error-policy=error>
> +
> +If a client sends a request which is smaller than the permitted
> +minimum size or larger than the permitted maximum size,
> +C<block-size-error-policy> chooses what the filter will do. The
> +default (and also nbdkit's default) is C<allow> which means pass the
> +request through to the plugin. Use C<error> to return an EIO error
> +back to the client. The plugin will not see the badly formed request
> +in this case.
The documentation looks promising.
Hmm - the blocksize filter supports oversized requests that do not
include data (such as a 4G zero, trim or cache); we may need a third
policy for whether to permit oversized non-data requests while still
rejecting oversized data requests.
> +++ b/configure.ac
> @@ -109,6 +109,7 @@ non_lang_plugins="\
> plugins="$(echo $(printf %s\\n $lang_plugins $non_lang_plugins | sort -u))"
> filters="\
> blocksize \
> + block-size-constraint \
This particular hunk makes the difference between 'blocksize' and
'block-size-constraint' obvious, where 'blocksize-constraint' or even
'blocksize-policy' sounds nicer.
> +++ b/filters/block-size-constraint/block-size-constraint.c
> +/* Error policy. */
> +static enum { ALLOW, ERROR } error_policy = ALLOW;
> +
> +static int
> +block_size_constraint_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
> + const char *key, const char *value)
> +{
> + int64_t r;
> +
> + if (strcmp (key, "block-size-error-policy") == 0) {
> + if (strcmp (value, "allow") == 0)
> + error_policy = ALLOW;
> + else if (strcmp (value, "error") == 0)
> + error_policy = ERROR;
> + else {
> + nbdkit_error ("unknown %s: %s", key, value);
> + return -1;
> + }
> + return 0;
> + }
> + else if (strcmp (key, "block-size-minimum") == 0) {
> + r = nbdkit_parse_size (value);
> + if (r == -1 || r > UINT32_MAX) {
> + parse_error:
> + nbdkit_error ("%s: could not parse %s", key, value);
> + return -1;
> + }
> + config_minimum = r;
> + return 0;
> + }
> + else if (strcmp (key, "block-size-preferred") == 0) {
> + r = nbdkit_parse_size (value);
> + if (r == -1 || r > UINT32_MAX) goto parse_error;
> + config_preferred = r;
> + return 0;
> + }
> + else if (strcmp (key, "block-size-maximum") == 0) {
> + r = nbdkit_parse_size (value);
> + if (r == -1 || r > UINT32_MAX) goto parse_error;
> + config_maximum = r;
> + return 0;
> + }
> +
> + return next (nxdata, key, value);
> +}
> +
> +static int
> +block_size_constraint_config_complete (nbdkit_next_config_complete *next,
> + nbdkit_backend *nxdata)
> +{
> + /* Just do some basic checks that the values are sane (if set). */
> + if (config_minimum) {
> + if (! is_power_of_2 (config_minimum)) {
> + nbdkit_error ("block-size-minimum must be a power of 2");
> + return -1;
> + }
> + }
> +
> + if (config_minimum && config_preferred) {
> + if (config_minimum > config_preferred) {
> + nbdkit_error ("block-size-minimum must be <= block-size-preferred");
> + return -1;
> + }
> + }
> +
> + if (config_preferred && config_maximum) {
> + if (config_preferred > config_maximum) {
> + nbdkit_error ("block-size-preferred must be <= block-size-maximum");
> + return -1;
> + }
> + }
> +
> + /* XXX We could also check the preferred and maximum are a
> + * multiple of minimum.
> + */
Hmm. This is only insisting on consistency for parameters to this filter...
> +
> + return next (nxdata);
> +}
> +
> +static int
> +block_size_constraint_block_size (nbdkit_next *next,
> + void *handle,
> + uint32_t *minimum, uint32_t *preferred,
> + uint32_t *maximum)
> +{
> + /* If the user has set all of the block size parameters then we
> + * don't need to ask the plugin, we can go ahead and advertise them.
> + */
> + if (config_minimum && config_preferred && config_maximum) {
> + *minimum = config_minimum;
> + *preferred = config_preferred;
> + *maximum = config_maximum;
> + return 0;
> + }
> +
> + /* Otherwise, ask the plugin. */
> + if (next->block_size (next, minimum, preferred, maximum) == -1)
> + return -1;
> +
> + /* If the user of this filter didn't configure anything, then return
> + * the plugin values (even if unset).
> + */
> + if (!config_minimum && !config_preferred && !config_maximum)
> + return 0;
> +
> + /* Now we get to the awkward case where the user configured some
> + * values but not others. There's all kinds of room for things to
> + * go wrong here, so try to check for obvious user errors as best we
> + * can.
> + */
...oh, I see. You do also try to ensure sane values even with what the
plugin set. That leads to some code duplication; maybe we need a
helper function, which when given three values (min, pref, max) checks
if they are self-consistent, and then reuse that helper from
server/plugin.c, above in block_size_constraint_config_complete, and
here.
> + if (*minimum == 0) { /* Plugin didn't set anything. */
> + if (config_minimum)
> + *minimum = config_minimum;
> + else
> + *minimum = 1;
> +
> + if (config_preferred)
> + *preferred = config_preferred;
> + else
> + *preferred = 4096;
> +
> + if (config_maximum)
> + *maximum = config_maximum;
> + else
> + *maximum = 0xffffffff;
> + }
> + else { /* Plugin set some values. */
Didn't we say back in server/plugin.c that if the plugin set any
value, it should set all three? It's one thing for the filter to allow
partial results (easier use from the command line), but another for a
plugin to have partial results (what does the plugin want us to supply
for the other values?)
> + if (config_minimum)
> + *minimum = config_minimum;
> +
> + if (config_preferred)
> + *preferred = config_preferred;
> +
> + if (config_maximum)
> + *maximum = config_maximum;
> + }
> +
> + if (*minimum > *preferred || *preferred > *maximum) {
> + nbdkit_error ("computed block size values are invalid, minimum %" PRIu32
> + " > preferred %" PRIu32
> + " or preferred > maximum %" PRIu32,
> + *minimum, *preferred, *maximum);
> + return -1;
> + }
> + return 0;
> +}
> +
> +static struct nbdkit_filter filter = {
> + .name = "block-size-constraint",
> + .longname = "nbdkit block size constraint filter",
> + .config = block_size_constraint_config,
> + .config_complete = block_size_constraint_config_complete,
> + .block_size = block_size_constraint_block_size,
> +};
And to enforce the policy, we'll need .pread, .pwrite, .zero, .trim,
.cache, and .extents.
But looks like a good start.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
More information about the Libguestfs
mailing list