[Libguestfs] [nbdkit PATCH] Introduce cacheextents filter

[Martin Kletzander]

On Wed, May 15, 2019 at 03:55:28PM +0100, Richard W.M. Jones wrote:
>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.
>

I was thinking about a test, that's right.  I'll see how to do one properly.

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

I got used to this pattern when we started introducing different errors with
different negative values and modifying all callers would be error-prone.  I'll
change this.  Should I change all the (r < 0) as well?

>> +      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.
>

Yes, you're right.

>> +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.
>

Funny that I actually missed this, it looks weird here to me as well.  Will fix
in v2.

Thanks for the review.

>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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190515/3e905541/attachment.sig>