mds: for logging generate only 10 final components of inode path

Generating full absolute path for inodes for printing in MDS logs slows
down the FS to a great extent especially when the path is very long
(imagine a path with 2000 components). Also printing such long paths in
MDS logs is not only pointless but also greatly reduces the readability
of the MDS logs.

Therefore, generate only 10 final components of inode paths for logging.

Fixes: https://tracker.ceph.com/issues/72779
Signed-off-by: Rishabh Dave <ridave@redhat.com>
(cherry picked from commit 1518690210f3a4473978c7a9274e902fccaad862)

Conflicts:
src/mds/CDir.cc
- Certain code region where trimmed inode path was generated was
  modified to generated inode path but that code region is absent on
  this branch.

diff --git a/src/mds/CDentry.cc b/src/mds/CDentry.cc
index b9a232798d88050ce68f9cd8edb3abdebb9efead..78fbaf72e1cef67251ee6b48a2580edada35f5ff 100644 (file)
--- a/src/mds/CDentry.cc
+++ b/src/mds/CDentry.cc
@@ -242,10 +242,11 @@ void CDentry::clear_auth()
   }
 }
 
-void CDentry::make_path_string(string& s, bool projected) const
+void CDentry::make_path_string(string& s, bool projected,
+                              int path_comp_count) const
 {
   if (dir) {
-    dir->inode->make_path_string(s, projected);
+    dir->inode->make_path_string(s, projected, NULL, path_comp_count);
   } else {
     s = "???";
   }

diff --git a/src/mds/CDentry.h b/src/mds/CDentry.h
index 2566395d185617bec6c4e0bbeb6719b2e6cdcdf0..99f4911ca5508327f80bbf32d457ee58ec3b59f0 100644 (file)
--- a/src/mds/CDentry.h
+++ b/src/mds/CDentry.h
@@ -239,7 +239,8 @@ public:
   const CDentry& operator= (const CDentry& right);
 
   // misc
-  void make_path_string(std::string& s, bool projected=false) const;
+  void make_path_string(std::string& s, bool projected=false,
+                       int path_comp_count=-1) const;
   void make_path(filepath& fp, bool projected=false) const;
 
   // -- version --

diff --git a/src/mds/CDir.cc b/src/mds/CDir.cc
index 7ae76cba27da4296c745d3f169cc147dbee36ef5..8d555995d082293cde83317d2b017bfc70641240 100644 (file)
--- a/src/mds/CDir.cc
+++ b/src/mds/CDir.cc
@@ -86,7 +86,7 @@ public:
 
 ostream& operator<<(ostream& out, const CDir& dir)
 {
-  out << "[dir " << dir.dirfrag() << " " << dir.get_path() << "/"
+  out << "[dir " << dir.dirfrag() << " " << dir.get_trimmed_path() << "/"
       << " [" << dir.first << ",head]";
   if (dir.is_auth()) {
     out << " auth";
@@ -2009,7 +2009,7 @@ void CDir::_omap_fetched(bufferlist& hdrbl, map<string, bufferlist>& omap,
     dout(0) << "_fetched missing object for " << *this << dendl;
 
     clog->error() << "dir " << dirfrag() << " object missing on disk; some "
-                     "files may be lost (" << get_path() << ")";
+                     "files may be lost (" << get_trimmed_path() << ")";
 
     go_bad(complete);
     return;
@@ -2024,14 +2024,14 @@ void CDir::_omap_fetched(bufferlist& hdrbl, map<string, bufferlist>& omap,
       derr << "Corrupt fnode in dirfrag " << dirfrag()
           << ": " << err.what() << dendl;
       clog->warn() << "Corrupt fnode header in " << dirfrag() << ": "
-                  << err.what() << " (" << get_path() << ")";
+                  << err.what() << " (" << get_trimmed_path() << ")";
       go_bad(complete);
       return;
     }
     if (!p.end()) {
       clog->warn() << "header buffer of dir " << dirfrag() << " has "
                  << hdrbl.length() - p.get_off() << " extra bytes ("
-                  << get_path() << ")";
+                  << get_trimmed_path() << ")";
       go_bad(complete);
       return;
     }
@@ -2149,7 +2149,7 @@ void CDir::_omap_fetched(bufferlist& hdrbl, map<string, bufferlist>& omap,
     } catch (const buffer::error &err) {
       mdcache->mds->clog->warn() << "Corrupt dentry '" << key.name << "' in "
                                   "dir frag " << dirfrag() << ": "
-                               << err.what() << "(" << get_path() << ")";
+                               << err.what() << "(" << get_trimmed_path() << ")";
 
       // Remember that this dentry is damaged.  Subsequent operations
       // that try to act directly on it will get their EIOs, but this
@@ -3765,7 +3765,22 @@ bool CDir::scrub_local()
   return good;
 }
 
-std::string CDir::get_path() const
+/* XXX: Return string containing only final 10 components of the path. This
+ * shortened path is to be used usually for only logging. This prevents the
+ * unnecessary generation of full version of a very long path (imagine a path
+ * with 2000 components) from inode because not only repeatedly printing long
+ * path in logs is unnecessary and unreadable but more importantly because it
+ * is an expensive process (since it's done for more or less individual log
+ * entries).
+ */
+std::string CDir::get_trimmed_path() const
+{
+  std::string path;
+  get_inode()->make_trimmed_path_string(path, true);
+  return path;
+}
+
+std::string CDir::get_path(bool trim_path) const
 {
   std::string path;
   get_inode()->make_path_string(path, true);

diff --git a/src/mds/CDir.h b/src/mds/CDir.h
index a5cd31017444eeb6735dda49bc8c65131e039b8d..e7092c95747596e394af6e0a529934c4cf659897 100644 (file)
--- a/src/mds/CDir.h
+++ b/src/mds/CDir.h
@@ -783,7 +783,8 @@ private:
   void steal_dentry(CDentry *dn);  // from another dir.  used by merge/split.
   void finish_old_fragment(MDSContext::vec& waiters, bool replay);
   void init_fragment_pins();
-  std::string get_path() const;
+  std::string get_trimmed_path() const;
+  std::string get_path(bool trim_path=false) const;
 
   // -- authority --
   /*

diff --git a/src/mds/CInode.cc b/src/mds/CInode.cc
index 0d5bf7a8c5f6655076a4857bca4961e7ae2fa000..e3dcaa04960d5663be8b35b7acedb954ed2fcd83 100644 (file)
--- a/src/mds/CInode.cc
+++ b/src/mds/CInode.cc
@@ -151,7 +151,7 @@ int num_cinode_locks = sizeof(cinode_lock_info) / sizeof(cinode_lock_info[0]);
 ostream& operator<<(ostream& out, const CInode& in)
 {
   string path;
-  in.make_path_string(path, true);
+  in.make_trimmed_path_string(path);
 
   out << "[inode " << in.ino();
   out << " [" 
@@ -1031,15 +1031,29 @@ bool CInode::is_projected_ancestor_of(const CInode *other) const
  * use_parent is NULL and projected is true, the primary parent's projected
  * inode is used all the way up the path chain. Otherwise the primary parent
  * stable inode is used.
+ *
+ * path_comp_count = path components count. default value is -1, which implies
+ * generate full path.
  */
-void CInode::make_path_string(string& s, bool projected, const CDentry *use_parent) const
+void CInode::make_path_string(string& s, bool projected,
+                             const CDentry *use_parent,
+                             int path_comp_count) const
 {
   if (!use_parent) {
     use_parent = projected ? get_projected_parent_dn() : parent;
   }
 
   if (use_parent) {
-    use_parent->make_path_string(s, projected);
+    if (path_comp_count == -1) {
+      use_parent->make_path_string(s, projected, path_comp_count);
+    } else if (path_comp_count >= 1) {
+      --path_comp_count;
+      use_parent->make_path_string(s, projected, path_comp_count);
+    } else if (path_comp_count == 0) {
+      // this indicates that path has been shortened.
+      s = "...";
+      return;
+    }
   } else if (is_root()) {
     s = "";
   } else if (is_mdsdir()) {
@@ -1056,6 +1070,25 @@ void CInode::make_path_string(string& s, bool projected, const CDentry *use_pare
   }
 }
 
+/* XXX Generating more than 10 components of a path for printing in logs will
+ * consume too much time when the path is too long (imagine a path with 2000
+ * components) since the path would've to be generated indidividually for each
+ * log entry.
+ *
+ * Besides consuming too much time, such long paths in logs are not only not
+ * useful but also it makes reading logs harder. Therefore, shorten the path
+ * when used for logging.
+ *
+ * path_comp_count = path components count. default value is 10, which implies
+ * generate full path.
+ */
+void CInode::make_trimmed_path_string(string& s, bool projected,
+                                     const CDentry* use_parent,
+                                     int path_comp_count) const
+{
+  make_path_string(s, projected, use_parent, path_comp_count);
+}
+
 void CInode::make_path(filepath& fp, bool projected) const
 {
   const CDentry *use_parent = projected ? get_projected_parent_dn() : parent;
@@ -2624,7 +2657,7 @@ void CInode::finish_scatter_gather_update(int type, MutationRef& mut)
 
       if (pi->dirstat.nfiles < 0 || pi->dirstat.nsubdirs < 0) {
         std::string path;
-        make_path_string(path);
+       make_trimmed_path_string(path);
        clog->error() << "Inconsistent statistics detected: fragstat on inode "
                       << ino() << " (" << path << "), inode has " << pi->dirstat;
        ceph_assert(!"bad/negative fragstat" == g_conf()->mds_verify_scatter);
@@ -4879,7 +4912,7 @@ next:
 
       if (!results->backtrace.passed && in->scrub_infop->header->get_repair()) {
         std::string path;
-        in->make_path_string(path);
+       in->make_trimmed_path_string(path);
         in->mdcache->mds->clog->warn() << "bad backtrace on inode " << in->ino()
                                        << "(" << path << "), rewriting it";
         in->mark_dirty_parent(in->mdcache->mds->mdlog->get_current_segment(),

diff --git a/src/mds/CInode.h b/src/mds/CInode.h
index 7258a45e89795a781a6a1845827ea20bb863df20..004468800fe36496d75396c08c9b3b47d0887784 100644 (file)
--- a/src/mds/CInode.h
+++ b/src/mds/CInode.h
@@ -718,8 +718,15 @@ class CInode : public MDSCacheObject, public InodeStoreBase, public Counter<CIno
   bool is_ancestor_of(const CInode *other, std::unordered_map<CInode const*,bool>* visited=nullptr) const;
   bool is_projected_ancestor_of(const CInode *other) const;
 
-  void make_path_string(std::string& s, bool projected=false, const CDentry *use_parent=NULL) const;
+  void make_path_string(std::string& s, bool projected=false,
+                       const CDentry *use_parent=NULL,
+                       int path_comp_count=-1) const;
   void make_path(filepath& s, bool projected=false) const;
+  void make_trimmed_path_string(std::string& s, bool projected=false,
+                               const CDentry *use_parent=NULL,
+                               int path_comp_count=10) const;
+  void make_path(filepath& s, bool projected=false,
+                int path_comp_count=-1) const;
   void name_stray_dentry(std::string& dname);
   
   // -- dirtyness --