From 120d70dc09a6e7a093284416afe39439ace9293d Mon Sep 17 00:00:00 2001 From: "Yan, Zheng" Date: Thu, 5 Dec 2019 17:24:47 +0800 Subject: [PATCH] mds: fix deadlock when xlocking policylock Previous commit makes Server::rdlock_path_pin_ref() rdlock snaplock and policylock. Deadlock happens if we use this function for requests that xlock snaplock or snaplock. Signed-off-by: "Yan, Zheng" --- src/mds/Server.cc | 168 ++++++++++++++++++++++++---------------------- src/mds/Server.h | 5 +- 2 files changed, 92 insertions(+), 81 deletions(-) diff --git a/src/mds/Server.cc b/src/mds/Server.cc index 919d161fda3..43c02b78be9 100644 --- a/src/mds/Server.cc +++ b/src/mds/Server.cc @@ -3305,8 +3305,7 @@ private: * as appropriate: forwarded on, or the client's been replied to */ CInode* Server::rdlock_path_pin_ref(MDRequestRef& mdr, bool want_auth, - bool no_want_auth, - bool want_layout) + bool no_want_auth) { const filepath& refpath = mdr->get_filepath(); dout(10) << "rdlock_path_pin_ref " << *mdr << " " << refpath << dendl; @@ -3325,8 +3324,6 @@ CInode* Server::rdlock_path_pin_ref(MDRequestRef& mdr, } if (want_auth) flags |= MDS_TRAVERSE_WANT_AUTH; - if (want_layout) - flags |= MDS_TRAVERSE_WANT_DIRLAYOUT; int r = mdcache->path_traverse(mdr, cf, refpath, flags, &mdr->dn[0], &mdr->in[0]); if (r > 0) return nullptr; // delayed @@ -5103,25 +5100,63 @@ void Server::handle_client_setlayout(MDRequestRef& mdr) journal_and_reply(mdr, cur, 0, le, new C_MDS_inode_update_finish(this, mdr, cur)); } +bool Server::xlock_policylock(MDRequestRef& mdr, CInode *in, bool want_layout, bool xlock_snaplock) +{ + if (mdr->locking_state & MutationImpl::ALL_LOCKED) + return true; + + MutationImpl::LockOpVec lov; + lov.add_xlock(&in->policylock); + if (xlock_snaplock) + lov.add_xlock(&in->snaplock); + else + lov.add_rdlock(&in->snaplock); + if (!mds->locker->acquire_locks(mdr, lov)) + return false; + + if (want_layout && in->get_projected_inode()->has_layout()) { + mdr->dir_layout = in->get_projected_inode()->layout; + want_layout = false; + } + if (CDentry *pdn = in->get_projected_parent_dn(); pdn) { + if (!mds->locker->try_rdlock_snap_layout(pdn->get_dir()->get_inode(), mdr, 0, want_layout)) + return false; + } + + mdr->locking_state |= MutationImpl::ALL_LOCKED; + return true; +} + +CInode* Server::try_get_auth_inode(MDRequestRef& mdr, inodeno_t ino) +{ + CInode *in = mdcache->get_inode(ino); + if (!in || in->state_test(CInode::STATE_PURGING)) { + respond_to_request(mdr, -ESTALE); + return nullptr; + } + if (!in->is_auth()) { + mdcache->request_forward(mdr, in->authority().first); + return nullptr; + } + + return in; +} + void Server::handle_client_setdirlayout(MDRequestRef& mdr) { const cref_t &req = mdr->client_request; - CInode *cur = rdlock_path_pin_ref(mdr, true, false, true); - if (!cur) return; - if (mdr->snapid != CEPH_NOSNAP) { - respond_to_request(mdr, -EROFS); + // can't use rdlock_path_pin_ref because we need to xlock snaplock/policylock + CInode *cur = try_get_auth_inode(mdr, req->get_filepath().get_ino()); + if (!cur) return; - } if (!cur->is_dir()) { respond_to_request(mdr, -ENOTDIR); return; } - MutationImpl::LockOpVec lov; - lov.add_xlock(&cur->policylock); - if (!mds->locker->acquire_locks(mdr, lov)) + if (!xlock_policylock(mdr, cur, true)) return; // validate layout @@ -5406,13 +5441,15 @@ void Server::handle_set_vxattr(MDRequestRef& mdr, CInode *cur) } bool new_realm = false; - MutationImpl::LockOpVec lov; if (name.compare(0, 15, "ceph.dir.layout") == 0) { if (!cur->is_dir()) { respond_to_request(mdr, -EINVAL); return; } + if (!xlock_policylock(mdr, cur, true)) + return; + file_layout_t layout; if (cur->get_projected_inode()->has_layout()) layout = cur->get_projected_inode()->layout; @@ -5425,10 +5462,6 @@ void Server::handle_set_vxattr(MDRequestRef& mdr, CInode *cur) if (check_layout_vxattr(mdr, rest, value, &layout) < 0) return; - lov.add_xlock(&cur->policylock); - if (!mds->locker->acquire_locks(mdr, lov)) - return; - auto &pi = cur->project_inode(); pi.inode.layout = layout; mdr->no_early_reply = true; @@ -5448,6 +5481,7 @@ void Server::handle_set_vxattr(MDRequestRef& mdr, CInode *cur) if (check_layout_vxattr(mdr, rest, value, &layout) < 0) return; + MutationImpl::LockOpVec lov; lov.add_xlock(&cur->filelock); if (!mds->locker->acquire_locks(mdr, lov)) return; @@ -5472,13 +5506,10 @@ void Server::handle_set_vxattr(MDRequestRef& mdr, CInode *cur) return; } - lov.add_xlock(&cur->policylock); - if (quota.is_enable() && !cur->get_projected_srnode()) { - lov.add_xlock(&cur->snaplock); + if (quota.is_enable() && !cur->get_projected_srnode()) new_realm = true; - } - if (!mds->locker->acquire_locks(mdr, lov)) + if (!xlock_policylock(mdr, cur, false, new_realm)) return; auto &pi = cur->project_inode(false, new_realm); @@ -5512,8 +5543,7 @@ void Server::handle_set_vxattr(MDRequestRef& mdr, CInode *cur) return; } - lov.add_xlock(&cur->policylock); - if (!mds->locker->acquire_locks(mdr, lov)) + if (!xlock_policylock(mdr, cur)) return; auto &pi = cur->project_inode(); @@ -5626,13 +5656,19 @@ void Server::handle_client_setxattr(MDRequestRef& mdr) { const cref_t &req = mdr->client_request; string name(req->get_path2()); - MutationImpl::LockOpVec lov; - CInode *cur; - if (name.compare(0, 15, "ceph.dir.layout") == 0) - cur = rdlock_path_pin_ref(mdr, true, false, true); - else - cur = rdlock_path_pin_ref(mdr, true); + // magic ceph.* namespace? + if (name.compare(0, 5, "ceph.") == 0) { + // can't use rdlock_path_pin_ref because we need to xlock snaplock/policylock + CInode *cur = try_get_auth_inode(mdr, req->get_filepath().get_ino()); + if (!cur) + return; + + handle_set_vxattr(mdr, cur); + return; + } + + CInode *cur = rdlock_path_pin_ref(mdr, true); if (!cur) return; @@ -5643,12 +5679,7 @@ void Server::handle_client_setxattr(MDRequestRef& mdr) int flags = req->head.args.setxattr.flags; - // magic ceph.* namespace? - if (name.compare(0, 5, "ceph.") == 0) { - handle_set_vxattr(mdr, cur); - return; - } - + MutationImpl::LockOpVec lov; lov.add_xlock(&cur->xattrlock); if (!mds->locker->acquire_locks(mdr, lov)) return; @@ -5725,11 +5756,17 @@ void Server::handle_client_removexattr(MDRequestRef& mdr) const cref_t &req = mdr->client_request; std::string name(req->get_path2()); - CInode *cur; - if (name == "ceph.dir.layout") - cur = rdlock_path_pin_ref(mdr, true, false, true); - else - cur = rdlock_path_pin_ref(mdr, true); + if (name.compare(0, 5, "ceph.") == 0) { + // can't use rdlock_path_pin_ref because we need to xlock snaplock/policylock + CInode *cur = try_get_auth_inode(mdr, req->get_filepath().get_ino()); + if (!cur) + return; + + handle_remove_vxattr(mdr, cur); + return; + } + + CInode* cur = rdlock_path_pin_ref(mdr, true); if (!cur) return; @@ -5738,11 +5775,6 @@ void Server::handle_client_removexattr(MDRequestRef& mdr) return; } - if (name.compare(0, 5, "ceph.") == 0) { - handle_remove_vxattr(mdr, cur); - return; - } - MutationImpl::LockOpVec lov; lov.add_xlock(&cur->xattrlock); if (!mds->locker->acquire_locks(mdr, lov)) @@ -9564,15 +9596,10 @@ void Server::handle_client_lssnap(MDRequestRef& mdr) const cref_t &req = mdr->client_request; // traverse to path - CInode *diri = mdcache->get_inode(req->get_filepath().get_ino()); - if (!diri || diri->state_test(CInode::STATE_PURGING)) { - respond_to_request(mdr, -ESTALE); - return; - } - if (!diri->is_auth()) { - mdcache->request_forward(mdr, diri->authority().first); + CInode *diri = try_get_auth_inode(mdr, req->get_filepath().get_ino()); + if (!diri) return; - } + if (!diri->is_dir()) { respond_to_request(mdr, -ENOTDIR); return; @@ -9686,16 +9713,9 @@ void Server::handle_client_mksnap(MDRequestRef& mdr) return; } - CInode *diri = mdcache->get_inode(req->get_filepath().get_ino()); - if (!diri || diri->state_test(CInode::STATE_PURGING)) { - respond_to_request(mdr, -ESTALE); - return; - } - - if (!diri->is_auth()) { // fw to auth? - mdcache->request_forward(mdr, diri->authority().first); + CInode *diri = try_get_auth_inode(mdr, req->get_filepath().get_ino()); + if (!diri) return; - } // dir only if (!diri->is_dir()) { @@ -9856,15 +9876,10 @@ void Server::handle_client_rmsnap(MDRequestRef& mdr) { const cref_t &req = mdr->client_request; - CInode *diri = mdcache->get_inode(req->get_filepath().get_ino()); - if (!diri || diri->state_test(CInode::STATE_PURGING)) { - respond_to_request(mdr, -ESTALE); + CInode *diri = try_get_auth_inode(mdr, req->get_filepath().get_ino()); + if (!diri) return; - } - if (!diri->is_auth()) { // fw to auth? - mdcache->request_forward(mdr, diri->authority().first); - return; - } + if (!diri->is_dir()) { respond_to_request(mdr, -ENOTDIR); return; @@ -9998,16 +10013,9 @@ void Server::handle_client_renamesnap(MDRequestRef& mdr) return; } - CInode *diri = mdcache->get_inode(req->get_filepath().get_ino()); - if (!diri || diri->state_test(CInode::STATE_PURGING)) { - respond_to_request(mdr, -ESTALE); - return; - } - - if (!diri->is_auth()) { // fw to auth? - mdcache->request_forward(mdr, diri->authority().first); + CInode *diri = try_get_auth_inode(mdr, req->get_filepath().get_ino()); + if (!diri) return; - } if (!diri->is_dir()) { // dir only respond_to_request(mdr, -ENOTDIR); diff --git a/src/mds/Server.h b/src/mds/Server.h index b8c1eb26af7..3160c69a263 100644 --- a/src/mds/Server.h +++ b/src/mds/Server.h @@ -213,7 +213,7 @@ public: void apply_allocated_inos(MDRequestRef& mdr, Session *session); CInode* rdlock_path_pin_ref(MDRequestRef& mdr, bool want_auth, - bool no_want_auth=false, bool want_layout=false); + bool no_want_auth=false); CDentry* rdlock_path_xlock_dentry(MDRequestRef& mdr, bool create, bool okexist=false, bool want_layout=false); std::pair @@ -232,6 +232,9 @@ public: void handle_client_file_setlock(MDRequestRef& mdr); void handle_client_file_readlock(MDRequestRef& mdr); + bool xlock_policylock(MDRequestRef& mdr, CInode *in, + bool want_layout=false, bool xlock_snaplock=false); + CInode* try_get_auth_inode(MDRequestRef& mdr, inodeno_t ino); void handle_client_setattr(MDRequestRef& mdr); void handle_client_setlayout(MDRequestRef& mdr); void handle_client_setdirlayout(MDRequestRef& mdr); -- 2.39.5