Skip to content

Commit

Permalink
Fix parallel mount's thread dispatching algorithm
Browse files Browse the repository at this point in the history
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 openzfs#8450
Closes openzfs#8833

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
  • Loading branch information
kusumi committed Jun 17, 2019
1 parent c1b5801 commit b6f1221
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 4 deletions.
5 changes: 3 additions & 2 deletions lib/libzfs/libzfs_mount.c
Original file line number Diff line number Diff line change
Expand Up @@ -1302,12 +1302,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)] == '/');
}

/*
Expand Down
3 changes: 2 additions & 1 deletion tests/runfiles/linux.run
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ tests = ['zfs_mount_001_pos', 'zfs_mount_002_pos', 'zfs_mount_003_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_multi_mount', 'zfs_mount_all_fail', 'zfs_mount_all_mountpoints',
'zfs_mount_test_race']
tags = ['functional', 'cli_root', 'zfs_mount']

[tests/functional/cli_root/zfs_program]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ dist_pkgdata_SCRIPTS = \
zfs_mount_all_mountpoints.ksh \
zfs_mount_encrypted.ksh \
zfs_mount_remount.ksh \
zfs_multi_mount.ksh
zfs_multi_mount.ksh \
zfs_mount_test_race.sh

dist_pkgdata_DATA = \
zfs_mount.cfg \
Expand Down
Original file line number Diff line number Diff line change
@@ -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 result 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"

log_note "Verify parallel mount result is consistent"

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
log_must zfs mount -a # remount unmounted zfs mounts
}
log_onexit cleanup

log_note "Testing github.com/zfsonlinux/zfs/issues/8833"

log_must mkfile $MINVDEVSIZE $DISK1
log_must mkfile $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 zfs list -o name,mounted,canmount,mountpoint
log_must mount | grep zfs
log_must zfs unmount -a
log_must zfs mount -a

log_must zfs list -o name,mounted,canmount,mountpoint
log_must mount | grep zfs
log_must zfs unmount -a # verify this succeeds

log_pass "Verify parallel mount result is consistent passed"

0 comments on commit b6f1221

Please sign in to comment.