search for: filter_free

...bc. This in fact reveals a race during filter shutdown (which this commit does not fix): Thread 2 (Thread 0x7f1caaa5ffc0 (LWP 7223)): #0 0x00007f1cab0a05f8 in pthread_rwlock_wrlock () from /lib64/libpthread.so.0 #1 0x0000000000408842 in lock_unload () at locks.c:97 #2 0x00000000004066ff in filter_free (b=0x203c330) at filters.c:77 #3 0x000000000040a6f4 in main (argc=11, argv=0x7ffc1f4486e8) at main.c:649 Thread 1 (Thread 0x7f1caaa5e700 (LWP 7226)): #0 0x000000000040732a in filter_finalize (b=0x203c330, conn=0x203d870) at filters.c:421 #1 0x0000000000404d07 in _handle_single_connecti...

...ce during filter shutdown (which this > commit does not fix): > > Thread 2 (Thread 0x7f1caaa5ffc0 (LWP 7223)): > #0 0x00007f1cab0a05f8 in pthread_rwlock_wrlock () from /lib64/libpthread.so.0 > #1 0x0000000000408842 in lock_unload () at locks.c:97 > #2 0x00000000004066ff in filter_free (b=0x203c330) at filters.c:77 Do we need some sort of pthread_join() in filter_free() to allow... > #3 0x000000000040a6f4 in main (argc=11, argv=0x7ffc1f4486e8) at main.c:649 > > Thread 1 (Thread 0x7f1caaa5e700 (LWP 7226)): > #0 0x000000000040732a in filter_finalize (b=0x203c330...

...LAGS = \ nbdkit_CFLAGS = \ -pthread \ $(WARNINGS_CFLAGS) \ - $(GNUTLS_CFLAGS) + $(GNUTLS_CFLAGS) \ + $(VALGRIND_CFLAGS) nbdkit_LDADD = \ $(GNUTLS_LIBS) \ -ldl diff --git a/src/filters.c b/src/filters.c index 3d2c07e..18948bc 100644 --- a/src/filters.c +++ b/src/filters.c @@ -80,7 +80,8 @@ filter_free (struct backend *b) if (f->filter.unload) f->filter.unload (); - dlclose (f->dl); + if (DO_DLCLOSE) + dlclose (f->dl); free (f->filename); unlock_unload (); diff --git a/src/internal.h b/src/internal.h index ec19841..96a68f2 100644 --- a/src/internal.h +++ b/sr...

...1]: debug: can_flush # plugin --- src/filters.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/filters.c b/src/filters.c index b68afab..d0692e1 100644 --- a/src/filters.c +++ b/src/filters.c @@ -76,7 +76,7 @@ filter_free (struct backend *b) */ lock_unload (); - debug ("%s: unload", f->filename); + debug ("%s: unload", f->name); if (f->filter.unload) f->filter.unload (); @@ -172,7 +172,7 @@ filter_config (struct backend *b, const char *key, const char *value) s...

