Merge pull request #50041 from cbodley/wip-cls-rgw-cancel-deleted

cls/rgw: remove index entry after cancelling last racing delete op Reviewed-by: J. Eric Ivancich <[email protected]> Reviewed-by: Cory Snyder <[email protected]>
ceph · Feb 17, 2023 · e556024 · e556024
2 parents bbd5415 + 5ddde6e
commit e556024
Show file tree

Hide file tree

Showing 2 changed files with 114 additions and 12 deletions.
diff --git a/src/cls/rgw/cls_rgw.cc b/src/cls/rgw/cls_rgw.cc
@@ -1172,18 +1172,32 @@ int rgw_bucket_complete_op(cls_method_context_t hctx, bufferlist *in, bufferlist
   if (op.op == CLS_RGW_OP_CANCEL) {
     log_op = false; // don't log cancelation
     if (op.tag.size()) {
-      // we removed this tag from pending_map so need to write the changes
-      CLS_LOG_BITX(bitx_inst, 20,
-		   "INFO: %s: setting map entry at key=%s",
-		   __func__, escape_str(idx).c_str());
-      bufferlist new_key_bl;
-      encode(entry, new_key_bl);
-      rc = cls_cxx_map_set_val(hctx, idx, &new_key_bl);
-      if (rc < 0) {
-	CLS_LOG_BITX(bitx_inst, 1,
-		     "ERROR: %s: unable to set map val, key=%s, rc=%d",
-		     __func__, escape_str(idx).c_str(), rc);
-        return rc;
+      if (!entry.exists && entry.pending_map.empty()) {
+        // a racing delete succeeded, and we canceled the last pending op
+        CLS_LOG_BITX(bitx_inst, 20,
+                     "INFO: %s: removing map entry with key=%s",
+                     __func__, escape_str(idx).c_str());
+        rc = cls_cxx_map_remove_key(hctx, idx);
+        if (rc < 0) {
+          CLS_LOG_BITX(bitx_inst, 1,
+                       "ERROR: %s: unable to remove map key, key=%s, rc=%d",
+                       __func__, escape_str(idx).c_str(), rc);
+          return rc;
+        }
+      } else {
+        // we removed this tag from pending_map so need to write the changes
+        CLS_LOG_BITX(bitx_inst, 20,
+                     "INFO: %s: setting map entry at key=%s",
+                     __func__, escape_str(idx).c_str());
+        bufferlist new_key_bl;
+        encode(entry, new_key_bl);
+        rc = cls_cxx_map_set_val(hctx, idx, &new_key_bl);
+        if (rc < 0) {
+          CLS_LOG_BITX(bitx_inst, 1,
+                       "ERROR: %s: unable to set map val, key=%s, rc=%d",
+                       __func__, escape_str(idx).c_str(), rc);
+          return rc;
+        }
       }
     }
   } // CLS_RGW_OP_CANCEL

diff --git a/src/test/cls_rgw/test_cls_rgw.cc b/src/test/cls_rgw/test_cls_rgw.cc
@@ -1252,3 +1252,91 @@ TEST_F(cls_rgw, bi_log_trim)
     EXPECT_FALSE(truncated);
   }
 }
+
+TEST_F(cls_rgw, index_racing_removes)
+{
+  string bucket_oid = str_int("bucket", 8);
+
+  ObjectWriteOperation op;
+  cls_rgw_bucket_init_index(op);
+  ASSERT_EQ(0, ioctx.operate(bucket_oid, &op));
+
+  int epoch = 0;
+  rgw_bucket_dir_entry dirent;
+  rgw_bucket_dir_entry_meta meta;
+
+  // prepare/complete add for single object
+  const cls_rgw_obj_key obj{"obj"};
+  std::string loc = "loc";
+  {
+    std::string tag = "tag-add";
+    index_prepare(ioctx, bucket_oid, CLS_RGW_OP_ADD, tag, obj, loc);
+    index_complete(ioctx, bucket_oid, CLS_RGW_OP_ADD, tag, ++epoch, obj, meta);
+    test_stats(ioctx, bucket_oid, RGWObjCategory::None, 1, 0);
+  }
+
+  // list to verify no pending ops
+  {
+    std::map<int, rgw_cls_list_ret> results;
+    list_entries(ioctx, bucket_oid, 1, results);
+    ASSERT_EQ(1, results.size());
+    const auto& entries = results.begin()->second.dir.m;
+    ASSERT_EQ(1, entries.size());
+    dirent = std::move(entries.begin()->second);
+    ASSERT_EQ(obj, dirent.key);
+    ASSERT_TRUE(dirent.exists);
+    ASSERT_TRUE(dirent.pending_map.empty());
+  }
+
+  // prepare three racing removals
+  std::string tag1 = "tag-rm1";
+  std::string tag2 = "tag-rm2";
+  std::string tag3 = "tag-rm3";
+  index_prepare(ioctx, bucket_oid, CLS_RGW_OP_DEL, tag1, obj, loc);
+  index_prepare(ioctx, bucket_oid, CLS_RGW_OP_DEL, tag2, obj, loc);
+  index_prepare(ioctx, bucket_oid, CLS_RGW_OP_DEL, tag3, obj, loc);
+
+  test_stats(ioctx, bucket_oid, RGWObjCategory::None, 1, 0);
+
+  // complete on tag2
+  index_complete(ioctx, bucket_oid, CLS_RGW_OP_DEL, tag2, ++epoch, obj, meta);
+  {
+    std::map<int, rgw_cls_list_ret> results;
+    list_entries(ioctx, bucket_oid, 1, results);
+    ASSERT_EQ(1, results.size());
+    const auto& entries = results.begin()->second.dir.m;
+    ASSERT_EQ(1, entries.size());
+    dirent = std::move(entries.begin()->second);
+    ASSERT_EQ(obj, dirent.key);
+    ASSERT_FALSE(dirent.exists);
+    ASSERT_FALSE(dirent.pending_map.empty());
+  }
+
+  // cancel on tag1
+  index_complete(ioctx, bucket_oid, CLS_RGW_OP_CANCEL, tag1, ++epoch, obj, meta);
+  {
+    std::map<int, rgw_cls_list_ret> results;
+    list_entries(ioctx, bucket_oid, 1, results);
+    ASSERT_EQ(1, results.size());
+    const auto& entries = results.begin()->second.dir.m;
+    ASSERT_EQ(1, entries.size());
+    dirent = std::move(entries.begin()->second);
+    ASSERT_EQ(obj, dirent.key);
+    ASSERT_FALSE(dirent.exists);
+    ASSERT_FALSE(dirent.pending_map.empty());
+  }
+
+  // final cancel on tag3
+  index_complete(ioctx, bucket_oid, CLS_RGW_OP_CANCEL, tag3, ++epoch, obj, meta);
+
+  // verify that the key was removed
+  {
+    std::map<int, rgw_cls_list_ret> results;
+    list_entries(ioctx, bucket_oid, 1, results);
+    EXPECT_EQ(1, results.size());
+    const auto& entries = results.begin()->second.dir.m;
+    ASSERT_EQ(0, entries.size());
+  }
+
+  test_stats(ioctx, bucket_oid, RGWObjCategory::None, 0, 0);
+}