Richard W.M. Jones
2022-Feb-17 14:36 UTC
[Libguestfs] [PATCH nbdkit v2 1/7] server: Add new plugin/filter .block_size callback
This commit implements backend_block_size() and wires it up to a new
.block_size callback from the plugin through any filters on top.
This callback will allow plugins to pass their minimum, preferred and
maximum block size through the NBD protocol NBD_INFO_BLOCK_SIZE info
option.
The new function is not called from anywhere yet, so for the moment
this commit does nothing.
---
include/nbdkit-filter.h | 4 +++
include/nbdkit-plugin.h | 3 ++
server/internal.h | 9 ++++++
server/backend.c | 29 +++++++++++++++++++
server/filters.c | 17 +++++++++++
server/plugins.c | 62 +++++++++++++++++++++++++++++++++++++++++
6 files changed, 124 insertions(+)
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 8b587d70..4c620b4f 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -84,6 +84,8 @@ struct nbdkit_next_ops {
/* These callbacks are the same as normal plugin operations. */
int64_t (*get_size) (nbdkit_next *nxdata);
const char * (*export_description) (nbdkit_next *nxdata);
+ int (*block_size) (nbdkit_next *nxdata,
+ uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum);
int (*can_write) (nbdkit_next *nxdata);
int (*can_flush) (nbdkit_next *nxdata);
@@ -219,6 +221,8 @@ struct nbdkit_filter {
int64_t (*get_size) (nbdkit_next *next,
void *handle);
const char * (*export_description) (nbdkit_next *next, void *handle);
+ int (*block_size) (nbdkit_next *next, void *handle,
+ uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum);
int (*can_write) (nbdkit_next *next,
void *handle);
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 59ec11b6..50ff93df 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -146,6 +146,9 @@ struct nbdkit_plugin {
const char * (*export_description) (void *handle);
void (*cleanup) (void);
+
+ int (*block_size) (void *handle,
+ uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum);
};
NBDKIT_EXTERN_DECL (void, nbdkit_set_error, (int err));
diff --git a/server/internal.h b/server/internal.h
index f4843164..c91d196e 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -216,6 +216,9 @@ struct context {
unsigned char state; /* Bitmask of HANDLE_* values */
uint64_t exportsize;
+ uint32_t minimum_block_size;
+ uint32_t preferred_block_size;
+ uint32_t maximum_block_size;
int can_write;
int can_flush;
int is_rotational;
@@ -368,6 +371,8 @@ struct backend {
const char *(*export_description) (struct context *);
int64_t (*get_size) (struct context *);
+ int (*block_size) (struct context *,
+ uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum);
int (*can_write) (struct context *);
int (*can_flush) (struct context *);
int (*is_rotational) (struct context *);
@@ -433,6 +438,10 @@ extern const char *backend_export_description (struct
context *c)
__attribute__((__nonnull__ (1)));
extern int64_t backend_get_size (struct context *c)
__attribute__((__nonnull__ (1)));
+extern int backend_block_size (struct context *c,
+ uint32_t *minimum, uint32_t *preferred,
+ uint32_t *maximum)
+ __attribute__((__nonnull__ (1, 2, 3, 4)));
extern int backend_can_write (struct context *c)
__attribute__((__nonnull__ (1)));
extern int backend_can_flush (struct context *c)
diff --git a/server/backend.c b/server/backend.c
index 1bb3dac9..7d46b99d 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -214,6 +214,7 @@ static struct nbdkit_next_ops next_ops = {
.finalize = backend_finalize,
.export_description = backend_export_description,
.get_size = backend_get_size,
+ .block_size = backend_block_size,
.can_write = backend_can_write,
.can_flush = backend_can_flush,
.is_rotational = backend_is_rotational,
@@ -265,6 +266,7 @@ backend_open (struct backend *b, int readonly, const char
*exportname,
c->conn = shared ? NULL : conn;
c->state = 0;
c->exportsize = -1;
+ c->minimum_block_size = c->preferred_block_size =
c->maximum_block_size = -1;
c->can_write = readonly ? 0 : -1;
c->can_flush = -1;
c->is_rotational = -1;
@@ -418,6 +420,33 @@ backend_get_size (struct context *c)
return c->exportsize;
}
+int
+backend_block_size (struct context *c,
+ uint32_t *minimum, uint32_t *preferred, uint32_t *maximum)
+{
+ PUSH_CONTEXT_FOR_SCOPE (c);
+ struct backend *b = c->b;
+ int r;
+
+ assert (c->handle && (c->state & HANDLE_CONNECTED));
+ if (c->minimum_block_size != -1) {
+ *minimum = c->minimum_block_size;
+ *preferred = c->preferred_block_size;
+ *maximum = c->maximum_block_size;
+ return 0;
+ }
+ else {
+ controlpath_debug ("%s: block_size", b->name);
+ r = b->block_size (c, minimum, preferred, maximum);
+ if (r == 0) {
+ c->minimum_block_size = *minimum;
+ c->preferred_block_size = *preferred;
+ c->maximum_block_size = *maximum;
+ }
+ return r;
+ }
+}
+
int
backend_can_write (struct context *c)
{
diff --git a/server/filters.c b/server/filters.c
index 93ee55f0..ae7e1a56 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -358,6 +358,22 @@ filter_get_size (struct context *c)
return backend_get_size (c_next);
}
+static int
+filter_block_size (struct context *c,
+ uint32_t *minimum, uint32_t *preferred, uint32_t *maximum)
+{
+ struct backend *b = c->b;
+ struct backend_filter *f = container_of (b, struct backend_filter, backend);
+ struct context *c_next = c->c_next;
+
+ if (f->filter.block_size)
+ return f->filter.block_size (c_next, c->handle,
+ minimum, preferred, maximum);
+ else
+ return backend_block_size (c_next,
+ minimum, preferred, maximum);
+}
+
static int
filter_can_write (struct context *c)
{
@@ -621,6 +637,7 @@ static struct backend filter_functions = {
.close = filter_close,
.export_description = filter_export_description,
.get_size = filter_get_size,
+ .block_size = filter_block_size,
.can_write = filter_can_write,
.can_flush = filter_can_flush,
.is_rotational = filter_is_rotational,
diff --git a/server/plugins.c b/server/plugins.c
index b633c107..346c6d52 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -46,6 +46,7 @@
#include "internal.h"
#include "minmax.h"
+#include "ispowerof2.h"
/* We extend the generic backend struct with extra fields relating
* to this plugin.
@@ -173,6 +174,7 @@ plugin_dump_fields (struct backend *b)
HAS (close);
HAS (export_description);
HAS (get_size);
+ HAS (block_size);
HAS (can_write);
HAS (can_flush);
HAS (is_rotational);
@@ -408,6 +410,65 @@ plugin_get_size (struct context *c)
return p->plugin.get_size (c->handle);
}
+static int
+plugin_block_size (struct context *c,
+ uint32_t *minimum, uint32_t *preferred, uint32_t *maximum)
+{
+ struct backend *b = c->b;
+ struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
+ int r;
+
+ if (p->plugin.block_size) {
+ r = p->plugin.block_size (c->handle, minimum, preferred, maximum);
+ if (r == 0) {
+ /* To make scripting easier, it's permitted to set
+ * minimum = preferred = maximum = 0 and return 0.
+ * That means "no information", and works the same
+ * way as the else clause below.
+ */
+ if (*minimum == 0 && *preferred == 0 && *maximum == 0)
+ return 0;
+
+ if (*minimum < 1 || *minimum > 65536) {
+ nbdkit_error ("plugin must set minimum block size between 1 and
64K");
+ r = -1;
+ }
+ if (! is_power_of_2 (*minimum)) {
+ nbdkit_error ("plugin must set minimum block size to a power of
2");
+ r = -1;
+ }
+ if (! is_power_of_2 (*preferred)) {
+ nbdkit_error ("plugin must set preferred block size to a power of
2");
+ r = -1;
+ }
+ if (*preferred < 512 || *preferred > 32 * 1024 * 1024) {
+ nbdkit_error ("plugin must set preferred block size "
+ "between 512 and 32M");
+ r = -1;
+ }
+ if (*maximum != (uint32_t)-1 && (*maximum % *minimum) != 0) {
+ nbdkit_error ("plugin must set maximum block size "
+ "to -1 or a multiple of minimum block size");
+ r = -1;
+ }
+ if (*minimum > *preferred || *preferred > *maximum) {
+ nbdkit_error ("plugin must set minimum block size "
+ "<= preferred <= maximum");
+ r = -1;
+ }
+ }
+ return r;
+ }
+ else {
+ /* If there is no .block_size call then return minimum = preferred
+ * = maximum = 0, which is a sentinel meaning don't send the
+ * NBD_INFO_BLOCK_SIZE message.
+ */
+ *minimum = *preferred = *maximum = 0;
+ return 0;
+ }
+}
+
static int
normalize_bool (int value)
{
@@ -824,6 +885,7 @@ static struct backend plugin_functions = {
.close = plugin_close,
.export_description = plugin_export_description,
.get_size = plugin_get_size,
+ .block_size = plugin_block_size,
.can_write = plugin_can_write,
.can_flush = plugin_can_flush,
.is_rotational = plugin_is_rotational,
--
2.35.1
Eric Blake
2022-Feb-17 16:19 UTC
[Libguestfs] [PATCH nbdkit v2 1/7] server: Add new plugin/filter .block_size callback
On Thu, Feb 17, 2022 at 02:36:42PM +0000, Richard W.M. Jones wrote:> This commit implements backend_block_size() and wires it up to a new > .block_size callback from the plugin through any filters on top. > > This callback will allow plugins to pass their minimum, preferred and > maximum block size through the NBD protocol NBD_INFO_BLOCK_SIZE info > option. > > The new function is not called from anywhere yet, so for the moment > this commit does nothing. > --- > include/nbdkit-filter.h | 4 +++ > include/nbdkit-plugin.h | 3 ++ > server/internal.h | 9 ++++++ > server/backend.c | 29 +++++++++++++++++++ > server/filters.c | 17 +++++++++++ > server/plugins.c | 62 +++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 124 insertions(+)I may still play with adding a 'bool strict' parameter as a followup series on top of yours in the next couple of days; as long as I post it prior to this going in a stable release, we will be able to see what the difference would look like. Or after playing with it, I may also decide that the work to add in the parameter just for the sake of the nbd plugin turns out too complex, at which point living with your signature would be the solution anyways. So I don't see a reason to hold up your series just waiting for me to do a proposed followup on top.> > diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h > index 8b587d70..4c620b4f 100644 > --- a/include/nbdkit-filter.h > +++ b/include/nbdkit-filter.h > @@ -84,6 +84,8 @@ struct nbdkit_next_ops { > /* These callbacks are the same as normal plugin operations. */ > int64_t (*get_size) (nbdkit_next *nxdata); > const char * (*export_description) (nbdkit_next *nxdata); > + int (*block_size) (nbdkit_next *nxdata, > + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum);I think even when we add 64-bit extensions that we will still want to keep maximum as a 32-bit size, because it controls the buffer size we pass to read and write. Part of the 64-bit extension work is figuring out how a server can advertise that it supports 64-bit zero/trim/cache even when it is limited to 32-bit (more specifically, 32M or 64M) read/write; so I will probably be proposing an additional NBD_INFO_* tag for advertising a 64-bit non-data limit down the road. In fact, maybe we should plan for that right away, by having four parameters, ending with 'uint32_t *maxdata, uint64_t *maxlen'. This would match the fact that right now, the blocksize filter allows maxdata=32M maxlen=3G to break up large pwrite requests differently than large zero requests. For the existing NBD protocol, only the first three will be advertised to the client (and we ignore the fourth for now), but down the road with 64-bit extensions, it will make it easier to advertise a 64-bit non-data max length alongside the existing 3 32-bit constraints, as the NBD protocol gains another extension. The alternative is that if NBD gains a new NBD_INFO_* tag, we would have to add a new plugin callback to supply the value for that tag. I still think that it is nicer to have one parameter for tag we know about (whether or not we can send the tag to the guest). I can think of a more extensible interface, shown below, but think it is harder for plugins fto use correctly, and therefore do not recommend it: enum nbdkit_size_tag { NBDKIT_SIZE_TAG_MINIMUM = 0, NBDKIT_SIZE_TAG_PREFERRED = 1, NBDKIT_SIZE_TAG_MAXDATA = 2, NBDKIT_SIZE_TAG_MAXLEN = 3, }; struct nbdkit_size_constraint_set; /* opaque */ int (*block_size) (nbdkit_next *next, void *handle, nbdkit_size_constraint_set *set); int nbdkit_size_constraint_add (nbdkit_size_constraint_set *set, enum nbdkit_size_tag tag, uint64_t value); where the plugin would then call something like: plugin_block_size (...) { nbdkit_size_constraint_add (set, NBDKIT_SIZE_TAG_MINIMUM, 1); nbdkit_size_constraint_add (set, NBDKIT_SIZE_TAG_PREFERRED, 64 * 1024); nbdkit_size_constraint_add (set, NBDKIT_SIZE_TAG_MAXLEN, 1ULL<<40); }> +++ b/server/backend.c > @@ -214,6 +214,7 @@ static struct nbdkit_next_ops next_ops = { > .finalize = backend_finalize, > .export_description = backend_export_description, > .get_size = backend_get_size, > + .block_size = backend_block_size, > .can_write = backend_can_write, > .can_flush = backend_can_flush, > .is_rotational = backend_is_rotational, > @@ -265,6 +266,7 @@ backend_open (struct backend *b, int readonly, const char *exportname, > c->conn = shared ? NULL : conn; > c->state = 0; > c->exportsize = -1; > + c->minimum_block_size = c->preferred_block_size = c->maximum_block_size = -1;So we actually implement two sentinels - all -1 (we haven't initialized yet, and a plugin cannot explicitly set things to this value, because it violates minimum <= 64k), and all 0 (initialized but nothing to advertise), in addition to all other values (initialized, and passed our constraint validator so worth advertising).> > +static int > +plugin_block_size (struct context *c, > + uint32_t *minimum, uint32_t *preferred, uint32_t *maximum) > +{ > + struct backend *b = c->b; > + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > + int r; > + > + if (p->plugin.block_size) { > + r = p->plugin.block_size (c->handle, minimum, preferred, maximum); > + if (r == 0) { > + /* To make scripting easier, it's permitted to set > + * minimum = preferred = maximum = 0 and return 0. > + * That means "no information", and works the same > + * way as the else clause below. > + */ > + if (*minimum == 0 && *preferred == 0 && *maximum == 0) > + return 0; > + > + if (*minimum < 1 || *minimum > 65536) { > + nbdkit_error ("plugin must set minimum block size between 1 and 64K"); > + r = -1; > + } > + if (! is_power_of_2 (*minimum)) { > + nbdkit_error ("plugin must set minimum block size to a power of 2"); > + r = -1; > + }Did we want to stick all of the constraint validation code in a common helper function, rather than open-coding it at each place that wants to perform validation? Otherwise, this is looking good. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org