mount-options: Add Mount Options Support #90

hswong3i · 2021-07-25T14:10:28Z

When using zram for file system, we need to enable discard mount option for issue discard/TRIM commands to the underlying zram block device when blocks are freed. Else deleted files are not zero-ed, so zram does not remove the compressed pages.

Fixes #89

nabijaczleweli · 2021-07-25T23:19:09Z

src/config.rs

@@ -38,6 +39,7 @@ impl Device {
            disksize: 0,
            swap_priority: 100,
            mount_point: None,
+            mount_options: Some("".to_string()),


This makes no sense. Either make this just String, or actually only write it if it's set.

@nabijaczleweli sorry that I have zero experience on rust, and what I could handle is modify from similar logic, e.g. fs-type and mount-point.

What am I looking for from here is: if mount-options is empty string or None, just print an empty string to the generated file with Options="".

May you provide some code snippet as reference, so I could modify it correctly?

hswong3i · 2021-07-26T14:31:34Z

@nabijaczleweli test case added and passing as expected; BTW, after reboot the device is now format and override as swap by systemd, so the mount failed (because discard is not support):

# cat /etc/systemd/zram-generator.conf 
[zram0]
max-zram-size = 8192
zram-fraction = 1.0
[zram1]
max-zram-size = 8192
zram-fraction = 1.0
mount-point = /var/lib/libvirt/images
fs-type = ext4
mount-options = discard

# systemctl status systemd-zram-setup@zram1.service 
● systemd-zram-setup@zram1.service - Create swap on /dev/zram1
     Loaded: loaded (/lib/systemd/system/systemd-zram-setup@.service; static)
    Drop-In: /run/systemd/generator/systemd-zram-setup@zram1.service.d
             └─bindsto-mount.conf
     Active: inactive (dead) since Mon 2021-07-26 22:25:53 CST; 45s ago
       Docs: man:zram-generator(8)
             man:zram-generator.conf(5)
    Process: 9827 ExecStart=/lib/systemd/system-generators/zram-generator --setup-device zram1 (code=exited, status=0/SUCCESS)
    Process: 9832 ExecStop=/lib/systemd/system-generators/zram-generator --reset-device zram1 (code=exited, status=0/SUCCESS)
   Main PID: 9827 (code=exited, status=0/SUCCESS)

Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: Superblock backups stored on blocks:
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]:         32768, 98304, 163840, 229376, 294912, 819200, 884736, 1605632
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: [49B blob data]
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: [46B blob data]
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: Creating journal (16384 blocks): done
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: [83B blob data]
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Finished Create swap on /dev/zram1.
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Stopping Create swap on /dev/zram1...
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: systemd-zram-setup@zram1.service: Succeeded.
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Stopped Create swap on /dev/zram1.

# journalctl -xef | egrep -e '^\w.*(systemd|zram)'
Jul 26 22:25:52 hswong3i-XPS-13-9300 kernel: zram1: detected capacity change from 0 to 16777216
Jul 26 22:25:52 hswong3i-XPS-13-9300 zram-generator[9830]: mke2fs 1.45.7 (28-Jan-2021)
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: [120B blob data]
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: Creating filesystem with 2097152 4k blocks and 524288 inodes
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: Filesystem UUID: c8cf6c3f-6dd1-48e6-86a2-9f46ae887980
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: Superblock backups stored on blocks:
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]:         32768, 98304, 163840, 229376, 294912, 819200, 884736, 1605632
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: [49B blob data]
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: [46B blob data]
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: update-notifier-download.service: Succeeded.
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Finished Download data for packages that failed at package install time.
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: Creating journal (16384 blocks): done
Jul 26 22:25:53 hswong3i-XPS-13-9300 zram-generator[9830]: [83B blob data]
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Finished Create swap on /dev/zram1.
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: var-lib-libvirt-images.mount: Directory /var/lib/libvirt/images to mount over is not empty, mounting anyway.
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Mounting Compressed Storage on /dev/zram1...
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: var-lib-libvirt-images.mount: Mount process exited, code=exited, status=1/FAILURE
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: var-lib-libvirt-images.mount: Failed with result 'exit-code'.
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Failed to mount Compressed Storage on /dev/zram1.
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Stopping Create swap on /dev/zram1...
Jul 26 22:25:53 hswong3i-XPS-13-9300 kernel: zram1: detected capacity change from 16777216 to 0
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: systemd-zram-setup@zram1.service: Succeeded.
Jul 26 22:25:53 hswong3i-XPS-13-9300 systemd[1]: Stopped Create swap on /dev/zram1.

Any idea why incorrectly detect fs-type as swap?

src/generator.rs

hswong3i · 2021-07-28T04:59:54Z

