From: Matt Benjamin Date: Sun, 9 Nov 2025 00:36:18 +0000 (-0500) Subject: rgw_file: restore ability of attributes to be...restored X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=71c9f546ce134b6275c7e5328ead6e1129534c03;p=ceph.git rgw_file: restore ability of attributes to be...restored 1. removes incorrect condition on decode_attrs() in lookup/stat paths 2. removes calls to update_fh(...) in those paths Also fix the program options for the second invocation of unit test ceph_test_librgw_nfsns, which was incorrect and caused several assertions to be skipped. Fixes: https://tracker.ceph.com/issues/73761 Signed-off-by: Matt Benjamin --- diff --git a/qa/workunits/rgw/test_librgw_file.sh b/qa/workunits/rgw/test_librgw_file.sh index 8a0f952ad63..ab2ce9a8183 100755 --- a/qa/workunits/rgw/test_librgw_file.sh +++ b/qa/workunits/rgw/test_librgw_file.sh @@ -34,7 +34,7 @@ ceph_test_librgw_file_nfsns ${K} --hier1 --dirs1 --create --rename --verbose # the older librgw_file can consume the namespace echo "phase 1.2" -ceph_test_librgw_file_nfsns ${K} --getattr --verbose +ceph_test_librgw_file_nfsns ${K} --dirs1 --verbose # and delete the hierarchy echo "phase 1.3" diff --git a/src/rgw/rgw_file.cc b/src/rgw/rgw_file.cc index de2ac1ea7b3..0365f3775d4 100644 --- a/src/rgw/rgw_file.cc +++ b/src/rgw/rgw_file.cc @@ -121,10 +121,7 @@ namespace rgw { auto ux_key = req.get_attr(RGW_ATTR_UNIX_KEY1); auto ux_attrs = req.get_attr(RGW_ATTR_UNIX1); if (ux_key && ux_attrs) { - DecodeAttrsResult dar = rgw_fh->decode_attrs(ux_key, ux_attrs); - if (get<0>(dar) || get<1>(dar)) { - update_fh(rgw_fh); - } + [[maybe_unused]] DecodeAttrsResult dar = rgw_fh->decode_attrs(ux_key, ux_attrs); } if (! (flags & RGWFileHandle::FLAG_LOCKED)) { rgw_fh->mtx.unlock(); @@ -213,13 +210,10 @@ namespace rgw { auto ux_attrs = req.get_attr(RGW_ATTR_UNIX1); rgw_fh->set_etag(*(req.get_attr(RGW_ATTR_ETAG))); rgw_fh->set_acls(*(req.get_attr(RGW_ATTR_ACL))); - if (!(flags & RGWFileHandle::FLAG_IN_CB) && - ux_key && ux_attrs) { - DecodeAttrsResult dar = rgw_fh->decode_attrs(ux_key, ux_attrs); - if (get<0>(dar) || get<1>(dar)) { - update_fh(rgw_fh); - } - } + if (ux_key && ux_attrs) { + /* restores unix attrs */ + [[maybe_unused]] DecodeAttrsResult dar = rgw_fh->decode_attrs(ux_key, ux_attrs); + } } goto done; } @@ -250,13 +244,11 @@ namespace rgw { auto ux_attrs = req.get_attr(RGW_ATTR_UNIX1); rgw_fh->set_etag(*(req.get_attr(RGW_ATTR_ETAG))); rgw_fh->set_acls(*(req.get_attr(RGW_ATTR_ACL))); - if (!(flags & RGWFileHandle::FLAG_IN_CB) && - ux_key && ux_attrs) { - DecodeAttrsResult dar = rgw_fh->decode_attrs(ux_key, ux_attrs); - if (get<0>(dar) || get<1>(dar)) { - update_fh(rgw_fh); - } - } + if (ux_key && ux_attrs) { + /* restores unix attrs */ + [[maybe_unused]] DecodeAttrsResult dar = + rgw_fh->decode_attrs(ux_key, ux_attrs); + } /* ux_key && ux_attrs */ } goto done; } @@ -1456,6 +1448,7 @@ namespace rgw { } } /* RGWFileHandle::encode_attrs */ + /* called with rgw_fh->mtx held */ DecodeAttrsResult RGWFileHandle::decode_attrs(const ceph::buffer::list* ux_key1, const ceph::buffer::list* ux_attrs1) { @@ -1475,25 +1468,28 @@ namespace rgw { decode(tmp_fh, bl_iter_unix1); fh.fh_type = tmp_fh.fh.fh_type; - // for file handles that represent files and whose file_ondisk_version - // is newer, no updates are need, otherwise, go updating the current - // file handle - if (!((fh.fh_type == RGW_FS_TYPE_FILE || - fh.fh_type == RGW_FS_TYPE_SYMBOLIC_LINK) && - file_ondisk_version >= tmp_fh.file_ondisk_version)) { - // make sure the following "encode" always encode a greater version - file_ondisk_version = tmp_fh.file_ondisk_version + 1; - state.dev = tmp_fh.state.dev; - state.size = tmp_fh.state.size; - state.nlink = tmp_fh.state.nlink; - state.owner_uid = tmp_fh.state.owner_uid; - state.owner_gid = tmp_fh.state.owner_gid; - state.unix_mode = tmp_fh.state.unix_mode; - state.ctime = tmp_fh.state.ctime; - state.mtime = tmp_fh.state.mtime; - state.atime = tmp_fh.state.atime; - state.version = tmp_fh.state.version; - } + /* XXXX the merged logic for fh versions is not sound--we are decoding + * an on-disk handle, so the most basic scenario of restoring an object + * with saved attributes fails in the common case: make object, setattr, + * restart nfs, consult attr: now on lookup the protype filehandle has + * version 0, and on-disk version is also 0, and attributes are not + * restored. + * + * It seems likely that the reasoning here was influenced by former calls + * to update_fh(...) in stat paths--which in retrospect seems unwise. The + * stat paths (incl. lookups) are reading in attrs, why should we immediately + * write them back? + */ + state.dev = tmp_fh.state.dev; + state.size = tmp_fh.state.size; + state.nlink = tmp_fh.state.nlink; + state.owner_uid = tmp_fh.state.owner_uid; + state.owner_gid = tmp_fh.state.owner_gid; + state.unix_mode = tmp_fh.state.unix_mode; + state.ctime = tmp_fh.state.ctime; + state.mtime = tmp_fh.state.mtime; + state.atime = tmp_fh.state.atime; + state.version = tmp_fh.state.version; if (this->state.version < 2) { get<1>(dar) = true; diff --git a/src/test/librgw_file_nfsns.cc b/src/test/librgw_file_nfsns.cc index de1f2e580ea..f14da4b5662 100644 --- a/src/test/librgw_file_nfsns.cc +++ b/src/test/librgw_file_nfsns.cc @@ -650,6 +650,9 @@ TEST(LibRGW, BAD_DELETES_DIRS1) { TEST(LibRGW, GETATTR_DIRS1) { if (do_dirs1) { + if (verbose) { + std::cout << "\ttesting Unix attributes " << std::endl; + } int rc; struct stat st; for (auto& dirs_rec : dirs_vec) {