Libguestfs - Mar 2019 - Re: [PATCH nbdkit] server: Implement extents/can_extents calls for plugins and filters.

On Tue, Mar 12, 2019 at 11:20:25AM +0000, Richard W.M. Jones
wrote:
>This pair of calls allows plugins to describe which extents in the
>virtual disk are allocated, holes or zeroes.
>---
> docs/nbdkit-filter.pod  | 22 +++++++++++
> docs/nbdkit-plugin.pod  | 86 +++++++++++++++++++++++++++++++++++++++++
> include/nbdkit-common.h | 14 ++++++-
> include/nbdkit-filter.h | 12 +++++-
> include/nbdkit-plugin.h |  6 ++-
> server/internal.h       |  7 +++-
> server/filters.c        | 61 +++++++++++++++++++++++++++++
> server/plugins.c        | 72 +++++++++++++++++++++++++++++++++-
> 8 files changed, 275 insertions(+), 5 deletions(-)
>
[...]
>diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
>index 47545f3..9cc62b6 100644
>--- a/docs/nbdkit-plugin.pod
>+++ b/docs/nbdkit-plugin.pod
[...]
>@@ -717,6 +731,78 @@ If there is an error, C<.zero> should call
C<nbdkit_error> with an
> error message, and C<nbdkit_set_error> to record an appropriate error
> (unless C<errno> is sufficient), then return C<-1>.
>
>+=head2 C<.extents>
>+
>+ int zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags,
s/zero/extents/

[...]
>diff --git a/server/filters.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,
backend);
>+  void *handle = connection_get_handle (conn, f->backend.i);
>+  struct b_conn nxdata = { .b = f->backend.next, .conn = conn };
>+
>+  assert (!(flags & ~NBDKIT_FLAG_REQ_ONE));
>+
>+  debug ("%s: extents count=%" PRIu32 " offset=%"
PRIu64 " flags=0x%" PRIx32,
>+         f->name, count, offset, flags);
>+
>+  if (f->filter.extents)
>+    return f->filter.extents (&next_ops, &nxdata, handle,
>+                              count, offset, flags,
>+                              nr_extents, extents, err);
>+  else
>+    return f->backend.next->extents (f->backend.next, conn,
>+                                     count, offset, flags,
>+                                     nr_extents, extents, err);
Do I understand it correctly that if a filter does not support extents, then its
function will only be applied on the allocated blocks?  It makes sense, but I
feel like it would be nice to have that mentioned somewhere.

Other than that it looks OK to me, I'll try to cook up a test filter to test
this, but mainly as a fun exercise for myself.

Have a nice day,
Martin

On Tue, Mar 12, 2019 at 01:23:37PM +0100, Martin Kletzander
wrote:
> >+=head2 C<.extents>
> >+
> >+ int zero (void *handle, uint32_t count, uint64_t offset, uint32_t
flags,
> 
> s/zero/extents/
Oops.
> >+  if (f->filter.extents)
> >+    return f->filter.extents (&next_ops, &nxdata, handle,
> >+                              count, offset, flags,
> >+                              nr_extents, extents, err);
> >+  else
> >+    return f->backend.next->extents (f->backend.next, conn,
> >+                                     count, offset, flags,
> >+                                     nr_extents, extents, err);
>
> Do I understand it correctly that if a filter does not support
> extents, then its function will only be applied on the allocated
> blocks?  It makes sense, but I feel like it would be nice to have
> that mentioned somewhere.
No.  The code snippet above allows filters to intercept the extents
call and modify it (either on the way in, or the way out, or most
likely in this case both).  The conditional ‘if (f->filter.extents)’
is checking if the filter has an extents callback, which is the
callback that the filter has to provide to do this interception.

I think most filters will need to be adjusted by adding an extents
callback.  Certainly any filter which deals with offsets (most
obviously nbdkit-offset-filter, nbdkit-partition-filter) will need to
implement the .extents call and both adjust the offset parameter that
is passed down, and examine the list of extents being returned through
the filter and adjust the offsets inside that list.

This is the broad reason why we don't offer a stable API for filters
(unlike plugins).  Nor do we advise that it's a good idea to
distribute filters separately from nbdkit.  If an old filter was used
with a plugin which was using the extents API then the filter would
basically corrupt data.
> Other than that it looks OK to me, I'll try to cook up a test filter
> to test this, but mainly as a fun exercise for myself.
So note at the moment this patch doesn't really do anything.  nbdkit
server still doesn't support block status, so neither of the functions
will ever be called.  This patch was just for discussion about what
the plugin API might look like.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Tue, Mar 12, 2019 at 12:41:44PM +0000, Richard W.M. Jones
wrote:
>On Tue, Mar 12, 2019 at 01:23:37PM +0100, Martin Kletzander wrote:
>> >+=head2 C<.extents>
>> >+
>> >+ int zero (void *handle, uint32_t count, uint64_t offset, uint32_t
flags,
>>
>> s/zero/extents/
>
>Oops.
>
>> >+  if (f->filter.extents)
>> >+    return f->filter.extents (&next_ops, &nxdata,
handle,
>> >+                              count, offset, flags,
>> >+                              nr_extents, extents, err);
>> >+  else
>> >+    return f->backend.next->extents (f->backend.next,
conn,
>> >+                                     count, offset, flags,
>> >+                                     nr_extents, extents, err);
>>
>> Do I understand it correctly that if a filter does not support
>> extents, then its function will only be applied on the allocated
>> blocks?  It makes sense, but I feel like it would be nice to have
>> that mentioned somewhere.
>
>No.  The code snippet above allows filters to intercept the extents
>call and modify it (either on the way in, or the way out, or most
>likely in this case both).  The conditional ‘if (f->filter.extents)’
>is checking if the filter has an extents callback, which is the
>callback that the filter has to provide to do this interception.
>
>I think most filters will need to be adjusted by adding an extents
>callback.  Certainly any filter which deals with offsets (most
>obviously nbdkit-offset-filter, nbdkit-partition-filter) will need to
>implement the .extents call and both adjust the offset parameter that
>is passed down, and examine the list of extents being returned through
>the filter and adjust the offsets inside that list.
>
>This is the broad reason why we don't offer a stable API for filters
>(unlike plugins).  Nor do we advise that it's a good idea to
>distribute filters separately from nbdkit.  If an old filter was used
>with a plugin which was using the extents API then the filter would
>basically corrupt data.
>
Oh, ok, makes sense.
>> Other than that it looks OK to me, I'll try to cook up a test
filter
>> to test this, but mainly as a fun exercise for myself.
>
>So note at the moment this patch doesn't really do anything.  nbdkit
>server still doesn't support block status, so neither of the functions
>will ever be called.  This patch was just for discussion about what
>the plugin API might look like.
>
I'll have it prepared when the time comes then.
>Rich.
>
>-- 
>Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
>Read my programming and virtualization blog: http://rwmj.wordpress.com
>virt-df lists disk usage of guests without needing to install any
>software inside the virtual machine.  Supports Linux and Windows.
>http://people.redhat.com/~rjones/virt-df/