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

Allow multiple whitespace-separated compression-algorithm=s, and give the ones after the first one to /recomp_algorithm #200

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

nabijaczleweli
Copy link
Collaborator

Ref: #178 (comment)

I've tested the error case, @OmegaSquad82 needs to test the "actually setting it" case.

A config like

compression-algorithm = a b c

was previously "compress with a b c" (naturally invalid) and is now "compress with a, recompress with b then c"

@nabijaczleweli nabijaczleweli added the enhancement New feature or request label Sep 20, 2024
@OmegaSquad82
Copy link

OmegaSquad82 commented Oct 21, 2024

Thank you so much I'll test it as soon as I can.

@nabijaczleweli
Copy link
Collaborator Author

Rebased

@keszybz
Copy link
Member

keszybz commented Nov 20, 2024

Sorry for the delay. I got the notification and then forgot to look at this.

Code-wise this looks good, but I'm not convinced that this is the right interface.
The kernel allows additional parameters to be specified, for example type=huge_idle max_pages=42 algo=zstd. If we allow just a list of algorithm types to be specified, it will be awkward to attach any options to this.

I wonder if the interface shouldn't be inverted, allowing the user to list each "recompression level" separately, with a list of options. But I'm not sure how to express this concisely.

@nabijaczleweli
Copy link
Collaborator Author

nabijaczleweli commented Nov 20, 2024

I didn't really notice this because the interface is kinda iffy (/recomp_algorithm takes no options, /recompress takes options for the recompression subsystem or for a single algorithm, depending on the calling convention).

I can't think of a multi-key system that'd be any good. Maybe

compression-algorithm = a b(type=huge,threshold=1000) c (threshold=3000)

for

echo a > /sys/block/zram0/comp_algorithm

echo algo=b    priority=1 > /sys/block/zramX/recomp_algorithm
echo type=huge threshold=1000 priority=1 > /sys/block/zramX/recompress

echo algo=c    priority=2 > /sys/block/zramX/recomp_algorithm

echo threshold=3000       > /sys/block/zramX/recompress

?

@keszybz
Copy link
Member

keszybz commented Nov 20, 2024

That seems pretty nice.

@nabijaczleweli
Copy link
Collaborator Author

Added

@nabijaczleweli
Copy link
Collaborator Author

As a side-effect we also support /algorithm_params (it was easier than not doing that). Needs to be tested by submitter.

@keszybz
Copy link
Member

keszybz commented Nov 20, 2024

LGTM. Let's wait for feedback from the reporter.

@keszybz
Copy link
Member

keszybz commented Nov 21, 2024

OK, let's do this. It'd be good to some testing on a real system, but we can do this later.

@keszybz keszybz merged commit 0fb828f into systemd:main Nov 21, 2024
5 checks passed
@OmegaSquad82
Copy link

LGTM, have tested few parameters. Seems to work as expected. Thank you both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants