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

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 03, 2013 at 05:08:38PM -0500, Mark Tinguely wrote:
> 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?

posix_acl_alloc() uses kmalloc, so if it's not failing to allocate
there, then we don't need to use vmalloc here. besides, if we've got
over a single page of acls, then we are talking about 350 ACLs on an
inode. Most admins go insane at more than 20, and the people asking
for more than 25 are using 30-40 ACLs at most. So i don't see there
being a problem here using kzalloc.

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

Ugh, I lost that in translation somewhere. Good catch.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs