[Libguestfs] [nbdkit PATCH 2/5] protocol: Validate request flags

[Richard W.M. Jones]

On Thu, Jan 26, 2017 at 08:08:46AM -0600, Eric Blake wrote:
> On 01/26/2017 04:18 AM, Richard W.M. Jones wrote:
> > On Wed, Jan 25, 2017 at 08:55:18PM -0600, Eric Blake wrote:
> >> On 01/20/2017 02:16 PM, Eric Blake wrote:
> >>> Reject rather than silently ignoring unknown client request flags.
> >>>
> >>
> >>>
> >>> +  /* Validate flags */
> >>> +  if (flags & ~NBD_CMD_FLAG_FUA) {
> >>> +    nbdkit_error ("invalid request: unknown flag (0x%x)", flags);
> >>> +    *error = EINVAL;
> >>> +    return 0;
> >>> +  }
> >>
> >> Right now, our NBD_CMD_FLAG_FUA implementation causes a full flush
> >> action from the plugin, even if it is possible to write a client that
> >> knows how to preserve FUA semantics in a lighter-weight manner than a
> >> full fdatasync().  In other words, it obeys the semantics required by
> >> the NBD protocol, but not necessarily in the most optimum way.
> > 
> > Does NBD (protocol) now support a more fine-grained flush?
> 
> Only via the Force-unit-access (FUA) flag.  The obvious candidate for
> this is anything targetting SCSI, which has FUA semantics (the command
> can't return until the sectors involved in the command are flushed, but
> it does NOT have to flush sectors unrelated to the command); the kernel
> exposes FUA semantics through its I/O layer to other file systems.  If
> we were to implement an NBD client plugin (to create proxy NBD
> connections), passing the FUA flag on to the ultimate server would make
> sense, especially if the ultimate server can handle FUA more efficiently
> than a full flush.
> 
> > 
> >> Unfortunately, the callback interfaces for a plugin do not have any way
> >> to pass flags from the client to the plugin (other than my new .zero
> >> callback, but right now it only supports a single may_trim argument used
> >> as a boolean, rather than an actual int flags argument).  Do we want or
> >> need to enhance the set of callback interfaces to allow plugins that can
> >> act on flag values, rather than always implementing fua semantics
> >> ourselves by the heavy-weight .flush call?
> > 
> > Can we add a flush2 method which adds flags?
> 
> It's not flush that would need the flag, but write, trim, and zero.  For
> now, I'm not too worried about it (we don't have any complaints of
> someone unable to implement FUA semantics in a plugin), but it is food
> for thought that we will probably have to add additional callbacks, and
> keep backwards compatibility for which callback to use, if we want to
> someday expose the flag all the way through the call stack.

OK now I understand.  We need to maintain compatibility with existing
plugins, but we can add new methods with flags (eg write2 etc).
There's probably not a pressing reason right now however.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top