[PATCH 0/4] xfstests: Add support for btrfs in a couple of xfstests

[PATCH 0/4] xfstests: Add support for btrfs in a couple of xfstests

[Stefan Behrens]

Added btrfs to the list of supported filesystems for tests 015, 062,
079, 083, 117, 120 and 192.
For test 079 to run, src/t_immutable.c was also modified (for ext2 and
btrfs filesystems).

Stefan Behrens (4):
  xfstests: Add support for btrfs in 062
  xfstests: Add support for btrfs in 083, 117, 120 and 192
  xfstests: Add support for btrfs in 015
  xfstests: Add support for btrfs in 079

 015               |   11 ++++++++---
 062               |    4 ++--
 062.out           |    6 ------
 079               |    4 ++--
 083               |   12 +++++++++---
 117               |    4 ++--
 120               |    4 ++--
 192               |    2 +-
 src/t_immutable.c |   23 +++++++++++++++--------
 9 files changed, 41 insertions(+), 29 deletions(-)

-- 
1.7.3.4

[PATCH 1/4] xfstests: Add support for btrfs in 062

[Stefan Behrens]

Added btrfs to the list of supported filesystems for this test.
Remove output of mkfs since this is specific to mkfs.xfs and now filtered
out.

Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
---
 062     |    4 ++--
 062.out |    6 ------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/062 b/062
index 5cb6f92..0f1e98f 100755
--- a/062
+++ b/062
@@ -71,7 +71,7 @@ _create_test_bed()
 }
 
 # real QA test starts here
-_supported_fs xfs udf nfs
+_supported_fs xfs udf nfs btrfs
 _supported_os Linux
 
 _require_scratch
@@ -80,7 +80,7 @@ _require_attrs
 rm -f $tmp.backup1 $tmp.backup2 $seq.full
 
 # real QA test starts here
-_scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs
+_scratch_mkfs > /dev/null || _fail "mkfs failed"
 _scratch_mount || _fail "mount failed"
 _create_test_bed
 
diff --git a/062.out b/062.out
index 73fd628..1d5a094 100644
--- a/062.out
+++ b/062.out
@@ -1,10 +1,4 @@
 QA output created by 062
-meta-data=DDEV isize=XXX agcount=N, agsize=XXX blks
-data     = bsize=XXX blocks=XXX, imaxpct=PCT
-         = sunit=XXX swidth=XXX, unwritten=X
-naming   =VERN bsize=XXX
-log      =LDEV bsize=XXX blocks=XXX
-realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX
 *** create test bed
 SCRATCH_MNT
 SCRATCH_MNT/dev
-- 
1.7.3.4

Re: [PATCH 1/4] xfstests: Add support for btrfs in 062

[Christoph Hellwig]

On Thu, Jul 28, 2011 at 10:27:58AM +0200, Stefan Behrens wrote:
> Added btrfs to the list of supported filesystems for this test.
> Remove output of mkfs since this is specific to mkfs.xfs and now filtered
> out.

Why can't it be generic?  Any reason this one doesn't work on e.g. ext2
or reiserfs?

Re: [PATCH 1/4] xfstests: Add support for btrfs in 062

[Stefan Behrens]

On 7/28/2011 10:45 AM, Christoph Hellwig wrote:
> On Thu, Jul 28, 2011 at 10:27:58AM +0200, Stefan Behrens wrote:
>> Added btrfs to the list of supported filesystems for this test.
>> Remove output of mkfs since this is specific to mkfs.xfs and now filtered
>> out.
> 
> Why can't it be generic?  Any reason this one doesn't work on e.g. ext2
> or reiserfs?

The only reason for not making the tests 015, 062, 083, 117, 120 and
192 "_supported_fs generic" was to keep the effect of the changes
minimal.
But I agree that they are generic ones and I have now changed it
accordingly in the revised patch series that I will send soon.
Thanks for your comments!

[PATCH 2/4] xfstests: Add support for btrfs in 083, 117, 120 and 192

