search for: filter_extents

..._cache", f->name); + + /* FIXME: Default to f->backend.next->can_cache, once all filters + have been audited */ + return 0; +} + static int filter_pread (struct backend *b, struct connection *conn, void *buf, uint32_t count, uint64_t offset, @@ -702,6 +714,23 @@ filter_extents (struct backend *b, struct connection *conn, extents, err); } +static int +filter_cache (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + struct backend_filter *f = cont...

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

...gt;backend.next->can_extents (f->backend.next, conn); +} + static int filter_can_fua (struct backend *b, struct connection *conn) { @@ -646,6 +680,31 @@ filter_zero (struct backend *b, struct connection *conn, count, offset, flags, err); } +static int +filter_extents (struct backend *b, struct connection *conn, + uint32_t count, uint64_t offset, uint32_t flags, + size_t *nr_extents, struct nbdkit_extent **extents, + int *err) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + void *...

...count, offset, flags, err); else - return f->backend.next->zero (f->backend.next, conn, - count, offset, flags, err); + return b->next->zero (b->next, conn, count, offset, flags, err); } static int @@ -757,8 +747,8 @@ filter_extents (struct backend *b, struct connection *conn, struct nbdkit_extents *extents, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - void *handle = connection_get_handle (conn, f->backend.i); - struct b_conn nxdata = { .b = f->backend....

..._ops-E<gt>extents> if +C<next_ops-E<gt>can_extents> did not return true. + +The C<extents_map> parameter passed to this function is empty. If the +filter does not need to adjust extents from the underlying plugin it +can simply pass this through to the layer below: + + myfilter_extents (...) + { + return next_ops->extents (nxdata, count, offset, flags, extents_map, err); + } + +It is also possible for filters to transform the extents map received +back from the layer below. Usually this must be done by allocating a +new map which is passed to the layer below. Without error...

...xt_ops-E<gt>can_extents> did not return true. + +The C<extents_map> parameter passed to this function is empty +(representing a fully allocated disk). If the filter does not need to +adjust extents from the underlying plugin it can simply pass this +through to the layer below: + + myfilter_extents (...) + { + return next_ops->extents (nxdata, count, offset, flags, + extents_map, err); + } + +It is also possible for filters to transform the extents map received +back from the layer below. Usually this must be done by allocating a +new, empty map which is pass...

....c b/server/filters.c >index 5b7abc4..d6cc00f 100644 >--- a/server/filters.c >+++ b/server/filters.c [...] >@@ -646,6 +680,31 @@ filter_zero (struct backend *b, struct connection *conn, > count, offset, flags, err); > } > >+static int >+filter_extents (struct backend *b, struct connection *conn, >+ uint32_t count, uint64_t offset, uint32_t flags, >+ size_t *nr_extents, struct nbdkit_extent **extents, >+ int *err) >+{ >+ struct backend_filter *f = container_of (b, struct backend_filter,...

...> returned false; in +turn, the filter should not call C<next_ops-E<gt>extents> if +C<next_ops-E<gt>can_extents> did not return true. + +The C<extents> parameter passed to this function is empty, and +should usually be passed straight to the underlying plugin: + + myfilter_extents (...) + { + return next_ops->extents (nxdata, count, offset, flags, + extents, err); + } + +It is also possible for filters to transform the extents list received +back from the layer below. Without error checking it would look like +this: + + myfilter_extents (......

...> returned false; in +turn, the filter should not call C<next_ops-E<gt>extents> if +C<next_ops-E<gt>can_extents> did not return true. + +The C<extents> parameter passed to this function is empty, and +should usually be passed straight to the underlying plugin: + + myfilter_extents (...) + { + return next_ops->extents (nxdata, count, offset, flags, + extents, err); + } + +It is also possible for filters to transform the extents list received +back from the layer below. Without error checking it would look like +this: + + myfilter_extents (......

I'm not sure which version we're up to now. Anyway I believe this addresses all the points that Eric raised in: https://www.redhat.com/archives/libguestfs/2019-March/msg00038.html https://www.redhat.com/archives/libguestfs/2019-March/msg00040.html In particular: - default state of extents_map is all allocated disk - support hole + non-zero - you can now iterate with bounds -

...false; in +turn, the filter should not call C<next_ops-E<gt>extents> if +C<next_ops-E<gt>can_extents> did not return true. + +It is possible for filters to transform the extents list received back +from the layer below. Without error checking it would look like this: + + myfilter_extents (..., uint32_t count, uint64_t offset, ...) + { + size_t i; + struct nbdkit_extents *extents2; + struct nbdkit_extent e; + int64_t size; + + size = next_ops->get_size (nxdata); + extents2 = nbdkit_extents_new (offset + shift, size - shift); + next_ops->extents (nxdata, count, of...

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

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.

...) - return f->filter.zero (&next_ops, &nxdata, handle, + return f->filter.zero (&next_ops, nxdata, nxdata->handle, count, offset, flags, err); else return backend_zero (b->next, conn, count, offset, flags, err); @@ -652,10 +690,11 @@ filter_extents (struct backend *b, struct connection *conn, void *handle, struct nbdkit_extents *extents, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_conn nxdata = { .b = b->next, .conn = conn }; + struct b_conn *nxdata = handle; +...

...ter.zero) - return f->filter.zero (&next_ops, nxdata, nxdata->handle, + return f->filter.zero (&next_ops, b->next, handle, count, offset, flags, err); else return backend_zero (b->next, count, offset, flags, err); @@ -688,11 +621,9 @@ filter_extents (struct backend *b, void *handle, struct nbdkit_extents *extents, int *err) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - struct b_h *nxdata = handle; - assert (nxdata->b == b->next); if (f->filter.extents) - return f->fi...

...xt_ops-E<gt>can_extents> did not return true. + +The C<extents_map> parameter passed to this function is empty +(representing a fully allocated disk). If the filter does not need to +adjust extents from the underlying plugin it can simply pass this +through to the layer below: + + myfilter_extents (...) + { + return next_ops->extents (nxdata, count, offset, flags, + extents_map, err); + } + +It is also possible for filters to transform the extents map received +back from the layer below. Usually this must be done by allocating a +new, empty map which is pass...

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

..."%s: zero count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - f->name, count, offset, flags); + b->name, count, offset, flags); if (f->filter.zero) return f->filter.zero (&next_ops, &nxdata, handle, @@ -753,7 +727,7 @@ filter_extents (struct backend *b, struct connection *conn, assert (!(flags & ~NBDKIT_FLAG_REQ_ONE)); debug ("%s: extents count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, - f->name, count, offset, flags); + b->name, count, offset, flags); i...

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

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