Linux: mount.zfs(8) didn't propagate dataset properties into mount options #16393

Low-power · 2024-07-28T11:14:21Z

Motivation and Context

This issue is basically #7947; while #13021 attempted fixing it, the fix is
incomplete. Specifically if the root dataset have mountpoint set to
legacy, the mount script in initrd will have to drop -o zfsutil, then the
original issue will appear again.

Description

To reprocude the issue, create a dataset with mountpoint=legacy and
atime=off:

# zfs create -o mountpoint=legacy -o atime=off storage/test
# zfs get mountpoint,atime storage/test
NAME          PROPERTY    VALUE       SOURCE
storage/test  mountpoint  legacy      local
storage/test  atime       off         local

Because mount point setting of this dataset is legacy, the user will have to
use mount.zfs(8), or indirectly use mount -t zfs ..., in order to mount
it; but then the noatime mount option didn't get passed by mount.zfs(8):

# mount -t zfs storage/test /mnt/test/
# grep ^storage/test /proc/mounts
storage/test /mnt/test zfs rw,relatime,xattr,noacl 0 0

The proposed fix is extending the cases of calling zfs_mount_at, without the
requirement of using -o zfsutil. I have carefully read the corresponding
source code, as well related commit log, I think there's no reason to not use
zfs_mount_at without -o zfsutil.

This issue didn't present in FreeBSD; from my testing, dataset with
mountpoint=legacy mounted using mount -t zfs ... have correctly set mount
options based on dataset properties.

How Has This Been Tested?

With the change applied, the noatime option got passed as expected:

# mount.zfs storage/test /mnt/test/
# grep ^storage/test /proc/mounts
storage/test /mnt/test zfs rw,noatime,xattr,noacl 0 0

Also, overriding mount options from command line works too:

# umount /mnt/test 
# mount.zfs storage/test /mnt/test/ -o atime
# grep ^storage/test /proc/mounts 
storage/test /mnt/test zfs rw,xattr,noacl 0 0
# umount /mnt/test 
# mount.zfs storage/test /mnt/test/ -o relatime
# grep ^storage/test /proc/mounts 
storage/test /mnt/test zfs rw,relatime,xattr,noacl 0 0

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

usaleem-ix · 2024-07-29T14:09:10Z

Do we intend to use mount.zfs in initrd to mount the rootfs or is this going to be limited to fixing the permissions for legacy datasets with mount.zfs? Using mount.zfs in initrd breaks snapshots on rootfs. See #14908 and this comment on issue#9461.

Low-power · 2024-07-29T14:36:27Z

Using mount.zfs in initrd breaks snapshots on rootfs.

I was aware this. Since I don't need to mount snapshot for root, I have modified Debian's initrd via /etc/initramfs-tools/scripts/zfs to use mount.zfs instead on my system; otherwise dataset properties would have no chance to be propagated into mount options for root mount.

Do we intend to use mount.zfs in initrd to mount the rootfs or is this going to be limited to fixing the permissions for legacy datasets with mount.zfs?

This fix is focused to generic uses of mount.zfs for mounting mountpoint=legacy datasets, usually interactively by human. Unlike busybox mount that used in initrd, the util-linux mount(8) invokes mount.zfs as an external helper automatically, so running mount -t zfs ... and mount.zfs ... is generally equivalent.

behlendorf

I agree, this should be fine.

behlendorf · 2024-08-19T16:29:16Z

@Low-power could you rebase this PR to get a fresh CI run.

Low-power · 2024-08-20T02:35:58Z

With my local branch updated to HEAD, building of libzpool started to fail:

/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_destroy_dir':
<local-build-path>/../module/zfs/ddt.c:1280: undefined reference to `ddt_log_destroy'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_sync_flush_log_incremental':
<local-build-path>/../module/zfs/ddt.c:1985: undefined reference to `ddt_log_take_first'
/usr/bin/ld: <local-build-path>/../module/zfs/ddt.c:1999: undefined reference to `ddt_log_truncate'
/usr/bin/ld: <local-build-path>/../module/zfs/ddt.c:2005: undefined reference to `ddt_log_checkpoint'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_sync_table_log':
<local-build-path>/../module/zfs/ddt.c:2107: undefined reference to `ddt_log_begin'
/usr/bin/ld: <local-build-path>/../module/zfs/ddt.c:2116: undefined reference to `ddt_log_entry'
/usr/bin/ld: <local-build-path>/../module/zfs/ddt.c:2121: undefined reference to `ddt_log_commit'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_fini':
<local-build-path>/../module/zfs/ddt.c:872: undefined reference to `ddt_log_fini'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_lookup':
<local-build-path>/../module/zfs/ddt.c:1095: undefined reference to `ddt_log_take_key'
/usr/bin/ld: <local-build-path>/../module/zfs/ddt.c:1099: undefined reference to `ddt_log_take_key'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_table_alloc':
<local-build-path>/../module/zfs/ddt.c:1459: undefined reference to `ddt_log_alloc'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_load':
<local-build-path>/../module/zfs/ddt.c:1529: undefined reference to `ddt_log_load'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_table_free':
<local-build-path>/../module/zfs/ddt.c:1476: undefined reference to `ddt_log_free'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_sync_flush_log':
<local-build-path>/../module/zfs/ddt.c:2069: undefined reference to `ddt_log_swap'
/usr/bin/ld: module/zfs/.libs/libzpool_la-ddt.o: in function `ddt_init':
<local-build-path>/../module/zfs/ddt.c:866: undefined reference to `ddt_log_init'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:6489: libzpool.la] Error 1
make[2]: Leaving directory '<local-build-path>'
make[1]: *** [Makefile:12320: all-recursive] Error 1
make[1]: Leaving directory '<local-build-path>'
make: *** [Makefile:4648: all] Error 2

