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 <mbenjamin@redhat.com>

diff --git a/qa/workunits/rgw/test_librgw_file.sh b/qa/workunits/rgw/test_librgw_file.sh
index 8a0f952ad63c240300ca22fff682d793d8d79ae8..ab2ce9a818331923864c361cb8192b5b7053eed5 100755 (executable)
--- 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 de2ac1ea7b354e68953520a373a0bd48dc41376c..0365f3775d4d08b6178740b60fa121692e18316b 100644 (file)
--- 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 de1f2e580ea8522480265999f0cbb4005bbcfb39..f14da4b56623b1b7ef7be22cdb3528a434fb887f 100644 (file)
--- 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) {