Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Set compression levels to upstream defaults #476

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

C0rn3j
Copy link
Contributor

@C0rn3j C0rn3j commented Jan 17, 2025

Currently, the compression defaults are maxed out to maximize storage gains (except for xz which is set to default?), at the cost of taking minutes instead of seconds to compress.

There is previous conversation about this comparing zstd gains at max vs minimum - #455

@skwerlman made a PR that that made the values configurable - #456

This PR changes this configuration to upstream defaults, as suggested in the above discussion.

The suggestion also considered dropping the parameters altogether to just always adapt upstream defaults - it should be more user friendly for those that are trying to modify the configuration to have a set example.

This is likely to break setups where people are putting the modules on an undersized ESP (notably, when people reuse the 100MiB Windows ESP for /boot), so bumping the filesizes by a couple megabytes could tip the size over the max.

Such people can either fix their ESP size, or more simply configure dkms to use the old values that sacrifice speed for space savings.

@christian-heusel
Copy link

Wouldn't it make more sense to omit these to also pick changed upstream defaults in the future?

@C0rn3j
Copy link
Contributor Author

C0rn3j commented Jan 17, 2025

Wouldn't it make more sense to omit these to also pick changed upstream defaults in the future?

The suggestion also considered dropping the parameters altogether to just always adapt upstream defaults - it should be more user friendly for those that are trying to modify the configuration to have a set example.

@scaronni
Copy link
Collaborator

@C0rn3j shall we leave the default configuration commented in the configuration file and just not pass any parameter in dkms.in? This way users still have a reference but the default is not explicitly declared.

@C0rn3j
Copy link
Contributor Author

C0rn3j commented Jan 20, 2025

Could do that for gzip, but I don't know how to cleanly do it for xz/zstd as they set more than just compression level value, and something like this looks mildly bad:

# compress_gzip_opts="-6"
compress_xz_opts="--check=crc32 --lzma2=dict=1MiB"
# compress_xz_opts="--check=crc32 --lzma2=dict=1MiB -6"
compress_zstd_opts="-q -T0"
# compress_zstd_opts="-q -T0 -3"

EDIT: Or do you mean to leave the compression values only in the framework file?

EDIT2: Did that - won't set the defaults, but the commented out framework file has them as an example.

@scaronni
Copy link
Collaborator

EDIT: Or do you mean to leave the compression values only in the framework file?

EDIT2: Did that - won't set the defaults, but the commented out framework file has them as an example.

Yes, exactly. Thank you, I think it's fine.

@skwerlman
Copy link
Contributor

This is probably a good place to add a note about the kernel not supporting zstd's --ultra flag or xz's crc64 csum mode: #386

@scaronni
Copy link
Collaborator

Merging for now, but the proper solution would be #319

@scaronni scaronni merged commit bcdefbf into dell:master Jan 22, 2025
25 checks passed
@C0rn3j C0rn3j deleted the faster-copmpression branch January 23, 2025 09:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants