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

Fix for zfs > 2.2.0 (closes #135) #136

Merged
merged 1 commit into from
Jan 28, 2024
Merged

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented Dec 31, 2023

No description provided.

- more reliable and readable stats file parsing
- uses original logic for older versions of zfs
  - I'm not convinced the original logic is right after reading
  https://utcc.utoronto.ca/~cks/space/blog/solaris/ZFSARCItsVariousSizes,
  but I don't want to potentially break things
@hakavlad
Copy link
Owner

@flaviut Could you add a description please? What has changed in ZFS and how was the problem solved in this PR? Will compatibility with older versions of ZFS be violated after merging this PR?

@flaviut
Copy link
Contributor Author

flaviut commented Jan 28, 2024

There's more explanation about what's changed at openzfs/zfs@a8d83e2, although that goes into too much detail and is hard to understand.

In short, new zfs no longer keeps metadata in a separate pool in the ARC. For old versions of zfs, exact same old behavior is retained. That commit message implies that there is almost no un-evictable metadata in the ARC in new zfs, so we don't need to consider it anymore (and we don't actually have the data to consider it if we wanted to).

with regards to parse_zfs_arcstats, this changeset parses the arcsats file into a dict, rather than line-by-line. The format has not changed, this is purely to make the code easier for me to understand.

# return c_min, size, arc_meta_used, arc_meta_min, zfs_available
# old zfs: consider arc_meta_used, arc_meta_min
if 'arc_meta_used' in stats and 'arc_meta_min' in stats:
meta_rec = max(stats['arc_meta_used'] - stats['arc_meta_min'], 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line is semantically exactly the same as the previous version:

meta_rec = arc_meta_used - arc_meta_min
if meta_rec < 0:
    meta_rec = 0

Comment on lines -3961 to -3982
try:
# find indexes
with open(arcstats_path, 'rb') as f:
a_list = f.read().decode().split('\n')
for n, line in enumerate(a_list):
if line.startswith('c_min '):
c_min_index = n

elif line.startswith('size '):
size_index = n

elif line.startswith('arc_meta_used '):
arc_meta_used_index = n

elif line.startswith('arc_meta_min '):
arc_meta_min_index = n

else:
continue
except Exception as e:
log(e)
ZFS = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of storing these into individual variables and writing fairly complicated code to parse this file again, we have a method that stores the whole contents of the file into an easy-to-read dict.

@hakavlad hakavlad merged commit bf477da into hakavlad:master Jan 28, 2024
3 checks passed
@hakavlad
Copy link
Owner

Merged, thanks!

@flaviut flaviut deleted the fix-135 branch January 28, 2024 02:25
@flaviut
Copy link
Contributor Author

flaviut commented Jan 28, 2024

thank you for your work writing and maintaining this project!

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