[Libguestfs] [PATCH nbdkit v2 7/7] New filter: nbdkit-block-size-constraint-filter

[Richard W.M. Jones]

> > Subject: Re: [PATCH nbdkit v2 7/7] New filter: nbdkit-block-size-constraint-filter

Now also notice that I didn't change the commit message!  Fixed in my
local copy.

On Thu, Feb 17, 2022 at 12:40:23PM -0600, Eric Blake wrote:
> On Thu, Feb 17, 2022 at 02:36:48PM +0000, Richard W.M. Jones wrote:
...
> > +  nbdkit --filter=blocksize-policy \
> > +         --filter=blocksize \
> > +         PLUGIN ... \
> > +         blocksize-error-policy=allow \
> > +         blocksize-minimum=64K minblock=64K
> > +
> > +This says to advertise a minimum block size of 64K.  Well-behaved
> > +clients will obey this.  Badly behaved clients will send requests
> > +S<E<lt> 64K> which will be converted to slow 64K read-modify-write
> > +cycles to the underlying plugin.  In either case the plugin will only
> > +see requests on 64K (or multiples of 64K) boundaries.
> 
> Should the blocksize filter implement .block_size?  But that's a
> separate patch compared to this one adding a new filter.  The example
> looks good.

I think there's an argument for the blocksize filter returning minimum
the same as minblock (if set) and maximum the same as
MAX (maxlen, maxdata), but I think it is a separate patch.

...
> > +    if (config_maximum)
> > +      *maximum = config_maximum;
> > +    else
> > +      *maximum = 0xffffffff;
> 
> I'd be inclined to advertise 64*1024*1024 here instead of 0xffffffff,
> to match the actual buffer size that we know nbdkit is constrained by.
>
> Then again, doing that may cause clients to self-limit calls to .zero
> with at most 64M in a chunk, instead of calling it with 4G in a chunk.
> It's a toss-up.  (The NBD protocol really does need a fourth value, to
> distinguish between maximum buffer and maximum length)

Agreed.  Clients shouldn't set > 32M pread/pwrite requests regardless
of the limit, if I'm reading the spec correctly.

...
> > +/* This function checks the error policy for all request functions below. */
> > +static int
> > +check_policy (nbdkit_next *next, const char *type,
> > +              uint32_t count, uint64_t offset, int *err)
> 
> Ah, new code compared to the last draft ;)
> 
> > +{
> > +  uint32_t minimum, preferred, maximum;
> > +
> > +  if (error_policy == ALLOW)
> > +    return 0;
> > +
> > +  /* Get the current block size constraints.  Note these are cached in
> > +   * the backend so if they've already been computed then this simply
> > +   * returns the cached values.  The plugin is only asked once per
> > +   * connection.
> > +   */
> > +  errno = 0;
> > +  if (next->block_size (next, &minimum, &preferred, &maximum) == -1) {
> > +    *err = errno ? : EIO;
> > +    return -1;
> > +  }
> 
> Correct.
> 
> > +
> > +  /* If there are no constraints, allow. */
> > +  if (minimum == 0)
> > +    return 0;
> > +
> > +  /* Check constraints. */
> > +  if (count < minimum) {
> > +    *err = EIO;
> > +    nbdkit_error ("client %s request rejected: "
> > +                  "count %" PRIu32 " is smaller than minimum size %" PRIu32,
> > +                  type, count, minimum);
> > +    return -1;
> > +  }
> > +  if (count > maximum) {
> > +    *err = EIO;
> > +    nbdkit_error ("client %s request rejected: "
> > +                  "count %" PRIu32 " is larger than maximum size %" PRIu32,
> > +                  type, count, maximum);
> > +    return -1;
> > +  }
> 
> This is where I worry about size constraints on data (pread/pwrite)
> vs. length (zero/trim/cache).  Again, I'm happy to propose a patch on
> top of this, so we can use this as our starting point.

Hmm, this is a good point that I didn't think about.

Maybe make the maximum check here conditional on it not being
a non-data request?

> > +  if ((count % minimum) != 0) {
> > +    *err = EIO;
> > +    nbdkit_error ("client %s request rejected: "
> > +                  "count %" PRIu32 " is not a multiple "
> > +                  "of minimum size %" PRIu32,
> > +                  type, count, minimum);
> > +    return -1;
> > +  }
> > +  if ((offset % minimum) != 0) {
> > +    *err = EIO;
> > +    nbdkit_error ("client %s request rejected: "
> > +                  "offset %" PRIu64 " is not aligned to a multiple "
> > +                  "of minimum size %" PRIu32,
> > +                  type, offset, minimum);
> > +    return -1;
> > +  }
> 
> Do we also want to override .get_size, to flag (or round down) the
> case where the size reported by the plugin is not a multiple of the
> plugin's minsize, at which point those trailing bytes are inaccessbile
> to the client?  Could be done on top.
> 
> Overall the series looks good, I'll leave it up to you if you think it
> is ready to push now or if you want to adjust for one more round of
> reviews, and meanwhile I can start working on my proposed tweaks to
> compare on top to see how they look.

I've got a bit of thinking to do on it.

BTW I extended nbdkit-nbd-plugin proxy, OCaml and Python already, will
send those patches separately.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v