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

Disk plugin: make hdparm device checks lazy #636

Merged
merged 1 commit into from
May 30, 2024

Conversation

zacikpa
Copy link
Contributor

@zacikpa zacikpa commented May 29, 2024

The disk plugin checks each device for hdparm
support during initialization. When using many disk devices, this can lead to unwanted delays in profile application.

This change makes the hdparm check lazy by postponing it to the moment when hdparm is actually needed, i.e., during dynamic tuning or when setting spindown/apm. Each device is checked at most once - the plugin now stores the sets of hdparm-supported devices and hdparm-unsupported devices).

Resolves: RHEL-6891

@jmencak
Copy link
Contributor

jmencak commented May 29, 2024

Thank you for the PR, @zacikpa ! I tested it and it works as intended. I like this solution, especially the "caching" of hdparm's support for a block device. A couple of nits:

  • In the commit message and in the description, I believe you mean [disk] plug-in, rather than Video plug-in.
  • Have you considered using associative array for hdparm_apm_devices with boolean values vs. two sets for supported/unsupported devices? This is an implementation detail and I guess it is a performance vs. readability issue. If the performance hit using associative array is not too high, it could help readability and get rid of one data structure.

I'd definitely love to see this in the upcoming FDP 24.D (June 5th deadline) release, @yarda .

@zacikpa zacikpa force-pushed the lazy-hdparm-checks branch from 1502a74 to 8824705 Compare May 30, 2024 06:08
@zacikpa zacikpa changed the title Video plugin: make hdparm device checks lazy Disk plugin: make hdparm device checks lazy May 30, 2024
The disk plugin checks each device for hdparm
support during initialization. When using many disk
devices, this can lead to unwanted delays in profile
application.

This change makes the hdparm check lazy by postponing
it to the moment when hdparm is actually needed, i.e.,
during dynamic tuning or when setting spindown/apm.
Each device is checked at most once - the plugin now stores
the sets of hdparm-supported devices and hdparm-unsupported
devices).

Resolves: RHEL-6891
@zacikpa zacikpa force-pushed the lazy-hdparm-checks branch from 8824705 to 0536382 Compare May 30, 2024 06:11
@zacikpa
Copy link
Contributor Author

zacikpa commented May 30, 2024

Thanks for the review, @jmencak. I updated the description with the correct plugin name and swapped the two sets for a dictionary.

@jmencak
Copy link
Contributor

jmencak commented May 30, 2024

Thank you for the changes, Pavol. LGTM and I've also retested this.

@yarda
Copy link
Contributor

yarda commented May 30, 2024

LGTM, thanks.

@yarda yarda merged commit 5385fa9 into redhat-performance:master May 30, 2024
14 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.

3 participants