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

Mark Tinguely
2013-06-03T22:08:49Z

On 06/03/13 00:28, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> 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@redhat.com>
> Reviewed-by: Brian Foster<bfoster@redhat.com>
> ---

> @@ -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);

Isn't that physical contiguous allocator? wouldn't a virtual contiguous
be good enough for the acl?

...

> 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)

XFS_ACL_MAX_ENTRIES(mp) == (65536 - 4) / 12 == 5461

> +
> +#define XFS_ACL_SIZE(mp) \
> + (sizeof(struct xfs_acl) + \
> + sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))

XFS_ACL_SIZE(mp) == (4 + 12) + 12 * ((64K - 4) / 12) == 65548

Did you want to add in the sizeof(struct xfs_acl) to the first term or
the sizeof(__be32)? I would think the acl_entry[0] is the start of the
array.

> +
> /* On-disk XFS extended attribute names */
> #define SGI_ACL_FILE (unsigned char *)"SGI_ACL_FILE"
> #define SGI_ACL_DEFAULT (unsigned char *)"SGI_ACL_DEFAULT"

--Mark.