| Message ID | 1446563847-14005-11-git-send-email-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Nov 3, 2015, at 8:16 AM, Andreas Gruenbacher <agruenba@redhat.com> wrote: > > POSIX ACLs and richacls are both objects allocated by kmalloc() with a > reference count which are freed by kfree_rcu(). An inode can either > cache an access and a default POSIX ACL, or a richacl (richacls do not > have default acls). To allow an inode to cache either of the two kinds > of acls, introduce a new base_acl type and convert i_acl and > i_default_acl to that type. In most cases, the vfs then doesn't have to > care which kind of acl an inode caches (if any). For new wrapper functions like this better to name them as "NOUN_VERB" so rather than "VERB_NOUN" so that related functions sort together, like base_acl_init(), base_acl_get(), base_acl_put(), base_acl_refcount(), etc. > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > drivers/staging/lustre/lustre/llite/llite_lib.c | 2 +- > fs/f2fs/acl.c | 4 ++-- > fs/inode.c | 4 ++-- > fs/jffs2/acl.c | 6 ++++-- > fs/posix_acl.c | 18 +++++++++--------- > include/linux/fs.h | 25 ++++++++++++++++++++++--- > include/linux/posix_acl.h | 12 ++++-------- > include/linux/richacl.h | 2 +- > 8 files changed, 45 insertions(+), 28 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c > index b4ed6c8..5766f69 100644 > --- a/drivers/staging/lustre/lustre/llite/llite_lib.c > +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c > @@ -1118,7 +1118,7 @@ void ll_clear_inode(struct inode *inode) > } > #ifdef CONFIG_FS_POSIX_ACL > else if (lli->lli_posix_acl) { > - LASSERT(atomic_read(&lli->lli_posix_acl->a_refcount) == 1); > + LASSERT(atomic_read(&lli->lli_posix_acl->a_base.ba_refcount) == 1); It probably makes sense to have a wrapper for this, like base_acl_refcount() so that the details are similarly abstracted from the callers, or there isn't much benefit to having get_base_acl() and put_base_acl() at all. > LASSERT(lli->lli_remote_perms == NULL); > posix_acl_release(lli->lli_posix_acl); > lli->lli_posix_acl = NULL; > diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c > index c8f25f7..a4207de 100644 > --- a/fs/f2fs/acl.c > +++ b/fs/f2fs/acl.c > @@ -270,7 +270,7 @@ static struct posix_acl *f2fs_acl_clone(const struct posix_acl *acl, > sizeof(struct posix_acl_entry); > clone = kmemdup(acl, size, flags); > if (clone) > - atomic_set(&clone->a_refcount, 1); > + atomic_set(&clone->a_base.ba_refcount, 1); This should be base_acl_init() since this should also reset the RCU state if it was just copied from "acl" above. That wouldn't be quite correct if there are other fields added to struct base_acl that don't need to be initialized when it is copied, so possibly base_acl_reinit() would be better here and below if that will be the case in the near future (I haven't looked through the whole patch series yet). > } > return clone; > } > @@ -282,7 +282,7 @@ static int f2fs_acl_create_masq(struct posix_acl *acl, umode_t *mode_p) > umode_t mode = *mode_p; > int not_equiv = 0; > > - /* assert(atomic_read(acl->a_refcount) == 1); */ > + /* assert(atomic_read(acl->a_base.ba_refcount) == 1); */ May as well make the comment use the base_acl_refcount() wrapper function too. > > FOREACH_ACL_ENTRY(pa, acl, pe) { > switch(pa->e_tag) { > diff --git a/fs/inode.c b/fs/inode.c > index 78a17b8..2a387f4 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -233,9 +233,9 @@ void __destroy_inode(struct inode *inode) > > #ifdef CONFIG_FS_POSIX_ACL > if (inode->i_acl && inode->i_acl != ACL_NOT_CACHED) > - posix_acl_release(inode->i_acl); > + put_base_acl(inode->i_acl); > if (inode->i_default_acl && inode->i_default_acl != ACL_NOT_CACHED) > - posix_acl_release(inode->i_default_acl); > + put_base_acl(inode->i_default_acl); > #endif > this_cpu_dec(nr_inodes); > } > diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c > index 2f7a3c0..04a5836 100644 > --- a/fs/jffs2/acl.c > +++ b/fs/jffs2/acl.c > @@ -294,13 +294,15 @@ int jffs2_init_acl_post(struct inode *inode) > int rc; > > if (inode->i_default_acl) { > - rc = __jffs2_set_acl(inode, JFFS2_XPREFIX_ACL_DEFAULT, inode->i_default_acl); > + rc = __jffs2_set_acl(inode, JFFS2_XPREFIX_ACL_DEFAULT, > + *acl_by_type(inode, ACL_TYPE_DEFAULT)); > if (rc) > return rc; > } > > if (inode->i_acl) { > - rc = __jffs2_set_acl(inode, JFFS2_XPREFIX_ACL_ACCESS, inode->i_acl); > + rc = __jffs2_set_acl(inode, JFFS2_XPREFIX_ACL_ACCESS, > + *acl_by_type(inode, ACL_TYPE_ACCESS)); > if (rc) > return rc; > } > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index 4fb17de..b3b2265 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -25,9 +25,9 @@ struct posix_acl **acl_by_type(struct inode *inode, int type) > { > switch (type) { > case ACL_TYPE_ACCESS: > - return &inode->i_acl; > + return (struct posix_acl **)&inode->i_acl; > case ACL_TYPE_DEFAULT: > - return &inode->i_default_acl; > + return (struct posix_acl **)&inode->i_default_acl; This would be better to use container_of() to unwrap struct base_acl from struct posix_acl. That avoids the hard requirement (which isn't documented anywhere) that base_acl needs to be the first member of struct posix_acl. I was originally going to write that you should add a comment that base_acl needs to be the first member of both richacl and posix_acl, but container_of() is both cleaner and safer. Looking further down, that IS actually needed due to the way kfree is used on the base_acl pointer, but using container_of() is still cleaner and safer than directly casting double pointers (which some compilers and static analysis tools will be unhappy with). > default: > BUG(); > } > @@ -83,16 +83,16 @@ EXPORT_SYMBOL(forget_cached_acl); > > void forget_all_cached_acls(struct inode *inode) > { > - struct posix_acl *old_access, *old_default; > + struct base_acl *old_access, *old_default; > spin_lock(&inode->i_lock); > old_access = inode->i_acl; > old_default = inode->i_default_acl; > inode->i_acl = inode->i_default_acl = ACL_NOT_CACHED; > spin_unlock(&inode->i_lock); > if (old_access != ACL_NOT_CACHED) > - posix_acl_release(old_access); > + put_base_acl(old_access); > if (old_default != ACL_NOT_CACHED) > - posix_acl_release(old_default); > + put_base_acl(old_default); > } > EXPORT_SYMBOL(forget_all_cached_acls); > > @@ -129,7 +129,7 @@ EXPORT_SYMBOL(get_acl); > void > posix_acl_init(struct posix_acl *acl, int count) > { > - atomic_set(&acl->a_refcount, 1); > + atomic_set(&acl->a_base.ba_refcount, 1); This should be base_acl_init(). > acl->a_count = count; > } > EXPORT_SYMBOL(posix_acl_init); > @@ -162,7 +162,7 @@ posix_acl_clone(const struct posix_acl *acl, gfp_t flags) > sizeof(struct posix_acl_entry); > clone = kmemdup(acl, size, flags); > if (clone) > - atomic_set(&clone->a_refcount, 1); > + atomic_set(&clone->a_base.ba_refcount, 1); Also base_acl_init() or base_acl_reinit(). > } > return clone; > } > @@ -384,7 +384,7 @@ static int posix_acl_create_masq(struct posix_acl *acl, umode_t *mode_p) > umode_t mode = *mode_p; > int not_equiv = 0; > > - /* assert(atomic_read(acl->a_refcount) == 1); */ > + /* assert(atomic_read(acl->a_base.ba_refcount) == 1); */ base_acl_refcount(). > FOREACH_ACL_ENTRY(pa, acl, pe) { > switch(pa->e_tag) { > @@ -439,7 +439,7 @@ static int __posix_acl_chmod_masq(struct posix_acl *acl, umode_t mode) > struct posix_acl_entry *group_obj = NULL, *mask_obj = NULL; > struct posix_acl_entry *pa, *pe; > > - /* assert(atomic_read(acl->a_refcount) == 1); */ > + /* assert(atomic_read(acl->a_base.ba_refcount) == 1); */ ... > FOREACH_ACL_ENTRY(pa, acl, pe) { > switch(pa->e_tag) { > diff --git a/include/linux/fs.h b/include/linux/fs.h > index ba91a89..3c22c92 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -576,6 +576,12 @@ static inline void mapping_allow_writable(struct address_space *mapping) > #define i_size_ordered_init(inode) do { } while (0) > #endif > > +struct base_acl { > + union { > + atomic_t ba_refcount; > + struct rcu_head ba_rcu; > + }; > +}; > struct posix_acl; Is this forward declaration of struct posix_acl even needed anymore after the change below? There shouldn't be references to the struct in the common code anymore (at least not by the end of the patch series. > #define ACL_NOT_CACHED ((void *)(-1)) > > @@ -595,9 +601,9 @@ struct inode { > kgid_t i_gid; > unsigned int i_flags; > > -#ifdef CONFIG_FS_POSIX_ACL > - struct posix_acl *i_acl; > - struct posix_acl *i_default_acl; > +#if defined(CONFIG_FS_POSIX_ACL) > + struct base_acl *i_acl; > + struct base_acl *i_default_acl; > #endif > > const struct inode_operations *i_op; > @@ -3059,4 +3065,17 @@ static inline bool dir_relax(struct inode *inode) > > extern bool path_noexec(const struct path *path); > > +static inline struct base_acl *get_base_acl(struct base_acl *acl) > +{ > + if (acl) > + atomic_inc(&acl->ba_refcount); > + return acl; > +} > + > +static inline void put_base_acl(struct base_acl *acl) > +{ > + if (acl && atomic_dec_and_test(&acl->ba_refcount)) > + kfree_rcu(acl, ba_rcu); Hmm, using the base_acl pointer as the pointer to kfree means that the base_acl structure DOES need to be the first one in both struct posix_acl and struct richacl, so that needs to be commented at each structure so it doesn't accidentally break in the future. > +} > + > #endif /* _LINUX_FS_H */ > diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h > index 3e96a6a..2c46441 100644 > --- a/include/linux/posix_acl.h > +++ b/include/linux/posix_acl.h > @@ -43,10 +43,7 @@ struct posix_acl_entry { > }; > > struct posix_acl { > - union { > - atomic_t a_refcount; > - struct rcu_head a_rcu; > - }; > + struct base_acl a_base; struct base_acl a_base; /* must be first, see base_acl_put() */ > unsigned int a_count; > struct posix_acl_entry a_entries[0]; > }; > @@ -61,8 +58,7 @@ struct posix_acl { > static inline struct posix_acl * > posix_acl_dup(struct posix_acl *acl) > { > - if (acl) > - atomic_inc(&acl->a_refcount); > + get_base_acl(&acl->a_base); > return acl; > } > > @@ -72,8 +68,8 @@ posix_acl_dup(struct posix_acl *acl) > static inline void > posix_acl_release(struct posix_acl *acl) > { > - if (acl && atomic_dec_and_test(&acl->a_refcount)) > - kfree_rcu(acl, a_rcu); > + BUILD_BUG_ON(offsetof(struct posix_acl, a_base) != 0); > + put_base_acl(&acl->a_base); > } > > > diff --git a/include/linux/richacl.h b/include/linux/richacl.h > index 1d9f5f7..2baef35 100644 > --- a/include/linux/richacl.h > +++ b/include/linux/richacl.h > @@ -57,7 +57,7 @@ static inline struct richacl * > richacl_get(struct richacl *acl) > { > if (acl) > - atomic_inc(&acl->a_refcount); > + atomic_inc(&acl->a_base.ba_refcount); > return acl; This should also use base_acl_get() for consistency. That said, where is the call to base_acl_put() in the richacl code? Also, where is the change to struct richacl? It looks like this patch would not be able to compile by itself. I was going to say that struct richacl also needs a comment like struct posix_acl that struct base_acl needs to be first. Cheers, Andreas
Andreas, On Tue, Nov 3, 2015 at 11:29 PM, Andreas Dilger <adilger@dilger.ca> wrote: > On Nov 3, 2015, at 8:16 AM, Andreas Gruenbacher <agruenba@redhat.com> wrote: >> >> POSIX ACLs and richacls are both objects allocated by kmalloc() with a >> reference count which are freed by kfree_rcu(). An inode can either >> cache an access and a default POSIX ACL, or a richacl (richacls do not >> have default acls). To allow an inode to cache either of the two kinds >> of acls, introduce a new base_acl type and convert i_acl and >> i_default_acl to that type. In most cases, the vfs then doesn't have to >> care which kind of acl an inode caches (if any). > > For new wrapper functions like this better to name them as "NOUN_VERB" so > rather than "VERB_NOUN" so that related functions sort together, like > base_acl_init(), base_acl_get(), base_acl_put(), base_acl_refcount(), etc. That's better, yes. I agree with all your comments and I've changed things accordingly. >> @@ -270,7 +270,7 @@ static struct posix_acl *f2fs_acl_clone(const struct posix_acl *acl, >> sizeof(struct posix_acl_entry); >> clone = kmemdup(acl, size, flags); >> if (clone) >> - atomic_set(&clone->a_refcount, 1); >> + atomic_set(&clone->a_base.ba_refcount, 1); > > This should be base_acl_init() since this should also reset the RCU state > if it was just copied from "acl" above. Yes. The rcu_head doesn't need initializing or resetting though. > That wouldn't be quite correct if > there are other fields added to struct base_acl that don't need to be > initialized when it is copied, so possibly base_acl_reinit() would be better > here and below if that will be the case in the near future (I haven't looked > through the whole patch series yet). We won't need a base_acl_reinit() function for now. >> @@ -25,9 +25,9 @@ struct posix_acl **acl_by_type(struct inode *inode, int type) >> { >> switch (type) { >> case ACL_TYPE_ACCESS: >> - return &inode->i_acl; >> + return (struct posix_acl **)&inode->i_acl; >> case ACL_TYPE_DEFAULT: >> - return &inode->i_default_acl; >> + return (struct posix_acl **)&inode->i_default_acl; > > This would be better to use container_of() to unwrap struct base_acl from > struct posix_acl. That avoids the hard requirement (which isn't documented > anywhere) that base_acl needs to be the first member of struct posix_acl. > > I was originally going to write that you should add a comment that base_acl > needs to be the first member of both richacl and posix_acl, but container_of() > is both cleaner and safer. > > Looking further down, that IS actually needed due to the way kfree is used on > the base_acl pointer, but using container_of() is still cleaner and safer > than directly casting double pointers (which some compilers and static > analysis tools will be unhappy with). Well, we would end up with &container_of() here which doesn't work and doesn't make sense, either. Let me change acl_by_type to return a base_acl ** to clean this up. >> @@ -576,6 +576,12 @@ static inline void mapping_allow_writable(struct address_space *mapping) >> #define i_size_ordered_init(inode) do { } while (0) >> #endif >> >> +struct base_acl { >> + union { >> + atomic_t ba_refcount; >> + struct rcu_head ba_rcu; >> + }; >> +}; >> struct posix_acl; > > Is this forward declaration of struct posix_acl even needed anymore after > the change below? There shouldn't be references to the struct in the common > code anymore (at least not by the end of the patch series. The get_acl and set_acl inode operations expect struct posix_acl to be declared. > Hmm, using the base_acl pointer as the pointer to kfree means that the > base_acl structure DOES need to be the first one in both struct posix_acl > and struct richacl, so that needs to be commented at each structure so > it doesn't accidentally break in the future. Yes. I've added comments; there are also BUILD_BUG_ON() asserts in posix_acl_release and richacl_put. >> @@ -57,7 +57,7 @@ static inline struct richacl * >> richacl_get(struct richacl *acl) >> { >> if (acl) >> - atomic_inc(&acl->a_refcount); >> + atomic_inc(&acl->a_base.ba_refcount); >> return acl; > > This should also use base_acl_get() for consistency. That said, where is > the call to base_acl_put() in the richacl code? > Also, where is the change to struct richacl? It looks like this patch would > not be able to compile by itself. Ah, a little problem in how the patches are split. I've fixed it. This code doesn't get pulled into the build because nothing requires CONFIG_FS_RICHACL at that point; that's why I didn't notice. Thanks, Andreas -- 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
diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c index b4ed6c8..5766f69 100644 --- a/drivers/staging/lustre/lustre/llite/llite_lib.c +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c @@ -1118,7 +1118,7 @@ void ll_clear_inode(struct inode *inode) } #ifdef CONFIG_FS_POSIX_ACL else if (lli->lli_posix_acl) { - LASSERT(atomic_read(&lli->lli_posix_acl->a_refcount) == 1); + LASSERT(atomic_read(&lli->lli_posix_acl->a_base.ba_refcount) == 1); LASSERT(lli->lli_remote_perms == NULL); posix_acl_release(lli->lli_posix_acl); lli->lli_posix_acl = NULL; diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c index c8f25f7..a4207de 100644 --- a/fs/f2fs/acl.c +++ b/fs/f2fs/acl.c @@ -270,7 +270,7 @@ static struct posix_acl *f2fs_acl_clone(const struct posix_acl *acl, sizeof(struct posix_acl_entry); clone = kmemdup(acl, size, flags); if (clone) - atomic_set(&clone->a_refcount, 1); + atomic_set(&clone->a_base.ba_refcount, 1); } return clone; } @@ -282,7 +282,7 @@ static int f2fs_acl_create_masq(struct posix_acl *acl, umode_t *mode_p) umode_t mode = *mode_p; int not_equiv = 0; - /* assert(atomic_read(acl->a_refcount) == 1); */ + /* assert(atomic_read(acl->a_base.ba_refcount) == 1); */ FOREACH_ACL_ENTRY(pa, acl, pe) { switch(pa->e_tag) { diff --git a/fs/inode.c b/fs/inode.c index 78a17b8..2a387f4 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -233,9 +233,9 @@ void __destroy_inode(struct inode *inode) #ifdef CONFIG_FS_POSIX_ACL if (inode->i_acl && inode->i_acl != ACL_NOT_CACHED) - posix_acl_release(inode->i_acl); + put_base_acl(inode->i_acl); if (inode->i_default_acl && inode->i_default_acl != ACL_NOT_CACHED) - posix_acl_release(inode->i_default_acl); + put_base_acl(inode->i_default_acl); #endif this_cpu_dec(nr_inodes); } diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c index 2f7a3c0..04a5836 100644 --- a/fs/jffs2/acl.c +++ b/fs/jffs2/acl.c @@ -294,13 +294,15 @@ int jffs2_init_acl_post(struct inode *inode) int rc; if (inode->i_default_acl) { - rc = __jffs2_set_acl(inode, JFFS2_XPREFIX_ACL_DEFAULT, inode->i_default_acl); + rc = __jffs2_set_acl(inode, JFFS2_XPREFIX_ACL_DEFAULT, + *acl_by_type(inode, ACL_TYPE_DEFAULT)); if (rc) return rc; } if (inode->i_acl) { - rc = __jffs2_set_acl(inode, JFFS2_XPREFIX_ACL_ACCESS, inode->i_acl); + rc = __jffs2_set_acl(inode, JFFS2_XPREFIX_ACL_ACCESS, + *acl_by_type(inode, ACL_TYPE_ACCESS)); if (rc) return rc; } diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 4fb17de..b3b2265 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -25,9 +25,9 @@ struct posix_acl **acl_by_type(struct inode *inode, int type) { switch (type) { case ACL_TYPE_ACCESS: - return &inode->i_acl; + return (struct posix_acl **)&inode->i_acl; case ACL_TYPE_DEFAULT: - return &inode->i_default_acl; + return (struct posix_acl **)&inode->i_default_acl; default: BUG(); } @@ -83,16 +83,16 @@ EXPORT_SYMBOL(forget_cached_acl); void forget_all_cached_acls(struct inode *inode) { - struct posix_acl *old_access, *old_default; + struct base_acl *old_access, *old_default; spin_lock(&inode->i_lock); old_access = inode->i_acl; old_default = inode->i_default_acl; inode->i_acl = inode->i_default_acl = ACL_NOT_CACHED; spin_unlock(&inode->i_lock); if (old_access != ACL_NOT_CACHED) - posix_acl_release(old_access); + put_base_acl(old_access); if (old_default != ACL_NOT_CACHED) - posix_acl_release(old_default); + put_base_acl(old_default); } EXPORT_SYMBOL(forget_all_cached_acls); @@ -129,7 +129,7 @@ EXPORT_SYMBOL(get_acl); void posix_acl_init(struct posix_acl *acl, int count) { - atomic_set(&acl->a_refcount, 1); + atomic_set(&acl->a_base.ba_refcount, 1); acl->a_count = count; } EXPORT_SYMBOL(posix_acl_init); @@ -162,7 +162,7 @@ posix_acl_clone(const struct posix_acl *acl, gfp_t flags) sizeof(struct posix_acl_entry); clone = kmemdup(acl, size, flags); if (clone) - atomic_set(&clone->a_refcount, 1); + atomic_set(&clone->a_base.ba_refcount, 1); } return clone; } @@ -384,7 +384,7 @@ static int posix_acl_create_masq(struct posix_acl *acl, umode_t *mode_p) umode_t mode = *mode_p; int not_equiv = 0; - /* assert(atomic_read(acl->a_refcount) == 1); */ + /* assert(atomic_read(acl->a_base.ba_refcount) == 1); */ FOREACH_ACL_ENTRY(pa, acl, pe) { switch(pa->e_tag) { @@ -439,7 +439,7 @@ static int __posix_acl_chmod_masq(struct posix_acl *acl, umode_t mode) struct posix_acl_entry *group_obj = NULL, *mask_obj = NULL; struct posix_acl_entry *pa, *pe; - /* assert(atomic_read(acl->a_refcount) == 1); */ + /* assert(atomic_read(acl->a_base.ba_refcount) == 1); */ FOREACH_ACL_ENTRY(pa, acl, pe) { switch(pa->e_tag) { diff --git a/include/linux/fs.h b/include/linux/fs.h index ba91a89..3c22c92 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -576,6 +576,12 @@ static inline void mapping_allow_writable(struct address_space *mapping) #define i_size_ordered_init(inode) do { } while (0) #endif +struct base_acl { + union { + atomic_t ba_refcount; + struct rcu_head ba_rcu; + }; +}; struct posix_acl; #define ACL_NOT_CACHED ((void *)(-1)) @@ -595,9 +601,9 @@ struct inode { kgid_t i_gid; unsigned int i_flags; -#ifdef CONFIG_FS_POSIX_ACL - struct posix_acl *i_acl; - struct posix_acl *i_default_acl; +#if defined(CONFIG_FS_POSIX_ACL) + struct base_acl *i_acl; + struct base_acl *i_default_acl; #endif const struct inode_operations *i_op; @@ -3059,4 +3065,17 @@ static inline bool dir_relax(struct inode *inode) extern bool path_noexec(const struct path *path); +static inline struct base_acl *get_base_acl(struct base_acl *acl) +{ + if (acl) + atomic_inc(&acl->ba_refcount); + return acl; +} + +static inline void put_base_acl(struct base_acl *acl) +{ + if (acl && atomic_dec_and_test(&acl->ba_refcount)) + kfree_rcu(acl, ba_rcu); +} + #endif /* _LINUX_FS_H */ diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h index 3e96a6a..2c46441 100644 --- a/include/linux/posix_acl.h +++ b/include/linux/posix_acl.h @@ -43,10 +43,7 @@ struct posix_acl_entry { }; struct posix_acl { - union { - atomic_t a_refcount; - struct rcu_head a_rcu; - }; + struct base_acl a_base; unsigned int a_count; struct posix_acl_entry a_entries[0]; }; @@ -61,8 +58,7 @@ struct posix_acl { static inline struct posix_acl * posix_acl_dup(struct posix_acl *acl) { - if (acl) - atomic_inc(&acl->a_refcount); + get_base_acl(&acl->a_base); return acl; } @@ -72,8 +68,8 @@ posix_acl_dup(struct posix_acl *acl) static inline void posix_acl_release(struct posix_acl *acl) { - if (acl && atomic_dec_and_test(&acl->a_refcount)) - kfree_rcu(acl, a_rcu); + BUILD_BUG_ON(offsetof(struct posix_acl, a_base) != 0); + put_base_acl(&acl->a_base); } diff --git a/include/linux/richacl.h b/include/linux/richacl.h index 1d9f5f7..2baef35 100644 --- a/include/linux/richacl.h +++ b/include/linux/richacl.h @@ -57,7 +57,7 @@ static inline struct richacl * richacl_get(struct richacl *acl) { if (acl) - atomic_inc(&acl->a_refcount); + atomic_inc(&acl->a_base.ba_refcount); return acl; }
POSIX ACLs and richacls are both objects allocated by kmalloc() with a reference count which are freed by kfree_rcu(). An inode can either cache an access and a default POSIX ACL, or a richacl (richacls do not have default acls). To allow an inode to cache either of the two kinds of acls, introduce a new base_acl type and convert i_acl and i_default_acl to that type. In most cases, the vfs then doesn't have to care which kind of acl an inode caches (if any). Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- drivers/staging/lustre/lustre/llite/llite_lib.c | 2 +- fs/f2fs/acl.c | 4 ++-- fs/inode.c | 4 ++-- fs/jffs2/acl.c | 6 ++++-- fs/posix_acl.c | 18 +++++++++--------- include/linux/fs.h | 25 ++++++++++++++++++++++--- include/linux/posix_acl.h | 12 ++++-------- include/linux/richacl.h | 2 +- 8 files changed, 45 insertions(+), 28 deletions(-)