Make alignment calculation consistent#1903
Merged
Merged
Conversation
…max(logical, physical) native_device::ProbeDioAlignment computed the sector size as max(logical_block_size, physical_block_size). On Azure NVMe (Lsv3) sysfs reports logical=512 / physical=262144 (256 KiB), so the allocator rounded every record I/O up to 256 KiB — ~100x read amplification for read-heavy larger-than-memory workloads that saturate disk IOPS. physical_block_size is a write-RMW optimization hint, not the kernel-required O_DIRECT alignment. Use the required alignment instead: statx STATX_DIOALIGN (preferred, Linux 6.1+), falling back to sysfs logical_block_size; never physical. Windows: BytesPerLogicalSector only. This matches File::GetDeviceAlignment, so the per-File DIO asserts and sector_size() agree. Unify all local-disk devices behind one probe so they cannot diverge: - Add NativeStorageDevice.ProbeSectorSize (Linux -> native probe, else Native32.GetDeviceSectorSize; power-of-two floor 512). Route RandomAccess / Managed / LocalStorage devices through it and drop their per-type static sectorSize cache (which ignored the volume, so multi-volume setups shared one stale value). - Remove dead Native32.GetVolumeSectorSize (also used the wrong max(logical, physical) algorithm). Add a cross-device-type consistency regression test, refresh the stale HardeningSectorSize comment, and rebuild the checked-in linux-x64 native binaries (libnative_device.so + libnative_device_libaio.so). The win-x64 native_device.dll must be rebuilt on Windows (MSVC, see cc/README) to pick up the Windows-branch change; it could not be regenerated in this Linux environment. Garnet's Windows path uses the managed LocalStorageDevice (already logical-only via GetDiskFreeSpace), so it is unaffected by the stale dll. Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Tsavorite’s device-sector-size (direct-I/O alignment) probing so that native and managed local-disk device implementations derive a consistent SectorSize from the same “required DIO alignment” signal (statx STATX_DIOALIGN / logical_block_size, never physical_block_size), and adds a regression test to enforce cross-device consistency.
Changes:
- Unifies managed local-disk device constructors to use
NativeStorageDevice.ProbeSectorSize(...)rather than per-type cached probing. - Updates the native probe (
ProbeDioAlignment) to return required O_DIRECT alignment only (ignore physical block size to avoid read amplification). - Removes unused Windows volume-sector query plumbing from
Native32, and adds a Linux-only regression test ensuring device types agree onSectorSize.
Reviewed changes
Copilot reviewed 7 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/storage/Tsavorite/cs/test/test.hlog/DeviceTests.cs | Updates alignment rationale comment and adds a Linux-only regression test to ensure sector-size consistency across device types. |
| libs/storage/Tsavorite/cs/src/core/Utilities/Native32.cs | Removes unused Windows GetFileInformationByHandleEx(FileStorageInfo) sector-size query helpers; keeps GetDiskFreeSpace logical sector-size probe. |
| libs/storage/Tsavorite/cs/src/core/Device/RandomAccessLocalStorageDevice.cs | Switches sector-size initialization to the shared NativeStorageDevice.ProbeSectorSize and removes static cached sector-size logic. |
| libs/storage/Tsavorite/cs/src/core/Device/NativeStorageDevice.cs | Adds shared ProbeSectorSize API and factors parent-dir materialization used to stabilize probe path resolution. |
| libs/storage/Tsavorite/cs/src/core/Device/ManagedLocalStorageDevice.cs | Switches sector-size initialization to the shared NativeStorageDevice.ProbeSectorSize and removes static cached sector-size logic. |
| libs/storage/Tsavorite/cs/src/core/Device/LocalStorageDevice.cs | Switches sector-size initialization to the shared NativeStorageDevice.ProbeSectorSize and removes static cached sector-size logic. |
| libs/storage/Tsavorite/cc/src/device/native_device.h | Changes Linux/Windows alignment probing to return required O_DIRECT alignment only (prefer STATX_DIOALIGN, fallback to logical_block_size). |
Comment-only cleanup: condense the verbose comments from the prior commit to short, factual notes. No code changes; the native binaries are unaffected (comments do not change generated code) and are intentionally not rebuilt. Co-authored-by: Copilot <[email protected]>
… review) Catch BadImageFormatException/FileLoadException from NativeDevice_ProbeAlignment so a present-but-wrong-arch/corrupt libnative_device falls back to the managed probe instead of breaking managed local-disk device construction. Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
TedHartMS
approved these changes
Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.