lockdep: separate from Mutex; include checks for RWLock

src/Makefile.am

@@ -167,7 +167,7 @@ libcommon_a_SOURCES = \
 	msg/Message.cc \
 	common/Logger.cc \
 	common/Clock.cc \
-	common/Mutex.cc \
+	common/lockdep.cc \
 	common/Timer.cc \
 	common/Finisher.cc \
 	common/sctp_crc32.c\
@@ -254,6 +254,7 @@ noinst_HEADERS = \
         client/fuse.h\
         client/fuse_ll.h\
         client/hadoop/CephFSInterface.h\
+	common/lockdep.h\
 	common/BackTrace.h\
         common/Clock.h\
         common/Cond.h\

src/common/Mutex.h

@@ -17,11 +17,10 @@
 
 #include <pthread.h>
 #include "include/assert.h"
+#include "lockdep.h"
 
 #define LOCKDEP
 
-extern int g_lockdep;
-
 
 class Mutex {
 private:
@@ -38,10 +37,18 @@ private:
   Mutex( const Mutex &M ) {}
 
 #ifdef LOCKDEP
-  void _register();  
-  void _will_lock(); // about to lock
-  void _locked();    // just locked
-  void _unlocked();  // just unlocked
+  void _register() {
+    id = lockdep_register(name);
+  }
+  void _will_lock() { // about to lock
+    id = lockdep_will_lock(name, id);
+  }
+  void _locked() {    // just locked
+    id = lockdep_locked(name, id);
+  }
+  void _unlocked() {  // just unlocked
+    id = lockdep_unlocked(name, id);
+  }
 #else
   void _register() {}
   void _will_lock() {} // about to lock

src/common/RWLock.h

@@ -18,15 +18,19 @@
 #define _RWLock_Posix_
 
 #include <pthread.h>
+#include "lockdep.h"
 
 class RWLock
 {
   mutable pthread_rwlock_t L;
+  const char *name;
+  int id;
 
   public:
 
-  RWLock() {
+  RWLock(const char *n) : name(n), id(-1) {
     pthread_rwlock_init(&L, NULL);
+    if (g_lockdep) id = lockdep_register(name);
   }
 
   virtual ~RWLock() {
@@ -35,14 +39,19 @@ class RWLock
   }
 
   void unlock() {
+    if (g_lockdep) id = lockdep_unlocked(name, id);
     pthread_rwlock_unlock(&L);
   }
   void get_read() {
+    if (g_lockdep) id = lockdep_will_lock(name, id);
     pthread_rwlock_rdlock(&L);    
+    if (g_lockdep) id = lockdep_locked(name, id);
   }
   void put_read() { unlock(); }
   void get_write() {
+    if (g_lockdep) id = lockdep_will_lock(name, id);
     pthread_rwlock_wrlock(&L);
+    if (g_lockdep) id = lockdep_locked(name, id);
   }
   void put_write() { unlock(); }
 };

src/common/lockdep.cc

@@ -0,0 +1,192 @@
+
+#include "lockdep.h"
+
+int g_lockdep = 0;
+
+#include "include/types.h"
+#include "Clock.h"
+#include "BackTrace.h"
+
+#include <ext/hash_map>
+
+#include "config.h"
+
+#define  dout(l)    if (l<=g_conf.debug_lockdep) *_dout << g_clock.now() << " " << pthread_self() << " lockdep: "
+#define  derr(l)    if (l<=g_conf.debug_lockdep) *_derr << g_clock.now() << " " << pthread_self() << " lockdep: "
+
+
+pthread_mutex_t lockdep_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+
+#define MAX_LOCKS  100   // increase me as needed
+
+hash_map<const char *, int> lock_ids;
+map<int, const char *> lock_names;
+
+int last_id = 0;
+
+hash_map<pthread_t, map<int,BackTrace*> > held;
+BackTrace *follows[MAX_LOCKS][MAX_LOCKS];       // follows[a][b] means b taken after a 
+
+
+#define BACKTRACE_SKIP 3
+
+
+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;
+  
+  hash_map<const char *, int>::iterator p = lock_ids.find(name);
+  if (p == lock_ids.end()) {
+    assert(last_id < MAX_LOCKS);
+    id = last_id++;
+    lock_ids[name] = id;
+    lock_names[id] = name;
+    dout(10) << "registered '" << name << "' as " << id << std::endl;
+  } else {
+    id = p->second;
+    dout(20) << "had '" << name << "' as " << id << std::endl;
+  }
+
+  pthread_mutex_unlock(&lockdep_mutex);
+
+  return id;
+}
+
+
+// does a follow b?
+static bool does_follow(int a, int b)
+{
+  if (follows[a][b]) {
+    *_dout << std::endl;
+    dout(0) << "------------------------------------" << std::endl;
+    dout(0) << "existing dependency " << lock_names[a] << " (" << a << ") -> " << lock_names[b] << " (" << b << ") at: " << std::endl;
+    follows[a][b]->print(*_dout);
+    *_dout << std::endl;
+    return true;
+  }
+
+  for (int i=0; i<MAX_LOCKS; i++) {
+    if (follows[a][i] &&
+	does_follow(i, b)) {
+      dout(0) << "existing intermediate dependency " << lock_names[a] << " (" << a << ") -> " << lock_names[i] << " (" << i << ") at:" << std::endl;
+      follows[a][i]->print(*_dout);
+      *_dout << std::endl;
+      return true;
+    }
+  }
+
+  return false;
+}
+
+int lockdep_will_lock(const char *name, int id)
+{
+  pthread_t p = pthread_self();
+  if (id < 0) id = lockdep_register(name);
+
+  pthread_mutex_lock(&lockdep_mutex);
+  dout(20) << "_will_lock " << name << " (" << id << ")" << std::endl;
+
+  // check dependency graph
+  map<int, BackTrace *> &m = held[p];
+  for (map<int, BackTrace *>::iterator p = m.begin();
+       p != m.end();
+       p++) {
+    if (p->first == id) {
+      *_dout << std::endl;
+      dout(0) << "recursive lock of " << name << " (" << id << ")" << std::endl;
+      BackTrace *bt = new BackTrace(BACKTRACE_SKIP);
+      bt->print(*_dout);
+      if (g_lockdep >= 2) {
+	*_dout << std::endl;
+	dout(0) << "previously locked at" << std::endl;
+	p->second->print(*_dout);
+      }
+      *_dout << std::endl;
+      assert(0);
+    }
+    else if (!follows[p->first][id]) {
+      // new dependency 
+      
+      // did we just create a cycle?
+      BackTrace *bt = new BackTrace(BACKTRACE_SKIP);
+      if (does_follow(id, p->first)) {
+	dout(0) << "new dependency " << lock_names[p->first] << " (" << p->first << ") -> " << name << " (" << id << ")"
+		<< " creates a cycle at"
+		<< std::endl;
+	bt->print(*_dout);
+	*_dout << std::endl;
+
+	dout(0) << "btw, i am holding these locks:" << std::endl;
+	for (map<int, BackTrace *>::iterator q = m.begin();
+	     q != m.end();
+	     q++) {
+	  dout(0) << "  " << lock_names[q->first] << " (" << q->first << ")" << std::endl;
+	  if (g_lockdep >= 2) {
+	    q->second->print(*_dout);
+	    *_dout << std::endl;
+	  }
+	}
+
+	*_dout << std::endl;
+	*_dout << std::endl;
+
+	// don't add this dependency, or we'll get aMutex. cycle in the graph, and
+	// does_follow() won't terminate.
+
+	assert(0);  // actually, we should just die here.
+      } else {
+	follows[p->first][id] = bt;
+	dout(10) << lock_names[p->first] << " -> " << name << " at" << std::endl;
+	//bt->print(*_dout);
+      }
+    }
+  }
+
+  pthread_mutex_unlock(&lockdep_mutex);
+  return id;
+}
+
+int lockdep_locked(const char *name, int id)
+{
+  pthread_t p = pthread_self();
+
+  if (id < 0) id = lockdep_register(name);
+
+  pthread_mutex_lock(&lockdep_mutex);
+  dout(20) << "_locked " << name << std::endl;
+  if (g_lockdep >= 2)
+    held[p][id] = new BackTrace(BACKTRACE_SKIP);
+  else
+    held[p][id] = 0;
+  pthread_mutex_unlock(&lockdep_mutex);
+  return id;
+}
+
+int lockdep_unlocked(const char *name, int id)
+{
+  pthread_t p = pthread_self();
+  
+  if (id < 0) id = lockdep_register(name);
+
+  pthread_mutex_lock(&lockdep_mutex);
+  dout(20) << "_unlocked " << name << std::endl;
+
+  // don't assert.. lockdep may be enabled at any point in time
+  //assert(held.count(p));
+  //assert(held[p].count(id));
+
+  delete held[p][id];
+  held[p].erase(id);
+  pthread_mutex_unlock(&lockdep_mutex);
+  return id;
+}
+
+

src/common/lockdep.h

@@ -0,0 +1,11 @@
+#ifndef _CEPH_LOCKDEP_H
+#define _CEPH_LOCKDEP_H
+
+extern int g_lockdep;
+
+extern int lockdep_register(const char *n);
+extern int lockdep_will_lock(const char *n, int id);
+extern int lockdep_locked(const char *n, int id);
+extern int lockdep_unlocked(const char *n, int id);
+
+#endif

src/os/JournalingObjectStore.h

@@ -101,6 +101,7 @@ protected:
 
 public:
   JournalingObjectStore() : op_seq(0), committing_op_seq(0), journal(0),
+			    op_lock("JournalingObjectStore::op_lock"),
 			    journal_lock("JournalingObjectStore::journal_lock"),
 			    lock("JournalingObjectStore::lock") { }