vitalif/vitastor
5
6
Fork 12
Code Issues 4 Pull Requests 6 Actions Releases 111 Wiki Activity

fix: logic error in boundary check for "exactly two partial blocks" during read request merging #80

Merged
vitalif merged 1 commits from changwei.ye/vitastor:fix-blockstore-read-merge-boundary into master 2026-09-05 00:17:40 +03:00
Conversation 0 Commits 1 Files Changed 2 +74 -1
changwei.ye commented 2026-09-01 06:30:58 +03:00
Contributor
Copy Link

Description

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.

Contributor Name

Changwei Ye (Datatom, https://www.datatom.com/)

CLA

### Description 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. ### Contributor Name Changwei Ye (Datatom, https://www.datatom.com/) ### CLA - [x] I accept Vitastor CLA agreement: https://git.yourcmc.ru/vitalif/vitastor/src/branch/master/CLA-en.md
changwei.ye added 1 commit 2026-09-01 06:30:59 +03:00
fix: logic error in boundary check for "exactly two partial blocks" during read request merging ed1d3a9a3f 
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.
changwei.ye requested review from vitalif 2026-09-01 09:29:24 +03:00
vitalif merged commit 4b95d248bf into master 2026-09-05 00:17:40 +03:00
Sign in to join this conversation.
Reviewers
No Reviewers
vitalif
Labels
No items
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
vitalif
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vitalif/vitastor#80
No description provided.
Delete Branch "changwei.ye/vitastor:fix-blockstore-read-merge-boundary"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?