search for: nbdkit_extents_map

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 -

...C<EOPNOTSUPP> (while plugins have automatic fallback to C<.pwrite>, filters do not). +=head2 C<.extents> + + int (*extents) (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t count, uint64_t offset, uint32_t flags, + struct nbdkit_extents_map *extents_map, + int *err); + +This intercepts the plugin C<.extents> method and can be used to +modify extent requests. + +This function will not be called if C<.can_extents> returned false; in +turn, the filter should not call C<next_ops-E<gt>extents> if +C&...

...C<EOPNOTSUPP> (while plugins have automatic fallback to C<.pwrite>, filters do not). +=head2 C<.extents> + + int (*extents) (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t count, uint64_t offset, uint32_t flags, + struct nbdkit_extents_map *extents_map, + int *err); + +This intercepts the plugin C<.extents> method and can be used to +modify extent requests. + +This function will not be called if C<.can_extents> returned false; in +turn, the filter should not call C<next_ops-E<gt>extents> if +C&...

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.

...C<EOPNOTSUPP> (while plugins have automatic fallback to C<.pwrite>, filters do not). +=head2 C<.extents> + + int (*extents) (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t count, uint64_t offset, uint32_t flags, + struct nbdkit_extents_map *extents_map, + int *err); + +This intercepts the plugin C<.extents> method and can be used to +modify extent requests. + +This function will not be called if C<.can_extents> returned false; in +turn, the filter should not call C<next_ops-E<gt>extents> if +C&...

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

...ssible 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 handling it > +would look like this: > + > + myfilter_extents (...) > + { > + struct nbdkit_extents_map *map2 = nbdkit_extents_new (); > + next_ops->extents (nxdata, count, offset, flags, map2, err); > + /* transform map2 and return results in extents_map */ > + nbdkit_extents_foreach (map2, transform_offset, extents_map); > + nbdkit_extents_free (map2); > + } And the fact...

...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 handling it > > +would look like this: > > + > > + myfilter_extents (...) > > + { > > + struct nbdkit_extents_map *map2 = nbdkit_extents_new (); > > + next_ops->extents (nxdata, count, offset, flags, map2, err); > > + /* transform map2 and return results in extents_map */ > > + nbdkit_extents_foreach (map2, transform_offset, extents_map); > > + nbdkit_extents_free (map2); &gt...

...> @@ -132,6 +133,47 @@ offset_zero (struct nbdkit_next_ops *next_ops, void *nxdata, return next_ops->zero (nxdata, count, offs + offset, flags, err); } +/* Extents. */ +static int +subtract_offset (uint64_t offs, uint64_t length, uint32_t type, + void *vp) +{ + struct nbdkit_extents_map *extents_map = vp; + + assert (offs >= offset); + offs -= offset; + return nbdkit_extent_add (extents_map, offs, length, type); +} + +static int +offset_extents (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t count, uint64_t offs, uint32_t flags, +...

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 > + > +One function is provided to filters only to iterate over the extents > +map: > + > + typedef int (*nbdkit_extents_callb...

...rror to send back to the client (0 @@ -173,7 +205,7 @@ validate_request (struct connection *conn, static uint32_t handle_request (struct connection *conn, uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, - void *buf) + void *buf, struct nbdkit_extents_map *extents_map) { uint32_t f = 0; bool fua = conn->can_fua && (flags & NBD_CMD_FLAG_FUA); @@ -217,6 +249,14 @@ handle_request (struct connection *conn, return err; break; + case NBD_CMD_BLOCK_STATUS: + if (flags & NBD_CMD_FLAG_REQ_ONE) + f |= NBDKIT_FL...

...2019 at 12:35:33AM -0500, Eric Blake wrote: > 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? Yes - otherwise CLEANUP_EXTENTS_FREE wouldn't work. Do we need to document this? > > +=item C<NBDKIT_EXTENTS_FOREACH_FLAG_RANGE> > > + > > +If th...

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.

Provides automatic cleanup of ‘struct nbdkit_extents_map *’ on exit from a scope or function. --- server/internal.h | 2 ++ server/cleanup.c | 8 +++++++- server/Makefile.am | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/server/internal.h b/server/internal.h index d0da213..825dd3e 100644 --- a/server/internal.h +++ b/server/...

...ords, the full flexibility of random-access extent probing rather than linear is over-engineered). I like the idea. I'm trying to figure out if filters would still need a foreach visitor. But my initial guess is no. Consider - the earlier proposal required the offset filter to do: + struct nbdkit_extents_map *map2; + + map2 = nbdkit_extents_new (); + if (map2 == NULL) + return -1; + if (next_ops->extents (nxdata, count, offs + offset, + flags, map2, err) == -1) { + nbdkit_extents_free (map2); + return -1; + } + + /* Transform offsets in map2, return result in ex...

...xtent > probing rather than linear is over-engineered). > > I like the idea. > > I'm trying to figure out if filters would still need a foreach visitor. > But my initial guess is no. Consider - the earlier proposal required > the offset filter to do: > > + struct nbdkit_extents_map *map2; > + > + map2 = nbdkit_extents_new (); > + if (map2 == NULL) > + return -1; > + if (next_ops->extents (nxdata, count, offs + offset, > + flags, map2, err) == -1) { > + nbdkit_extents_free (map2); > + return -1; > + } > + &g...

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

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.