From f57530dbbb1a9925b782296c27b85387a580dbf4 Mon Sep 17 00:00:00 2001 From: huanwen ren Date: Tue, 16 Jan 2018 14:18:46 +0800 Subject: [PATCH] client: fixup parallel calls to ceph_ll_lookup_inode() in NFS FASL fixup parallel calls to ceph_ll_lookup_inode() in NFS FASL, Because the original fuction does not have a global Lock to ensure the atomicity of the lookup inode process,It eventually caused an exception when the NFS-Ganesha FSAL module called this interface in parallel,see the Bug report. Fixes: http://tracker.ceph.com/issues/22683 Signed-off-by: huanwen ren (cherry picked from commit b1248273d67d32eaba35cadf0f6f02942f9da81f) Conflicts: src/client/Client.cc src/libcephfs.cc --- src/client/Client.cc | 80 ++++++++++++++++++++++++++++++++++++++------ src/client/Client.h | 5 +++ src/libcephfs.cc | 36 +------------------- 3 files changed, 76 insertions(+), 45 deletions(-) diff --git a/src/client/Client.cc b/src/client/Client.cc index 41eff1b4b2a2..4cdfee70d55d 100644 --- a/src/client/Client.cc +++ b/src/client/Client.cc @@ -8243,9 +8243,8 @@ int Client::lookup_hash(inodeno_t ino, inodeno_t dirino, const char *name, * the resulting Inode object in one operation, so that caller * can safely assume inode will still be there after return. */ -int Client::lookup_ino(inodeno_t ino, const UserPerm& perms, Inode **inode) +int Client::_lookup_ino(inodeno_t ino, const UserPerm& perms, Inode **inode) { - Mutex::Locker lock(client_lock); ldout(cct, 3) << "lookup_ino enter(" << ino << ")" << dendl; if (unmounting) @@ -8267,16 +8266,19 @@ int Client::lookup_ino(inodeno_t ino, const UserPerm& perms, Inode **inode) return r; } - +int Client::lookup_ino(inodeno_t ino, const UserPerm& perms, Inode **inode) +{ + Mutex::Locker lock(client_lock); + return _lookup_ino(ino, perms, inode); +} /** * Find the parent inode of `ino` and insert it into * our cache. Conditionally also set `parent` to a referenced * Inode* if caller provides non-NULL value. */ -int Client::lookup_parent(Inode *ino, const UserPerm& perms, Inode **parent) +int Client::_lookup_parent(Inode *ino, const UserPerm& perms, Inode **parent) { - Mutex::Locker lock(client_lock); ldout(cct, 3) << "lookup_parent enter(" << ino->ino << ")" << dendl; if (unmounting) @@ -8315,16 +8317,19 @@ int Client::lookup_parent(Inode *ino, const UserPerm& perms, Inode **parent) return r; } +int Client::lookup_parent(Inode *ino, const UserPerm& perms, Inode **parent) +{ + Mutex::Locker lock(client_lock); + return _lookup_parent(ino, perms, parent); +} /** * Populate the parent dentry for `ino`, provided it is * a child of `parent`. */ -int Client::lookup_name(Inode *ino, Inode *parent, const UserPerm& perms) +int Client::_lookup_name(Inode *ino, Inode *parent, const UserPerm& perms) { assert(parent->is_dir()); - - Mutex::Locker lock(client_lock); ldout(cct, 3) << "lookup_name enter(" << ino->ino << ")" << dendl; if (unmounting) @@ -8340,6 +8345,11 @@ int Client::lookup_name(Inode *ino, Inode *parent, const UserPerm& perms) return r; } +int Client::lookup_name(Inode *ino, Inode *parent, const UserPerm& perms) +{ + Mutex::Locker lock(client_lock); + return _lookup_name(ino, parent, perms); +} Fh *Client::_create_fh(Inode *in, int flags, int cmode, const UserPerm& perms) { @@ -10325,6 +10335,51 @@ int Client::ll_lookup(Inode *parent, const char *name, struct stat *attr, return r; } +int Client::ll_lookup_inode( + struct inodeno_t ino, + const UserPerm& perms, + Inode **inode) +{ + Mutex::Locker lock(client_lock); + ldout(cct, 3) << "ll_lookup_inode " << ino << dendl; + + // Num1: get inode and *inode + int r = _lookup_ino(ino, perms, inode); + if (r) { + return r; + } + assert(inode != NULL); + assert(*inode != NULL); + + // Num2: Request the parent inode, so that we can look up the name + Inode *parent; + r = _lookup_parent(*inode, perms, &parent); + if (r && r != -EINVAL) { + // Unexpected error + _ll_forget(*inode, 1); + return r; + } else if (r == -EINVAL) { + // EINVAL indicates node without parents (root), drop out now + // and don't try to look up the non-existent dentry. + return 0; + } + // FIXME: I don't think this works; lookup_parent() returns 0 if the parent + // is already in cache + assert(parent != NULL); + + // Num3: Finally, get the name (dentry) of the requested inode + r = _lookup_name(*inode, parent, perms); + if (r) { + // Unexpected error + _ll_forget(parent, 1); + _ll_forget(*inode, 1); + return r; + } + + _ll_forget(parent, 1); + return 0; +} + int Client::ll_lookupx(Inode *parent, const char *name, Inode **out, struct ceph_statx *stx, unsigned want, unsigned flags, const UserPerm& perms) @@ -10443,9 +10498,8 @@ void Client::_ll_drop_pins() } } -bool Client::ll_forget(Inode *in, int count) +bool Client::_ll_forget(Inode *in, int count) { - Mutex::Locker lock(client_lock); inodeno_t ino = _get_inodeno(in); ldout(cct, 3) << "ll_forget " << ino << " " << count << dendl; @@ -10473,6 +10527,12 @@ bool Client::ll_forget(Inode *in, int count) return last; } +bool Client::ll_forget(Inode *in, int count) +{ + Mutex::Locker lock(client_lock); + return _ll_forget(in, count); +} + bool Client::ll_put(Inode *in) { /* ll_forget already takes the lock */ diff --git a/src/client/Client.h b/src/client/Client.h index 9c076214ac76..0ee6cde8b944 100644 --- a/src/client/Client.h +++ b/src/client/Client.h @@ -945,6 +945,10 @@ private: mds_rank_t _get_random_up_mds() const; int _ll_getattr(Inode *in, int caps, const UserPerm& perms); + int _lookup_parent(Inode *in, const UserPerm& perms, Inode **parent=NULL); + int _lookup_name(Inode *in, Inode *parent, const UserPerm& perms); + int _lookup_ino(inodeno_t ino, const UserPerm& perms, Inode **inode=NULL); + bool _ll_forget(Inode *in, int count); public: int mount(const std::string &mount_root, const UserPerm& perms, @@ -1142,6 +1146,7 @@ public: Inode *ll_get_inode(vinodeno_t vino); int ll_lookup(Inode *parent, const char *name, struct stat *attr, Inode **out, const UserPerm& perms); + int ll_lookup_inode(struct inodeno_t ino, const UserPerm& perms, Inode **inode); int ll_lookupx(Inode *parent, const char *name, Inode **out, struct ceph_statx *stx, unsigned want, unsigned flags, const UserPerm& perms); diff --git a/src/libcephfs.cc b/src/libcephfs.cc index f3fa5a97b2ef..6b99359d8c83 100644 --- a/src/libcephfs.cc +++ b/src/libcephfs.cc @@ -1393,41 +1393,7 @@ extern "C" int ceph_ll_lookup_inode( struct inodeno_t ino, Inode **inode) { - int r = (cmount->get_client())->lookup_ino(ino, cmount->default_perms, inode); - if (r) { - return r; - } - - assert(inode != NULL); - assert(*inode != NULL); - - // Request the parent inode, so that we can look up the name - Inode *parent; - r = (cmount->get_client())->lookup_parent(*inode, cmount->default_perms, &parent); - if (r && r != -EINVAL) { - // Unexpected error - (cmount->get_client())->ll_forget(*inode, 1); - return r; - } else if (r == -EINVAL) { - // EINVAL indicates node without parents (root), drop out now - // and don't try to look up the non-existent dentry. - return 0; - } - // FIXME: I don't think this works; lookup_parent() returns 0 if the parent - // is already in cache - assert(parent != NULL); - - // Finally, get the name (dentry) of the requested inode - r = (cmount->get_client())->lookup_name(*inode, parent, cmount->default_perms); - if (r) { - // Unexpected error - (cmount->get_client())->ll_forget(parent, 1); - (cmount->get_client())->ll_forget(*inode, 1); - return r; - } - - (cmount->get_client())->ll_forget(parent, 1); - return 0; + return (cmount->get_client())->ll_lookup_inode(ino, cmount->default_perms, inode); } extern "C" int ceph_ll_lookup(struct ceph_mount_info *cmount, -- 2.47.3