[Stefan Behrens]

Added btrfs to the list of supported filesystems for tests 083, 117,
120 and 192. For test 083 also changed to use _scratch_mkfs_sized
instead of _scratch_mks_xfs for all filesystem types except for XFS.

Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
---
 083 |   12 +++++++++---
 117 |    4 ++--
 120 |    4 ++--
 192 |    2 +-
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/083 b/083
index 7342baa..898d202 100755
--- a/083
+++ b/083
@@ -54,7 +54,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 # real QA test starts here
-_supported_fs xfs
+_supported_fs xfs btrfs
 _supported_os IRIX Linux
 
 _require_scratch
@@ -72,8 +72,14 @@ workout()
 	umount $SCRATCH_DEV >/dev/null 2>&1
 	echo "*** mkfs -dsize=$fsz,agcount=$ags"    >>$seq.full
 	echo ""                                     >>$seq.full
-	_scratch_mkfs_xfs -dsize=$fsz,agcount=$ags  >>$seq.full 2>&1 \
-		|| _fail "size=$fsz,agcount=$ags mkfs failed"
+	if [ $FSTYP = xfs ]
+	then
+		_scratch_mkfs_xfs -dsize=$fsz,agcount=$ags  >>$seq.full 2>&1 \
+			|| _fail "size=$fsz,agcount=$ags mkfs failed"
+	else
+		_scratch_mkfs_sized $fsz >>$seq.full 2>&1 \
+			|| _fail "size=$fsz mkfs failed"
+	fi
 	_scratch_mount                              >>$seq.full 2>&1 \
 		|| _fail "mount failed"
 
diff --git a/117 b/117
index dd880cb..92949d0 100755
--- a/117
+++ b/117
@@ -51,7 +51,7 @@ _cleanup()
 . ./common.attr
 
 # real QA test starts here
-_supported_fs xfs
+_supported_fs xfs btrfs
 _supported_os IRIX Linux
 
 _setup_testdir
@@ -62,7 +62,7 @@ rm -f $seq.full
 umount $SCRATCH_DEV >/dev/null 2>&1
 echo "*** MKFS ***"                         >>$seq.full
 echo ""                                     >>$seq.full
-_scratch_mkfs_xfs                           >>$seq.full 2>&1 \
+_scratch_mkfs                               >>$seq.full 2>&1 \
     || _fail "mkfs failed"
 _scratch_mount                              >>$seq.full 2>&1 \
     || _fail "mount failed"
diff --git a/120 b/120
index bf1e617..f8d3aad 100755
--- a/120
+++ b/120
@@ -37,11 +37,11 @@ trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 # real QA test starts here
-_supported_fs xfs
+_supported_fs xfs btrfs
 _supported_os Linux IRIX
 
 _require_scratch
-_scratch_mkfs_xfs >/dev/null 2>&1
+_scratch_mkfs >/dev/null 2>&1 || _fail "mkfs failed"
 
 _compare_access_times()
 {
diff --git a/192 b/192
index d8301d5..0c488ef 100755
--- a/192
+++ b/192
@@ -45,7 +45,7 @@ _access_time()
 
 # real QA test starts here
 
-_supported_fs xfs udf nfs
+_supported_fs xfs udf nfs btrfs
 _supported_os Linux
 #delay=150
 #delay=75
-- 
1.7.3.4

Re: [PATCH 2/4] xfstests: Add support for btrfs in 083, 117, 120 and 192

[Christoph Hellwig]

Same questions as for the previous one.

[PATCH 3/4] xfstests: Add support for btrfs in 015

[Stefan Behrens]

Added btrfs to the list of supported filesystems for test 015, and
increased free space reporting tolerance to 10% for btrfs.
Replaced the call to _scratch_mkfs_xfs with the XFS specific size
parameter by the generic one for sized filesystem creation which is
_scratch_mkfs_sized.

Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
---
 015 |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/015 b/015
index 8f2be7c..7020e67 100755
--- a/015
+++ b/015
@@ -48,13 +48,13 @@ _free()
 }
 
 # real QA test starts here
