Libguestfs - Apr 2019 - Re: [nbdkit PATCH 2/4] truncate: Factor out reading real_size under mutex

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         | 23 +++++++++----------
 filters/cow/cow.c             | 19 +++++++---------
 filters/error/error.c         |  7 +++---
 filters/log/log.c             |  3 +--
 filters/rate/rate.c           | 10 ++++-----
 filters/readahead/readahead.c |  3 +--
 filters/stats/stats.c         | 18 +++++----------
 filters/truncate/truncate.c   | 41 +++++++++++-----------------------
 plugins/file/file.c           |  3 +--
 server/connections.c          | 20 ++++++++++-------
 server/locks.c                | 42 ++++++++++++++++++++++-------------
 filters/error/Makefile.am     |  5 ++++-
 12 files changed, 92 insertions(+), 102 deletions(-)

-- 
2.20.1

Low-level pthread locks should not fail except in extreme cases of
programmer bugs; we're better off calling attention to such bugs
rather than just assuming that they work and continuing on with
possibly inconsistent state.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/connections.c | 20 ++++++++++++--------
 server/locks.c       | 42 ++++++++++++++++++++++++++----------------
 2 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/server/connections.c b/server/connections.c
index a30a541..b7d9a6a 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -91,11 +91,13 @@ connection_get_status (struct connection *conn)
 {
   int r;

-  if (conn->nworkers)
-    pthread_mutex_lock (&conn->status_lock);
+  if (conn->nworkers &&
+      pthread_mutex_lock (&conn->status_lock))
+    abort ();
   r = conn->status;
-  if (conn->nworkers)
-    pthread_mutex_unlock (&conn->status_lock);
+  if (conn->nworkers &&
+      pthread_mutex_unlock (&conn->status_lock))
+    abort ();
   return r;
 }

@@ -105,12 +107,14 @@ connection_get_status (struct connection *conn)
 int
 connection_set_status (struct connection *conn, int value)
 {
-  if (conn->nworkers)
-    pthread_mutex_lock (&conn->status_lock);
+  if (conn->nworkers &&
+      pthread_mutex_lock (&conn->status_lock))
+    abort ();
   if (value < conn->status)
     conn->status = value;
-  if (conn->nworkers)
-    pthread_mutex_unlock (&conn->status_lock);
+  if (conn->nworkers &&
+      pthread_mutex_unlock (&conn->status_lock))
+    abort ();
   return value;
 }

diff --git a/server/locks.c b/server/locks.c
index f4d6497..d70baf2 100644
--- a/server/locks.c
+++ b/server/locks.c
@@ -55,49 +55,59 @@ lock_init_thread_model (void)
 void
 lock_connection (void)
 {
-  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS)
-    pthread_mutex_lock (&connection_lock);
+  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS &&
+      pthread_mutex_lock (&connection_lock))
+    abort ();
 }

 void
 unlock_connection (void)
 {
-  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS)
-    pthread_mutex_unlock (&connection_lock);
+  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS &&
+      pthread_mutex_unlock (&connection_lock))
+    abort ();
 }

 void
 lock_request (struct connection *conn)
 {
-  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS)
-    pthread_mutex_lock (&all_requests_lock);
+  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS &&
+      pthread_mutex_lock (&all_requests_lock))
+    abort ();

-  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS)
-    pthread_mutex_lock (&conn->request_lock);
+  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS &&
+      pthread_mutex_lock (&conn->request_lock))
+    abort ();

-  pthread_rwlock_rdlock (&unload_prevention_lock);
+  if (pthread_rwlock_rdlock (&unload_prevention_lock))
+    abort ();
 }

 void
 unlock_request (struct connection *conn)
 {
-  pthread_rwlock_unlock (&unload_prevention_lock);
+  if (pthread_rwlock_unlock (&unload_prevention_lock))
+    abort ();

-  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS)
-    pthread_mutex_unlock (&conn->request_lock);
+  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS &&
+      pthread_mutex_unlock (&conn->request_lock))
+    abort ();

-  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS)
-    pthread_mutex_unlock (&all_requests_lock);
+  if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS &&
+      pthread_mutex_unlock (&all_requests_lock))
+    abort ();
 }

 void
 lock_unload (void)
 {
-  pthread_rwlock_wrlock (&unload_prevention_lock);
+  if (pthread_rwlock_wrlock (&unload_prevention_lock))
+    abort ();
 }

 void
 unlock_unload (void)
 {
-  pthread_rwlock_unlock (&unload_prevention_lock);
+  if (pthread_rwlock_unlock (&unload_prevention_lock))
+    abort ();
 }
