common/TrackedOp: use explicit ref count, intrusive_ptr

This is more efficient and more explicit than shared_ptr.  It will also
allow us to put these in intrusive lists.

Note that before we used the shared_ptr OnDelete property only for the
live ops and left it off for the history.  Here we rename is_tracked to
state and explicit note whether we are untracked, live, or history.

Also, now that we #include OpRequest in osd_types.h, we have to include
the .cc file (and ~OpRequest definition) in libcommon to avoid a link
error on all the unit tests etc.

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index d7cf3a2b57c1acd98982b985aecb0726edb1f5d9..35980e319a93c902273df493638ecc020a470280 100644 (file)
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -461,6 +461,7 @@ set(libcommon_files
   osd/OSDMap.cc
   common/histogram.cc
   osd/osd_types.cc
+  osd/OpRequest.cc
   common/blkdev.cc
   common/common_init.cc
   common/pipe.c
@@ -595,6 +596,7 @@ add_library(common_utf8 STATIC common/utf8.c)
 
 if(${WITH_LTTNG})
   add_subdirectory(tracing)
+  add_dependencies(common-objs oprequest-tp)
 endif(${WITH_LTTNG})
 
 add_subdirectory(global)

diff --git a/src/common/TrackedOp.cc b/src/common/TrackedOp.cc
index 00f66c7cefae087194ebf9c5066f56e4b623b155..52396bec37ba38fec983fc9ebfa2a80530f6a762 100644 (file)
--- a/src/common/TrackedOp.cc
+++ b/src/common/TrackedOp.cc
@@ -182,7 +182,7 @@ bool OpTracker::register_inflight_op(xlist<TrackedOp*>::item *i)
 void OpTracker::unregister_inflight_op(TrackedOp *i)
 {
   // caller checks;
-  assert(i->is_tracked);
+  assert(i->state);
 
   uint32_t shard_index = i->seq % num_optracker_shards;
   ShardedTrackingData* sdata = sharded_in_flight_list[shard_index];
@@ -198,6 +198,7 @@ void OpTracker::unregister_inflight_op(TrackedOp *i)
   if (!tracking_enabled)
     delete i;
   else {
+    i->state = TrackedOp::STATE_HISTORY;
     utime_t now = ceph_clock_now();
     history.insert(now, TrackedOpRef(i));
   }
@@ -315,7 +316,7 @@ void OpTracker::get_age_ms_histogram(pow2_hist_t *h)
 
 void OpTracker::mark_event(TrackedOp *op, const string &dest, utime_t time)
 {
-  if (!op->is_tracked)
+  if (!op->state)
     return;
   return _mark_event(op, dest, time);
 }
@@ -332,20 +333,9 @@ void OpTracker::_mark_event(TrackedOp *op, const string &evt,
      
 }
 
-void OpTracker::RemoveOnDelete::operator()(TrackedOp *op) {
-  if (!op->is_tracked) {
-    op->_unregistered();
-    delete op;
-    return;
-  }
-  op->mark_event("done");
-  tracker->unregister_inflight_op(op);
-  // Do not delete op, unregister_inflight_op took control
-}
-
 void TrackedOp::mark_event(const string &event)
 {
-  if (!is_tracked)
+  if (!state)
     return;
 
   utime_t now = ceph_clock_now();
@@ -360,7 +350,7 @@ void TrackedOp::mark_event(const string &event)
 void TrackedOp::dump(utime_t now, Formatter *f) const
 {
   // Ignore if still in the constructor
-  if (!is_tracked)
+  if (!state)
     return;
   stringstream name;
   _dump_op_descriptor_unlocked(name);

diff --git a/src/common/TrackedOp.h b/src/common/TrackedOp.h
index 5b21c5aee1f2ef791e45508f98057c88e4eca2b3..7717df01ec71a99096e9b688de753e75ef77985e 100644 (file)
--- a/src/common/TrackedOp.h
+++ b/src/common/TrackedOp.h
@@ -25,7 +25,7 @@
 #include <atomic>
 
 class TrackedOp;
-typedef ceph::shared_ptr<TrackedOp> TrackedOpRef;
+typedef boost::intrusive_ptr<TrackedOp> TrackedOpRef;
 
 class OpHistory {
   set<pair<utime_t, TrackedOpRef> > arrived;
@@ -54,13 +54,6 @@ public:
 
 struct ShardedTrackingData;
 class OpTracker {
-  class RemoveOnDelete {
-    OpTracker *tracker;
-  public:
-    explicit RemoveOnDelete(OpTracker *tracker) : tracker(tracker) {}
-    void operator()(TrackedOp *op);
-  };
-  friend class RemoveOnDelete;
   friend class OpHistory;
   atomic64_t seq;
   vector<ShardedTrackingData*> sharded_in_flight_list;
@@ -114,8 +107,7 @@ public:
   template <typename T, typename U>
   typename T::Ref create_request(U params)
   {
-    typename T::Ref retval(new T(params, this),
-                          RemoveOnDelete(this));
+    typename T::Ref retval(new T(params, this));
     retval->tracking_start();
     return retval;
   }
@@ -127,7 +119,8 @@ private:
   friend class OpTracker;
   xlist<TrackedOp*>::item xitem;
 protected:
-  OpTracker *tracker; /// the tracker we are associated with
+  OpTracker *tracker;          ///< the tracker we are associated with
+  std::atomic_int nref = {0};  ///< ref count
 
   utime_t initiated_at;
   list<pair<utime_t, string> > events; /// list of events and their times
@@ -136,16 +129,21 @@ protected:
   uint64_t seq; /// a unique value set by the OpTracker
 
   uint32_t warn_interval_multiplier; // limits output of a given op warning
-  // Transitions from false -> true without locks being held
-  atomic<bool> is_tracked; //whether in tracker and out of constructor
+
+  enum {
+    STATE_UNTRACKED = 0,
+    STATE_LIVE,
+    STATE_HISTORY
+  };
+  atomic<int> state = {STATE_UNTRACKED};
+
   TrackedOp(OpTracker *_tracker, const utime_t& initiated) :
     xitem(this),
     tracker(_tracker),
     initiated_at(initiated),
     lock("TrackedOp::lock"),
     seq(0),
-    warn_interval_multiplier(1),
-    is_tracked(false)
+    warn_interval_multiplier(1)
   { }
 
   /// output any type-specific data you want to get when dump() is called
@@ -181,7 +179,35 @@ public:
   void tracking_start() {
     if (tracker->register_inflight_op(&xitem)) {
       events.push_back(make_pair(initiated_at, "initiated"));
-      is_tracked = true;
+      state = STATE_LIVE;
+    }
+  }
+
+  // ref counting via intrusive_ptr, with special behavior on final
+  // put for historical op tracking
+  friend void intrusive_ptr_add_ref(TrackedOp *o) {
+    ++o->nref;
+  }
+  friend void intrusive_ptr_release(TrackedOp *o) {
+    if (--o->nref == 0) {
+      switch (o->state.load()) {
+      case STATE_UNTRACKED:
+       o->_unregistered();
+       delete o;
+       break;
+
+      case STATE_LIVE:
+       o->mark_event("done");
+       o->tracker->unregister_inflight_op(o);
+       break;
+
+      case STATE_HISTORY:
+       delete o;
+       break;
+
+      default:
+       ceph_abort();
+      }
     }
   }
 };

diff --git a/src/mds/CInode.h b/src/mds/CInode.h
index 9ce9a6a13c4a964d31d5e216553b08d06c2d360e..b8bd8206fd17735fb3292aca8a83abe7546c5e29 100644 (file)
--- a/src/mds/CInode.h
+++ b/src/mds/CInode.h
@@ -32,6 +32,7 @@
 #include "LocalLock.h"
 #include "Capability.h"
 #include "SnapRealm.h"
+#include "Mutation.h"
 
 #include <list>
 #include <set>
@@ -51,8 +52,6 @@ class Session;
 class MClientCaps;
 struct ObjectOperation;
 class EMetaBlob;
-struct MDRequestImpl;
-typedef ceph::shared_ptr<MDRequestImpl> MDRequestRef;
 
 
 ostream& operator<<(ostream& out, const CInode& in);

diff --git a/src/mds/Locker.h b/src/mds/Locker.h
index 7c3fcd9b277f06cff30feb57ff7e10360ac77083..c34c7f827096bd47f2880a1a4550c14ba6f0ab56 100644 (file)
--- a/src/mds/Locker.h
+++ b/src/mds/Locker.h
@@ -40,10 +40,8 @@ class SimpleLock;
 class ScatterLock;
 class LocalLock;
 
-class MDCache;
-typedef ceph::shared_ptr<MDRequestImpl> MDRequestRef;
-
 #include "SimpleLock.h"
+#include "Mutation.h"
 
 class Locker {
 private:

diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
index 6717adb9ed12e54111bfb4789eb9ea0d36c573c8..2cd1f78d058eff996dd2890ad75f274924f80477 100644 (file)
--- a/src/mds/MDCache.cc
+++ b/src/mds/MDCache.cc
@@ -2213,8 +2213,7 @@ void MDCache::predirty_journal_parents(MutationRef mut, EMetaBlob *blob,
     }
 
     // can cast only because i'm passing nowait=true in the sole user
-    MDRequestRef mdmut =
-      ceph::static_pointer_cast<MDRequestImpl,MutationImpl>(mut);
+    MDRequestRef mdmut = static_cast<MDRequestImpl*>(mut.get());
     if (!stop &&
        mut->wrlocks.count(&pin->nestlock) == 0 &&
        (!pin->versionlock.can_wrlock() ||                   // make sure we can take versionlock, too

diff --git a/src/mds/MDCache.h b/src/mds/MDCache.h
index a9c41193478a79f48c2aeb528d9f8788445333cf..bd00ffcfb76b5eb5b122c2afcbab5d177aa2b7a4 100644 (file)
--- a/src/mds/MDCache.h
+++ b/src/mds/MDCache.h
@@ -31,6 +31,7 @@
 #include "StrayManager.h"
 #include "MDSContext.h"
 #include "MDSMap.h"
+#include "Mutation.h"
 
 #include "messages/MClientRequest.h"
 #include "messages/MMDSSlaveRequest.h"
@@ -68,10 +69,6 @@ class MMDSFragmentNotify;
 
 class ESubtreeMap;
 
-struct MDRequestImpl;
-typedef ceph::shared_ptr<MDRequestImpl> MDRequestRef;
-struct MDSlaveUpdate;
-
 enum {
   l_mdc_first = 3000,
   // How many inodes currently in stray dentries

diff --git a/src/mds/Migrator.cc b/src/mds/Migrator.cc
index e57bffdec08a41d425b9d178269d68b0f39bded3..1b50c42adc0caea29ddd383476a3baad7d43f9bc 100644 (file)
--- a/src/mds/Migrator.cc
+++ b/src/mds/Migrator.cc
@@ -358,7 +358,7 @@ void Migrator::export_try_cancel(CDir *dir, bool notify_peer)
 
     // drop locks
     if (state == EXPORT_LOCKING || state == EXPORT_DISCOVERING) {
-      MDRequestRef mdr = ceph::static_pointer_cast<MDRequestImpl, MutationImpl>(mut);
+      MDRequestRef mdr = static_cast<MDRequestImpl*>(mut.get());
       assert(mdr);
       if (mdr->more()->waiting_on_slave.empty())
        mds->mdcache->request_finish(mdr);
@@ -876,8 +876,7 @@ void Migrator::handle_export_discover_ack(MExportDirDiscoverAck *m)
   } else {
     assert(it->second.state == EXPORT_DISCOVERING);
     // release locks to avoid deadlock
-    MDRequestRef mdr = ceph::static_pointer_cast<MDRequestImpl,
-                                                MutationImpl>(it->second.mut);
+    MDRequestRef mdr = static_cast<MDRequestImpl*>(it->second.mut.get());
     assert(mdr);
     mds->mdcache->request_finish(mdr);
     it->second.mut.reset();

diff --git a/src/mds/Mutation.h b/src/mds/Mutation.h
index dcb2ed40dce58071af68ab3e1d499d58e33ee9aa..8711d9f50a9bf944794f779a749bf2774a555542 100644 (file)
--- a/src/mds/Mutation.h
+++ b/src/mds/Mutation.h
@@ -164,7 +164,7 @@ inline ostream& operator<<(ostream &out, const MutationImpl &mut)
   return out;
 }
 
-typedef ceph::shared_ptr<MutationImpl> MutationRef;
+typedef boost::intrusive_ptr<MutationImpl> MutationRef;
 
 
 
@@ -334,13 +334,13 @@ struct MDRequestImpl : public MutationImpl {
   void dump(Formatter *f) const override;
 
   // TrackedOp stuff
-  typedef ceph::shared_ptr<MDRequestImpl> Ref;
+  typedef boost::intrusive_ptr<MDRequestImpl> Ref;
 protected:
   void _dump(Formatter *f) const;
   void _dump_op_descriptor_unlocked(ostream& stream) const;
 };
 
-typedef ceph::shared_ptr<MDRequestImpl> MDRequestRef;
+typedef boost::intrusive_ptr<MDRequestImpl> MDRequestRef;
 
 
 struct MDSlaveUpdate {

diff --git a/src/mds/Server.h b/src/mds/Server.h
index 8cea19cecebe08ce4e737bfd68a7cb6331d164eb..bf6e05bae00bc5f47efd81d9dd176f1361af2437 100644 (file)
--- a/src/mds/Server.h
+++ b/src/mds/Server.h
@@ -16,6 +16,7 @@
 #define CEPH_MDS_SERVER_H
 
 #include "MDSRank.h"
+#include "Mutation.h"
 
 class OSDMap;
 class PerfCounters;
@@ -28,11 +29,6 @@ class MClientRequest;
 class MClientReply;
 class MDLog;
 
-struct MutationImpl;
-struct MDRequestImpl;
-typedef ceph::shared_ptr<MutationImpl> MutationRef;
-typedef ceph::shared_ptr<MDRequestImpl> MDRequestRef;
-
 enum {
   l_mdss_first = 1000,
   l_mdss_handle_client_request,

diff --git a/src/mds/SimpleLock.h b/src/mds/SimpleLock.h
index e5527bfea53a25ee9afc39d226bee7f0d5500265..27eae7bebd4a0937b4daad52406eb2a608106678 100644 (file)
--- a/src/mds/SimpleLock.h
+++ b/src/mds/SimpleLock.h
@@ -16,6 +16,8 @@
 #ifndef CEPH_SIMPLELOCK_H
 #define CEPH_SIMPLELOCK_H
 
+#include <boost/intrusive_ptr.hpp>
+
 #include "MDSCacheObject.h"
 #include "MDSContext.h"
 
@@ -42,8 +44,9 @@ inline const char *get_lock_type_name(int t) {
 }
 
 #include "include/memory.h"
+
 struct MutationImpl;
-typedef ceph::shared_ptr<MutationImpl> MutationRef;
+typedef boost::intrusive_ptr<MutationImpl> MutationRef;
 
 extern "C" {
 #include "locks.h"

diff --git a/src/mon/MonOpRequest.h b/src/mon/MonOpRequest.h
index f34d477350738d093745f0ddb68624fcef7652b1..2080b606e5690916c102d9e9676186df18da9e08 100644 (file)
--- a/src/mon/MonOpRequest.h
+++ b/src/mon/MonOpRequest.h
@@ -178,7 +178,7 @@ public:
     return (con && con->get_peer_type() & CEPH_ENTITY_TYPE_MON);
   }
 
-  typedef ceph::shared_ptr<MonOpRequest> Ref;
+  typedef boost::intrusive_ptr<MonOpRequest> Ref;
 
   void set_op_type(op_type_t t) {
     op_type = t;

diff --git a/src/osd/CMakeLists.txt b/src/osd/CMakeLists.txt
index c817e724980e44fe3a410453e34591451051dc59..681d9fd36edd726d4b750c69b96a9a8092e264ec 100644 (file)
--- a/src/osd/CMakeLists.txt
+++ b/src/osd/CMakeLists.txt
@@ -6,7 +6,6 @@ set(osd_srcs
   OSD.cc
   Watch.cc
   ClassHandler.cc
-  OpRequest.cc
   PG.cc
   PGLog.cc
   PrimaryLogPG.cc
@@ -36,7 +35,7 @@ add_library(osd STATIC ${osd_srcs}
   $<TARGET_OBJECTS:common_util_obj>)
 target_link_libraries(osd ${LEVELDB_LIBRARIES} ${CMAKE_DL_LIBS} ${ALLOC_LIBS})
 if(WITH_LTTNG)
-  add_dependencies(osd oprequest-tp osd-tp pg-tp)
+  add_dependencies(osd osd-tp pg-tp)
 endif()
 if(WITH_LTTNG AND WITH_EVENTTRACE)
   add_dependencies(osd eventtrace_tp)

diff --git a/src/osd/OpRequest.h b/src/osd/OpRequest.h
index d26e3972cbc5c5deeb86b4c9f8303a9f3e2c7784..d92042e50b1001c29c72a20681fcd639f7467a94 100644 (file)
--- a/src/osd/OpRequest.h
+++ b/src/osd/OpRequest.h
@@ -193,7 +193,7 @@ public:
     return reqid;
   }
 
-  typedef ceph::shared_ptr<OpRequest> Ref;
+  typedef boost::intrusive_ptr<OpRequest> Ref;
 
 private:
   void set_rmw_flags(int flags);