-_supported_fs xfs
+_supported_fs xfs btrfs
 _supported_os IRIX Linux
 
 _require_scratch
 _require_nobigloopfs
 
-_scratch_mkfs_xfs -d size=50m >/dev/null || _fail "mkfs failed"
+_scratch_mkfs_sized `expr 50 \* 1024 \* 1024` >/dev/null || _fail "mkfs failed"
 _scratch_mount || _fail "mount failed"
 out=$SCRATCH_MNT/fillup.$$
 rm -f $seq.full
@@ -120,7 +120,12 @@ fi
 echo "free space after delete $free2" >> $seq.full
 
 echo -n "   !!! "
-_within_tolerance "free space" $free2 $free0 1% -v
+if [ $FSTYP = btrfs ]
+then
+	_within_tolerance "free space" $free2 $free0 10% -v
+else
+	_within_tolerance "free space" $free2 $free0 1% -v
+fi
 
 status=0
 exit
-- 
1.7.3.4

Re: [PATCH 3/4] xfstests: Add support for btrfs in 015

[Christoph Hellwig]

On Thu, Jul 28, 2011 at 10:28:00AM +0200, Stefan Behrens wrote:
> Added btrfs to the list of supported filesystems for test 015, and
> increased free space reporting tolerance to 10% for btrfs.
> Replaced the call to _scratch_mkfs_xfs with the XFS specific size
> parameter by the generic one for sized filesystem creation which is
> _scratch_mkfs_sized.

ACK for the _scratch_mkfs_sized changed, but I'm really curious why
we would allow so much more tolerance for btrfs.

And again, why can't these be marked generic?

Re: [PATCH 3/4] xfstests: Add support for btrfs in 015

[Stefan Behrens]

On 7/28/2011 10:47 AM, Christoph Hellwig wrote:
> On Thu, Jul 28, 2011 at 10:28:00AM +0200, Stefan Behrens wrote:
>> Added btrfs to the list of supported filesystems for test 015, and
>> increased free space reporting tolerance to 10% for btrfs.
>> Replaced the call to _scratch_mkfs_xfs with the XFS specific size
>> parameter by the generic one for sized filesystem creation which is
>> _scratch_mkfs_sized.
> 
> ACK for the _scratch_mkfs_sized changed, but I'm really curious why
> we would allow so much more tolerance for btrfs.
> 
> And again, why can't these be marked generic?

To add a 10% tolerance for btrfs was a bad idea.
Since the output of df(1) is not yet reliable on btrfs volumes while
data is not flushed to disk, the better implementation would be to
either let this test fail, or to force a flush to disk before taking
the output of df(1).
The latter is what I have implemented now.

This is a demo of what currently happens:
# mkfs.btrfs -b 2048M --mixed $SCRATCH_DEV
WARNING! - Btrfs v0.19-104-gd0fa1a8-dirty IS EXPERIMENTAL
WARNING! - see http://btrfs.wiki.kernel.org before using

Created a data/metadata chunk of size 8388608
fs created label (null) on /dev/sdu1
        nodesize 4096 leafsize 4096 sectorsize 4096 size 2.00GB
