Skip to content
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

Hang when ioengine=net and verify_backlog #1513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

horshack-dpreview
Copy link
Contributor

The net engine only supports uni-direction workloads. This means verify operations for write workloads aren't supported. There is already logic to prevent post-workload verifies but no logic to prevent the in-workload verifies performed when verify_backlog is enabled.

The fix is to prevent in-workload verifies for FIO_UNIDIR engines. There are several options on where to place this conditional but I found an existing conditional that prevents the logging of writes to log_io_piece() when verify is disabled, and check_get_verify() already depends on this behavior to not perform in-workload verifies for other cases (it doesn't check if verify is disabled - it relies on the absence of entries in log_io_piece). So I placed the check for FIO_UNIDIR there.

My grasp of these distributed, interdependent state checks across modules is tenuous so please review this fix carefully.

Signed-off-by: Adam Horshack ([email protected])

The net engine only supports uni-direction workloads. This means verify
operations for write workloads aren't supported. There is already logic to
prevent post-workload verifies but no logic to prevent the in-workload
verifies performed when verify_backlog is enabled.

The fix is to prevent in-workload verifies for FIO_UNIDIR engines. There
are several options on where to place this conditional but I found an
existing conditional that prevents the logging of writes to log_io_piece()
when verify is disabled, and check_get_verify() already depends on this
behavior to not perform in-workload verifies for other cases (it doesn't
check if verify is disabled - it relies on the absence of entries in
log_io_piece). So I placed the check for FIO_UNIDIR there.

My grasp of these distributed, interdependent state checks across modules
is tenuous so please review this fix carefully.

Signed-off-by: Adam Horshack ([email protected])
@horshack-dpreview
Copy link
Contributor Author

@vincentkfu, Is this check failure real and something I need to address?

Test 1012 PASSED t/log_compression.py
28 test(s) passed, 1 failed, 12 skipped
Command exited with code 1
bash.exe -lc "cd \"${APPVEYOR_BUILD_FOLDER}\" && [ -d test-artifacts ] && 7z a -t7z test-artifacts.7z test-artifacts -xr!foo.0.0 -xr!latency.?.0 -xr!fio_jsonplus_clat2csv.test && appveyor PushArtifact test-artifacts.7z
7-Zip 22.01 (x64) : Copyright (c) 1999-2022 Igor Pavlov : 2022-07-15

@vincentkfu
Copy link
Collaborator

This is a real failure but it's not due to your patch.

@vincentkfu
Copy link
Collaborator

Instead of just failing silently it would be better the print an error message and fail the job because this is an invalid configuration.

@horshack-dpreview
Copy link
Contributor Author

Instead of just failing silently it would be better the print an error message and fail the job because this is an invalid configuration.

Hi @vincentkfu, With this fix the workload doesn't fail - it bypasses the configured verified operation. I modeled this from existing fio behavior, which seems to favor bypassing logic paths not supported for a given configuration operation rather than preemptively disallowing them.

@vincentkfu
Copy link
Collaborator

Fio can't satisfy the options provided and should notify the user right away about this problem and give him/her a chance to correct it instead of running to completion with a workload different from what the user specified.

There are plenty of examples in fixup_options() following a similar philosophy.

BTW you can have git automatically add a sign-off to your commits with git commit -s.

@horshack-dpreview
Copy link
Contributor Author

Fio can't satisfy the options provided and should notify the user right away about this problem and give him/her a chance to correct it instead of running to completion with a workload different from what the user specified.

There are plenty of examples in fixup_options() following a similar philosophy.

BTW you can have git automatically add a sign-off to your commits with git commit -s.

Thanks.The current implementation of FIO_UNIDIR already bypasses user-specified verify operations for write-only workloads (ie, not possible since only all-writes or all-reads are supported). My fix only corrects the implementation to handle a case it missed. Are you saying the original implementation of avoiding verifies for FIO_UNIDIR was misguided and needs to be corrected?

@horshack-dpreview
Copy link
Contributor Author

@vincentkfu, If your answer to my above question is yes, here is an alternate fix that uses your suggestion to prevent the config from executing. As part of this change I also removed the existing check for FIO_UNIDIR for verifies. If this looks good I can either replace the existing pull request with this alternate branch or create a new pull request, whichever you prefer.

horshack-dpreview@ec2805f

@vincentkfu
Copy link
Collaborator

Thanks for the explanation. I see two competing principles here:

  • notifying users of invalid configurations
  • avoiding breakage of existing job files

A reasonable balance seems to be:

  • no change to existing behavior with do_verify verifies and FIO_UNIDIR ioengines
  • reject verify_backlog workloads with FIO_UNIDIR ioengines

@axboe feel free to chime in if you feel differently.

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.

2 participants