common: MemoryModel: do not issue error messages directly

leave error reporting to the caller.
This change achieves two targets:
- no need for a context when creating the MemoryModel object; and
- (more importantly) the error messages are now reported in the context
  of the caller, and can be made to appear only once in the logs.

The existing client of the MemoryModel is modified to reflect the
changes to its API.

Signed-off-by: Ronen Friedman <[email protected]>

Author: ronen-fr

src/common/MemoryModel.cc

@@ -17,8 +17,6 @@
 using namespace std;
 using mem_snap_t = MemoryModel::mem_snap_t;
 
-MemoryModel::MemoryModel(CephContext *cct_) : cct(cct_) {}
-
 inline bool MemoryModel::cmp_against(
     const std::string &ln,
     std::string_view param,
@@ -41,14 +39,10 @@ inline bool MemoryModel::cmp_against(
 }
 
 
-std::optional<int64_t> MemoryModel::get_mapped_heap()
+tl::expected<int64_t, std::string> MemoryModel::get_mapped_heap()
 {
   if (!proc_maps.is_open()) {
-    ldout(cct, 0) << fmt::format(
-			 "MemoryModel::get_mapped_heap() unable to open {}",
-			 proc_maps_fn)
-		  << dendl;
-    return std::nullopt;
+    return tl::unexpected("unable to open proc/maps");
   }
   // always rewind before reading
   proc_maps.clear();
@@ -99,14 +93,10 @@ std::optional<int64_t> MemoryModel::get_mapped_heap()
 }
 
 
-std::optional<mem_snap_t> MemoryModel::full_sample()
+tl::expected<mem_snap_t, std::string> MemoryModel::full_sample()
 {
   if (!proc_status.is_open()) {
-    ldout(cct, 0) << fmt::format(
-			 "MemoryModel::sample() unable to open {}",
-			 proc_stat_fn)
-		  << dendl;
-    return std::nullopt;
+    return tl::unexpected("unable to open proc/status");
   }
   // always rewind before reading
   proc_status.clear();

src/common/MemoryModel.h

@@ -16,10 +16,11 @@
 #define CEPH_MEMORYMODEL_H
 
 #include <fstream>
-#include <optional>
+#include <string>
 #include <string_view>
 #include "include/common_fwd.h"
 #include "include/compat.h"
+#include "include/expected.hpp"
 
 
 class MemoryModel {
@@ -50,8 +51,14 @@ class MemoryModel {
   std::ifstream proc_status{proc_stat_fn};
   std::ifstream proc_maps{proc_maps_fn};
 
-  CephContext *cct;
-  std::optional<int64_t> get_mapped_heap();
+  /**
+   * @brief Get the mapped heap size
+   *
+   * Read /proc/self/maps to get the heap size.
+   * \retval the mapped heap size, or an error message if the file had not been opened
+   *    when the object was constructed.
+   */
+  tl::expected<int64_t, std::string> get_mapped_heap();
 
   /**
    * @brief Compare a line against an expected data label
@@ -62,8 +69,19 @@ class MemoryModel {
   bool cmp_against(const std::string& ln, std::string_view param, long& v) const;
 
 public:
-  explicit MemoryModel(CephContext *cct);
-  std::optional<mem_snap_t> full_sample();
+  /**
+   * @brief extract memory usage information from /proc/self/status &
+   *        /proc/self/maps
+   *
+   * Read /proc/self/status and /proc/self/maps to get memory usage information.
+   * \retval a structure containing the memory usage information, or an error
+   *    message if /proc/status had not been opened when the object was
+   *    constructed.
+   *    Note that no error is returned if only /proc/maps is not open (the heap
+   *    size will be reported as 0).
+   */
+  tl::expected<mem_snap_t, std::string> full_sample();
+
   void sample(mem_snap_t *p = nullptr);
 };
 

src/mds/MDCache.cc

@@ -7812,10 +7812,17 @@ void MDCache::trim_client_leases()
   }
 }
 
-void MDCache::check_memory_usage(const MemoryModel::mem_snap_t &baseline)
+void MDCache::check_memory_usage()
 {
-  MemoryModel mm(g_ceph_context);
-  mm.sample();
+  MemoryModel::mem_snap_t memory_snap;
+
+  if (upkeep_memory_stats) {
+    auto maybe_memory_snap = upkeep_memory_stats->full_sample();
+    if (maybe_memory_snap) {
+      memory_snap = *maybe_memory_snap;
+    }
+  }
+  // else - no access to memory stats. The problem was already reported at thread's init.
 
   // check client caps
   ceph_assert(CInode::count() == inode_map.size() + snap_inode_map.size() + num_shadow_inodes);
@@ -7824,17 +7831,17 @@ void MDCache::check_memory_usage(const MemoryModel::mem_snap_t &baseline)
     caps_per_inode = (double)Capability::count() / (double)CInode::count();
 
   dout(2) << "Memory usage: "
-	   << " total " << mm.last.get_total()
-	   << ", rss " << mm.last.get_rss()
-	   << ", heap " << mm.last.get_heap()
-	   << ", baseline " << baseline.get_heap()
+	   << " total " << memory_snap.get_total()
+	   << ", rss " << memory_snap.get_rss()
+	   << ", heap " << memory_snap.get_heap()
+	   << ", baseline " << upkeep_mem_baseline.get_heap()
 	   << ", " << num_inodes_with_caps << " / " << CInode::count() << " inodes have caps"
 	   << ", " << Capability::count() << " caps, " << caps_per_inode << " caps per inode"
 	   << dendl;
 
   mds->update_mlogger();
-  mds->mlogger->set(l_mdm_rss, mm.last.get_rss());
-  mds->mlogger->set(l_mdm_heap, mm.last.get_heap());
+  mds->mlogger->set(l_mdm_rss, memory_snap.get_rss());
+  mds->mlogger->set(l_mdm_heap, memory_snap.get_heap());
 }
 
 
@@ -14171,11 +14178,24 @@ void MDCache::upkeep_main(void)
 {
   std::unique_lock lock(upkeep_mutex);
 
+  // create a "memory model" for the upkeep thread. The object maintains
+  // the relevant '/proc' files open. We only check for a failure to open
+  // the files once, and then we'll just keep the object around.
+  upkeep_memory_stats = MemoryModel{};
+
   // get initial sample upon thread creation
-  MemoryModel::mem_snap_t baseline;
   {
-    MemoryModel mm(g_ceph_context);
-    mm.sample(&baseline);
+    auto maybe_base = upkeep_memory_stats->full_sample();
+    if (!maybe_base) {
+      dout(1) << fmt::format(
+		     "{}: Failed to get initial memory sample ({}). No more "
+		     "sampling will be attempted",
+		     __func__, maybe_base.error())
+	      << dendl;
+      upkeep_memory_stats = std::nullopt;
+    } else {
+      upkeep_mem_baseline = *maybe_base;
+    }
   }
 
   while (!upkeep_trim_shutdown.load()) {
@@ -14188,7 +14208,7 @@ void MDCache::upkeep_main(void)
       lock.lock();
       if (upkeep_trim_shutdown.load())
         return;
-      check_memory_usage(baseline);
+      check_memory_usage();
       if (mds->is_cache_trimmable()) {
         dout(20) << "upkeep thread trimming cache; last trim " << since << " ago" << dendl;
         bool active_with_clients = mds->is_active() || mds->is_clientreplay() || mds->is_stopping();

src/mds/MDCache.h

@@ -776,7 +776,7 @@ class MDCache {
   bool expire_recursive(CInode *in, expiremap& expiremap);
 
   void trim_client_leases();
-  void check_memory_usage(const MemoryModel::mem_snap_t &baseline);
+  void check_memory_usage();
 
   void shutdown_start();
   void shutdown_check();
@@ -1540,6 +1540,8 @@ class MDCache {
   time upkeep_last_trim = clock::zero();
   time upkeep_last_release = clock::zero();
   std::atomic<bool> upkeep_trim_shutdown{false};
+  std::optional<MemoryModel> upkeep_memory_stats;
+  MemoryModel::mem_snap_t upkeep_mem_baseline;
 
   uint64_t kill_shutdown_at = 0;