Unify object level lock in GenericObjectMap

Before we copy lock implementation from DBObjectMap which provide with two locks
for header. Here just unify it to make ease.

Signed-off-by: Haomai Wang <haomaiwang@gmail.com>

diff --git a/src/os/GenericObjectMap.cc b/src/os/GenericObjectMap.cc
index 94c6b3bb8c0038a7e67b0b4f5d0bb9ad1cb25341..4d41c50ae3a59026679a9d8498bea7432cd127a3 100644 (file)
--- a/src/os/GenericObjectMap.cc
+++ b/src/os/GenericObjectMap.cc
@@ -624,6 +624,7 @@ int GenericObjectMap::get_keys(const coll_t &cid, const ghobject_t &oid,
   }
   return 0;
 }
+
 int GenericObjectMap::get_values(const coll_t &cid, const ghobject_t &oid,
                                  const string &prefix,
                                  const set<string> &keys,
@@ -916,25 +917,44 @@ int GenericObjectMap::write_state(KeyValueDB::Transaction t)
   return 0;
 }
 
+// NOTE(haomai): It may occur dead lock if thread A hold header A try to header
+// B and thread hold header B try to get header A
 GenericObjectMap::Header GenericObjectMap::_lookup_header(
     const coll_t &cid, const ghobject_t &oid)
 {
-  // FIXME
-  while (map_header_in_use.count(oid))
-    header_cond.Wait(header_lock);
-
-  map<string, bufferlist> out;
   set<string> to_get;
   to_get.insert(header_key(cid, oid));
-  int r = db->get(GHOBJECT_TO_SEQ_PREFIX, to_get, &out);
-  if (r < 0)
-    return Header();
-  if (out.empty())
-    return Header();
+  _Header header;
 
-  Header ret(new _Header(), RemoveMapHeaderOnDelete(this, cid, oid));
-  bufferlist::iterator iter = out.begin()->second.begin();
-  ret->decode(iter);
+  while (1) {
+    map<string, bufferlist> out;
+    bool try_again = false;
+
+    int r = db->get(GHOBJECT_TO_SEQ_PREFIX, to_get, &out);
+    if (r < 0)
+      return Header();
+    if (out.empty())
+      return Header();
+
+    bufferlist::iterator iter = out.begin()->second.begin();
+    header.decode(iter);
+
+    while (in_use.count(header.seq)) {
+      header_cond.Wait(header_lock);
+
+      // Another thread is hold this header, wait for it.
+      // Because the seq of this object may change, such as clone
+      // and rename operation, here need to look up "seq" again
+      try_again = true;
+    }
+
+    if (!try_again) {
+      break;
+    }
+  }
+
+  Header ret = Header(new _Header(header), RemoveOnDelete(this));
+  in_use.insert(ret->seq);
   return ret;
 }
 
@@ -968,6 +988,7 @@ GenericObjectMap::Header GenericObjectMap::lookup_parent(Header input)
 
   dout(20) << "lookup_parent: parent " << input->parent
        << " for seq " << input->seq << dendl;
+
   int r = db->get(parent_seq_prefix(input->parent), keys, &out);
   if (r < 0) {
     assert(0);
@@ -983,7 +1004,7 @@ GenericObjectMap::Header GenericObjectMap::lookup_parent(Header input)
   bufferlist::iterator iter = out.begin()->second.begin();
   header->decode(iter);
   dout(20) << "lookup_parent: parent seq is " << header->seq << " with parent "
-       << header->parent << dendl;
+           << header->parent << dendl;
   in_use.insert(header->seq);
   return header;
 }
@@ -1018,6 +1039,7 @@ void GenericObjectMap::clear_header(Header header, KeyValueDB::Transaction t)
   t->rmkeys(parent_seq_prefix(header->seq), keys);
 }
 
+// only remove GHOBJECT_TO_SEQ
 void GenericObjectMap::remove_header(const coll_t &cid,
                                      const ghobject_t &oid, Header header,
                                      KeyValueDB::Transaction t)

diff --git a/src/os/GenericObjectMap.h b/src/os/GenericObjectMap.h
index 88e7b87074a692f4237acaba9a60780c195450f8..c9c64bc701efeddb5306f6f14b786b7625c323f0 100644 (file)
--- a/src/os/GenericObjectMap.h
+++ b/src/os/GenericObjectMap.h
@@ -30,6 +30,8 @@
 #include "osd/osd_types.h"
 #include "common/Mutex.h"
 #include "common/Cond.h"
+#include "common/simple_cache.hpp"
+
 
 /**
  * Genericobjectmap: Provide with key/value associated to ghobject_t APIs to caller
@@ -73,13 +75,11 @@ class GenericObjectMap {
    */
   Mutex header_lock;
   Cond header_cond;
-  Cond map_header_cond;
 
   /**
    * Set of headers currently in use
    */
   set<uint64_t> in_use;
-  set<ghobject_t> map_header_in_use;
 
   GenericObjectMap(KeyValueDB *db) : db(db), header_lock("GenericObjectMap") {}
 
@@ -365,7 +365,7 @@ private:
     int adjust();
   };
 
- protected:
+protected:
   typedef ceph::shared_ptr<GenericObjectMapIteratorImpl> GenericObjectMapIterator;
   GenericObjectMapIterator _get_iterator(Header header, string prefix) {
     return GenericObjectMapIterator(new GenericObjectMapIteratorImpl(this, header, prefix));
@@ -376,6 +376,7 @@ private:
            set<string> *out_keys, map<string, bufferlist> *out_values);
 
  private:
+
   /// Removes node corresponding to header
   void clear_header(Header header, KeyValueDB::Transaction t);
 
@@ -425,28 +426,9 @@ private:
   void _set_header(Header header, const bufferlist &bl,
                    KeyValueDB::Transaction t);
 
-  /** 
-   * Removes map header lock once Header is out of scope
-   * @see lookup_header
-   */
-  class RemoveMapHeaderOnDelete {
-  public:
-    GenericObjectMap *db;
-    coll_t cid;
-    ghobject_t oid;
-    RemoveMapHeaderOnDelete(GenericObjectMap *db, const coll_t &cid,
-        const ghobject_t &oid) :
-      db(db), cid(cid), oid(oid) {}
-    void operator() (_Header *header) {
-      Mutex::Locker l(db->header_lock);
-      db->map_header_in_use.erase(oid);
-      db->map_header_cond.Signal();
-      delete header;
-    }
-  };
-
   /** 
    * Removes header seq lock once Header is out of scope
+   * @see _lookup_header
    * @see lookup_parent
    * @see generate_new_header
    */