Update

Nevermind, it just needs an autoreconf(1) (Makefile.in was outdated). What weird is that this should have be triggered automatically by make(1).

behlendorf · 2024-08-22T00:06:20Z

This sure looked safe on initial review but the CI tells a different story. This is causing the .zfs snapshot tests to block although I haven't look any deeper than that.

https://build.openzfs.org/builders/CentOS%209%20x86_64%20%28TEST%29/builds/3710/steps/shell_4/logs/console

Commit 329e2ff has made mount.zfs(8) to call libzfs function 'zfs_mount_at', in order to propagate dataset properties into mount options. This fix however, is limited to a special use case where mount.zfs(8) is used in initrd with option '-o zfsutil'. If either initrd or the user need to use mount.zfs(8) to mount a file system with 'mountpoint' set to 'legacy', '-o zfsutil' can't be used and the original issue openzfs#7947 will still happen. Since the existing code already excluded the possibility of calling 'zfs_mount_at' when it was invoked as a helper program from zfs(8), by checking 'ZFS_MOUNT_HELPER' environment variable, it makes no sense to avoid calling 'zfs_mount_at' without '-o zfsutil'. An exception however, is when mount.zfs(8) was invoked with '-o remount' to update the mount options for an existing mount point. In this case call mount(2) directly without modifying the mount options passed from command line. Signed-off-by: WHR <whr@rivoreo.one>

The last commit which makes mount.zfs(8) to call 'zfs_mount_at' apparently caused it to trigger an automount for the snapshot directory. When the helper was invoked as a result of a snapshot automount, an infinite recursion will occur. Since the need of invoking user mode mount(8) for automounting was to overcome that the 'vfs_kern_mount' being GPL-only, just run mount(8) without the mount.zfs(8) helper by adding option '-i'. Signed-off-by: WHR <whr@rivoreo.one>

Low-power · 2024-08-22T07:19:56Z

Excluded -o remount cases.

behlendorf

Looks good, thanks for sorting that out.

Commit 329e2ff has made mount.zfs(8) to call libzfs function 'zfs_mount_at', in order to propagate dataset properties into mount options. This fix however, is limited to a special use case where mount.zfs(8) is used in initrd with option '-o zfsutil'. If either initrd or the user need to use mount.zfs(8) to mount a file system with 'mountpoint' set to 'legacy', '-o zfsutil' can't be used and the original issue openzfs#7947 will still happen. Since the existing code already excluded the possibility of calling 'zfs_mount_at' when it was invoked as a helper program from zfs(8), by checking 'ZFS_MOUNT_HELPER' environment variable, it makes no sense to avoid calling 'zfs_mount_at' without '-o zfsutil'. An exception however, is when mount.zfs(8) was invoked with '-o remount' to update the mount options for an existing mount point. In this case call mount(2) directly without modifying the mount options passed from command line. Furthermore, don't run mount.zfs(8) helper for automounting snapshot. The above change to make mount.zfs(8) to call 'zfs_mount_at' apparently caused it to trigger an automount for the snapshot directory. When the helper was invoked as a result of a snapshot automount, an infinite recursion will occur. Since the need of invoking user mode mount(8) for automounting was to overcome that the 'vfs_kern_mount' being GPL-only, just run mount(8) without the mount.zfs(8) helper by adding option '-i'. Reviewed-by: Umer Saleem <usaleem@ixsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <whr@rivoreo.one> Closes openzfs#16393

behlendorf approved these changes Aug 16, 2024

View reviewed changes

behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Aug 16, 2024

usaleem-ix approved these changes Aug 19, 2024

View reviewed changes

Low-power force-pushed the mount-zfs-use-properties branch from 6b7aa6a to 65aeb67 Compare August 20, 2024 02:50

Low-power force-pushed the mount-zfs-use-properties branch 2 times, most recently from d70f685 to a349bda Compare August 22, 2024 05:29

Low-power added 2 commits August 22, 2024 15:04

Low-power force-pushed the mount-zfs-use-properties branch from a349bda to d3b568f Compare August 22, 2024 07:10

behlendorf approved these changes Aug 22, 2024

View reviewed changes

behlendorf merged commit 34118ea into openzfs:master Aug 23, 2024
21 of 23 checks passed

yocalebo mentioned this pull request Sep 9, 2024

NAS-130821 / 24.10.0 / Remove noexec permission for /etc truenas/scale-build#708

Merged

usaleem-ix mentioned this pull request Sep 9, 2024

NAS-127825 / 25.04 / Remove noexec permission for /etc truenas/scale-build#709

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linux: mount.zfs(8) didn't propagate dataset properties into mount options #16393

Linux: mount.zfs(8) didn't propagate dataset properties into mount options #16393

Low-power commented Jul 28, 2024 •

edited

Loading

usaleem-ix commented Jul 29, 2024

Low-power commented Jul 29, 2024

behlendorf left a comment

behlendorf commented Aug 19, 2024

Low-power commented Aug 20, 2024 •

edited

Loading

behlendorf commented Aug 22, 2024

Low-power commented Aug 22, 2024

behlendorf left a comment

Linux: mount.zfs(8) didn't propagate dataset properties into mount options #16393

Linux: mount.zfs(8) didn't propagate dataset properties into mount options #16393

Conversation

Low-power commented Jul 28, 2024 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

usaleem-ix commented Jul 29, 2024

Low-power commented Jul 29, 2024

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf commented Aug 19, 2024

Low-power commented Aug 20, 2024 • edited Loading

Update

behlendorf commented Aug 22, 2024

Low-power commented Aug 22, 2024

behlendorf left a comment

Choose a reason for hiding this comment

Low-power commented Jul 28, 2024 •

edited

Loading

Low-power commented Aug 20, 2024 •

edited

Loading