-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
zinject: count matches and injections for each handler #16938
Open
robn
wants to merge
2
commits into
openzfs:master
Choose a base branch
from
robn:zinject-counts
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+236
−39
Conversation
This file contains 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
skalthoff
approved these changes
Jan 9, 2025
amotin
reviewed
Jan 9, 2025
tonyhutter
reviewed
Jan 9, 2025
tests/zfs-tests/tests/functional/cli_root/zinject/zinject_counts.ksh
Outdated
Show resolved
Hide resolved
tonyhutter
reviewed
Jan 9, 2025
tests/zfs-tests/tests/functional/cli_root/zinject/zinject_counts.ksh
Outdated
Show resolved
Hide resolved
This is a really nice quality of life feature. I will definitely be using it! |
For real! I've had a hacky version of this for about a year and I while I still don't love zinject for a bunch of reasons, I can't imagine how I ever used it without this! |
amotin
approved these changes
Jan 10, 2025
tests/zfs-tests/tests/functional/cli_root/zinject/zinject_counts.ksh
Outdated
Show resolved
Hide resolved
When building tests with zinject, it can be quite difficult to work out if you're producing the right kind of IO to match the rules you've set up. So, here we extend injection records to count the number of times a handler matched the operation, and how often an error was actually injected (ie after frequency and other exclusions are applied). Then, display those counts in the `zinject` output. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>
It turns out we can count a matching delay injection handler earlier, by counting it once we decide its worth considering if it has a spare lane for this IO. With that in hand, it becomes useful to show separate match and inject counts, as well as the frequency, so update the output and the test to show all that. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Suggested-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]>
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.
[Sponsors: Klara, Inc., Wasabi Technology, Inc.]
Motivation and Context
When building tests with
zinject
, it can be quite difficult to work out if you're producing the right kind of IO to match the rules you've set up. Imagine if instead, when you runzinject
and get the list of handlers, it also showed you how many times it had matched one, and how many times it actually injected one (can be different when using frequencies).Imagine no longer!
Description
Extends
zinject_record_t
with two new fields,zi_match_count
andzi_inject_count
.For each injection type, increments one or both of these as appropriate. For record types that don't have a notion of "frequency", they are necessarily incremented together.
Then, for the injection records that have appropriate display, show the counts. Device and object injections show both, while delay shows only the single count. The others don't have reasonable display methods and/or counts don't really make sense for them, so I haven't bothered showing them.
This is technically an ABI break:
zinject_record_t
is now 16 bytes longer than it was. Old userspace on new kernel won't care, but new userspace on old kernel may read garbage or segfault on those count fields. I don't really care; I consider the injection facility to be a swiss-army chainsaw tied deeply into the matching module version, and a crashing userspace program is the least of your concerns. If I'm wrong, say so, and I'll see what I can do to make the ABI a bit more stable.REVIEW BONUS!
@amotin suggested that with a small adjustment, we could usefully count delay handler matches separately from injections. We can, and it's very nice, so that's added as a second commit.
How Has This Been Tested?
Test included.
ZTS runs on a handful of zinject-using tests pass cleanly.
Types of changes
Checklist:
Signed-off-by
.