| Message ID | 20170517095531.11818-2-david.oberhollenzer@sigma-star.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote: > This patch is mostly based on the previous attempts of Eric Biggers > and Dongsheng Yang at adding UBIFS support to xfstests. > > In addition to rebasing the previous attempts to recent xfstests, > most of the encryption tests now also support UBIFS with this patch. > > Since mkfs.ubifs doesn't support creating encryption-capable filesystems > yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI > volume. The filesystem is then created when mounting the emtpy volume. > > Some tests that require SCRATCH_DEV to be a block device, despite > not using device-mapper or otherwise doing something block device > specific have that requirement removed. > > Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at> Thanks for the patch! I know only very little about ubifs (from previous reviews to attempts to add ubifs support to fstests), but it's still good to have some words to introduce ubifs briefly in commit log, especially the fact that it uses char device not block device. > --- > check | 2 ++ > common/config | 7 +++++++ > common/encrypt | 3 +++ > common/rc | 24 ++++++++++++++++++++++++ > tests/generic/076 | 1 - > tests/generic/409 | 1 - > tests/generic/410 | 1 - > tests/generic/411 | 1 - > 8 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/check b/check > index 9cef58b4..f8db3cd6 100755 > --- a/check > +++ b/check > @@ -70,6 +70,7 @@ check options > -overlay test overlay > -pvfs2 test PVFS2 > -tmpfs test TMPFS > + -ubifs test ubifs > -l line mode diff > -udiff show unified diff (default) > -n show me, do not run tests > @@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do > -overlay) FSTYP=overlay; export OVERLAY=true ;; > -pvfs2) FSTYP=pvfs2 ;; > -tmpfs) FSTYP=tmpfs ;; > + -ubifs) FSTYP=ubifs ;; As being pointed out in previous reviews, it'll be great if we can probe ubifs from the char device if possible instead of adding new fs-specific option, just as what we're doing at the end of common/config for other local filesystems. But I'm not sure if blkid works for char device and ubifs (probably not). > > -g) group=$2 ; shift ; > GROUP_LIST="$GROUP_LIST ${group//,/ }" > diff --git a/common/config b/common/config > index 59041a39..6c58e888 100644 > --- a/common/config > +++ b/common/config > @@ -336,6 +336,9 @@ _mount_opts() > # We need to specify the size at mount, use 1G by default > export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS" > ;; > + ubifs) > + export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS > + ;; > *) > ;; > esac > @@ -475,6 +478,10 @@ _check_device() > if [ ! -d "$dev" ]; then > _fatal "common/config: $name ($dev) is not a directory for overlay" > fi > + elif [ "$FSTYP" == "ubifs" ]; then > + if [ ! -c "$dev" ]; then > + _fatal "common/config: $name ($dev) is not a character device" > + fi > else > _fatal "common/config: $name ($dev) is not a block device or a network filesystem" This error message should be updated too. And turning this if-elif-fi block to a case switch on $FSTYP seems cleaner. And you need to setup MKFS_UBIFS_PROG and all other needed tools in common/config too, and check if the tools are available in common/rc and abort if the required tools are not met. e.g. [ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found" > fi > diff --git a/common/encrypt b/common/encrypt > index 723f1b11..b444c82d 100644 > --- a/common/encrypt > +++ b/common/encrypt > @@ -71,6 +71,9 @@ _scratch_mkfs_encrypted() > ext4|f2fs) > _scratch_mkfs -O encrypt > ;; > + ubifs) > + ubiupdatevol ${SCRATCH_DEV} -t > + ;; Need a commont on this. > *) > _notrun "No encryption support for $FSTYP" > ;; > diff --git a/common/rc b/common/rc > index 257b1903..6191a0c0 100644 > --- a/common/rc > +++ b/common/rc > @@ -1556,6 +1556,15 @@ _require_scratch_nocheck() > _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV" > fi > ;; > + ubifs) > + # ubifs needs an UBI volume. This will be a char device, not a block device. > + if [ ! -c "$SCRATCH_DEV" ]; then > + _notrun "this test requires a valid UBI volume for \$SCRATCH_DEV" > + fi > + if [ ! -d "$SCRATCH_MNT" ]; then > + _notrun "this test requires a valid \$SCRATCH_MNT" > + fi > + ;; > *) > if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ] > then > @@ -1650,6 +1659,15 @@ _require_test() > _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV" > fi > ;; > + ubifs) > + # ubifs needs an UBI volume. This will be a char device, not a block device. > + if [ ! -c "$TEST_DEV" ]; then > + _notrun "this test requires a valid UBI volume for \$TEST_DEV" > + fi > + if [ ! -d "$TEST_DIR" ]; then > + _notrun "this test requires a valid \$TEST_DIR" > + fi > + ;; > *) > if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ] > then > @@ -2490,6 +2508,9 @@ _check_test_fs() > tmpfs) > # no way to check consistency for tmpfs > ;; > + ubifs) > + # there is no fsck program for ubifs yet > + ;; > *) > _check_generic_filesystem $TEST_DEV > ;; > @@ -2539,6 +2560,9 @@ _check_scratch_fs() > tmpfs) > # no way to check consistency for tmpfs > ;; > + ubifs) > + # there is no fsck program for ubifs yet > + ;; > *) > _check_generic_filesystem $device > ;; > diff --git a/tests/generic/076 b/tests/generic/076 > index 64e69583..53e56a69 100755 > --- a/tests/generic/076 > +++ b/tests/generic/076 > @@ -56,7 +56,6 @@ _supported_fs generic > _supported_os IRIX Linux > > _require_scratch > -_require_block_device $SCRATCH_DEV This test does "cat $SCRATCH_DEV >/dev/null &" which assumes $SCRATCH_DEV is a local device, and test fails if it's a network device, or an overlayfs pseudo mount device. And similarly generic/409 to generic/411 fail if SCRATCH_DEV is a overlay pseudo mount device. So I think what's needed here is a new helper that requires a local device to make sure the given device resides on local host and is either block or char device, e.g. something like _require_local_device $SCRATCH_DEV Thanks, Eryu > > echo "*** init fs" > > diff --git a/tests/generic/409 b/tests/generic/409 > index 4bfedf73..3be58ba5 100755 > --- a/tests/generic/409 > +++ b/tests/generic/409 > @@ -64,7 +64,6 @@ _supported_fs generic > _supported_os Linux > _require_test > _require_scratch > -_require_block_device $SCRATCH_DEV > > fs_stress() > { > diff --git a/tests/generic/410 b/tests/generic/410 > index f2e0d1bb..e3a933ce 100755 > --- a/tests/generic/410 > +++ b/tests/generic/410 > @@ -72,7 +72,6 @@ _supported_fs generic > _supported_os Linux > _require_test > _require_scratch > -_require_block_device $SCRATCH_DEV > > fs_stress() > { > diff --git a/tests/generic/411 b/tests/generic/411 > index 7b2dd33b..08b861c5 100755 > --- a/tests/generic/411 > +++ b/tests/generic/411 > @@ -53,7 +53,6 @@ _supported_fs generic > _supported_os Linux > _require_test > _require_scratch > -_require_block_device $SCRATCH_DEV > > fs_stress() > { > -- > 2.12.0 > > -- > 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
On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote: > On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote: > > This patch is mostly based on the previous attempts of Eric Biggers > > and Dongsheng Yang at adding UBIFS support to xfstests. > > > > In addition to rebasing the previous attempts to recent xfstests, > > most of the encryption tests now also support UBIFS with this patch. > > > > Since mkfs.ubifs doesn't support creating encryption-capable filesystems > > yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI > > volume. The filesystem is then created when mounting the emtpy volume. > > > > Some tests that require SCRATCH_DEV to be a block device, despite > > not using device-mapper or otherwise doing something block device > > specific have that requirement removed. > > > > Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at> > > Thanks for the patch! I know only very little about ubifs (from previous > reviews to attempts to add ubifs support to fstests), but it's still > good to have some words to introduce ubifs briefly in commit log, > especially the fact that it uses char device not block device. > > > --- > > check | 2 ++ > > common/config | 7 +++++++ > > common/encrypt | 3 +++ > > common/rc | 24 ++++++++++++++++++++++++ > > tests/generic/076 | 1 - > > tests/generic/409 | 1 - > > tests/generic/410 | 1 - > > tests/generic/411 | 1 - > > 8 files changed, 36 insertions(+), 4 deletions(-) > > > > diff --git a/check b/check > > index 9cef58b4..f8db3cd6 100755 > > --- a/check > > +++ b/check > > @@ -70,6 +70,7 @@ check options > > -overlay test overlay > > -pvfs2 test PVFS2 > > -tmpfs test TMPFS > > + -ubifs test ubifs > > -l line mode diff > > -udiff show unified diff (default) > > -n show me, do not run tests > > @@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do > > -overlay) FSTYP=overlay; export OVERLAY=true ;; > > -pvfs2) FSTYP=pvfs2 ;; > > -tmpfs) FSTYP=tmpfs ;; > > + -ubifs) FSTYP=ubifs ;; > > As being pointed out in previous reviews, it'll be great if we can probe > ubifs from the char device if possible instead of adding new fs-specific > option, just as what we're doing at the end of common/config for other > local filesystems. But I'm not sure if blkid works for char device and > ubifs (probably not). > It seems to work fine without the -ubifs option: # blkid -o value -s TYPE /dev/ubi0_0 ubifs # TEST_DEV=/dev/ubi0_0 TEST_DIR=/vdb ./check generic/001 FSTYP -- ubifs PLATFORM -- Linux/x86_64 kvm-xfstests 4.12.0-rc1-xfstests-00083-ga844e08648f0-dirty generic/001 [ 372.213194] run fstests generic/001 at 2017-05-17 11:43:47 8s Ran: generic/001 Passed all 1 tests - Eric -- 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 05/17/2017 01:53 PM, Eryu Guan wrote: > On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote: >> -g) group=$2 ; shift ; >> GROUP_LIST="$GROUP_LIST ${group//,/ }" >> diff --git a/common/config b/common/config >> index 59041a39..6c58e888 100644 >> --- a/common/config >> +++ b/common/config >> @@ -336,6 +336,9 @@ _mount_opts() >> # We need to specify the size at mount, use 1G by default >> export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS" >> ;; >> + ubifs) >> + export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS >> + ;; >> *) >> ;; >> esac >> @@ -475,6 +478,10 @@ _check_device() >> if [ ! -d "$dev" ]; then >> _fatal "common/config: $name ($dev) is not a directory for overlay" >> fi >> + elif [ "$FSTYP" == "ubifs" ]; then >> + if [ ! -c "$dev" ]; then >> + _fatal "common/config: $name ($dev) is not a character device" >> + fi >> else >> _fatal "common/config: $name ($dev) is not a block device or a network filesystem" > > This error message should be updated too. And turning this if-elif-fi > block to a case switch on $FSTYP seems cleaner. > > And you need to setup MKFS_UBIFS_PROG and all other needed tools in > common/config too, and check if the tools are available in common/rc and > abort if the required tools are not met. e.g. > > [ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found" > mkfs.ubifs itself isn't needed as empty ubi volumes are formated when mounting them with UBIFS. I think it would make sense to add a check for ubiupdatevol to _require_scratch_encryption (used in _scratch_mkfs_encrypted to whipe an existing volume). On 05/17/2017 08:45 PM, Eric Biggers wrote: > On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote: >> As being pointed out in previous reviews, it'll be great if we can probe >> ubifs from the char device if possible instead of adding new fs-specific >> option, just as what we're doing at the end of common/config for other >> local filesystems. But I'm not sure if blkid works for char device and >> ubifs (probably not). >> > > It seems to work fine without the -ubifs option: > > # blkid -o value -s TYPE /dev/ubi0_0 > ubifs I can't really reproduce this on my end. Neither on my Debian test VM, nor on the OpenSUSE system that I'm working on right now. I get no output from blkid, neither before nor after mounting the ubi volume. To be fair, the Debian version (and thus its blkid version) is rather old (blkid 1.0.0 (12-Feb-2003)), but the one on OpenSUSE seems to be fairly recent: $ blkid -v blkid from util-linux 2.28 (libblkid 2.28., 12-Apr-2016) Would it make sense to patch the check in common/config instead, to default to ubifs if FSTYP is not set and the target is a character device? Or simply require on FSTYP to be set in the config file? Thanks, David -- 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 18, 2017 at 10:41:27AM +0200, David Oberhollenzer wrote: > On 05/17/2017 01:53 PM, Eryu Guan wrote: > > On Wed, May 17, 2017 at 11:55:29AM +0200, David Oberhollenzer wrote: > >> -g) group=$2 ; shift ; > >> GROUP_LIST="$GROUP_LIST ${group//,/ }" > >> diff --git a/common/config b/common/config > >> index 59041a39..6c58e888 100644 > >> --- a/common/config > >> +++ b/common/config > >> @@ -336,6 +336,9 @@ _mount_opts() > >> # We need to specify the size at mount, use 1G by default > >> export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS" > >> ;; > >> + ubifs) > >> + export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS > >> + ;; > >> *) > >> ;; > >> esac > >> @@ -475,6 +478,10 @@ _check_device() > >> if [ ! -d "$dev" ]; then > >> _fatal "common/config: $name ($dev) is not a directory for overlay" > >> fi > >> + elif [ "$FSTYP" == "ubifs" ]; then > >> + if [ ! -c "$dev" ]; then > >> + _fatal "common/config: $name ($dev) is not a character device" > >> + fi > >> else > >> _fatal "common/config: $name ($dev) is not a block device or a network filesystem" > > > > This error message should be updated too. And turning this if-elif-fi > > block to a case switch on $FSTYP seems cleaner. > > > > And you need to setup MKFS_UBIFS_PROG and all other needed tools in > > common/config too, and check if the tools are available in common/rc and > > abort if the required tools are not met. e.g. > > > > [ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found" > > > > mkfs.ubifs itself isn't needed as empty ubi volumes are formated when > mounting them with UBIFS. But there's still a mkfs.ubifs binary, right? I searched fstests mail archive and found that Dongsheng Yang did set MKFS_UBIFS_PROG in his patch in 2015. Then I suspect if mkfs.ubifs binary is unavailable, then _scratch_mkfs would complain mkfs.ubifs not found and fail the test. If it's not needed or there's no mkfs.ubifs exists, then _scratch_mkfs needs some updates I guess, and describe this in commit log too, because it's too different from other block device based local filesystems. > > I think it would make sense to add a check for ubiupdatevol to > _require_scratch_encryption (used in _scratch_mkfs_encrypted to > whipe an existing volume). Agreed. > > > On 05/17/2017 08:45 PM, Eric Biggers wrote: > > On Wed, May 17, 2017 at 07:53:55PM +0800, Eryu Guan wrote: > >> As being pointed out in previous reviews, it'll be great if we can probe > >> ubifs from the char device if possible instead of adding new fs-specific > >> option, just as what we're doing at the end of common/config for other > >> local filesystems. But I'm not sure if blkid works for char device and > >> ubifs (probably not). > >> > > > > It seems to work fine without the -ubifs option: > > > > # blkid -o value -s TYPE /dev/ubi0_0 > > ubifs > > I can't really reproduce this on my end. Neither on my Debian test VM, > nor on the OpenSUSE system that I'm working on right now. I get no > output from blkid, neither before nor after mounting the ubi volume. > > To be fair, the Debian version (and thus its blkid version) is rather > old (blkid 1.0.0 (12-Feb-2003)), but the one on OpenSUSE seems to be > fairly recent: > > $ blkid -v > blkid from util-linux 2.28 (libblkid 2.28., 12-Apr-2016) > > > Would it make sense to patch the check in common/config instead, > to default to ubifs if FSTYP is not set and the target is a > character device? Or simply require on FSTYP to be set in the > config file? If there's really no standard way to probe for ubifs, a "-ubifs" option would be the only choice I think, and better to have some comments too. Thanks, Eryu -- 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/check b/check index 9cef58b4..f8db3cd6 100755 --- a/check +++ b/check @@ -70,6 +70,7 @@ check options -overlay test overlay -pvfs2 test PVFS2 -tmpfs test TMPFS + -ubifs test ubifs -l line mode diff -udiff show unified diff (default) -n show me, do not run tests @@ -267,6 +268,7 @@ while [ $# -gt 0 ]; do -overlay) FSTYP=overlay; export OVERLAY=true ;; -pvfs2) FSTYP=pvfs2 ;; -tmpfs) FSTYP=tmpfs ;; + -ubifs) FSTYP=ubifs ;; -g) group=$2 ; shift ; GROUP_LIST="$GROUP_LIST ${group//,/ }" diff --git a/common/config b/common/config index 59041a39..6c58e888 100644 --- a/common/config +++ b/common/config @@ -336,6 +336,9 @@ _mount_opts() # We need to specify the size at mount, use 1G by default export MOUNT_OPTIONS="-o size=1G $TMPFS_MOUNT_OPTIONS" ;; + ubifs) + export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS + ;; *) ;; esac @@ -475,6 +478,10 @@ _check_device() if [ ! -d "$dev" ]; then _fatal "common/config: $name ($dev) is not a directory for overlay" fi + elif [ "$FSTYP" == "ubifs" ]; then + if [ ! -c "$dev" ]; then + _fatal "common/config: $name ($dev) is not a character device" + fi else _fatal "common/config: $name ($dev) is not a block device or a network filesystem" fi diff --git a/common/encrypt b/common/encrypt index 723f1b11..b444c82d 100644 --- a/common/encrypt +++ b/common/encrypt @@ -71,6 +71,9 @@ _scratch_mkfs_encrypted() ext4|f2fs) _scratch_mkfs -O encrypt ;; + ubifs) + ubiupdatevol ${SCRATCH_DEV} -t + ;; *) _notrun "No encryption support for $FSTYP" ;; diff --git a/common/rc b/common/rc index 257b1903..6191a0c0 100644 --- a/common/rc +++ b/common/rc @@ -1556,6 +1556,15 @@ _require_scratch_nocheck() _notrun "this test requires a valid \$SCRATCH_MNT and unique $SCRATCH_DEV" fi ;; + ubifs) + # ubifs needs an UBI volume. This will be a char device, not a block device. + if [ ! -c "$SCRATCH_DEV" ]; then + _notrun "this test requires a valid UBI volume for \$SCRATCH_DEV" + fi + if [ ! -d "$SCRATCH_MNT" ]; then + _notrun "this test requires a valid \$SCRATCH_MNT" + fi + ;; *) if [ -z "$SCRATCH_DEV" -o "`_is_block_dev "$SCRATCH_DEV"`" = "" ] then @@ -1650,6 +1659,15 @@ _require_test() _notrun "this test requires a valid \$TEST_DIR and unique $TEST_DEV" fi ;; + ubifs) + # ubifs needs an UBI volume. This will be a char device, not a block device. + if [ ! -c "$TEST_DEV" ]; then + _notrun "this test requires a valid UBI volume for \$TEST_DEV" + fi + if [ ! -d "$TEST_DIR" ]; then + _notrun "this test requires a valid \$TEST_DIR" + fi + ;; *) if [ -z "$TEST_DEV" ] || [ "`_is_block_dev "$TEST_DEV"`" = "" ] then @@ -2490,6 +2508,9 @@ _check_test_fs() tmpfs) # no way to check consistency for tmpfs ;; + ubifs) + # there is no fsck program for ubifs yet + ;; *) _check_generic_filesystem $TEST_DEV ;; @@ -2539,6 +2560,9 @@ _check_scratch_fs() tmpfs) # no way to check consistency for tmpfs ;; + ubifs) + # there is no fsck program for ubifs yet + ;; *) _check_generic_filesystem $device ;; diff --git a/tests/generic/076 b/tests/generic/076 index 64e69583..53e56a69 100755 --- a/tests/generic/076 +++ b/tests/generic/076 @@ -56,7 +56,6 @@ _supported_fs generic _supported_os IRIX Linux _require_scratch -_require_block_device $SCRATCH_DEV echo "*** init fs" diff --git a/tests/generic/409 b/tests/generic/409 index 4bfedf73..3be58ba5 100755 --- a/tests/generic/409 +++ b/tests/generic/409 @@ -64,7 +64,6 @@ _supported_fs generic _supported_os Linux _require_test _require_scratch -_require_block_device $SCRATCH_DEV fs_stress() { diff --git a/tests/generic/410 b/tests/generic/410 index f2e0d1bb..e3a933ce 100755 --- a/tests/generic/410 +++ b/tests/generic/410 @@ -72,7 +72,6 @@ _supported_fs generic _supported_os Linux _require_test _require_scratch -_require_block_device $SCRATCH_DEV fs_stress() { diff --git a/tests/generic/411 b/tests/generic/411 index 7b2dd33b..08b861c5 100755 --- a/tests/generic/411 +++ b/tests/generic/411 @@ -53,7 +53,6 @@ _supported_fs generic _supported_os Linux _require_test _require_scratch -_require_block_device $SCRATCH_DEV fs_stress() {
This patch is mostly based on the previous attempts of Eric Biggers and Dongsheng Yang at adding UBIFS support to xfstests. In addition to rebasing the previous attempts to recent xfstests, most of the encryption tests now also support UBIFS with this patch. Since mkfs.ubifs doesn't support creating encryption-capable filesystems yet, _scratch_mkfs_encrypted() is modified to wipe the underlying UBI volume. The filesystem is then created when mounting the emtpy volume. Some tests that require SCRATCH_DEV to be a block device, despite not using device-mapper or otherwise doing something block device specific have that requirement removed. Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at> --- check | 2 ++ common/config | 7 +++++++ common/encrypt | 3 +++ common/rc | 24 ++++++++++++++++++++++++ tests/generic/076 | 1 - tests/generic/409 | 1 - tests/generic/410 | 1 - tests/generic/411 | 1 - 8 files changed, 36 insertions(+), 4 deletions(-)