xfs
[Top] [All Lists]

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

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH 6/6] xfs: increase number of ACL entries for V5 superblocks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 4 Jun 2013 13:26:40 +1000
Cc: xfs@xxxxxxxxxxx, bpm@xxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <51AD13E6.2090502@xxxxxxx>
References: <1370237332-24757-1-git-send-email-david@xxxxxxxxxxxxx> <1370237332-24757-7-git-send-email-david@xxxxxxxxxxxxx> <51AD13E6.2090502@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@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>
> >Reviewed-by: Brian Foster<bfoster@xxxxxxxxxx>
> >---
> 
> >@@ -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@xxxxxxxxxxxxx

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