generic/065: f2fs serves 64KB size with zero data

PATCH In review Jaegeuk Kim 5 participants 14 messages Feb 27, 2015 – Mar 18, 2015

lore

Share & tools

lore

Jaegeuk Kim

author

2015-02-27 01:23 UTC #0 lore raw

PATCH 1/2 generic/065: f2fs serves 64KB size with zero data

The f2fs provides 64KB size with 0 data after fsync was done to directory file.

Cc Filipe Manana <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 tests/generic/065 | 4 ++++
 1 file changed, 4 insertions(+)

tests/generic/065 +4 -0

index b5a296d..3d2b437 100755
@@ -139,6 +139,10 @@ ext3)
 	# a 64Kb file, with all bytes having the value 0xff
 	[ $hello_digest == "ecb99e6ffea7be1e5419350f725da86b" ] && digest_ok=yes
 	;;
+f2fs)
+	# a 64Kb file, with all bytes having the value 0
+	[ $hello_digest == "fcd6bcb56c1689fcef28b57c22475bad" ] && digest_ok=yes
+	;;
 *)
 	# an empty file
 	[ $hello_digest == "d41d8cd98f00b204e9800998ecf8427e" ] && digest_ok=yes

Signature · 1 line

2.1.1

Jaegeuk Kim

author

2015-02-27 01:23 UTC #1 lore raw

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.
Otherwise, file A is recovered to #3.

Cc Filipe Manana <[email protected]>

Cc Eric Sandeen <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 common/rc         | 18 ++++++++++++++++++
 tests/generic/066 |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)

common/rc +18 -0

index 1ed9df5..e6e8d1f 100644
@@ -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()
 {

tests/generic/066 +1 -1

index cb36506..3fefda4 100755
@@ -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()
 {

Signature · 1 line

2.1.1

Lukáš Czerner

2015-02-27 11:34 UTC #2 lore raw

Re: [PATCH 2/2] generic/066: add _require_metadata_replay

Quoted 17 lines · Jaegeuk Kim

Date: Thu, 26 Feb 2015 17:23:47 -0800
From: Jaegeuk Kim <[email protected]>
To: Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>, Eric Sandeen <[email protected]>
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

Quoted 8 lines

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 ?

Thanks!
-Lukas

Quoted 55 lines

Otherwise, file A is recovered to #3.

Cc Filipe Manana <[email protected]>

Cc Eric Sandeen <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 common/rc         | 18 ++++++++++++++++++
 tests/generic/066 |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)

common/rc +18 -0

index 1ed9df5..e6e8d1f 100644
@@ -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()
 {

tests/generic/066 +1 -1

index cb36506..3fefda4 100755
@@ -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()
 {

Filipe David Manana

2015-02-27 11:43 UTC #3 lore raw

Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay

Quoted 65 lines · Lukáš Czerner

Quoted 17 lines · Jaegeuk Kim
Date: Thu, 26 Feb 2015 17:23:47 -0800
From: Jaegeuk Kim <[email protected]>
To: Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>, Eric Sandeen <[email protected]>
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
Quoted 8 lines
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.

Quoted 1 line

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

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).

Thanks

Quoted 70 lines

Thanks!
-Lukas
Quoted 47 lines
Otherwise, file A is recovered to #3.
Cc Filipe Manana <[email protected]>
Cc Eric Sandeen <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 common/rc         | 18 ++++++++++++++++++
 tests/generic/066 |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)
common/rc +18 -0
index 1ed9df5..e6e8d1f 100644
@@ -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()
 {
tests/generic/066 +0 -0
index cb36506..3fefda4 100755
tests/generic/066 _need_to_be_root · line 67
Quoted 8 lines
tests/generic/066 +1 -1
@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Signature · 5 lines

Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Lukáš Czerner

2015-02-27 13:10 UTC #4 lore raw

Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay

Quoted 74 lines · Filipe David Manana

Date: Fri, 27 Feb 2015 11:43:36 +0000
From: Filipe David Manana <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>,
Eric Sandeen <[email protected]>, Dave Chinner <[email protected]>, [email protected], [email protected]
Subject: Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay
Quoted 65 lines · Lukáš Czerner
Quoted 17 lines · Jaegeuk Kim
Date: Thu, 26 Feb 2015 17:23:47 -0800
From: Jaegeuk Kim <[email protected]>
To: Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>, Eric Sandeen <[email protected]>
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
Quoted 8 lines
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.

Quoted 7 lines

Quoted 1 line
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 ?

Quoted 5 lines

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.

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.

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...

Thanks!
-Lukas

Quoted 77 lines

Thanks
Quoted 70 lines
Thanks!
-Lukas
Quoted 47 lines
Otherwise, file A is recovered to #3.
Cc Filipe Manana <[email protected]>
Cc Eric Sandeen <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 common/rc         | 18 ++++++++++++++++++
 tests/generic/066 |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)
common/rc +18 -0
index 1ed9df5..e6e8d1f 100644
@@ -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()
 {
tests/generic/066 +0 -0
index cb36506..3fefda4 100755
tests/generic/066 _need_to_be_root · line 67
Quoted 8 lines
tests/generic/066 +1 -1
@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Filipe David Manana

2015-02-27 14:34 UTC #5 lore raw

Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay

Quoted 94 lines · Lukáš Czerner

Quoted 74 lines · Filipe David Manana
Date: Fri, 27 Feb 2015 11:43:36 +0000
From: Filipe David Manana <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>,
Eric Sandeen <[email protected]>, Dave Chinner <[email protected]>, [email protected], [email protected]
Subject: Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay
Quoted 65 lines · Lukáš Czerner
Quoted 17 lines · Jaegeuk Kim
Date: Thu, 26 Feb 2015 17:23:47 -0800
From: Jaegeuk Kim <[email protected]>
To: Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>, Eric Sandeen <[email protected]>
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
Quoted 8 lines
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.
Quoted 7 lines
Quoted 1 line
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 ?
Quoted 5 lines
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).

Quoted 4 lines

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?

Quoted 6 lines

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).

