From 11d73295030bfd8ac80397862200db35547bf018 Mon Sep 17 00:00:00 2001 From: Hui Xiao Date: Wed, 26 Jan 2022 10:14:56 -0800 Subject: [PATCH] Clarify status-handling logic in BlockBasedTableBuilder::WriteRawBlock (#9393) Summary: **Context:** Inside `BlockBasedTableBuilder::WriteRawBlock`, there are multiple places that change local variables `io_s` and `s` while depend on them. This PR attempts to clarify the relevant logics so that it's easier to read and add places of changing these local variables later (like https://github.com/facebook/rocksdb/pull/9342.) without changing the current behavior. **Summary:** - Shorten the lifetime of local var `io_s` and `s` as much as possible to avoid if-else branches by early return **Test** - Reasoned against original behavior to verify new changes do not break existing behaviors. - Rely on CI tests since we are not changing current behavior. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9393 Reviewed By: pdillinger Differential Revision: D33626095 Pulled By: hx235 fbshipit-source-id: 6184d1e1d85d2650d16617c449971988d062ed3f --- .../block_based/block_based_table_builder.cc | 151 +++++++++--------- 1 file changed, 77 insertions(+), 74 deletions(-) diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index 817c81544..8c2d41f69 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -386,6 +386,7 @@ struct BlockBasedTableBuilder::Rep { } // Never erase an existing I/O status that is not OK. + // Calling this will also SetStatus(ios) void SetIOStatus(IOStatus ios) { if (!ios.ok() && io_status_ok.load(std::memory_order_relaxed)) { // Locking is an overkill for non compression_opts.parallel_threads @@ -395,6 +396,7 @@ struct BlockBasedTableBuilder::Rep { io_status = ios; io_status_ok.store(false, std::memory_order_relaxed); } + SetStatus(ios); } Rep(const BlockBasedTableOptions& table_opt, const TableBuilderOptions& tbo, @@ -1231,92 +1233,94 @@ void BlockBasedTableBuilder::WriteRawBlock(const Slice& block_contents, bool is_top_level_filter_block) { Rep* r = rep_; bool is_data_block = block_type == BlockType::kData; - Status s = Status::OK(); - IOStatus io_s = IOStatus::OK(); StopWatch sw(r->ioptions.clock, r->ioptions.stats, WRITE_RAW_BLOCK_MICROS); handle->set_offset(r->get_offset()); handle->set_size(block_contents.size()); assert(status().ok()); assert(io_status().ok()); - io_s = r->file->Append(block_contents); - if (io_s.ok()) { - std::array trailer; - trailer[0] = type; - uint32_t checksum = ComputeBuiltinChecksumWithLastByte( - r->table_options.checksum, block_contents.data(), block_contents.size(), - /*last_byte*/ type); - EncodeFixed32(trailer.data() + 1, checksum); - - assert(io_s.ok()); - TEST_SYNC_POINT_CALLBACK( - "BlockBasedTableBuilder::WriteRawBlock:TamperWithChecksum", - trailer.data()); - io_s = r->file->Append(Slice(trailer.data(), trailer.size())); - if (io_s.ok()) { - assert(s.ok()); - bool warm_cache; - switch (r->table_options.prepopulate_block_cache) { - case BlockBasedTableOptions::PrepopulateBlockCache::kFlushOnly: - warm_cache = (r->reason == TableFileCreationReason::kFlush); - break; - case BlockBasedTableOptions::PrepopulateBlockCache::kDisable: - warm_cache = false; - break; - default: - // missing case - assert(false); - warm_cache = false; - } - if (warm_cache) { - if (type == kNoCompression) { - s = InsertBlockInCacheHelper(block_contents, handle, block_type, - is_top_level_filter_block); - } else if (raw_block_contents != nullptr) { - s = InsertBlockInCacheHelper(*raw_block_contents, handle, block_type, - is_top_level_filter_block); - } - if (!s.ok()) { - r->SetStatus(s); - } + + { + IOStatus io_s = r->file->Append(block_contents); + if (!io_s.ok()) { + r->SetIOStatus(io_s); + return; + } + } + + std::array trailer; + trailer[0] = type; + uint32_t checksum = ComputeBuiltinChecksumWithLastByte( + r->table_options.checksum, block_contents.data(), block_contents.size(), + /*last_byte*/ type); + EncodeFixed32(trailer.data() + 1, checksum); + TEST_SYNC_POINT_CALLBACK( + "BlockBasedTableBuilder::WriteRawBlock:TamperWithChecksum", + trailer.data()); + { + IOStatus io_s = r->file->Append(Slice(trailer.data(), trailer.size())); + if (!io_s.ok()) { + r->SetIOStatus(io_s); + return; + } + } + + { + Status s = Status::OK(); + bool warm_cache; + switch (r->table_options.prepopulate_block_cache) { + case BlockBasedTableOptions::PrepopulateBlockCache::kFlushOnly: + warm_cache = (r->reason == TableFileCreationReason::kFlush); + break; + case BlockBasedTableOptions::PrepopulateBlockCache::kDisable: + warm_cache = false; + break; + default: + // missing case + assert(false); + warm_cache = false; + } + if (warm_cache) { + if (type == kNoCompression) { + s = InsertBlockInCacheHelper(block_contents, handle, block_type, + is_top_level_filter_block); + } else if (raw_block_contents != nullptr) { + s = InsertBlockInCacheHelper(*raw_block_contents, handle, block_type, + is_top_level_filter_block); } - // TODO:: Should InsertBlockInCompressedCache take into account error from - // InsertBlockInCache or ignore and overwrite it. - s = InsertBlockInCompressedCache(block_contents, type, handle); if (!s.ok()) { r->SetStatus(s); + return; } + } + s = InsertBlockInCompressedCache(block_contents, type, handle); + if (!s.ok()) { + r->SetStatus(s); + return; + } + } + + r->set_offset(r->get_offset() + block_contents.size() + kBlockTrailerSize); + if (r->table_options.block_align && is_data_block) { + size_t pad_bytes = + (r->alignment - + ((block_contents.size() + kBlockTrailerSize) & (r->alignment - 1))) & + (r->alignment - 1); + IOStatus io_s = r->file->Pad(pad_bytes); + if (io_s.ok()) { + r->set_offset(r->get_offset() + pad_bytes); } else { r->SetIOStatus(io_s); + return; } - if (s.ok() && io_s.ok()) { - r->set_offset(r->get_offset() + block_contents.size() + - kBlockTrailerSize); - if (r->table_options.block_align && is_data_block) { - size_t pad_bytes = - (r->alignment - ((block_contents.size() + kBlockTrailerSize) & - (r->alignment - 1))) & - (r->alignment - 1); - io_s = r->file->Pad(pad_bytes); - if (io_s.ok()) { - r->set_offset(r->get_offset() + pad_bytes); - } else { - r->SetIOStatus(io_s); - } - } - if (r->IsParallelCompressionEnabled()) { - if (is_data_block) { - r->pc_rep->file_size_estimator.ReapBlock(block_contents.size(), - r->get_offset()); - } else { - r->pc_rep->file_size_estimator.SetEstimatedFileSize(r->get_offset()); - } - } - } - } else { - r->SetIOStatus(io_s); } - if (!io_s.ok() && s.ok()) { - r->SetStatus(io_s); + + if (r->IsParallelCompressionEnabled()) { + if (is_data_block) { + r->pc_rep->file_size_estimator.ReapBlock(block_contents.size(), + r->get_offset()); + } else { + r->pc_rep->file_size_estimator.SetEstimatedFileSize(r->get_offset()); + } } } @@ -1800,7 +1804,6 @@ void BlockBasedTableBuilder::WriteFooter(BlockHandle& metaindex_block_handle, r->set_offset(r->get_offset() + footer.GetSlice().size()); } else { r->SetIOStatus(ios); - r->SetStatus(ios); } }