Merge pull request ceph#63917 from rishabh-d-dave/vols-purge-trash-entry

pybind/cephfs, mgr/volumes: introduce non-recurisve rmtree(), refactor purge() to use it and add MDS optimizations

Reviewed-by: Patrick Donnelly <[email protected]>

Author: rishabh-d-dave

qa/workunits/fs/test_python.sh

@@ -1,6 +1,4 @@
 #!/bin/sh -ex
 
-# Running as root because the filesystem root directory will be
-# owned by uid 0, and that's where we're writing.
-sudo python3 -m pytest -v $(dirname $0)/../../../src/test/pybind/test_cephfs.py
+python3 -m pytest -v $(dirname $0)/../../../src/test/pybind/test_cephfs.py
 exit 0

src/client/Client.cc

@@ -7729,7 +7729,10 @@ int Client::path_walk(InodeRef dirinode, const filepath& origpath, InodeRef *end
   return rc;
 }
 
-int Client::path_walk(InodeRef dirinode, const filepath& origpath, walk_dentry_result* result, const UserPerm& perms, const PathWalk_ExtraOptions& extra_options)
+int Client::path_walk(InodeRef dirinode, const filepath& origpath,
+		      walk_dentry_result* result, const UserPerm& perms,
+		      const PathWalk_ExtraOptions& extra_options,
+		      std::string trimmed_path)
 {
   int rc = 0;
   filepath path = origpath;
@@ -7753,7 +7756,11 @@ int Client::path_walk(InodeRef dirinode, const filepath& origpath, walk_dentry_r
   int symlinks = 0;
   unsigned i = 0;
 
-  ldout(cct, 10) << __func__ << ": cur=" << *diri << " path=" << path << dendl;
+  if (trimmed_path == "") {
+    std::string trimmed_path = path.get_trimmed_path();
+  }
+
+  ldout(cct, 10) << __func__ << ": cur=" << *diri << " path=" << trimmed_path << dendl;
 
   if (path.depth() == 0) {
     /* diri/dname can also be used as a filepath; or target */
@@ -7767,7 +7774,7 @@ int Client::path_walk(InodeRef dirinode, const filepath& origpath, walk_dentry_r
     int caps = 0;
     dname = path[i];
     ldout(cct, 10) << " " << i << " " << *diri << " " << dname << dendl;
-    ldout(cct, 20) << "  (path is " << path << ")" << dendl;
+    ldout(cct, 20) << "  (path is " << trimmed_path << ")" << dendl;
     InodeRef next;
     if (!diri.get()->is_dir()) {
       ldout(cct, 20) << diri.get() << " is not a dir inode, name " << dname.c_str() << dendl;
@@ -15471,11 +15478,12 @@ int Client::ll_unlink(Inode *in, const char *name, const UserPerm& perm)
 
 int Client::_rmdir(Inode *dir, const char *name, const UserPerm& perms, bool check_perms)
 {
-  ldout(cct, 8) << "_rmdir(" << dir->ino << " " << name << " uid "
+  std::string trimmed_path = filepath(name).get_trimmed_path();
+  ldout(cct, 8) << "_rmdir(" << dir->ino << " " << trimmed_path << " uid "
 		<< perms.uid() << " gid " << perms.gid() << ")" << dendl;
 
   walk_dentry_result wdr;
-  if (int rc = path_walk(dir, filepath(name), &wdr, perms, {.followsym = false}); rc < 0) {
+  if (int rc = path_walk(dir, filepath(name), &wdr, perms, {.followsym = false}, trimmed_path); rc < 0) {
     return rc;
   }
 

src/client/Client.h

@@ -1108,7 +1108,7 @@ class Client : public Dispatcher, public md_config_obs_t {
     bool is_rename = false;
     bool require_target = true;
   };
-  int path_walk(InodeRef dirinode, const filepath& fp, struct walk_dentry_result* result, const UserPerm& perms, const PathWalk_ExtraOptions& extra_options);
+  int path_walk(InodeRef dirinode, const filepath& fp, struct walk_dentry_result* result, const UserPerm& perms, const PathWalk_ExtraOptions& extra_options, std::string trimmed_path=std::string());
   int path_walk(InodeRef dirinode, const filepath& fp, InodeRef *end, const UserPerm& perms, const PathWalk_ExtraOptions& extra_options);
 
   // fake inode number for 32-bits ino_t

src/common/strescape.h

@@ -34,4 +34,21 @@ inline std::string binstrprint(std::string_view sv, size_t maxlen=0)
   return s;
 }
 
+inline std::string get_trimmed_path_str(const std::string& path)
+{
+  // index of '/' before 10th component (count from end of the path).
+  size_t n = 0;
+
+  for (int i = 1; i <= 10; ++i) {
+    n = path.rfind("/", n - 1);
+    if (n == std::string::npos) {
+      // path doesn't contain 10 components, return path as it is.
+      return path;
+      break;
+    }
+  }
+
+  return std::string("..." + path.substr(n, -1));
+}
+
 #endif

src/include/filepath.cc

@@ -17,6 +17,21 @@
 
 #include <ostream>
 
+/* Trim given path to final 10 components and return it by prefixing it with
+ * "..."  to indicate that the path has been trimmed. */
+std::string filepath::get_trimmed_path() const
+{
+  std::size_t n = 0;
+  for (int i = 1; i <= 10; ++i) {
+    n = path.rfind("/", n - 1);
+    if (n == std::string::npos) {
+      return std::string(path);
+    }
+  }
+
+  return std::string("..." + path.substr(n, -1));
+}
+
 void filepath::rebuild_path() {
   path.clear();
   for (unsigned i=0; i<bits.size(); i++) {
@@ -41,6 +56,14 @@ void filepath::parse_bits() const {
   }
 }
 
+void filepath::set_trimmed() {
+  if (trimmed)
+    return;
+  // indicates that the path has been shortened.
+  path += "...";
+  trimmed = true;
+}
+
 void filepath::set_path(std::string_view s) {
   if (!s.empty() && s[0] == '/') {
     path = s.substr(1);

src/include/filepath.h

@@ -38,6 +38,9 @@ namespace ceph { class Formatter; }
 class filepath {
   inodeno_t ino = 0;   // base inode.  ino=0 implies pure relative path.
   std::string path;     // relative path.
+  // tells get_path() whether it should prefix path with "..." to indicate that
+  // it was shortened.
+  bool trimmed = false;
 
   /** bits - path segments
    * this is ['a', 'b', 'c'] for both the aboslute and relative case.
@@ -82,6 +85,8 @@ class filepath {
   // accessors
   inodeno_t get_ino() const { return ino; }
   const std::string& get_path() const { return path; }
+  void set_trimmed();
+  std::string get_trimmed_path() const;
   const char *c_str() const { return path.c_str(); }
 
   int length() const { return path.length(); }

src/mds/CDentry.cc

@@ -85,7 +85,7 @@ const LockType CDentry::versionlock_type(CEPH_LOCK_DVERSION);
 ostream& operator<<(ostream& out, const CDentry& dn)
 {
   filepath path;
-  dn.make_path(path);
+  dn.make_trimmed_path(path);
 
   out << "[dentry " << path;
 
@@ -300,24 +300,74 @@ 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 = "???";
   }
   s += "/";
   s.append(name.data(), name.length());
 }
 
-void CDentry::make_path(filepath& fp, bool projected) const
+/* path_comp_count = path component count. default value is 10 which implies
+ * generate entire path.
+ *
+ * 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.
+ */
+void CDentry::make_trimmed_path_string(string& s, bool projected,
+				       int path_comp_count) const
 {
-  ceph_assert(dir);
-  dir->inode->make_path(fp, projected);
-  fp.push_dentry(get_name());
+  make_path_string(s, projected, path_comp_count);
+}
+
+/* path_comp_count = path component count. default value is -1 which implies
+ * generate entire path.
+ */
+void CDentry::make_path(filepath& fp, bool projected,
+		        int path_comp_count) const
+{
+  fp.set_trimmed();
+
+  if (path_comp_count == -1) {
+    ceph_assert(dir);
+    dir->inode->make_path(fp, projected, path_comp_count);
+    fp.push_dentry(get_name());
+  } else if (path_comp_count >= 1) {
+    --path_comp_count;
+
+    ceph_assert(dir);
+    dir->inode->make_path(fp, projected, path_comp_count);
+    fp.push_dentry(get_name());
+  }
 }
 
+/* path_comp_count = path component count. default value is 10 which implies
+ * generate entire path.
+ *
+ * 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.
+ */
+void CDentry::make_trimmed_path(filepath& fp, bool projected,
+				int path_comp_count) const
+{
+  make_path(fp, projected, path_comp_count);
+}
 /*
  * we only add ourselves to remote_parents when the linkage is
  * active (no longer projected).  if the passed dnl is projected,

src/mds/CDentry.h

@@ -252,8 +252,14 @@ class CDentry : public MDSCacheObject, public LRUObject, public Counter<CDentry>
   const CDentry& operator= (const CDentry& right);
 
   // misc
-  void make_path_string(std::string& s, bool projected=false) const;
-  void make_path(filepath& fp, bool projected=false) const;
+  void make_trimmed_path_string(std::string& s, bool projected,
+				int path_comp_count=10) 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,
+		 int path_comp_count=-1) const;
+  void make_trimmed_path(filepath& fp, bool projected=false,
+			 int path_comp_count=10) const;
 
   // -- version --
   version_t get_version() const { return version; }

src/mds/CDir.cc

@@ -91,7 +91,7 @@ class CDirIOContext : public MDSIOContextBase
 
 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);

src/mds/CDir.h

@@ -786,7 +786,8 @@ class CDir : public MDSCacheObject, public Counter<CDir> {
   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 --
   /*