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

Don't apply the implicit runtime always #138

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

Conversation

nobuto-m
Copy link
Contributor

@nobuto-m nobuto-m commented Feb 5, 2024

When --size is supplied, it makes sense to complete the testing of the specified size rather than ending the test after the default runtime (60s). However, we should leave a space for a specific use case that an user specifies both --size and --runtime as per the fio manual.

runtime=time

The test will run until it completes the configured I/O workload or
until it has run for this specified amount of time, whichever occurs
first

Closes: #137

When `--size` is supplied, it makes sense to complete the testing of the
specified size rather than ending the test after the default runtime
(60s). However, we should leave a space for a specific use case that an
user specifies both `--size` and `--runtime` as per the fio manual.

> runtime=time
>
> The test will run until it completes the configured I/O workload or
> until it has run for this specified amount of time, whichever occurs
> first

Closes: louwrentius#137
@TjerkNan
Copy link

TjerkNan commented Feb 5, 2024

Thanks for the clarification and pr!
I understand and agree with your assessment.
I have to find some time to evaluate this pr for myself

@louwrentius
Copy link
Owner

louwrentius commented Feb 10, 2024

Unfortunately, this fix doesn't seem to work for me.

This step: settings = {**defaultsettings, **customsettings} merges all variables, both default and custom. So when I run with parameter --size 10G but no runtime parameter, I still get the Estimated Duration of 1 minute.

So this code:

  if not settings["runtime"] and not settings["size"]:
          # if there is no explicit runtime nor size supplied, fallback to
          # the default runtime
          settings["runtime"] = defaultsettings["runtime"]

The settings["runtime"] is already the default so this won't work as I see it.

I fear that the best solution here is just to document that you should specify --runtime 0 if you specify the --size parameter with a device but don't want to use --entire-device.

I'm open to ideas!

@nobuto-m
Copy link
Contributor Author

Hmm, I thought the patch worked for me. I might be wrong so let me retest it and get back to you.

@nobuto-m nobuto-m marked this pull request as draft February 10, 2024 15:06
@jackeichen
Copy link
Contributor

jackeichen commented May 21, 2024

Hi all,
@louwrentius @nobuto-m @TjerkNan

I do not think it is a good idea to change the code here. The conflict problems should be solved by user, why not make the paraini.py be able to identify a None value, like we did in function checks.check_settings:

if settings["entire_device"]:
        settings["runtime"] = None
        settings["size"] = "100%"

Then we can update the customsettings["runtime"] None to settings["runtime"].

@louwrentius louwrentius marked this pull request as ready for review May 21, 2024 15:55
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.

bench-fio --size is overridden by the implicit --runtime=60
4 participants