From f41c755d61baa78bffb06a3900b1f0b4b465ec94 Mon Sep 17 00:00:00 2001 From: Tomohiro Kusumi Date: Sun, 16 Jun 2019 21:48:14 +0900 Subject: [PATCH] Fix race in parallel mount's thread dispatching algorithm Strategy of parallel mount is as follows. 1) Initial thread dispatching selects sets of mount points that don't have dependencies on other sets, hence threads can/should go lock-less and shouldn't race with other threads for other sets. Each thread dispatched corresponds to top level directory which may or may not have datasets mounted on sub directories. 2) Subsequent recursive thread dispatching for each thread from 1) is to mount datasets for each set of mount points. The mount points within each set have dependencies (i.e. child directories), so child directories are processed only after parent directory completes. The problem is that initial thread dispatching can spawn >1 threads for datasets with the same top level directory, and this puts threads under race condition. This appeared as mount issue on ZoL for ZoL having different timing regarding mount(2) execution due to fork(2)/exec(2) of mount(8) on mount. Fix it by libzfs_path_contains() returning true for same paths. Closes #8450 Closes #8833 Signed-off-by: Tomohiro Kusumi --- lib/libzfs/libzfs_mount.c | 5 +- tests/runfiles/linux.run | 5 +- .../functional/cli_root/zfs_mount/Makefile.am | 1 + .../cli_root/zfs_mount/zfs_mount_test_race.sh | 93 +++++++++++++++++++ 4 files changed, 100 insertions(+), 4 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_test_race.sh diff --git a/lib/libzfs/libzfs_mount.c b/lib/libzfs/libzfs_mount.c index 39ca2be05019..301113484d83 100644 --- a/lib/libzfs/libzfs_mount.c +++ b/lib/libzfs/libzfs_mount.c @@ -1310,12 +1310,13 @@ mountpoint_cmp(const void *arga, const void *argb) } /* - * Return true if path2 is a child of path1. + * Return true if path2 is a child of path1 or path2 equals path1. */ static boolean_t libzfs_path_contains(const char *path1, const char *path2) { - return (strstr(path2, path1) == path2 && path2[strlen(path1)] == '/'); + return ((strcmp(path1, path2) == 0) || + (strstr(path2, path1) == path2 && path2[strlen(path1)] == '/')); } /* diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run index b73528e43a0c..9b201b745f92 100644 --- a/tests/runfiles/linux.run +++ b/tests/runfiles/linux.run @@ -181,8 +181,9 @@ tests = ['zfs_mount_001_pos', 'zfs_mount_002_pos', 'zfs_mount_003_pos', 'zfs_mount_004_pos', 'zfs_mount_005_pos', 'zfs_mount_006_pos', 'zfs_mount_007_pos', 'zfs_mount_008_pos', 'zfs_mount_009_neg', 'zfs_mount_010_neg', 'zfs_mount_011_neg', 'zfs_mount_012_neg', - 'zfs_mount_all_001_pos', 'zfs_mount_encrypted', 'zfs_mount_remount', - 'zfs_multi_mount', 'zfs_mount_all_fail', 'zfs_mount_all_mountpoints'] + 'zfs_mount_all_001_pos', 'zfs_mount_all_fail', 'zfs_mount_all_mountpoints', + 'zfs_mount_encrypted', 'zfs_mount_remount', 'zfs_mount_test_race', + 'zfs_multi_mount'] tags = ['functional', 'cli_root', 'zfs_mount'] [tests/functional/cli_root/zfs_program] diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am index b2de98934b74..c208a1c378dc 100644 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/Makefile.am @@ -19,6 +19,7 @@ dist_pkgdata_SCRIPTS = \ zfs_mount_all_mountpoints.ksh \ zfs_mount_encrypted.ksh \ zfs_mount_remount.ksh \ + zfs_mount_test_race.sh \ zfs_multi_mount.ksh dist_pkgdata_DATA = \ diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_test_race.sh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_test_race.sh new file mode 100755 index 000000000000..08a0746fdd2b --- /dev/null +++ b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_test_race.sh @@ -0,0 +1,93 @@ +#!/bin/ksh + +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# http://www.illumos.org/license/CDDL. +# + +# +# Copyright (c) 2019 by Tomohiro Kusumi. All rights reserved. +# + +. $STF_SUITE/include/libtest.shlib + +# +# DESCRIPTION: +# Verify parallel mount ordering is consistent. +# See github.com/zfsonlinux/zfs/issues/8833 for details. +# +# STRATEGY: +# 1. Create pools and filesystems. +# 2. Set same mount point for >1 datasets. +# 3. Unmount all datasets. +# 4. Mount all datasets. +# 5. Unmount all datasets. +# 6. Mount all datasets. +# + +verify_runnable "both" + +TMPDIR=${TMPDIR:-$TEST_BASE_DIR} +MNTPT=$TMPDIR/zfs_mount_test_race_mntpt + +DISK1="$TMPDIR/zfs_mount_test_race_disk1" +TESTPOOL1=zfs_mount_test_race_tp1 +TESTFS1=zfs_mount_test_race_tf1 + +DISK2="$TMPDIR/zfs_mount_test_race_disk2" +TESTPOOL2=zfs_mount_test_race_tp2 +TESTFS2=zfs_mount_test_race_tf2 + +log_must zfs unmount -a # unmount zfs mounts from previous tests +log_must rm -rf $MNTPT +log_must rm -rf /$TESTPOOL1 +log_must rm -rf /$TESTPOOL2 + +function cleanup +{ + zpool destroy $TESTPOOL1 + zpool destroy $TESTPOOL2 + rm -rf $MNTPT + rm -rf /$TESTPOOL1 + rm -rf /$TESTPOOL2 + rm -f $DISK1 + rm -f $DISK2 + log_must zfs mount -a # remount unmounted zfs mounts +} +log_onexit cleanup + +log_note "Verify parallel mount ordering is consistent" + +log_must truncate -s $MINVDEVSIZE $DISK1 +log_must truncate -s $MINVDEVSIZE $DISK2 + +log_must zpool create -f $TESTPOOL1 $DISK1 +log_must zpool create -f $TESTPOOL2 $DISK2 + +log_must zfs create $TESTPOOL1/$TESTFS1 +log_must zfs create $TESTPOOL2/$TESTFS2 + +log_must zfs set mountpoint=none $TESTPOOL1 +log_must zfs set mountpoint=$MNTPT $TESTPOOL1/$TESTFS1 + +# Note that unmount can fail (due to race condition on `zfs mount -a`) with or +# without `canmount=off`. The race has nothing to do with canmount property. +log_must zfs set canmount=off $TESTPOOL2 +log_must zfs set mountpoint=$MNTPT $TESTPOOL2 + +log_must ismounted $TESTPOOL1/$TESTFS1 +log_must ismounted $TESTPOOL2/$TESTFS2 +log_must zfs unmount -a +log_must zfs mount -a + +log_must ismounted $TESTPOOL1/$TESTFS1 +log_must ismounted $TESTPOOL2/$TESTFS2 +log_must zfs unmount -a # verify this succeeds + +log_pass "Verify parallel mount ordering is consistent passed"