On Thu, May 3, 2018 at 9:35 PM, Theodore Y. Ts'o <tytso@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
