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

Disabled fstrim on BTRFS by default #2109

Merged
merged 2 commits into from Sep 26, 2023
Merged

Disabled fstrim on BTRFS by default #2109

merged 2 commits into from Sep 26, 2023

Conversation

Torxed
Copy link
Member

@Torxed Torxed commented Sep 25, 2023

Copy link
Contributor

@lavafroth lavafroth left a comment

Choose a reason for hiding this comment

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

Looks great. I overlooked the case where the filesystem is not BTRFS. Good catch!

Copy link

@kojq kojq left a comment

Choose a reason for hiding this comment

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

looks good; just heads up that this could go beyond btrfs as new filesystems such as bcachefs, PuzzleFS, and/or SSDFS could be implemented into the kernel later in some way, and some future filesystem(s) may not need periodic trim as well. You could consider changing the variable name to _need_periodic_trim or something else, and consider reversing the boolean values (this also remove the not).

I will approve otherwise, if desired.

@Torxed
Copy link
Member Author

Torxed commented Sep 26, 2023

Good idea, changed it to _disable_fstrim to be more generic. As we for most filesystems enable it, I thought of keeping the naming consistent with the if-logic and default value of being False for now.

@kojq
Copy link

kojq commented Sep 26, 2023

Good idea, changed it to _disable_fstrim to be more generic. As we for most filesystems enable it, I thought of keeping the naming consistent with the if-logic and default value of being False for now.

Great. Also, is there a particular reason we want a variable, as I don't see anything technical like a scope issue with directly using self.enable_periodic_trim() when checking for btrfs?

@Torxed
Copy link
Member Author

Torxed commented Sep 26, 2023

Are you referring to why we don't do the check outside before calling enable_fstrim?
The only reason is to keep minimal_installation small and easy to read :)

And if enable_fstrim is called as part of a custom script it's good if we do the check in the function to not cause issues for lib users.

@Torxed Torxed merged commit c427391 into master Sep 26, 2023
11 checks passed
@Torxed Torxed deleted the remove-fstrim branch September 26, 2023 07:56
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.

i am encountering fstrim error No need to enable periodic TRIM for btrfs
3 participants