On 5/13/19 2:56 AM, Richard W.M. Jones wrote:
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@redhat.com>
---
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.
Yes, I still need to revisit my NBD protocol proposal for an additional flag during WRITE_ZEROES for a client to easily get fast failure if it would otherwise be emulated. It may make .can_zero turn into tri-state :)
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.
Okay, I'll rework the series along those lines, where .can_cache has to be set for a plugin/filter to opt-in to advertising the feature to clients (rather than always blindly advertising it). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org