common: lockdep now support unregistering once destructed

librbd use of an image hierarchy resulted in lock names being
re-used and incorrectly analyzed.  librbd now uses unique lock
names per instance, but to prevent an unbounded growth of
tracked locks, we now remove lock tracking once a lock is
destructed.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 7c7df2ce9f837628535d21df61ae4f13d809c4fa)

diff --git a/src/common/Mutex.cc b/src/common/Mutex.cc
index a0c1202183dcdc0ac42c74df8893a59539289bb3..011b6af09d65d38f796f4ed3ee36a0126139523d 100644 (file)
--- a/src/common/Mutex.cc
+++ b/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,
@@ -74,6 +75,9 @@ Mutex::~Mutex() {
     cct->get_perfcounters_collection()->remove(logger);
     delete logger;
   }
+  if (g_lockdep) {
+    lockdep_unregister(id);
+  }
 }
 
 void Mutex::Lock(bool no_lockdep) {

diff --git a/src/common/Mutex.h b/src/common/Mutex.h
index 758157536b708fd160aaf1c2e71e51f4cd0c4d6b..fb91d612a8b96a6595a10854e078259c7669ebe1 100644 (file)
--- a/src/common/Mutex.h
+++ b/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 @@ private:
   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 {

diff --git a/src/common/RWLock.h b/src/common/RWLock.h
index 6f0ab8ed0943714891bc70b40dc83bd2fee37d85..c82a23cccc60c44920aaf744fc885e99c59ae70a 100644 (file)
--- a/src/common/RWLock.h
+++ b/src/common/RWLock.h
@@ -18,6 +18,7 @@
 #define CEPH_RWLock_Posix__H
 
 #include <pthread.h>
+#include <string>
 #include <include/assert.h>
 #include "lockdep.h"
 #include "include/atomic.h"
@@ -25,17 +26,19 @@
 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 @@ public:
     // 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 @@ public:
       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 @@ public:
 
   // 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;
     }

diff --git a/src/common/lockdep.cc b/src/common/lockdep.cc
index 6639d8ad731bbafde6311b8d396631ba86066af3..e0b69e137249f3f74f29bd91b02f8856adaf4ede 100644 (file)
--- a/src/common/lockdep.cc
+++ b/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)

diff --git a/src/common/lockdep.h b/src/common/lockdep.h
index 1dcf05378ee9ee5bf71ac9ac65125b6ecfa9316d..e9949fd76fa17a902fcd3f6bd71d38f495c65df3 100644 (file)
--- a/src/common/lockdep.h
+++ 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);