@nabijaczleweli any more hits could be provided for #90 (comment)?

keszybz · 2021-07-28T08:44:08Z

Please drop the first commit from this pull request.

hswong3i · 2021-07-28T08:45:15Z

Please drop the first commit from this pull request.

Then the CI will fail due to missing kernel module, are you sure?

keszybz · 2021-08-08T09:16:04Z

Please drop the first commit from this pull request.

Then the CI will fail due to missing kernel module, are you sure?

You are pulling in unrelated changes into the pull request. If the requirement for a new module is added in this pull request, then the commit to install the module should also be in this pull request. If not, then not. And the other changes to Makefiles make this pull request unmergeable. Please clean it up to keep only the relevant parts.

hswong3i · 2021-08-08T12:06:17Z

You are pulling in unrelated changes into the pull request. If the requirement for a new module is added in this pull request, then the commit to install the module should also be in this pull request. If not, then not. And the other changes to Makefiles make this pull request unmergeable. Please clean it up to keep only the relevant parts.

Could we make #92 and #93 get merged? I will follow up this PR afterward.

keszybz · 2021-08-08T16:27:24Z

Could we make #92 and #93 get merged? I will follow up this PR afterward.

Done.

hswong3i · 2021-08-09T06:37:40Z

Could we make #92 and #93 get merged? I will follow up this PR afterward.

Done.

Done

keszybz

This mostly looks reasonable, but:

