handle out-of-order write completions in verify state (Issue #1950)#1951
Merged
Conversation
axboe
reviewed
Jul 27, 2025
Owner
|
I think this looks pretty good. I did notice that you're missing signed-off-by in all your commit messages, though. |
io_u->numberio is used to keep track of the sequence number of writes and verify reads. It is entirely feasible to issue millions or even billions of IOs in a singe load, so let's use enough bits to handle that. numberio is copied into io_piece and verify_header, so update those structs accordingly. Signed-off-by: Riley Thomasson <[email protected]>
9624596 to
8cf217e
Compare
Contributor
Author
|
Added "Signed-off-by" to all commits. |
Contributor
Author
|
Added versioning for |
sitsofe
reviewed
Jul 30, 2025
vincentkfu
reviewed
Jul 30, 2025
vincentkfu
reviewed
Jul 30, 2025
vincentkfu
reviewed
Jul 30, 2025
vincentkfu
reviewed
Jul 30, 2025
vincentkfu
reviewed
Jul 30, 2025
vincentkfu
reviewed
Jul 30, 2025
vincentkfu
reviewed
Jul 30, 2025
Currently, fio keeps track of a finite number of recent write completions for each file in fio_file->last_write_comp. This information is saved/loaded as part of the "verify state." The verify code (verify_state_should_stop(), specifically) assumes that any write issued before these recorded writes must have been successfully completed. This is not generally true for iodepth > 1 and if writes are completed out-of-order. Consider this example: a single write stalls while all other writes complete normally. This condition can persist for an arbitrarily long time, and the stalled write will fall out of the range "covered" by last_write_comp. Saving state at this point (e.g. via a trigger) and halting the workload (e.g. via power-cycling the machine) will result in that stalled write being verified when the state is loaded despite the fact that the write may have never completed. Instead of tracking the last N write completions, we can instead track (1) the maximum issued sequence number, and (2) the sequence numbers for all in-flight writes. The "sequence number" here is a monotonically increasing value assigned to each write and verifying read (this is already implemented as io_u->numberio). An "in-flight" write is a write which has been issued but not yet completed. Furthermore, the number of in-flight writes is bounded by the iodepth. We can accomplish this by using a simple array of sequence numbers, which are initialized to an invalid value. Before issuing a write, its sequence number is written to a "free" slot, and then the maximum issued sequence number is incremented. After completing a write, its slot is changed back to the invalid value. On the verify side, we are allowed to verify as long as the current sequence number is <= the maximum issued sequence number AND it is not present in the inflight list. Saving/loading this information in the verify state and using it in verify_state_should_stop() is covered in a subsequent patch. Fixes: Issue axboe#1950 Signed-off-by: Riley Thomasson <[email protected]>
Plumb the new inflight write information from thread_data through thread_io_list and the saved/loaded verify state. Use this information in verify_state_should_stop() to halt verify as soon as the first inflight sequence number is reached. Fixes: Issue axboe#1950 Signed-off-by: Riley Thomasson <[email protected]>
When loops are used, the sequence number invariants in the inflight log are broken. In particular, experimental verify can issue writes in between loops, which ends up incrementing numberio without logging the writes to the inflight log. The intended interaction between loops and verify state save/load is a bit murky to me, but it seems reasonable to clear the inflight log in between each loop. Signed-off-by: Riley Thomasson <[email protected]>
Add a version field to verify_header and print a helpful message when it does not match expectations. This can happen if a user tries to verify data written by a different version of fio. The version field is split from the verify_type field in order to avoid messing with the layout of the struct. This is also makes distinguishing the new versioned header from older unversioned headers easier, as the old header format has a very limited set of valid values at this offset, regardless of endian-ness. Set the MSB in the new version value to distinguish it from the old header format. Signed-off-by: Riley Thomasson <[email protected]>
75b00e6 to
e1a250c
Compare
Contributor
Author
|
Gentle ping. |
Owner
|
Looks good to me! |
Closed
1 task
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.
See #1950 for a detailed description of the issue.
Details are in the individual commit messages.
I have seen 0 hits in ~30 runs of the repro setup I have. Without the fix the hit rate was 174 / 200.