Quoted 7 lines

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

Quoted 81 lines

Thanks!
-Lukas
Quoted 77 lines
Thanks
Quoted 70 lines
Thanks!
-Lukas
Quoted 47 lines
Otherwise, file A is recovered to #3.
Cc Filipe Manana <[email protected]>
Cc Eric Sandeen <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 common/rc         | 18 ++++++++++++++++++
 tests/generic/066 |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)
common/rc +18 -0
index 1ed9df5..e6e8d1f 100644
@@ -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()
 {
tests/generic/066 +0 -0
index cb36506..3fefda4 100755
tests/generic/066 _need_to_be_root · line 67
Quoted 8 lines
tests/generic/066 +1 -1
@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Signature · 5 lines

Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Lukáš Czerner

2015-02-27 15:05 UTC #6 lore raw

Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay

Quoted 116 lines · Filipe David Manana

Date: Fri, 27 Feb 2015 14:34:35 +0000
From: Filipe David Manana <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>,
Eric Sandeen <[email protected]>, Dave Chinner <[email protected]>, [email protected], [email protected]
Subject: Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay
Quoted 94 lines · Lukáš Czerner
Quoted 74 lines · Filipe David Manana
Date: Fri, 27 Feb 2015 11:43:36 +0000
From: Filipe David Manana <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>,
Eric Sandeen <[email protected]>, Dave Chinner <[email protected]>, [email protected], [email protected]
Subject: Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay
Quoted 65 lines · Lukáš Czerner
Quoted 17 lines · Jaegeuk Kim
Date: Thu, 26 Feb 2015 17:23:47 -0800
From: Jaegeuk Kim <[email protected]>
To: Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>, Eric Sandeen <[email protected]>
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
Quoted 8 lines
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.
Quoted 7 lines
Quoted 1 line
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 ?
Quoted 5 lines
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).
Quoted 4 lines
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

Quoted 7 lines

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 :(

Quoted 11 lines

Quoted 7 lines
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

Quoted 98 lines

Quoted 8 lines
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
Quoted 81 lines
Thanks!
-Lukas
Quoted 77 lines
Thanks
Quoted 70 lines
Thanks!
-Lukas
Quoted 47 lines
Otherwise, file A is recovered to #3.
Cc Filipe Manana <[email protected]>
Cc Eric Sandeen <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 common/rc         | 18 ++++++++++++++++++
 tests/generic/066 |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)
