Merge pull request ceph#4528 from ceph/wip-librbd-helgrind

librbd: correct issues discovered via lockdep / helgrind

Reviewed-by: Josh Durgin <[email protected]>

configure.ac

@@ -1154,11 +1154,14 @@ AC_ARG_ENABLE([valgrind],
               [AS_HELP_STRING([--enable-valgrind], [enable valgrind unit tests])],
               [enable_valgrind=$enableval], [enable_valgrind=])
 AC_CHECK_PROG(HAVE_VALGRIND, valgrind, yes)
-AS_IF([test "x$HAVE_VALGRIND" = "x"],
-  AS_IF([test "x$enable_valgrind" = "xyes"], [AC_MSG_ERROR([valgrind not found])]),
+AS_IF(
+  [test "x$HAVE_VALGRIND" = "x"], AS_IF([test "x$enable_valgrind" = "xyes"], [AC_MSG_ERROR([valgrind not found])]),
   [test "x$enable_valgrind" = "x"], [enable_valgrind=yes])
 
 AM_CONDITIONAL([VALGRIND_ENABLED], [test "x$enable_valgrind" = "xyes"])
+if test "x$enable_valgrind" = "xyes"; then
+  AC_CHECK_HEADERS([valgrind/helgrind.h])
+fi
 
 dnl systemd-libexec-dir
 AC_SUBST(systemd_libexec_dir)

src/common/Makefile.am

@@ -233,7 +233,8 @@ noinst_HEADERS += \
 	common/Initialize.h \
 	common/ContextCompletion.h \
 	common/bit_vector.hpp \
-	common/SubProcess.h
+	common/SubProcess.h \
+	common/valgrind.h
 
 if ENABLE_XIO
 noinst_HEADERS += \

src/common/Mutex.cc

@@ -17,14 +17,15 @@
 #include "common/perf_counters.h"
 #include "common/ceph_context.h"
 #include "common/config.h"
+#include "include/stringify.h"
 #include "include/utime.h"
 #include "common/Clock.h"
 
-Mutex::Mutex(const char *n, bool r, bool ld,
+Mutex::Mutex(const std::string &n, bool r, bool ld,
 	     bool bt,
 	     CephContext *cct) :
-  name(n), id(-1), recursive(r), lockdep(ld), backtrace(bt),
-  nlock(0), locked_by(0), cct(cct), logger(0)
+  name(n), id(-1), recursive(r), lockdep(ld), backtrace(bt), nlock(0),
+  locked_by(0), cct(cct), logger(0)
 {
   if (cct) {
     PerfCountersBuilder b(cct, string("mutex-") + name,
@@ -75,6 +76,9 @@ Mutex::~Mutex() {
     cct->get_perfcounters_collection()->remove(logger);
     delete logger;
   }
+  if (g_lockdep) {
+    lockdep_unregister(id);
+  }
 }
 
 void Mutex::Lock(bool no_lockdep) {

src/common/Mutex.h

@@ -33,7 +33,7 @@ enum {
 
 class Mutex {
 private:
-  const char *name;
+  std::string name;
   int id;
   bool recursive;
   bool lockdep;
@@ -50,20 +50,20 @@ class Mutex {
   Mutex(const Mutex &M);
 
   void _register() {
-    id = lockdep_register(name);
+    id = lockdep_register(name.c_str());
   }
   void _will_lock() { // about to lock
-    id = lockdep_will_lock(name, id);
+    id = lockdep_will_lock(name.c_str(), id);
   }
   void _locked() {    // just locked
-    id = lockdep_locked(name, id, backtrace);
+    id = lockdep_locked(name.c_str(), id, backtrace);
   }
   void _will_unlock() {  // about to unlock
-    id = lockdep_will_unlock(name, id);
+    id = lockdep_will_unlock(name.c_str(), id);
   }
 
 public:
-  Mutex(const char *n, bool r = false, bool ld=true, bool bt=false,
+  Mutex(const std::string &n, bool r = false, bool ld=true, bool bt=false,
 	CephContext *cct = 0);
   ~Mutex();
   bool is_locked() const {

src/common/RWLock.h

@@ -18,24 +18,27 @@
 #define CEPH_RWLock_Posix__H
 
 #include <pthread.h>
+#include <string>
 #include <include/assert.h>
 #include "lockdep.h"
 #include "include/atomic.h"
 
 class RWLock
 {
   mutable pthread_rwlock_t L;
-  const char *name;
+  std::string name;
   mutable int id;
   mutable atomic_t nrlock, nwlock;
 
+  std::string unique_name(const char* name) const;
+
 public:
   RWLock(const RWLock& other);
   const RWLock& operator=(const RWLock& other);
 
-  RWLock(const char *n) : name(n), id(-1), nrlock(0), nwlock(0) {
+  RWLock(const std::string &n) : name(n), id(-1), nrlock(0), nwlock(0) {
     pthread_rwlock_init(&L, NULL);
-    if (g_lockdep) id = lockdep_register(name);
+    if (g_lockdep) id = lockdep_register(name.c_str());
   }
 
   bool is_locked() const {
@@ -50,6 +53,9 @@ class RWLock
     // the object and we assume that there are no other users.
     assert(!is_locked());
     pthread_rwlock_destroy(&L);
+    if (g_lockdep) {
+      lockdep_unregister(id);
+    }
   }
 
   void unlock(bool lockdep=true) const {
@@ -59,23 +65,23 @@ class RWLock
       assert(nrlock.read() > 0);
       nrlock.dec();
     }
-    if (lockdep && g_lockdep) id = lockdep_will_unlock(name, id);
+    if (lockdep && g_lockdep) id = lockdep_will_unlock(name.c_str(), id);
     int r = pthread_rwlock_unlock(&L);
     assert(r == 0);
   }
 
   // read
   void get_read() const {
-    if (g_lockdep) id = lockdep_will_lock(name, id);
+    if (g_lockdep) id = lockdep_will_lock(name.c_str(), id);
     int r = pthread_rwlock_rdlock(&L);
     assert(r == 0);
-    if (g_lockdep) id = lockdep_locked(name, id);
+    if (g_lockdep) id = lockdep_locked(name.c_str(), id);
     nrlock.inc();
   }
   bool try_get_read() const {
     if (pthread_rwlock_tryrdlock(&L) == 0) {
       nrlock.inc();
-      if (g_lockdep) id = lockdep_locked(name, id);
+      if (g_lockdep) id = lockdep_locked(name.c_str(), id);
       return true;
     }
     return false;
@@ -86,16 +92,16 @@ class RWLock
 
   // write
   void get_write(bool lockdep=true) {
-    if (lockdep && g_lockdep) id = lockdep_will_lock(name, id);
+    if (lockdep && g_lockdep) id = lockdep_will_lock(name.c_str(), id);
     int r = pthread_rwlock_wrlock(&L);
     assert(r == 0);
-    if (g_lockdep) id = lockdep_locked(name, id);
+    if (g_lockdep) id = lockdep_locked(name.c_str(), id);
     nwlock.inc();
 
   }
   bool try_get_write(bool lockdep=true) {
     if (pthread_rwlock_trywrlock(&L) == 0) {
-      if (lockdep && g_lockdep) id = lockdep_locked(name, id);
+      if (lockdep && g_lockdep) id = lockdep_locked(name.c_str(), id);
       nwlock.inc();
       return true;
     }

src/common/WorkQueue.h

@@ -433,35 +433,36 @@ class C_QueueInWQ : public Context {
   }
 };
 
-class ContextWQ : public ThreadPool::WorkQueueVal<Context *> {
+class ContextWQ : public ThreadPool::WorkQueueVal<std::pair<Context *, int> > {
 public:
   ContextWQ(const string &name, time_t ti, ThreadPool *tp)
-    : ThreadPool::WorkQueueVal<Context *>(name, ti, 0, tp) {}
+    : ThreadPool::WorkQueueVal<std::pair<Context *, int> >(name, ti, 0, tp) {}
 
-  void queue(Context *ctx) {
-    ThreadPool::WorkQueueVal<Context *>::queue(ctx);
+  void queue(Context *ctx, int result = 0) {
+    ThreadPool::WorkQueueVal<std::pair<Context *, int> >::queue(
+      std::make_pair(ctx, result));
   }
 
 protected:
-  virtual void _enqueue(Context *item) {
+  virtual void _enqueue(std::pair<Context *, int> item) {
     _queue.push_back(item);
   }
-  virtual void _enqueue_front(Context *item) {
+  virtual void _enqueue_front(std::pair<Context *, int> item) {
     _queue.push_front(item);
   }
   virtual bool _empty() {
     return _queue.empty();
   }
-  virtual Context *_dequeue() {
-    Context *item = _queue.front();
+  virtual std::pair<Context *, int> _dequeue() {
+    std::pair<Context *, int> item = _queue.front();
     _queue.pop_front();
     return item;
   }
-  virtual void _process(Context *item) {
-    item->complete(0);
+  virtual void _process(std::pair<Context *, int> item) {
+    item.first->complete(item.second);
   }
 private:
-  list<Context *> _queue;
+  list<std::pair<Context *, int> > _queue;
 };
 
 class ShardedThreadPool {

src/common/lockdep.cc

@@ -49,9 +49,10 @@ struct lockdep_stopper_t {
 static pthread_mutex_t lockdep_mutex = PTHREAD_MUTEX_INITIALIZER;
 static CephContext *g_lockdep_ceph_ctx = NULL;
 static lockdep_stopper_t lockdep_stopper;
-static ceph::unordered_map<const char *, int> lock_ids;
-static map<int, const char *> lock_names;
-static int last_id = 0;
+static ceph::unordered_map<std::string, int> lock_ids;
+static map<int, std::string> lock_names;
+static map<int, int> lock_refs;
+static list<int> free_ids;
 static ceph::unordered_map<pthread_t, map<int,BackTrace*> > held;
 static BackTrace *follows[MAX_LOCKS][MAX_LOCKS];       // follows[a][b] means b taken after a
 
@@ -62,6 +63,10 @@ void lockdep_register_ceph_context(CephContext *cct)
   if (g_lockdep_ceph_ctx == NULL) {
     g_lockdep_ceph_ctx = cct;
     lockdep_dout(0) << "lockdep start" << dendl;
+
+    for (int i=0; i<MAX_LOCKS; ++i) {
+      free_ids.push_back(i);
+    }
   }
   pthread_mutex_unlock(&lockdep_mutex);
 }
@@ -82,7 +87,8 @@ void lockdep_unregister_ceph_context(CephContext *cct)
 	follows[i][j] = NULL;
     lock_names.clear();
     lock_ids.clear();
-    last_id = 0;
+    lock_refs.clear();
+    free_ids.clear();
   }
   pthread_mutex_unlock(&lockdep_mutex);
 }
@@ -115,15 +121,12 @@ int lockdep_register(const char *name)
   int id;
 
   pthread_mutex_lock(&lockdep_mutex);
-  if (last_id == 0)
-    for (int i=0; i<MAX_LOCKS; i++)
-      for (int j=0; j<MAX_LOCKS; j++)
-	follows[i][j] = NULL;
-
-  ceph::unordered_map<const char *, int>::iterator p = lock_ids.find(name);
+  ceph::unordered_map<std::string, int>::iterator p = lock_ids.find(name);
   if (p == lock_ids.end()) {
-    assert(last_id < MAX_LOCKS);
-    id = last_id++;
+    assert(!free_ids.empty());
+    id = free_ids.front();
+    free_ids.pop_front();
+
     lock_ids[name] = id;
     lock_names[id] = name;
     lockdep_dout(10) << "registered '" << name << "' as " << id << dendl;
@@ -132,11 +135,47 @@ int lockdep_register(const char *name)
     lockdep_dout(20) << "had '" << name << "' as " << id << dendl;
   }
 
+  ++lock_refs[id];
   pthread_mutex_unlock(&lockdep_mutex);
 
   return id;
 }
 
+void lockdep_unregister(int id)
+{
+  if (id < 0) {
+    return;
+  }
+
+  pthread_mutex_lock(&lockdep_mutex);
+
+  map<int, std::string>::iterator p = lock_names.find(id);
+  assert(p != lock_names.end());
+
+  int &refs = lock_refs[id];
+  if (--refs == 0) {
+    // reset dependency ordering
+    for (int i=0; i<MAX_LOCKS; ++i) {
+      delete follows[id][i];
+      follows[id][i] = NULL;
+
+      delete follows[i][id];
+      follows[i][id] = NULL;
+    }
+
+    lockdep_dout(10) << "unregistered '" << p->second << "' from " << id
+                     << dendl;
+    lock_ids.erase(p->second);
+    lock_names.erase(id);
+    lock_refs.erase(id);
+    free_ids.push_back(id);
+  } else {
+    lockdep_dout(20) << "have " << refs << " of '" << p->second << "' "
+                     << "from " << id << dendl;
+  }
+  pthread_mutex_unlock(&lockdep_mutex);
+}
+
 
 // does b follow a?
 static bool does_follow(int a, int b)

src/common/lockdep.h

@@ -22,6 +22,7 @@ extern int g_lockdep;
 extern void lockdep_register_ceph_context(CephContext *cct);
 extern void lockdep_unregister_ceph_context(CephContext *cct);
 extern int lockdep_register(const char *n);
+extern void lockdep_unregister(int id);
 extern int lockdep_will_lock(const char *n, int id);
 extern int lockdep_locked(const char *n, int id, bool force_backtrace=false);
 extern int lockdep_will_unlock(const char *n, int id);

src/common/valgrind.h

@@ -0,0 +1,15 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
+
+#ifndef CEPH_VALGRIND_H
+#define CEPH_VALGRIND_H
+
+#ifdef HAVE_VALGRIND_HELGRIND_H
+  #include <valgrind/helgrind.h>
+#else
+  #define ANNOTATE_HAPPENS_AFTER(x)             do {} while (0)
+  #define ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(x) ANNOTATE_HAPPENS_AFTER(x)
+  #define ANNOTATE_HAPPENS_BEFORE(x)            ANNOTATE_HAPPENS_AFTER(x)
+#endif
+
+#endif // CEPH_VALGRIND_H

src/librbd/AioCompletion.cc

@@ -89,7 +89,9 @@ namespace librbd {
     }
 
     if (complete_cb) {
+      lock.Unlock();
       complete_cb(rbd_comp, complete_arg);
+      lock.Lock();
     }
     done = true;
     cond.Signal();
@@ -171,6 +173,17 @@ namespace librbd {
     m_completion->complete_request(m_cct, r);
   }
 
+  void C_CacheRead::complete(int r) {
+    if (!m_enqueued) {
+      // cache_lock creates a lock ordering issue -- so re-execute this context
+      // outside the cache_lock
+      m_enqueued = true;
+      m_image_ctx.op_work_queue->queue(this, r);
+      return;
+    }
+    Context::complete(r);
+  }
+
   void C_CacheRead::finish(int r)
   {
     m_req->complete(r);