[-V1,22/22] ext4: Add Ext4 compat richacl feature flag
diff mbox

Message ID 1398615293-22931-23-git-send-email-aneesh.kumar@linux.vnet.ibm.com
State New, archived
Headers show

Commit Message

Aneesh Kumar K.V April 27, 2014, 4:14 p.m. UTC
This feature flag can be used to enable richacl on
the file system. Once enabled the "acl" mount option
will enable richacl instead of posix acl

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 fs/ext4/ext4.h  |  3 ++-
 fs/ext4/super.c | 47 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 40 insertions(+), 10 deletions(-)

Comments

Andreas Dilger April 28, 2014, 9:31 p.m. UTC | #1
On Apr 27, 2014, at 10:14 AM, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
> This feature flag can be used to enable richacl on
> the file system. Once enabled the "acl" mount option
> will enable richacl instead of posix acl

I was going to complain about this patch, because re-using the "acl"
mount option to specify richacl instead of POSIX ACL would be very
confusing, since older kernels used the "acl" mount option to enable
POSIX ACLs.

Looking closer, I see that "acl" and "noacl" just means enable or disable
the ACL functionality on the filesystem.  Please fix up the commit comment.

Some more comments inline.

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 6f9e6fadac04..2a0221652d79 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1274,6 +1274,30 @@ static ext4_fsblk_t get_sb_block(void **data)
> 	return sb_block;
> }
> 
> +static void enable_acl(struct super_block *sb)
> +{
> +#if !defined(CONFIG_EXT4_FS_POSIX_ACL) && !defined(CONFIG_EXT4_FS_RICHACL)
> +	return;
> +#endif
> +	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
> +		sb->s_flags |= MS_RICHACL;
> +		sb->s_flags &= ~MS_POSIXACL;
> +	} else {
> +		sb->s_flags |= MS_POSIXACL;
> +		sb->s_flags &= ~MS_RICHACL;
> +	}

This should put the #ifdef around the code that is being enabled/disabled,
otherwise it just becomes dead code:

static int enable_acl(struct super_block *sb)
{
	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
#if defined(CONFIG_EXT4_FS_RICHACL)
		sb->s_flags |= MS_RICHACL;
		sb->s_flags &= ~MS_POSIXACL;
#else
		return -EOPNOTSUPP;
#endif
	} else {
#if defined(CONFIG_EXT4_FS_POSIX_ACL)
		sb->s_flags |= MS_POSIXACL;
		sb->s_flags &= ~MS_RICHACL;
#else
		return -EOPNOTSUPP;
#endif
	}
	return 0;
}

> +
> +static void disable_acl(struct super_block *sb)
> +{
> +#if !defined(CONFIG_EXT4_FS_POSIX_ACL) && !defined(CONFIG_EXT4_FS_RICHACL)
> +	return;
> +#endif
> +	sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL);
> +	return;
> +}

"return" is not needed at the end of void functions. Same comment on #ifdef:

static void disable_acl(struct super_block *sb)
{
#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
	sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL);
#endif
}


> +
> #define DEFAULT_JOURNAL_IOPRIO (IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 3))
> static char deprecated_msg[] = "Mount option \"%s\" will be removed by %s\n"
> 	"Contact linux-ext4@vger.kernel.org if you think we should keep it.\n";
> @@ -1417,9 +1441,9 @@ static const struct mount_opts {
> 	 MOPT_NO_EXT2 | MOPT_DATAJ},
> 	{Opt_user_xattr, EXT4_MOUNT_XATTR_USER, MOPT_SET},
> 	{Opt_nouser_xattr, EXT4_MOUNT_XATTR_USER, MOPT_CLEAR},
> -#ifdef CONFIG_EXT4_FS_POSIX_ACL
> -	{Opt_acl, EXT4_MOUNT_POSIX_ACL, MOPT_SET},
> -	{Opt_noacl, EXT4_MOUNT_POSIX_ACL, MOPT_CLEAR},
> +#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
> +	{Opt_acl, EXT4_MOUNT_ACL, MOPT_SET},
> +	{Opt_noacl, EXT4_MOUNT_ACL, MOPT_CLEAR},
> #else
> 	{Opt_acl, 0, MOPT_NOSUPPORT},
> 	{Opt_noacl, 0, MOPT_NOSUPPORT},
> @@ -3496,8 +3520,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 		set_opt(sb, NO_UID32);
> 	/* xattr user namespace & acls are now defaulted on */
> 	set_opt(sb, XATTR_USER);
> -#ifdef CONFIG_EXT4_FS_POSIX_ACL
> -	set_opt(sb, POSIX_ACL);
> +#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
> +	set_opt(sb, ACL);
> #endif
> 	if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_DATA)
> 		set_opt(sb, JOURNAL_DATA);
> @@ -3569,8 +3593,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 			clear_opt(sb, DELALLOC);
> 	}
> 
> -	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
> -		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
> +	/*
> +	 * clear ACL flags
> +	 */
> +	disable_acl(sb);