common/rc +18 -0
index 1ed9df5..e6e8d1f 100644
@@ -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()
 {
tests/generic/066 +0 -0
index cb36506..3fefda4 100755
tests/generic/066 _need_to_be_root · line 67
Quoted 8 lines
tests/generic/066 +1 -1
@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Filipe David Manana

2015-02-27 15:09 UTC #7 lore raw

Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay

Quoted 124 lines · Lukáš Czerner

Quoted 122 lines · Filipe David Manana
Date: Fri, 27 Feb 2015 14:34:35 +0000
From: Filipe David Manana <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>,
Eric Sandeen <[email protected]>, Dave Chinner <[email protected]>, [email protected], [email protected]
Subject: Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay
Quoted 94 lines · Lukáš Czerner
Quoted 74 lines · Filipe David Manana
Date: Fri, 27 Feb 2015 11:43:36 +0000
From: Filipe David Manana <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>,
Eric Sandeen <[email protected]>, Dave Chinner <[email protected]>, [email protected], [email protected]
Subject: Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay
Quoted 65 lines · Lukáš Czerner
Quoted 17 lines · Jaegeuk Kim
Date: Thu, 26 Feb 2015 17:23:47 -0800
From: Jaegeuk Kim <[email protected]>
To: Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>, Eric Sandeen <[email protected]>
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
Quoted 8 lines
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.
Quoted 7 lines
Quoted 1 line
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 ?
Quoted 5 lines
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).
Quoted 4 lines
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.

Quoted 4 lines

Quoted 1 line
How did you test?
Simply created file, fsync, added xattr and crash. But I think I am confusing consistency vs. persistence again :(

Ah in that case it's normal. You need to be sure the journal/log is persisted by fsyncing the other file (named 'qwerty'). For the xattr removal case, if you don't fsync 'qwerty', the xattr 'user.attr1' will still be there for file 'foobar' after crash + mount.

Quoted 116 lines

Quoted 9 lines
Quoted 8 lines
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.
Quoted 3 lines
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
Quoted 98 lines
Quoted 8 lines
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
Quoted 81 lines
Thanks!
-Lukas
Quoted 77 lines
Thanks
Quoted 70 lines
Thanks!
-Lukas
Quoted 47 lines
Otherwise, file A is recovered to #3.
Cc Filipe Manana <[email protected]>
Cc Eric Sandeen <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 common/rc         | 18 ++++++++++++++++++
 tests/generic/066 |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)
common/rc +18 -0
index 1ed9df5..e6e8d1f 100644
@@ -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()
 {
tests/generic/066 +0 -0
index cb36506..3fefda4 100755
tests/generic/066 _need_to_be_root · line 67
Quoted 8 lines
tests/generic/066 +1 -1
@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Signature · 5 lines

Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Dave Chinner

2015-03-18 03:33 UTC #8 lore raw

Re: [f2fs-dev] [PATCH 2/2] generic/066: add _require_metadata_replay

In reply to Lukáš Czerner

Quoted 3 lines · Lukáš Czerner

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 ?

Just to close the circle here, seeing as I don't think this was
answered: XFS considers all xattrs as metadata.

Quoted 9 lines

Quoted 4 lines
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.

It is expected of metadata if there was no fsync.

Quoted 5 lines

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.

xattrs are part of the journalled inode metadata in XFS, just like the size and data extent tree.

Quoted 2 lines

Also I think that his wording that fsync on the file implies fsync on the directory is unfortunate because it does not.

POSIX does not define how file/directory synchronisation should work - it allows fsync() to be a complete no-op, so we are really on our own here. i.e. we define the behaviour ourselves.

Quoted 3 lines

However it
implies that the directory will actually be stabilised as well due to journalling. But the results are the same.

Exactly - what I've described previously is based on the transactional model that ext4, XFS and btrfs use - they all use a strongly ordered atomic transaction model. That is, if we commit transaction N to stable storage, we also commit N-1, N-2, ... and N-m. i.e. we commit everything from the last synchronisation point up to the current sync target.

That gives quite clear dependency rules to fsync. e.g:

create file "X" in dir "Y" (tx N)
write 1 byte to X (tx N+1)
fsync X (force out tx N, N+1)

When fsync completes, we are guaranteeing that the application will be able to find the byte we wrote to X. That also implies that directory Y has a dirent that points to X, and that X has a file size of 1 and and extent that points to the allocated block.

i.e. fsync() implies that all metadata needed to reference the data that has been synced is present on disk. that means "fsync X" also implies "fsync Y" because Y is the only way of finding X. However, if we do this:

create file "X" in dir "Y" (tx N)
write 1 byte to X (tx N+1)
add xattr to Y (tx N+2)
fsync X (force out tx N, N+1)

the fsync of X is not guaranteed to stabilise "xattr Y" because that change occurred *after* the dependency between X and Y was created and is not required to be synced to resolve the dependency between X and Y...

The devil is in the detail, but we really should see XFS, ext4 and btrfs all provide the same fsync behaviour w.r.t. metadata and fsync. Consistency is data integrity behaviour across different filesystems is a good thing. :)

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Jaegeuk Kim

