Merge pull request ceph#64995 from ifed01/wip-ifed-fix-snapdiff-fragment

mds: fix snapdiff result fragmentation

Reviewed-by: Venky Shankar <[email protected]>

Author: vshankar

src/mds/CDir.h

@@ -352,6 +352,7 @@ class CDir : public MDSCacheObject, public Counter<CDir> {
   dentry_key_map::iterator begin() { return items.begin(); }
   dentry_key_map::iterator end() { return items.end(); }
   dentry_key_map::iterator lower_bound(dentry_key_t key) { return items.lower_bound(key); }
+  dentry_key_map::iterator upper_bound(dentry_key_t key) { return items.upper_bound(key); }
 
   unsigned get_num_head_items() const { return num_head_items; }
   unsigned get_num_head_null() const { return num_head_null; }

src/mds/Server.cc

@@ -12550,7 +12550,7 @@ void Server::handle_client_readdir_snapdiff(const MDRequestRef& mdr)
     offset_hash = (__u32)req->head.args.snapdiff.offset_hash;
   }
 
-  dout(10) << " frag " << fg << " offset '" << offset_str << "'"
+  dout(10) << __func__ << " frag " << fg << " offset '" << offset_str << "'"
     << " offset_hash " << offset_hash << " flags " << req_flags << dendl;
 
   // does the frag exist?
@@ -12707,8 +12707,18 @@ void Server::_readdir_diff(
     std::swap(snapid, snapid_prev);
   }
   bool from_the_beginning = !offset_hash && offset_str.empty();
-  // skip all dns < dentry_key_t(snapid, offset_str, offset_hash)
-  dentry_key_t skip_key(snapid_prev, offset_str.c_str(), offset_hash);
+  // skip all dns <= dentry_key_t(*, offset_str, offset_hash)
+  dentry_key_t skip_key(CEPH_NOSNAP, offset_str.c_str(), offset_hash);
+
+  // We need to rollback all the entries with the same name
+  // when some entries with this name don't fit into the same fragment.
+  // This is caused by the limited ability for offset provisioning between
+  // fragments - there is no way to identify specific snapshot for the last entry.
+  // The following vars denote the potential rollback position for such a case.
+  // Fixes: https://tracker.ceph.com/issues/72518
+  string last_name;
+  size_t rollback_pos = 0;
+  size_t rollback_num = 0;
 
   bool end = build_snap_diff(
     mdr,
@@ -12727,7 +12737,16 @@ void Server::_readdir_diff(
       effective_snapid = exists ? snapid : snapid_prev;
       name.append(dn_name);
       if ((int)(dnbl.length() + name.length() + sizeof(__u32) + sizeof(LeaseStat)) > bytes_left) {
-	dout(10) << " ran out of room, stopping at " << dnbl.length() << " < " << bytes_left << dendl;
+	dout(10) << " ran out of room for name, stopping at " << dnbl.length() << " < " << bytes_left << dendl;
+        if (name == last_name) {
+	  bufferlist keep;
+	  keep.substr_of(dnbl, 0, rollback_pos);
+	  dnbl.swap(keep);
+          last_name.clear();
+          rollback_pos = 0;
+          numfiles = rollback_num;
+          rollback_num = 0;
+        }
 	return false;
       }
 
@@ -12736,6 +12755,7 @@ void Server::_readdir_diff(
       unsigned start_len = dnbl.length();
       dout(10) << "inc dn " << *dn << " as " << name
                << std::hex << " hash 0x" << hash << std::dec
+               << " " << effective_snapid
                << dendl;
       encode(name, dnbl);
       mds->locker->issue_client_lease(dn, in, mdr, now, dnbl);
@@ -12748,11 +12768,24 @@ void Server::_readdir_diff(
 	dout(10) << " ran out of room, stopping at "
 	         << start_len << " < " << bytes_left << dendl;
 	bufferlist keep;
-	keep.substr_of(dnbl, 0, start_len);
+
+	keep.substr_of(dnbl, 0,
+          name == last_name ? rollback_pos : start_len);
 	dnbl.swap(keep);
+
+        last_name.clear();
+        rollback_pos = 0;
+        numfiles = rollback_num;
+        rollback_num = 0;
 	return false;
       }
 
+      // set rollback position
+      if (name != last_name) {
+        last_name = name;
+        rollback_pos = start_len;
+        rollback_num = numfiles;
+      }
       // touch dn
       mdcache->lru.lru_touch(dn);
       ++numfiles;
@@ -12800,7 +12833,7 @@ bool Server::build_snap_diff(
     return r;
   };
 
-  auto it = !skip_key ? dir->begin() : dir->lower_bound(*skip_key);
+  auto it = !skip_key ? dir->begin() : dir->upper_bound(*skip_key);
 
   while(it != dir->end()) {
     CDentry* dn = it->second;
@@ -12821,11 +12854,6 @@ bool Server::build_snap_diff(
       dout(20) << __func__ << " not in range, skipping" << dendl;
       continue;
     }
-    if (skip_key) {
-      skip_key->snapid = dn->last;
-      if (!(*skip_key < dn->key()))
-	continue;
-    }
 
     CInode* in = dnl->get_inode();
     if (in && in->ino() == CEPH_INO_CEPH)
@@ -12864,7 +12892,6 @@ bool Server::build_snap_diff(
     ceph_assert(in);
 
     utime_t mtime = in->get_inode()->mtime;
-
     if (in->is_dir()) {
 
       // we need to maintain the order of entries (determined by their name hashes)

src/test/libcephfs/snapdiff.cc

@@ -34,6 +34,7 @@
 #include <dirent.h>
 #include <optional>
 #include <random>
+#include <string.h>
 
 using namespace std;
 class TestMount {
@@ -2137,3 +2138,57 @@ TEST(LibCephFS, SnapDiffChangedBlockWithCustomObjectSize)
   ASSERT_EQ(0, test_mount.rmsnap("snap1"));
   ASSERT_EQ(0, test_mount.rmsnap("snap2"));
 }
+
+TEST(LibCephFS, SnapDiffDeletionRecreation) {
+  int bulk_count = 1 << 15;
+  TestMount test_mount("/SnapdiffDeletionRecreation");
+
+  ASSERT_EQ(0, test_mount.mkdir("bulk"));
+  ASSERT_EQ(0, test_mount.mkdir("test"));
+
+  int i;
+  char path[PATH_MAX];
+  for (i = 0; i < bulk_count; i++) {
+    snprintf(path, PATH_MAX - 1, "bulk/%d", i);
+    test_mount.write_full(path, path);
+  }
+  ASSERT_EQ(0, test_mount.mksnap("snap1"));
+  // creation of snap1 done
+
+  for (i = 0; i < bulk_count / 2; ++i) {
+    snprintf(path, PATH_MAX - 1, "bulk/%d", i);
+    ASSERT_EQ(0, test_mount.unlink(path));
+    if (i >= bulk_count / 4) {
+      ASSERT_EQ(0, test_mount.mkdir(path));
+    }
+  }
+  for (i = bulk_count; i < 2 * bulk_count; ++i) {
+    snprintf(path, PATH_MAX - 1, "bulk/%d", i);
+    test_mount.write_full(path, path);
+  }
+  ASSERT_EQ(0, test_mount.mksnap("snap2"));
+
+  uint64_t snapid1;
+  uint64_t snapid2;
+
+  // learn snapshot ids and do basic verification
+  ASSERT_EQ(0, test_mount.get_snapid("snap1", &snapid1));
+  ASSERT_EQ(0, test_mount.get_snapid("snap2", &snapid2));
+
+  vector <pair <string, uint64_t>> expected;
+  for (int i = 0; i < 2 * bulk_count; ++i) {
+    string dir = to_string(i);
+    if (i < bulk_count / 4) {
+      expected.push_back({dir, snapid1});
+    } else if (i >= bulk_count / 4 && i < bulk_count / 2) {
+      expected.push_back({dir, snapid1});
+      expected.push_back({dir, snapid2});
+    } else if (i >= bulk_count) {
+      expected.push_back({dir, snapid2});
+    }
+  }
+  test_mount.verify_snap_diff(expected, "bulk", "snap1", "snap2");
+
+  test_mount.rmsnap("snap1");
+  test_mount.rmsnap("snap2");
+}