Is there any expectation that the flags would be set on a newly mounted
filesystem?

> +	if (test_opt(sb, ACL))
> +		enable_acl(sb);
> 
> 	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
> 	    (EXT4_HAS_COMPAT_FEATURE(sb, ~0U) ||
> @@ -4844,8 +4872,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> 	if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED)
> 		ext4_abort(sb, "Abort forced by user");
> 
> -	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
> -		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
> +	disable_acl(sb);
> +	if (test_opt(sb, ACL))
> +		enable_acl(sb);

Similarly, it seems racy to me to disable ACL support and then re-enable
it here during remount, since that might cause some concurrent operations
to fail.  It seems like enable_acl() already handles clearing the flags
correctly, so something like the following would be better:

	if (test_opt(sb, ACL))
		enable_acl(sb);
	else
		disable_acl(sb);


Cheers, Andreas
Aneesh Kumar K.V May 1, 2014, 3:48 p.m. UTC | #2
Andreas Dilger <adilger@dilger.ca> writes:

> On Apr 27, 2014, at 10:14 AM, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>> This feature flag can be used to enable richacl on
>> the file system. Once enabled the "acl" mount option
>> will enable richacl instead of posix acl
>
> I was going to complain about this patch, because re-using the "acl"
> mount option to specify richacl instead of POSIX ACL would be very
> confusing, since older kernels used the "acl" mount option to enable
> POSIX ACLs.
>
> Looking closer, I see that "acl" and "noacl" just means enable or disable
> the ACL functionality on the filesystem.  Please fix up the commit
> comment.

Will clarify in the commit message.

>
> Some more comments inline.
>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 6f9e6fadac04..2a0221652d79 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1274,6 +1274,30 @@ static ext4_fsblk_t get_sb_block(void **data)
>> 	return sb_block;
>> }
>> 
>> +static void enable_acl(struct super_block *sb)
>> +{
>> +#if !defined(CONFIG_EXT4_FS_POSIX_ACL) && !defined(CONFIG_EXT4_FS_RICHACL)
>> +	return;
>> +#endif
>> +	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
>> +		sb->s_flags |= MS_RICHACL;
>> +		sb->s_flags &= ~MS_POSIXACL;
>> +	} else {
>> +		sb->s_flags |= MS_POSIXACL;
>> +		sb->s_flags &= ~MS_RICHACL;
>> +	}
>
> This should put the #ifdef around the code that is being enabled/disabled,
> otherwise it just becomes dead code:
>
> static int enable_acl(struct super_block *sb)
> {
> 	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
> #if defined(CONFIG_EXT4_FS_RICHACL)
> 		sb->s_flags |= MS_RICHACL;
> 		sb->s_flags &= ~MS_POSIXACL;
> #else
> 		return -EOPNOTSUPP;
> #endif
> 	} else {
> #if defined(CONFIG_EXT4_FS_POSIX_ACL)
> 		sb->s_flags |= MS_POSIXACL;
> 		sb->s_flags &= ~MS_RICHACL;
> #else
> 		return -EOPNOTSUPP;
> #endif
> 	}
> 	return 0;
> }

That is too much #ifdef with no real benefit ?

>
>> +
>> +static void disable_acl(struct super_block *sb)
>> +{
>> +#if !defined(CONFIG_EXT4_FS_POSIX_ACL) && !defined(CONFIG_EXT4_FS_RICHACL)
>> +	return;
>> +#endif
>> +	sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL);
>> +	return;
>> +}
>
> "return" is not needed at the end of void functions. Same comment on
> #ifdef:

ok

>
> static void disable_acl(struct super_block *sb)
> {
> #if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
> 	sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL);
> #endif
> }
>
>
>> +
>> #define DEFAULT_JOURNAL_IOPRIO (IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 3))
>> static char deprecated_msg[] = "Mount option \"%s\" will be removed by %s\n"
>> 	"Contact linux-ext4@vger.kernel.org if you think we should keep it.\n";
>> @@ -1417,9 +1441,9 @@ static const struct mount_opts {
>> 	 MOPT_NO_EXT2 | MOPT_DATAJ},
>> 	{Opt_user_xattr, EXT4_MOUNT_XATTR_USER, MOPT_SET},
>> 	{Opt_nouser_xattr, EXT4_MOUNT_XATTR_USER, MOPT_CLEAR},

