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

I want to post this but mainly for discussion and early review. It's not safe for these patches to all go upstream yet (because not all filters have been checked/adjusted), but if any patches were to go upstream then probably 1 & 2 only are safe. File, VDDK, memory and data plugins all work, although I have only done minimal testing on them. The current tests, such as they are, all

Second version based on nbdkit_extent* calls, as discussed here: https://www.redhat.com/archives/libguestfs/2019-March/msg00033.html Note in particular there is some subtlety about how filters would work in this implementation. See docs/nbdkit-filter.pod for the details. Rich.

On 3/12/19 12:11 PM, Richard W.M. Jones wrote: > This pair of calls allows plugins to describe which extents in the > virtual disk are allocated, holes or zeroes. > --- > +++ b/docs/nbdkit-filter.pod > +The C<extents_map> parameter passed to this function is empty. True only if any earlier filter also passed in an empty map. Maybe it's worth explicitly mentioning that

I think the extents map is just too complicated and is unnecessarily so. How about instead we define the plugin interface to be: int can_extents (void *handle); // as before int extents (void *handle, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents_list *list); and have the extents_list be a simple list. The first extent you add must start at offset.

This pair of calls allows plugins to describe which extents in the virtual disk are allocated, holes or zeroes. --- docs/nbdkit-filter.pod | 64 +++++++++++++++++++++ docs/nbdkit-plugin.pod | 123 ++++++++++++++++++++++++++++++++++++++++ include/nbdkit-common.h | 20 ++++++- include/nbdkit-filter.h | 15 ++++- include/nbdkit-plugin.h | 6 +- server/internal.h | 7 ++-

On 3/19/19 11:34 AM, Richard W.M. Jones wrote: > This pair of calls allows plugins to describe which extents in the > virtual disk are allocated, holes or zeroes. > --- > + void nbdkit_extents_free (struct nbdkit_extents_map *); > + > +Frees an existing extents map. Is this like free() where it is safe to call on NULL? > + > +=head3 Iterating over nbdkit_extents_map

This pair of calls allows plugins to describe which extents in the virtual disk are allocated, holes or zeroes. --- docs/nbdkit-filter.pod | 107 ++++++++ docs/nbdkit-plugin.pod | 103 +++++++ include/nbdkit-common.h | 10 +- include/nbdkit-filter.h | 25 +- include/nbdkit-plugin.h | 6 +- server/internal.h | 5 + server/extents.c | 574

This tentative commit implements extents/can_extents, roughly as discussed in the previous thread here: https://www.redhat.com/archives/libguestfs/2019-March/msg00017.html I can't say that I'm a big fan of having the plugin allocate an extents array. There are no other plugin callbacks currently where we require the plugin to allocate complex data structures (or indeed do any allocation

I'm not sure exactly which version we're up to, but let's say it's version 4. I'm a lot happier with this version: - all filters have been reviewed and changed where I think that's necessary - can_extents is properly defined and implemented now - NBD protocol is followed - I believe it addresses all previous review points where possible The "only" thing

Not sure what version we're up to, but this reimplements extents using the new simpler structure described in this thread: https://www.redhat.com/archives/libguestfs/2019-March/msg00077.html I also fixed most of the things that Eric pointed out in the previous review, although I need to go back over his replies and check I've got everything. This needs a bit more testing. However the

I still don't have .prepare/.finalize working cleanly across reopen, but did find a nasty bug where a botched assertion means we failed to notice reads beyond EOF in both the xz and retry filter. Refactoring backend.c will make .finalize work easier. Eric Blake (5): xz: Avoid reading beyond EOF retry: Check size before transactions tests: Test retry when get_size values change

It's easy to use the sh script to demonstrate that nbdkit is inefficiently calling into .get_size, .can_fua, and friends more than necessary. We've also commented on the list in the past that it would be nice to ensure that when filters call into next_ops, they are not violating constraints (as we've have to fix several bugs in the past where we did not have such checking to protect

I'm still working my way through the filters before this series will be complete, but this is enough of a start to at least get some feedback on the idea of implementing another NBD protocol extension. Eric Blake (9): server: Internal hooks for implementing NBD_CMD_CACHE plugins: Add .cache callback file, split: Implement .cache with posix_fadvise nbd: Implement NBD_CMD_CACHE

This has already been pushed upstream. I am simply posting these here so we have a reference in the mailing list in case we find bugs later (as I'm sure we will - it's a complex patch series). Great thanks to Eric Blake for tireless review on this one. It also seems to have identified a few minor bugs in qemu along the way. Rich.

I think this is my last round of patches for issues I identified with the retry filter. With this in place, it should be safe to interject another filter in between retry and the plugin. Eric Blake (5): retry: Don't call into closed plugin tests: Refactor test-retry-reopen-fail.sh tests: Enhance retry test to cover failed reopen server: Move prepare/finalize/close recursion to

Since v1: - rework .can_cache to be tri-state, with default of no advertisement (ripple effect through other patches) - add a lot more patches in order to round out filter support And in the meantime, Rich pushed NBD_CMD_CACHE support into libnbd, so in theory we now have a way to test cache commands through the entire stack. Eric Blake (24): server: Internal hooks for implementing

The third patch is a large but mechanical change which gets rid of passing around struct connection * entirely within the server, preferring instead to reference the connection through thread-local storage. I hope this is a gateway to simplifying other parts of the code. Rich.

v1 was here: https://www.redhat.com/archives/libguestfs/2020-February/msg00081.html v2 replaces struct connection *conn = GET_CONN; with GET_CONN; which sets conn implicitly and asserts that it is non-NULL. If we actually want to test if conn is non-NULL or behave differently, then you must use threadlocal_get_conn() instead, and some existing uses do that. Rich.

This pair of calls allows plugins to describe which extents in the virtual disk are allocated, holes or zeroes. --- docs/nbdkit-filter.pod | 45 +++++++++++++++++ docs/nbdkit-plugin.pod | 106 ++++++++++++++++++++++++++++++++++++++++ include/nbdkit-common.h | 18 ++++++- include/nbdkit-filter.h | 12 ++++- include/nbdkit-plugin.h | 6 ++- server/internal.h | 7 ++- server/extents.c

I will be following up to this email with four separate threads each addressed to the appropriate single list, with proposed changes to: - the NBD protocol - qemu: both server and client - libnbd: client - nbdkit: server The feature in question adds a new optional NBD_INFO_ packet to the NBD_OPT_GO portion of handshake, adding up to 16 bits of information that the server can advertise to the