From 797e92692196122fb9dc26617904379cc777bd2b Mon Sep 17 00:00:00 2001 From: Xiubo Li Date: Mon, 4 Mar 2024 11:38:28 +0800 Subject: [PATCH] mds/client: return -ENODATA when xattr doesn't exist for removexattr The POSIX says we should return -ENODATA when the corresponding attribute doesn't exist when removing it. While there is one exception for the acl ones in the local filesystems, for exmaple for xfs, which will treat it as success. While in the MDS side there have two ways to remove the xattr: sending a CEPH_MDS_OP_SETXATTR request by setting the 'flags' with CEPH_XATTR_REMOVE and just issued a CEPH_MDS_OP_RMXATTR request directly. For the first one it will always return 0 when the corresponding xattr doesn't exist, while for the later one it will return -ENODATA instead, this should be fixed in MDS to make them to be consistent. Added a CEPH_XATTR_REMOVE2 new flags and will return -ENODATA errno directly when the crresponding xattr doesn't exist. Just keeps the old CEPH_XATTR_REMOVE flags to make it to be compatible with old clients. Fixes: https://tracker.ceph.com/issues/64679 Signed-off-by: Xiubo Li --- src/client/Client.cc | 10 ++++++++-- src/include/ceph_fs.h | 1 + src/mds/Server.cc | 7 ++++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/client/Client.cc b/src/client/Client.cc index fffbd11f600..71ec083c929 100644 --- a/src/client/Client.cc +++ b/src/client/Client.cc @@ -13868,7 +13868,7 @@ int Client::_do_setxattr(Inode *in, const char *name, const void *value, int xattr_flags = 0; if (!value) - xattr_flags |= CEPH_XATTR_REMOVE; + xattr_flags |= CEPH_XATTR_REMOVE | CEPH_XATTR_REMOVE2; if (flags & XATTR_CREATE) xattr_flags |= CEPH_XATTR_CREATE; if (flags & XATTR_REPLACE) @@ -13926,6 +13926,7 @@ int Client::_setxattr(Inode *in, const char *name, const void *value, mode_t new_mode = in->mode; if (value) { int ret = posix_acl_equiv_mode(value, size, &new_mode); + ldout(cct, 3) << __func__ << "(" << in->ino << ", \"" << name << "\") = " << ret << dendl; if (ret < 0) return ret; if (ret == 0) { @@ -13975,6 +13976,11 @@ int Client::_setxattr(Inode *in, const char *name, const void *value, ret = -CEPHFS_EOPNOTSUPP; } + if ((!strcmp(name, ACL_EA_ACCESS) || + !strcmp(name, ACL_EA_DEFAULT)) && + ret == -CEPHFS_ENODATA) + ret = 0; + return ret; } @@ -14063,7 +14069,7 @@ int Client::ll_setxattr(Inode *in, const char *name, const void *value, vinodeno_t vino = _get_vino(in); - ldout(cct, 3) << __func__ << " " << vino << " " << name << " size " << size << dendl; + ldout(cct, 3) << __func__ << " " << vino << " " << name << " size " << size << " value " << !!value << dendl; tout(cct) << __func__ << std::endl; tout(cct) << vino.ino.val << std::endl; tout(cct) << name << std::endl; diff --git a/src/include/ceph_fs.h b/src/include/ceph_fs.h index a6ee2737710..57eb18b0d3e 100644 --- a/src/include/ceph_fs.h +++ b/src/include/ceph_fs.h @@ -488,6 +488,7 @@ int ceph_flags_sys2wire(int flags); */ #define CEPH_XATTR_CREATE (1 << 0) #define CEPH_XATTR_REPLACE (1 << 1) +#define CEPH_XATTR_REMOVE2 (1 << 30) #define CEPH_XATTR_REMOVE (1 << 31) /* diff --git a/src/mds/Server.cc b/src/mds/Server.cc index 81b90f823f4..3e35ec322fc 100644 --- a/src/mds/Server.cc +++ b/src/mds/Server.cc @@ -6534,6 +6534,11 @@ int Server::xattr_validate(CInode *cur, const InodeStoreBase::xattr_map_const_pt return -CEPHFS_ENODATA; } + if ((flags & CEPH_XATTR_REMOVE2) && !(xattrs && xattrs->count(mempool::mds_co::string(xattr_name)))) { + dout(10) << "setxattr '" << xattr_name << "' XATTR_REMOVE2 and CEPHFS_ENODATA on " << *cur << dendl; + return -CEPHFS_ENODATA; + } + return 0; } @@ -6743,7 +6748,7 @@ void Server::handle_client_setxattr(const MDRequestRef& mdr) pi.inode->change_attr++; pi.inode->xattr_version++; - if ((flags & CEPH_XATTR_REMOVE)) { + if ((flags & (CEPH_XATTR_REMOVE | CEPH_XATTR_REMOVE2))) { std::invoke(handler->removexattr, this, cur, pi.xattrs, xattr_op); } else { std::invoke(handler->setxattr, this, cur, pi.xattrs, xattr_op); -- 2.39.5