rgw: cleanups for sync tracing

RGWSyncTraceManager::add_node() now allocates the nodes themselves. this
simplifies some of the interactions between the Manager and Node
classes, and saves a lot of typing for the callers

Signed-off-by: Casey Bodley <cbodley@redhat.com>

diff --git a/src/rgw/rgw_data_sync.cc b/src/rgw/rgw_data_sync.cc
index ca98c0c886535d7cc2ba81aa6560ea35f0c6f5f8..81756d770c2d8f355337766e7f4abeacddd2b598 100644 (file)
--- a/src/rgw/rgw_data_sync.cc
+++ b/src/rgw/rgw_data_sync.cc
@@ -519,7 +519,8 @@ public:
                                  rgw_data_sync_status *status)
     : RGWCoroutine(_sync_env->cct), sync_env(_sync_env), store(sync_env->store),
       pool(store->get_zone_params().log_pool),
-      num_shards(num_shards), status(status) {
+      num_shards(num_shards), status(status),
+      tn(sync_env->sync_tracer->add_node(_tn_parent, "init_data_sync_status")) {
     lock_name = "sync_lock";
 
     status->sync_info.instance_id = instance_id;
@@ -532,11 +533,6 @@ public:
 
     sync_status_oid = RGWDataSyncStatusManager::sync_status_oid(sync_env->source_zone);
 
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                         sync_env->sync_tracer, 
-                                         _tn_parent,
-                                         "init_data_sync_status",
-                                         string()));
   }
 
   int operate() override {
@@ -677,11 +673,7 @@ int RGWRemoteDataLog::init(const string& _source_zone, RGWRESTConn *_conn, RGWSy
     return ret;
   }
 
-  tn = sync_env.sync_tracer->add_node(new RGWSyncTraceNode(sync_env.cct,
-                                                           sync_env.sync_tracer, 
-                                                           sync_env.sync_tracer->root_node,
-                                                           "data",
-                                                           string()));
+  tn = sync_env.sync_tracer->add_node(sync_env.sync_tracer->root_node, "data");
 
   initialized = true;
 
@@ -1024,14 +1016,10 @@ class RGWRunBucketSyncCoroutine : public RGWCoroutine {
 public:
   RGWRunBucketSyncCoroutine(RGWDataSyncEnv *_sync_env, const rgw_bucket_shard& bs, const RGWSyncTraceNodeRef& _tn_parent)
     : RGWCoroutine(_sync_env->cct), sync_env(_sync_env), bs(bs),
-      status_oid(RGWBucketSyncStatusManager::status_oid(sync_env->source_zone, bs)) {
+      status_oid(RGWBucketSyncStatusManager::status_oid(sync_env->source_zone, bs)),
+      tn(sync_env->sync_tracer->add_node(_tn_parent, "bucket",
+                                         SSTR(bucket_shard_str{bs}))) {
     logger.init(sync_env, "Bucket", bs.get_key());
-    string bss = SSTR(bucket_shard_str{bs});
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                                              sync_env->sync_tracer, 
-                                                              _tn_parent,
-                                                              "bucket",
-                                                              bss));
   }
   ~RGWRunBucketSyncCoroutine() override {
     if (lease_cr) {
@@ -1072,11 +1060,7 @@ public:
                                                       marker_tracker(_marker_tracker),
                                                       error_repo(_error_repo), remove_from_repo(_remove_from_repo) {
     set_description() << "data sync single entry (source_zone=" << sync_env->source_zone << ") key=" <<_raw_key << " entry=" << entry_marker;
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                                              sync_env->sync_tracer, 
-                                                              _tn_parent,
-                                                              "entry",
-                                                              raw_key));
+    tn = sync_env->sync_tracer->add_node(_tn_parent, "entry", raw_key);
   }
 
   int operate() override {
@@ -1525,11 +1509,7 @@ public:
                                                      pool(_pool),
                                                      shard_id(_shard_id),
                                                      sync_marker(_marker) {
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                         sync_env->sync_tracer, 
-                                         _tn_parent,
-                                         "shard",
-                                         SSTR(shard_id)));
+    tn = sync_env->sync_tracer->add_node(_tn_parent, "shard", std::to_string(shard_id));
   }
 
   RGWCoroutine *alloc_cr() override {
@@ -1746,11 +1726,7 @@ public:
   RGWDataSyncControlCR(RGWDataSyncEnv *_sync_env, uint32_t _num_shards,
                        RGWSyncTraceNodeRef& _tn_parent) : RGWBackoffControlCR(_sync_env->cct, exit_on_error),
                                                           sync_env(_sync_env), num_shards(_num_shards) {
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                         sync_env->sync_tracer, 
-                                         _tn_parent,
-                                         "sync",
-                                         string()));
+    tn = sync_env->sync_tracer->add_node(_tn_parent, "sync");
   }
 
   RGWCoroutine *alloc_cr() override {
@@ -2586,11 +2562,7 @@ public:
     set_status("init");
 
     logger.init(sync_env, "Object", ss.str());
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                                              sync_env->sync_tracer, 
-                                                              _tn_parent,
-                                                              "entry",
-                                                              SSTR(key)));
+    tn = sync_env->sync_tracer->add_node(_tn_parent, "entry", SSTR(key));
 
     tn->log(20, SSTR("bucket sync single entry (source_zone=" << sync_env->source_zone << ") b=" << ss.str() << " log_entry=" << entry_marker << " op=" << (int)op << " op_state=" << (int)op_state));
     error_injection = (sync_env->cct->_conf->rgw_sync_data_inject_err_probability > 0);
