Skip to content

Commit

Permalink
crimson/os/seastore: disable crc calculation if end to end data prote…
Browse files Browse the repository at this point in the history
…ction is enabled

Signed-off-by: Myoungwon Oh <[email protected]>
  • Loading branch information
myoungwon committed Jul 22, 2024
1 parent 169a81e commit e92273a
Show file tree
Hide file tree
Showing 11 changed files with 76 additions and 14 deletions.
8 changes: 7 additions & 1 deletion src/crimson/os/seastore/cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1562,7 +1562,13 @@ void Cache::complete_commit(
is_inline = true;
i->set_paddr(final_block_start.add_relative(i->get_paddr()));
}
assert(i->get_last_committed_crc() == i->calc_crc32c());
#ifndef NDEBUG
if (i->get_paddr().is_root() || epm.get_checksum_needed(i->get_paddr())) {
assert(i->get_last_committed_crc() == i->calc_crc32c());
} else {
assert(i->get_last_committed_crc() == CRC_NULL);
}
#endif
i->pending_for_transaction = TRANS_ID_NULL;
i->on_initial_write();

Expand Down
8 changes: 6 additions & 2 deletions src/crimson/os/seastore/cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -1651,7 +1651,7 @@ class Cache {
extent->get_length(),
extent->get_bptr()
).safe_then(
[extent=std::move(extent)]() mutable {
[extent=std::move(extent), this]() mutable {
LOG_PREFIX(Cache::read_extent);
if (likely(extent->state == CachedExtent::extent_state_t::CLEAN_PENDING)) {
extent->state = CachedExtent::extent_state_t::CLEAN;
Expand All @@ -1662,7 +1662,11 @@ class Cache {
if (extent->is_valid()) {
// crc will be checked against LBA leaf entry for logical extents,
// or check against in-extent crc for physical extents.
extent->last_committed_crc = extent->calc_crc32c();
if (epm.get_checksum_needed(extent->get_paddr())) {
extent->last_committed_crc = extent->calc_crc32c();
} else {
extent->last_committed_crc = CRC_NULL;
}
extent->on_clean_read();
}
extent->complete_io();
Expand Down
9 changes: 9 additions & 0 deletions src/crimson/os/seastore/extent_placement_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,15 @@ class ExtentPlacementManager {
return background_process.run_until_halt();
}

bool get_checksum_needed(paddr_t addr) {
// checksum offloading only for blocks physically stored in the device
if (addr.is_fake()) {
return true;
}
assert(addr.is_absolute());
return !devices_by_id[addr.get_device_id()]->is_end_to_end_data_protection();
}

private:
rewrite_gen_t adjust_generation(
data_category_t category,
Expand Down
2 changes: 2 additions & 0 deletions src/crimson/os/seastore/journal.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ class Journal {
virtual ~Journal() {}

virtual backend_type_t get_type() = 0;

virtual bool is_checksum_needed() = 0;
};
using JournalRef = std::unique_ptr<Journal>;

Expand Down
4 changes: 4 additions & 0 deletions src/crimson/os/seastore/journal/circular_bounded_journal.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,10 @@ class CircularBoundedJournal : public Journal, RecordScanner {
return get_journal_end();
}

bool is_checksum_needed() final {
return cjs.is_checksum_needed();
}

// Test interfaces

CircularJournalSpace& get_cjs() {
Expand Down
4 changes: 4 additions & 0 deletions src/crimson/os/seastore/journal/circular_journal_space.h
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,10 @@ class CircularJournalSpace : public JournalAllocator {
return header;
}

bool is_checksum_needed() {
return !device->is_end_to_end_data_protection();
}

private:
std::string print_name;
cbj_header_t header;
Expand Down
5 changes: 5 additions & 0 deletions src/crimson/os/seastore/journal/segmented_journal.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ class SegmentedJournal : public Journal {
return seastar::now();
}

bool is_checksum_needed() final {
// segmented journal always requires checksum
return true;
}

private:
submit_record_ret do_submit_record(
record_t &&record,
Expand Down
1 change: 0 additions & 1 deletion src/crimson/os/seastore/lba_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ LBAManager::update_mappings(
{
return trans_intr::do_for_each(extents,
[this, &t](auto &extent) {
assert(extent->get_last_committed_crc());
return update_mapping(
t,
extent->get_laddr(),
Expand Down
1 change: 1 addition & 0 deletions src/crimson/os/seastore/seastore_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ inline depth_le_t init_depth_le(uint32_t i) {
}

using checksum_t = uint32_t;
constexpr checksum_t CRC_NULL = 0;

// Immutable metadata for seastore to set at mkfs time
struct seastore_meta_t {
Expand Down
41 changes: 31 additions & 10 deletions src/crimson/os/seastore/transaction_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ TransactionManager::update_lba_mappings(
std::list<LogicalCachedExtentRef>(),
std::list<CachedExtentRef>(),
[this, &t, &pre_allocated_extents](auto &lextents, auto &pextents) {
auto chksum_func = [&lextents, &pextents](auto &extent) {
auto chksum_func = [&lextents, &pextents, this](auto &extent) {
if (!extent->is_valid() ||
!extent->is_fully_loaded() ||
// EXIST_MUTATION_PENDING extents' crc will be calculated when
Expand All @@ -343,10 +343,20 @@ TransactionManager::update_lba_mappings(
// for rewritten extents, last_committed_crc should have been set
// because the crc of the original extent may be reused.
// also see rewrite_logical_extent()
if (!extent->get_last_committed_crc()) {
extent->set_last_committed_crc(extent->calc_crc32c());
}
assert(extent->calc_crc32c() == extent->get_last_committed_crc());
if (!extent->get_last_committed_crc()) {
if (get_checksum_needed(extent->get_paddr())) {
extent->set_last_committed_crc(extent->calc_crc32c());
} else {
extent->set_last_committed_crc(CRC_NULL);
}
}
#ifndef NDEBUG
if (get_checksum_needed(extent->get_paddr())) {
assert(extent->get_last_committed_crc() == extent->calc_crc32c());
} else {
assert(extent->get_last_committed_crc() == CRC_NULL);
}
#endif
lextents.emplace_back(extent->template cast<LogicalCachedExtent>());
} else {
pextents.emplace_back(extent);
Expand All @@ -367,15 +377,20 @@ TransactionManager::update_lba_mappings(

return lba_manager->update_mappings(
t, lextents
).si_then([&pextents] {
).si_then([&pextents, this] {
for (auto &extent : pextents) {
assert(!extent->is_logical() && extent->is_valid());
// for non-logical extents, we update its last_committed_crc
// and in-extent checksum fields
// For pre-allocated fresh physical extents, update in-extent crc.
auto crc = extent->calc_crc32c();
extent->set_last_committed_crc(crc);
extent->update_in_extent_chksum_field(crc);
checksum_t crc;
if (get_checksum_needed(extent->get_paddr())) {
crc = extent->calc_crc32c();
} else {
crc = CRC_NULL;
}
extent->set_last_committed_crc(crc);
extent->update_in_extent_chksum_field(crc);
}
});
});
Expand Down Expand Up @@ -516,7 +531,13 @@ TransactionManager::rewrite_logical_extent(

DEBUGT("rewriting logical extent -- {} to {}", t, *lextent, *nlextent);

assert(lextent->get_last_committed_crc() == lextent->calc_crc32c());
#ifndef NDEBUG
if (get_checksum_needed(lextent->get_paddr())) {
assert(lextent->get_last_committed_crc() == lextent->calc_crc32c());
} else {
assert(lextent->get_last_committed_crc() == CRC_NULL);
}
#endif
nlextent->set_last_committed_crc(lextent->get_last_committed_crc());
/* This update_mapping is, strictly speaking, unnecessary for delayed_alloc
* extents since we're going to do it again once we either do the ool write
Expand Down
7 changes: 7 additions & 0 deletions src/crimson/os/seastore/transaction_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -991,6 +991,13 @@ class TransactionManager : public ExtentCallbackInterface {
});
}

bool get_checksum_needed(paddr_t paddr) {
if (paddr.is_record_relative()) {
return journal->is_checksum_needed();
}
return epm->get_checksum_needed(paddr);
}

public:
// Testing interfaces
auto get_epm() {
Expand Down

0 comments on commit e92273a

Please sign in to comment.