author

2015-02-27 19:02 UTC #9 lore raw

Re: [PATCH 2/2] generic/066: add _require_metadata_replay

In reply to Lukáš Czerner

Hi Lukas,

Quoted 25 lines · Lukáš Czerner

Quoted 23 lines · Jaegeuk Kim
Date: Thu, 26 Feb 2015 17:23:47 -0800
From: Jaegeuk Kim <[email protected]>
To: Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Jaegeuk Kim <[email protected]>, Filipe Manana <[email protected]>, Eric Sandeen <[email protected]>
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!

Quoted 8 lines

Quoted 2 lines
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...

I totally wrote this description insanely.
My apologies.

Yes, what I wanted to point out was the coverage of xattr recovery, not data.
1. create file A
2. write file A
3. fsync file A

*4. setxattr file A

5. create file B
6. fsync file B
7. crash!

What I've focussed on actually was the coverage of metadata replay which depends on filesystem's design.

In this case, I think the metadata log contains:
...
1. fsync'ed file A
2. changed xattrs on file A
3. fsync'ed file B

Currently, xfs and ext4 recover #2, and btrfs without Filipe's patch does not. And, I understood that this testcase intends to check #2 is recoverable or not.

IMHO, however, basically filesystem doesn't need to recover #2 at all. So, I tried to add _require_something to check whether filesystem recovers written xattrs followed by any fsync'ed file coincidentally.

Thanks,

Quoted 93 lines

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 ?
Thanks!
-Lukas
Quoted 55 lines
Otherwise, file A is recovered to #3.
Cc Filipe Manana <[email protected]>
Cc Eric Sandeen <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 common/rc         | 18 ++++++++++++++++++
 tests/generic/066 |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)
common/rc +18 -0
index 1ed9df5..e6e8d1f 100644
@@ -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()
 {
tests/generic/066 +1 -1
index cb36506..3fefda4 100755
@@ -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()
 {

Eric Sandeen

2015-02-27 02:45 UTC #10 lore raw

In reply to Jaegeuk Kim

Quoted 13 lines · Jaegeuk Kim

The f2fs provides 64KB size with 0 data after fsync was done to directory file.
Cc Filipe Manana <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 tests/generic/065 | 4 ++++
 1 file changed, 4 insertions(+)
tests/generic/065 +0 -0
index b5a296d..3d2b437 100755

tests/generic/065 ext3) · line 145

Quoted 7 lines

tests/generic/065 +4 -0

@@ -139,6 +139,10 @@ ext3)
 	# a 64Kb file, with all bytes having the value 0xff
 	[ $hello_digest == "ecb99e6ffea7be1e5419350f725da86b" ] && digest_ok=yes
 	;;
+f2fs)
+	# a 64Kb file, with all bytes having the value 0
+	[ $hello_digest == "fcd6bcb56c1689fcef28b57c22475bad" ] && digest_ok=yes
+	;;

whoa... I will admit to having poorly reviewed this test. Given that this file was never fsynced, I don't think the test should be looking at file contents *at all* I'll do an ex post facto review, I think, and really, I think all of the above should just be removed from the test. Without fsync, we don't know what's in the file. (ext3 could be mounted with writeback mode, for example).

-Eric

Quoted 4 lines

tests/generic/065 +0 -0

 *)
 	# an empty file
 	[ $hello_digest == "d41d8cd98f00b204e9800998ecf8427e" ] && digest_ok=yes

Lukáš Czerner

2015-02-27 09:54 UTC #11 lore raw

Quoted 26 lines · Eric Sandeen

