[Libguestfs] [PATCH nbdkit 1/8] server: Implement extents/can_extents calls for plugins and filters.

[Eric Blake]

On 3/25/19 12:01 PM, Richard W.M. Jones wrote:

>>> +This callback is not required.  If omitted, then we return true iff a
>>> +C<.extents> callback has been defined.
>>
>> Should we default this to returning true always, _because_ we have the
>> sane fallback of treating the entire image as allocated when .extents is
>> absent?  (That is, a plugin has to explicitly opt out of advertising
>> NBD_CMD_BLOCK_STATUS support, because our default works well enough even
>> if the plugin didn't do anything). It would match how filters can call
>> next_ops->zero() even for plugins that do not have .zero (because
>> .can_zero defaults to true).
> 
> There is definitely a case for removing can_extents completely, and
> relying on the fallback.  Would this affect language bindings?  I
> think no because in the language bindings we can also emulate the
> .extents callback.
> 
> Associated with this change would be to change the server so it always
> advertises and enables base:allocation (if the client requests it).
> 
> I'm trying to think if there's any reason not to make both of those
> changes, and failing to think of a reason at the moment ...

The 'fua' filter is a case where not advertising something, for the
purpose of timing comparisons (basically, comparisons of whether it is
more efficient for the client to always be told that the entire image is
allocated, vs. the client having to assume that the entire image is
allocated because it can't even query), is the only reason I can think
of for why we would want to let a filter/plugin forcefully tell nbdkit
to not advertise block status.


>>> +++ b/server/extents.c
>>> @@ -0,0 +1,171 @@
>>
>>> +struct nbdkit_extents {
>>> +  struct nbdkit_extent *extents;
>>> +  size_t nr_extents, allocated;
>>> +  uint64_t start;
>>> +};
>>
>> Is it also worth tracking cumulative length covered by extents?
>> Tracking it would give us an O(1) instead of O(n) answer to "how many
>> bytes did the plugin tell us about" - which MAY matter to filters that
>> want to append additional data about a hole after the underlying plugin
> 
> I may have misunderstood, but isn't that given by:
> extents[nr_extents-1].offset + extents[nr_extents-1].length - start ?

Oh, good call.  So as long as .offset is part of each nbdkit_extent,
yes, a filter can compute total length in O(1) instead of O(n). (If
nbdkit_extent tracks only length, then you have to iterate over all n
extents to add the cumulative length back up).

> 
> Anyway at the moment I'm not greatly worried about the implementation
> of extents or how filters are implemented, as long as we have got the
> plugin API right.

I also think you are right that we can change the implementation of
extents without hurting plugins (even if we change the content of struct
nbdkit_extent, only filters use that struct).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190325/7182eb3b/attachment.sig>