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 1a921309ca0..435e7edb028 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 9f5f1a49f55..8694dfc72a2 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 837ce2b8818..2566395d185 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 025e4d17b81..3828fe04d7b 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 b70725f714e..76ac7e21cc0 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 49a2abd83b4..8fe15c0047f 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 45555a07d8b..254ca7efc65 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 7bc820e4234..d322a05851a 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 e94a988703f..b7769319cb1 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 d7d74de82df..e1b905b33d8 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 589abf9e8de..e46f69b3297 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 1a007364d4a..1799ab361a1 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