....

>> 	if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_DATA)
>> 		set_opt(sb, JOURNAL_DATA);
>> @@ -3569,8 +3593,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>> 			clear_opt(sb, DELALLOC);
>> 	}
>> 
>> -	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
>> -		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
>> +	/*
>> +	 * clear ACL flags
>> +	 */
>> +	disable_acl(sb);
>
> Is there any expectation that the flags would be set on a newly mounted
> filesystem?
>
>> +	if (test_opt(sb, ACL))
>> +		enable_acl(sb);
>> 
>> 	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
>> 	    (EXT4_HAS_COMPAT_FEATURE(sb, ~0U) ||
>> @@ -4844,8 +4872,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>> 	if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED)
>> 		ext4_abort(sb, "Abort forced by user");
>> 
>> -	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
>> -		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
>> +	disable_acl(sb);
>> +	if (test_opt(sb, ACL))
>> +		enable_acl(sb);
>
> Similarly, it seems racy to me to disable ACL support and then re-enable
> it here during remount, since that might cause some concurrent operations
> to fail.  It seems like enable_acl() already handles clearing the flags
> correctly, so something like the following would be better:
>
> 	if (test_opt(sb, ACL))
> 		enable_acl(sb);
> 	else
> 		disable_acl(sb);
>
>

ok

-aneesh

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger May 1, 2014, 5:52 p.m. UTC | #3
On May 1, 2014, at 9:48 AM, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:

> Andreas Dilger <adilger@dilger.ca> writes:
> 
>> On Apr 27, 2014, at 10:14 AM, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>> This feature flag can be used to enable richacl on
>>> the file system. Once enabled the "acl" mount option
>>> will enable richacl instead of posix acl
>> 
>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>> index 6f9e6fadac04..2a0221652d79 100644
>>> --- a/fs/ext4/super.c
>>> +++ b/fs/ext4/super.c
>>> @@ -1274,6 +1274,30 @@ static ext4_fsblk_t get_sb_block(void **data)
>>> 	return sb_block;
>>> }
>>> 
>>> +static void enable_acl(struct super_block *sb)
>>> +{
>>> +#if !defined(CONFIG_EXT4_FS_POSIX_ACL) && !defined(CONFIG_EXT4_FS_RICHACL)
>>> +	return;
>>> +#endif
>>> +	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
>>> +		sb->s_flags |= MS_RICHACL;
>>> +		sb->s_flags &= ~MS_POSIXACL;
>>> +	} else {
>>> +		sb->s_flags |= MS_POSIXACL;
>>> +		sb->s_flags &= ~MS_RICHACL;
>>> +	}
>> 
>> This should put the #ifdef around the code that is being enabled/disabled,
>> otherwise it just becomes dead code:
>> 
>> static int enable_acl(struct super_block *sb)
>> {
>> 	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
>> #if defined(CONFIG_EXT4_FS_RICHACL)
>> 		sb->s_flags |= MS_RICHACL;
>> 		sb->s_flags &= ~MS_POSIXACL;
>> #else
>> 		return -EOPNOTSUPP;
>> #endif
>> 	} else {
>> #if defined(CONFIG_EXT4_FS_POSIX_ACL)
>> 		sb->s_flags |= MS_POSIXACL;
>> 		sb->s_flags &= ~MS_RICHACL;
>> #else
>> 		return -EOPNOTSUPP;
>> #endif
>> 	}
>> 	return 0;
>> }
> 
> That is too much #ifdef with no real benefit ?

The benefit is that if neither CONFIG_EXT4_FS_RICHACL nor CONFIG_EXT4_FS_POSIX_ACL are defined there isn't unreachable code
after "return" at the start of the function.  Some static code
analysis tools will complain about this.

Cheers, Andreas

