From 7b4561091e11b9c941ef8fb96a90ab31a1d6ccec Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Thu, 17 Nov 2016 21:05:03 -0500 Subject: [PATCH] mds: use projected path construction for access A new CDentry will not have a parent until its projected parent is flushed to journal. During path construction a given dentry may have no parent yet which will cause fallbacks to be used (the inode number). This can cause access checks to fail: 2016-11-17 19:50:43.830207 7eff9977a700 20 Session check_access path #10000000002/3 compare to an earlier check: 2016-11-17 19:50:43.824223 7eff9977a700 20 Session check_access path /test/1/2 This commit refactors path construction to optionally use projected parents for the entire chain of directories. Existing use of real stable parents is unchanged. For example, this line is the same before and after the patch: 2016-11-18 23:17:15.611680 7f153f97a700 12 mds.0.cache.dir(10000000002) add_null_dentry [dentry #10000000002/3 [2,head] auth NULL (dversion lock) pv=0 v=1 inode=0 0x55e0f771f5f0] Here inode "#10000000002" has no stable parent yet. So the path is constructed as "#10000000002/3". One notable change in this commit is the removal of make_path_string_projected which was only used in debugging code. Here's an example difference: 2016-11-17 19:50:43.827915 7eff9977a700 10 mds.0.server traverse_to_auth_dir [dir 10000000003 {#10000000003 #10000000002/3}/ [2,head] auth v=1 cv=0/0 state=1073741826|complete f() n() hs=0+0,ss=0+0 0x55f5d35e2ee0] to: 2016-11-18 23:17:15.617757 7f153f97a700 10 mds.0.server traverse_to_auth_dir [dir 10000000003 /test/1/2/3/ [2,head] auth v=1 cv=0/0 state=1073741826|complete f() n() hs=0+0,ss=0+0 0x55e0f7706ee0] Fixes: http://tracker.ceph.com/issues/17858 Signed-off-by: Patrick Donnelly --- src/mds/CDentry.cc | 30 +++------------------ src/mds/CDentry.h | 4 +-- src/mds/CDir.cc | 2 +- src/mds/CInode.cc | 62 +++++++++++++++++-------------------------- src/mds/CInode.h | 6 ++--- src/mds/ScrubStack.cc | 2 +- src/mds/SessionMap.cc | 2 +- 7 files changed, 36 insertions(+), 72 deletions(-) diff --git a/src/mds/CDentry.cc b/src/mds/CDentry.cc index 2b953a03c34..c842a6555b1 100644 --- a/src/mds/CDentry.cc +++ b/src/mds/CDentry.cc @@ -204,10 +204,10 @@ void CDentry::mark_new() state_set(STATE_NEW); } -void CDentry::make_path_string(string& s) const +void CDentry::make_path_string(string& s, bool projected) const { if (dir) { - dir->inode->make_path_string(s); + dir->inode->make_path_string(s, projected); } else { s = "???"; } @@ -215,35 +215,13 @@ void CDentry::make_path_string(string& s) const s.append(name.data(), name.length()); } -void CDentry::make_path(filepath& fp) const +void CDentry::make_path(filepath& fp, bool projected) const { assert(dir); - if (dir->inode->is_base()) - fp = filepath(dir->inode->ino()); // base case - else if (dir->inode->get_parent_dn()) - dir->inode->get_parent_dn()->make_path(fp); // recurse - else - fp = filepath(dir->inode->ino()); // relative but not base? hrm! + dir->inode->make_path(fp, projected); fp.push_dentry(name); } -/* -void CDentry::make_path(string& s, inodeno_t tobase) -{ - assert(dir); - - if (dir->inode->is_root()) { - s += "/"; // make it an absolute path (no matter what) if we hit the root. - } - else if (dir->inode->get_parent_dn() && - dir->inode->ino() != tobase) { - dir->inode->get_parent_dn()->make_path(s, tobase); - s += "/"; - } - s += name; -} -*/ - /* * we only add ourselves to remote_parents when the linkage is * active (no longer projected). if the passed dnl is projected, diff --git a/src/mds/CDentry.h b/src/mds/CDentry.h index a6b647e5ae4..2c75f5f9440 100644 --- a/src/mds/CDentry.h +++ b/src/mds/CDentry.h @@ -276,8 +276,8 @@ public: const CDentry& operator= (const CDentry& right); // misc - void make_path_string(std::string& s) const; - void make_path(filepath& fp) const; + void make_path_string(std::string& s, bool projected=false) const; + void make_path(filepath& fp, bool projected=false) const; // -- version -- version_t get_version() const { return version; } diff --git a/src/mds/CDir.cc b/src/mds/CDir.cc index 9d20d16de6f..141b3fde1a8 100644 --- a/src/mds/CDir.cc +++ b/src/mds/CDir.cc @@ -3115,7 +3115,7 @@ bool CDir::scrub_local() std::string CDir::get_path() const { std::string path; - get_inode()->make_path_string_projected(path); + get_inode()->make_path_string(path, true); return path; } diff --git a/src/mds/CInode.cc b/src/mds/CInode.cc index 6bf463fc2c9..dad9be7f71b 100644 --- a/src/mds/CInode.cc +++ b/src/mds/CInode.cc @@ -101,7 +101,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_projected(path); + in.make_path_string(path, true); out << "[inode " << in.inode.ino; out << " [" @@ -813,61 +813,47 @@ bool CInode::is_projected_ancestor_of(CInode *other) } /* - * If use_parent is NULL (it should be one of inode's projected parents), - * we use it to make path string. Otherwise, we use inode's parent dentry - * to make path string + * Because a non-directory inode may have multiple links, the use_parent + * argument allows selecting which parent to use for path construction. This + * argument is only meaningful for the final component (i.e. the first of the + * nested calls) because directories cannot have multiple hard links. If + * 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. */ -void CInode::make_path_string(string& s, CDentry *use_parent) const +void CInode::make_path_string(string& s, bool projected, const CDentry *use_parent) const { - if (!use_parent) - use_parent = parent; + if (!use_parent) { + use_parent = projected ? get_projected_parent_dn() : parent; + } if (use_parent) { - use_parent->make_path_string(s); - } - else if (is_root()) { - s = ""; // root - } - else if (is_mdsdir()) { + use_parent->make_path_string(s, projected); + } else if (is_root()) { + s = ""; + } else if (is_mdsdir()) { char t[40]; uint64_t eino(ino()); eino -= MDS_INO_MDSDIR_OFFSET; snprintf(t, sizeof(t), "~mds%" PRId64, eino); s = t; - } - else { + } else { char n[40]; uint64_t eino(ino()); snprintf(n, sizeof(n), "#%" PRIx64, eino); s += n; } } -void CInode::make_path_string_projected(string& s) const -{ - make_path_string(s); - - if (!projected_parent.empty()) { - string q; - q.swap(s); - s = "{" + q; - for (list::const_iterator p = projected_parent.begin(); - p != projected_parent.end(); - ++p) { - string q; - make_path_string(q, *p); - s += " "; - s += q; - } - s += "}"; - } -} -void CInode::make_path(filepath& fp) const +void CInode::make_path(filepath& fp, bool projected) const { - if (parent) - parent->make_path(fp); - else + const CDentry *use_parent = projected ? get_projected_parent_dn() : parent; + if (use_parent) { + assert(!is_base()); + use_parent->make_path(fp, projected); + } else { fp = filepath(ino()); + } } void CInode::name_stray_dentry(string& dname) diff --git a/src/mds/CInode.h b/src/mds/CInode.h index ceab2e8949b..39806862357 100644 --- a/src/mds/CInode.h +++ b/src/mds/CInode.h @@ -739,9 +739,9 @@ public: // -- misc -- bool is_projected_ancestor_of(CInode *other); - void make_path_string(std::string& s, CDentry *use_parent=NULL) const; - void make_path_string_projected(std::string& s) const; - void make_path(filepath& s) const; + + void make_path_string(std::string& s, bool projected=false, const CDentry *use_parent=NULL) const; + void make_path(filepath& s, bool projected=false) const; void name_stray_dentry(std::string& dname); // -- dirtyness -- diff --git a/src/mds/ScrubStack.cc b/src/mds/ScrubStack.cc index 3448a496627..afc91c3961b 100644 --- a/src/mds/ScrubStack.cc +++ b/src/mds/ScrubStack.cc @@ -387,7 +387,7 @@ void ScrubStack::_validate_inode_done(CInode *in, int r, // Inform the cluster log if we found an error if (!result.passed_validation) { std::string path; - in->make_path_string_projected(path); + in->make_path_string(path, true); clog->warn() << "Scrub error on inode " << *in << " (" << path << ") see " << g_conf->name << " log for details"; diff --git a/src/mds/SessionMap.cc b/src/mds/SessionMap.cc index ddbefad46c0..5ea655ea0f3 100644 --- a/src/mds/SessionMap.cc +++ b/src/mds/SessionMap.cc @@ -894,7 +894,7 @@ int Session::check_access(CInode *in, unsigned mask, path = in->get_projected_inode()->stray_prior_path; dout(20) << __func__ << " stray_prior_path " << path << dendl; } else { - in->make_path_string(path, in->get_projected_parent_dn()); + in->make_path_string(path, true); dout(20) << __func__ << " path " << path << dendl; } if (path.length()) -- 2.47.3