-- 
2.20.1

Add a helper function get_real_size() to make it easier to add sanity
checking that mutex calls don't fail.

Signed-off-by: Eric Blake <eblake@redhat.com>

---

I'm a bit unsure why truncate.c needs a lock anyway. I guess it's
because we are storing 'size' and 'real_size' as globals rather
than
per-connection values in the handle, and we're worrying about parallel
connections where we don't want one thread reading inconsistent values
while another thread is in the middle of truncate_get_size()? At any
rate, as long as we don't have dynamic resize in the NBD protocol,
size can't change within the confines of a single connection. And even
if we DO assume that the underlying plugin reports different sizes for
different connections, our use of a global does not play well (if
client A sees size 1k, then client B sees size 2k, that changes client
A's use of 'real_size' to be something different than client A was
expecting).

So, I think we have to move 'size' and 'real_size' into the
per-connection handle, at which point, can't we just set them once
during truncate_prepare by moving the existing guts of
truncate_get_size there, and then letting truncate_get_size just
return h->size while all other truncate_* functions just access
h->real_size without a lock?

I guess I should look to see what other global filter state is better
tracked per-filter rather than globally?
---
 filters/truncate/truncate.c | 41 ++++++++++++-------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c
index 5f3370d..38745da 100644
--- a/filters/truncate/truncate.c
+++ b/filters/truncate/truncate.c
@@ -63,6 +63,13 @@ static uint64_t size;
 /* This lock protects the real_size and size fields. */
 static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;

+static uint64_t
+get_real_size (void)
+{
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+  return real_size;
+}
+
 static int
 parse_round_param (const char *key, const char *value, unsigned *ret)
 {
@@ -147,7 +154,7 @@ truncate_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata,
   if (r == -1)
     return -1;

-  pthread_mutex_lock (&lock);
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);

   real_size = size = r;

@@ -163,8 +170,6 @@ truncate_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 *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)
@@ -246,11 +243,7 @@ truncate_trim (struct nbdkit_next_ops *next_ops, void
*nxdata,
                uint32_t flags, int *err)
 {
   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)
@@ -269,11 +262,7 @@ truncate_zero (struct nbdkit_next_ops *next_ops, void
*nxdata,
                uint32_t flags, int *err)
 {
   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)
@@ -292,14 +281,10 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
                   uint32_t flags, struct nbdkit_extents *extents, int *err)
 {
   uint32_t n;
-  uint64_t real_size_copy;
+  uint64_t real_size_copy = get_real_size ();
   CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL;
   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 up to the end of the
    * file.
-- 
2.20.1

Commit 9ceb4712 argued that for simple lock/unlock sequences, it was
easier to avoid the cleanup.h macros. But since that time, we added
additional sanity checking to the macros, at which point the
boilerplate of inlining that sanity checking is outweighed compared to
just using the macro in the last remaining bare use.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/file/file.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/plugins/file/file.c b/plugins/file/file.c
index ebfb71d..d2bab9c 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -529,9 +529,8 @@ file_can_extents (void *handle)
   /* A simple test to see whether SEEK_HOLE etc is likely to work on
    * the current filesystem.
    */
-  pthread_mutex_lock (&lseek_lock);
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock);
   r = lseek (h->fd, 0, SEEK_HOLE);