Date: Thu, 26 Feb 2015 20:45:59 -0600
From: Eric Sandeen <[email protected]>
To: Jaegeuk Kim <[email protected]>, Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Filipe Manana <[email protected]>
Subject: Re: [PATCH 1/2] generic/065: f2fs serves 64KB size with zero data
Quoted 18 lines · Jaegeuk Kim
The f2fs provides 64KB size with 0 data after fsync was done to directory file.
Cc Filipe Manana <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 tests/generic/065 | 4 ++++
 1 file changed, 4 insertions(+)
tests/generic/065 +2 -0
index b5a296d..3d2b437 100755
@@ -139,6 +139,10 @@ ext3)
 	# a 64Kb file, with all bytes having the value 0xff
 	[ $hello_digest == "ecb99e6ffea7be1e5419350f725da86b" ] && digest_ok=yes
 	;;
+f2fs)
+	# a 64Kb file, with all bytes having the value 0

Quoted 8 lines

tests/generic/065 ext3) · line 145
Quoted 2 lines
tests/generic/065 +2 -0
+	[ $hello_digest == "fcd6bcb56c1689fcef28b57c22475bad" ] && digest_ok=yes
+	;;
whoa... I will admit to having poorly reviewed this test. Given that this file was never fsynced, I don't think the test should be looking at file contents *at all* I'll do an ex post facto review, I think, and really, I think all of the above should just be removed from the test. Without fsync, we don't know what's in the file. (ext3 could be mounted with writeback mode, for example).

wow :D

Different mdsums for different file systems and

"file 'hello' has expected size and content"

You have to have some fun from time to time ;)

I can not tell whether the content of the file is important, or whether the explicit fsync on the file defeats the purpose of the test, but the easier would be to just fsync the file and then print out md5sum since it should be always the same no matter what file system you use.

Or we can just not test the content of the file at all. But regardless of the decision we do not need this patch.

Felipe what would be the best solution ?

Thanks!

Quoted 13 lines

-Eric
tests/generic/065 ext3) · line 148
Quoted 4 lines
tests/generic/065 +0 -0
 *)
 	# an empty file
 	[ $hello_digest == "d41d8cd98f00b204e9800998ecf8427e" ] && digest_ok=yes
--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Filipe David Manana

2015-02-27 10:31 UTC #12 lore raw

Quoted 37 lines · Lukáš Czerner

Quoted 26 lines · Eric Sandeen
Date: Thu, 26 Feb 2015 20:45:59 -0600
From: Eric Sandeen <[email protected]>
To: Jaegeuk Kim <[email protected]>, Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Filipe Manana <[email protected]>
Subject: Re: [PATCH 1/2] generic/065: f2fs serves 64KB size with zero data
Quoted 18 lines · Jaegeuk Kim
The f2fs provides 64KB size with 0 data after fsync was done to directory file.
Cc Filipe Manana <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 tests/generic/065 | 4 ++++
 1 file changed, 4 insertions(+)
tests/generic/065 +2 -0
index b5a296d..3d2b437 100755
@@ -139,6 +139,10 @@ ext3)
    # a 64Kb file, with all bytes having the value 0xff
    [ $hello_digest == "ecb99e6ffea7be1e5419350f725da86b" ] && digest_ok=yes
    ;;
+f2fs)
+   # a 64Kb file, with all bytes having the value 0
Quoted 8 lines
tests/generic/065 ext3) · line 145
Quoted 2 lines
tests/generic/065 +2 -0
+   [ $hello_digest == "fcd6bcb56c1689fcef28b57c22475bad" ] && digest_ok=yes
+   ;;
whoa... I will admit to having poorly reviewed this test. Given that this file was never fsynced, I don't think the test should be looking at file contents *at all* I'll do an ex post facto review, I think, and really, I think all of the above should just be removed from the test. Without fsync, we don't know what's in the file. (ext3 could be mounted with writeback mode, for example).

Quoted 7 lines

wow :D
Different mdsums for different file systems and
"file 'hello' has expected size and content"
You have to have some fun from time to time ;)

My bad.

Quoted 6 lines

I can not tell whether the content of the file is important, or whether the explicit fsync on the file defeats the purpose of the test, but the easier would be to just fsync the file and then print out md5sum since it should be always the same no matter what file system you use.

I added this check because while doing the btrfs fix I came into a scenario where the file's metadata was inconsistent (i.e. the dir fsync would cause metadata inconsistency for non-empty child files). This was detected by fsck alone, but then I added this check anyway.

Quoted 5 lines

