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

Add persistent and perf options to the luks_device #434

Merged
merged 1 commit into from
Apr 10, 2022

Conversation

jsirex
Copy link
Contributor

@jsirex jsirex commented Mar 31, 2022

Read and write work queue significantly degrades performance on
SSD/NVME devices[1].

In Debian 11 crypttab does not support no-read-workqueue and
no-write-workqueue flags, so the persistent flag is workaround: once
opened with perf parameters persists forever.

[1] https://blog.cloudflare.com/speeding-up-linux-disk-encryption/

Signed-off-by: Yauhen Artsiukhou jsirex@gmail.com

Fixes: #427

@jsirex
Copy link
Contributor Author

jsirex commented Mar 31, 2022

I don't know python/ansible well enough so feel free to rewrite PR

@MarkusTeufelberger
Copy link
Contributor

Thanks a lot for the feature! :-)

Maybe you can think of some ways to test/ensure this is actually applied? They would go here: https://github.com/ansible-collections/community.crypto/tree/main/tests/integration/targets/luks_device/tasks/tests

Also if there are already limitations on Debian11, there might be other ones too, these should be documented at least.

@jsirex
Copy link
Contributor Author

jsirex commented Mar 31, 2022

Regarding Debian 11 - I think it is temporary bug. Cryptsetup supports everything, but crypttab-related scripts where not updated yet. So it doesn't relate to this collection at all.

Speaking about test - its quite hard for me: after luks container is opened you must check it with
dmsetup table <device> and check persistence with cryptsetup luksDump <device> for flags. I'm not ready now to do this.
Anyway tested locally with vagrant. And I'm not sure we should re-test library functions (that cryptsetup flags work), I just pass them.

@jsirex jsirex force-pushed the luks-perf-flags branch 5 times, most recently from 3e47349 to af846d2 Compare April 1, 2022 09:54
@jsirex
Copy link
Contributor Author

jsirex commented Apr 1, 2022

Added some tests, but looks like kernel too old.

Copy link
Contributor

@felixfontein felixfontein 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 your contribution!

plugins/modules/luks_device.py Outdated Show resolved Hide resolved
plugins/modules/luks_device.py Outdated Show resolved Hide resolved
plugins/modules/luks_device.py Outdated Show resolved Hide resolved
plugins/modules/luks_device.py Outdated Show resolved Hide resolved
plugins/modules/luks_device.py Outdated Show resolved Hide resolved
plugins/modules/luks_device.py Outdated Show resolved Hide resolved
@jsirex jsirex force-pushed the luks-perf-flags branch 2 times, most recently from 841c4cc to f613f0b Compare April 2, 2022 21:50
plugins/modules/luks_device.py Outdated Show resolved Hide resolved
plugins/modules/luks_device.py Outdated Show resolved Hide resolved
@jsirex jsirex force-pushed the luks-perf-flags branch 2 times, most recently from 965d9a0 to f7d5744 Compare April 3, 2022 07:59
Copy link
Contributor

@felixfontein felixfontein 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 to me.

@MarkusTeufelberger
Copy link
Contributor

Do we also need a changelog fragment?

@MarkusTeufelberger
Copy link
Contributor

Other than that looks good to me too - sorry for causing this much hassle just to add a literal handful of parameters to a command... but in the end it is important that documentation is available and there are no immediate regressions or problems down the line.

I'm sure this feature will be appreciated by users, especially since Cloudflare detailed how to actually get decent performance on encrypted setups on modern systems, thanks for adding it!

@jsirex jsirex force-pushed the luks-perf-flags branch 2 times, most recently from 890bf02 to 1cd5eae Compare April 4, 2022 09:31
@jsirex
Copy link
Contributor Author

jsirex commented Apr 4, 2022

Hi!
One last thought: I see that cryptsetup uses options prefixed with --perf-, however in the crypttab and dmsetup there is no such prefix. If it makes sense I can remove perf_ from options:

perf_same_cpu_crypt -> same_cpu_crypt
perf_submit_from_crypt_cpus -> submit_from_crypt_cpus
perf_no_read_workqueue -> no_read_workqueue
perf_no_write_workqueue -> no_write_workqueue

@felixfontein
Copy link
Contributor

One last thought: I see that cryptsetup uses options prefixed with --perf-, however in the crypttab and dmsetup there is no such prefix. If it makes sense I can remove perf_ from options:

perf_same_cpu_crypt -> same_cpu_crypt
perf_submit_from_crypt_cpus -> submit_from_crypt_cpus
perf_no_read_workqueue -> no_read_workqueue
perf_no_write_workqueue -> no_write_workqueue

Hmm, that's a tough question (IMO). I wish they'd be more consistent :)

Keeping the prefix ensures that these four options will be shown next to each other in the documentation. For me this slightly tips the scale to "keep the prefix".

Read and write work queue significantly degrades performance on
SSD/NVME devices[1].

In Debian 11 crypttab does not support no-read-workqueue and
no-write-workqueue flags, so the persistent flag is workaround: once
opened with perf parameters persists forever.

[1] https://blog.cloudflare.com/speeding-up-linux-disk-encryption/

Signed-off-by: Yauhen Artsiukhou <jsirex@gmail.com>
@felixfontein felixfontein merged commit 041fff5 into ansible-collections:main Apr 10, 2022
@felixfontein
Copy link
Contributor

@jsirex thanks for contributing this!
@MarkusTeufelberger thanks for reviewing!

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.

Allow additional options to the cryptsetup
3 participants