From bf760602a4f02cc07072db2da5cb987e3072afce Mon Sep 17 00:00:00 2001 From: Leonid Usov Date: Sun, 12 May 2024 19:19:34 +0300 Subject: [PATCH] revert: mds: provide a mechanism to authpin while freezing This is a functional revert of a9964a7ccc4394f923fb0f1c76eb8fa03fe8733d git revert was giving too many conflicts, as the code has changed too much since the original commit. The bypass freezing mechanism lead us into several deadlocks, and when we found out that a freezing inode defers reclaiming client caps, we realized that we needed to try a different approach. This commit removes the bypass freezing related changes to clear way for a different approach to resolving the conflict between quiesce and freezing. Signed-off-by: Leonid Usov --- src/common/TrackedOp.h | 15 ++++++++------- src/mds/CDentry.cc | 4 ++-- src/mds/CDentry.h | 2 +- src/mds/CDir.cc | 13 +++---------- src/mds/CDir.h | 2 +- src/mds/CInode.cc | 13 +++---------- src/mds/CInode.h | 2 +- src/mds/Locker.cc | 4 +--- src/mds/MDSCacheObject.h | 2 +- src/mds/Mutation.cc | 4 ++-- src/mds/Mutation.h | 2 +- src/mds/Server.cc | 6 +----- src/messages/MMDSPeerRequest.h | 3 --- 13 files changed, 25 insertions(+), 47 deletions(-) diff --git a/src/common/TrackedOp.h b/src/common/TrackedOp.h index 1a921309ca01b..435e7edb028ec 100644 --- a/src/common/TrackedOp.h +++ b/src/common/TrackedOp.h @@ -204,13 +204,15 @@ public: } ~OpTracker(); - template - typename T::Ref create_request(U params) + // NB: P is ref-like, i.e. `params` should be dereferenced for members + template + typename R::Ref create_request(P params) { - constexpr bool has_is_continuous = requires(U u) { - { u->is_continuous() } -> std::same_as; + constexpr bool enable_mark_continuous = requires(typename R::Ref r, P p) { + { p->is_continuous() } -> std::same_as; + r->mark_continuous(); }; - typename T::Ref retval(new T(params, this)); + typename R::Ref retval(new R(params, this)); retval->tracking_start(); if (is_tracking()) { retval->mark_event("header_read", params->get_recv_stamp()); @@ -218,12 +220,11 @@ public: retval->mark_event("all_read", params->get_recv_complete_stamp()); retval->mark_event("dispatched", params->get_dispatch_stamp()); } - if constexpr (has_is_continuous) { + if constexpr (enable_mark_continuous) { if (params->is_continuous()) { retval->mark_continuous(); } } - return retval; } }; diff --git a/src/mds/CDentry.cc b/src/mds/CDentry.cc index 9f5f1a49f55ec..8694dfc72a2e6 100644 --- a/src/mds/CDentry.cc +++ b/src/mds/CDentry.cc @@ -368,10 +368,10 @@ int CDentry::get_num_dir_auth_pins() const return auth_pins; } -bool CDentry::can_auth_pin(int *err_ret, bool bypassfreezing) const +bool CDentry::can_auth_pin(int *err_ret) const { ceph_assert(dir); - return dir->can_auth_pin(err_ret, bypassfreezing); + return dir->can_auth_pin(err_ret); } void CDentry::auth_pin(void *by) diff --git a/src/mds/CDentry.h b/src/mds/CDentry.h index 837ce2b8818d8..2566395d18561 100644 --- a/src/mds/CDentry.h +++ b/src/mds/CDentry.h @@ -222,7 +222,7 @@ public: void _put() override; // auth pins - bool can_auth_pin(int *err_ret=nullptr, bool bypassfreezing=false) const override; + bool can_auth_pin(int *err_ret=nullptr) const override; void auth_pin(void *by) override; void auth_unpin(void *by) override; void adjust_nested_auth_pins(int diradj, void *by); diff --git a/src/mds/CDir.cc b/src/mds/CDir.cc index 025e4d17b81fa..3828fe04d7b2a 100644 --- a/src/mds/CDir.cc +++ b/src/mds/CDir.cc @@ -3450,23 +3450,16 @@ void CDir::adjust_freeze_after_rename(CDir *dir) mdcache->mds->queue_waiters(unfreeze_waiters); } -bool CDir::can_auth_pin(int *err_ret, bool bypassfreezing) const +bool CDir::can_auth_pin(int *err_ret) const { int err; if (!is_auth()) { err = ERR_NOT_AUTH; - } else if (is_freezing_dir()) { - if (bypassfreezing) { - dout(20) << "allowing authpin with freezing" << dendl; - err = 0; - } else { - err = ERR_FRAGMENTING_DIR; - } - } else if (is_frozen_dir()) { + } else if (is_freezing_dir() || is_frozen_dir()) { err = ERR_FRAGMENTING_DIR; } else { auto p = is_freezing_or_frozen_tree(); - if (p.first && !bypassfreezing) { + if (p.first) { err = ERR_EXPORTING_TREE; } else if (p.second) { err = ERR_EXPORTING_TREE; diff --git a/src/mds/CDir.h b/src/mds/CDir.h index b70725f714ed7..76ac7e21cc090 100644 --- a/src/mds/CDir.h +++ b/src/mds/CDir.h @@ -527,7 +527,7 @@ public: void abort_import(); // -- auth pins -- - bool can_auth_pin(int *err_ret=nullptr, bool bypassfreezing=false) const override; + bool can_auth_pin(int *err_ret=nullptr) const override; int get_auth_pins() const { return auth_pins; } int get_dir_auth_pins() const { return dir_auth_pins; } void auth_pin(void *who) override; diff --git a/src/mds/CInode.cc b/src/mds/CInode.cc index 49a2abd83b42d..8fe15c0047fc3 100644 --- a/src/mds/CInode.cc +++ b/src/mds/CInode.cc @@ -2970,22 +2970,15 @@ void CInode::clear_ambiguous_auth() } // auth_pins -bool CInode::can_auth_pin(int *err_ret, bool bypassfreezing) const { +bool CInode::can_auth_pin(int *err_ret) const { int err; if (!is_auth()) { err = ERR_NOT_AUTH; - } else if (is_freezing_inode()) { - if (bypassfreezing) { - dout(20) << "allowing authpin with freezing" << dendl; - err = 0; - } else { - err = ERR_EXPORTING_INODE; - } - } else if (is_frozen_inode() || is_frozen_auth_pin()) { + } else if (is_freezing_inode() || is_frozen_inode() || is_frozen_auth_pin()) { err = ERR_EXPORTING_INODE; } else { if (parent) - return parent->can_auth_pin(err_ret, bypassfreezing); + return parent->can_auth_pin(err_ret); err = 0; } if (err && err_ret) diff --git a/src/mds/CInode.h b/src/mds/CInode.h index 45555a07d8b8c..254ca7efc6543 100644 --- a/src/mds/CInode.h +++ b/src/mds/CInode.h @@ -930,7 +930,7 @@ class CInode : public MDSCacheObject, public InodeStoreBase, public Countercan_auth_pin(&err, skip_quiesce)) { + if (!object->can_auth_pin(&err)) { if (mdr->lock_cache) { CDir *dir; if (CInode *in = dynamic_cast(object)) { @@ -521,8 +521,6 @@ bool Locker::acquire_locks(const MDRequestRef& mdr, } if (auth_pin_nonblocking) req->mark_nonblocking(); - if (skip_quiesce) - req->mark_bypassfreezing(); else if (!mdr->locks.empty()) req->mark_notify_blocking(); diff --git a/src/mds/MDSCacheObject.h b/src/mds/MDSCacheObject.h index 7bc820e4234bd..d322a05851a56 100644 --- a/src/mds/MDSCacheObject.h +++ b/src/mds/MDSCacheObject.h @@ -220,7 +220,7 @@ class MDSCacheObject { ERR_FRAGMENTING_DIR, ERR_EXPORTING_INODE, }; - virtual bool can_auth_pin(int *err_code=nullptr, bool bypassfreezing=false) const = 0; + virtual bool can_auth_pin(int *err_code=nullptr) const = 0; virtual void auth_pin(void *who) = 0; virtual void auth_unpin(void *who) = 0; virtual bool is_frozen() const = 0; diff --git a/src/mds/Mutation.cc b/src/mds/Mutation.cc index e94a988703f05..b7769319cb1cf 100644 --- a/src/mds/Mutation.cc +++ b/src/mds/Mutation.cc @@ -364,9 +364,9 @@ void MDRequestImpl::clear_ambiguous_auth() more()->is_ambiguous_auth = false; } -bool MDRequestImpl::can_auth_pin(MDSCacheObject *object, bool bypassfreezing) +bool MDRequestImpl::can_auth_pin(MDSCacheObject *object) { - return object->can_auth_pin(nullptr, bypassfreezing) || + return object->can_auth_pin(nullptr) || (is_auth_pinned(object) && has_more() && more()->is_freeze_authpin && more()->rename_inode == object); diff --git a/src/mds/Mutation.h b/src/mds/Mutation.h index d7d74de82dfd2..e1b905b33d8cb 100644 --- a/src/mds/Mutation.h +++ b/src/mds/Mutation.h @@ -405,7 +405,7 @@ struct MDRequestImpl : public MutationImpl { bool freeze_auth_pin(CInode *inode); void unfreeze_auth_pin(bool clear_inode=false); void set_remote_frozen_auth_pin(CInode *inode); - bool can_auth_pin(MDSCacheObject *object, bool bypassfreezing=false); + bool can_auth_pin(MDSCacheObject *object); void drop_local_auth_pins(); void set_ambiguous_auth(CInode *inode); void clear_ambiguous_auth(); diff --git a/src/mds/Server.cc b/src/mds/Server.cc index 589abf9e8de7b..e46f69b3297ca 100644 --- a/src/mds/Server.cc +++ b/src/mds/Server.cc @@ -3171,13 +3171,9 @@ void Server::handle_peer_auth_pin(const MDRequestRef& mdr) list objects; CInode *auth_pin_freeze = NULL; bool nonblocking = mdr->peer_request->is_nonblocking(); - bool bypassfreezing = mdr->peer_request->is_bypassfreezing(); bool fail = false, wouldblock = false, readonly = false; ref_t reply; - dout(15) << " nonblocking=" << nonblocking - << " bypassfreezing=" << bypassfreezing << dendl; - if (mdcache->is_readonly()) { dout(10) << " read-only FS" << dendl; readonly = true; @@ -3209,7 +3205,7 @@ void Server::handle_peer_auth_pin(const MDRequestRef& mdr) } if (mdr->is_auth_pinned(obj)) continue; - if (!mdr->can_auth_pin(obj, bypassfreezing)) { + if (!mdr->can_auth_pin(obj)) { if (nonblocking) { dout(10) << " can't auth_pin (freezing?) " << *obj << " nonblocking" << dendl; fail = true; diff --git a/src/messages/MMDSPeerRequest.h b/src/messages/MMDSPeerRequest.h index 1a007364d4abb..1799ab361a115 100644 --- a/src/messages/MMDSPeerRequest.h +++ b/src/messages/MMDSPeerRequest.h @@ -106,7 +106,6 @@ public: static constexpr unsigned FLAG_INTERRUPTED = 1<<5; static constexpr unsigned FLAG_NOTIFYBLOCKING = 1<<6; static constexpr unsigned FLAG_REQBLOCKED = 1<<7; - static constexpr unsigned FLAG_BYPASSFREEZING = 1<<8; // for locking __u16 lock_type; // lock object type @@ -161,8 +160,6 @@ public: void clear_notify_blocking() const { flags &= ~FLAG_NOTIFYBLOCKING; } bool is_req_blocked() const { return (flags & FLAG_REQBLOCKED); } void mark_req_blocked() { flags |= FLAG_REQBLOCKED; } - bool is_bypassfreezing() const { return (flags & FLAG_BYPASSFREEZING); } - void mark_bypassfreezing() { flags |= FLAG_BYPASSFREEZING; } void set_lock_type(int t) { lock_type = t; } const ceph::buffer::list& get_lock_data() const { return inode_export; } -- 2.39.5