[Libguestfs] [PATCH nbdkit v2 1/7] server: Add new plugin/filter .block_size callback

[Richard W.M. Jones]

On Thu, Feb 17, 2022 at 10:19:54AM -0600, Eric Blake wrote:
> 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.

Presuming that we're going to need a new NBDKIT_API_VERSION for this
anyway, we can in fact change the signature of .block_size by adding a
new parameter.  This doesn't break backwards compatiblity.

[...]
> > +++ 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).

Hopefully the -1 sentinel is never visible outside the server, and
could in fact be changed (eg. adding an extra "initialized" boolean).

If I've got it right, it should neither be seen by plugins, filters or
the client(!), nor should the plugin or filter be allowed to set any
of these to -1, except maximum.

> >  
> > +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?

The code is a bit different unfortunately ..  In the filter code we
have to consider the case where some of the config_* variables are
unset.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW