On Apr 27, 2014, at 10:14 AM, Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx 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
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
