| Message ID | 1398615293-22931-23-git-send-email-aneesh.kumar@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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;
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(-)