| Message ID | 1366899176-12876-1-git-send-email-jbacik@fusionio.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Apr 25, 2013 at 10:12:56AM -0400, Josef Bacik wrote: > This test sets up a dm flakey target and then runs my fsync tester I've been > using to verify btrfs's fsync() is working properly. It will create a dm flakey > device, mount it, run my test, make the flakey device start dropping writes, and > then unmount the fs. Then we mount it back up and make sure the md5sums match > and then run fsck on the device to make sure we got a consistent fs. I used the > output from a run on BTRFS since it's the only one that passes this test > properly. I verified each test manually to make sure they were in fact valid > files. XFS and Ext4 both fail this test in one way or another. Thanks, > > Signed-off-by: Josef Bacik <jbacik@fusionio.com> > --- > V1->V2 > -make _test_check_fs take an argument on wether or not to force an exit, this is > because if we failed to fsck we'd leave the dmflakey device around which was > super annoying. Why doesn't the standard trap command in the test code catch the exit case and run the _cleanup function and unmount it? FWIW, if this change is actually necessary, then it needs to be in a separate patch. > -fixed the drop caches bug (thanks Zach!) > -fixed the output since XFS has a bug with that particular test, it leaves a 0 > length file behind which isn't right. What test, what bug, and why have you changed the test to work around it? ..... > @@ -478,7 +485,7 @@ _scratch_mkfs_ext4() > { > local tmp_dir=/tmp/ > > - /sbin/mkfs -t $FSTYP -- $MKFS_OPTIONS $* $SCRATCH_DEV \ > + /sbin/mkfs -t $FSTYP -- -F $MKFS_OPTIONS $* $SCRATCH_DEV \ > 2>$tmp_dir.mkfserr 1>$tmp_dir.mkfsstd > local mkfs_status=$? That seems like an unrelated bug fix? > @@ -1041,6 +1048,27 @@ _require_command() > [ -n "$1" -a -x "$1" ] || _notrun "$_cmd utility required, skipped this test" > } > > +# this test requires the device mapper flakey target > +# > +_require_dm_flakey() > +{ > + if [ "$HOSTOS" != "Linux" ] > + then > + _notrun "This test requires linux for dm flakey support" > + fi No need to check this - any test that uses dm-flakey should have a "_supported_os Linux" line in it. > + $DMSETUP_PROG targets | grep flakey >/dev/null 2>&1 > + if [ $? -eq 0 ] > + then > + : > + else > + _notrun "This test requires dm flakey support" > + fi [ $? -ne 0 ] && _notrun "This test requires dm flakey support" <snip all the "force exit" mess> <snip the fsync-tester.c code> FWIW, rebooting the machine at the end of the test should not be the default behaviour of fsync-tester.c... > diff --git a/tests/generic/311 b/tests/generic/311 > new file mode 100755 > index 0000000..3f7abe2 > --- /dev/null > +++ b/tests/generic/311 > @@ -0,0 +1,177 @@ > +#! /bin/bash > +# FS QA Test No. 311 > +# > +#Verify a file systems fsync is working properly. This won't catch problems > +#with blockdev flushing, but at the very least it makes sure the file system is > +#doing the right thing with fsync logically. How is this different to any of the other tests that test fsync() is working properly? Please describe what aspect of fsync is being tested.... > +# creator > +owner=jbacik@fusionio.com We don't need to add these any more. > +seq=`basename $0` > +seqres=$RESULT_DIR/$seq > +echo "QA output created by $seq" > + > +here=`pwd` > +status=1 # failure is the default! > + > +_cleanup() > +{ > + $UMOUNT_PROG $SCRATCH_MNT > /dev/null 2>&1 > + $DMSETUP_PROG remove flakey-test > /dev/null 2>&1 > +} > + > +_cleanup If we are called with a mounted scratch device, then something has gone badly wrong somewhere else - tests *always* start with unmounted test and scratch devices. What I suspect has gone wrong here is that you've removed the line: trap "_cleanup; exit \$status" 0 1 2 3 15 That will trigger the cleanup function whenever the test exits. I'd say this is the cause of the problem you have that has caused you to add the "force exit" crap to the filesytsem check functions... > +# get standard environment, filters and checks > +. ./common/rc > +. ./common/filter > + > +# real QA test starts here > +_supported_fs generic > +_supported_os Linux > +_need_to_be_root > +_require_scratch > +_require_dm_flakey > + > +[ -x $here/src/fsync-tester ] || _notrun "fsync-tester not build" > + > +rm -f $seqres.full > +BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` > +FLAKEY_DEV=/dev/mapper/flakey-test > +SEED=1 > +testfile=$SCRATCH_MNT/$seq.fsync > + > +_mount_flakey() > +{ > + _scratch_options mount > + _mount -t $FSTYP $SCRATCH_OPTIONS $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $* $FLAKEY_DEV $SCRATCH_MNT > +} Why do you need to open code all this? We typically don't do this for loopback device mounts, so I'm not sure it is necessary here. Indeed, if you are testing on the flakey device, you don't want XFS using external log or realtime devices, so really just something like: mount -t $FSTYP $MOUNT_OPTIONS $FLAKEY_DEV $SCRATCH_MNT will suffice... > +_unmount_flakey() > +{ > + $UMOUNT_PROG $FLAKEY_DEV > +} Empty line after this needed. FWIW, why use $FLAKEY_DEV here and not $SCRATCH_MNT like in the cleanup function? > +_drop_writes() > +{ > + $DMSETUP_PROG suspend flakey-test That freezes the filesystem, right? > + if [ $? -ne 0 ]; then > + echo "failed to suspend flakey-test" > + _unmount_flakey > + _cleanup > + exit > + fi With a properly functioning trap that calls _cleanup(), this can be replaced with: [ $? -ne 0 ] && _fatal "failed to suspend flakey-test" Same for all the other error cases. > + $DMSETUP_PROG load flakey-test --table "0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 0 180 1 drop_writes" Given that there are 2 different table configurations, perhaps defining them as variables will make it more obvious. e.g. FLAKEY_TABLE="0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 0 180 0" FLAKEY_WRITE_TABLE="0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 0 180 1 drop_writes" > +_run_test() > +{ > + test_num=$1 > + extra="" > + > + [ $2 -eq 1 ] && extra="-d" I'm assuming that $2 == 1 means "use direct IO" given it is not actually documented? Perhaps "extra" is not such a good name? > + $here/src/fsync-tester -s $SEED -r -t $test_num $extra $testfile > + if [ $? -ne 0 ]; then > + _unmount_flakey > + _cleanup > + exit > + fi > + > + _md5_checksum $testfile > + _drop_writes > + _unmount_flakey So, _drop_writes suspends the dm-flakey device, freezes the filesystem, turns off writes then thaws the filesystem, right? If so, doesn't that mean you're not actually testing fsync() as the freeze will effectively sync the entire filesystem before you start dropping writes? I can see why you want to stop unmount from writing back metadata to simulate a crash, but if you've already frozen the filesystem then writeback has already occurred before you stop the writes. So I can't see how this is actually testing fsync - what it appears to be testing is the fileystem freeze code... [ This is precisely the issue that XFS shutdown ioctls deal with to trigger an immediate forced shutdown of the filesystem that prevents *any* further writes from being issued by the filesystem - no sync operations get in the way and change the state of the filesystem after then fsync call, so we know that what is on disk is what was written by the sync/fsync calls being tested. This is how we test sync/fsync in other XFS tests (e.g. xfs/137-140), and this is the reason why us XFS people have suggested that other filesystems should implement the ioctls for this functionality rather than try to invent new ways of trying to stop filesystems from writing back dirty metadata for fsync/sync testing.... Besides, if a corruption is detected, you need a method of stopping all dirty metadata from being written back in the filesystem to prevent propagation of the corruption. These ioctls should just be an interface into that mechanism. ] > +_cleanup > +status=0 > +exit No need to call _cleanup if you have a functioning trap. And, more importantly, the only reason that status variable exists is so that the trap function can ensure a correct exit value from the test..... > diff --git a/tests/generic/group b/tests/generic/group > index eb52833..a0830c1 100644 > --- a/tests/generic/group > +++ b/tests/generic/group > @@ -113,3 +113,4 @@ > 308 auto quick > 309 auto quick > 310 auto > +311 auto How long does this take to run? It seems like the quick group would be appropriate if it takes less than a minute. Also, fsync tests fall under the category of "metadata" and "log", so they probably should be added, too. Cheers, Dave.
On Thu, Apr 25, 2013 at 04:45:56PM -0600, Dave Chinner wrote: > On Thu, Apr 25, 2013 at 10:12:56AM -0400, Josef Bacik wrote: > > This test sets up a dm flakey target and then runs my fsync tester I've been > > using to verify btrfs's fsync() is working properly. It will create a dm flakey > > device, mount it, run my test, make the flakey device start dropping writes, and > > then unmount the fs. Then we mount it back up and make sure the md5sums match > > and then run fsck on the device to make sure we got a consistent fs. I used the > > output from a run on BTRFS since it's the only one that passes this test > > properly. I verified each test manually to make sure they were in fact valid > > files. XFS and Ext4 both fail this test in one way or another. Thanks, > > > > Signed-off-by: Josef Bacik <jbacik@fusionio.com> > > --- > > V1->V2 > > -make _test_check_fs take an argument on wether or not to force an exit, this is > > because if we failed to fsck we'd leave the dmflakey device around which was > > super annoying. > > Why doesn't the standard trap command in the test code catch > the exit case and run the _cleanup function and unmount it? > Yeah I did the trap thing wrong it seems, I will fix that up and back out all of the related changes, thanks. > FWIW, if this change is actually necessary, then it needs to be in a > separate patch. > > > -fixed the drop caches bug (thanks Zach!) > > -fixed the output since XFS has a bug with that particular test, it leaves a 0 > > length file behind which isn't right. > > What test, what bug, and why have you changed the test to work > around it? > I didn't change the test, I just had to change the golden output. I was running it against btrfs and noticed that xfs had different output so I ran the test to just get the file (no flakey or unmount or anything) and the md5sum matched what btrfs did and then I looked at the file on xfs and it was empty, so xfs is messing up one of the testscases. Sorry I don't remember which one it was, I was going to run it again and try and track down what was going on tomorrow. > ..... > > @@ -478,7 +485,7 @@ _scratch_mkfs_ext4() > > { > > local tmp_dir=/tmp/ > > > > - /sbin/mkfs -t $FSTYP -- $MKFS_OPTIONS $* $SCRATCH_DEV \ > > + /sbin/mkfs -t $FSTYP -- -F $MKFS_OPTIONS $* $SCRATCH_DEV \ > > 2>$tmp_dir.mkfserr 1>$tmp_dir.mkfsstd > > local mkfs_status=$? > > That seems like an unrelated bug fix? > Ergh yeah sorry, I needed this to run the test on ext4, I'll take this out and send it seperately. > > @@ -1041,6 +1048,27 @@ _require_command() > > [ -n "$1" -a -x "$1" ] || _notrun "$_cmd utility required, skipped this test" > > } > > > > +# this test requires the device mapper flakey target > > +# > > +_require_dm_flakey() > > +{ > > + if [ "$HOSTOS" != "Linux" ] > > + then > > + _notrun "This test requires linux for dm flakey support" > > + fi > > No need to check this - any test that uses dm-flakey should have a > "_supported_os Linux" line in it. > Ok, just wanted to make sure in case somebody forgot to do the supported os thing. > > + $DMSETUP_PROG targets | grep flakey >/dev/null 2>&1 > > + if [ $? -eq 0 ] > > + then > > + : > > + else > > + _notrun "This test requires dm flakey support" > > + fi > > [ $? -ne 0 ] && _notrun "This test requires dm flakey support" > > <snip all the "force exit" mess> > > <snip the fsync-tester.c code> > > FWIW, rebooting the machine at the end of the test should not be the > default behaviour of fsync-tester.c... > Yeah I can change it to be an option, this is just what I was using originally to test and I was rebooting the box by default. > > diff --git a/tests/generic/311 b/tests/generic/311 > > new file mode 100755 > > index 0000000..3f7abe2 > > --- /dev/null > > +++ b/tests/generic/311 > > @@ -0,0 +1,177 @@ > > +#! /bin/bash > > +# FS QA Test No. 311 > > +# > > +#Verify a file systems fsync is working properly. This won't catch problems > > +#with blockdev flushing, but at the very least it makes sure the file system is > > +#doing the right thing with fsync logically. > > How is this different to any of the other tests that test fsync() is > working properly? Please describe what aspect of fsync is being > tested.... > > > +# creator > > +owner=jbacik@fusionio.com > > We don't need to add these any more. > Ok I'll remove it. > > +seq=`basename $0` > > +seqres=$RESULT_DIR/$seq > > +echo "QA output created by $seq" > > + > > +here=`pwd` > > +status=1 # failure is the default! > > + > > +_cleanup() > > +{ > > + $UMOUNT_PROG $SCRATCH_MNT > /dev/null 2>&1 > > + $DMSETUP_PROG remove flakey-test > /dev/null 2>&1 > > +} > > + > > +_cleanup > > If we are called with a mounted scratch device, then something has > gone badly wrong somewhere else - tests *always* start with > unmounted test and scratch devices. > > What I suspect has gone wrong here is that you've removed the line: > > trap "_cleanup; exit \$status" 0 1 2 3 15 > > That will trigger the cleanup function whenever the test exits. I'd > say this is the cause of the problem you have that has caused you to > add the "force exit" crap to the filesytsem check functions... > I figured I was doing something stupid, I should have just asked. > > +# get standard environment, filters and checks > > +. ./common/rc > > +. ./common/filter > > + > > +# real QA test starts here > > +_supported_fs generic > > +_supported_os Linux > > +_need_to_be_root > > +_require_scratch > > +_require_dm_flakey > > + > > +[ -x $here/src/fsync-tester ] || _notrun "fsync-tester not build" > > + > > +rm -f $seqres.full > > +BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` > > +FLAKEY_DEV=/dev/mapper/flakey-test > > +SEED=1 > > +testfile=$SCRATCH_MNT/$seq.fsync > > + > > +_mount_flakey() > > +{ > > + _scratch_options mount > > + _mount -t $FSTYP $SCRATCH_OPTIONS $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $* $FLAKEY_DEV $SCRATCH_MNT > > +} > > Why do you need to open code all this? We typically don't do > this for loopback device mounts, so I'm not sure it is necessary > here. Indeed, if you are testing on the flakey device, you don't > want XFS using external log or realtime devices, so really just > something like: > > mount -t $FSTYP $MOUNT_OPTIONS $FLAKEY_DEV $SCRATCH_MNT > > will suffice... Ok I will do that, I just wanted to be as close to what normal _scratch_mount does. > > > +_unmount_flakey() > > +{ > > + $UMOUNT_PROG $FLAKEY_DEV > > +} > > Empty line after this needed. FWIW, why use $FLAKEY_DEV here and not > $SCRATCH_MNT like in the cleanup function? > Just mindlessly copying _umount_scratch() > > > +_drop_writes() > > +{ > > + $DMSETUP_PROG suspend flakey-test > > That freezes the filesystem, right? > > > + if [ $? -ne 0 ]; then > > + echo "failed to suspend flakey-test" > > + _unmount_flakey > > + _cleanup > > + exit > > + fi > > With a properly functioning trap that calls _cleanup(), this can be > replaced with: > > [ $? -ne 0 ] && _fatal "failed to suspend flakey-test" > > Same for all the other error cases. > > > + $DMSETUP_PROG load flakey-test --table "0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 0 180 1 drop_writes" > > Given that there are 2 different table configurations, perhaps > defining them as variables will make it more obvious. e.g. > > FLAKEY_TABLE="0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 0 180 0" > FLAKEY_WRITE_TABLE="0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 0 180 1 drop_writes" > Good idea I will do that. > > +_run_test() > > +{ > > + test_num=$1 > > + extra="" > > + > > + [ $2 -eq 1 ] && extra="-d" > > I'm assuming that $2 == 1 means "use direct IO" given it is not > actually documented? Perhaps "extra" is not such a good name? > Yes I was going to add some other stuff but I will just change it to say direct to make it more obvious. > > + $here/src/fsync-tester -s $SEED -r -t $test_num $extra $testfile > > + if [ $? -ne 0 ]; then > > + _unmount_flakey > > + _cleanup > > + exit > > + fi > > + > > + _md5_checksum $testfile > > + _drop_writes > > + _unmount_flakey > > So, _drop_writes suspends the dm-flakey device, freezes the > filesystem, turns off writes then thaws the filesystem, right? > > If so, doesn't that mean you're not actually testing fsync() as the > freeze will effectively sync the entire filesystem before you start > dropping writes? > > I can see why you want to stop unmount from writing back metadata to > simulate a crash, but if you've already frozen the filesystem then > writeback has already occurred before you stop the writes. So I > can't see how this is actually testing fsync - what it appears to be > testing is the fileystem freeze code... > > [ This is precisely the issue that XFS shutdown ioctls deal with to > trigger an immediate forced shutdown of the filesystem that prevents > *any* further writes from being issued by the filesystem - no sync > operations get in the way and change the state of the filesystem > after then fsync call, so we know that what is on disk is what was > written by the sync/fsync calls being tested. > > This is how we test sync/fsync in other XFS tests (e.g. > xfs/137-140), and this is the reason why us XFS people have > suggested that other filesystems should implement the ioctls for > this functionality rather than try to invent new ways of trying > to stop filesystems from writing back dirty metadata for fsync/sync > testing.... > > Besides, if a corruption is detected, you need a method of stopping > all dirty metadata from being written back in the filesystem to > prevent propagation of the corruption. These ioctls should just be > an interface into that mechanism. ] > So I need to look at what this does. I don't think it freezes the file system, because I've run this test and I definitely have a tree log on the file system after I unmount, which means we didn't actually sync(). Either that or freeze() is broken for btrfs and it's not actually causing the transaction to commit. If it turns out that it is freeze() then it's not actually a fsync tester, but it seems to still be a valid test since it's definitely causing problems for all the file systems I've tested and I'll leave this as it is and then write another real fsync tester. > > +_cleanup > > +status=0 > > +exit > > No need to call _cleanup if you have a functioning trap. And, more > importantly, the only reason that status variable exists is so that > the trap function can ensure a correct exit value from the test..... > > > diff --git a/tests/generic/group b/tests/generic/group > > index eb52833..a0830c1 100644 > > --- a/tests/generic/group > > +++ b/tests/generic/group > > @@ -113,3 +113,4 @@ > > 308 auto quick > > 309 auto quick > > 310 auto > > +311 auto > > How long does this take to run? It seems like the quick group would > be appropriate if it takes less than a minute. Also, fsync tests > fall under the category of "metadata" and "log", so they probably > should be added, too. > It takes 66 seconds to run on btrfs, is that fast enough for quick? I'll add metadata and log to the list as well. Thanks for the thorough review, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 25, 2013 at 08:24:04PM -0400, Josef Bacik wrote: > On Thu, Apr 25, 2013 at 04:45:56PM -0600, Dave Chinner wrote: > > On Thu, Apr 25, 2013 at 10:12:56AM -0400, Josef Bacik wrote: ..... > > > + $here/src/fsync-tester -s $SEED -r -t $test_num $extra $testfile > > > + if [ $? -ne 0 ]; then > > > + _unmount_flakey > > > + _cleanup > > > + exit > > > + fi > > > + > > > + _md5_checksum $testfile > > > + _drop_writes > > > + _unmount_flakey > > > > So, _drop_writes suspends the dm-flakey device, freezes the > > filesystem, turns off writes then thaws the filesystem, right? > > > > If so, doesn't that mean you're not actually testing fsync() as the > > freeze will effectively sync the entire filesystem before you start > > dropping writes? > > > > I can see why you want to stop unmount from writing back metadata to > > simulate a crash, but if you've already frozen the filesystem then > > writeback has already occurred before you stop the writes. So I > > can't see how this is actually testing fsync - what it appears to be > > testing is the fileystem freeze code... > > > > [ This is precisely the issue that XFS shutdown ioctls deal with to > > trigger an immediate forced shutdown of the filesystem that prevents > > *any* further writes from being issued by the filesystem - no sync > > operations get in the way and change the state of the filesystem > > after then fsync call, so we know that what is on disk is what was > > written by the sync/fsync calls being tested. > > > > This is how we test sync/fsync in other XFS tests (e.g. > > xfs/137-140), and this is the reason why us XFS people have > > suggested that other filesystems should implement the ioctls for > > this functionality rather than try to invent new ways of trying > > to stop filesystems from writing back dirty metadata for fsync/sync > > testing.... > > > > Besides, if a corruption is detected, you need a method of stopping > > all dirty metadata from being written back in the filesystem to > > prevent propagation of the corruption. These ioctls should just be > > an interface into that mechanism. ] > > > > So I need to look at what this does. I don't think it freezes the file system, `dmsetup suspend` ends up in dm_suspend(). This calls lock_fs(), which calls freeze_bdev().... If you do `dmsetup suspend --nolockfs` then it won't freeze the filesystem during the suspend... > because I've run this test and I definitely have a tree log on the file system > after I unmount, which means we didn't actually sync(). Either that or freeze() > is broken for btrfs and it's not actually causing the transaction to commit. Entirely possible. /me muses randomly about why we call them bugs when they are almost always layered like onions..... > If > it turns out that it is freeze() then it's not actually a fsync tester, but it > seems to still be a valid test since it's definitely causing problems for all > the file systems I've tested and I'll leave this as it is and then write another > real fsync tester. ext4 as well? > > How long does this take to run? It seems like the quick group would > > be appropriate if it takes less than a minute. Also, fsync tests > > fall under the category of "metadata" and "log", so they probably > > should be added, too. > > > > It takes 66 seconds to run on btrfs, is that fast enough for quick? I'll add > metadata and log to the list as well. Thanks for the thorough review, Probably a bit too long for my liking - it won't be quick when there are several instances of VMs running xfstests on the same set of spindles.... Cheers, Dave.
On Thu, Apr 25, 2013 at 07:08:29PM -0600, Dave Chinner wrote: > On Thu, Apr 25, 2013 at 08:24:04PM -0400, Josef Bacik wrote: > > On Thu, Apr 25, 2013 at 04:45:56PM -0600, Dave Chinner wrote: > > > On Thu, Apr 25, 2013 at 10:12:56AM -0400, Josef Bacik wrote: > ..... > > > > + $here/src/fsync-tester -s $SEED -r -t $test_num $extra $testfile > > > > + if [ $? -ne 0 ]; then > > > > + _unmount_flakey > > > > + _cleanup > > > > + exit > > > > + fi > > > > + > > > > + _md5_checksum $testfile > > > > + _drop_writes > > > > + _unmount_flakey > > > > > > So, _drop_writes suspends the dm-flakey device, freezes the > > > filesystem, turns off writes then thaws the filesystem, right? > > > > > > If so, doesn't that mean you're not actually testing fsync() as the > > > freeze will effectively sync the entire filesystem before you start > > > dropping writes? > > > > > > I can see why you want to stop unmount from writing back metadata to > > > simulate a crash, but if you've already frozen the filesystem then > > > writeback has already occurred before you stop the writes. So I > > > can't see how this is actually testing fsync - what it appears to be > > > testing is the fileystem freeze code... > > > > > > [ This is precisely the issue that XFS shutdown ioctls deal with to > > > trigger an immediate forced shutdown of the filesystem that prevents > > > *any* further writes from being issued by the filesystem - no sync > > > operations get in the way and change the state of the filesystem > > > after then fsync call, so we know that what is on disk is what was > > > written by the sync/fsync calls being tested. > > > > > > This is how we test sync/fsync in other XFS tests (e.g. > > > xfs/137-140), and this is the reason why us XFS people have > > > suggested that other filesystems should implement the ioctls for > > > this functionality rather than try to invent new ways of trying > > > to stop filesystems from writing back dirty metadata for fsync/sync > > > testing.... > > > > > > Besides, if a corruption is detected, you need a method of stopping > > > all dirty metadata from being written back in the filesystem to > > > prevent propagation of the corruption. These ioctls should just be > > > an interface into that mechanism. ] > > > > > > > So I need to look at what this does. I don't think it freezes the file system, > > `dmsetup suspend` ends up in dm_suspend(). This calls lock_fs(), which > calls freeze_bdev().... > > If you do `dmsetup suspend --nolockfs` then it won't freeze the > filesystem during the suspend... > Ok so I think I'll just make this test do all the iterations of the fsync tester with and without --nolockfs, since without --nolockfs I'm still seeing problems, does that sound reasonable? > > because I've run this test and I definitely have a tree log on the file system > > after I unmount, which means we didn't actually sync(). Either that or freeze() > > is broken for btrfs and it's not actually causing the transaction to commit. > > Entirely possible. > > /me muses randomly about why we call them bugs when they are almost > always layered like onions..... > No kidding, I've spent all week unraveling various problems that this test has uncovered. > > If > > it turns out that it is freeze() then it's not actually a fsync tester, but it > > seems to still be a valid test since it's definitely causing problems for all > > the file systems I've tested and I'll leave this as it is and then write another > > real fsync tester. > > ext4 as well? > Yeah ext4 blows up right after the first test, the md5sums match but fsck complains loudly. > > > How long does this take to run? It seems like the quick group would > > > be appropriate if it takes less than a minute. Also, fsync tests > > > fall under the category of "metadata" and "log", so they probably > > > should be added, too. > > > > > > > It takes 66 seconds to run on btrfs, is that fast enough for quick? I'll add > > metadata and log to the list as well. Thanks for the thorough review, > > Probably a bit too long for my liking - it won't be quick when there > are several instances of VMs running xfstests on the same set of > spindles.... > Yeah I had quick originally but when I finally got it to run I took it out since 66 seconds seemed too long to me. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 25, 2013 at 09:32:37PM -0400, Josef Bacik wrote: > On Thu, Apr 25, 2013 at 07:08:29PM -0600, Dave Chinner wrote: > > On Thu, Apr 25, 2013 at 08:24:04PM -0400, Josef Bacik wrote: > > > On Thu, Apr 25, 2013 at 04:45:56PM -0600, Dave Chinner wrote: > > > > On Thu, Apr 25, 2013 at 10:12:56AM -0400, Josef Bacik wrote: > > ..... > > > > > + $here/src/fsync-tester -s $SEED -r -t $test_num $extra $testfile > > > > > + if [ $? -ne 0 ]; then > > > > > + _unmount_flakey > > > > > + _cleanup > > > > > + exit > > > > > + fi > > > > > + > > > > > + _md5_checksum $testfile > > > > > + _drop_writes > > > > > + _unmount_flakey > > > > > > > > So, _drop_writes suspends the dm-flakey device, freezes the > > > > filesystem, turns off writes then thaws the filesystem, right? > > > > > > > > If so, doesn't that mean you're not actually testing fsync() as the > > > > freeze will effectively sync the entire filesystem before you start > > > > dropping writes? > > > > > > > > I can see why you want to stop unmount from writing back metadata to > > > > simulate a crash, but if you've already frozen the filesystem then > > > > writeback has already occurred before you stop the writes. So I > > > > can't see how this is actually testing fsync - what it appears to be > > > > testing is the fileystem freeze code... > > > > > > > > [ This is precisely the issue that XFS shutdown ioctls deal with to > > > > trigger an immediate forced shutdown of the filesystem that prevents > > > > *any* further writes from being issued by the filesystem - no sync > > > > operations get in the way and change the state of the filesystem > > > > after then fsync call, so we know that what is on disk is what was > > > > written by the sync/fsync calls being tested. > > > > > > > > This is how we test sync/fsync in other XFS tests (e.g. > > > > xfs/137-140), and this is the reason why us XFS people have > > > > suggested that other filesystems should implement the ioctls for > > > > this functionality rather than try to invent new ways of trying > > > > to stop filesystems from writing back dirty metadata for fsync/sync > > > > testing.... > > > > > > > > Besides, if a corruption is detected, you need a method of stopping > > > > all dirty metadata from being written back in the filesystem to > > > > prevent propagation of the corruption. These ioctls should just be > > > > an interface into that mechanism. ] > > > > > > > > > > So I need to look at what this does. I don't think it freezes the file system, > > > > `dmsetup suspend` ends up in dm_suspend(). This calls lock_fs(), which > > calls freeze_bdev().... > > > > If you do `dmsetup suspend --nolockfs` then it won't freeze the > > filesystem during the suspend... > > > > Ok so I think I'll just make this test do all the iterations of the fsync tester > with and without --nolockfs, since without --nolockfs I'm still seeing problems, > does that sound reasonable? Sounds like a fine plan to me ;) Cheers, Dave.
On Thu, Apr 25, 2013 at 08:12:14PM -0600, Dave Chinner wrote: > On Thu, Apr 25, 2013 at 09:32:37PM -0400, Josef Bacik wrote: > > On Thu, Apr 25, 2013 at 07:08:29PM -0600, Dave Chinner wrote: > > > On Thu, Apr 25, 2013 at 08:24:04PM -0400, Josef Bacik wrote: > > > > On Thu, Apr 25, 2013 at 04:45:56PM -0600, Dave Chinner wrote: > > > > > On Thu, Apr 25, 2013 at 10:12:56AM -0400, Josef Bacik wrote: > > > ..... > > > > > > + $here/src/fsync-tester -s $SEED -r -t $test_num $extra $testfile > > > > > > + if [ $? -ne 0 ]; then > > > > > > + _unmount_flakey > > > > > > + _cleanup > > > > > > + exit > > > > > > + fi > > > > > > + > > > > > > + _md5_checksum $testfile > > > > > > + _drop_writes > > > > > > + _unmount_flakey > > > > > > > > > > So, _drop_writes suspends the dm-flakey device, freezes the > > > > > filesystem, turns off writes then thaws the filesystem, right? > > > > > > > > > > If so, doesn't that mean you're not actually testing fsync() as the > > > > > freeze will effectively sync the entire filesystem before you start > > > > > dropping writes? > > > > > > > > > > I can see why you want to stop unmount from writing back metadata to > > > > > simulate a crash, but if you've already frozen the filesystem then > > > > > writeback has already occurred before you stop the writes. So I > > > > > can't see how this is actually testing fsync - what it appears to be > > > > > testing is the fileystem freeze code... > > > > > > > > > > [ This is precisely the issue that XFS shutdown ioctls deal with to > > > > > trigger an immediate forced shutdown of the filesystem that prevents > > > > > *any* further writes from being issued by the filesystem - no sync > > > > > operations get in the way and change the state of the filesystem > > > > > after then fsync call, so we know that what is on disk is what was > > > > > written by the sync/fsync calls being tested. > > > > > > > > > > This is how we test sync/fsync in other XFS tests (e.g. > > > > > xfs/137-140), and this is the reason why us XFS people have > > > > > suggested that other filesystems should implement the ioctls for > > > > > this functionality rather than try to invent new ways of trying > > > > > to stop filesystems from writing back dirty metadata for fsync/sync > > > > > testing.... > > > > > > > > > > Besides, if a corruption is detected, you need a method of stopping > > > > > all dirty metadata from being written back in the filesystem to > > > > > prevent propagation of the corruption. These ioctls should just be > > > > > an interface into that mechanism. ] > > > > > > > > > > > > > So I need to look at what this does. I don't think it freezes the file system, > > > > > > `dmsetup suspend` ends up in dm_suspend(). This calls lock_fs(), which > > > calls freeze_bdev().... > > > > > > If you do `dmsetup suspend --nolockfs` then it won't freeze the > > > filesystem during the suspend... > > > > > > > Ok so I think I'll just make this test do all the iterations of the fsync tester > > with and without --nolockfs, since without --nolockfs I'm still seeing problems, > > does that sound reasonable? > > Sounds like a fine plan to me ;) > Btw its test 19 O_DIRECT that gives me a 0 length file, the buffered case is fine. The test just does a randomly sized sub-block sized write over and over again for a random number of times and fsync()'s in there randomly. The number is 3072 because that's the largest inline extent we can have in btrfs, I added it specifically to test our inline extent logging. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 26, 2013 at 03:31:01PM -0400, Josef Bacik wrote: > On Thu, Apr 25, 2013 at 08:12:14PM -0600, Dave Chinner wrote: > > > Ok so I think I'll just make this test do all the iterations of the fsync tester > > > with and without --nolockfs, since without --nolockfs I'm still seeing problems, > > > does that sound reasonable? > > > > Sounds like a fine plan to me ;) > > > > Btw its test 19 O_DIRECT that gives me a 0 length file, the buffered case is > fine. The test just does a randomly sized sub-block sized write over and over > again for a random number of times and fsync()'s in there randomly. The number > is 3072 because that's the largest inline extent we can have in btrfs, I added > it specifically to test our inline extent logging. Thanks, Interesting - it only runs fsync every 8 iterations of the loop. Can you check that it is running enough loops to execute a fsync? Cheers, Dave.
On Fri, Apr 26, 2013 at 04:05:22PM -0600, Dave Chinner wrote: > On Fri, Apr 26, 2013 at 03:31:01PM -0400, Josef Bacik wrote: > > On Thu, Apr 25, 2013 at 08:12:14PM -0600, Dave Chinner wrote: > > > > Ok so I think I'll just make this test do all the iterations of the fsync tester > > > > with and without --nolockfs, since without --nolockfs I'm still seeing problems, > > > > does that sound reasonable? > > > > > > Sounds like a fine plan to me ;) > > > > > > > Btw its test 19 O_DIRECT that gives me a 0 length file, the buffered case is > > fine. The test just does a randomly sized sub-block sized write over and over > > again for a random number of times and fsync()'s in there randomly. The number > > is 3072 because that's the largest inline extent we can have in btrfs, I added > > it specifically to test our inline extent logging. Thanks, > > Interesting - it only runs fsync every 8 iterations of the loop. Can > you check that it is running enough loops to execute a fsync? > If the loop doesn't fsync it still fsyncs before the program exits. Side note I once wasted a week because Chris's fsync tester _didn't_ fsync() before exit so it would tell you a md5sum of a file that hadn't fsync()ed before the md5sum and I just assumed btrfs was broken. This test does not make this mistake for that reason :). Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 26, 2013 at 06:32:14PM -0400, Josef Bacik wrote: > On Fri, Apr 26, 2013 at 04:05:22PM -0600, Dave Chinner wrote: > > On Fri, Apr 26, 2013 at 03:31:01PM -0400, Josef Bacik wrote: > > > On Thu, Apr 25, 2013 at 08:12:14PM -0600, Dave Chinner wrote: > > > > > Ok so I think I'll just make this test do all the iterations of the fsync tester > > > > > with and without --nolockfs, since without --nolockfs I'm still seeing problems, > > > > > does that sound reasonable? > > > > > > > > Sounds like a fine plan to me ;) > > > > > > > > > > Btw its test 19 O_DIRECT that gives me a 0 length file, the buffered case is > > > fine. The test just does a randomly sized sub-block sized write over and over > > > again for a random number of times and fsync()'s in there randomly. The number > > > is 3072 because that's the largest inline extent we can have in btrfs, I added > > > it specifically to test our inline extent logging. Thanks, > > > > Interesting - it only runs fsync every 8 iterations of the loop. Can > > you check that it is running enough loops to execute a fsync? > > > > If the loop doesn't fsync it still fsyncs before the program exits. Doh! I noticed that yesterday but forgot about it. Not enough coffee. I'll have a closer look, then. > Side note I once wasted a week because Chris's fsync tester > _didn't_ fsync() before exit so it would tell you a md5sum of a > file that hadn't fsync()ed before the md5sum and I just assumed > btrfs was broken. This test does not make this mistake for that > reason :). Thanks, I think we've all made mistakes like that at least once.... :/ Cheers, Dave.
diff --git a/common/config b/common/config index dfbb5c2..b8ab593 100644 --- a/common/config +++ b/common/config @@ -177,6 +177,7 @@ export FIO_PROG="`set_prog_path fio`" export FILEFRAG_PROG="`set_prog_path filefrag`" export E4DEFRAG_PROG="`set_prog_path e4defrag`" export LOGGER_PROG="`set_prog_path logger`" +export DMSETUP_PROG="`set_prog_path dmsetup`" # Generate a comparable xfsprogs version number in the form of # major * 10000 + minor * 100 + release diff --git a/common/punch b/common/punch index cfbe576..b9f9acd 100644 --- a/common/punch +++ b/common/punch @@ -234,13 +234,6 @@ _filter_hole_fiemap() _coalesce_extents } - -# Prints the md5 checksum of a given file -_md5_checksum() -{ - md5sum $1 | cut -d ' ' -f1 -} - _filter_bmap() { awk ' diff --git a/common/rc b/common/rc index b980697..0eea37d 100644 --- a/common/rc +++ b/common/rc @@ -57,6 +57,13 @@ dd() fi } +# Prints the md5 checksum of a given file +_md5_checksum() +{ + md5sum $1 | cut -d ' ' -f1 +} + + # ls -l w/ selinux sometimes puts a dot at the end: # -rwxrw-r--. id1 id2 file1 @@ -478,7 +485,7 @@ _scratch_mkfs_ext4() { local tmp_dir=/tmp/ - /sbin/mkfs -t $FSTYP -- $MKFS_OPTIONS $* $SCRATCH_DEV \ + /sbin/mkfs -t $FSTYP -- -F $MKFS_OPTIONS $* $SCRATCH_DEV \ 2>$tmp_dir.mkfserr 1>$tmp_dir.mkfsstd local mkfs_status=$? @@ -1041,6 +1048,27 @@ _require_command() [ -n "$1" -a -x "$1" ] || _notrun "$_cmd utility required, skipped this test" } +# this test requires the device mapper flakey target +# +_require_dm_flakey() +{ + if [ "$HOSTOS" != "Linux" ] + then + _notrun "This test requires linux for dm flakey support" + fi + + _require_command $DMSETUP_PROG + + modprobe dm-flakey >/dev/null 2>&1 + $DMSETUP_PROG targets | grep flakey >/dev/null 2>&1 + if [ $? -eq 0 ] + then + : + else + _notrun "This test requires dm flakey support" + fi +} + # this test requires the projid32bit feature to be available in # mkfs.xfs # @@ -1356,7 +1384,9 @@ _mount_or_remount_rw() _check_generic_filesystem() { device=$1 + force_exit=$2 + ret=0 # If type is set, we're mounted type=`_fs_type $device` ok=1 @@ -1395,22 +1425,24 @@ _check_generic_filesystem() if [ $ok -eq 0 ]; then status=1 - exit 1 + ret=1 + [ $force_exit -eq 1 ] && exit 1 fi - return 0 + return $ret } # run xfs_check and friends on a FS. _check_xfs_filesystem() { - if [ $# -ne 3 ] + if [ $# -ne 4 ] then - echo "Usage: _check_xfs_filesystem device <logdev>|none <rtdev>|none" 1>&2 + echo "Usage: _check_xfs_filesystem device <logdev>|none <rtdev>|none force_exit" 1>&2 exit 1 fi + force_exit=$4 extra_mount_options="" device=$1 if [ "$2" != "none" ]; then @@ -1428,6 +1460,7 @@ _check_xfs_filesystem() type=`_fs_type $device` ok=1 + ret=0 if [ "$type" = "xfs" ] then @@ -1494,10 +1527,11 @@ _check_xfs_filesystem() if [ $ok -eq 0 ]; then status=1 - exit 1 + ret=1 + [ $force_exit -eq 1 ] && exit 1 fi - return 0 + return $ret } # Filter the knowen errors the UDF Verifier reports. @@ -1553,7 +1587,7 @@ _check_xfs_test_fs() [ "$USE_EXTERNAL" = yes -a ! -z "$TEST_RTDEV" ] && \ TEST_RT="$TEST_RTDEV" - _check_xfs_filesystem $TEST_DEV $TEST_LOG $TEST_RT + _check_xfs_filesystem $TEST_DEV $TEST_LOG $TEST_RT 1 # check for ipath consistency if $XFS_GROWFS_PROG -n $TEST_DIR | grep -q 'inode-paths=1'; then @@ -1566,10 +1600,12 @@ _check_xfs_test_fs() _check_btrfs_filesystem() { device=$1 + force_exit=$2 # If type is set, we're mounted type=`_fs_type $device` ok=1 + ret=0 if [ "$type" = "$FSTYP" ] then @@ -1605,10 +1641,11 @@ _check_btrfs_filesystem() if [ $ok -eq 0 ]; then status=1 - exit 1 + ret=1 + [ $force_exit -eq 1 ] && exit 1 fi - return 0 + return $ret } _check_test_fs() @@ -1624,16 +1661,18 @@ _check_test_fs() # do nothing for now ;; btrfs) - _check_btrfs_filesystem $TEST_DEV + _check_btrfs_filesystem $TEST_DEV 1 ;; *) - _check_generic_filesystem $TEST_DEV + _check_generic_filesystem $TEST_DEV 1 ;; esac } _check_scratch_fs() { + force_exit=1 + [ $# -eq 1 ] && force_exit=$1 case $FSTYP in xfs) SCRATCH_LOG="none" @@ -1644,19 +1683,21 @@ _check_scratch_fs() [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_RTDEV" ] && \ SCRATCH_RT="$SCRATCH_RTDEV" - _check_xfs_filesystem $SCRATCH_DEV $SCRATCH_LOG $SCRATCH_RT + _check_xfs_filesystem $SCRATCH_DEV $SCRATCH_LOG $SCRATCH_RT $force_exit ;; udf) + # Seems _check_udf_filesystem doesnt exit 1 if it fails so we're good + # here _check_udf_filesystem $SCRATCH_DEV $udf_fsize ;; nfs*) # Don't know how to check an NFS filesystem, yet. ;; btrfs) - _check_btrfs_filesystem $SCRATCH_DEV + _check_btrfs_filesystem $SCRATCH_DEV $force_exit ;; *) - _check_generic_filesystem $SCRATCH_DEV + _check_generic_filesystem $SCRATCH_DEV $force_exit ;; esac } diff --git a/src/Makefile b/src/Makefile index 8d8e97f..c18ffc9 100644 --- a/src/Makefile +++ b/src/Makefile @@ -18,7 +18,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ locktest unwritten_mmap bulkstat_unlink_test t_stripealign \ bulkstat_unlink_test_modified t_dir_offset t_futimens t_immutable \ stale_handle pwrite_mmap_blocked t_dir_offset2 seek_sanity_test \ - seek_copy_test t_readdir_1 t_readdir_2 + seek_copy_test t_readdir_1 t_readdir_2 fsync-tester SUBDIRS = diff --git a/src/fsync-tester.c b/src/fsync-tester.c new file mode 100644 index 0000000..092d953 --- /dev/null +++ b/src/fsync-tester.c @@ -0,0 +1,538 @@ +#include <sys/time.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <errno.h> +#include <fcntl.h> +#include <limits.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <unistd.h> + +static int test_fd; +static char *buf; +static char *fname; + +/* + * Just creates a random file, overwriting the file in a random number of loops + * and fsyncing between each loop. + */ +static int test_one(int *max_blocks) +{ + int loops = (random() % 20) + 5; + + lseek(test_fd, 0, SEEK_SET); + while (loops--) { + int character = (random() % 126) + 33; /* printable character */ + int blocks = (random() % 100) + 1; + + if (blocks > *max_blocks) + *max_blocks = blocks; + lseek(test_fd, 0, SEEK_SET); + memset(buf, character, 4096); + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed, test results will be " + "invalid: %d\n", errno); + return 1; + } + + while (blocks--) { + if (write(test_fd, buf, 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + } + } + + return 0; +} + +/* + * Preallocate a randomly sized file and then overwrite the entire thing and + * then fsync. + */ +static int test_two(int *max_blocks) +{ + int blocks = (random() % 1024) + 1; + int character = (random() % 126) + 33; + + *max_blocks = blocks; + + if (fallocate(test_fd, 0, 0, blocks * 4096)) { + fprintf(stderr, "Error fallocating %d (%s)\n", errno, + strerror(errno)); + return 1; + } + + lseek(test_fd, 0, SEEK_SET); + memset(buf, character, 4096); + while (blocks--) { + if (write(test_fd, buf, 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + } + + return 0; +} + +static void drop_all_caches() +{ + char value[] = "3\n"; + int fd; + + if ((fd = open("/proc/sys/vm/drop_caches", O_WRONLY)) < 0) { + fprintf(stderr, "Error opening drop caches: %d\n", errno); + return; + } + + write(fd, value, sizeof(value)-1); + close(fd); +} + +/* + * Randomly write inside of a file, either creating a sparse file or prealloc + * the file and randomly write within it, depending on the prealloc flag + */ +static int test_three(int *max_blocks, int prealloc, int rand_fsync, + int sync, int drop_caches) +{ + int size = (random() % 2048) + 4; + int blocks = size / 2; + int sync_block = blocks / 2; + int rand_sync_interval = (random() % blocks) + 1; + int character = (random() % 126) + 33; + + printf("Size is %d, blocks is %d\n", size, blocks); + if (prealloc && fallocate(test_fd, 0, 0, size * 4096)) { + fprintf(stderr, "Error fallocating %d (%s)\n", errno, + strerror(errno)); + return 1; + } + + if (prealloc) + *max_blocks = size; + + memset(buf, character, 4096); + while (blocks--) { + int block = (random() % size); + + if ((block + 1) > *max_blocks) + *max_blocks = block + 1; + + if (rand_fsync && !(blocks % rand_sync_interval)) { + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed, test results " + "will be invalid: %d\n", errno); + return 1; + } + } + + /* Force a transaction commit in between just for fun */ + if (blocks == sync_block && (sync || drop_caches)) { + if (sync) + syncfs(test_fd); + else + sync_file_range(test_fd, 0, 0, + SYNC_FILE_RANGE_WRITE| + SYNC_FILE_RANGE_WAIT_AFTER); + + if (drop_caches) { + close(test_fd); + drop_all_caches(); + test_fd = open(fname, O_RDWR); + if (test_fd < 0) { + test_fd = 0; + fprintf(stderr, "Error re-opening file: %d\n", + errno); + return 1; + } + } + } + + if (pwrite(test_fd, buf, 4096, block * 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + } + + return 0; +} + +static void timeval_subtract(struct timeval *result,struct timeval *x, + struct timeval *y) +{ + if (x->tv_usec < y->tv_usec) { + int nsec = (y->tv_usec - x->tv_usec) / 1000000 + 1; + y->tv_usec -= 1000000 * nsec; + y->tv_sec += nsec; + } + + if (x->tv_usec - y->tv_usec > 1000000) { + int nsec = (x->tv_usec - y->tv_usec) / 1000000; + y->tv_usec += 1000000 * nsec; + y->tv_sec -= nsec; + } + + result->tv_sec = x->tv_sec - y->tv_sec; + result->tv_usec = x->tv_usec - y->tv_usec; +} + +static int test_four(int *max_blocks) +{ + size_t size = 2621440; /* 10 gigabytes */ + size_t blocks = size / 2; + size_t sync_block = blocks / 8; /* fsync 8 times */ + int character = (random() % 126) + 33; + struct timeval start, end, diff; + + memset(buf, character, 4096); + while (blocks--) { + off_t block = (random() % size); + + if ((block + 1) > *max_blocks) + *max_blocks = block + 1; + + if ((blocks % sync_block) == 0) { + if (gettimeofday(&start, NULL)) { + fprintf(stderr, "Error getting time: %d\n", + errno); + return 1; + } + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed, test results " + "will be invalid: %d\n", errno); + return 1; + } + if (gettimeofday(&end, NULL)) { + fprintf(stderr, "Error getting time: %d\n", + errno); + return 1; + } + timeval_subtract(&diff, &end, &start); + printf("Fsync time was %ds and %dus\n", + (int)diff.tv_sec, (int)diff.tv_usec); + } + + if (pwrite(test_fd, buf, 4096, block * 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + } + + return 0; +} + +static int test_five() +{ + int character = (random() % 126) + 33; + int runs = (random() % 100) + 1; + int i; + + memset(buf, character, 3072); + for (i = 0; i < runs; i++) { + size_t write_size = (random() % 3072) + 1; + + if (pwrite(test_fd, buf, write_size, 0) < write_size) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + + if ((i % 8) == 0) { + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed, test results " + "will be invalid: %d\n", errno); + return 1; + } + } + } + + return 0; +} + +/* + * Reproducer for something like this + * + * [data][prealloc][data] + * + * and then in the [prealloc] section we have + * + * [ pre ][pre][ pre ] + * [d][pp][dd][ppp][d][ppp][d] + * + * where each letter represents on block of either data or prealloc. + * + * This explains all the weirdly specific numbers. + */ +static int test_six() +{ + int character = (random() % 126) + 33; + int i; + + memset(buf, character, 4096); + + /* Write on either side of the file, leaving a hole in the middle */ + for (i = 0; i < 10; i++) { + if (pwrite(test_fd, buf, 4096, i * 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + } + + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed %d\n", errno); + return 1; + } + + /* + * The test fs I had the prealloc extent was 13 4k blocks long so I'm + * just using that to give myself the best chances of reproducing. + */ + for (i = 23; i < 33; i++) { + if (pwrite(test_fd, buf, 4096, i * 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + } + + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed %d\n", errno); + return 1; + } + + if (fallocate(test_fd, 0, 10 * 4096, 4 * 4096)) { + fprintf(stderr, "Error fallocating %d\n", errno); + return 1; + } + + if (fallocate(test_fd, 0, 14 * 4096, 5 * 4096)) { + fprintf(stderr, "Error fallocating %d\n", errno); + return 1; + } + + if (fallocate(test_fd, 0, 19 * 4096, 4 * 4096)) { + fprintf(stderr, "Error fallocating %d\n", errno); + return 1; + } + + if (pwrite(test_fd, buf, 4096, 10 * 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed %d\n", errno); + return 1; + } + + for (i = 13; i < 15; i++) { + if (pwrite(test_fd, buf, 4096, i * 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + } + + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed %d\n", errno); + return 1; + } + + if (pwrite(test_fd, buf, 4096, 18 * 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed %d\n", errno); + return 1; + } + + if (pwrite(test_fd, buf, 4096, 22 * 4096) < 4096) { + fprintf(stderr, "Short write %d\n", errno); + return 1; + } + + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed %d\n", errno); + return 1; + } + + return 0; +} + +static void usage() +{ + printf("Usage fsync-tester [-s <seed>] [-r] [-d] -t <test-num> <filename>\n"); + printf(" -s seed : seed for teh random map generator (defaults to reading /dev/urandom)\n"); + printf(" -r : don't reboot the box immediately\n"); + printf(" -d : use O_DIRECT\n"); + printf(" -t test : test nr to run, required\n"); + exit(1); +} + +int main(int argc, char **argv) +{ + int opt; + int fd; + int max_blocks = 0; + char *endptr; + unsigned int seed = 123; + int reboot = 1; + int direct_io = 0; + long int test = 1; + long int tmp; + int ret = 0; + int flags = O_RDWR|O_CREAT|O_TRUNC; + + if (argc < 2) + usage(); + + fd = open("/dev/urandom", O_RDONLY); + if (fd >= 0) { + read(fd, &seed, sizeof(seed)); + close(fd); + } + + while ((opt = getopt(argc, argv, "s:rdt:")) != -1) { + switch (opt) { + case 's': + tmp = strtol(optarg, &endptr, 10); + if (tmp == LONG_MAX || endptr == optarg) + usage(); + seed = tmp; + break; + case 'r': + reboot = 0; + break; + case 'd': + direct_io = 1; + break; + case 't': + test = strtol(optarg, &endptr, 10); + if (test == LONG_MAX || endptr == optarg) + usage(); + break; + default: + usage(); + } + } + + if (optind >= argc) + usage(); + + fname = argv[optind]; + if (!fname) + usage(); + + printf("Random seed is %u\n", seed); + srandom(seed); + + if (direct_io) { + flags |= O_DIRECT; + ret = posix_memalign((void **)&buf, getpagesize(), 4096); + if (ret) + buf = NULL; + } else { + buf = malloc(4096); + } + + if (!buf) { + fprintf(stderr, "Error allocating buf: %d\n", errno); + return 1; + } + + test_fd = open(fname, flags, 0644); + if (test_fd < 0) { + fprintf(stderr, "Error opening file %d (%s)\n", errno, + strerror(errno)); + return 1; + } + + switch (test) { + case 1: + ret = test_one(&max_blocks); + break; + case 2: + ret = test_two(&max_blocks); + break; + case 3: + ret = test_three(&max_blocks, 0, 0, 0, 0); + break; + case 4: + ret = test_three(&max_blocks, 1, 0, 0, 0); + break; + case 5: + ret = test_three(&max_blocks, 0, 1, 0, 0); + break; + case 6: + ret = test_three(&max_blocks, 1, 1, 0, 0); + break; + case 7: + ret = test_three(&max_blocks, 0, 0, 1, 0); + break; + case 8: + ret = test_three(&max_blocks, 1, 0, 1, 0); + break; + case 9: + ret = test_three(&max_blocks, 0, 1, 1, 0); + break; + case 10: + ret = test_three(&max_blocks, 1, 1, 1, 0); + break; + case 11: + ret = test_three(&max_blocks, 0, 0, 0, 1); + break; + case 12: + ret = test_three(&max_blocks, 0, 1, 0, 1); + break; + case 13: + ret = test_three(&max_blocks, 0, 0, 1, 1); + break; + case 14: + ret = test_three(&max_blocks, 0, 1, 1, 1); + break; + case 15: + ret = test_three(&max_blocks, 1, 0, 0, 1); + break; + case 16: + ret = test_three(&max_blocks, 1, 1, 0, 1); + break; + case 17: + ret = test_three(&max_blocks, 1, 0, 1, 1); + break; + case 18: + ret = test_three(&max_blocks, 1, 1, 1, 1); + break; + case 19: + ret = test_five(); + break; + case 20: + ret = test_six(); + break; + case 21: + /* + * This is just a perf test, keep moving it down so it's always + * the last test option. + */ + reboot = 0; + ret = test_four(&max_blocks); + goto out; + default: + usage(); + } + + if (ret) + goto out; + + if (fsync(test_fd)) { + fprintf(stderr, "Fsync failed, test results will be invalid: " + "%d\n", errno); + return 1; + } + if (reboot) + system("reboot -fn"); +out: + free(buf); + close(test_fd); + return ret; +} diff --git a/tests/generic/311 b/tests/generic/311 new file mode 100755 index 0000000..3f7abe2 --- /dev/null +++ b/tests/generic/311 @@ -0,0 +1,177 @@ +#! /bin/bash +# FS QA Test No. 311 +# +#Verify a file systems fsync is working properly. This won't catch problems +#with blockdev flushing, but at the very least it makes sure the file system is +#doing the right thing with fsync logically. +# +#----------------------------------------------------------------------- +# Copyright (c) 2013 Fusion IO. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# +# creator +owner=jbacik@fusionio.com + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +_cleanup() +{ + $UMOUNT_PROG $SCRATCH_MNT > /dev/null 2>&1 + $DMSETUP_PROG remove flakey-test > /dev/null 2>&1 +} + +_cleanup + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_fs generic +_supported_os Linux +_need_to_be_root +_require_scratch +_require_dm_flakey + +[ -x $here/src/fsync-tester ] || _notrun "fsync-tester not build" + +rm -f $seqres.full +BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` +FLAKEY_DEV=/dev/mapper/flakey-test +SEED=1 +testfile=$SCRATCH_MNT/$seq.fsync + +_mount_flakey() +{ + _scratch_options mount + _mount -t $FSTYP $SCRATCH_OPTIONS $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $* $FLAKEY_DEV $SCRATCH_MNT +} + +_unmount_flakey() +{ + $UMOUNT_PROG $FLAKEY_DEV +} +_drop_writes() +{ + $DMSETUP_PROG suspend flakey-test + if [ $? -ne 0 ]; then + echo "failed to suspend flakey-test" + _unmount_flakey + _cleanup + exit + fi + $DMSETUP_PROG load flakey-test --table "0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 0 180 1 drop_writes" + if [ $? -ne 0 ]; then + echo "failed to load table into flakey-test" + _unmount_flakey + _cleanup + exit + fi + $DMSETUP_PROG resume flakey-test + if [ $? -ne 0 ]; then + echo "failed to resumeflakey-test" + _unmount_flakey + _cleanup + exit + fi +} + +_resume_writes() +{ + $DMSETUP_PROG suspend flakey-test + if [ $? -ne 0 ]; then + echo "failed to suspend flakey-test" + _unmount_flakey + _cleanup + exit + fi + $DMSETUP_PROG load flakey-test --table "0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 180 0" + if [ $? -ne 0 ]; then + echo "failed to load table into flakey-test" + _unmount_flakey + _cleanup + exit + fi + $DMSETUP_PROG resume flakey-test + if [ $? -ne 0 ]; then + echo "failed to resumeflakey-test" + _unmount_flakey + _cleanup + exit + fi +} + +_run_test() +{ + test_num=$1 + extra="" + + [ $2 -eq 1 ] && extra="-d" + + $here/src/fsync-tester -s $SEED -r -t $test_num $extra $testfile + if [ $? -ne 0 ]; then + _unmount_flakey + _cleanup + exit + fi + + _md5_checksum $testfile + _drop_writes + _unmount_flakey + + #Ok mount so that any recovery that needs to happen is done + _resume_writes + _mount_flakey + _md5_checksum $testfile + + #Unmount and fsck to make sure we got a valid fs after replay + _unmount_flakey + _check_scratch_fs 0 + if [ $? -ne 0 ]; then + echo "fs inconsistent" + _cleanup + exit + fi + _mount_flakey +} + +_scratch_mkfs >> $seqres.full 2>&1 + +# Create a basic flakey device that will never error out +$DMSETUP_PROG create flakey-test --table "0 $BLK_DEV_SIZE flakey $SCRATCH_DEV 0 180 0" +if [ $? -ne 0 ]; then + echo "failed to create flakey device" + status=1 + exit +fi + +_mount_flakey + +for i in $(seq 1 20); do + echo "Running test $i buffered" + _run_test $i 0 + echo "Running test $i direct" + _run_test $i 1 +done + +_cleanup +status=0 +exit diff --git a/tests/generic/311.out b/tests/generic/311.out new file mode 100644 index 0000000..59d173b --- /dev/null +++ b/tests/generic/311.out @@ -0,0 +1,193 @@ +QA output created by 311 +Running test 1 buffered +Random seed is 1 +ee6103415276cde95544b11b2675f132 +ee6103415276cde95544b11b2675f132 +Running test 1 direct +Random seed is 1 +ee6103415276cde95544b11b2675f132 +ee6103415276cde95544b11b2675f132 +Running test 2 buffered +Random seed is 1 +6ec7dd6da414514b832d9a8fc9098a06 +6ec7dd6da414514b832d9a8fc9098a06 +Running test 2 direct +Random seed is 1 +6ec7dd6da414514b832d9a8fc9098a06 +6ec7dd6da414514b832d9a8fc9098a06 +Running test 3 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 3 direct +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 4 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 4 direct +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 5 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 5 direct +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 6 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 6 direct +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 7 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 7 direct +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 8 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 8 direct +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 9 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 9 direct +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 10 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 10 direct +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 11 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 11 direct +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 12 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 12 direct +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 13 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 13 direct +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 14 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 14 direct +Random seed is 1 +Size is 1924, blocks is 962 +c80ba7f17988006f10bdc488efa9b372 +c80ba7f17988006f10bdc488efa9b372 +Running test 15 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 15 direct +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 16 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 16 direct +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 17 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 17 direct +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 18 buffered +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 18 direct +Random seed is 1 +Size is 1924, blocks is 962 +c5f2b19e14d0f3e6755cac2986b8366d +c5f2b19e14d0f3e6755cac2986b8366d +Running test 19 buffered +Random seed is 1 +9cc0323438bb10f5cc88cfbd5aaf5cfa +9cc0323438bb10f5cc88cfbd5aaf5cfa +Running test 19 direct +Random seed is 1 +9cc0323438bb10f5cc88cfbd5aaf5cfa +9cc0323438bb10f5cc88cfbd5aaf5cfa +Running test 20 buffered +Random seed is 1 +764e24deb80dc89aaecdadce4a656052 +764e24deb80dc89aaecdadce4a656052 +Running test 20 direct +Random seed is 1 +764e24deb80dc89aaecdadce4a656052 +764e24deb80dc89aaecdadce4a656052 diff --git a/tests/generic/group b/tests/generic/group index eb52833..a0830c1 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -113,3 +113,4 @@ 308 auto quick 309 auto quick 310 auto +311 auto
This test sets up a dm flakey target and then runs my fsync tester I've been using to verify btrfs's fsync() is working properly. It will create a dm flakey device, mount it, run my test, make the flakey device start dropping writes, and then unmount the fs. Then we mount it back up and make sure the md5sums match and then run fsck on the device to make sure we got a consistent fs. I used the output from a run on BTRFS since it's the only one that passes this test properly. I verified each test manually to make sure they were in fact valid files. XFS and Ext4 both fail this test in one way or another. Thanks, Signed-off-by: Josef Bacik <jbacik@fusionio.com> --- V1->V2 -make _test_check_fs take an argument on wether or not to force an exit, this is because if we failed to fsck we'd leave the dmflakey device around which was super annoying. -fixed the drop caches bug (thanks Zach!) -fixed the output since XFS has a bug with that particular test, it leaves a 0 length file behind which isn't right. common/config | 1 + common/punch | 7 - common/rc | 71 +++++-- src/Makefile | 2 +- src/fsync-tester.c | 538 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/311 | 177 ++++++++++++++++ tests/generic/311.out | 193 ++++++++++++++++++ tests/generic/group | 1 + 8 files changed, 967 insertions(+), 23 deletions(-) create mode 100644 src/fsync-tester.c create mode 100755 tests/generic/311 create mode 100644 tests/generic/311.out