Libguestfs - Jan 2017 - Re: [nbdkit PATCH 2/5] protocol: Validate request flags

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.

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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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?
> 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?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org