On Fri, 27 Feb 2015, Filipe David Manana wrote: > Date: Fri, 27 Feb 2015 14:34:35 +0000 > From: Filipe David Manana <fdmanana@xxxxxxxxx> > To: Lukáš Czerner <lczerner@xxxxxxxxxx> > Cc: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>, Filipe Manana <fdmanana@xxxxxxxx>, > Eric Sandeen <sandeen@xxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>, > fstests@xxxxxxxxxxxxxxx, linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay > > On Fri, Feb 27, 2015 at 1:10 PM, Lukáš Czerner <lczerner@xxxxxxxxxx> wrote: > > On Fri, 27 Feb 2015, Filipe David Manana wrote: > > > >> Date: Fri, 27 Feb 2015 11:43:36 +0000 > >> From: Filipe David Manana <fdmanana@xxxxxxxxx> > >> To: Lukáš Czerner <lczerner@xxxxxxxxxx> > >> Cc: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>, Filipe Manana <fdmanana@xxxxxxxx>, > >> Eric Sandeen <sandeen@xxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>, > >> fstests@xxxxxxxxxxxxxxx, linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx > >> Subject: Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay > >> > >> On Fri, Feb 27, 2015 at 11:34 AM, Lukáš Czerner <lczerner@xxxxxxxxxx> wrote: > >> > On Thu, 26 Feb 2015, Jaegeuk Kim wrote: > >> > > >> >> Date: Thu, 26 Feb 2015 17:23:47 -0800 > >> >> From: Jaegeuk Kim <jaegeuk@xxxxxxxxxx> > >> >> To: Dave Chinner <david@xxxxxxxxxxxxx> > >> >> Cc: fstests@xxxxxxxxxxxxxxx, linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx, > >> >> Jaegeuk Kim <jaegeuk@xxxxxxxxxx>, Filipe Manana <fdmanana@xxxxxxxx>, > >> >> Eric Sandeen <sandeen@xxxxxxxxxx> > >> >> Subject: [PATCH 2/2] generic/066: add _require_metadata_replay > >> >> > >> >> This patch adds _require_metadata_replay to detect whether or not filesystem > >> >> supports metadata replay. > >> >> > >> >> This should be used when: > >> >> > >> >> 1. create file A > >> >> 2. write file A > >> >> 3. fsync file A > >> >> > >> >> 4. write file A > >> >> > >> >> 5. create file B > >> >> 6. fsync file B > >> >> 7. crash! > >> >> > >> >> In this case, if filesystem supports metadata_replay, file A's data written > >> >> by #4 should be recovered. > >> > > >> > What ? No it should not! file A was never fsync after the last > >> > write, so there is no guarantee that we'll have the new content. > >> > "Metadata replay" suggests that we only replay metadata, not data > >> > and that's what file systems usually do with they journals, cows, > >> > and so on... > >> > > >> > But this test is not about writing to a file, it's about changing > >> > file xattr. And while extended attributes are metadata, they are > >> > _not_ file system metadata and as such I think it can be very well > >> > treated as data. Hence explicit fsync is needed to make sure it's > >> > written to the disk. > >> > > >> > Now let's look at the test itself: > >> > > >> > > >> > echo "hello world!" >> $SCRATCH_MNT/foobar > >> > $XFS_IO_PROG -c "fsync" $SCRATCH_MNT/foobar > >> > $SETFATTR_PROG -x user.attr1 $SCRATCH_MNT/foobar > >> > ln $SCRATCH_MNT/foobar $SCRATCH_MNT/foobar_link > >> > touch $SCRATCH_MNT/qwerty > >> > $XFS_IO_PROG -c "fsync" $SCRATCH_MNT/qwerty > >> > > >> > _crash_and_mount > >> > > >> > # Now only the xattr with name user.attr3 should be set in our file. > >> > echo "xattr names and values after second fsync log replay:" > >> > $GETFATTR_PROG --absolute-names --dump $SCRATCH_MNT/foobar | > >> > _filter_scratch > >> > > >> > No it should not have only attr3 because $SCRATCH_MNT/foobar was > >> > never fsynced after attr3 removal. If the internal design of the > >> > file system accidentally synces unrelated xattrs on unrelated fsync, > >> > than that's fine, but that's not what we need to test for at all > >> > because it might not be guaranteed by all the file systems. So I > >> > think this last part of the test is rather pointless. So I think > >> > this patch is not needed as well. > >> > > >> > Or am I missing something Filipe ? > >> > >> So that second part of the test, essentially comes from this ordered > >> metadata dependency explanation Dave gave me recently on another > >> thread: > >> > >> http://www.spinics.net/lists/linux-btrfs/msg42086.html > > > > It's interesting, but it really applies only to metadata updates > > since really we normally only journal metadata. We do not > > consider extended attributes to be metadata, do we ? > > > >> > >> Yes, I'm considering xattrs as metadata (even though they can be seen > >> as data as well). This behaviour I'm testing for applies to ext3/4 and > >> xfs for example (and apparently intentional, since the test passes on > >> these filesystems). > > > > Ok, I am confused. Clearly ext4, nor xfs consider xattrs metadata > > which can be tested simply by attaching xattr and crashing the file > > system immediately afterwards - the new xattr will not be there - > > that's expected for data, but unexpected for metadata. > > Hum, my testing (on a 3.19 kernel) for both ext4 and xfs shows me > something different. > Perhaps we're testing differently. > > So I just replaced the the line that adds a hard link to our inode with: > > $SETFATTR_PROG -n user.attr4 -v val4 $SCRATCH_MNT/foobar > > Then after the crash + mount, the new xattr is listed (with the > correct value val4). > I also tried the variants of leaving the 'ln' command and adding the > xattr either right before or right after the 'ln' command. For both > cases, the new xattr was there after crash + mount. > > How did you test? Simply created file, fsync, added xattr and crash. But I think I am confusing consistency vs. persistence again :( > > > > > Now the fact that it works might be just a coincidence. Btw in the > > discussion Dave never mentioned xattr, he only talks about inode > > size and extent list changes which makes sense since those are > > metadata and it's expected to be "stabilised" as he very well > > described. I just do not think this applies to this case. > > Correct, I assumed his explanation applied to metadata in general and > I'm considering xattrs as metadata (though as I said before, it's not > unreasonable to consider them as data as well imho). Well that's the thing I am not really sure about. Can not find any documentation about this anywhere at all. -Lukas > > > > > Also I think that his wording that fsync on the file implies fsync > > on the directory is unfortunate because it does not. However it > > implies that the directory will actually be stabilised as well due > > to journalling. But the results are the same. > > > > Anyway, maybe Dave can sprinkle some of his wisdom over this... > > Yes :) > > Thanks > > > > > Thanks! > > -Lukas > > > >> > >> Thanks > >> > >> > > >> > Thanks! > >> > -Lukas > >> > > >> >> Otherwise, file A is recovered to #3. > >> > > >> > > >> >> > >> >> Cc: Filipe Manana <fdmanana@xxxxxxxx> > >> >> Cc: Eric Sandeen <sandeen@xxxxxxxxxx> > >> >> Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx> > >> >> --- > >> >> common/rc | 18 ++++++++++++++++++ > >> >> tests/generic/066 | 2 +- > >> >> 2 files changed, 19 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/common/rc b/common/rc > >> >> index 1ed9df5..e6e8d1f 100644 > >> >> --- a/common/rc > >> >> +++ b/common/rc > >> >> @@ -2372,6 +2372,24 @@ _require_metadata_journaling() > >> >> esac > >> >> } > >> >> > >> >> +# Does this filesystem support metadata replay? > >> >> +# Filesystem is able to recover metadata which were not written by fsync > >> >> +# exlicitly. But another fsync'ed metadata should be followed by them. > >> >> +_require_metadata_replay() > >> >> +{ > >> >> + _require_metadata_journaling $1 > >> >> + > >> >> + case "$FSTYP" in > >> >> + f2fs) > >> >> + # f2fs supports metadata_journaling, but does not recover any > >> >> + # intermediate metadata which was not fsync'ed explicitly. > >> >> + _notrun "$FSTYP does not support metadata replay" > >> >> + ;; > >> >> + *) > >> >> + ;; > >> >> + esac > >> >> +} > >> >> + > >> >> # Does fiemap support? > >> >> _require_fiemap() > >> >> { > >> >> diff --git a/tests/generic/066 b/tests/generic/066 > >> >> index cb36506..3fefda4 100755 > >> >> --- a/tests/generic/066 > >> >> +++ b/tests/generic/066 > >> >> @@ -61,7 +61,7 @@ _need_to_be_root > >> >> _require_scratch > >> >> _require_dm_flakey > >> >> _require_attrs > >> >> -_require_metadata_journaling $SCRATCH_DEV > >> >> +_require_metadata_replay $SCRATCH_DEV > >> >> > >> >> _crash_and_mount() > >> >> { > >> >> > >> > > >> > ------------------------------------------------------------------------------ > >> > Dive into the World of Parallel Programming The Go Parallel Website, sponsored > >> > by Intel and developed in partnership with Slashdot Media, is your hub for all > >> > things parallel software development, from weekly thought leadership blogs to > >> > news, videos, case studies, tutorials and more. Take a look and join the > >> > conversation now. http://goparallel.sourceforge.net/ > >> > _______________________________________________ > >> > Linux-f2fs-devel mailing list > >> > Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx > >> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > >> > >> > >> > >> > > > >