Btrfs v0.19-104-gd0fa1a8-dirty
# mount $SCRATCH_DEV $SCRATCH_MNT
# df -k $SCRATCH_MNT
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/sdu1              2097152        28   2093040   1% /mnt3
# dd if=/dev/zero of=$SCRATCH_MNT/foo bs=1M
dd: writing `/mnt3/foo': No space left on device
1881+0 records in
1880+0 records out
1971322880 bytes (2.0 GB) copied, 7.61947 s, 259 MB/s
# df -k $SCRATCH_MNT
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/sdu1              2097152       756   2092304   1% /mnt3
# sync
# df -k $SCRATCH_MNT
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/sdu1              2097152   1928420    164640  93% /mnt3
# rm -f $SCRATCH_MNT/foo
# df -k $SCRATCH_MNT
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/sdu1              2097152   1928420    164640  93% /mnt3
# sync
# df -k $SCRATCH_MNT
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/sdu1              2097152       676   2092384   1% /mnt3

Re: [PATCH 3/4] xfstests: Add support for btrfs in 015

[Christoph Hellwig]

On Thu, Jul 28, 2011 at 07:54:45PM +0200, Stefan Behrens wrote:
> To add a 10% tolerance for btrfs was a bad idea.
> Since the output of df(1) is not yet reliable on btrfs volumes while
> data is not flushed to disk, the better implementation would be to
> either let this test fail, or to force a flush to disk before taking
> the output of df(1).
> The latter is what I have implemented now.

I don't think it's correct either.  dellalloc blocks should be included
in the statfs output, else it it is pretty pointless.  Can you send the
patch to make it generic without that adjustment for now.

[PATCH 4/4] xfstests: Add support for btrfs in 079

[Stefan Behrens]

Added btrfs to the list of supported filesystems for test 079.
In src/t_immutable.c which is compiled for Linux only, add support for
btrfs by replacing the ioctl(EXT2_IOC_SETFLAGS) with
ioctl(FS_IOC_SETFLAGS) which is defined to be the same.
Afterwards in src/t_immutable.c in function fsetflag(), share the code
branch for the ext2 case also for the btrfs case.
Furthermore, added missing call to ioctl(FS_IOC_GETFLAGS) to the ext3
and btrfs code branch, this was a difference to the way the XFS code
branch was implemented.

Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
---
 079               |    4 ++--
 src/t_immutable.c |   23 +++++++++++++++--------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/079 b/079
index 6c43fe7..02f7607 100755
--- a/079
+++ b/079
@@ -46,7 +46,7 @@ _cleanup()
 . ./common.filter
 . ./common.attr
 
-_supported_fs xfs
+_supported_fs xfs btrfs
 _supported_os Linux
 
 _require_attrs
@@ -55,7 +55,7 @@ _require_scratch
 [ -x $timmutable ] || _notrun "t_immutable was not built for this platform"
 
 # real QA test starts here
-_scratch_mkfs_xfs 2>&1 >/dev/null || _fail "mkfs failed"
+_scratch_mkfs 2>&1 >/dev/null || _fail "mkfs failed"
 _scratch_mount || _fail "mount failed"
 
 echo "*** starting up"
diff --git a/src/t_immutable.c b/src/t_immutable.c
index 7bb3154..9be0c2e 100644
--- a/src/t_immutable.c
+++ b/src/t_immutable.c
@@ -41,6 +41,8 @@
 #include <xfs/xfs.h>
 #include <xfs/handle.h>
 #include <xfs/jdm.h>
+#include <linux/fs.h>
+#include <linux/magic.h>
 
 #define EXT2_SUPER_MAGIC	0xEF53
 #define EXT2_IMMUTABLE_FL       0x00000010
@@ -55,18 +57,18 @@ extern const char *__progname;
 
 static int fsetflag(const char *path, int fd, int on, int immutable)
 {
-     int e2flags = 0;
+     int fsflags = 0;
      struct fsxattr attr;
      struct statfs stfs;
      int xfsfl;
-     int e2fl;
+     int fsfl;
 
      if (immutable) {
 	  xfsfl = XFS_XFLAG_IMMUTABLE;
-	  e2fl = EXT2_IMMUTABLE_FL;
+	  fsfl = FS_IMMUTABLE_FL;
      } else {
 	  xfsfl = XFS_XFLAG_APPEND;
-	  e2fl = EXT2_APPEND_FL;
+	  fsfl = FS_APPEND_FL;
      }
 
      if (fstatfs(fd, &stfs) != 0)
@@ -85,12 +87,17 @@ static int fsetflag(const char *path, int fd, int on, int immutable)
 	       close(fd);
 	       return 1;
 	  }
-     } else if (stfs.f_type == EXT2_SUPER_MAGIC) {
+     } else if (stfs.f_type == EXT2_SUPER_MAGIC ||
+	        stfs.f_type == BTRFS_SUPER_MAGIC) {
+	  if (ioctl(fd, FS_IOC_GETFLAGS, &fsflags) < 0) {
+	       close(fd);
+	       return 1;
+	  }
 	  if (on)
-	       e2flags |= e2fl;
+	       fsflags |= fsfl;
 	  else
-	       e2flags &= ~e2fl;
-	  if (ioctl(fd, EXT2_IOC_SETFLAGS, &e2flags) < 0) {
+	       fsflags &= ~fsfl;
+	  if (ioctl(fd, FS_IOC_SETFLAGS, &fsflags) < 0) {
 	       close(fd);
 	       return 1;
 	  }
-- 
1.7.3.4

Re: [PATCH 4/4] xfstests: Add support for btrfs in 079

[Christoph Hellwig]

On Thu, Jul 28, 2011 at 10:28:01AM +0200, Stefan Behrens wrote:
> Added btrfs to the list of supported filesystems for test 079.
> In src/t_immutable.c which is compiled for Linux only, add support for
> btrfs by replacing the ioctl(EXT2_IOC_SETFLAGS) with
> ioctl(FS_IOC_SETFLAGS) which is defined to be the same.

That has nothing to do with btrfs support.  Your patch means we recent
kernel headers to get the FS_IOC_SETFLAGS instead of having a local one.
I don't care what name to use for the local one, and I also don't
mind an ifdef to pick up a header one in preference, but as-is the patch
isn't too useful as FS_IOC_SETFLAGS is a fairly recent addition to the
kernel headers, and we will break existing working setups.

> Afterwards in src/t_immutable.c in function fsetflag(), share the code
> branch for the ext2 case also for the btrfs case.
> Furthermore, added missing call to ioctl(FS_IOC_GETFLAGS) to the ext3
> and btrfs code branch, this was a difference to the way the XFS code
> branch was implemented.

I'd suggest to completely drop the stat check, and use the ext2 branch
unconditionally.  The ioctl is suppored by all major filesystems.

This also means we can make the test generic, maybe with a _notrun
instead of an error if FS_IOC_GETFLAGS/FS_IOC_SETFLAGS isn't supported.

Re: [PATCH 4/4] xfstests: Add support for btrfs in 079

[Stefan Behrens]

On 7/28/2011 10:51 AM, Christoph Hellwig wrote:
[...]
> I'd suggest to completely drop the stat check, and use the ext2 branch
> unconditionally.  The ioctl is suppored by all major filesystems.
> 
> This also means we can make the test generic, maybe with a _notrun
> instead of an error if FS_IOC_GETFLAGS/FS_IOC_SETFLAGS isn't supported.

I changed it according to your suggestion:
- Get rid of the check for the specific filesystem type.
- Always use FS_IOC_GETFLAGS/FS_IOC_SETFLAGS. This code is inside an
  '#ifdef FS_IOC_SETFLAGS" block in order to never fail compilation.
- Without support for FS_IOC_SETFLAGS, the test completes with _notrun.


What is your opinion about the issue that the test 079 fails on
ext2, ext3, ext4 and btrfs filesystems. Only XFS filesystems succeed
test 079.

mkdir("/mnt3/foo/append-only.d", 0777)  = 0
open("/mnt3/foo/append-only.d", O_RDONLY) = 3
ioctl(3, FS_IOC32_SETFLAGS or FS_IOC_SETFLAGS, 0x7fffaf60b07c) = 0
(this ioctl enables FS_APPEND_FL for the directory)
close(3)
open("/mnt3/foo/append-only.d/newfile-0", O_RDWR|O_CREAT, 0666) = -1
EPERM (Operation not permitted)

One issue is that the file is there (the creation did succeed but
the open for writing did not) what IEEE 1003.1-2004 prohibits
(open() must not create or modify any files if -1 is returned).

The difference between the filesystems is whether the append-only
flag from the directory is inherited to the newly create file inside
that directory. XFS does not inherit that append-only flag, ext2,
ext3, ext4 and btrfs do inherit it.
Test 079 fails when the open("/mnt3/foo/append-only.d/newfile-0",
O_RDWR|O_CREAT, 0666) fails due to the O_RDWR flag. The O_RDWR
flag lets the open() fail when the file has the append-only flag
set. On one type of filesystem the flag is inherited from the
directory, on the other type it is not. Test 079 expects that flag
to not be inherited.

What is your opinion? I would prefer to either change the test to
detect whether the append-only flag is inherited and then interpret
the following system call result depending on the state of the flag,
or to force the flag to a defined state to be independent of the
inheritance behaviour.

Re: [PATCH 4/4] xfstests: Add support for btrfs in 079

[Christoph Hellwig]

On Fri, Jul 29, 2011 at 02:24:30PM +0200, Stefan Behrens wrote:
> I changed it according to your suggestion:
> - Get rid of the check for the specific filesystem type.
> - Always use FS_IOC_GETFLAGS/FS_IOC_SETFLAGS. This code is inside an
>   '#ifdef FS_IOC_SETFLAGS" block in order to never fail compilation.
> - Without support for FS_IOC_SETFLAGS, the test completes with _notrun.

Thanks!

> One issue is that the file is there (the creation did succeed but
> the open for writing did not) what IEEE 1003.1-2004 prohibits
> (open() must not create or modify any files if -1 is returned).

That sounds like something we need to fix, and it seems like we'll
need to fix it in the VFS.  Can you start a thread about that particular
issue on fsdevel?

> The difference between the filesystems is whether the append-only
> flag from the directory is inherited to the newly create file inside
> that directory. XFS does not inherit that append-only flag, ext2,
> ext3, ext4 and btrfs do inherit it.
> Test 079 fails when the open("/mnt3/foo/append-only.d/newfile-0",
> O_RDWR|O_CREAT, 0666) fails due to the O_RDWR flag. The O_RDWR
> flag lets the open() fail when the file has the append-only flag
> set. On one type of filesystem the flag is inherited from the
> directory, on the other type it is not. Test 079 expects that flag
> to not be inherited.
> 
> What is your opinion? I would prefer to either change the test to
> detect whether the append-only flag is inherited and then interpret
> the following system call result depending on the state of the flag,
> or to force the flag to a defined state to be independent of the
> inheritance behaviour.

Having different behaviour for different filesystems is a bad thing,
and given that XFS is the lonely one out there I think we should
remove the inheritance.  I'll preparate a patch for it.

Re: [PATCH 4/4] xfstests: Add support for btrfs in 079

[Stefan Behrens]

On 7/29/2011 2:30 PM, Christoph Hellwig wrote:
> On Fri, Jul 29, 2011 at 02:24:30PM +0200, Stefan Behrens wrote:

>> The difference between the filesystems is whether the append-only
>> flag from the directory is inherited to the newly create file inside
>> that directory. XFS does not inherit that append-only flag, ext2,
>> ext3, ext4 and btrfs do inherit it.

> Having different behaviour for different filesystems is a bad thing,
> and given that XFS is the lonely one out there I think we should
> remove the inheritance.  I'll preparate a patch for it.

In order to make it consistent, it would be needed to _add_ the
inheritance to XFS, not to remove it from XFS. Or to remove it from
ext2, ext3, ext4 and btrfs.

A different thread is whether it makes sense to inherit this flag
from directories to files. I would prefer to not inherit the
append-only flag from a directory to files created in that
directory, because the use case for setting the append-only flag
on directories is different to the use case for having the flag set
on files. I cannot imagine use cases where the inheritance of this
flag from the directory to the file is useful.
But I cannot find real-world use cases for setting this flag on
directories anyway, to all imaginable needs in this area the
solution is the sticky bit on the directory or ACLs.