lockdep: free_ids and lock_ref hashes must be truly global

It's possible for the teardown of g_lockdep_ceph_ctx to occur, followed
by a new context being registered as the lockdep context. When that
occurs, we can end up reusing lock id's that were previously handed out
to consumers. We need for those IDs to be persistent across lockdep
enablement and disablement.

Make both the free_ids table, and the lock_refs map persistent across
lockdep_unregister_ceph_context and lockdep_register_ceph_context cycles.
Entries in those tables will only be deleted by the destruction of the
associated mutex.

When lockdep_unregister is called, do the refcounting like we normally
would, but only clear out the state when the lockid is registered
in the lock_names hash.

Finally, we do still need to handle the case where g_lockdep has gone
false even when there are outstanding references after the decrement.
Only log the message if that's not the case.

With this, we can deal with the case of multiple clients enabling and
disabling lockdep in an unsynchronized way.

Tracker: http://tracker.ceph.com/issues/21512
Signed-off-by: Jeff Layton <jlayton@redhat.com>

diff --git a/src/common/lockdep.cc b/src/common/lockdep.cc
index 0ca8f777cc5a64263ac5467eb3a683445dc60e96..251a80150221a8290c9977fa3e35a5ef8a9dbf51 100644 (file)
--- a/src/common/lockdep.cc
+++ b/src/common/lockdep.cc
@@ -51,7 +51,8 @@ static ceph::unordered_map<pthread_t, map<int,BackTrace*> > held;
 static char follows[MAX_LOCKS][MAX_LOCKS/8]; // follows[a][b] means b taken after a
 static BackTrace *follows_bt[MAX_LOCKS][MAX_LOCKS];
 unsigned current_maxid;
-int last_freed_id;
+int last_freed_id = -1;
+static bool free_ids_inited;
 
 static bool lockdep_force_backtrace()
 {
@@ -73,10 +74,10 @@ void lockdep_register_ceph_context(CephContext *cct)
     g_lockdep = true;
     g_lockdep_ceph_ctx = cct;
     lockdep_dout(1) << "lockdep start" << dendl;
-    current_maxid = 0;
-       last_freed_id = -1;
-
-    memset((void*) &free_ids[0], 255, sizeof(free_ids));
+    if (!free_ids_inited) {
+      free_ids_inited = true;
+      memset((void*) &free_ids[0], 255, sizeof(free_ids));
+    }
   }
   pthread_mutex_unlock(&lockdep_mutex);
 }
@@ -100,12 +101,8 @@ void lockdep_unregister_ceph_context(CephContext *cct)
     held.clear();
     lock_names.clear();
     lock_ids.clear();
-    lock_refs.clear();
-    memset((void*)&free_ids[0], 0, sizeof(free_ids));
     memset((void*)&follows[0][0], 0, current_maxid * MAX_LOCKS/8);
     memset((void*)&follows_bt[0][0], 0, sizeof(BackTrace*) * current_maxid * MAX_LOCKS);
-    current_maxid = 0;
-    last_freed_id = -1;
   }
   pthread_mutex_unlock(&lockdep_mutex);
 }
@@ -215,40 +212,38 @@ void lockdep_unregister(int id)
   }
 
   pthread_mutex_lock(&lockdep_mutex);
-  if (!g_lockdep) {
-    pthread_mutex_unlock(&lockdep_mutex);
-    return;
-  }
 
+  std::string name;
   map<int, std::string>::iterator p = lock_names.find(id);
-  if (p == lock_names.end()) {
-    pthread_mutex_unlock(&lockdep_mutex);
-    return;
-  }
+  if (p == lock_names.end())
+    name = { "unknown" };
+  else
+    name = p->second;
 
   int &refs = lock_refs[id];
   if (--refs == 0) {
-    // reset dependency ordering
-    memset((void*)&follows[id][0], 0, MAX_LOCKS/8);
-    for (unsigned i=0; i<current_maxid; ++i) {
-      delete follows_bt[id][i];
-      follows_bt[id][i] = NULL;
-
-      delete follows_bt[i][id];
-      follows_bt[i][id] = NULL;
-      follows[i][id / 8] &= 255 - (1 << (id % 8));
-    }
+    if (p != lock_names.end()) {
+      // reset dependency ordering
+      memset((void*)&follows[id][0], 0, MAX_LOCKS/8);
+      for (unsigned i=0; i<current_maxid; ++i) {
+        delete follows_bt[id][i];
+        follows_bt[id][i] = NULL;
+
+        delete follows_bt[i][id];
+        follows_bt[i][id] = NULL;
+        follows[i][id / 8] &= 255 - (1 << (id % 8));
+      }
 
-    lockdep_dout(10) << "unregistered '" << p->second << "' from " << id
-                     << dendl;
-    lock_ids.erase(p->second);
-    lock_names.erase(id);
+      lockdep_dout(10) << "unregistered '" << name << "' from " << id << dendl;
+      lock_ids.erase(p->second);
+      lock_names.erase(id);
+    }
     lock_refs.erase(id);
     free_ids[id/8] |= (1 << (id % 8));
-       last_freed_id = id;
-  } else {
-    lockdep_dout(20) << "have " << refs << " of '" << p->second << "' "
-                     << "from " << id << dendl;
+    last_freed_id = id;
+  } else if (g_lockdep) {
+    lockdep_dout(20) << "have " << refs << " of '" << name << "' " <<
+                       "from " << id << dendl;
   }
   pthread_mutex_unlock(&lockdep_mutex);
 }