]> git.apps.os.sepia.ceph.com Git - ceph-ci.git/commitdiff
common/TrackedOp: fix race updating description with proper lock
authorPatrick Donnelly <pdonnell@redhat.com>
Wed, 26 Jul 2023 16:55:07 +0000 (12:55 -0400)
committerPatrick Donnelly <pdonnell@redhat.com>
Tue, 8 Aug 2023 12:58:40 +0000 (08:58 -0400)
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>
src/common/TrackedOp.h
src/mds/Mutation.cc
src/mds/Mutation.h
src/mon/MonOpRequest.h
src/osd/OpRequest.cc
src/osd/OpRequest.h

index 31afba1853e3af3691d0247608f8e90cc231dc6f..49ddf019bdd0b4c77359c939b233c47e3fb51d17 100644 (file)
@@ -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;
index 39eee47217baefdd57f6333fca9ced9b10ee6e84..6a244b154c94ee96944c8db680d121f4ae17234e 100644 (file)
@@ -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;
index 092651a7c1f76cf8ebb999107291a6b46bf1217a..3bfff9691205b9d106f9a47491e47b12f131cd39 100644 (file)
@@ -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;
 };
index 73275e81eb1456ddb605606f11cfd23eb5820292..0c4379910af73353963937c27206e7794d19c2a1 100644 (file)
@@ -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);
   }
 
index 86ce351930d188f599738e19cbbb130aa8dbb286..ba77dc3ba281d5767c3f6db111d6c985d8d37937 100644 (file)
@@ -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);
 }
index 49ea1b5d1c973b7924456acab00fcba6271e0128..c1a69e08e7e84e6b5bd4055c65d4e8fe02658852 100644 (file)
@@ -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;