Skip to content

handle out-of-order write completions in verify state (Issue #1950)#1951

Merged
axboe merged 5 commits into
axboe:masterfrom
noclip-code:verify_inflight
Aug 5, 2025
Merged

handle out-of-order write completions in verify state (Issue #1950)#1951
axboe merged 5 commits into
axboe:masterfrom
noclip-code:verify_inflight

Conversation

@noclip-code
Copy link
Copy Markdown
Contributor

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.

Comment thread verify.h
@axboe
Copy link
Copy Markdown
Owner

axboe commented Jul 27, 2025

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]>
@noclip-code
Copy link
Copy Markdown
Contributor Author

Added "Signed-off-by" to all commits.

@noclip-code
Copy link
Copy Markdown
Contributor Author

Added versioning for verify_header. Please take another look. The CI failure looks like a timeout to me - it passed on my fork.

Comment thread libfio.c
Comment thread backend.c Outdated
Comment thread backend.c Outdated
Comment thread backend.c Outdated
Comment thread backend.c Outdated
Comment thread verify.c Outdated
Comment thread backend.c Outdated
Comment thread backend.c Outdated
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]>
@noclip-code
Copy link
Copy Markdown
Contributor Author

Gentle ping.

@axboe
Copy link
Copy Markdown
Owner

axboe commented Aug 5, 2025

Looks good to me!

@axboe axboe merged commit ab8b141 into axboe:master Aug 5, 2025
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants