From 2c3d7283b46f0690b671ea31a1577aefe0e8884d Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Tue, 8 Jun 2021 07:36:43 -0600 Subject: [PATCH] libzfs: On FreeBSD, use MNT_NOWAIT with getfsstat `getfsstat(2)` is used to retrieve the list of mounted file systems, which libzfs uses when fetching properties like mountpoint, atime, setuid, etc. The `mode` parameter may be `MNT_NOWAIT`, which uses information in the VFS's cache, or `MNT_WAIT`, which effectively does a `statfs` on every single mounted file system in order to fetch the most up-to-date information. As far as I can tell, the only fields that libzfs cares about are the filesystem's name, mountpoint, fstypename, and mount flags. Those things are always updated on mount and unmount, so they will always be accurate in the VFS's mount cache except in two circumstances: 1) When a file system is busy unmounting 2) When a ZFS file system changes the value of a mount-overridable property like atime or setuid, but doesn't remount the file system. Right now that only happens when the property is changed by an unprivileged user who has delegated authority to change the property but not to mount the dataset. But perhaps libzfs could choose to do it for other reasons in the future. Switching to `MNT_NOWAIT` will greatly improve speed with no downside, as long as we explicitly update the mount cache whenever we change a mount-overridable property. For comparison, Illumos gets this information using the native `getmntany` and `getmntent` functions, which also use cached information. The illumos function that would refresh the cache, `resetmnttab`, is never called by libzfs. And on GNU/Linux, `getmntany` and `getmntent` don't even communicate with the kernel directly. They simply parse the file they are given, which is usually /etc/mtab or /proc/mounts. Perhaps the implementation of /proc/mounts is synchronous, ala MNT_WAIT; I don't know. Sponsored-by: Axcient Reviewed-by: Ryan Moeller Reviewed-by: Brian Behlendorf Signed-off-by: Alan Somers Closes: #12091 --- include/sys/zfs_ioctl_impl.h | 1 + lib/libspl/os/freebsd/mnttab.c | 4 ++-- module/os/freebsd/zfs/zfs_ioctl_os.c | 17 +++++++++++++++++ module/os/linux/zfs/zfs_ioctl_os.c | 6 ++++++ module/zfs/zfs_ioctl.c | 27 +++++++++++++++++++++++++++ 5 files changed, 53 insertions(+), 2 deletions(-) diff --git a/include/sys/zfs_ioctl_impl.h b/include/sys/zfs_ioctl_impl.h index 787475cf39..5774e9e0e3 100644 --- a/include/sys/zfs_ioctl_impl.h +++ b/include/sys/zfs_ioctl_impl.h @@ -82,6 +82,7 @@ void zfs_ioctl_register(const char *, zfs_ioc_t, zfs_ioc_func_t *, boolean_t, boolean_t, const zfs_ioc_key_t *, size_t); uint64_t zfs_max_nvlist_src_size_os(void); +void zfs_ioctl_update_mount_cache(const char *dsname); void zfs_ioctl_init_os(void); boolean_t zfs_vfs_held(zfsvfs_t *); diff --git a/lib/libspl/os/freebsd/mnttab.c b/lib/libspl/os/freebsd/mnttab.c index 5b9e6429d9..bd3e3e4e3e 100644 --- a/lib/libspl/os/freebsd/mnttab.c +++ b/lib/libspl/os/freebsd/mnttab.c @@ -140,14 +140,14 @@ statfs_init(void) free(gsfs); gsfs = NULL; } - allfs = getfsstat(NULL, 0, MNT_WAIT); + allfs = getfsstat(NULL, 0, MNT_NOWAIT); if (allfs == -1) goto fail; gsfs = malloc(sizeof (gsfs[0]) * allfs * 2); if (gsfs == NULL) goto fail; allfs = getfsstat(gsfs, (long)(sizeof (gsfs[0]) * allfs * 2), - MNT_WAIT); + MNT_NOWAIT); if (allfs == -1) goto fail; sfs = realloc(gsfs, allfs * sizeof (gsfs[0])); diff --git a/module/os/freebsd/zfs/zfs_ioctl_os.c b/module/os/freebsd/zfs/zfs_ioctl_os.c index 0e0c16033b..7f7e2b72c5 100644 --- a/module/os/freebsd/zfs/zfs_ioctl_os.c +++ b/module/os/freebsd/zfs/zfs_ioctl_os.c @@ -138,6 +138,23 @@ zfs_ioc_nextboot(const char *unused, nvlist_t *innvl, nvlist_t *outnvl) return (error); } +/* Update the VFS's cache of mountpoint properties */ +void +zfs_ioctl_update_mount_cache(const char *dsname) +{ + zfsvfs_t *zfsvfs; + + if (getzfsvfs(dsname, &zfsvfs) == 0) { + struct mount *mp = zfsvfs->z_vfs; + VFS_STATFS(mp, &mp->mnt_stat); + zfs_vfs_rele(zfsvfs); + } + /* + * Ignore errors; we can't do anything useful if either getzfsvfs or + * VFS_STATFS fails. + */ +} + uint64_t zfs_max_nvlist_src_size_os(void) { diff --git a/module/os/linux/zfs/zfs_ioctl_os.c b/module/os/linux/zfs/zfs_ioctl_os.c index 812f9c0ea1..79b9d777dc 100644 --- a/module/os/linux/zfs/zfs_ioctl_os.c +++ b/module/os/linux/zfs/zfs_ioctl_os.c @@ -212,6 +212,12 @@ zfs_max_nvlist_src_size_os(void) return (MIN(ptob(zfs_totalram_pages) / 4, 128 * 1024 * 1024)); } +/* Update the VFS's cache of mountpoint properties */ +void +zfs_ioctl_update_mount_cache(const char *dsname) +{ +} + void zfs_ioctl_init_os(void) { diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index 7f929df167..b0eee81beb 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -2521,6 +2521,26 @@ zfs_prop_set_special(const char *dsname, zprop_source_t source, return (err); } +static boolean_t +zfs_is_namespace_prop(zfs_prop_t prop) +{ + switch (prop) { + + case ZFS_PROP_ATIME: + case ZFS_PROP_RELATIME: + case ZFS_PROP_DEVICES: + case ZFS_PROP_EXEC: + case ZFS_PROP_SETUID: + case ZFS_PROP_READONLY: + case ZFS_PROP_XATTR: + case ZFS_PROP_NBMAND: + return (B_TRUE); + + default: + return (B_FALSE); + } +} + /* * This function is best effort. If it fails to set any of the given properties, * it continues to set as many as it can and returns the last error @@ -2540,6 +2560,7 @@ zfs_set_prop_nvlist(const char *dsname, zprop_source_t source, nvlist_t *nvl, int rv = 0; uint64_t intval; const char *strval; + boolean_t should_update_mount_cache = B_FALSE; nvlist_t *genericnvl = fnvlist_alloc(); nvlist_t *retrynvl = fnvlist_alloc(); @@ -2637,6 +2658,9 @@ retry: fnvlist_add_int32(errlist, propname, err); rv = err; } + + if (zfs_is_namespace_prop(prop)) + should_update_mount_cache = B_TRUE; } if (nvl != retrynvl && !nvlist_empty(retrynvl)) { @@ -2685,6 +2709,9 @@ retry: } } } + if (should_update_mount_cache) + zfs_ioctl_update_mount_cache(dsname); + nvlist_free(genericnvl); nvlist_free(retrynvl);