From 1518690210f3a4473978c7a9274e902fccaad862 Mon Sep 17 00:00:00 2001 From: Rishabh Dave Date: Sun, 17 Aug 2025 23:43:40 +0530 Subject: [PATCH] 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 --- src/mds/CDentry.cc | 5 +++-- src/mds/CDentry.h | 3 ++- src/mds/CDir.cc | 31 +++++++++++++++++++++++-------- src/mds/CDir.h | 3 ++- src/mds/CInode.cc | 43 ++++++++++++++++++++++++++++++++++++++----- src/mds/CInode.h | 9 ++++++++- 6 files changed, 76 insertions(+), 18 deletions(-) diff --git a/src/mds/CDentry.cc b/src/mds/CDentry.cc index d373f82aa22..d2796d4c709 100644 --- 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 26c56fc2d3b..67cf9323666 100644 --- 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 321d059af71..d39dd757bae 100644 --- 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& 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& 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& 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 34eeb3a8a82..441d61d204b 100644 --- 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& 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 244f348b253..2686e887a7a 100644 --- 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 f87fd408104..53ff63ceaef 100644 --- a/src/mds/CInode.h +++ b/src/mds/CInode.h @@ -742,8 +742,15 @@ class CInode : public MDSCacheObject, public InodeStoreBase, public Counter* 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 -- -- 2.39.5