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

Linux 6.12 compat: Rename range_tree_* to zfs_range_tree_* #17010

Closed
wants to merge 2 commits into from

Conversation

IvanVolosyuk
Copy link
Contributor

@IvanVolosyuk IvanVolosyuk commented Jan 30, 2025

Linux 6.12 has conflicting range_tree_{find,destroy,clear} symbols.

Motivation and Context

Linux 6.12 introduced conflicting symbols which cause issues when compiling zfs with --enable-linux-builtin
See #16932 for context.
The change fix compilation in this configuration.

Description

The change is basically:
find . -type f -exec sed -i 's/range_tree_/zfs_range_tree_/g' {} +
... and manually fixing lines > 80 characters to get checkstyle pass.

How Has This Been Tested?

No functional changes, linux kernel 6.12 with ZFS builtin compiled successfully.

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:

@github-actions github-actions bot added the Status: Work in Progress Not yet ready for general review label Jan 30, 2025
@IvanVolosyuk IvanVolosyuk marked this pull request as ready for review January 30, 2025 13:11
@github-actions github-actions bot added Status: Code Review Needed Ready for review and testing and removed Status: Work in Progress Not yet ready for general review labels Jan 30, 2025
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.

This is incredibly invasive change. Wish there was some easier one, but whatever. Just a couple thoughts:

  • I would not rename metaslab_calculate_range_tree_type();
  • You are renaming some things in range_tree.h, but not others. Sure I don't want even more changes, just the logic looks inconsistent.

@IvanVolosyuk
Copy link
Contributor Author

Yeah, the change was pretty mechanical, I literally ran:
find . -type f -exec sed -i 's/range_tree_/zfs_range_tree_/g' {} +

What did I miss in range_tree.h? I can definitely revert change to metaslab_calculate_range_tree_type.
What about range_tree_t, I can keep it as it is - types cannot conflict with kernel symbols and it will remove a lot of churn.

@behlendorf
Copy link
Contributor

What about range_tree_t, I can keep it as it is - types cannot conflict with kernel symbols and it will remove a lot of churn.

It's unfortunate it's so invasive, but if we're going to have to make this change let's be consistent about it and use zfs_range_tree_t. We had to do the same thing for zfs_btree_t.

@IvanVolosyuk
Copy link
Contributor Author

IvanVolosyuk commented Jan 31, 2025

I reverted renaming of metaslab_calculate_range_tree_type(), which was changed by mistake.
If I understand @amotin correctly for consistency we want to rename rs_*, range_seg*, RANGE_SEG*. I did that in a separate commit for now. Let me know if I should squash it into the main one.

@IvanVolosyuk
Copy link
Contributor Author

Alternative to this change would be adding following lines to range_tree.h:

#define range_tree_find zfs_range_tree_find
#define range_tree_find_in zfs_range_tree_find_in
#define range_tree_create zfs_range_tree_create
#define range_tree_destroy zfs_range_tree_destroy
#define range_tree_clear zfs_range_tree_clear

May be also add #ifdef guard, so that the renaming only happening in cases when zfs is compiled in into the kernel. It is kinda hacky way and will make stack traces strange for the bug reports coming from people with zfs compiled into the kernel.

@tonyhutter
Copy link
Contributor

It is kinda hacky way and will make stack traces strange for the bug reports coming from people with zfs compiled into the kernel.

It's an interesting idea, but it does bring up the issues you mentioned. My vote is just to do the big rename.

@IvanVolosyuk
Copy link
Contributor Author

I should probably rebase and squash the commits. Let me know when I should do that.

@tonyhutter
Copy link
Contributor

@IvanVolosyuk yes, please squash these commits.

Could we get another reviewer to take a look at this? It looks nearly ready to go.

Copy link
Member

@robn robn 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 this; the mix of prefixed vs unprefixed things across the codebase make my eye do a twitchy thing :)

I'd prefer the remaining things in range_tree.h be converted, but also I know it sucks when these things drag on. So here's an approval, and a commit that I think does the remaining things: robn@7a3edae. Take it if you like, if not, I'll turn it into a PR to follow this one.

Good stuff!

IvanVolosyuk and others added 2 commits February 11, 2025 09:43
Linux 6.12 has conflicting range_tree_{find,destroy,clear} symbols.

Signed-off-by: Ivan Volosyuk <[email protected]>
@IvanVolosyuk
Copy link
Contributor Author

@tonyhutter I squashed and pulled in @robn additional changes on top.

@tonyhutter
Copy link
Contributor

@IvanVolosyuk thanks, looks good.

@IvanVolosyuk
Copy link
Contributor Author

IvanVolosyuk commented Feb 12, 2025

"Waiting on 1 reapproval from someone other than the last pusher. Review from tonyhutter is stale because it was submitted before the most recent code changes."

Looks like I need another approval or re-approval. @behlendorf may be? I don't know if @robn approval counts if I include his changes as well :)

@tonyhutter
Copy link
Contributor

@robn any additional comments on this? If you're happy with it then I'll pull it in.

@robn
Copy link
Member

robn commented Feb 14, 2025

@tonyhutter still good! ✔️

@tonyhutter
Copy link
Contributor

Merged as d4a5a7e 68473c4

@tonyhutter tonyhutter closed this Feb 14, 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.

5 participants