diff mbox

[v13,10/51] vfs: Cache base_acl objects in inodes

Message ID 1446563847-14005-11-git-send-email-agruenba@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andreas Gruenbacher Nov. 3, 2015, 3:16 p.m. UTC
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(-)

Comments

Andreas Dilger Nov. 3, 2015, 10:29 p.m. UTC | #1
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 Gruenbacher Nov. 4, 2015, 9:54 p.m. UTC | #2
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-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 }