-  pthread_mutex_unlock (&lseek_lock);
   if (r == -1) {
     nbdkit_debug ("extents disabled: lseek: SEEK_HOLE: %m");
     return 0;
-- 
2.20.1

Commit 975dab14 argued that for simple lock/unlock sequences, it was
easier to avoid the cleanup.h macros. But since that time, we added
additional sanity checking to the macros, at which point the
boilerplate of inlining that sanity checking is outweighed compared to
just using the macros in more places.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 filters/cache/cache.c         | 23 +++++++++++------------
 filters/cow/cow.c             | 19 ++++++++-----------
 filters/error/error.c         |  7 ++++---
 filters/log/log.c             |  3 +--
 filters/rate/rate.c           | 10 +++++-----
 filters/readahead/readahead.c |  3 +--
 filters/stats/stats.c         | 18 ++++++------------
 filters/error/Makefile.am     |  5 ++++-
 8 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/filters/cache/cache.c b/filters/cache/cache.c
index 6a9966e..b3fef42 100644
--- a/filters/cache/cache.c
+++ b/filters/cache/cache.c
@@ -209,9 +209,8 @@ cache_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata,

   nbdkit_debug ("cache: underlying file size: %" PRIi64, size);

-  pthread_mutex_lock (&lock);
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
   r = blk_set_size (size);
-  pthread_mutex_unlock (&lock);
   if (r == -1)
     return -1;

@@ -266,9 +265,10 @@ cache_pread (struct nbdkit_next_ops *next_ops, void
*nxdata,
     if (n > count)
       n = count;

-    pthread_mutex_lock (&lock);
-    r = blk_read (next_ops, nxdata, blknum, block, err);
-    pthread_mutex_unlock (&lock);
+    {
+      ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+      r = blk_read (next_ops, nxdata, blknum, block, err);
+    }
     if (r == -1)
       return -1;

@@ -316,13 +316,12 @@ cache_pwrite (struct nbdkit_next_ops *next_ops, void
*nxdata,
     /* Do a read-modify-write operation on the current block.
      * Hold the lock over the whole operation.
      */
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     r = blk_read (next_ops, nxdata, blknum, block, err);
     if (r != -1) {
       memcpy (&block[blkoffs], buf, n);
       r = blk_write (next_ops, nxdata, blknum, block, flags, err);
     }
-    pthread_mutex_unlock (&lock);
     if (r == -1)
       return -1;

@@ -371,13 +370,12 @@ cache_zero (struct nbdkit_next_ops *next_ops, void
*nxdata,
     /* Do a read-modify-write operation on the current block.
      * Hold the lock over the whole operation.
      */
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     r = blk_read (next_ops, nxdata, blknum, block, err);
     if (r != -1) {
       memset (&block[blkoffs], 0, n);
       r = blk_write (next_ops, nxdata, blknum, block, flags, err);
     }
-    pthread_mutex_unlock (&lock);
     if (r == -1)
       return -1;

@@ -429,9 +427,10 @@ cache_flush (struct nbdkit_next_ops *next_ops, void
*nxdata, void *handle,
    * to be sure.  Also we still need to issue the flush to the
    * underlying storage.
    */
-  pthread_mutex_lock (&lock);
-  for_each_dirty_block (flush_dirty_block, &data);
-  pthread_mutex_unlock (&lock);
+  {
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+    for_each_dirty_block (flush_dirty_block, &data);
+  }

   /* Now issue a flush request to the underlying storage. */
   if (next_ops->flush (nxdata, 0,
diff --git a/filters/cow/cow.c b/filters/cow/cow.c
index 04ec44f..35ad718 100644
--- a/filters/cow/cow.c
+++ b/filters/cow/cow.c
@@ -92,9 +92,8 @@ cow_get_size (struct nbdkit_next_ops *next_ops, void *nxdata,

   nbdkit_debug ("cow: underlying file size: %" PRIi64, size);

-  pthread_mutex_lock (&lock);
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
   r = blk_set_size (size);
-  pthread_mutex_unlock (&lock);
   if (r == -1)
     return -1;

@@ -174,9 +173,10 @@ cow_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
     if (n > count)
       n = count;

-    pthread_mutex_lock (&lock);
-    r = blk_read (next_ops, nxdata, blknum, block, err);
-    pthread_mutex_unlock (&lock);
+    {
+      ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+      r = blk_read (next_ops, nxdata, blknum, block, err);
+    }
     if (r == -1)
       return -1;

@@ -218,13 +218,12 @@ cow_pwrite (struct nbdkit_next_ops *next_ops, void
*nxdata,
     /* Do a read-modify-write operation on the current block.
      * Hold the lock over the whole operation.
      */
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     r = blk_read (next_ops, nxdata, blknum, block, err);
     if (r != -1) {
       memcpy (&block[blkoffs], buf, n);
       r = blk_write (blknum, block, err);
     }
-    pthread_mutex_unlock (&lock);
     if (r == -1)
       return -1;

@@ -269,13 +268,12 @@ cow_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
      * XXX There is the possibility of optimizing this: ONLY if we are
      * writing a whole, aligned block, then use FALLOC_FL_ZERO_RANGE.
      */
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     r = blk_read (next_ops, nxdata, blknum, block, err);
     if (r != -1) {
       memset (&block[blkoffs], 0, n);
       r = blk_write (blknum, block, err);
     }
-    pthread_mutex_unlock (&lock);
     if (r == -1)
       return -1;

@@ -294,11 +292,10 @@ cow_flush (struct nbdkit_next_ops *next_ops, void *nxdata,
void *handle,
 {
   int r;

-  pthread_mutex_lock (&lock);
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
   r = blk_flush ();
   if (r == -1)
     *err = errno;
-  pthread_mutex_unlock (&lock);
   return r;
 }

diff --git a/filters/error/error.c b/filters/error/error.c
index 0f84f12..22ebd1c 100644
--- a/filters/error/error.c
+++ b/filters/error/error.c
@@ -283,9 +283,10 @@ random_error (const struct error_settings *error_settings,
    * representable in a 64 bit integer, and because we don't need all
    * this precision anyway, let's work in 32 bits.
    */
-  pthread_mutex_lock (&lock);
-  rand = xrandom (&random_state) & UINT32_MAX;
-  pthread_mutex_unlock (&lock);
+  {
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+    rand = xrandom (&random_state) & UINT32_MAX;
+  }
   if (rand >= error_settings->rate * UINT32_MAX)
     return false;

diff --git a/filters/log/log.c b/filters/log/log.c
index 63afcf3..76af98b 100644
--- a/filters/log/log.c
+++ b/filters/log/log.c
@@ -217,9 +217,8 @@ log_open (nbdkit_next_open *next, void *nxdata, int
readonly)
     return NULL;
   }

-  pthread_mutex_lock (&lock);
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
   h->connection = ++connections;
-  pthread_mutex_unlock (&lock);
   h->id = 0;
   return h;
 }
diff --git a/filters/rate/rate.c b/filters/rate/rate.c
index 978cdc3..cf03541 100644
--- a/filters/rate/rate.c
+++ b/filters/rate/rate.c
@@ -222,9 +222,8 @@ maybe_adjust (const char *file, struct bucket *bucket,
pthread_mutex_t *lock)
   if (new_rate == -1)
     return;

-  pthread_mutex_lock (lock);
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (lock);
   old_rate = bucket_adjust_rate (bucket, new_rate);
-  pthread_mutex_unlock (lock);

   if (old_rate != new_rate)
     nbdkit_debug ("rate adjusted from %" PRIu64 " to %"
PRIi64,
@@ -245,9 +244,10 @@ maybe_sleep (struct bucket *bucket, pthread_mutex_t *lock,
uint32_t count)

   while (bits > 0) {
     /* Run the token bucket algorithm. */
-    pthread_mutex_lock (lock);
-    bits = bucket_run (bucket, bits, &ts);
-    pthread_mutex_unlock (lock);
+    {
+      ACQUIRE_LOCK_FOR_CURRENT_SCOPE (lock);
+      bits = bucket_run (bucket, bits, &ts);
+    }

     if (bits > 0)
       nanosleep (&ts, NULL);
diff --git a/filters/readahead/readahead.c b/filters/readahead/readahead.c
index f46b6b0..dc27bae 100644
--- a/filters/readahead/readahead.c
+++ b/filters/readahead/readahead.c
@@ -103,9 +103,8 @@ readahead_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata,
   if (r == -1)
     return -1;

-  pthread_mutex_lock (&lock);
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
   size = r;
-  pthread_mutex_unlock (&lock);

   return r;
 }
diff --git a/filters/stats/stats.c b/filters/stats/stats.c
index 96de154..7cbf129 100644
--- a/filters/stats/stats.c
+++ b/filters/stats/stats.c
@@ -100,9 +100,8 @@ stats_unload (void)
   gettimeofday (&now, NULL);
   usecs = tvdiff_usec (&start_t, &now);
   if (fp && usecs > 0) {
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     print_stats (usecs);
-    pthread_mutex_unlock (&lock);
   }

   if (fp)
@@ -163,10 +162,9 @@ stats_pread (struct nbdkit_next_ops *next_ops, void
*nxdata,

   r = next_ops->pread (nxdata, buf, count, offset, flags, err);
   if (r == 0) {
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     pread_ops++;
     pread_bytes += count;
-    pthread_mutex_unlock (&lock);
   }
   return r;
 }
@@ -182,10 +180,9 @@ stats_pwrite (struct nbdkit_next_ops *next_ops, void
*nxdata,

   r = next_ops->pwrite (nxdata, buf, count, offset, flags, err);
   if (r == 0) {
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     pwrite_ops++;
     pwrite_bytes += count;
-    pthread_mutex_unlock (&lock);
   }
   return r;
 }
@@ -201,10 +198,9 @@ stats_trim (struct nbdkit_next_ops *next_ops, void *nxdata,

   r = next_ops->trim (nxdata, count, offset, flags, err);
   if (r == 0) {
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     trim_ops++;
     trim_bytes += count;
-    pthread_mutex_unlock (&lock);
   }
   return r;
 }
@@ -220,10 +216,9 @@ stats_zero (struct nbdkit_next_ops *next_ops, void *nxdata,

   r = next_ops->zero (nxdata, count, offset, flags, err);
   if (r == 0) {
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     zero_ops++;
     zero_bytes += count;
-    pthread_mutex_unlock (&lock);
   }
   return r;
 }
@@ -239,14 +234,13 @@ stats_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,

   r = next_ops->extents (nxdata, count, offset, flags, extents, err);
   if (r == 0) {
-    pthread_mutex_lock (&lock);
+    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
     extents_ops++;
     /* XXX There's a case for trying to determine how long the extents
      * will be that are returned to the client, given the flags and
      * the complex rules in the protocol.
      */
     extents_bytes += count;
-    pthread_mutex_unlock (&lock);
   }
   return r;
 }
diff --git a/filters/error/Makefile.am b/filters/error/Makefile.am
index 9f116fe..1fb85ec 100644
--- a/filters/error/Makefile.am
+++ b/filters/error/Makefile.am
@@ -41,12 +41,15 @@ nbdkit_error_filter_la_SOURCES = \

 nbdkit_error_filter_la_CPPFLAGS = \
 	-I$(top_srcdir)/include \
-	-I$(top_srcdir)/common/include
+	-I$(top_srcdir)/common/include \
+	-I$(top_srcdir)/common/utils
 nbdkit_error_filter_la_CFLAGS = \
 	$(WARNINGS_CFLAGS)
 nbdkit_error_filter_la_LDFLAGS = \
 	-module -avoid-version -shared \
 	-Wl,--version-script=$(top_srcdir)/filters/filters.syms
+nbdkit_error_filter_la_LIBADD = \
+	$(top_builddir)/common/utils/libutils.la

 if HAVE_POD

-- 
2.20.1

On 4/24/19 5:24 PM, Eric Blake wrote:
> Commit 975dab14 argued that for simple lock/unlock sequences, it was
> easier to avoid the cleanup.h macros. But since that time, we added
> additional sanity checking to the macros, at which point the
> boilerplate of inlining that sanity checking is outweighed compared to
> just using the macros in more places.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  filters/cache/cache.c         | 23 +++++++++++------------
>  filters/cow/cow.c             | 19 ++++++++-----------
>  filters/error/error.c         |  7 ++++---
>  filters/log/log.c             |  3 +--
>  filters/rate/rate.c           | 10 +++++-----
>  filters/readahead/readahead.c |  3 +--
>  filters/stats/stats.c         | 18 ++++++------------
>  filters/error/Makefile.am     |  5 ++++-
>  8 files changed, 40 insertions(+), 48 deletions(-)
I forgot to squash in:

diff --git i/filters/error/error.c w/filters/error/error.c
index 22ebd1c..add7566 100644
--- i/filters/error/error.c
+++ w/filters/error/error.c
@@ -45,6 +45,7 @@

 #include <nbdkit-filter.h>

+#include "cleanup.h"
 #include "random.h"

 #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL

(Oddly enough, without that change, 'make' succeeds with status 0,
merely warning about an implicit symbol declaration rather than failing
the build, where it wasn't until 'make check' failing with .so
failing
to find all symbols that I noticed the problem. That's a bit
disconcerting that we don't fail sooner in the build...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

ACK

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 Wed, Apr 24, 2019 at 05:24:39PM -0500, Eric Blake
wrote:
> Add a helper function get_real_size() to make it easier to add sanity
> checking that mutex calls don't fail.
This patch is fine, ACK.
> I'm a bit unsure why truncate.c needs a lock anyway. I guess it's
> because we are storing 'size' and 'real_size' as globals
rather than
> per-connection values in the handle, and we're worrying about parallel
> connections where we don't want one thread reading inconsistent values
> while another thread is in the middle of truncate_get_size()? At any
> rate, as long as we don't have dynamic resize in the NBD protocol,
> size can't change within the confines of a single connection. And even
> if we DO assume that the underlying plugin reports different sizes for
> different connections, our use of a global does not play well (if
> client A sees size 1k, then client B sees size 2k, that changes client
> A's use of 'real_size' to be something different than client A
was
> expecting).
>
> So, I think we have to move 'size' and 'real_size' into the
> per-connection handle, at which point, can't we just set them once
> during truncate_prepare by moving the existing guts of
> truncate_get_size there, and then letting truncate_get_size just
> return h->size while all other truncate_* functions just access
> h->real_size without a lock?
Even storing fields in the handle is not necessarily safe when using
the parallel thread model, because you can have multiple requests
outstanding on the same handle.  (However I believe it is safe given
the current implementation of get_size / lack of resize support in
nbdkit.)

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/