| Message ID | 0cbb002def872039fd8c0bb90ceb5f6bf0e15b02.1311776403.git.sbehrens@giantdisaster.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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
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
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; }
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(-)