| Message ID | 20171211172742.3490-1-tytso@mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 11, 2017 at 12:27:42PM -0500, Theodore Ts'o wrote: > The test previously blindly tried block sizes from 512 to 65536 and > relied on mkfs to fail. This is problematic for a number of file > systems. For example, when testing btrfs, _scratch_mkfs_sized simply > ignores the blocksize, so the test will pointlessly run the same test > multiple times. For ext4, when encryption is enabled, the only block > size which is supported is the page size. > > So define two new functions _fs_min_blocksize and _fs_max_blocksize, > and use it so that generic/466 will only try using the block sizes > that will work. > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- > common/rc | 23 +++++++++++++++++++++++ > tests/generic/466 | 6 +++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/common/rc b/common/rc > index 59d4b961..7aa747ad 100644 > --- a/common/rc > +++ b/common/rc > @@ -1067,6 +1067,29 @@ _scratch_mkfs_blocksized() > esac > } > > +_fs_min_blocksize() > +{ > + case $FSTYP in > + ext4) > + if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then > + get_page_size > + else > + echo 1024 > + fi > + ;; > + btrfs) > + get_page_size > + ;; > + *) > + echo 512 FWIW XFS' minimum block size is 1k for v5 filesystems, though you can't really tell until you run mkfs.xfs -N. > + esac > +} > + > +_fs_max_blocksize() > +{ > + get_page_size Also, one can run xfstests against a fuse2fs-mounted 64k-block ext4 fs. --D > +} > + > _scratch_resvblks() > { > case $FSTYP in > diff --git a/tests/generic/466 b/tests/generic/466 > index 07f24a74..a1d944d3 100755 > --- a/tests/generic/466 > +++ b/tests/generic/466 > @@ -54,7 +54,11 @@ unset MKFS_OPTIONS > > echo "Starting test" > $seqres.full > devsize=$(blockdev --getsize64 $SCRATCH_DEV) > -for blocksize in 512 1024 2048 4096 8192 16384 32768 65536; do > +min_blocksize=$(_fs_min_blocksize) > +max_blocksize=$(_fs_max_blocksize) > + > +for (( blocksize = min_blocksize ; blocksize <= max_blocksize ; > + blocksize = blocksize * 2)); do > echo "+ Format blocksize $blocksize and mount" >> $seqres.full > _scratch_unmount > /dev/null 2>&1 > # Try to format and mount with the given blocksize. If they don't > -- > 2.11.0.rc0.7.gbe5a750 > > -- > 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, Dec 13, 2017 at 09:36:29AM -0800, Darrick J. Wong wrote: > > + get_page_size > > + ;; > > + *) > > + echo 512 > > FWIW XFS' minimum block size is 1k for v5 filesystems, though you can't > really tell until you run mkfs.xfs -N. Ok, but I assume we should keep this at 512 since it could be a v4 file systems that are being tested? > > +_fs_max_blocksize() > > +{ > > + get_page_size > > Also, one can run xfstests against a fuse2fs-mounted 64k-block ext4 fs. Really? Does mmap work on a fuse2fs-mounted 64k-block ext4 file system? I suppose can just force the block size to be 64k for fuse2fs, although I don't think the using some file system like ext4 for fuse2fs testing isn't going to work right now anyway, yes? - 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 Sun, Dec 31, 2017 at 01:45:08PM -0500, Theodore Ts'o wrote: > On Wed, Dec 13, 2017 at 09:36:29AM -0800, Darrick J. Wong wrote: > > > + get_page_size > > > + ;; > > > + *) > > > + echo 512 > > > > FWIW XFS' minimum block size is 1k for v5 filesystems, though you can't > > really tell until you run mkfs.xfs -N. > > Ok, but I assume we should keep this at 512 since it could be a v4 > file systems that are being tested? That depends on whether this function returns the smallest blocksize guaranteed to pass mkfs given the current set of options or the theoretical smallest blocksize supported by that fs given the right set of options. IOWs, if this is a v5 fs being tested, then "mkfs.xfs -b size=$(_fs_min_blocksize) -m crc=1" will fail. > > > +_fs_max_blocksize() > > > +{ > > > + get_page_size > > > > Also, one can run xfstests against a fuse2fs-mounted 64k-block ext4 fs. > > Really? Does mmap work on a fuse2fs-mounted 64k-block ext4 file system? Yes: # uname -a Linux magnolia 4.14.8-67-magnolia #2 SMP PREEMPT Fri Dec 22 17:23:52 PST 2017 x86_64 x86_64 x86_64 GNU/Linux # truncate -s 1g /tmp/a # mkfs.ext4 -b 65536 -F /tmp/a ... # fuse2fs /tmp/a /mnt /tmp/a: Writing to the journal is not supported. # fallocate -l 128k /mnt/a # xfs_io -c 'mmap -rw 0 128k' -c 'mwrite -S 0x58 0 6144' /mnt/a # od -tx1 -Ad -c /mnt/a 0000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 X X X X X X X X X X X X X X X X * 0006144 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 * 0131072 > I suppose can just force the block size to be 64k for fuse2fs, > although I don't think the using some file system like ext4 for > fuse2fs testing isn't going to work right now anyway, yes? I haven't tried it in some time, but the last time I ran xfstests against fuse2fs it more or less worked (modulo all the fancy fallocate stuff that it doesn't support). --D > > - 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 Tue, Jan 02, 2018 at 12:36:50PM -0800, Darrick J. Wong wrote: > On Sun, Dec 31, 2017 at 01:45:08PM -0500, Theodore Ts'o wrote: > > On Wed, Dec 13, 2017 at 09:36:29AM -0800, Darrick J. Wong wrote: > > > > + get_page_size > > > > + ;; > > > > + *) > > > > + echo 512 > > > > > > FWIW XFS' minimum block size is 1k for v5 filesystems, though you can't > > > really tell until you run mkfs.xfs -N. > > > > Ok, but I assume we should keep this at 512 since it could be a v4 > > file systems that are being tested? > > That depends on whether this function returns the smallest blocksize > guaranteed to pass mkfs given the current set of options or the > theoretical smallest blocksize supported by that fs given the right set > of options. IOWs, if this is a v5 fs being tested, then "mkfs.xfs -b > size=$(_fs_min_blocksize) -m crc=1" will fail. ...but we /could/ just try the known minimums with "_scratch_mkfs -b size=XXX -N" because (so long as it doesn't get smart and try to override our -b setting) it'll fail on any unsupported size. > > > > +_fs_max_blocksize() > > > > +{ > > > > + get_page_size > > > > > > Also, one can run xfstests against a fuse2fs-mounted 64k-block ext4 fs. > > > > Really? Does mmap work on a fuse2fs-mounted 64k-block ext4 file system? > > Yes: > > # uname -a > Linux magnolia 4.14.8-67-magnolia #2 SMP PREEMPT Fri Dec 22 17:23:52 PST 2017 x86_64 x86_64 x86_64 GNU/Linux > # truncate -s 1g /tmp/a > # mkfs.ext4 -b 65536 -F /tmp/a > ... > # fuse2fs /tmp/a /mnt > /tmp/a: Writing to the journal is not supported. > # fallocate -l 128k /mnt/a > # xfs_io -c 'mmap -rw 0 128k' -c 'mwrite -S 0x58 0 6144' /mnt/a > # od -tx1 -Ad -c /mnt/a > 0000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 > X X X X X X X X X X X X X X X X > * > 0006144 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 > * > 0131072 > > > I suppose can just force the block size to be 64k for fuse2fs, > > although I don't think the using some file system like ext4 for > > fuse2fs testing isn't going to work right now anyway, yes? > > I haven't tried it in some time, but the last time I ran xfstests > against fuse2fs it more or less worked (modulo all the fancy fallocate > stuff that it doesn't support). (Maybe we should separate out ext4/fuse2fs?) --D > --D > > > > > - 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 -- 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
> > That depends on whether this function returns the smallest blocksize > > guaranteed to pass mkfs given the current set of options or the > > theoretical smallest blocksize supported by that fs given the right set > > of options. IOWs, if this is a v5 fs being tested, then "mkfs.xfs -b > > size=$(_fs_min_blocksize) -m crc=1" will fail. Some of the discussion that we had on our weekly ext4 developer's conference call. One of the motivations for this patch is due to a local change that I have in my xfstests tree, which adds: || _die "${MKFS_PROG}.$FSTYP failed!" to the ${MKFS_PROG}.$FSTYP call in _scratch_mkfs_sized and similar functions. The reason for that is there a number of tests which use additional file system options, different block sizes, unusually small file system sizes, etc. --- and 99.97% of them don't bother to do any error checking. So what ends up happening is the test will not notice that the mkfs had failed, and would continue staggering on, sometimes succeeding, and sometimes failing --- depending on what test was running previously and the state of the scratch device. Ugh. Hence, my addition of the "|| die ..." The problem is that generic/466 is one of the few tests that actually (a) deliberately feed arguments that cause _scratch_mkfs_sized to fail, and (b) actually does error checking. Darrick mentioned on the call that he's seen similar problems which he's had to track down when testing xfs, and it was simiarly annoying. So instead of it being a local hack in my xfstests tree, perhaps this is something we should try to fix more generally. There a number of I could imagine going about this. 1) The approach I took in this patch, which is requires accurately determine min and max blocksizes --- and then to add error checks to all of the mkfs invocations in _scratch_mkfs_blocksized for all file systems. 2) Add an argument to _scratch_mkfs_blockized() and similar functions which indicates that error checking should _not_ be done, since it will be done by the caller (such as generic/466). And then add error checks in _scratch_mkfs_blocksized() and similar functions unless the optional flag is passed by the test. 3) Sweep through all of the tests scripts adding error checks. It seems to me (2) might be the simplest approach, although (1) does speed up the test somewhat, and so maybe we should do (1) as well as (2). Eryu, do you have any thoughts/opinions? > > I haven't tried it in some time, but the last time I ran xfstests > > against fuse2fs it more or less worked (modulo all the fancy fallocate > > stuff that it doesn't support). > > (Maybe we should separate out ext4/fuse2fs?) Darrick explained this require bind mounting mount.fuse2fs on mount.ext4, and other unnatural hacks. It seems to me that it might be useful to add first class support for fuse2fs to xfstests, with arguments ala overlayfs to specify what the fuse driver (such as fuse2fs) should be used; what should be used for mkfs, etc. Once we do this, we can use a different max blocksize for fuse2fs. - 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, Jan 04, 2018 at 10:28:43PM -0500, Theodore Ts'o wrote: > > > That depends on whether this function returns the smallest blocksize > > > guaranteed to pass mkfs given the current set of options or the > > > theoretical smallest blocksize supported by that fs given the right set > > > of options. IOWs, if this is a v5 fs being tested, then "mkfs.xfs -b > > > size=$(_fs_min_blocksize) -m crc=1" will fail. > > Some of the discussion that we had on our weekly ext4 developer's > conference call. One of the motivations for this patch is due to a > local change that I have in my xfstests tree, which adds: > > || _die "${MKFS_PROG}.$FSTYP failed!" > > to the ${MKFS_PROG}.$FSTYP call in _scratch_mkfs_sized and similar > functions. The reason for that is there a number of tests which use > additional file system options, different block sizes, unusually small > file system sizes, etc. --- and 99.97% of them don't bother to do any > error checking. My first reaction is to fix _scratch_mkfs_sized so it could retry with only user specified mkfs options, if the first mkfs failed due to conflicts between MKFS_OPTIONS and addtional mkfs options specified in the tests. (We've done similar update to _scratch_mkfs in commit 596a068bf130 ("fstests: teach _scratch_mkfs to handle mkfs option conflicts"), and Dave just brought up that _scratch_mkfs_sized couldn't handle conflicts in mkfs options just a few days ago.) And we can fix the tests that fail mkfs due to small fs size, enlarge the default fs size so mkfs succeeds even with additional mkfs options specified. But I'd like to know some exact example tests that fail for you, so I can dig into them more. > > So what ends up happening is the test will not notice that the mkfs > had failed, and would continue staggering on, sometimes succeeding, > and sometimes failing --- depending on what test was running > previously and the state of the scratch device. Ugh. Hence, my > addition of the "|| die ..." Yeah, this sometimes does happen. Historically we kind prefer letting the tests continue even if mkfs and/or mount failed, so the tests could run in some uncommon setups and exercise some corner cases (this did help find bugs that are hard to hit before), and in most cases tests would fail in the end so we noticed such failures. But we would check mkfs and/or mount failure explicitly if the test will do destructive operations on scratch dev, e.g. freeze $SCRATCH_DEV or fulfill $SCRATCH_MNT, so tests won't destroy the underlying filesystem (usually root fs). Thanks, Eryu > > The problem is that generic/466 is one of the few tests that actually > (a) deliberately feed arguments that cause _scratch_mkfs_sized to > fail, and (b) actually does error checking. > > Darrick mentioned on the call that he's seen similar problems which > he's had to track down when testing xfs, and it was simiarly annoying. > > So instead of it being a local hack in my xfstests tree, perhaps this > is something we should try to fix more generally. There a number of I > could imagine going about this. > > 1) The approach I took in this patch, which is requires accurately > determine min and max blocksizes --- and then to add error checks to > all of the mkfs invocations in _scratch_mkfs_blocksized for all file > systems. > > 2) Add an argument to _scratch_mkfs_blockized() and similar functions > which indicates that error checking should _not_ be done, since it > will be done by the caller (such as generic/466). And then add error > checks in _scratch_mkfs_blocksized() and similar functions unless the > optional flag is passed by the test. > > 3) Sweep through all of the tests scripts adding error checks. > > It seems to me (2) might be the simplest approach, although (1) does > speed up the test somewhat, and so maybe we should do (1) as well as > (2). Eryu, do you have any thoughts/opinions? > > > > I haven't tried it in some time, but the last time I ran xfstests > > > against fuse2fs it more or less worked (modulo all the fancy fallocate > > > stuff that it doesn't support). > > > > (Maybe we should separate out ext4/fuse2fs?) > > Darrick explained this require bind mounting mount.fuse2fs on > mount.ext4, and other unnatural hacks. It seems to me that it might > be useful to add first class support for fuse2fs to xfstests, with > arguments ala overlayfs to specify what the fuse driver (such as > fuse2fs) should be used; what should be used for mkfs, etc. > > Once we do this, we can use a different max blocksize for fuse2fs. > > - 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 -- 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 59d4b961..7aa747ad 100644 --- a/common/rc +++ b/common/rc @@ -1067,6 +1067,29 @@ _scratch_mkfs_blocksized() esac } +_fs_min_blocksize() +{ + case $FSTYP in + ext4) + if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then + get_page_size + else + echo 1024 + fi + ;; + btrfs) + get_page_size + ;; + *) + echo 512 + esac +} + +_fs_max_blocksize() +{ + get_page_size +} + _scratch_resvblks() { case $FSTYP in diff --git a/tests/generic/466 b/tests/generic/466 index 07f24a74..a1d944d3 100755 --- a/tests/generic/466 +++ b/tests/generic/466 @@ -54,7 +54,11 @@ unset MKFS_OPTIONS echo "Starting test" > $seqres.full devsize=$(blockdev --getsize64 $SCRATCH_DEV) -for blocksize in 512 1024 2048 4096 8192 16384 32768 65536; do +min_blocksize=$(_fs_min_blocksize) +max_blocksize=$(_fs_max_blocksize) + +for (( blocksize = min_blocksize ; blocksize <= max_blocksize ; + blocksize = blocksize * 2)); do echo "+ Format blocksize $blocksize and mount" >> $seqres.full _scratch_unmount > /dev/null 2>&1 # Try to format and mount with the given blocksize. If they don't
The test previously blindly tried block sizes from 512 to 65536 and relied on mkfs to fail. This is problematic for a number of file systems. For example, when testing btrfs, _scratch_mkfs_sized simply ignores the blocksize, so the test will pointlessly run the same test multiple times. For ext4, when encryption is enabled, the only block size which is supported is the page size. So define two new functions _fs_min_blocksize and _fs_max_blocksize, and use it so that generic/466 will only try using the block sizes that will work. Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- common/rc | 23 +++++++++++++++++++++++ tests/generic/466 | 6 +++++- 2 files changed, 28 insertions(+), 1 deletion(-)