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

ZTS: new kstat helper #16950

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ZTS: new kstat helper #16950

wants to merge 3 commits into from

Conversation

robn
Copy link
Member

@robn robn commented Jan 15, 2025

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

Tests use a wide range of methods to get at kstat values. This is error prone, and usually ends up limiting the test to a single platform.

This came up again in #16947, and I decided to just fix it.

Description

libtest.shlib had a kstat helper that was barely used, I suspect in part because it was very limited in the kinds of kstats it could gather. This adds new functions to replace it, for each kind of thing that can have stats: global, pool and dataset. There's options in there to get a single stat value, or all values within a group.

Most importantly, the interface is the same for both platforms. This is quite complicated, but will be worth it when its very easy to use kstats from within test.

See comments at stop of kstat.shlib for the detail..

After that, all existing uses of kstats, through either the old kstat or get_arcstat helpers, or directly through /proc/spl/kstat/zfs or sysctl, have been updated to use the new functions.

Actually, almost all. I've left the procfs test suite for now, since it is explicitly testing the /proc/spl/kstat/zfs interface.

Mostly this has been a mechanical conversion, but some tests have been adjusted slightly to not assume things about kstat name structure or data format.

I have not looked closely, but this also might make it easier for some of the platform-specific tests to work everywhere. That's an exercise for the future.

How Has This Been Tested?

All modified tests have passed on Linux and FreeBSD.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. Few issues I spotted:

tests/zfs-tests/include/libtest.shlib Outdated Show resolved Hide resolved
tests/zfs-tests/tests/functional/direct/dio.kshlib Outdated Show resolved Hide resolved
@robn robn force-pushed the zts-new-kstat branch 2 times, most recently from 02c914a to e8e8626 Compare January 15, 2025 21:20
@tonyhutter
Copy link
Contributor

Nice cleanup!

I suspect future generations will re-implement get_arcstat as a helper function though. Did you consider keeping it in place but just doing?:

function get_arcstat # stat
{
	kstat arcstats.$1
}

@robn robn force-pushed the zts-new-kstat branch 2 times, most recently from e079b6f to e52acd7 Compare January 16, 2025 06:47
@robn
Copy link
Member Author

robn commented Jan 16, 2025

Alright, here's v2, dealing with some of the complications. Top post updated.

@robn
Copy link
Member Author

robn commented Jan 16, 2025

I suspect future generations will re-implement get_arcstat as a helper function though. Did you consider keeping it in place but just doing?

I did at first (as you see, it's removed in a separate commit). It seemed not worth keeping - it did nothing of interest, but looked different to other things, inviting questions like "why is this special?" and "why aren't other things not special?"

I don't think anyone will bother implementing it, because there's nothing gained from doing so, and nothing in the existing code to even give the idea. But, if they do, at least it will work properly :)

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Just few comments:

tests/zfs-tests/include/kstat.shlib Show resolved Hide resolved
tests/zfs-tests/include/kstat.shlib Outdated Show resolved Hide resolved
tests/zfs-tests/include/kstat.shlib Outdated Show resolved Hide resolved
robn added 3 commits January 17, 2025 07:57
The old kstat helper function was barely used, I suspect in part because
it was very limited in the kinds of kstats it could gather.

This adds new functions to replace it, for each kind of thing that can
have stats: global, pool and dataset. There's options in there to get a
single stat value, or all values within a group.

Most importantly, the interface is the same for both platforms.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
Removes other custom helpers and direct accesses to /proc.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
It's now a simple wrapper, so lets just call kstat direct.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants