xfs
[Top] [All Lists]

Re: [PATCH 3/9] xfs: increase number of ACL entries for V5 superblocks

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/9] xfs: increase number of ACL entries for V5 superblocks
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 29 May 2013 12:40:50 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1369636707-15150-4-git-send-email-david@xxxxxxxxxxxxx>
References: <1369636707-15150-1-git-send-email-david@xxxxxxxxxxxxx> <1369636707-15150-4-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6
On 05/27/2013 02:38 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The limit of 25 ACL entries is arbitrary, but baked into the on-disk
> format.  For version 5 superblocks, increase it to the maximum nuber
> of ACLs that can fit into a single xattr.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_acl.c |   31 ++++++++++++++++++-------------
>  fs/xfs/xfs_acl.h |   30 +++++++++++++++++++++++-------
>  2 files changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 1d32f1d..b6bed9b 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -21,6 +21,8 @@
>  #include "xfs_bmap_btree.h"
>  #include "xfs_inode.h"
>  #include "xfs_vnodeops.h"
> +#include "xfs_sb.h"
> +#include "xfs_mount.h"
>  #include "xfs_trace.h"
>  #include <linux/slab.h>
>  #include <linux/xattr.h>
> @@ -34,7 +36,9 @@
>   */
>  
>  STATIC struct posix_acl *
> -xfs_acl_from_disk(struct xfs_acl *aclp)
> +xfs_acl_from_disk(
> +     struct xfs_acl  *aclp,
> +     int             max_entries)
>  {
>       struct posix_acl_entry *acl_e;
>       struct posix_acl *acl;
> @@ -42,7 +46,7 @@ xfs_acl_from_disk(struct xfs_acl *aclp)
>       unsigned int count, i;
>  
>       count = be32_to_cpu(aclp->acl_cnt);
> -     if (count > XFS_ACL_MAX_ENTRIES)
> +     if (count > max_entries)
>               return ERR_PTR(-EFSCORRUPTED);
>  
>       acl = posix_acl_alloc(count, GFP_KERNEL);
> @@ -108,7 +112,7 @@ xfs_get_acl(struct inode *inode, int type)
>       struct xfs_inode *ip = XFS_I(inode);
>       struct posix_acl *acl;
>       struct xfs_acl *xfs_acl;
> -     int len = sizeof(struct xfs_acl);
> +     int len = XFS_ACL_SIZE(ip->i_mount);
>       unsigned char *ea_name;
>       int error;
>  
> @@ -133,8 +137,8 @@ xfs_get_acl(struct inode *inode, int type)
>        * If we have a cached ACLs value just return it, not need to
>        * go out to the disk.
>        */
> -
> -     xfs_acl = kzalloc(sizeof(struct xfs_acl), GFP_KERNEL);
> +     len = XFS_ACL_SIZE(ip->i_mount);

Duplicate initialization of len. Harmless (perhaps Ben can drop it) and
the parens in XFS_ACL_MAX_ENTRIES() are fixed, so:

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

> +     xfs_acl = kzalloc(len, GFP_KERNEL);
>       if (!xfs_acl)
>               return ERR_PTR(-ENOMEM);
>  
> @@ -153,7 +157,7 @@ xfs_get_acl(struct inode *inode, int type)
>               goto out;
>       }
>  
> -     acl = xfs_acl_from_disk(xfs_acl);
> +     acl = xfs_acl_from_disk(xfs_acl, XFS_ACL_MAX_ENTRIES(ip->i_mount));
>       if (IS_ERR(acl))
>               goto out;
>  
> @@ -189,16 +193,17 @@ xfs_set_acl(struct inode *inode, int type, struct 
> posix_acl *acl)
>  
>       if (acl) {
>               struct xfs_acl *xfs_acl;
> -             int len;
> +             int len = XFS_ACL_SIZE(ip->i_mount);
>  
> -             xfs_acl = kzalloc(sizeof(struct xfs_acl), GFP_KERNEL);
> +             xfs_acl = kzalloc(len, GFP_KERNEL);
>               if (!xfs_acl)
>                       return -ENOMEM;
>  
>               xfs_acl_to_disk(xfs_acl, acl);
> -             len = sizeof(struct xfs_acl) -
> -                     (sizeof(struct xfs_acl_entry) *
> -                      (XFS_ACL_MAX_ENTRIES - acl->a_count));
> +
> +             /* subtract away the unused acl entries */
> +             len -= sizeof(struct xfs_acl_entry) *
> +                      (XFS_ACL_MAX_ENTRIES(ip->i_mount) - acl->a_count);
>  
>               error = -xfs_attr_set(ip, ea_name, (unsigned char *)xfs_acl,
>                               len, ATTR_ROOT);
> @@ -243,7 +248,7 @@ xfs_set_mode(struct inode *inode, umode_t mode)
>  static int
>  xfs_acl_exists(struct inode *inode, unsigned char *name)
>  {
> -     int len = sizeof(struct xfs_acl);
> +     int len = XFS_ACL_SIZE(XFS_M(inode->i_sb));
>  
>       return (xfs_attr_get(XFS_I(inode), name, NULL, &len,
>                           ATTR_ROOT|ATTR_KERNOVAL) == 0);
> @@ -379,7 +384,7 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
>               goto out_release;
>  
>       error = -EINVAL;
> -     if (acl->a_count > XFS_ACL_MAX_ENTRIES)
> +     if (acl->a_count > XFS_ACL_MAX_ENTRIES(XFS_M(inode->i_sb)))
>               goto out_release;
>  
>       if (type == ACL_TYPE_ACCESS) {
> diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
> index 39632d9..0da8725 100644
> --- a/fs/xfs/xfs_acl.h
> +++ b/fs/xfs/xfs_acl.h
> @@ -22,19 +22,35 @@ struct inode;
>  struct posix_acl;
>  struct xfs_inode;
>  
> -#define XFS_ACL_MAX_ENTRIES 25
>  #define XFS_ACL_NOT_PRESENT (-1)
>  
>  /* On-disk XFS access control list structure */
> +struct xfs_acl_entry {
> +     __be32  ae_tag;
> +     __be32  ae_id;
> +     __be16  ae_perm;
> +     __be16  ae_pad;         /* fill the implicit hole in the structure */
> +};
> +
>  struct xfs_acl {
> -     __be32          acl_cnt;
> -     struct xfs_acl_entry {
> -             __be32  ae_tag;
> -             __be32  ae_id;
> -             __be16  ae_perm;
> -     } acl_entry[XFS_ACL_MAX_ENTRIES];
> +     __be32                  acl_cnt;
> +     struct xfs_acl_entry    acl_entry[0];
>  };
>  
> +/*
> + * The number of ACL entries allowed is defined by the on-disk format.
> + * For v4 superblocks, that is limited to 25 entries. For v5 superblocks, it 
> is
> + * limited only by the maximum size of the xattr that stores the information.
> + */
> +#define XFS_ACL_MAX_ENTRIES(mp)      \
> +     (xfs_sb_version_hascrc(&mp->m_sb) \
> +        ?  (XATTR_SIZE_MAX - sizeof(__be32)) / sizeof(struct xfs_acl_entry) \
> +        : 25)
> +
> +#define XFS_ACL_SIZE(mp) \
> +     (sizeof(struct xfs_acl) + \
> +             sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))
> +
>  /* On-disk XFS extended attribute names */
>  #define SGI_ACL_FILE         (unsigned char *)"SGI_ACL_FILE"
>  #define SGI_ACL_DEFAULT              (unsigned char *)"SGI_ACL_DEFAULT"
> 

<Prev in Thread] Current Thread [Next in Thread>