Re: [Libguestfs] [nbdkit PATCH] Introduce cacheextents filter

On Wed, May 15, 2019 at 02:54:17PM +0200, Martin Kletzander wrote: > This filter caches the last result of the extents() call and offers a nice > speed-up for clients that only support req_on=1 in combination with plugins like > vddk, which has no overhead for returning information for multiple extents in > one call, but that call is very time-consuming. > > Quick test showed that on a fast connection and a sparsely allocated 16G disk > with a OS installed `qemu-img map` runs 16s instead of 33s (out of which it > takes 5s to the first extents request). For 100G disk with no data on it, that > is one hole extent spanning the whole disk (where there is no space for > improvement) this does not add any noticeable overhead. > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > Yes, I hate the filter name, so if anyone has a better idea, feel free to > suggest (or rename) it. Basically it's fine, however it could really do with having a test. I have no particular comment about the name.

A few more things inline below. > +static int > +cacheextents_add (struct nbdkit_extents *extents) > +{ > + size_t i = 0; > + > + for (i = 0; i < nbdkit_extents_count (cache_extents); i++) { > + struct nbdkit_extent ex = nbdkit_get_extent (cache_extents, i); > + if (nbdkit_add_extent (extents, ex.offset, ex.length, ex.type) < 0) This really only returns -1 on failure, and not any other negative values.

> + return -1; > + } > + > + return 0; > +} > + > +static int > +cacheextents_fill (struct nbdkit_extents *extents) > +{ > + size_t i = 0; > + size_t count = nbdkit_extents_count (extents); > + struct nbdkit_extent first = nbdkit_get_extent (extents, 0); > + struct nbdkit_extent last = nbdkit_get_extent (extents, count - 1); > + > + nbdkit_extents_free (cache_extents); > + cache_start = first.offset; > + cache_end = last.offset + last.length; > + cache_extents = nbdkit_extents_new (cache_start, cache_end); > + > + for (i = 0; i < count; i++) { > + struct nbdkit_extent ex = nbdkit_get_extent (extents, i); > + nbdkit_debug ("cacheextents: updating cache with" > + ": offset=%" PRIu64 > + ": length=%" PRIu64 > + "; type=%x", > + ex.offset, ex.length, ex.type); > + if (nbdkit_add_extent (cache_extents, ex.offset, ex.length, ex.type) < 0) .. and the same here. > + return -1; > + } > + > + return 0; > +} > + > +static int > +cacheextents_extents (struct nbdkit_next_ops *next_ops, void *nxdata, > + void *handle, uint32_t count, uint64_t offset, uint32_t flags, > + struct nbdkit_extents *extents, > + int *err) > +{ > + nbdkit_debug ("cacheextents:" > + " cache_start=%" PRIu64 > + " cache_end=%" PRIu64 > + " cache_extents=%p", > + cache_start, cache_end, cache_extents); > + > + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); Shouldn't the debug statement be protected by the lock too? Even reading global state can be racy.

> +static struct nbdkit_filter filter = { > + .name = "cacheextents", > + .longname = "nbdkit cacheextents filter", > + .version = PACKAGE_VERSION, > + .unload = cacheextents_unload, > + .pwrite = cacheextents_pwrite, > + .trim = cacheextents_trim, > + .zero = cacheextents_zero, > + .extents = cacheextents_extents, > +}; I know that for some reason emacs C mode has started to indent struct fields like this, which is very annoying, but these would be better indented in the same way as the other plugins and filters.

Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org