| Message ID | 20180503034340.GA20908@thunk.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 3, 2018 at 6:43 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote: [...] > From 8b40b28866dca119d0c807c31ae48f153ec2dc53 Mon Sep 17 00:00:00 2001 > From: Theodore Ts'o <tytso@mit.edu> > Date: Sat, 17 Sep 2016 19:08:18 -0400 > Subject: [PATCH] common: add support for the "local" file system type > > It is sometimes useful to be able to test the local file system > provided in a restricted execution environment (such as that which is > provided by Docker, for example) where it is not possible to mount and > unmount the file system under test. > > To support this test case, add support for a new file system type > called "local". The TEST_DEV and SCRATCH_DEV should be have a > non-block device format (e.g., local:/test or local:/scratch), and the > TEST_DIR and SCRATCH_MNT directories should be pre-existing > directories provided by the execution environment. Ted, This looks like a very useful feature, but I suspect that some bits of the patch may be a bit too specific to your use case (see below) - I may be wrong. I bet there is a large variety of out of tree xfstests patches named "Add support for XXX fs" that could be avoided with this feature. The ultimate proof would be to demonstrate the usefulness of the feature to more than a single use case - how about FUSE passthrough example? Perhaps FSTYP=user would be more descriptive in general to the use case at hand, because 'local' is usually the counter of 'remote', but I'm fine with FSTYP=local. Another way to "market" the feature is FSTYP=generic, which is the prototype of all other filesystems. Naturally, it only runs tests under tests/generic (as FSTYP=local does) and any test that requires an operation that has no generic implementation is notrun. > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- > common/rc | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 70 insertions(+), 3 deletions(-) > > diff --git a/common/rc b/common/rc > index 9ffab7fd..d5cb0fe4 100644 > --- a/common/rc > +++ b/common/rc > @@ -351,6 +351,18 @@ _supports_filetype() > esac > } > > +_local_validate_mount_opt() > +{ > + case "$*" in > + ro|ro,*|*,ro) _notrun "ro mount option not supported" ;; > + *nosuid*) _notrun "nosuid mount option not supported" ;; > + *noatime*) _notrun "noatime mount option not supported" ;; > + *relatime*) _notrun "relatime mount option not supported" ;; > + *diratime*) _notrun "diratime mount option not supported" ;; > + *strictatime*) _notrun "strictatime mount option not supported" ;; > + esac > +} Why specifically these mount options? Is this really generic? > + > # mount scratch device with given options but don't check mount status > _try_scratch_mount() > { > @@ -376,6 +388,9 @@ _scratch_unmount() > btrfs) > $UMOUNT_PROG $SCRATCH_MNT > ;; > + local) > + rm -rf $SCRATCH_MNT/* > + ;; _scratch_mkfs already does that. Why does it make sense in _scratch_unmount? > *) > $UMOUNT_PROG $SCRATCH_DEV > ;; > @@ -386,6 +401,10 @@ _scratch_remount() > { > local opts="$1" > > + if [ "$FSTYP" = "local" ]; then > + _local_validate_mount_opt "$*" > + return 0; > + fi It makes more sense to me to _require_scratch_remount for tests that need to remount in the beginning of tests - yeh that's a bit more work. > if test -n "$opts"; then > mount -o "remount,$opts" $SCRATCH_MNT > fi > @@ -395,7 +414,7 @@ _scratch_cycle_mount() > { > local opts="$1" > > - if [ "$FSTYP" = tmpfs ]; then > + if [ "$FSTYP" = "tmpfs" -o "$FSTYP" = "local" ] ; then > _scratch_remount "$opts" That seems like cheating - seems better to implement and use _require_scratch_cycle_mount > return > fi > @@ -429,6 +448,10 @@ _test_mount() > _overlay_test_mount $* > return $? > fi > + if [ "$FSTYP" == "local" ]; then > + mkdir -p $TEST_DIR > + return $? > + fi What is the desired logic here? can you add a comment? Seems to me that we need to verify there is something mounted at $TEST_DIR. no? > _test_options mount > _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR > } > @@ -437,7 +460,7 @@ _test_unmount() > { > if [ "$FSTYP" == "overlay" ]; then > _overlay_test_unmount > - else > + elif [ "$FSTYP" != "local" ]; then > $UMOUNT_PROG $TEST_DEV > fi > } > @@ -723,7 +746,7 @@ _scratch_mkfs() > local mkfs_status > > case $FSTYP in > - nfs*|cifs|ceph|overlay|glusterfs|pvfs2|9p) > + nfs*|cifs|ceph|overlay|glusterfs|pvfs2|9p|local) > # unable to re-create this fstyp, just remove all files in > # $SCRATCH_MNT to avoid EEXIST caused by the leftover files > # created in previous runs > @@ -1465,6 +1488,10 @@ _check_mounted_on() > local mnt=$4 > local type=$5 > > + if [ "$FSTYP" == "local" ]; then > + return 0 > + fi > + Shouldn't we check that "something" is mounted on $mnt? > # find $dev as the source, and print result in "$dev $mnt" format > local mount_rec=`findmnt -rncv -S $dev -o SOURCE,TARGET` > [ -n "$mount_rec" ] || return 1 # 1 = not mounted > @@ -1562,6 +1589,13 @@ _require_scratch_nocheck() > _notrun "this test requires a valid \$SCRATCH_MNT" > fi > ;; > + local) > + if [ -z "$SCRATCH_DEV" -o ! -d "$SCRATCH_MNT" ]; > + then > + _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV" > + fi > + return 0 > + ;; > *) > if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ] > then > @@ -1683,6 +1717,13 @@ _require_test() > _notrun "this test requires a valid \$TEST_DIR" > fi > ;; > + local) > + if [ -z "$TEST_DEV" -o ! -d "$TEST_DIR" ]; > + then > + _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV" > + fi > + return 0 > + ;; > *) > if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ] > then > @@ -2438,6 +2479,10 @@ _remount() > local device=$1 > local mode=$2 > > + if [ "$FSTYP" == "local" ] ; then > + return 0 > + fi > + > if ! mount -o remount,$mode $device > then > echo "_remount: failed to remount filesystem on $device as $mode" > @@ -2483,6 +2528,10 @@ _mount_or_remount_rw() > local device=$2 > local mountpoint=$3 > > + if [ "$FSTYP" == "local" ] ; then > + return 0 > + fi > + > if [ $USE_REMOUNT -eq 0 ]; then > if [ "$FSTYP" != "overlay" ]; then > _mount -t $FSTYP $mount_opts $device $mountpoint > @@ -2636,6 +2685,9 @@ _check_test_fs() > ubifs) > # there is no fsck program for ubifs yet > ;; > + local) > + # no way to check consistency for local > + ;; > *) > _check_generic_filesystem $TEST_DEV > ;; > @@ -2691,6 +2743,9 @@ _check_scratch_fs() > ubifs) > # there is no fsck program for ubifs yet > ;; > + local) > + # no way to check consistency for local > + ;; > *) > _check_generic_filesystem $device > ;; > @@ -3003,6 +3058,9 @@ _require_fio() > # Does freeze work on this fs? > _require_freeze() > { > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support freeze" When users read this warning they may be confused, same for shutdown/dax/morecovery. Something like "user defined fs does not support freeze" Thanks, Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 03, 2018 at 12:22:42PM +0300, Amir Goldstein wrote: > > This looks like a very useful feature, but I suspect that some bits of the > patch may be a bit too specific to your use case (see below) - I may be wrong. The primary characteristic of "local" is there nothing to mount or unmount. So for example, it should be possible to use this Microsoft's Subsystem for Linux. > The ultimate proof would be to demonstrate the usefulness of the feature > to more than a single use case - how about FUSE passthrough example? > Perhaps FSTYP=user would be more descriptive in general to the use > case at hand, because 'local' is usually the counter of 'remote', > but I'm fine with FSTYP=local. It certainly wouldn't be problem to demo using the local file system type for FUSE --- but it would not be _ideal_ for fuse, since fuse has the concept of mounting and unmounting. > > +{ > > + case "$*" in > > + ro|ro,*|*,ro) _notrun "ro mount option not supported" ;; > > + *nosuid*) _notrun "nosuid mount option not supported" ;; > > + *noatime*) _notrun "noatime mount option not supported" ;; > > + *relatime*) _notrun "relatime mount option not supported" ;; > > + *diratime*) _notrun "diratime mount option not supported" ;; > > + *strictatime*) _notrun "strictatime mount option not supported" ;; > > + esac > > +} > > Why specifically these mount options? Is this really generic? The local file system type does not support mount, umount, or remount command. So there is no way to modulate noatime, relatime, etc. So this would be useful if someone wanted to test Windows System for Linux, for example. > > + > > # mount scratch device with given options but don't check mount status > > _try_scratch_mount() > > { > > @@ -376,6 +388,9 @@ _scratch_unmount() > > btrfs) > > $UMOUNT_PROG $SCRATCH_MNT > > ;; > > + local) > > + rm -rf $SCRATCH_MNT/* > > + ;; > > _scratch_mkfs already does that. Why does it make sense in _scratch_unmount? Just for cleanup purposes. > > @@ -386,6 +401,10 @@ _scratch_remount() > > { > > local opts="$1" > > > > + if [ "$FSTYP" = "local" ]; then > > + _local_validate_mount_opt "$*" > > + return 0; > > + fi > > It makes more sense to me to _require_scratch_remount > for tests that need to remount in the beginning of tests - yeh > that's a bit more work. There are some tests where the remount operation is just to reset the file system. So the test is just to unmount and remount the file system, and making sure the file status are the same. So I didn't want to block all remounts; instead, to let some remounts be no-op's so the rest of the test could still be used to provide test coverage, and only block those remounts which were modulating things like ro/rw, noatime, etc. > > @@ -395,7 +414,7 @@ _scratch_cycle_mount() > > { > > local opts="$1" > > > > - if [ "$FSTYP" = tmpfs ]; then > > + if [ "$FSTYP" = "tmpfs" -o "$FSTYP" = "local" ] ; then > > _scratch_remount "$opts" > > That seems like cheating - seems better to implement > and use _require_scratch_cycle_mount Again, I didn't want to end up skipping a huge number of tests. I didn't consider this "cheating"; I considered it a case of "let's maximize test coverage". > > + if [ "$FSTYP" == "local" ]; then > > + mkdir -p $TEST_DIR > > + return $? > > + fi > > What is the desired logic here? can you add a comment? > Seems to me that we need to verify there is something mounted > at $TEST_DIR. no? Again -- the big thing about the "local" file system is there is no way to mount, umount, or remount. So no block device, no mount point, nothing to shutdown, etc. The mkdir -p isn't strictly necessary here. It could be done by the kvm-xfstests or whatever is running the xfstests. It was more of a safety thing, but if people don't like it, we can drop it. > > @@ -1465,6 +1488,10 @@ _check_mounted_on() > > local mnt=$4 > > local type=$5 > > > > + if [ "$FSTYP" == "local" ]; then > > + return 0 > > + fi > > + > > Shouldn't we check that "something" is mounted on $mnt? Nope. See above. "local" means never having to say you need to mount something. > > @@ -3003,6 +3058,9 @@ _require_fio() > > # Does freeze work on this fs? > > _require_freeze() > > { > > + if [ "$FSTYP" == "local" ]; then > > + _notrun "local does not support freeze" > > When users read this warning they may be confused, > same for shutdown/dax/morecovery. > Something like "user defined fs does not support freeze" I think you have a very different idea of what "local" means. The basic model was that this is something where the file system is provided for use by the docker infrastructure (or by Windows 10 in the WSL case), and so the docker job can't actually mount or unmount the file system. However, it is still desirable to check to see how POSIX compliant the underlying file system might be, and it can also be used to exercise the underlying file system. I'm not tied to the name "local", but for me what it means is "the local file system". If people want to nominate a different name, I'm certainly open to suggestions. - Ted -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 3, 2018 at 9:35 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote: > On Thu, May 03, 2018 at 12:22:42PM +0300, Amir Goldstein wrote: >> >> This looks like a very useful feature, but I suspect that some bits of the >> patch may be a bit too specific to your use case (see below) - I may be wrong. > > The primary characteristic of "local" is there nothing to mount or > unmount. So for example, it should be possible to use this > Microsoft's Subsystem for Linux. > >> The ultimate proof would be to demonstrate the usefulness of the feature >> to more than a single use case - how about FUSE passthrough example? >> Perhaps FSTYP=user would be more descriptive in general to the use >> case at hand, because 'local' is usually the counter of 'remote', >> but I'm fine with FSTYP=local. > > It certainly wouldn't be problem to demo using the local file system > type for FUSE --- but it would not be _ideal_ for fuse, since fuse has > the concept of mounting and unmounting. > >> > +{ >> > + case "$*" in >> > + ro|ro,*|*,ro) _notrun "ro mount option not supported" ;; >> > + *nosuid*) _notrun "nosuid mount option not supported" ;; >> > + *noatime*) _notrun "noatime mount option not supported" ;; >> > + *relatime*) _notrun "relatime mount option not supported" ;; >> > + *diratime*) _notrun "diratime mount option not supported" ;; >> > + *strictatime*) _notrun "strictatime mount option not supported" ;; >> > + esac >> > +} >> >> Why specifically these mount options? Is this really generic? > > The local file system type does not support mount, umount, or remount > command. So there is no way to modulate noatime, relatime, etc. So > this would be useful if someone wanted to test Windows System for > Linux, for example. > >> > + >> > # mount scratch device with given options but don't check mount status >> > _try_scratch_mount() >> > { >> > @@ -376,6 +388,9 @@ _scratch_unmount() >> > btrfs) >> > $UMOUNT_PROG $SCRATCH_MNT >> > ;; >> > + local) >> > + rm -rf $SCRATCH_MNT/* >> > + ;; >> >> _scratch_mkfs already does that. Why does it make sense in _scratch_unmount? > > Just for cleanup purposes. > >> > @@ -386,6 +401,10 @@ _scratch_remount() >> > { >> > local opts="$1" >> > >> > + if [ "$FSTYP" = "local" ]; then >> > + _local_validate_mount_opt "$*" >> > + return 0; >> > + fi >> >> It makes more sense to me to _require_scratch_remount >> for tests that need to remount in the beginning of tests - yeh >> that's a bit more work. > > There are some tests where the remount operation is just to reset the > file system. So the test is just to unmount and remount the file > system, and making sure the file status are the same. So I didn't > want to block all remounts; instead, to let some remounts be no-op's > so the rest of the test could still be used to provide test coverage, > and only block those remounts which were modulating things like ro/rw, > noatime, etc. > >> > @@ -395,7 +414,7 @@ _scratch_cycle_mount() >> > { >> > local opts="$1" >> > >> > - if [ "$FSTYP" = tmpfs ]; then >> > + if [ "$FSTYP" = "tmpfs" -o "$FSTYP" = "local" ] ; then >> > _scratch_remount "$opts" >> >> That seems like cheating - seems better to implement >> and use _require_scratch_cycle_mount > > Again, I didn't want to end up skipping a huge number of tests. I > didn't consider this "cheating"; I considered it a case of "let's > maximize test coverage". > OK. I've used _scratch_cycle_mount as well in exportfs tests to make sure caches are evicted, so perhaps the "not cheating" edition of _scratch_cycle_mount for local should at least drop caches? Thanks, Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 02, 2018 at 11:43:40PM -0400, Theodore Y. Ts'o wrote: > About a year and half ago, I sent the patch attached below to add > support for a "local" file system type where the file system could not > be mounted or dismounted, but where writes to TEST_DIR and SCRATCH_MNT > would be testing the file system in question. At the time, I couldn't > describe the use case in any greater detail than what I had in the > commit description, and Dave Chinner at the time rejected the patch > since he didn't like xfstests being used to test a proprietary file > system for which I was not able to explain the details of what we were > trying to do. > > Not a big deal, I just kept the patch in my private fork[1] of > xfstests on github. > > [1] https://github.com/tytso/xfstests > > However, happily, we can now talk a lot more about what the "local" > file system type in xfstests was used to test. Earlier today, Google > announced gVisor[2], and published it as open source on github[3]. > gVisor works much like User Mode Linux, and I suspect much like the > Windows Subsystem for Linux in that it uses the x86_64's hardware > virtualization extensions (so it has the security fencing much like a > VM) but instead of emulating hardware, instead the emulation layer is > done at the system call level (so it's more efficient than a VM, since > there is no guest kernel). > > [2] https://cloudplatform.googleblog.com/2018/05/Open-sourcing-gVisor-a-sandboxed-container-runtime.html > [3] https://github.com/google/gvisor > > File systems can be implemented using Gophers[4] in a separate > process, where the communication between the gVisor sandbox and the > Gopher is via the 9P2000.L protocol. This means that if you want to > try to exploit a buffer overflow in the userspace file system, first > you have to get past the system call validation checks done by the > gVisor Sentry process (which is written in Go which makes this rather > more difficult, especially since the Sentry process itself is > protected using seccomp[5]). Then the buffer overflow attack has to > make it past the 9P protocol encoding/decoding, and then survive the > 9P protocol validation checks in the Gopher. The Gopher process > provides an emulated file system service using the Cloud Provider's > internal cluster storage services, much like in GCE, the Persistent > Disk service provides an emulated block device service. > > [4] https://news.ycombinator.com/item?id=16979126 > [5] https://news.ycombinator.com/item?id=16976557 > > This whole system was designed with security first and foremost. Of > course, we also want it to pass the file system checks, which is why > were using xfstests. > > The way we actually tested the gVisor file system was via a Docker > container (the Dockerfile[6] is in the xfstests-bld git repo) which > would then get consumed by gVisor's Docker/Kubernetes integration > layer. > > [6] https://github.com/tytso/xfstests-bld/blob/master/Dockerfile > > Anyway, back in September, 2016, Dave Chinner was peeved that I > couldn't give this full description, and I'm glad to say, now we can > finally rectify that gap. :-) > > Could this commit therefore please be considered for inclusion in > xfstests upstream? I still think it's useful, and thanks for resending with such detailed information! And I'm fine with the 'local' file system type. But I may need some time to do careful testing and go into the details. Just some random comments inline. > > Many thanks! > > - Ted > > > From 8b40b28866dca119d0c807c31ae48f153ec2dc53 Mon Sep 17 00:00:00 2001 > From: Theodore Ts'o <tytso@mit.edu> > Date: Sat, 17 Sep 2016 19:08:18 -0400 > Subject: [PATCH] common: add support for the "local" file system type > > It is sometimes useful to be able to test the local file system > provided in a restricted execution environment (such as that which is > provided by Docker, for example) where it is not possible to mount and > unmount the file system under test. > > To support this test case, add support for a new file system type > called "local". The TEST_DEV and SCRATCH_DEV should be have a > non-block device format (e.g., local:/test or local:/scratch), and the Does it require a non-block device format in this version of the patch? I don't think so, as we have the new 'local' FSTYP introduced now. > TEST_DIR and SCRATCH_MNT directories should be pre-existing > directories provided by the execution environment. > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- > common/rc | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- We really need some documentation added in README :) > 1 file changed, 70 insertions(+), 3 deletions(-) > > diff --git a/common/rc b/common/rc > index 9ffab7fd..d5cb0fe4 100644 > --- a/common/rc > +++ b/common/rc > @@ -351,6 +351,18 @@ _supports_filetype() > esac > } > > +_local_validate_mount_opt() > +{ > + case "$*" in > + ro|ro,*|*,ro) _notrun "ro mount option not supported" ;; > + *nosuid*) _notrun "nosuid mount option not supported" ;; > + *noatime*) _notrun "noatime mount option not supported" ;; > + *relatime*) _notrun "relatime mount option not supported" ;; > + *diratime*) _notrun "diratime mount option not supported" ;; > + *strictatime*) _notrun "strictatime mount option not supported" ;; > + esac > +} Would be good to have some comments on why these mount options are not supported for 'local'. > + > # mount scratch device with given options but don't check mount status > _try_scratch_mount() > { > @@ -376,6 +388,9 @@ _scratch_unmount() > btrfs) > $UMOUNT_PROG $SCRATCH_MNT > ;; > + local) > + rm -rf $SCRATCH_MNT/* > + ;; We do this in _scratch_mkfs by calling _scratch_cleanup_files. I noticed that you already did this in _scratch_mkfs, just return here for 'local'? > *) > $UMOUNT_PROG $SCRATCH_DEV > ;; > @@ -386,6 +401,10 @@ _scratch_remount() > { > local opts="$1" > > + if [ "$FSTYP" = "local" ]; then > + _local_validate_mount_opt "$*" > + return 0; > + fi > if test -n "$opts"; then > mount -o "remount,$opts" $SCRATCH_MNT > fi > @@ -395,7 +414,7 @@ _scratch_cycle_mount() > { > local opts="$1" > > - if [ "$FSTYP" = tmpfs ]; then > + if [ "$FSTYP" = "tmpfs" -o "$FSTYP" = "local" ] ; then > _scratch_remount "$opts" > return > fi > @@ -429,6 +448,10 @@ _test_mount() > _overlay_test_mount $* > return $? > fi > + if [ "$FSTYP" == "local" ]; then > + mkdir -p $TEST_DIR > + return $? > + fi $TEST_DIR is guaranteed to be there (in common/config at initialization time and in _require_test ). Perhaps we can just return in the 'local' case? > _test_options mount > _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR > } > @@ -437,7 +460,7 @@ _test_unmount() > { > if [ "$FSTYP" == "overlay" ]; then > _overlay_test_unmount > - else > + elif [ "$FSTYP" != "local" ]; then > $UMOUNT_PROG $TEST_DEV > fi > } > @@ -723,7 +746,7 @@ _scratch_mkfs() > local mkfs_status > > case $FSTYP in > - nfs*|cifs|ceph|overlay|glusterfs|pvfs2|9p) > + nfs*|cifs|ceph|overlay|glusterfs|pvfs2|9p|local) > # unable to re-create this fstyp, just remove all files in > # $SCRATCH_MNT to avoid EEXIST caused by the leftover files > # created in previous runs > @@ -1465,6 +1488,10 @@ _check_mounted_on() > local mnt=$4 > local type=$5 > > + if [ "$FSTYP" == "local" ]; then > + return 0 > + fi > + > # find $dev as the source, and print result in "$dev $mnt" format > local mount_rec=`findmnt -rncv -S $dev -o SOURCE,TARGET` > [ -n "$mount_rec" ] || return 1 # 1 = not mounted > @@ -1562,6 +1589,13 @@ _require_scratch_nocheck() > _notrun "this test requires a valid \$SCRATCH_MNT" > fi > ;; > + local) > + if [ -z "$SCRATCH_DEV" -o ! -d "$SCRATCH_MNT" ]; > + then > + _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV" > + fi > + return 0 > + ;; > *) > if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ] > then > @@ -1683,6 +1717,13 @@ _require_test() > _notrun "this test requires a valid \$TEST_DIR" > fi > ;; > + local) > + if [ -z "$TEST_DEV" -o ! -d "$TEST_DIR" ]; > + then > + _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV" > + fi > + return 0 > + ;; It seems $SCRATCH_DEV and $TEST_DEV are not important for 'local' type, as long as we have SCRATCH_MNT and/or TEST_DIR defined as directories. Thanks, Eryu > *) > if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ] > then > @@ -2438,6 +2479,10 @@ _remount() > local device=$1 > local mode=$2 > > + if [ "$FSTYP" == "local" ] ; then > + return 0 > + fi > + > if ! mount -o remount,$mode $device > then > echo "_remount: failed to remount filesystem on $device as $mode" > @@ -2483,6 +2528,10 @@ _mount_or_remount_rw() > local device=$2 > local mountpoint=$3 > > + if [ "$FSTYP" == "local" ] ; then > + return 0 > + fi > + > if [ $USE_REMOUNT -eq 0 ]; then > if [ "$FSTYP" != "overlay" ]; then > _mount -t $FSTYP $mount_opts $device $mountpoint > @@ -2636,6 +2685,9 @@ _check_test_fs() > ubifs) > # there is no fsck program for ubifs yet > ;; > + local) > + # no way to check consistency for local > + ;; > *) > _check_generic_filesystem $TEST_DEV > ;; > @@ -2691,6 +2743,9 @@ _check_scratch_fs() > ubifs) > # there is no fsck program for ubifs yet > ;; > + local) > + # no way to check consistency for local > + ;; > *) > _check_generic_filesystem $device > ;; > @@ -3003,6 +3058,9 @@ _require_fio() > # Does freeze work on this fs? > _require_freeze() > { > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support freeze" > + fi > xfs_freeze -f "$TEST_DIR" >/dev/null 2>&1 > local result=$? > xfs_freeze -u "$TEST_DIR" >/dev/null 2>&1 > @@ -3024,6 +3082,9 @@ _require_scratch_shutdown() > { > [ -x src/godown ] || _notrun "src/godown executable not found" > > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support shutdown" > + fi > _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV" > _scratch_mount > > @@ -3049,6 +3110,9 @@ _require_scratch_shutdown() > # Does dax mount option work on this dev/fs? > _require_scratch_dax() > { > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support dax" > + fi > _require_scratch > _scratch_mkfs > /dev/null 2>&1 > _try_scratch_mount -o dax || \ > @@ -3063,6 +3127,9 @@ _require_scratch_dax() > # Does norecovery support by this fs? > _require_norecovery() > { > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support norecovery" > + fi > _try_scratch_mount -o ro,norecovery || \ > _notrun "$FSTYP does not support norecovery" > _scratch_unmount > -- > 2.16.1.72.g5be1f00a9a > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ted, On Wed, May 02, 2018 at 11:43:40PM -0400, Theodore Y. Ts'o wrote: [snip the background details] > From 8b40b28866dca119d0c807c31ae48f153ec2dc53 Mon Sep 17 00:00:00 2001 > From: Theodore Ts'o <tytso@mit.edu> > Date: Sat, 17 Sep 2016 19:08:18 -0400 > Subject: [PATCH] common: add support for the "local" file system type > > It is sometimes useful to be able to test the local file system > provided in a restricted execution environment (such as that which is > provided by Docker, for example) where it is not possible to mount and > unmount the file system under test. > > To support this test case, add support for a new file system type > called "local". The TEST_DEV and SCRATCH_DEV should be have a > non-block device format (e.g., local:/test or local:/scratch), and the > TEST_DIR and SCRATCH_MNT directories should be pre-existing > directories provided by the execution environment. > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> I tested the patch with XFS and ext4 as the mounted underlying filesystem for multiple rounds and found some more issues (in addition to previous review comments) that need to be addressed. I'll reply inline. > --- > common/rc | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 70 insertions(+), 3 deletions(-) I think we need a new "-local" option in './check' to do some initial setups, e.g. FSTYP=local, as other fs types that can't be retrieved by running blkid on $TEST_DEV. Also need some documentations in README to describe "local"'s use case and mention that TEST_DEV and SCRATCH_DEV should be have a non-block device format. > > diff --git a/common/rc b/common/rc > index 9ffab7fd..d5cb0fe4 100644 > --- a/common/rc > +++ b/common/rc > @@ -351,6 +351,18 @@ _supports_filetype() > esac > } > > +_local_validate_mount_opt() > +{ > + case "$*" in > + ro|ro,*|*,ro) _notrun "ro mount option not supported" ;; > + *nosuid*) _notrun "nosuid mount option not supported" ;; > + *noatime*) _notrun "noatime mount option not supported" ;; > + *relatime*) _notrun "relatime mount option not supported" ;; > + *diratime*) _notrun "diratime mount option not supported" ;; > + *strictatime*) _notrun "strictatime mount option not supported" ;; > + esac > +} > + > # mount scratch device with given options but don't check mount status > _try_scratch_mount() > { We need to add a check for "local" in _try_scratch_mount() to let it do nothing for "local" too, otherwise 'FSTYP=local ./check' won't run for me, as _scratch_mount() failed and exit the test in 'check' line 629. (Note that the exit on _scratch_mount() failure behavior was introduced by commit 69eb6281a9d3 ("fstests: _fail test by default when _scratch_mount fails")) > @@ -376,6 +388,9 @@ _scratch_unmount() > btrfs) > $UMOUNT_PROG $SCRATCH_MNT > ;; > + local) > + rm -rf $SCRATCH_MNT/* > + ;; As noted in previous review, we could just return for 'local' in _scratch_unmount(), _scratch_mkfs() already removed all the files there. > *) > $UMOUNT_PROG $SCRATCH_DEV > ;; > @@ -386,6 +401,10 @@ _scratch_remount() > { > local opts="$1" > > + if [ "$FSTYP" = "local" ]; then > + _local_validate_mount_opt "$*" > + return 0; > + fi I found that there're more places that need to do _local_validate_mount_opt, e.g. in _try_scratch_mount() and _test_mount, otherwise tests that use _try_scratch_mount to do the remount would fail, like generic/294. > if test -n "$opts"; then > mount -o "remount,$opts" $SCRATCH_MNT > fi > @@ -395,7 +414,7 @@ _scratch_cycle_mount() > { > local opts="$1" > > - if [ "$FSTYP" = tmpfs ]; then > + if [ "$FSTYP" = "tmpfs" -o "$FSTYP" = "local" ] ; then > _scratch_remount "$opts" > return > fi > @@ -429,6 +448,10 @@ _test_mount() > _overlay_test_mount $* > return $? > fi > + if [ "$FSTYP" == "local" ]; then > + mkdir -p $TEST_DIR Probably need _local_validate_mount_opt here as mentioned above. And there're some tests or functions that call umount on $SCRATCH_MNT directly, which would cause the underlying filesystem to be umounted if it can be umounted (thouth this is not an intended use case for "local", I think it's better to fix them too). e.g. generic/034 generic/330 and generic/332 need to use _scratch_unmount instead of a bare umount. All the "$UMOUNT_PROG $SCRATCH_MNT"s in dm device helper functions, we need to change them to umount the actual device, e.g. $FLAKEY_DEV generic/108 needs to umount $SCSI_DEBUG_DEV in _cleanup() Similarly, generic/459 needs to umount the lvm snapshot device /dev/mapper/$vgname-$snapname Thanks, Eryu > + return $? > + fi > _test_options mount > _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR > } > @@ -437,7 +460,7 @@ _test_unmount() > { > if [ "$FSTYP" == "overlay" ]; then > _overlay_test_unmount > - else > + elif [ "$FSTYP" != "local" ]; then > $UMOUNT_PROG $TEST_DEV > fi > } > @@ -723,7 +746,7 @@ _scratch_mkfs() > local mkfs_status > > case $FSTYP in > - nfs*|cifs|ceph|overlay|glusterfs|pvfs2|9p) > + nfs*|cifs|ceph|overlay|glusterfs|pvfs2|9p|local) > # unable to re-create this fstyp, just remove all files in > # $SCRATCH_MNT to avoid EEXIST caused by the leftover files > # created in previous runs > @@ -1465,6 +1488,10 @@ _check_mounted_on() > local mnt=$4 > local type=$5 > > + if [ "$FSTYP" == "local" ]; then > + return 0 > + fi > + > # find $dev as the source, and print result in "$dev $mnt" format > local mount_rec=`findmnt -rncv -S $dev -o SOURCE,TARGET` > [ -n "$mount_rec" ] || return 1 # 1 = not mounted > @@ -1562,6 +1589,13 @@ _require_scratch_nocheck() > _notrun "this test requires a valid \$SCRATCH_MNT" > fi > ;; > + local) > + if [ -z "$SCRATCH_DEV" -o ! -d "$SCRATCH_MNT" ]; > + then > + _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV" > + fi > + return 0 > + ;; > *) > if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ] > then > @@ -1683,6 +1717,13 @@ _require_test() > _notrun "this test requires a valid \$TEST_DIR" > fi > ;; > + local) > + if [ -z "$TEST_DEV" -o ! -d "$TEST_DIR" ]; > + then > + _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV" > + fi > + return 0 > + ;; > *) > if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ] > then > @@ -2438,6 +2479,10 @@ _remount() > local device=$1 > local mode=$2 > > + if [ "$FSTYP" == "local" ] ; then > + return 0 > + fi > + > if ! mount -o remount,$mode $device > then > echo "_remount: failed to remount filesystem on $device as $mode" > @@ -2483,6 +2528,10 @@ _mount_or_remount_rw() > local device=$2 > local mountpoint=$3 > > + if [ "$FSTYP" == "local" ] ; then > + return 0 > + fi > + > if [ $USE_REMOUNT -eq 0 ]; then > if [ "$FSTYP" != "overlay" ]; then > _mount -t $FSTYP $mount_opts $device $mountpoint > @@ -2636,6 +2685,9 @@ _check_test_fs() > ubifs) > # there is no fsck program for ubifs yet > ;; > + local) > + # no way to check consistency for local > + ;; > *) > _check_generic_filesystem $TEST_DEV > ;; > @@ -2691,6 +2743,9 @@ _check_scratch_fs() > ubifs) > # there is no fsck program for ubifs yet > ;; > + local) > + # no way to check consistency for local > + ;; > *) > _check_generic_filesystem $device > ;; > @@ -3003,6 +3058,9 @@ _require_fio() > # Does freeze work on this fs? > _require_freeze() > { > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support freeze" > + fi > xfs_freeze -f "$TEST_DIR" >/dev/null 2>&1 > local result=$? > xfs_freeze -u "$TEST_DIR" >/dev/null 2>&1 > @@ -3024,6 +3082,9 @@ _require_scratch_shutdown() > { > [ -x src/godown ] || _notrun "src/godown executable not found" > > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support shutdown" > + fi > _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV" > _scratch_mount > > @@ -3049,6 +3110,9 @@ _require_scratch_shutdown() > # Does dax mount option work on this dev/fs? > _require_scratch_dax() > { > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support dax" > + fi > _require_scratch > _scratch_mkfs > /dev/null 2>&1 > _try_scratch_mount -o dax || \ > @@ -3063,6 +3127,9 @@ _require_scratch_dax() > # Does norecovery support by this fs? > _require_norecovery() > { > + if [ "$FSTYP" == "local" ]; then > + _notrun "local does not support norecovery" > + fi > _try_scratch_mount -o ro,norecovery || \ > _notrun "$FSTYP does not support norecovery" > _scratch_unmount > -- > 2.16.1.72.g5be1f00a9a > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/common/rc b/common/rc index 9ffab7fd..d5cb0fe4 100644 --- a/common/rc +++ b/common/rc @@ -351,6 +351,18 @@ _supports_filetype() esac } +_local_validate_mount_opt() +{ + case "$*" in + ro|ro,*|*,ro) _notrun "ro mount option not supported" ;; + *nosuid*) _notrun "nosuid mount option not supported" ;; + *noatime*) _notrun "noatime mount option not supported" ;; + *relatime*) _notrun "relatime mount option not supported" ;; + *diratime*) _notrun "diratime mount option not supported" ;; + *strictatime*) _notrun "strictatime mount option not supported" ;; + esac +} + # mount scratch device with given options but don't check mount status _try_scratch_mount() { @@ -376,6 +388,9 @@ _scratch_unmount() btrfs) $UMOUNT_PROG $SCRATCH_MNT ;; + local) + rm -rf $SCRATCH_MNT/* + ;; *) $UMOUNT_PROG $SCRATCH_DEV ;; @@ -386,6 +401,10 @@ _scratch_remount() { local opts="$1" + if [ "$FSTYP" = "local" ]; then + _local_validate_mount_opt "$*" + return 0; + fi if test -n "$opts"; then mount -o "remount,$opts" $SCRATCH_MNT fi @@ -395,7 +414,7 @@ _scratch_cycle_mount() { local opts="$1" - if [ "$FSTYP" = tmpfs ]; then + if [ "$FSTYP" = "tmpfs" -o "$FSTYP" = "local" ] ; then _scratch_remount "$opts" return fi @@ -429,6 +448,10 @@ _test_mount() _overlay_test_mount $* return $? fi + if [ "$FSTYP" == "local" ]; then + mkdir -p $TEST_DIR + return $? + fi _test_options mount _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR } @@ -437,7 +460,7 @@ _test_unmount() { if [ "$FSTYP" == "overlay" ]; then _overlay_test_unmount - else + elif [ "$FSTYP" != "local" ]; then $UMOUNT_PROG $TEST_DEV fi } @@ -723,7 +746,7 @@ _scratch_mkfs() local mkfs_status case $FSTYP in - nfs*|cifs|ceph|overlay|glusterfs|pvfs2|9p) + nfs*|cifs|ceph|overlay|glusterfs|pvfs2|9p|local) # unable to re-create this fstyp, just remove all files in # $SCRATCH_MNT to avoid EEXIST caused by the leftover files # created in previous runs @@ -1465,6 +1488,10 @@ _check_mounted_on() local mnt=$4 local type=$5 + if [ "$FSTYP" == "local" ]; then + return 0 + fi + # find $dev as the source, and print result in "$dev $mnt" format local mount_rec=`findmnt -rncv -S $dev -o SOURCE,TARGET` [ -n "$mount_rec" ] || return 1 # 1 = not mounted @@ -1562,6 +1589,13 @@ _require_scratch_nocheck() _notrun "this test requires a valid \$SCRATCH_MNT" fi ;; + local) + if [ -z "$SCRATCH_DEV" -o ! -d "$SCRATCH_MNT" ]; + then + _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV" + fi + return 0 + ;; *) if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ] then @@ -1683,6 +1717,13 @@ _require_test() _notrun "this test requires a valid \$TEST_DIR" fi ;; + local) + if [ -z "$TEST_DEV" -o ! -d "$TEST_DIR" ]; + then + _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV" + fi + return 0 + ;; *) if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ] then @@ -2438,6 +2479,10 @@ _remount() local device=$1 local mode=$2 + if [ "$FSTYP" == "local" ] ; then + return 0 + fi + if ! mount -o remount,$mode $device then echo "_remount: failed to remount filesystem on $device as $mode" @@ -2483,6 +2528,10 @@ _mount_or_remount_rw() local device=$2 local mountpoint=$3 + if [ "$FSTYP" == "local" ] ; then + return 0 + fi + if [ $USE_REMOUNT -eq 0 ]; then if [ "$FSTYP" != "overlay" ]; then _mount -t $FSTYP $mount_opts $device $mountpoint @@ -2636,6 +2685,9 @@ _check_test_fs() ubifs) # there is no fsck program for ubifs yet ;; + local) + # no way to check consistency for local + ;; *) _check_generic_filesystem $TEST_DEV ;; @@ -2691,6 +2743,9 @@ _check_scratch_fs() ubifs) # there is no fsck program for ubifs yet ;; + local) + # no way to check consistency for local + ;; *) _check_generic_filesystem $device ;; @@ -3003,6 +3058,9 @@ _require_fio() # Does freeze work on this fs? _require_freeze() { + if [ "$FSTYP" == "local" ]; then + _notrun "local does not support freeze" + fi xfs_freeze -f "$TEST_DIR" >/dev/null 2>&1 local result=$? xfs_freeze -u "$TEST_DIR" >/dev/null 2>&1 @@ -3024,6 +3082,9 @@ _require_scratch_shutdown() { [ -x src/godown ] || _notrun "src/godown executable not found" + if [ "$FSTYP" == "local" ]; then + _notrun "local does not support shutdown" + fi _scratch_mkfs > /dev/null 2>&1 || _notrun "_scratch_mkfs failed on $SCRATCH_DEV" _scratch_mount @@ -3049,6 +3110,9 @@ _require_scratch_shutdown() # Does dax mount option work on this dev/fs? _require_scratch_dax() { + if [ "$FSTYP" == "local" ]; then + _notrun "local does not support dax" + fi _require_scratch _scratch_mkfs > /dev/null 2>&1 _try_scratch_mount -o dax || \ @@ -3063,6 +3127,9 @@ _require_scratch_dax() # Does norecovery support by this fs? _require_norecovery() { + if [ "$FSTYP" == "local" ]; then + _notrun "local does not support norecovery" + fi _try_scratch_mount -o ro,norecovery || \ _notrun "$FSTYP does not support norecovery" _scratch_unmount