From ed1d3a9a3f471e8e1f3d725e0e347701897ac3ef Mon Sep 17 00:00:00 2001 From: "ye.changwei" Date: Thu, 13 Aug 2026 10:16:55 +0800 Subject: [PATCH] fix: logic error in boundary check for "exactly two partial blocks" during read request merging When a read request is expanded outwards to align with checksum boundaries, `blk_end` becomes a multiple of `csum_block_size`. The original code used `blk_end/dsk.csum_block_size == blk_start/dsk.csum_block_size+1` to determine whether this request with padding crosses exactly two checksum blocks. However, since `blk_end` perfectly aligns with the physical boundary, the division here will inevitably cause a carry, rendering the condition invalid. This incorrectly caused the engine to split an I/O that should have been issued at once into multiple separate I/O requests, increasing unnecessary memcpy overhead. For example, in a real scenario (32KB csum_block_size): - User reads logical range 28KB ~ 36KB (start=28672, end=36864). The write record (wr) spans 16KB ~ 64KB, so the initial logical clipping leaves start and end unchanged. - To verify checksum, the engine expands the physical boundary: blk_start=0, blk_end=65536. - After the second MVCC record clipping on physical boundaries, the effective read boundary of the physical data is 16KB ~ 64KB (blk_start=16384, blk_end=65536). - Physically, these are exactly two adjacent partial blocks (16K~32K and 32K~64K, with padding waste at both ends). For performance, they should be read in a single I/O. - But the original code executed: `65536/32768 (i.e., 2) == 16384/32768 (i.e., 0) + 1`, evaluating to `2 == 1` (False). - As a result, this read was sub-optimally split into two independent I/Os (16K~32K and 32K~64K). Fixing the condition to `(blk_end-1)/dsk.csum_block_size` accurately finds the block that actually contains the last valid byte, restoring the expected read merging behavior of this branch and avoiding breaking the zero-copy fallback optimization. --- src/blockstore/blockstore_read.cpp | 2 +- src/test/test_blockstore.cpp | 73 ++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/blockstore/blockstore_read.cpp b/src/blockstore/blockstore_read.cpp index c39ea0b6..5dc6ae53 100644 --- a/src/blockstore/blockstore_read.cpp +++ b/src/blockstore/blockstore_read.cpp @@ -258,7 +258,7 @@ uint32_t blockstore_impl_t::prepare_read_simple(std::vector & rea blk_end = blk_end > wr->small().offset+wr->small().len ? wr->small().offset+wr->small().len : blk_end; } if ((blk_end-1)/dsk.csum_block_size == blk_start/dsk.csum_block_size || - blk_end/dsk.csum_block_size == blk_start/dsk.csum_block_size+1 && blk_end != end && blk_start != start || + (blk_end-1)/dsk.csum_block_size == blk_start/dsk.csum_block_size+1 && blk_end != end && blk_start != start || blk_end == end && blk_start == start) { // single block, exactly two partial blocks, or any number of full blocks diff --git a/src/test/test_blockstore.cpp b/src/test/test_blockstore.cpp index 880d45b9..77d851f8 100644 --- a/src/test/test_blockstore.cpp +++ b/src/test/test_blockstore.cpp @@ -1396,6 +1396,78 @@ static void test_compact_big_intent_survives_crash() free(buf); } +static void test_read_merge_exact_partial_blocks() +{ + printf("\n-- test_read_merge_exact_partial_blocks\n"); + + bs_test_t test; + test.default_cfg(); + test.config["csum_block_size"] = "32768"; + test.config["perfect_csum_update"] = "0"; + test.config["inmemory_journal"] = "0"; + test.init(); + + // 1. Initial big write to make the object exist + blockstore_op_t op; + op.opcode = BS_OP_WRITE_STABLE; + op.oid = { .inode = 1, .stripe = 0 }; + op.version = 1; + op.offset = 0; + op.len = 128*1024; + op.buf = (uint8_t*)memalign_or_die(MEM_ALIGNMENT, 128*1024); + memset(op.buf, 0x11, 128*1024); + test.exec_op(&op); + assert(op.retval == op.len); + + // Flush to make it stable + test.bs->flusher->request_trim(); + while (test.bs->heap->get_compact_queue_size() || test.bs->flusher->is_active()) + test.ringloop->loop(); + test.bs->flusher->release_trim(); + + // 2. Intent write: 16KB ~ 64KB + blockstore_op_t op2; + op2.opcode = BS_OP_WRITE_STABLE; + op2.oid = { .inode = 1, .stripe = 0 }; + op2.version = 2; + op2.offset = 16384; + op2.len = 49152; + op2.buf = (uint8_t*)memalign_or_die(MEM_ALIGNMENT, 49152); + memset(op2.buf, 0x22, 49152); + test.exec_op(&op2); + assert(op2.retval == op2.len); + + // 3. Read 28KB ~ 36KB + int read_count = 0; + test.sqe_handler = [&](io_uring_sqe *sqe) { + if (sqe->fd == MOCK_DATA_FD) { + // Only count reads that fall within our expected disk range (or just any read) + if (sqe->opcode == IORING_OP_READV || sqe->opcode == IORING_OP_READ) { + read_count++; + } + } + return false; + }; + + blockstore_op_t op3; + op3.opcode = BS_OP_READ; + op3.oid = { .inode = 1, .stripe = 0 }; + op3.version = UINT64_MAX; + op3.offset = 28672; + op3.len = 8192; + op3.buf = (uint8_t*)memalign_or_die(MEM_ALIGNMENT, 8192); + test.exec_op(&op3); + assert(op3.retval == op3.len); + + // Verify exactly 1 read IO was issued + assert(read_count == 1); + + test.sqe_handler = nullptr; + free(op.buf); + free(op2.buf); + free(op3.buf); +} + int main(int narg, char *args[]) { test_simple(); @@ -1418,5 +1490,6 @@ int main(int narg, char *args[]) test_write_no_space_eagain(); test_write_stays_in_same_meta_block(); test_compact_big_intent_survives_crash(); + test_read_merge_exact_partial_blocks(); return 0; } -- 2.47.3