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