@@ -2717,14 +2689,11 @@ public:
     : RGWCoroutine(_sync_env->cct), sync_env(_sync_env), bs(bs),
       bucket_info(_bucket_info), lease_cr(lease_cr), sync_info(sync_info),
       marker_tracker(sync_env, status_oid, sync_info.full_marker),
-      status_oid(status_oid) {
+      status_oid(status_oid),
+      tn(sync_env->sync_tracer->add_node(tn_parent, "full_sync",
+                                         SSTR(bucket_shard_str{bs}))) {
     logger.init(sync_env, "BucketFull", bs.get_key());
     zones_trace.insert(sync_env->source_zone);
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                                              sync_env->sync_tracer, 
-                                                              tn_parent,
-                                                              "full_sync",
-                                                              SSTR(bucket_shard_str{bs})));
     marker_tracker.set_tn(tn);
   }
 
@@ -2876,17 +2845,14 @@ public:
     : RGWCoroutine(_sync_env->cct), sync_env(_sync_env), bs(bs),
       bucket_info(_bucket_info), lease_cr(lease_cr), sync_info(sync_info),
       marker_tracker(sync_env, status_oid, sync_info.inc_marker),
-      status_oid(status_oid), zone_id(_sync_env->store->get_zone().id)
+      status_oid(status_oid), zone_id(_sync_env->store->get_zone().id),
+      tn(sync_env->sync_tracer->add_node(_tn_parent, "inc_sync",
+                                         SSTR(bucket_shard_str{bs})))
   {
     set_description() << "bucket shard incremental sync bucket="
         << bucket_shard_str{bs};
     set_status("init");
     logger.init(sync_env, "BucketInc", bs.get_key());
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                                              sync_env->sync_tracer, 
-                                                              _tn_parent,
-                                                              "inc_sync",
-                                                              SSTR(bucket_shard_str{bs})));
     marker_tracker.set_tn(tn);
   }
 

diff --git a/src/rgw/rgw_sync.cc b/src/rgw/rgw_sync.cc
index 70f9328399db5ccf84a106a950cef2d0a9139f6d..5f03da96a141d78efe63a477cb3eb703d8f80f14 100644 (file)
--- a/src/rgw/rgw_sync.cc
+++ b/src/rgw/rgw_sync.cc
@@ -287,11 +287,7 @@ int RGWRemoteMetaLog::init()
 
   init_sync_env(&sync_env);
 
-  tn = sync_env.sync_tracer->add_node(new RGWSyncTraceNode(sync_env.cct,
-                                                           sync_env.sync_tracer, 
-                                                           sync_env.sync_tracer->root_node,
-                                                           "meta",
-                                                           string()));
+  tn = sync_env.sync_tracer->add_node(sync_env.sync_tracer->root_node, "meta");
 
   return 0;
 }
@@ -819,11 +815,7 @@ public:
                                                      num_shards(_num_shards),
                                                      ret_status(0), lease_cr(nullptr), lease_stack(nullptr),
                                                       lost_lock(false), failed(false), markers(_markers) {
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                         sync_env->sync_tracer, 
-                                         _tn_parent,
-                                         "fetch_all_meta",
-                                         string()));
+    tn = sync_env->sync_tracer->add_node(_tn_parent, "fetch_all_meta");
   }
 
   ~RGWFetchAllMetaCR() override {
@@ -1013,11 +1005,8 @@ public:
                                                       section(_section),
                                                       key(_key),
                                                      pbl(_pbl) {
-    tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                         sync_env->sync_tracer, 
-                                         _tn_parent,
-                                         "read_remote_meta",
-                                         section + ":" + key));
+    tn = sync_env->sync_tracer->add_node(_tn_parent, "read_remote_meta",
+                                         section + ":" + key);
   }
 
   int operate() override {
@@ -1231,11 +1220,7 @@ RGWMetaSyncSingleEntryCR::RGWMetaSyncSingleEntryCR(RGWMetaSyncEnv *_sync_env,
                                                       pos(0), sync_status(0),
                                                       marker_tracker(_marker_tracker), tries(0) {
   error_injection = (sync_env->cct->_conf->rgw_sync_meta_inject_err_probability > 0);
-  tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                                            sync_env->sync_tracer, 
-                                                            _tn_parent,
-                                                            "entry",
-                                                            raw_key));
+  tn = sync_env->sync_tracer->add_node(_tn_parent, "entry", raw_key);
 }
 
 int RGWMetaSyncSingleEntryCR::operate() {
@@ -1844,7 +1829,6 @@ class RGWMetaSyncShardControlCR : public RGWBackoffControlCR
   rgw_meta_sync_marker sync_marker;
   const std::string period_marker;
 
-  RGWSyncTraceNodeRef tn_parent;
   RGWSyncTraceNodeRef tn;
 
   static constexpr bool exit_on_error = false; // retry on all errors
@@ -1858,14 +1842,10 @@ public:
     : RGWBackoffControlCR(_sync_env->cct, exit_on_error), sync_env(_sync_env),
       pool(_pool), period(period), realm_epoch(realm_epoch), mdlog(mdlog),
       shard_id(_shard_id), sync_marker(_marker),
-      period_marker(std::move(period_marker)),
-      tn_parent(_tn_parent) {
-        tn = sync_env->sync_tracer->add_node(new RGWSyncTraceNode(sync_env->cct,
-                                                                  sync_env->sync_tracer, 
-                                                                  tn_parent,
-                                                                  "shard",
-                                                                  SSTR(shard_id)));
-      }
+      period_marker(std::move(period_marker)) {
+    tn = sync_env->sync_tracer->add_node(_tn_parent, "shard",
+                                         std::to_string(shard_id));
+  }
 
   RGWCoroutine *alloc_cr() override {
     return new RGWMetaSyncShardCR(sync_env, pool, period, realm_epoch, mdlog,

diff --git a/src/rgw/rgw_sync_trace.cc b/src/rgw/rgw_sync_trace.cc
index 8b3dd8cd802b1066f148cc4fc12c47ca3ca701f7..1566ff4c8b03e9a391b00cd830aa78fc8b723cfe 100644 (file)
--- a/src/rgw/rgw_sync_trace.cc
+++ b/src/rgw/rgw_sync_trace.cc
@@ -13,13 +13,14 @@
 #define dout_context g_ceph_context
 #define dout_subsys ceph_subsys_rgw_sync
 
-RGWSyncTraceNode::RGWSyncTraceNode(CephContext *_cct, RGWSyncTraceManager *_manager,
+RGWSyncTraceNode::RGWSyncTraceNode(CephContext *_cct, uint64_t _handle,
                                    const RGWSyncTraceNodeRef& _parent,
                                    const string& _type, const string& _id) : cct(_cct),
-                                                                             manager(_manager),
                                                                              parent(_parent),
                                                                              type(_type),
-                                                                             id(_id), history(cct->_conf->rgw_sync_trace_per_node_log_size)
+                                                                             id(_id),
+                                                                             handle(_handle),
+                                                                             history(cct->_conf->rgw_sync_trace_per_node_log_size)
 {
   if (parent.get()) {
     prefix = parent->get_prefix();
@@ -32,7 +33,6 @@ RGWSyncTraceNode::RGWSyncTraceNode(CephContext *_cct, RGWSyncTraceManager *_mana
     }
     prefix += ":";
   }
-  handle = manager->alloc_handle();
 }
 
 void RGWSyncTraceNode::log(int level, const string& s)
@@ -49,11 +49,6 @@ void RGWSyncTraceNode::log(int level, const string& s)
   }
 }
 
-void RGWSyncTraceNode::finish()
-{
-  manager->finish_node(this);
-}
-
 
 class RGWSyncTraceServiceMapThread : public RGWRadosThread {
   RGWRados *store;
@@ -80,15 +75,18 @@ int RGWSyncTraceServiceMapThread::process()
   return 0;
 }
 
-RGWSyncTraceNodeRef RGWSyncTraceManager::add_node(RGWSyncTraceNode *node)
+RGWSyncTraceNodeRef RGWSyncTraceManager::add_node(const RGWSyncTraceNodeRef& parent,
+                                                  const std::string& type,
+                                                  const std::string& id)
 {
   shunique_lock wl(lock, ceph::acquire_unique);
-  RGWSyncTraceNodeRef& ref = nodes[node->handle];
-  ref.reset(node);
+  auto handle = alloc_handle();
+  RGWSyncTraceNodeRef& ref = nodes[handle];
+  ref.reset(new RGWSyncTraceNode(cct, handle, parent, type, id));
   // return a separate shared_ptr that calls finish() on the node instead of
   // deleting it. the lambda capture holds a reference to the original 'ref'
-  auto deleter = [ref] (RGWSyncTraceNode *node) { node->finish(); };
-  return {node, deleter};
+  auto deleter = [ref, this] (RGWSyncTraceNode *node) { finish_node(node); };
+  return {ref.get(), deleter};
 }
 
 bool RGWSyncTraceNode::match(const string& search_term, bool search_history)

diff --git a/src/rgw/rgw_sync_trace.h b/src/rgw/rgw_sync_trace.h
index 8fe9a8a6f837f427980ed35d4420fcf82a53657b..2a372f3a34fcd93e01745ba6c9bc7fc1d99e3ea6 100644 (file)
--- a/src/rgw/rgw_sync_trace.h
+++ b/src/rgw/rgw_sync_trace.h
@@ -31,12 +31,10 @@ class RGWSyncTraceServiceMapThread;
 
 using RGWSyncTraceNodeRef = std::shared_ptr<RGWSyncTraceNode>;
 
-class RGWSyncTraceNode {
+class RGWSyncTraceNode final {
   friend class RGWSyncTraceManager;
 
   CephContext *cct;
-
-  RGWSyncTraceManager *manager{nullptr};
   RGWSyncTraceNodeRef parent;
 
   uint16_t state{0};
@@ -44,7 +42,6 @@ class RGWSyncTraceNode {
 
   Mutex lock{"RGWSyncTraceNode::lock"};
 
-protected:
   std::string type;
   std::string id;
 
@@ -55,10 +52,13 @@ protected:
   uint64_t handle;
 
   boost::circular_buffer<string> history;
-public:
-  RGWSyncTraceNode(CephContext *_cct, RGWSyncTraceManager *_manager, const RGWSyncTraceNodeRef& _parent,
-           const std::string& _type, const std::string& _id);
 
+  // private constructor, create with RGWSyncTraceManager::add_node()
+  RGWSyncTraceNode(CephContext *_cct, uint64_t _handle,
+                   const RGWSyncTraceNodeRef& _parent,
+                   const std::string& _type, const std::string& _id);
+
+ public:
   void set_resource_name(const string& s) {
     resource_name = s;
   }
@@ -77,7 +77,6 @@ public:
     return (state & f) == f;
   }
   void log(int level, const std::string& s);
-  void finish();
 
   std::string to_str() {
     return prefix + " " + status;
@@ -114,10 +113,11 @@ class RGWSyncTraceManager : public AdminSocketHook {
   std::atomic<uint64_t> count = { 0 };
 
   std::list<std::array<string, 3> > admin_commands;
-protected:
+
   uint64_t alloc_handle() {
     return ++count;
   }
+  void finish_node(RGWSyncTraceNode *node);
 
 public:
   RGWSyncTraceManager(CephContext *_cct, int max_lru) : cct(_cct), complete_nodes(max_lru) {}
@@ -127,8 +127,9 @@ public:
 
   const RGWSyncTraceNodeRef root_node;
 
-  RGWSyncTraceNodeRef add_node(RGWSyncTraceNode *node);
-  void finish_node(RGWSyncTraceNode *node);
+  RGWSyncTraceNodeRef add_node(const RGWSyncTraceNodeRef& parent,
+                               const std::string& type,
+                               const std::string& id = "");
 
   int hook_to_admin_command();
   bool call(std::string_view command, const cmdmap_t& cmdmap,