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

On 3/12/19 12:11 PM, Richard W.M. Jones wrote: True only if any earlier filter also passed in an empty map. Maybe it's worth explicitly mentioning that the filter must in turn pass an empty map if it calls next_ops? And the fact that we can easily call nbdkit_extents_new() as needed means that a filter could, for example, call next_ops->extents() even for its pread implementation (of course, only for a plugin that .can_extents), if the filter knowing which parts of the plugin are sparse can make the filter run more efficiently on calls other than NBD_CMD_BLOCK_STATUS. Is it worth mentioning that nbdkit's default behavior is to treat the entire image as allocated non-zero if .can_extents returns false? Is an initial hole the wisest default, or should the initial state be data, and the plugin can do an nbdkit_extent_add(0, size, HOLE) if the client finds it easier to start with all holes? After all, the NBD protocol states that returning data/allocated is always the safest default (returning hole and/or read-zeroes should only be done when we are sure about it, but returning data/allocated is fine even if it causes the client to be less efficient for reading holes that it could have otherwise skipped). Is it worth mentioning that speed is slightly more important than accuracy (that is, it's better to quickly return DATA than to slowly read the are in question and memcmp() to see if it is all zeros in order to return a more precise ZERO; ideally, returning anything other than DATA should only be done when the time collecting that information is proportionate to the number of requests rather to the size of the region being queried). Is nbdkit allowed to cache the information learned from the plugin for regions outside the client's initial query for later use, or are we better off always asking for information again (even if the answer we get will be the same)? The protocol allows all four combinations of the two bits (you could have a non-allocated area not known to read as zeroes, for example, some SCSI disks have this property); should we likewise have four potential different types, rather than just three? That's a nice aspect - it means a client CAN call add(0, full_size, HOLE) and then refine it with subsequent add(offset, length, DATA). It also seems like a fairly straightforward implementation - keep a sorted list of extents, and then split/coalescse into the right sorted location as more add() requests come in. Then it is an implementation detail whether we track the list by array, linked list, or binary tree, which in turn may be determined by how many extents we expect to be dealing with (choosing an implementation with O(log n) lookup/insertion rather than O(n) in case the client gives us LOTS of add() calls may suggest that a tree is best, but prototyping with an array for the initial cut is less code complexity). Question - should foreach allow the caller to pass in bounds? That is, since the plugin can populate 0 - full_size, but where we know the client only asked for information on offset - length, it may be more efficient to visit just the extents lying within those bounds. (Maybe allow -1 as a shortcut rather than an actual end size, when the ending offset is less important). Is it worth an explicit guarantee that foreach() visits extents in ascending order, regardless of whether add() was called out of order? (Ties us to our implementation where we split/coalesce per add - but seems like it can make life easier for filters if we do have that guarantee). either the failing C<fn>, or the caller of foreach(). No documentation of nbdkit_extents_{new,free}? Repeating the question above about whether to expose all four bit combinations possible in the NBD protocol. Okay, so we're still in the design phase :) There's probably a lot of the .can_FOO calls that should be cached per connection. Still, it's looking fairly reasonable. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org