common/TrackedOp: Handle dump racing with constructor

Use is_tracked to prevent TrackedOp::dump() from trying to call
virtual function while still in OpRequest constructor.

Fixes: #8885
Signed-off-by: David Zafman <dzafman@redhat.com>
(cherry picked from commit ad13e05499669a79bde9a219ba1089f929e0388e)

Conflicts:
	src/common/TrackedOp.cc
	src/common/TrackedOp.h
Hammer can't use atomic<bool> so use atomic_t instead

diff --git a/src/common/TrackedOp.cc b/src/common/TrackedOp.cc
index c5d36dfe258966e09bbcd31b2ab78e1f62ab4426..8692b368754dc829a44072f42a5fb26533cf2efb 100644 (file)
--- a/src/common/TrackedOp.cc
+++ b/src/common/TrackedOp.cc
@@ -148,7 +148,7 @@ void OpTracker::register_inflight_op(xlist<TrackedOp*>::item *i)
 void OpTracker::unregister_inflight_op(TrackedOp *i)
 {
   // caller checks;
-  assert(tracking_enabled);
+  assert(i->is_tracked.read());
 
   uint32_t shard_index = i->seq % num_optracker_shards;
   ShardedTrackingData* sdata = sharded_in_flight_list[shard_index];
@@ -288,7 +288,7 @@ void OpTracker::get_age_ms_histogram(pow2_hist_t *h)
 
 void OpTracker::mark_event(TrackedOp *op, const string &dest, utime_t time)
 {
-  if (!tracking_enabled)
+  if (!op->is_tracked.read())
     return;
   return _mark_event(op, dest, time);
 }
@@ -306,7 +306,7 @@ void OpTracker::_mark_event(TrackedOp *op, const string &evt,
 }
 
 void OpTracker::RemoveOnDelete::operator()(TrackedOp *op) {
-  if (!tracker->tracking_enabled) {
+  if (!op->is_tracked.read()) {
     op->_unregistered();
     delete op;
     return;
@@ -318,7 +318,7 @@ void OpTracker::RemoveOnDelete::operator()(TrackedOp *op) {
 
 void TrackedOp::mark_event(const string &event)
 {
-  if (!tracker->tracking_enabled)
+  if (!is_tracked.read())
     return;
 
   utime_t now = ceph_clock_now(g_ceph_context);
@@ -332,6 +332,9 @@ 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.read())
+    return;
   stringstream name;
   _dump_op_descriptor_unlocked(name);
   f->dump_string("description", name.str().c_str()); // this TrackedOp

diff --git a/src/common/TrackedOp.h b/src/common/TrackedOp.h
index dc2452f28f41a45be0097e2cc1b8e8df5e4d65b4..b6445a0d48d7f05ec1a2feea6834bc628d50ea39 100644 (file)
--- a/src/common/TrackedOp.h
+++ b/src/common/TrackedOp.h
@@ -21,6 +21,7 @@
 #include "include/xlist.h"
 #include "msg/Message.h"
 #include "include/memory.h"
+#include "include/atomic.h"
 
 class TrackedOp;
 typedef ceph::shared_ptr<TrackedOp> TrackedOpRef;
@@ -132,6 +133,7 @@ public:
   {
     typename T::Ref retval(new T(params, this),
                           RemoveOnDelete(this));
+    retval->tracking_start();
     return retval;
   }
 };
@@ -151,7 +153,8 @@ 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_t is_tracked; //whether in tracker and out of constructor
   TrackedOp(OpTracker *_tracker, const utime_t& initiated) :
     xitem(this),
     tracker(_tracker),
@@ -159,11 +162,7 @@ protected:
     lock("TrackedOp::lock"),
     seq(0),
     warn_interval_multiplier(1)
-  {
-    tracker->register_inflight_op(&xitem);
-    if (tracker->tracking_enabled)
-      events.push_back(make_pair(initiated_at, "initiated"));
-  }
+  { }
 
   /// output any type-specific data you want to get when dump() is called
   virtual void _dump(utime_t now, Formatter *f) const {}
@@ -194,6 +193,14 @@ public:
     return events.rbegin()->second.c_str();
   }
   void dump(utime_t now, Formatter *f) const;
+  void tracking_start() {
+    RWLock::RLocker l(tracker->lock);
+    if (tracker->tracking_enabled) {
+      tracker->register_inflight_op(&xitem);
+      events.push_back(make_pair(initiated_at, "initiated"));
+      is_tracked.set(1);
+    }
+  }
 };
 
 #endif