Patch
diff mbox

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 66946aa62127..17ff4a1cf91e 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -976,7 +976,7 @@  struct ext4_inode_info {
 #define EXT4_MOUNT_UPDATE_JOURNAL	0x01000	/* Update the journal format */
 #define EXT4_MOUNT_NO_UID32		0x02000  /* Disable 32-bit UIDs */
 #define EXT4_MOUNT_XATTR_USER		0x04000	/* Extended user attributes */
-#define EXT4_MOUNT_POSIX_ACL		0x08000	/* POSIX Access Control Lists */
+#define EXT4_MOUNT_ACL			0x08000	/* Access Control Lists */
 #define EXT4_MOUNT_NO_AUTO_DA_ALLOC	0x10000	/* No auto delalloc mapping */
 #define EXT4_MOUNT_BARRIER		0x20000 /* Use block barriers */
 #define EXT4_MOUNT_QUOTA		0x80000 /* Some quota option set */
@@ -1505,6 +1505,7 @@  static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
 #define EXT4_FEATURE_COMPAT_EXT_ATTR		0x0008
 #define EXT4_FEATURE_COMPAT_RESIZE_INODE	0x0010
 #define EXT4_FEATURE_COMPAT_DIR_INDEX		0x0020
+#define EXT4_FEATURE_COMPAT_RICHACL		0x0200
 
 #define EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER	0x0001
 #define EXT4_FEATURE_RO_COMPAT_LARGE_FILE	0x0002
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6f9e6fadac04..2a0221652d79 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1274,6 +1274,30 @@  static ext4_fsblk_t get_sb_block(void **data)
 	return sb_block;
 }
 
+static void enable_acl(struct super_block *sb)
+{
+#if !defined(CONFIG_EXT4_FS_POSIX_ACL) && !defined(CONFIG_EXT4_FS_RICHACL)
+	return;
+#endif
+	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
+		sb->s_flags |= MS_RICHACL;
+		sb->s_flags &= ~MS_POSIXACL;
+	} else {
+		sb->s_flags |= MS_POSIXACL;
+		sb->s_flags &= ~MS_RICHACL;
+	}
+	return;
+}
+
+static void disable_acl(struct super_block *sb)
+{
+#if !defined(CONFIG_EXT4_FS_POSIX_ACL) && !defined(CONFIG_EXT4_FS_RICHACL)
+	return;
+#endif
+	sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL);
+	return;
+}
+
 #define DEFAULT_JOURNAL_IOPRIO (IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 3))
 static char deprecated_msg[] = "Mount option \"%s\" will be removed by %s\n"
 	"Contact linux-ext4@vger.kernel.org if you think we should keep it.\n";
@@ -1417,9 +1441,9 @@  static const struct mount_opts {
 	 MOPT_NO_EXT2 | MOPT_DATAJ},
 	{Opt_user_xattr, EXT4_MOUNT_XATTR_USER, MOPT_SET},
 	{Opt_nouser_xattr, EXT4_MOUNT_XATTR_USER, MOPT_CLEAR},
-#ifdef CONFIG_EXT4_FS_POSIX_ACL
-	{Opt_acl, EXT4_MOUNT_POSIX_ACL, MOPT_SET},
-	{Opt_noacl, EXT4_MOUNT_POSIX_ACL, MOPT_CLEAR},
+#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
+	{Opt_acl, EXT4_MOUNT_ACL, MOPT_SET},
+	{Opt_noacl, EXT4_MOUNT_ACL, MOPT_CLEAR},
 #else
 	{Opt_acl, 0, MOPT_NOSUPPORT},
 	{Opt_noacl, 0, MOPT_NOSUPPORT},
@@ -3496,8 +3520,8 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		set_opt(sb, NO_UID32);
 	/* xattr user namespace & acls are now defaulted on */
 	set_opt(sb, XATTR_USER);
-#ifdef CONFIG_EXT4_FS_POSIX_ACL
-	set_opt(sb, POSIX_ACL);
+#if defined(CONFIG_EXT4_FS_POSIX_ACL) || defined(CONFIG_EXT4_FS_RICHACL)
+	set_opt(sb, ACL);
 #endif
 	if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_DATA)
 		set_opt(sb, JOURNAL_DATA);
@@ -3569,8 +3593,12 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 			clear_opt(sb, DELALLOC);
 	}
 
-	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
-		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
+	/*
+	 * clear ACL flags
+	 */
+	disable_acl(sb);
+	if (test_opt(sb, ACL))
+		enable_acl(sb);
 
 	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
 	    (EXT4_HAS_COMPAT_FEATURE(sb, ~0U) ||
@@ -4844,8 +4872,9 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED)
 		ext4_abort(sb, "Abort forced by user");
 
-	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
-		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
+	disable_acl(sb);
+	if (test_opt(sb, ACL))
+		enable_acl(sb);
 
 	es = sbi->s_es;