search for: real_size_copy

...get_size (struct nbdkit_next_ops *next_ops, void *nxdata, size = ROUND_DOWN (size, round_down); ret = size; - pthread_mutex_unlock (&lock); - return ret; } @@ -176,11 +181,7 @@ truncate_pread (struct nbdkit_next_ops *next_ops, void *nxdata, { int r; uint32_t n; - uint64_t real_size_copy; - - pthread_mutex_lock (&lock); - real_size_copy = real_size; - pthread_mutex_unlock (&lock); + uint64_t real_size_copy = get_real_size (); if (offset < real_size_copy) { if (offset + count <= real_size_copy) @@ -209,11 +210,7 @@ truncate_pwrite (struct nbdkit_next_ops *...

...ace: thread A thread B .prepare - lock - next_ops->get_size returns A - set real_size based on A - unlock .pread .prepare - lock - next_ops->get_size returns B - set real_size based on B - unlock - lock - set real_size_copy based on B - unlock - decide whether to call next_ops->pread using wrong real_size If we are worried that next_ops->get_size() can change (even if our current documentation says it should not), then we must call it every time, rather than relying on an older cached version of the size; con...

While working on adding assertions to pthread_mutex_lock calls, I noticed that the truncate filter's use of mutex didn't really protect us, and isn't really necessary. Cleaning that up also spotted a couple of other potential cleanups. Eric Blake (4): filters: Drop useless .open callbacks truncate: Fix corruption when plugin changes per-connection size truncate: Test for safe

...utex_unlock (&lock); + + return ret; +} + +/* Read data. */ +static int +truncate_pread (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, void *buf, uint32_t count, uint64_t offset, + uint32_t flags, int *err) +{ + int r; + uint32_t n; + uint64_t real_size_copy; + + pthread_mutex_lock (&lock); + real_size_copy = real_size; + pthread_mutex_unlock (&lock); + + if (offset < real_size_copy) { + if (offset + count <= real_size_copy) + n = count; + else + n = real_size_copy - offset; + r = next_ops->pread (nxdata, buf, n,...

I do have a question about whether patch 2 is right, or whether I've exposed a bigger problem in the truncate (and possibly other) filter, but the rest seem fairly straightforward. Eric Blake (4): server: Check for pthread lock failures truncate: Factor out reading real_size under mutex plugins: Check for mutex failures filters: Check for mutex failures filters/cache/cache.c

...void *nxdata, return 0; } +/* Extents. */ +static int +truncate_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) +{ + uint32_t n; + uint64_t real_size_copy; + struct nbdkit_extents *extents2; + size_t i; + + pthread_mutex_lock (&lock); + real_size_copy = real_size; + pthread_mutex_unlock (&lock); + + /* If the entire request is beyond the end of the underlying plugin + * then this is the easy case: return a hole. + */ + if (offset &...

On 3/28/19 11:18 AM, Richard W.M. Jones wrote: > --- > filters/truncate/truncate.c | 55 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > + extents2 = nbdkit_extents_new (0, real_size_copy); > + if (offset + count <= real_size_copy) > + n = count; > + else > + n = real_size_copy - offset; > + if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) { > + nbdkit_extents_free (extents2); > + return -1; > + } > + > + for...

...-- > filters/truncate/truncate.c | 55 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > + > + /* If the entire request is beyond the end of the underlying plugin > + * then this is the easy case: return a hole. > + */ > + if (offset >= real_size_copy) > + return nbdkit_add_extent (extents, offset, (uint64_t) count, > + NBDKIT_EXTENT_ZERO|NBDKIT_EXTENT_HOLE); Why not return a hole up to the end of the file, rather than limiting things at the end of the client's request? > + > + /* We're aske...

...2019 at 09:39:23PM -0500, Eric Blake wrote: > On 3/28/19 11:18 AM, Richard W.M. Jones wrote: > > --- > > filters/truncate/truncate.c | 55 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > > > + extents2 = nbdkit_extents_new (0, real_size_copy); > > + if (offset + count <= real_size_copy) > > + n = count; > > + else > > + n = real_size_copy - offset; > > + if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) { > > + nbdkit_extents_free (extents2); > > + retu...

I have dropped the map filter from this series for now while I try to get it working. However I think the truncate filter is in a good shape. This incorporates all feedback from Eric's review. Also there are three small fixes to the filter code, all revealed when I was testing using multiple filters which we'd not done much of before. Rich.

...5 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > > > + > > + /* If the entire request is beyond the end of the underlying plugin > > + * then this is the easy case: return a hole. > > + */ > > + if (offset >= real_size_copy) > > + return nbdkit_add_extent (extents, offset, (uint64_t) count, > > + NBDKIT_EXTENT_ZERO|NBDKIT_EXTENT_HOLE); > > Why not return a hole up to the end of the file, rather than limiting > things at the end of the client's request? Yes, ve...

...rs/truncate/truncate.c index 076ae22..0c5dedb 100644 --- a/filters/truncate/truncate.c +++ b/filters/truncate/truncate.c @@ -319,6 +319,10 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, * returned data to the original array. */ extents2 = nbdkit_extents_new (offset, real_size_copy); + if (extents2 == NULL) { + *err = errno; + return -1; + } if (offset + count <= real_size_copy) n = count; else @@ -329,8 +333,10 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, for (i = 0; i < nbdkit_extents_count (extents2); ++i) { struct n...

...> filters/partition/Makefile.am | 5 ++++- > filters/truncate/Makefile.am | 5 ++++- > 6 files changed, 25 insertions(+), 27 deletions(-) > > @@ -322,20 +323,15 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, > n = count; > else > n = real_size_copy - offset; > - if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) { > - nbdkit_extents_free (extents2); > + if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) > return -1; > - } > > for (i = 0; i < nbdkit_extents_co...

...@ -43,6 +43,7 @@ #include <nbdkit-filter.h> +#include "cleanup.h" #include "ispowerof2.h" #include "iszero.h" #include "rounding.h" @@ -292,7 +293,7 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, { uint32_t n; uint64_t real_size_copy; - struct nbdkit_extents *extents2; + CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL; size_t i; pthread_mutex_lock (&lock); @@ -322,20 +323,15 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, n = count; else n = real_size_copy - offset; -...

This fix isn't exhaustive but it fixes some obvious problems in the filters. Rich.

I'm not sure exactly which version we're up to, but let's say it's version 4. I'm a lot happier with this version: - all filters have been reviewed and changed where I think that's necessary - can_extents is properly defined and implemented now - NBD protocol is followed - I believe it addresses all previous review points where possible The "only" thing

This has already been pushed upstream. I am simply posting these here so we have a reference in the mailing list in case we find bugs later (as I'm sure we will - it's a complex patch series). Great thanks to Eric Blake for tireless review on this one. It also seems to have identified a few minor bugs in qemu along the way. Rich.

There's more that can be done (in particular, use of CLEANUP_FREE), but this is enough to at least see if I'm on the right track. I couldn't figure out an obvious difference between common/include and common/utils, but it looks like the former is for things that are inlineable via .h only, while the latter is when you need to link in a convenience library, so this landed in the