From 71a526c01a76d29b3289f2c3601a8643a5c18ba5 Mon Sep 17 00:00:00 2001 From: Venky Shankar Date: Thu, 19 Oct 2023 22:38:42 +0530 Subject: [PATCH] Revert "quincy: ceph_fs.h: add separate owner_{u,g}id fields" This seems to be causing test failures: https://pulpito.ceph.com/yuriw-2023-10-19_16:09:31-smoke-quincy-release-distro-default-smithi/7432399 Where the MDS crashes with In function 'CInode* Server::prepare_new_inode(MDRequestRef&, CDir*, inodeno_t, unsigned int, const file_layout_t*)' 3449: FAILED ceph_assert(_inode->gid != (unsigned)-1) 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x14f) [0x7f991631c89e] 2: /usr/lib/ceph/libceph-common.so.2(+0x27eab0) [0x7f991631cab0] 3: (Server::prepare_new_inode(boost::intrusive_ptr&, CDir*, inodeno_t, unsigned int, file_layout_t const*)+0x2791) [0x55563bec4831] 4: (Server::handle_client_mkdir(boost::intrusive_ptr&)+0x1ec) [0x55563bede88c] Revert this change till the failure is fixed. Signed-off-by: Venky Shankar --- src/client/Client.cc | 11 +----- src/client/MetaRequest.cc | 3 -- src/client/MetaRequest.h | 9 ----- src/include/ceph_fs.h | 66 ++--------------------------------- src/mds/Server.cc | 6 ++-- src/mds/cephfs_features.cc | 2 -- src/mds/cephfs_features.h | 5 +-- src/messages/MClientRequest.h | 23 +++--------- 8 files changed, 12 insertions(+), 113 deletions(-) diff --git a/src/client/Client.cc b/src/client/Client.cc index 58bcd60492beb..47d4ab6b5c347 100644 --- a/src/client/Client.cc +++ b/src/client/Client.cc @@ -2553,7 +2553,7 @@ ref_t Client::build_client_request(MetaRequest *request, mds_ran } } - auto req = make_message(request->get_op(), session->mds_features); + auto req = make_message(request->get_op(), old_version); req->set_tid(request->tid); req->set_stamp(request->op_stamp); memcpy(&req->head, &request->head, sizeof(ceph_mds_request_head)); @@ -13471,8 +13471,6 @@ int Client::_mknod(Inode *dir, const char *name, mode_t mode, dev_t rdev, MetaRequest *req = new MetaRequest(CEPH_MDS_OP_MKNOD); - req->set_inode_owner_uid_gid(perms.uid(), perms.gid()); - filepath path; dir->make_nosnap_relative_path(path); path.push_dentry(name); @@ -13622,8 +13620,6 @@ int Client::_create(Inode *dir, const char *name, int flags, mode_t mode, MetaRequest *req = new MetaRequest(CEPH_MDS_OP_CREATE); - req->set_inode_owner_uid_gid(perms.uid(), perms.gid()); - filepath path; dir->make_nosnap_relative_path(path); path.push_dentry(name); @@ -13706,9 +13702,6 @@ int Client::_mkdir(Inode *dir, const char *name, mode_t mode, const UserPerm& pe MetaRequest *req = new MetaRequest(is_snap_op ? CEPH_MDS_OP_MKSNAP : CEPH_MDS_OP_MKDIR); - if (!is_snap_op) - req->set_inode_owner_uid_gid(perm.uid(), perm.gid()); - filepath path; dir->make_nosnap_relative_path(path); path.push_dentry(name); @@ -13852,8 +13845,6 @@ int Client::_symlink(Inode *dir, const char *name, const char *target, MetaRequest *req = new MetaRequest(CEPH_MDS_OP_SYMLINK); - req->set_inode_owner_uid_gid(perms.uid(), perms.gid()); - filepath path; dir->make_nosnap_relative_path(path); path.push_dentry(name); diff --git a/src/client/MetaRequest.cc b/src/client/MetaRequest.cc index 6d709db5831d2..3994424e79360 100644 --- a/src/client/MetaRequest.cc +++ b/src/client/MetaRequest.cc @@ -51,9 +51,6 @@ void MetaRequest::dump(Formatter *f) const f->dump_unsigned("num_releases", head.num_releases); f->dump_int("abort_rc", abort_rc); - - f->dump_unsigned("owner_uid", head.owner_uid); - f->dump_unsigned("owner_gid", head.owner_gid); } MetaRequest::~MetaRequest() diff --git a/src/client/MetaRequest.h b/src/client/MetaRequest.h index d6d27ee009fe6..dd3c47481426f 100644 --- a/src/client/MetaRequest.h +++ b/src/client/MetaRequest.h @@ -78,8 +78,6 @@ public: unsafe_target_item(this) { memset(&head, 0, sizeof(head)); head.op = op; - head.owner_uid = -1; - head.owner_gid = -1; } ~MetaRequest(); @@ -153,13 +151,6 @@ public: return v == 0; } - void set_inode_owner_uid_gid(unsigned u, unsigned g) { - /* it makes sense to set owner_{u,g}id only for OPs which create inodes */ - ceph_assert(IS_CEPH_MDS_OP_NEWINODE(head.op)); - head.owner_uid = u; - head.owner_gid = g; - } - // normal fields void set_tid(ceph_tid_t t) { tid = t; } void set_oldest_client_tid(ceph_tid_t t) { head.oldest_client_tid = t; } diff --git a/src/include/ceph_fs.h b/src/include/ceph_fs.h index 542c1d155ad94..07ba579978eab 100644 --- a/src/include/ceph_fs.h +++ b/src/include/ceph_fs.h @@ -428,11 +428,6 @@ enum { CEPH_MDS_OP_RDLOCK_FRAGSSTATS = 0x01507 }; -#define IS_CEPH_MDS_OP_NEWINODE(op) (op == CEPH_MDS_OP_CREATE || \ - op == CEPH_MDS_OP_MKNOD || \ - op == CEPH_MDS_OP_MKDIR || \ - op == CEPH_MDS_OP_SYMLINK) - extern const char *ceph_mds_op_name(int op); #ifndef CEPH_SETATTR_MODE @@ -627,7 +622,7 @@ union ceph_mds_request_args { } __attribute__ ((packed)) lookupino; } __attribute__ ((packed)); -#define CEPH_MDS_REQUEST_HEAD_VERSION 3 +#define CEPH_MDS_REQUEST_HEAD_VERSION 2 /* * Note that any change to this structure must ensure that it is compatible @@ -648,12 +643,9 @@ struct ceph_mds_request_head { __le32 ext_num_retry; /* new count retry attempts */ __le32 ext_num_fwd; /* new count fwd attempts */ - - __le32 struct_len; /* to store size of struct ceph_mds_request_head */ - __le32 owner_uid, owner_gid; /* used for OPs which create inodes */ } __attribute__ ((packed)); -void inline encode(const struct ceph_mds_request_head& h, ceph::buffer::list& bl) { +void inline encode(const struct ceph_mds_request_head& h, ceph::buffer::list& bl, bool old_version) { using ceph::encode; encode(h.version, bl); encode(h.oldest_client_tid, bl); @@ -673,30 +665,14 @@ void inline encode(const struct ceph_mds_request_head& h, ceph::buffer::list& bl encode(h.ino, bl); bl.append((char*)&h.args, sizeof(h.args)); - if (h.version >= 2) { + if (!old_version) { encode(h.ext_num_retry, bl); encode(h.ext_num_fwd, bl); } - - if (h.version >= 3) { - __u32 struct_len = sizeof(struct ceph_mds_request_head); - encode(struct_len, bl); - encode(h.owner_uid, bl); - encode(h.owner_gid, bl); - - /* - * Please, add new fields handling here. - * You don't need to check h.version as we do it - * in decode(), because decode can properly skip - * all unsupported fields if h.version >= 3. - */ - } } void inline decode(struct ceph_mds_request_head& h, ceph::buffer::list::const_iterator& bl) { using ceph::decode; - unsigned struct_end = bl.get_off(); - decode(h.version, bl); decode(h.oldest_client_tid, bl); decode(h.mdsmap_epoch, bl); @@ -717,42 +693,6 @@ void inline decode(struct ceph_mds_request_head& h, ceph::buffer::list::const_it h.ext_num_retry = h.num_retry; h.ext_num_fwd = h.num_fwd; } - - if (h.version >= 3) { - decode(h.struct_len, bl); - struct_end += h.struct_len; - - decode(h.owner_uid, bl); - decode(h.owner_gid, bl); - } else { - /* - * client is old: let's take caller_{u,g}id as owner_{u,g}id - * this is how it worked before adding of owner_{u,g}id fields. - */ - h.owner_uid = h.caller_uid; - h.owner_gid = h.caller_gid; - } - - /* add new fields handling here */ - - /* - * From version 3 we have struct_len field. - * It allows us to properly handle a case - * when client send struct ceph_mds_request_head - * bigger in size than MDS supports. In this - * case we just want to skip all remaining bytes - * at the end. - * - * See also DECODE_FINISH macro. Unfortunately, - * we can't start using it right now as it will be - * an incompatible protocol change. - */ - if (h.version >= 3) { - if (bl.get_off() > struct_end) - throw ::ceph::buffer::malformed_input(DECODE_ERR_PAST(__PRETTY_FUNCTION__)); - if (bl.get_off() < struct_end) - bl += struct_end - bl.get_off(); - } } /* cap/lease release record */ diff --git a/src/mds/Server.cc b/src/mds/Server.cc index 6cb43c90a550d..ff1bf2e4992fa 100644 --- a/src/mds/Server.cc +++ b/src/mds/Server.cc @@ -3445,12 +3445,10 @@ CInode* Server::prepare_new_inode(MDRequestRef& mdr, CDir *dir, inodeno_t useino _inode->mode |= S_ISGID; } } else { - _inode->gid = mdr->client_request->get_owner_gid(); - ceph_assert(_inode->gid != (unsigned)-1); + _inode->gid = mdr->client_request->get_caller_gid(); } - _inode->uid = mdr->client_request->get_owner_uid(); - ceph_assert(_inode->uid != (unsigned)-1); + _inode->uid = mdr->client_request->get_caller_uid(); _inode->btime = _inode->ctime = _inode->mtime = _inode->atime = mdr->get_op_stamp(); diff --git a/src/mds/cephfs_features.cc b/src/mds/cephfs_features.cc index bf7b1f3cc72e2..3ea19e93fab49 100644 --- a/src/mds/cephfs_features.cc +++ b/src/mds/cephfs_features.cc @@ -26,8 +26,6 @@ static const std::array feature_names "notify_session_state", "op_getvxattr", "32bits_retry_fwd", - "new_snaprealm_info", - "has_owner_uidgid", }; static_assert(feature_names.size() == CEPHFS_FEATURE_MAX + 1); diff --git a/src/mds/cephfs_features.h b/src/mds/cephfs_features.h index 6fe114274bb61..44c7b223ab30f 100644 --- a/src/mds/cephfs_features.h +++ b/src/mds/cephfs_features.h @@ -46,9 +46,7 @@ namespace ceph { #define CEPHFS_FEATURE_NOTIFY_SESSION_STATE 16 #define CEPHFS_FEATURE_OP_GETVXATTR 17 #define CEPHFS_FEATURE_32BITS_RETRY_FWD 18 -#define CEPHFS_FEATURE_NEW_SNAPREALM_INFO 19 -#define CEPHFS_FEATURE_HAS_OWNER_UIDGID 20 -#define CEPHFS_FEATURE_MAX 20 +#define CEPHFS_FEATURE_MAX 18 #define CEPHFS_FEATURES_ALL { \ 0, 1, 2, 3, 4, \ @@ -68,7 +66,6 @@ namespace ceph { CEPHFS_FEATURE_NOTIFY_SESSION_STATE, \ CEPHFS_FEATURE_OP_GETVXATTR, \ CEPHFS_FEATURE_32BITS_RETRY_FWD, \ - CEPHFS_FEATURE_HAS_OWNER_UIDGID, \ } #define CEPHFS_METRIC_FEATURES_ALL { \ diff --git a/src/messages/MClientRequest.h b/src/messages/MClientRequest.h index 3a192efd8f8ad..4a6a21d3c5503 100644 --- a/src/messages/MClientRequest.h +++ b/src/messages/MClientRequest.h @@ -38,7 +38,6 @@ #include "include/filepath.h" #include "mds/mdstypes.h" #include "include/ceph_features.h" -#include "mds/cephfs_features.h" #include "messages/MMDSOp.h" #include @@ -74,7 +73,7 @@ private: public: mutable struct ceph_mds_request_head head; /* XXX HACK! */ utime_t stamp; - feature_bitset_t mds_features; + bool peer_old_version = false; struct Release { mutable ceph_mds_request_release item; @@ -111,16 +110,12 @@ protected: MClientRequest() : MMDSOp(CEPH_MSG_CLIENT_REQUEST, HEAD_VERSION, COMPAT_VERSION) { memset(&head, 0, sizeof(head)); - head.owner_uid = -1; - head.owner_gid = -1; } - MClientRequest(int op, feature_bitset_t features = 0) + MClientRequest(int op, bool over=true) : MMDSOp(CEPH_MSG_CLIENT_REQUEST, HEAD_VERSION, COMPAT_VERSION) { memset(&head, 0, sizeof(head)); head.op = op; - mds_features = features; - head.owner_uid = -1; - head.owner_gid = -1; + peer_old_version = over; } ~MClientRequest() final {} @@ -203,8 +198,6 @@ public: int get_op() const { return head.op; } unsigned get_caller_uid() const { return head.caller_uid; } unsigned get_caller_gid() const { return head.caller_gid; } - unsigned get_owner_uid() const { return head.owner_uid; } - unsigned get_owner_gid() const { return head.owner_gid; } const std::vector& get_caller_gid_list() const { return gid_list; } const std::string& get_path() const { return path.get_path(); } @@ -262,16 +255,14 @@ public: * client will just copy the 'head' memory and isn't * that smart to skip them. */ - if (!mds_features.test(CEPHFS_FEATURE_32BITS_RETRY_FWD)) { + if (peer_old_version) { head.version = 1; - } else if (!mds_features.test(CEPHFS_FEATURE_HAS_OWNER_UIDGID)) { - head.version = 2; } else { head.version = CEPH_MDS_REQUEST_HEAD_VERSION; } if (features & CEPH_FEATURE_FS_BTIME) { - encode(head, payload); + encode(head, payload, peer_old_version); } else { struct ceph_mds_request_head_legacy old_mds_head; @@ -292,10 +283,6 @@ public: out << "client_request(" << get_orig_source() << ":" << get_tid() << " " << ceph_mds_op_name(get_op()); - if (IS_CEPH_MDS_OP_NEWINODE(head.op)) { - out << " owner_uid=" << head.owner_uid - << ", owner_gid=" << head.owner_gid; - } if (head.op == CEPH_MDS_OP_GETATTR) out << " " << ccap_string(head.args.getattr.mask); if (head.op == CEPH_MDS_OP_SETATTR) { -- 2.39.5