Repository navigation
libbdm: handle failed speculative read-ahead correctly - #935
Merged
Merged
Conversation
Retry the requested range when speculative read-ahead fails. Track valid sector counts and invalidate failed fills so incomplete data cannot be returned as a cache hit. Add a host regression test covering read-ahead fallback, failed and short fills, cache hits, write invalidation and direct reads. Replace BDM cache regression test with C
Member
|
Changes lgtm |
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.
Summary
Fix the
libbdmblock cache so a failed or short speculative read-ahead does not cause a valid read request to fail or leave incomplete data marked as a valid cache entry.The
BDMcache currently fills a cache block by requesting 8 sectors even when the caller requested fewer sectors. This read-ahead is an optimization and therefore should not change the result of the original read.Some block devices can reject the larger speculative request even though the range originally requested by the caller is valid. A simple example is a read near the end of a device:
libbdmattempts to read 8 sectors to populate its cache;Previously, the cache did not validate the result of the read-ahead before treating the cache block as populated. This could make the operation appear to succeed using incomplete or invalid cache contents, and could also allow that data to be returned by subsequent cache hits.
Changes
Why this belongs in PS2SDK
This is a correctness issue in
libbdmitself rather than in any particular filesystem, USB driver, or application.The cache performs the larger read internally as an optimization. Callers of the BDM API do not request that additional range and should not have to account for whether the cache decides to perform read-ahead.
Likewise, individual BDM block-device drivers should be allowed to reject a request that exceeds the valid range of their device. They should not need special handling for a cache implementation detail.
Handling the fallback in
libbdmkeeps the expected abstraction:libbdmmay speculatively read additional sectors for caching;requested range;
data.
This makes the behavior consistent for every block device using the libbdm
cache.
Real-world example
This was encountered while using
NJEMUon PS2 with its data stored on a USB mass-storage device.NJEMU performs many small/random reads from large cache files. BDM's 8-sector (4 KiB) read-ahead is useful for this workload, so disabling the BDM cache or read-ahead globally would lose a useful optimization.
However, a small valid read near a device boundary can cause the speculative 8-sector fill to extend beyond the available sectors. The underlying block device is allowed to reject that larger request even though the application's
original read is valid.
With this change, the common case still gets the existing 4 KiB read-ahead, while boundary and error cases fall back to the exact requested range.
This is also applicable to other BDM-backed storage devices and applications;
NJEMU is only the workload that exposed the issue.
Testing
A host-side C regression test has been added which compiles the actual
bd_cache.cagainst a small fault-injecting block-device implementation.The test runs on the host machine, not on the PS2 IOP. This makes the cache logic deterministic and easy to exercise in CI without requiring PS2 hardware or PCSX2.
It verifies:
The test can be run with:
make -C iop/fs/libbdm/tests testIt requires only a standard C99 host compiler.