On Tue, Feb 25, 2014 at 10:34:07PM +0000, Filipe David Manana wrote: > On Tue, Feb 25, 2014 at 10:11 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Tue, Feb 25, 2014 at 09:02:43PM +0000, Filipe David Manana wrote: > >> On Tue, Feb 25, 2014 at 7:54 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > >> > On Tue, Feb 25, 2014 at 06:44:08PM +0000, Filipe David Borba Manana wrote: > >> >> This is a regression test to verify that the restore feature of btrfs-progs > >> >> is able to correctly recover files that have compressed extents, specially when > >> >> the respective file extent items have a non-zero data offset field. > >> >> > >> >> This issue is fixed by the following btrfs-progs patch: > >> >> > >> >> Btrfs-progs: fix restore of files with compressed extents > >> >> > >> >> Signed-off-by: Filipe David Borba Manana <fdmanana@xxxxxxxxx> > >> > .... > >> >> +seq=`basename $0` > >> >> +seqres=$RESULT_DIR/$seq > >> >> +echo "QA output created by $seq" > >> >> + > >> >> +tmp=/tmp/$$ > >> >> +status=1 # failure is the default! > >> >> +trap "_cleanup; exit \$status" 0 1 2 3 15 > >> > > >> > here=`pwd` > >> > >> Didn't we agree before, for a previous path, to export "here" from the > >> main control skip and then cleanup tests to not redefine it? > >> I am confused now :) > > > > Yes, we did, but there's no patch to do that yet. Hence tests need > > to define it until the infrastructure is changed..... > > There's a patch flying around that adds the _require_fssum() and then > removes definition of "here" for all btrfs tests that use fssum. changing how $here is defined needs to be in a patch of it's own, and that patch needs to remove it from every single test in the xfstests code base that declares it. test harness infrastructure changes should not be buried in an unrelated btrfs test changes.... > >> >> + | _filter_xfs_io > >> >> + $XFS_IO_PROG -c "pwrite -S 0xd0 -b 11 33000 11" $SCRATCH_MNT/foo \ > >> >> + | _filter_xfs_io > >> >> + $XFS_IO_PROG -c "pwrite -S 0xbc -b 100 99000 100" $SCRATCH_MNT/foo \ > >> >> + | _filter_xfs_io > >> >> + > >> >> + md5sum $SCRATCH_MNT/foo | _filter_scratch > >> > > >> > So you are doing this with first having "persisted" the new extents. > >> > Seems kind of strange that you need to persist some and not > >> > others... > > All I want is to have different file extent items. Yes, I get that. What is not clear is where you expect the failure to be detected - in memory or on disk? > >> I need to make sure there's fragmentation (i.e. several file extent > >> items in the fs btree with data offset fields > 0). > > > > Right, but my question is why you haven't ensured that btree is on > > disk at the time you run the md5sum. > > Because it's not needed. > The sync is only to make sure the first 2 extent items aren't merged > together. And that is needed to trigger the failure. Yes, it's a pre-condition. That's not the answer to the question I've been asking, though. > > That seems important to me > > because the above sync commands indicate that having the extents on > > disk rather than in memory is important here. e.g. are you expecting > > the md5sum to be correct before the data is synced to disk, and then > > incorrect after the data is synced to disk by the unmount? > > Again what's important is having multiple extent items after unmounting. Ah, so syncing the data after the second set of writes is important, and that's what you are testing. So, why aren't you testing this with sync and fiemap? Or is it the unmount that matters, rather than the data writeback? Do you see what I'm trying to understand? If it's data writeback that triggers the bug or enables it to be detected, then that's what the test should use as the trigger. If unmount is necessary, then a comment saying "data writeback is not sufficient to trigger or detect the corruption - we need can only detect it via unmount and a fsck pass".... > > code in it to check and unmount the scratch device, so if that is > > not happening then there's something broken that needs to be fixed. > > So _check_btrfs_filesystem() will unmount the fs if it's mounted, do > the fsck thing and then remount it. If the isn't mounted when it's > called, it will not mount/remount it after doing the fsck. Very > explicit, I don't know the motivation for that behaviour. Ok, so the problem is not as you described - it's not that _check_scratch_fs doesn't unmount the filesystem, it's that it mounts it again after the check and the test requires it to be unmounted *after* it has been checked. See how much time a simple comment can save? :) > >> matter what its filesystem is (btrfs, extN, xfs, etc) > > > > Needs a comment. > > Ok... so should I make a comment to explain what btrfs restore does? > Is it unreasonable to expect an unfamiliar reader to run btrfs --help > or check the man page for example to see what this command is? It is unreasonable to expect them to understand why you used btrfs restore rather than just doing "_scratch_check_fs; md5sum $testfile". That's what the comment needs to explain, because I still don't understand why the test uses btrfs restore or why it would give any different result to the script fragment in the previous sentence... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