please rebase on top of current main
please explain in the commit message what the change is. (Right now there's a discussion of why the change is useful, which is good, but cannot be the only content of the commit message.)
the new option needs to be described in the docs (at least in man/zram-generator.conf.md).
why are the test cases renamed? Either drop that change, or if there's some good reason for it, make it a separate commit and justify.

src/generator.rs

hswong3i · 2021-08-09T09:42:11Z

This mostly looks reasonable, but:
1. please rebase on top of current `main`

Done

2. please explain in the commit message what the change is. (Right now there's a discussion of why the change is useful, which is good, but cannot be the only content of the commit message.)

Doesn't the commit title mount-options: Add Mount Options Support already enough?

3. the new option needs to be described in the docs (at least in `man/zram-generator.conf.md`).

Done

4. why are the test cases renamed? Either drop that change, or if there's some good reason for it, make it a separate commit and justify.

After reorder, the options provided is now increased from 1 -> 2 -> 3, which simpler for tracing:

$ cat tests/07-plain-device/etc/systemd/zram-generator.conf 
[zram11]
fs-type = ext4

$ cat tests/08-mount-point/etc/systemd/zram-generator.conf 
[zram11]
mount-point = /var/compressed
fs-type = ext4

$ cat tests/09-mount-options/etc/systemd/zram-generator.conf 
[zram11]
mount-point = /var/compressed
fs-type = ext4
mount-options = discard

keszybz · 2021-08-10T06:36:08Z

why are the test cases renamed? Either drop that change, or if there's some good reason for it, make it a separate commit and justify.

OK. But it needs to be a separate commit.

Doesn't the commit title mount-options: Add Mount Options Support already enough?

The title doesn't explain what the option name is, and what it does. A person looking at the commit list should be able to figure out what a commit does from the description. A much better title would be "Add Options= setting to set mount options" and then have some more details in the commit body, for example a very simple example of the option and the effect on the generated unit.

All units in units/ follow this pattern, as do all other generators that we provide. The question of the order was raised in systemd/zram-generator#90 (comment), and I think it's nice to make it consistent everywhere (What= before Where= matches mount(8) and fstab(5)).

keszybz · 2021-08-10T06:42:18Z

Please undo all the unrelated changes from commit, like the file system type and default options. Let's try to do one thing first correctly.

hswong3i · 2021-08-10T10:57:17Z

Please undo all the unrelated changes from commit, like the file system type and default options. Let's try to do one thing first correctly.

Done, also split PR to #95 and #96

All units in units/ follow this pattern, as do all other generators that we provide. The question of the order was raised in systemd/zram-generator#90 (comment), and I think it's nice to make it consistent everywhere (What= before Where= matches mount(8) and fstab(5)).

man/zram-generator.conf.md

hswong3i · 2021-08-10T13:32:33Z

why are the test cases renamed? Either drop that change, or if there's some good reason for it, make it a separate commit and justify.

OK. But it needs to be a separate commit.

See #96

Doesn't the commit title mount-options: Add Mount Options Support already enough?

The title doesn't explain what the option name is, and what it does. A person looking at the commit list should be able to figure out what a commit does from the description. A much better title would be "Add Options= setting to set mount options" and then have some more details in the commit body, for example a very simple example of the option and the effect on the generated unit.

Done

When using zram for file system, we need to enable `discard` mount option for issue discard/TRIM commands to the underlying zram block device when blocks are freed. Else deleted files are not zero-ed, so zram does not remove the compressed pages. For example, setting `/etc/systemd/zram-generator.conf` as below could format the file system as ext4 with TRIM enabled: $ cat /etc/systemd/zram-generator.conf [zram11] mount-point = /var/compressed fs-type = ext4 mount-options = discard Fixes systemd#89 Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>

keszybz · 2021-10-13T11:47:44Z

Replaced by #103.

nabijaczleweli requested changes Jul 25, 2021

View reviewed changes

hswong3i force-pushed the main-mount_options branch 4 times, most recently from 66ded65 to 9e44984 Compare July 26, 2021 14:24

nabijaczleweli requested changes Jul 26, 2021

View reviewed changes

src/generator.rs Outdated Show resolved Hide resolved

hswong3i force-pushed the main-mount_options branch from 9e44984 to 064ab12 Compare July 26, 2021 14:40

hswong3i force-pushed the main-mount_options branch 2 times, most recently from 83abe00 to 7f0eb95 Compare July 28, 2021 07:59

hswong3i force-pushed the main-mount_options branch 3 times, most recently from 455e4be to 5150a6b Compare July 31, 2021 06:15

hswong3i force-pushed the main-mount_options branch 2 times, most recently from 35bb962 to e678eb5 Compare August 9, 2021 05:40

keszybz requested changes Aug 9, 2021

View reviewed changes

src/generator.rs Outdated Show resolved Hide resolved

src/generator.rs Outdated Show resolved Hide resolved

hswong3i force-pushed the main-mount_options branch 4 times, most recently from 1eab6ef to d074e71 Compare August 9, 2021 09:41

hswong3i force-pushed the main-mount_options branch from d074e71 to f0a866c Compare August 9, 2021 12:06

keszybz mentioned this pull request Aug 10, 2021

fstab-generator: put What= before Where= systemd/systemd#20411

Merged

hswong3i force-pushed the main-mount_options branch from f0a866c to 410ed7d Compare August 10, 2021 10:53

hswong3i force-pushed the main-mount_options branch from 410ed7d to 4b45efd Compare August 10, 2021 10:59

keszybz requested changes Aug 10, 2021

View reviewed changes

man/zram-generator.conf.md Outdated Show resolved Hide resolved

hswong3i force-pushed the main-mount_options branch from 4b45efd to 23cd05b Compare August 10, 2021 13:21

hswong3i force-pushed the main-mount_options branch from 23cd05b to c7c5cf1 Compare August 13, 2021 09:11

keszybz mentioned this pull request Oct 13, 2021

Mount filesystems with "discard" option by default #102

Closed

keszybz closed this Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mount-options: Add Mount Options Support #90

mount-options: Add Mount Options Support #90

hswong3i commented Jul 25, 2021

nabijaczleweli Jul 25, 2021

hswong3i Jul 26, 2021

hswong3i commented Jul 26, 2021

hswong3i commented Jul 28, 2021

keszybz commented Jul 28, 2021

hswong3i commented Jul 28, 2021

keszybz commented Aug 8, 2021

hswong3i commented Aug 8, 2021

keszybz commented Aug 8, 2021

hswong3i commented Aug 9, 2021

keszybz left a comment

hswong3i commented Aug 9, 2021 •

edited

Loading

keszybz commented Aug 10, 2021

keszybz commented Aug 10, 2021 •

edited

Loading

hswong3i commented Aug 10, 2021

hswong3i commented Aug 10, 2021

keszybz commented Oct 13, 2021

mount-options: Add Mount Options Support #90

mount-options: Add Mount Options Support #90

Conversation

hswong3i commented Jul 25, 2021

nabijaczleweli Jul 25, 2021

Choose a reason for hiding this comment

hswong3i Jul 26, 2021

Choose a reason for hiding this comment

hswong3i commented Jul 26, 2021

hswong3i commented Jul 28, 2021

keszybz commented Jul 28, 2021

hswong3i commented Jul 28, 2021

keszybz commented Aug 8, 2021

hswong3i commented Aug 8, 2021

keszybz commented Aug 8, 2021

hswong3i commented Aug 9, 2021

keszybz left a comment

Choose a reason for hiding this comment

hswong3i commented Aug 9, 2021 • edited Loading

keszybz commented Aug 10, 2021

keszybz commented Aug 10, 2021 • edited Loading

hswong3i commented Aug 10, 2021

hswong3i commented Aug 10, 2021

keszybz commented Oct 13, 2021

hswong3i commented Aug 9, 2021 •

edited

Loading

keszybz commented Aug 10, 2021 •

edited

Loading