From: Patrick Donnelly <pdonnell@redhat.com>
Date: Wed, 26 Jul 2023 16:55:07 +0000 (-0400)
Subject: common/TrackedOp: fix race updating description with proper lock
X-Git-Tag: v19.0.0~692^2~10
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=36eaa495eb37485796ce57c8c4b1cca603efc193;p=ceph-ci.git

common/TrackedOp: fix race updating description with proper lock

It is not safe to "cache" a member which may be changed by a racing thread.

This reworks the locking so we can do a light-weight check if the description
is already generated wihtout acquiring the heavier TrackedOp::lock. If it's not
available yet or it needs regenerated, then get the proper locks to generate
it.

Fixes: e45f5c2c33508bec3b177e51fd6cc5fde1f0e674
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
---

diff --git a/src/common/TrackedOp.h b/src/common/TrackedOp.h
index 31afba1853e..49ddf019bdd 100644
--- a/src/common/TrackedOp.h
+++ b/src/common/TrackedOp.h
@@ -15,6 +15,7 @@
 #define TRACKEDREQUEST_H_
 
 #include <atomic>
+#include "common/StackStringStream.h"
 #include "common/ceph_mutex.h"
 #include "common/histogram.h"
 #include "common/Thread.h"
@@ -278,10 +279,6 @@ protected:
   };
   std::atomic<int> state = {STATE_UNTRACKED};
 
-  mutable std::string desc_str;   ///< protected by lock
-  mutable const char *desc = nullptr;  ///< readable without lock
-  mutable std::atomic<bool> want_new_desc = {false};
-
   TrackedOp(OpTracker *_tracker, const utime_t& initiated) :
     tracker(_tracker),
     initiated_at(initiated)
@@ -294,7 +291,7 @@ protected:
   /// if you want something else to happen when events are marked, implement
   virtual void _event_marked() {}
   /// return a unique descriptor of the Op; eg the message it's attached to
-  virtual void _dump_op_descriptor_unlocked(std::ostream& stream) const = 0;
+  virtual void _dump_op_descriptor(std::ostream& stream) const = 0;
   /// called when the last non-OpTracker reference is dropped
   virtual void _unregistered() {}
 
@@ -346,21 +343,32 @@ public:
     }
   }
 
-  const char *get_desc() const {
-    if (!desc || want_new_desc.load()) {
-      std::lock_guard l(lock);
-      _gen_desc();
+  std::string get_desc() const {
+    std::string ret;
+    {
+      std::lock_guard l(desc_lock);
+      ret = desc;
+    }
+    if (ret.size() == 0 || want_new_desc.load()) {
+      CachedStackStringStream css;
+      std::scoped_lock l(lock, desc_lock);
+      if (desc.size() && !want_new_desc.load()) {
+        return desc;
+      }
+      _dump_op_descriptor(*css);
+      desc = css->strv();
+      want_new_desc = false;
+      return desc;
+    } else {
+      return ret;
     }
-    return desc;
   }
+
 private:
-  void _gen_desc() const {
-    std::ostringstream ss;
-    _dump_op_descriptor_unlocked(ss);
-    desc_str = ss.str();
-    desc = desc_str.c_str();
-    want_new_desc = false;
-  }
+  mutable ceph::mutex desc_lock = ceph::make_mutex("OpTracker::desc_lock");
+  mutable std::string desc;   ///< protected by desc_lock
+  mutable std::atomic<bool> want_new_desc = {false};
+
 public:
   void reset_desc() {
     want_new_desc = true;
diff --git a/src/mds/Mutation.cc b/src/mds/Mutation.cc
index 39eee47217b..6a244b154c9 100644
--- a/src/mds/Mutation.cc
+++ b/src/mds/Mutation.cc
@@ -268,7 +268,7 @@ void MutationImpl::cleanup()
   drop_pins();
 }
 
-void MutationImpl::_dump_op_descriptor_unlocked(ostream& stream) const
+void MutationImpl::_dump_op_descriptor(ostream& stream) const
 {
   stream << "Mutation";
 }
@@ -537,7 +537,7 @@ void MDRequestImpl::_dump(Formatter *f) const
   }
 }
 
-void MDRequestImpl::_dump_op_descriptor_unlocked(ostream& stream) const
+void MDRequestImpl::_dump_op_descriptor(ostream& stream) const
 {
   msg_lock.lock();
   auto _client_request = client_request;
diff --git a/src/mds/Mutation.h b/src/mds/Mutation.h
index 092651a7c1f..3bfff969120 100644
--- a/src/mds/Mutation.h
+++ b/src/mds/Mutation.h
@@ -221,7 +221,7 @@ public:
   }
 
   virtual void dump(ceph::Formatter *f) const {}
-  void _dump_op_descriptor_unlocked(std::ostream& stream) const override;
+  void _dump_op_descriptor(std::ostream& stream) const override;
 
   metareqid_t reqid;
   __u32 attempt = 0;      // which attempt for this request
@@ -454,7 +454,7 @@ struct MDRequestImpl : public MutationImpl {
 
 protected:
   void _dump(ceph::Formatter *f) const override;
-  void _dump_op_descriptor_unlocked(std::ostream& stream) const override;
+  void _dump_op_descriptor(std::ostream& stream) const override;
 private:
   mutable ceph::spinlock msg_lock;
 };
diff --git a/src/mon/MonOpRequest.h b/src/mon/MonOpRequest.h
index 73275e81eb1..0c4379910af 100644
--- a/src/mon/MonOpRequest.h
+++ b/src/mon/MonOpRequest.h
@@ -131,7 +131,7 @@ private:
   }
 
 protected:
-  void _dump_op_descriptor_unlocked(std::ostream& stream) const override {
+  void _dump_op_descriptor(std::ostream& stream) const override {
     get_req()->print(stream);
   }
 
diff --git a/src/osd/OpRequest.cc b/src/osd/OpRequest.cc
index 86ce351930d..ba77dc3ba28 100644
--- a/src/osd/OpRequest.cc
+++ b/src/osd/OpRequest.cc
@@ -88,7 +88,7 @@ void OpRequest::_dump(Formatter *f) const
   }
 }
 
-void OpRequest::_dump_op_descriptor_unlocked(ostream& stream) const
+void OpRequest::_dump_op_descriptor(ostream& stream) const
 {
   get_req()->print(stream);
 }
diff --git a/src/osd/OpRequest.h b/src/osd/OpRequest.h
index 49ea1b5d1c9..c1a69e08e7e 100644
--- a/src/osd/OpRequest.h
+++ b/src/osd/OpRequest.h
@@ -80,7 +80,7 @@ private:
   OpRequest(Message *req, OpTracker *tracker);
 
 protected:
-  void _dump_op_descriptor_unlocked(std::ostream& stream) const override;
+  void _dump_op_descriptor(std::ostream& stream) const override;
   void _unregistered() override;
   bool filter_out(const std::set<std::string>& filters) override;