Hang when ioengine=net and verify_backlog

[horshack-dpreview]

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]

@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]

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

[vincentkfu]

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]

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]

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]

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]

@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]

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.