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: Wed, 5 Jun 2013 11:58:17 +1000
Cc: bpm@xxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130604032640.GE29466@dastard>
References: <1370237332-24757-1-git-send-email-david@xxxxxxxxxxxxx> <1370237332-24757-7-git-send-email-david@xxxxxxxxxxxxx> <51AD13E6.2090502@xxxxxxx> <20130604032640.GE29466@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Jun 04, 2013 at 01:26:40PM +1000, Dave Chinner wrote:
> 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>
> > >---
....
> > >  /* 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.

Actually, I didn't lose anything - your calculation of sizeof(struct
xfs_acl) is wrong. I went back to the output from pahole to check
this:

struct xfs_acl {
        __be32                     acl_cnt;              /*     0     4 */
        struct xfs_acl_entry       acl_entry[0];         /*     4     0 */

        /* size: 4, cachelines: 1, members: 2 */
        /* last cacheline: 4 bytes */
};

sizeof(struct xfs_acl) = 4, not 16 as you calculated above. Therefore
code as posted is correct, if not immediately obvious.

The needed fix is to use sizeof(struct xfs_acl) in the
XFS_ACL_MAX_ENTRIES() calcluation rather than sizeof(__be32) so that
the two macros are clearly using the same structures for the
calculations...

-Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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