[Libguestfs] [nbdkit PATCH] Introduce cacheextents filter
Richard W.M. Jones
rjones at redhat.com
Wed May 15 14:55:28 UTC 2019
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 at redhat.com>
> ---
> 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
More information about the Libguestfs
mailing list