From: Matty Williams Date: Tue, 19 May 2026 08:27:02 +0000 (+0100) Subject: osd: Fix bugs with omap support in EC pools X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=20991721bbd52d44aaf0e67ca33b4917a42ab5ed;p=ceph.git osd: Fix bugs with omap support in EC pools Change handling of reads_sent bool in do_read_op. Use errors to determine omap_satisfied in handle_sub_read_reply. When a head object is deleted, and a whiteout is created, a REPLACE op should be used so that the ECOmapJournal knows to handle the operation as a delete. This should avoid the scenario where omap updates that should be applied to a clone, are instead applied to the whiteout object. Fixes: https://tracker.ceph.com/issues/77329 Signed-off-by: Matty Williams --- diff --git a/src/osd/ECBackend.cc b/src/osd/ECBackend.cc index d12ab9d8fc5..4378eec3930 100644 --- a/src/osd/ECBackend.cc +++ b/src/osd/ECBackend.cc @@ -952,9 +952,20 @@ void ECBackend::handle_sub_read_reply( const auto& to_read = rop.to_read.at(oid); const auto& complete = rop.complete.at(oid); bool attrs_satisfied = !to_read.want_attrs || complete.attrs; - bool omap_satisfied = !get_parent()->get_pool().supports_omap() - || (!to_read.want_omap_header && !to_read.want_omap_keys) - || (complete.omap_header && complete.omap_complete); + + bool omap_satisfied = true; + if (get_parent()->get_pool().supports_omap() + && (to_read.want_omap_header || to_read.want_omap_keys)) { + for (const auto &[shard_id, shard_read] : to_read.shard_reads) { + if (shard_read.omap_source) { + if (complete.errors.contains(shard_read.pg_shard)) { + omap_satisfied = false; + } + break; + } + } + } + if (attrs_satisfied && omap_satisfied) { err = ec_impl->minimum_to_decode(want_to_read, have, dummy_minimum, nullptr); @@ -1009,6 +1020,7 @@ void ECBackend::handle_sub_read_reply( rop.to_read.at(oid).shard_reads.clear(); rop.to_read.at(oid).want_attrs = false; rop.to_read.at(oid).want_omap_header = false; + rop.to_read.at(oid).want_omap_keys = false; ++is_complete; } } diff --git a/src/osd/ECCommon.cc b/src/osd/ECCommon.cc index 9982b0ffc05..7b501183ffa 100644 --- a/src/osd/ECCommon.cc +++ b/src/osd/ECCommon.cc @@ -360,8 +360,9 @@ int ECCommon::ReadPipeline::get_remaining_shards( read_request_t &read_request, const bool for_recovery, bool want_attrs, - bool want_omap_header) { - if (want_omap_header) { + bool want_omap_header, + bool want_omap_keys) { + if (want_omap_header || want_omap_keys) { ceph_assert(get_parent()->get_pool().supports_omap()); } @@ -385,9 +386,10 @@ int ECCommon::ReadPipeline::get_remaining_shards( return -EIO; } - bool need_attr_request = want_attrs || want_omap_header; + bool need_attr_request = want_attrs || want_omap_header || want_omap_keys; read_request.want_attrs = want_attrs; read_request.want_omap_header = want_omap_header; + read_request.want_omap_keys = want_omap_keys; // Rather than repeating whole read, we can remove everything we already have. for (auto iter = read_request.shard_reads.begin(); @@ -414,30 +416,85 @@ int ECCommon::ReadPipeline::get_remaining_shards( } if (need_attr_request) { - // This happens if we got an error from the shard where we were requesting - // the attributes from and the recovery does not require any non primary - // shards. The example seen in test was a 2+1 EC being recovered. shards 0 - // and 2 were being requested and read as part of recovery. Shard was reading - // the attributes and failed. The recovery required shard 1, but that does - // not have valid attributes on it, so the attribute read failed. - // This is a pretty obscure case, so no need to optimise that much. Do an - // empty read! - shard_id_set have; - shard_id_map pg_shards(sinfo.get_k_plus_m()); - get_all_avail_shards(hoid, have, pg_shards, for_recovery, error_shards); - for (auto shard : have) { - if (!sinfo.is_nonprimary_shard(shard)) { - shard_read_t shard_read; - shard_read.pg_shard = pg_shards[shard]; - read_request.shard_reads.insert(shard, shard_read_t()); - break; - } + int r = ensure_primary_shard_for_omap(hoid, read_request, for_recovery, error_shards); + if (r != 0) { + return r; } } + if (read_request.shard_reads.empty()) { + dout(0) << __func__ << " no shards to read for " << hoid + << " after filtering already-read extents" << dendl; + return -EIO; + } + return 0; } +int ECCommon::ReadPipeline::ensure_primary_shard_for_omap( + const hobject_t &hoid, + read_request_t &read_request, + const bool for_recovery, + const std::optional> &error_shards) { + + if (!get_parent()->get_pool().supports_omap()) { + return 0; + } + if (!read_request.want_omap_header && !read_request.want_omap_keys) { + return 0; + } + + // Check if there is already a suitable primary-capable shard in shard_reads + for (auto &[shard_id, shard_read] : read_request.shard_reads) { + if (!sinfo.is_nonprimary_shard(shard_id)) { + // Found a primary-capable shard, verify it has clean omap + if (for_recovery) { + const pg_missing_t &missing = get_parent()->get_shard_missing(shard_read.pg_shard); + auto miter = missing.get_items().find(hoid); + if (miter != missing.get_items().end() && miter->second.clean_regions.omap_is_dirty()) { + // This shard has dirty omap, keep looking + continue; + } + } + dout(20) << __func__ << ": existing shard " << shard_id + << " can handle omap read for " << hoid << dendl; + return 0; + } + } + + // No suitable shard exists in shard_reads, need to add one + shard_id_set have; + shard_id_map pg_shards(sinfo.get_k_plus_m()); + get_all_avail_shards(hoid, have, pg_shards, for_recovery, error_shards); + for (auto shard : have) { + if (!sinfo.is_nonprimary_shard(shard)) { + // Check if this shard has clean omap + if (for_recovery) { + const pg_missing_t &missing = get_parent()->get_shard_missing(pg_shards[shard]); + auto miter = missing.get_items().find(hoid); + if (miter != missing.get_items().end() && miter->second.clean_regions.omap_is_dirty()) { + dout(20) << __func__ << ": skipping shard " << shard + << " for " << hoid << " due to dirty omap" << dendl; + continue; + } + } + + // Found a suitable shard, add it to shard_reads + shard_read_t shard_read; + shard_read.pg_shard = pg_shards[shard]; + read_request.shard_reads.insert(shard, shard_read); + dout(20) << __func__ << ": added shard " << shard + << " for omap read of " << hoid << dendl; + return 0; + } + } + + // No suitable shard found + dout(0) << __func__ << " no primary-capable shard with clean omap available for " + << hoid << dendl; + return -EIO; +} + void ECCommon::ReadPipeline::start_read_op( const int priority, map &to_read, @@ -478,35 +535,44 @@ void ECCommon::ReadPipeline::do_read_op(ReadOp &rop) { bool need_attrs = read_request.want_attrs; bool need_omap_header = read_request.want_omap_header; bool need_omap_keys = read_request.want_omap_keys; - if (need_omap_header || need_omap_keys) { + bool need_omap = need_omap_header || need_omap_keys; + if (need_omap) { ceph_assert(get_parent()->get_pool().supports_omap()); } for (auto &&[shard, shard_read]: read_request.shard_reads) { if (need_attrs && !sinfo.is_nonprimary_shard(shard)) { messages[shard_read.pg_shard].attrs_to_read.insert(hoid); + reads_sent = true; need_attrs = false; } - if (need_omap_header && !sinfo.is_nonprimary_shard(shard)) { - messages[shard_read.pg_shard].omap_headers_to_read.insert(hoid); + if (need_omap && !sinfo.is_nonprimary_shard(shard)) { + if (need_omap_header) { + messages[shard_read.pg_shard].omap_headers_to_read.insert(hoid); + reads_sent = true; + } + if (need_omap_keys) { + messages[shard_read.pg_shard].omap_read_from.insert( + {hoid, {read_request.omap_read_from, read_request.omap_max_bytes}}); + reads_sent = true; + } + shard_read.omap_source = true; + need_omap = false; need_omap_header = false; - } - if (need_omap_keys && !sinfo.is_nonprimary_shard(shard)) { - messages[shard_read.pg_shard].omap_read_from.insert( - {hoid, {read_request.omap_read_from, read_request.omap_max_bytes}}); need_omap_keys = false; } if (shard_read.subchunk) { messages[shard_read.pg_shard].subchunks[hoid] = *shard_read.subchunk; + reads_sent = true; } else { static const std::vector default_sub_chunk = {make_pair(0, 1)}; messages[shard_read.pg_shard].subchunks[hoid] = default_sub_chunk; + reads_sent = true; } rop.obj_to_source[hoid].insert(shard_read.pg_shard); rop.source_to_obj[shard_read.pg_shard].insert(hoid); } for (auto &[_, shard_read]: read_request.shard_reads) { - ceph_assert(!shard_read.extents.empty()); rop.debug_log.emplace_back(ECUtil::READ_REQUEST, shard_read.pg_shard, shard_read.extents); for (auto &[start, len]: shard_read.extents) { @@ -516,6 +582,7 @@ void ECCommon::ReadPipeline::do_read_op(ReadOp &rop) { } } ceph_assert(!need_attrs); + ceph_assert(!need_omap); ceph_assert(!need_omap_header); ceph_assert(!need_omap_keys); } @@ -803,11 +870,21 @@ int ECCommon::ReadPipeline::send_all_remaining_reads( dout(10) << __func__ << " want omap_header again" << dendl; } + // Check if we need to read omap_keys again + const bool want_omap_keys = + rop.to_read.at(hoid).want_omap_keys && + (!rop.complete.at(hoid).omap_entries || rop.complete.at(hoid).omap_entries->empty()); + if (want_omap_keys) { + ceph_assert(get_parent()->get_pool().supports_omap()); + dout(10) << __func__ << " want omap_keys again" << dendl; + } + read_request_t &read_request = rop.to_read.at(hoid); // reset the old shard reads, we are going to read them again. read_request.shard_reads.clear(); return get_remaining_shards(hoid, rop.complete.at(hoid), read_request, - rop.for_recovery, want_attrs, want_omap_header); + rop.for_recovery, want_attrs, want_omap_header, + want_omap_keys); } void ECCommon::ReadPipeline::kick_reads() { @@ -825,7 +902,8 @@ bool ec_align_t::operator==(const ec_align_t &other) const { bool ECCommon::shard_read_t::operator==(const shard_read_t &other) const { return extents == other.extents && subchunk == other.subchunk && - pg_shard == other.pg_shard; + pg_shard == other.pg_shard && + omap_source == other.omap_source; } bool ECCommon::read_request_t::operator==(const read_request_t &other) const { @@ -1591,36 +1669,12 @@ void ECCommon::RecoveryBackend::continue_recovery_op( recovery_ops.erase(op.hoid); return; } - if (get_parent()->get_pool().supports_omap()) { - shard_id_set have; - shard_id_map pg_shards(sinfo.get_k_plus_m()); - read_pipeline.get_all_avail_shards(op.hoid, have, pg_shards, true, {}); - bool found_omap_shard = false; - for (const auto shard : have) { - if (!sinfo.is_nonprimary_shard(shard)) { - const pg_missing_t &missing = get_parent()->get_shard_missing(pg_shards[shard]); - auto miter = missing.get_items().find(op.hoid); - if (miter != missing.get_items().end() && miter->second.clean_regions.omap_is_dirty()) { - dout(20) << __func__ << ": skipping shard " << shard - << " for " << op.hoid << " due to dirty omap" << dendl; - continue; - } - shard_read_t shard_read; - shard_read.pg_shard = pg_shards[shard]; - read_request.shard_reads.insert(shard, shard_read); - found_omap_shard = true; - dout(10) << __func__ << ": selected shard " << shard - << " for omap read of " << op.hoid << dendl; - break; - } - } - if (!found_omap_shard) { - dout(10) << __func__ << ": ERROR: no shard with clean omap found for " - << op.hoid << ", canceling recovery" << dendl; - get_parent()->cancel_pull(op.hoid); - recovery_ops.erase(op.hoid); - return; - } + int r2 = read_pipeline.ensure_primary_shard_for_omap( + op.hoid, read_request, true, {}); + if (r2 != 0) { + get_parent()->cancel_pull(op.hoid); + recovery_ops.erase(op.hoid); + return; } if (read_request.shard_reads.empty()) { ceph_assert(op.obc); diff --git a/src/osd/ECCommon.h b/src/osd/ECCommon.h index 1da3390d205..c51a3150ce7 100644 --- a/src/osd/ECCommon.h +++ b/src/osd/ECCommon.h @@ -101,12 +101,14 @@ struct ECCommon { extent_set extents; std::optional>> subchunk; pg_shard_t pg_shard; + bool omap_source = false; bool operator==(const shard_read_t &other) const; void print(std::ostream &os) const { os << "shard_read_t(extents=[" << extents << "]" << ", subchunk=" << subchunk << ", pg_shard=" << pg_shard + << ", omap_source=" << omap_source << ")"; } }; @@ -453,7 +455,20 @@ struct ECCommon { read_request_t &read_request, bool for_recovery, bool want_attrs, - bool want_omap_header); + bool want_omap_header, + bool want_omap_keys); + + /** + * Ensures a primary-capable shard with clean omap is present in shard_reads. + * + * @param error_shards Optional set of shards with errors to exclude + * @return 0 on success, -EIO if no suitable shard found + */ + int ensure_primary_shard_for_omap( + const hobject_t &hoid, + read_request_t &read_request, + bool for_recovery, + const std::optional> &error_shards = std::nullopt); void get_all_avail_shards( const hobject_t &hoid, diff --git a/src/osd/ECTransaction.cc b/src/osd/ECTransaction.cc index 8368a219040..fc366a10f59 100644 --- a/src/osd/ECTransaction.cc +++ b/src/osd/ECTransaction.cc @@ -327,6 +327,9 @@ void ECTransaction::Generate::shards_written(const shard_id_set &shards) { } } +// This function converts a truncate-to-zero operation into a delete+recreate, +// which clears the object's omap data. Therefore, this function should not be +// called for objects with omap data. void ECTransaction::Generate::zero_truncate_to_delete() { ceph_assert(obc); @@ -737,7 +740,10 @@ ECTransaction::Generate::Generate(PGTransaction &t, } if (op.is_none() && op.truncate && op.truncate->first == 0) { - zero_truncate_to_delete(); + // Skip zero_truncate_to_delete if object has omap data to preserve it + if (!obc || !obc->obs.oi.is_omap()) { + zero_truncate_to_delete(); + } } if (op.delete_first) { diff --git a/src/osd/PrimaryLogPG.cc b/src/osd/PrimaryLogPG.cc index 58b661ce856..9a479331b24 100644 --- a/src/osd/PrimaryLogPG.cc +++ b/src/osd/PrimaryLogPG.cc @@ -8503,6 +8503,7 @@ inline int PrimaryLogPG::_delete_oid( oi.set_flag(object_info_t::FLAG_WHITEOUT); ctx->delta_stats.num_whiteouts++; t->create(soid); + ctx->use_replace_op = true; osd->logger->inc(l_osd_tier_whiteout); return 0; } diff --git a/src/test/librados/omap_cxx.cc b/src/test/librados/omap_cxx.cc index effe0a37080..eff925454b8 100644 --- a/src/test/librados/omap_cxx.cc +++ b/src/test/librados/omap_cxx.cc @@ -488,6 +488,82 @@ TEST_P(OmapTest, OmapClear) { ASSERT_TRUE(returned_keys.empty()); } +TEST_P(OmapTest, OmapPreservedAfterTruncateToZero) { + SKIP_IF_CRIMSON(); + + const std::string oid = "test_omap_truncate_to_zero"; + const std::string omap_header = "test_header_for_truncate"; + bufferlist omap_header_bl; + encode(omap_header, omap_header_bl); + + // Create object with data and omap + auto omap_map = get_test_omap_data(); + create_test_object_with_omap(oid, omap_map, omap_header_bl); + + // Verify initial state - object has data + bufferlist read_bl; + int ret = ioctx.read(oid, read_bl, 0, 0); + EXPECT_GT(read_bl.length(), 0); // Object has data + + // Truncate object to zero + ObjectWriteOperation write_op; + write_op.truncate(0); + ret = ioctx.operate(oid, &write_op); + EXPECT_EQ(ret, 0); + + // Verification 1: Object data is truncated to zero + read_bl.clear(); + ret = ioctx.read(oid, read_bl, 0, 0); + EXPECT_EQ(ret, 0); // No data to read + EXPECT_EQ(read_bl.length(), 0); // Zero length + + // Verification 2: Omap header is preserved + bufferlist result_header_bl; + int err = 0; + ObjectReadOperation read_op; + read_op.omap_get_header(&result_header_bl, &err); + ret = ioctx.operate(oid, &read_op, nullptr); + EXPECT_EQ(ret, 0); + EXPECT_EQ(err, 0); + + // Check if header bufferlist has data before decoding + ASSERT_GT(result_header_bl.length(), 0u) + << "Expected omap header to be preserved after truncate to zero, but header is empty"; + + std::string result_header; + decode(result_header, result_header_bl); + EXPECT_EQ(result_header, omap_header) + << "Expected omap header '" << omap_header << "' but got '" << result_header << "'"; + + // Verification 3: All omap keys are preserved + std::map vals_read; + err = 0; + ObjectReadOperation read_op2; + read_op2.omap_get_vals2("", LONG_MAX, &vals_read, nullptr, &err); + ret = ioctx.operate(oid, &read_op2, nullptr); + EXPECT_EQ(ret, 0); + EXPECT_EQ(err, 0); + EXPECT_EQ(vals_read.size(), omap_map.size()) + << "Expected " << omap_map.size() << " omap keys to be preserved after truncate to zero, but got " << vals_read.size(); + + // Verify each key-value pair is preserved + for (const auto& [key, expected_bl] : omap_map) { + ASSERT_TRUE(vals_read.find(key) != vals_read.end()) + << "Expected omap key '" << key << "' to be preserved after truncate to zero, but it is missing"; + + // Check if value bufferlist has data before decoding + ASSERT_GT(vals_read[key].length(), 0u) + << "Expected omap value for key '" << key << "' to be preserved, but value is empty"; + + std::string expected_val, actual_val; + decode(expected_val, expected_bl); + decode(actual_val, vals_read[key]); + EXPECT_EQ(actual_val, expected_val) + << "Expected omap value '" << expected_val << "' for key '" << key << "' but got '" << actual_val << "'"; + } +} + + TEST_P(OmapTest, OmapRecovery) { SKIP_IF_CRIMSON(); turn_balancing_off(); diff --git a/src/test/osd/TestECBackend.cc b/src/test/osd/TestECBackend.cc index ebb8c7869d4..1b56be742a6 100644 --- a/src/test/osd/TestECBackend.cc +++ b/src/test/osd/TestECBackend.cc @@ -990,7 +990,7 @@ TEST(ECCommon, get_remaining_shards) ECCommon::read_result_t read_result(&s); read_result.errors.emplace(pg_shards[missing_shard], -EIO); - pipeline.get_remaining_shards(hoid, read_result, read_request, false, false, false); + pipeline.get_remaining_shards(hoid, read_result, read_request, false, false, false, false); ECCommon::read_request_t ref( to_read, ECCommon::WantAttrs::No, ECCommon::WantOmapHeader::No, @@ -1029,7 +1029,7 @@ TEST(ECCommon, get_remaining_shards) read_result.buffers_read.insert_in_shard(shard_id_t(0), chunk_size/2, bl); read_result.processed_read_requests[shard_id_t(0)].insert(chunk_size/2, bl.length()); - pipeline.get_remaining_shards(hoid, read_result, read_request, false, false, false); + pipeline.get_remaining_shards(hoid, read_result, read_request, false, false, false, false); // The result should be a read request for the first 4k of shard 0, as that // is currently missing.