...index 287c8747..1f76bf61 100644 --- a/server/filters.c +++ b/server/filters.c @@ -48,9 +48,6 @@ */ struct backend_filter { struct backend backend; - char *name; /* copy of filter.name */ - char *filename; - void *dl; struct nbdkit_filter filter; }; @@ -70,22 +67,7 @@ filter_free (struct backend *b) b->next->free (b->next); - /* Acquiring this lock prevents any filter callbacks from running - * simultaneously. - */ - lock_unload (); - - debug ("%s: unload", f->name); - if (f->filter.unload) - f->filter.unload (); - - if (DO_DLCLO...

...clude "internal.h" + +/* We extend the generic backend struct with extra fields relating + * to this filter. + */ +struct backend_filter { + struct backend backend; + char *filename; + void *dl; + struct nbdkit_filter filter; +}; + +/* Note this frees the whole chain. */ +static void +filter_free (struct backend *b) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + + f->backend.next->free (f->backend.next); + + /* Acquiring this lock prevents any filter callbacks from running + * simultaneously. + */ + lock_unload (); + + debug ("%s:...

...x%" PRIx32, + f->name, count, offset, flags); + + /* FIXME: Allow filter to rewrite request */ + return f->backend.next->cache (f->backend.next, conn, + count, offset, flags, err); +} + static struct backend filter_functions = { .free = filter_free, .thread_model = filter_thread_model, @@ -726,12 +755,14 @@ static struct backend filter_functions = { .can_extents = filter_can_extents, .can_fua = filter_can_fua, .can_multi_conn = filter_can_multi_conn, + .can_cache = filter_can_cache, .pread = filter_pread, .pwrite = filter_p...

...nr_extents, extents, err); + else + return f->backend.next->extents (f->backend.next, conn, + count, offset, flags, + nr_extents, extents, err); +} + static struct backend filter_functions = { .free = filter_free, .thread_model = filter_thread_model, @@ -667,6 +726,7 @@ static struct backend filter_functions = { .is_rotational = filter_is_rotational, .can_trim = filter_can_trim, .can_zero = filter_can_zero, + .can_extents = filter_can_extents, .can_fua = filter_can_fua, .can_multi_conn =...

...r; }; -/* Literally a backend and the filter's handle. - * - * This is the implementation of our handle in .open, and serves as - * a stable ‘void *nxdata’ in the filter API. - */ -struct b_h { - struct backend *b; - void *handle; -}; - /* Note this frees the whole chain. */ static void filter_free (struct backend *b) @@ -186,20 +176,19 @@ filter_config_complete (struct backend *b) static int next_preconnect (void *nxdata, int readonly) { - struct b_h *b_h = nxdata; - return b_h->b->preconnect (b_h->b, readonly); + struct backend *b_next = nxdata; + return b_next->preconnec...

...<eblake@redhat.com> --- server/filters.c | 170 ++++++++++++++++++++++------------------------- 1 file changed, 79 insertions(+), 91 deletions(-) diff --git a/server/filters.c b/server/filters.c index bb6394fb..287c8747 100644 --- a/server/filters.c +++ b/server/filters.c @@ -68,7 +68,7 @@ filter_free (struct backend *b) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - f->backend.next->free (f->backend.next); + b->next->free (b->next); /* Acquiring this lock prevents any filter callbacks from running * simultaneously. @@ -94,7 +94,...

...extents_map, err); + else + return f->backend.next->extents (f->backend.next, conn, + count, offset, flags, + extents_map, err); +} + static struct backend filter_functions = { .free = filter_free, .thread_model = filter_thread_model, @@ -667,6 +726,7 @@ static struct backend filter_functions = { .is_rotational = filter_is_rotational, .can_trim = filter_can_trim, .can_zero = filter_can_zero, + .can_extents = filter_can_extents, .can_fua = filter_can_fua, .can_multi_conn =...

...extents_map, err); + else + return f->backend.next->extents (f->backend.next, conn, + count, offset, flags, + extents_map, err); +} + static struct backend filter_functions = { .free = filter_free, .thread_model = filter_thread_model, @@ -667,6 +726,7 @@ static struct backend filter_functions = { .is_rotational = filter_is_rotational, .can_trim = filter_can_trim, .can_zero = filter_can_zero, + .can_extents = filter_can_extents, .can_fua = filter_can_fua, .can_multi_conn =...

...s, + extents, err); + else + return f->backend.next->extents (f->backend.next, conn, + count, offset, flags, + extents, err); +} + static struct backend filter_functions = { .free = filter_free, .thread_model = filter_thread_model, @@ -667,6 +724,7 @@ static struct backend filter_functions = { .is_rotational = filter_is_rotational, .can_trim = filter_can_trim, .can_zero = filter_can_zero, + .can_extents = filter_can_extents, .can_fua = filter_can_fua, .can_multi_conn =...

This tentative commit implements extents/can_extents, roughly as discussed in the previous thread here: https://www.redhat.com/archives/libguestfs/2019-March/msg00017.html I can't say that I'm a big fan of having the plugin allocate an extents array. There are no other plugin callbacks currently where we require the plugin to allocate complex data structures (or indeed do any allocation

...s, + extents, err); + else + return f->backend.next->extents (f->backend.next, conn, + count, offset, flags, + extents, err); +} + static struct backend filter_functions = { .free = filter_free, .thread_model = filter_thread_model, @@ -667,6 +724,7 @@ static struct backend filter_functions = { .is_rotational = filter_is_rotational, .can_trim = filter_can_trim, .can_zero = filter_can_zero, + .can_extents = filter_can_extents, .can_fua = filter_can_fua, .can_multi_conn =...

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 -

...s, + extents, err); + else + return f->backend.next->extents (f->backend.next, conn, + count, offset, flags, + extents, err); +} + static struct backend filter_functions = { .free = filter_free, .thread_model = filter_thread_model, @@ -667,6 +724,7 @@ static struct backend filter_functions = { .is_rotational = filter_is_rotational, .can_trim = filter_can_trim, .can_zero = filter_can_zero, + .can_extents = filter_can_extents, .can_fua = filter_can_fua, .can_multi_conn =...

...clude "internal.h" + +/* We extend the generic backend struct with extra fields relating + * to this filter. + */ +struct backend_filter { + struct backend backend; + char *filename; + void *dl; + struct nbdkit_filter filter; +}; + +/* Note this frees the whole chain. */ +static void +filter_free (struct backend *b) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + + f->backend.next->free (f->backend.next); + + /* Acquiring this lock prevents any filter callbacks from running + * simultaneously. + */ + lock_unload (); + + debug ("%s:...

...+ void *dl; + struct nbdkit_filter filter; +}; + +/* Literally a backend + a connection pointer. This is the + * implementation of ‘void *nxdata’ in the filter API. + */ +struct b_conn { + struct backend *b; + struct connection *conn; +}; + +/* Note this frees the whole chain. */ +static void +filter_free (struct backend *b) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + + f->backend.next->free (f->backend.next); + + /* Acquiring this lock prevents any filter callbacks from running + * simultaneously. + */ + lock_unload (); + + debug ("%s:...

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.