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

Message ID 0cbb002def872039fd8c0bb90ceb5f6bf0e15b02.1311776403.git.sbehrens@giantdisaster.de
State Not Applicable
Headers show

Commit Message

Stefan Behrens July 28, 2011, 8:28 a.m. UTC
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(-)

Comments

Christoph Hellwig July 28, 2011, 8:51 a.m. UTC | #1
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Behrens July 29, 2011, 12:24 p.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig July 29, 2011, 12:30 p.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Behrens July 29, 2011, 2:30 p.m. UTC | #4
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;
 	  }