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>

diff --git a/src/mds/CDentry.cc b/src/mds/CDentry.cc
index d373f82aa22ee48fe4c16efe5237254f4e911ecc..d2796d4c70935d3b0bf00fdf489d520addd40e88 100644 (file)
--- a/src/mds/CDentry.cc
+++ b/src/mds/CDentry.cc
@@ -300,10 +300,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 26c56fc2d3b6af779c70ba151ca45b7002ff6ecd..67cf9323666e5741d76074bdbe630cc0392cb598 100644 (file)
--- a/src/mds/CDentry.h
+++ b/src/mds/CDentry.h
@@ -252,7 +252,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 321d059af7109e8b394d3bbe280cfe5ac88f9541..d39dd757bae8579252d6c63dac9cbc040af3df3b 100644 (file)
--- a/src/mds/CDir.cc
+++ b/src/mds/CDir.cc
@@ -91,7 +91,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";
@@ -2094,8 +2094,8 @@ CDentry *CDir::_load_dentry(
                 << " mode " << ref_in->get_inode()->mode
                 << " mtime " << ref_in->get_inode()->mtime << dendl;
         string dirpath, inopath;
-        this->inode->make_path_string(dirpath);
-        ref_in->make_path_string(inopath);
+        this->inode->make_trimmed_path_string(dirpath);
+        ref_in->make_trimmed_path_string(inopath);
         mdcache->mds->clog->error() << "loaded dup referent inode " << inode_data.inode->ino
           << " [" << first << "," << last << "] v" << inode_data.inode->version
           << " at " << dirpath << "/" << dname
@@ -2245,7 +2245,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;
@@ -2260,14 +2260,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;
     }
@@ -2385,7 +2385,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
@@ -4057,7 +4057,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 34eeb3a8a826636c5c8cbe620894a00d3d4fb862..441d61d204b6dc1cfd9c06624e2fe87abb5a5a89 100644 (file)
--- a/src/mds/CDir.h
+++ b/src/mds/CDir.h
@@ -786,7 +786,8 @@ private:
   void steal_dentry(CDentry *dn);  // from another dir.  used by merge/split.
   void finish_old_fragment(std::vector<MDSContext*>& 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 244f348b25346209b5166dd0b07a285c393f1e50..2686e887a7aa333dd3447e5a4063a65045decb8f 100644 (file)
--- a/src/mds/CInode.cc
+++ b/src/mds/CInode.cc
@@ -177,7 +177,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 << " [" 
@@ -1095,15 +1095,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()) {
@@ -1120,6 +1134,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;
@@ -2695,7 +2728,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);
@@ -5000,7 +5033,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 f87fd408104586884ebb7414fd2d815e879c2626..53ff63ceaefbfbe60fe84862a733953a7bf32e30 100644 (file)
--- a/src/mds/CInode.h
+++ b/src/mds/CInode.h
@@ -742,8 +742,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 --