Or we can just not test the content of the file at all. But regardless of the decision we do not need this patch.
Felipe what would be the best solution ?

(Felipe -> Filipe :))

I think explicitly fsyncing the file, after fsyncing the directory, and then check its md5sum/content is a good thing to test. Like this we know there's only one expected result for all filesystems. I'll send a patch for that soon.

thanks

Quoted 27 lines

Thanks!
Quoted 8 lines
-Eric
tests/generic/065 ext3) · line 148
Quoted 4 lines
tests/generic/065 +0 -0
 *)
    # an empty file
    [ $hello_digest == "d41d8cd98f00b204e9800998ecf8427e" ] && digest_ok=yes
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
------------------------------------------------------------------------------
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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Signature · 5 lines

Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Lukáš Czerner

2015-02-27 11:03 UTC #13 lore raw

Quoted 70 lines · Filipe David Manana

Date: Fri, 27 Feb 2015 10:31:02 +0000
From: Filipe David Manana <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Eric Sandeen <[email protected]>, Filipe Manana <[email protected]>,
Jaegeuk Kim <[email protected]>, Dave Chinner <[email protected]>, [email protected], [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/2] generic/065: f2fs serves 64KB size with
zero data
Quoted 37 lines · Lukáš Czerner
Quoted 26 lines · Eric Sandeen
Date: Thu, 26 Feb 2015 20:45:59 -0600
From: Eric Sandeen <[email protected]>
To: Jaegeuk Kim <[email protected]>, Dave Chinner <[email protected]>
Cc: [email protected], [email protected],
Filipe Manana <[email protected]>
Subject: Re: [PATCH 1/2] generic/065: f2fs serves 64KB size with zero data
Quoted 18 lines · Jaegeuk Kim
The f2fs provides 64KB size with 0 data after fsync was done to directory file.
Cc Filipe Manana <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 tests/generic/065 | 4 ++++
 1 file changed, 4 insertions(+)
tests/generic/065 +2 -0
index b5a296d..3d2b437 100755
@@ -139,6 +139,10 @@ ext3)
    # a 64Kb file, with all bytes having the value 0xff
    [ $hello_digest == "ecb99e6ffea7be1e5419350f725da86b" ] && digest_ok=yes
    ;;
+f2fs)
+   # a 64Kb file, with all bytes having the value 0
Quoted 8 lines
tests/generic/065 ext3) · line 145
Quoted 2 lines
tests/generic/065 +2 -0
+   [ $hello_digest == "fcd6bcb56c1689fcef28b57c22475bad" ] && digest_ok=yes
+   ;;
whoa... I will admit to having poorly reviewed this test. Given that this file was never fsynced, I don't think the test should be looking at file contents *at all* I'll do an ex post facto review, I think, and really, I think all of the above should just be removed from the test. Without fsync, we don't know what's in the file. (ext3 could be mounted with writeback mode, for example).
Quoted 7 lines
wow :D
Different mdsums for different file systems and
"file 'hello' has expected size and content"
You have to have some fun from time to time ;)
My bad.
Quoted 6 lines
I can not tell whether the content of the file is important, or whether the explicit fsync on the file defeats the purpose of the test, but the easier would be to just fsync the file and then print out md5sum since it should be always the same no matter what file system you use.
I added this check because while doing the btrfs fix I came into a scenario where the file's metadata was inconsistent (i.e. the dir fsync would cause metadata inconsistency for non-empty child files). This was detected by fsck alone, but then I added this check anyway.
Quoted 1 line

Quoted 6 lines

Quoted 4 lines
Or we can just not test the content of the file at all. But regardless of the decision we do not need this patch.
Felipe what would be the best solution ?
(Felipe -> Filipe :))

Ah sorry about that Filipe :)

Quoted 5 lines

I think explicitly fsyncing the file, after fsyncing the directory, and then check its md5sum/content is a good thing to test. Like this we know there's only one expected result for all filesystems. I'll send a patch for that soon.

Great, thanks!
-Lukas

Quoted 34 lines

thanks
Quoted 27 lines
Thanks!
Quoted 8 lines
-Eric
tests/generic/065 ext3) · line 148
Quoted 4 lines
tests/generic/065 +0 -0
 *)
    # an empty file
    [ $hello_digest == "d41d8cd98f00b204e9800998ecf8427e" ] && digest_ok=yes
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
------------------------------------------------------------------------------
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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel