From f4854ad73cd6dd137ace276bd686162cbf11280c Mon Sep 17 00:00:00 2001
From: Xuehan Xu <xuxuehan@qianxin.com>
Date: Tue, 12 Aug 2025 19:45:38 +0800
Subject: [PATCH] crimson/os/seastore/object_data_handler: remove redundant
 indirect mappings at the end of ObjectDataHandler::clone()

Fixes: https://tracker.ceph.com/issues/72539
Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
---
 src/crimson/os/seastore/lba_mapping.h         |  3 ++
 .../os/seastore/object_data_handler.cc        | 11 +++--
 src/crimson/os/seastore/transaction_manager.h | 43 ++++++++++++++-----
 3 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/src/crimson/os/seastore/lba_mapping.h b/src/crimson/os/seastore/lba_mapping.h
index 7828a7e5f8052..8e9cf8462da77 100644
--- a/src/crimson/os/seastore/lba_mapping.h
+++ b/src/crimson/os/seastore/lba_mapping.h
@@ -111,6 +111,9 @@ public:
   bool is_zero_reserved() const {
     return !is_indirect() && get_val().is_zero();
   }
+  bool is_real() const {
+    return !is_indirect() && !get_val().is_zero();
+  }
 
   extent_len_t get_length() const {
     assert(!is_null());
diff --git a/src/crimson/os/seastore/object_data_handler.cc b/src/crimson/os/seastore/object_data_handler.cc
index 8ed405180881b..7a3692e6e78f4 100644
--- a/src/crimson/os/seastore/object_data_handler.cc
+++ b/src/crimson/os/seastore/object_data_handler.cc
@@ -804,7 +804,7 @@ ObjectDataHandler::punch_inner_mappings(
     extent_len_t>(overwrite_range.aligned_begin);
   return ctx.tm.remove_mappings_in_range(
     ctx.t, overwrite_range.aligned_begin,
-    unaligned_len, std::move(first_mapping));
+    unaligned_len, std::move(first_mapping), {});
 }
 
 // The last step in the multi-mapping-hole-punching scenario: remap
@@ -1403,6 +1403,8 @@ ObjectDataHandler::clone_ret ObjectDataHandler::clone(
     }
     return ctx.tm.get_pin(ctx.t, object_data.get_reserved_data_base()
     ).si_then([this, &object_data, &d_object_data, ctx](auto mapping) {
+      auto old_base = object_data.get_reserved_data_base();
+      auto old_len = object_data.get_reserved_data_len();
       return prepare_data_reservation(
 	ctx,
 	d_object_data,
@@ -1435,12 +1437,15 @@ ObjectDataHandler::clone_ret ObjectDataHandler::clone(
 	    object_data.get_reserved_data_len());
 	  return ctx.tm.remove(ctx.t, std::move(*mapping));
 	});
-      }).si_then([ctx, &object_data,
-		  mapping=std::move(mapping)](auto pos) mutable {
+      }).si_then([ctx, &object_data, mapping](auto pos) mutable {
 	auto base = object_data.get_reserved_data_base();
 	auto len = object_data.get_reserved_data_len();
 	return ctx.tm.clone_range(
 	  ctx.t, base, len, std::move(pos), std::move(mapping), false);
+      }).si_then([ctx, mapping, old_base, old_len] {
+	return ctx.tm.remove_mappings_in_range(
+	  ctx.t, old_base, old_len, std::move(mapping), {false, true}
+	).discard_result();
       });
     }).handle_error_interruptible(
       clone_iertr::pass_further{},
diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h
index 2a35c7e34d10a..4f3eac692110b 100644
--- a/src/crimson/os/seastore/transaction_manager.h
+++ b/src/crimson/os/seastore/transaction_manager.h
@@ -1100,6 +1100,10 @@ public:
     });
   }
 
+  struct remove_mappings_param_t {
+    bool cascade_remove_on_indirect = true;
+    bool skip_direct_mapping = false;
+  };
   /*
    * remove_mappings_in_range
    *
@@ -1111,7 +1115,8 @@ public:
     Transaction &t,
     laddr_t start,
     objaddr_t unaligned_len,
-    LBAMapping first_mapping)
+    LBAMapping first_mapping,
+    remove_mappings_param_t params)
   {
     LOG_PREFIX(TransactionManager::remove_mappings_in_range);
     SUBDEBUGT(seastore_tm, "{}~{}, first_mapping: {}",
@@ -1119,20 +1124,36 @@ public:
     auto mapping = co_await first_mapping.refresh();
     while (!mapping.is_end()) {
       assert(mapping.get_key() >= start);
-      auto mapping_end =
-	(mapping.get_key() + mapping.get_length()).checked_to_laddr();
+      auto mapping_end = (mapping.get_key() + mapping.get_length()
+	).checked_to_laddr();
       if (mapping_end > start + unaligned_len) {
 	break;
       }
-      mapping = co_await remove(t, std::move(mapping)
-      ).handle_error_interruptible(
-	punch_mappings_iertr::pass_further{},
-	crimson::ct_error::assert_all{
-	  "remove_mappings_in_range hit invalid error"
-	}
-      );
+      if (params.skip_direct_mapping && mapping.is_real()) {
+	mapping = co_await mapping.next();
+	continue;
+      }
+      if (params.cascade_remove_on_indirect ||
+	  mapping.is_zero_reserved()) {
+	mapping = co_await remove(t, std::move(mapping)
+	).handle_error_interruptible(
+	  punch_mappings_iertr::pass_further{},
+	  crimson::ct_error::assert_all{
+	    "remove_mappings_in_range hit invalid error"
+	  }
+	);
+      } else {
+	mapping = co_await _remove_indirect_mapping_only(
+	  t, std::move(mapping)
+	).handle_error_interruptible(
+	  punch_mappings_iertr::pass_further{},
+	  crimson::ct_error::assert_all{
+	    "remove_mappings_in_range hit invalid error"
+	  }
+	);
+      }
     }
-    co_return std::move(mapping);
+    co_return mapping;
   }
 
   ~TransactionManager();
-- 
2.39.5