Re: [Libguestfs] [nbdkit PATCH 1/9] server: Internal hooks for implementing NBD_CMD_CACHE

On Thu, May 09, 2019 at 10:03:34PM -0500, Eric Blake wrote: > Until the next few patches expose and implement new callbacks for > filters and plugins, our initial implementation for NBD_CMD_CACHE is > to just blindly advertise the feature, and call into .pread with an > ignored buffer. > > Note that for bisection reasons, this patch treats any use of a filter > as a forced .can_cache of false to bypass the filter's caching; once > all affected filters are patched to handle cache requests correctly, a > later patch will then switch the filter default to passthrough for the > sake of remaining filters. > > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > > --- > > Note: we could also choose to always advertise caching, but make the > nbdkit default version be a no-op rather than call out to .pread. For > that matter, maybe a plugin's .can_cache should allow the plugin to > choose between a tri-state of no-op, fallback to .pread, or call > .cache; I'm not sure which is the saner default for a random plugin > compiled against older nbdkit. At least it's fairly easy to write a > filter that changes the default from no-op to .pread or from .pread to > no-op. Be thinking about that as you review the rest of the series. I'm worried that we end up in the same situation that we do with .zero: because we emulate it, we end up with a slow zero support, whereas clients [well, Nir S. mainly] actually want NBD_FLAG_SEND_WRITE_ZEROES == *fast* zeroing is available.

In particular in the NBD_CMD_CACHE case: for some plugins emulating this behind the scenes with pread makes some sense. eg. It works for nbdkit-file-plugin because it will prefetch the data into Linux's page cache. However if a plugin makes a network access, eg. nbdkit-vddk-plugin, that can be both very expensive and has no prefetching benefit. Therefore I do think we should default to ignoring NBD_CMD_CACHE commands and *not* advertising the feature, except when the plugin says that it is able to handle it. For tri-stating .can_cache: yes that makes sense, because there is moderate difficulty for a plugin to emulate .cache using .pread. The plugin has to at least allocate a dummy buffer and throw it away, and we could probably do something more efficient than that if we implement an emulate mode in the server.