On 2/23/15 2:24 PM, Filipe David Manana wrote: > On Mon, Feb 23, 2015 at 8:14 PM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote: >> On 2/23/15 1:55 PM, Filipe Manana wrote: >>> This test is motivated by an fsync issue discovered in btrfs. >>> The issue was that the fsync log replay code did not remove xattrs that >>> were deleted before the inode was fsynced. The result was unexpected >>> and differed from xfs and ext3/4 for example. >>> >>> The btrfs issue was fixed by the following linux kernel patch: >>> >>> Btrfs: remove deleted xattrs on fsync log replay >>> >>> Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx> >>> --- >>> tests/generic/061 | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> tests/generic/061.out | 10 +++++ >>> tests/generic/group | 1 + >>> 3 files changed, 126 insertions(+) >>> create mode 100755 tests/generic/061 >>> create mode 100644 tests/generic/061.out >>> >>> diff --git a/tests/generic/061 b/tests/generic/061 >>> new file mode 100755 >>> index 0000000..a5eb668 >>> --- /dev/null >>> +++ b/tests/generic/061 >>> @@ -0,0 +1,115 @@ >>> +#! /bin/bash >>> +# FS QA Test No. 061 >> >> Could you describe what the test actually does first in the header >> comment? You have the btrfs-specific flaw, but (I say this after >> having looked at 034 just this morning) sometimes it's nice to >> have a concise description of the test. >> >> i.e.: >> >> # Test that log replay properly handles deleted xattrs after an fsync >> <or whatever is the proper operational description> >> >> at the very top, so it's clear from a glance what the test *does* >> without having to read through it. > > Hi Eric, > > I thought the initial summary was clear enough: > > # FS QA Test No. 061 > # > # This test is motivated by an fsync issue discovered in btrfs. > # The issue was that the fsync log replay code did not remove xattrs that > # were deleted before the inode was fsynced. > > How more detailed/clear would you put it? Eh, it's ok; it describes why you decided to write the test, not what the test does. Subtle difference? ... >> I think maybe a >> >> _requires_metadata_journaling >> >> or similar might be a good idea. Thoughts? Unfortunately that would >> need some special-casing for ext4, to see if it has jbd2 turned on or >> not, but I can help with that. >> >> (dumpe2fs -h $TEST_DEV | grep has_journal) > > Thanks for the heads up on that detail. > > For the ext4 case, since the test creates the fs, if FSTYP == ext4, > could we force passing -O has_journal to mkfs (or make sure -O > ^has_journal is filtered out). Well, if someone is explicitly testing nojournal ext4, I wouldn't turn it on behind their backs just for this test; that'd be a bit odd. Let me take a stab at a _requires_metadata_journaling() helper. -Eric -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
