xfs: fixes for 3.10-rc4

From: Dave Chinner <[email protected]>

Calculating dquot CRCs when the backing buffer is written back just doesn't work reliably. There are several places which manipulate dquots directly in the buffers, and they don't calculate CRCs appropriately, nor do they always set the buffer up to calculate CRCs appropriately.

Firstly, if we log a dquot buffer (e.g. during allocation) it gets logged without valid CRC, and so on recovery we end up with a dquot that is not valid.

Secondly, if we recover/repair a dquot, we don't have a verifier attached to the buffer and hence CRCs arenot calculate don the way down to disk.

Thirdly, calculating the CRC after we've changed the contents means that if we re-read the dquot from the buffer, we cannot verify the contents of the dquot are valid, as the CRC is invalid.

So, to avoid all the dquot CRC errors that are being detected by the read verifier, change to using the same model as for inodes. that is, dquot CRCs are calculated and written to the backing buffer at the time the dquot is flushed to the backing buffer. If we modify the dquuot directly in the backing buffer, calculate the CRC immediately after the modification is complete. Hence the dquot in the on-disk buffer should always have a valid CRC.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_dquot.c       |   37 ++++++++++++++++---------------------
 fs/xfs/xfs_log_recover.c |   10 ++++++++++
 fs/xfs/xfs_qm.c          |   40 ++++++++++++++++++++++++++++++----------
 fs/xfs/xfs_quota.h       |    2 ++
 4 files changed, 58 insertions(+), 31 deletions(-)

fs/xfs/xfs_dquot.c +16 -21

index a41f8bf..044e97a 100644
@@ -249,8 +249,11 @@ xfs_qm_init_dquot_blk(
 		d->dd_diskdq.d_version = XFS_DQUOT_VERSION;
 		d->dd_diskdq.d_id = cpu_to_be32(curid);
 		d->dd_diskdq.d_flags = type;
-		if (xfs_sb_version_hascrc(&mp->m_sb))
+		if (xfs_sb_version_hascrc(&mp->m_sb)) {
 			uuid_copy(&d->dd_uuid, &mp->m_sb.sb_uuid);
+			xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
+					 XFS_DQUOT_CRC_OFF);
+		}
 	}
 
 	xfs_trans_dquot_buf(tp, bp,

@@ -286,23 +289,6 @@ xfs_dquot_set_prealloc_limits(struct xfs_dquot *dqp)
 	dqp->q_low_space[XFS_QLOWSP_5_PCNT] = space * 5;
 }
 
-STATIC void
-xfs_dquot_buf_calc_crc(
-	struct xfs_mount	*mp,
-	struct xfs_buf		*bp)
-{
-	struct xfs_dqblk	*d = (struct xfs_dqblk *)bp->b_addr;
-	int			i;
-
-	if (!xfs_sb_version_hascrc(&mp->m_sb))
-		return;
-
-	for (i = 0; i < mp->m_quotainfo->qi_dqperchunk; i++, d++) {
-		xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
-				 offsetof(struct xfs_dqblk, dd_crc));
-	}
-}
-
 STATIC bool
 xfs_dquot_buf_verify_crc(
 	struct xfs_mount	*mp,

@@ -328,12 +314,11 @@ xfs_dquot_buf_verify_crc(
 
 	for (i = 0; i < ndquots; i++, d++) {
 		if (!xfs_verify_cksum((char *)d, sizeof(struct xfs_dqblk),
-				 offsetof(struct xfs_dqblk, dd_crc)))
+				 XFS_DQUOT_CRC_OFF))
 			return false;
 		if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_uuid))
 			return false;
 	}
-
 	return true;
 }
 

@@ -393,6 +378,11 @@ xfs_dquot_buf_read_verify(
 	}
 }
 
+/*
+ * we don't calculate the CRC here as that is done when the dquot is flushed to
+ * the buffer after the update is done. This ensures that the dquot in the
+ * buffer always has an up-to-date CRC value.
+ */
 void
 xfs_dquot_buf_write_verify(
 	struct xfs_buf	*bp)

@@ -404,7 +394,6 @@ xfs_dquot_buf_write_verify(
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
 		return;
 	}
-	xfs_dquot_buf_calc_crc(mp, bp);
 }
 
 const struct xfs_buf_ops xfs_dquot_buf_ops = {

@@ -1151,11 +1140,17 @@ xfs_qm_dqflush(
 	 * copy the lsn into the on-disk dquot now while we have the in memory
 	 * dquot here. This can't be done later in the write verifier as we
 	 * can't get access to the log item at that point in time.
+	 *
+	 * We also calculate the CRC here so that the on-disk dquot in the
+	 * buffer always has a valid CRC. This ensures there is no possibility
+	 * of a dquot without an up-to-date CRC getting to disk.
 	 */
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_dqblk *dqb = (struct xfs_dqblk *)ddqp;
 
 		dqb->dd_lsn = cpu_to_be64(dqp->q_logitem.qli_item.li_lsn);
+		xfs_update_cksum((char *)dqb, sizeof(struct xfs_dqblk),
+				 XFS_DQUOT_CRC_OFF);
 	}
 
 	/*

fs/xfs/xfs_log_recover.c +10 -0

index d9e4d3c..d6204d1 100644
@@ -2266,6 +2266,12 @@ xfs_qm_dqcheck(
 	d->dd_diskdq.d_flags = type;
 	d->dd_diskdq.d_id = cpu_to_be32(id);
 
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		uuid_copy(&d->dd_uuid, &mp->m_sb.sb_uuid);
+		xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
+				 XFS_DQUOT_CRC_OFF);
+	}
+
 	return errs;
 }
 

@@ -2793,6 +2799,10 @@ xlog_recover_dquot_pass2(
 	}
 
 	memcpy(ddq, recddq, item->ri_buf[1].i_len);
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		xfs_update_cksum((char *)ddq, sizeof(struct xfs_dqblk),
+				 XFS_DQUOT_CRC_OFF);
+	}
 
 	ASSERT(dq_f->qlf_size == 2);
 	ASSERT(bp->b_target->bt_mount == mp);

fs/xfs/xfs_qm.c +30 -10

index f41702b..b75c9bb 100644
@@ -41,6 +41,7 @@
 #include "xfs_qm.h"
 #include "xfs_trace.h"
 #include "xfs_icache.h"
+#include "xfs_cksum.h"
 
 /*
  * The global quota manager. There is only one of these for the entire

@@ -839,7 +840,7 @@ xfs_qm_reset_dqcounts(
 	xfs_dqid_t	id,
 	uint		type)
 {
-	xfs_disk_dquot_t	*ddq;
+	struct xfs_dqblk	*dqb;
 	int			j;
 
 	trace_xfs_reset_dqcounts(bp, _RET_IP_);

@@ -853,8 +854,12 @@ xfs_qm_reset_dqcounts(
 	do_div(j, sizeof(xfs_dqblk_t));
 	ASSERT(mp->m_quotainfo->qi_dqperchunk == j);
 #endif
-	ddq = bp->b_addr;
+	dqb = bp->b_addr;
 	for (j = 0; j < mp->m_quotainfo->qi_dqperchunk; j++) {
+		struct xfs_disk_dquot	*ddq;
+
+		ddq = (struct xfs_disk_dquot *)&dqb[j];
+
 		/*
 		 * Do a sanity check, and if needed, repair the dqblk. Don't
 		 * output any warnings because it's perfectly possible to

@@ -871,7 +876,12 @@ xfs_qm_reset_dqcounts(
 		ddq->d_bwarns = 0;
 		ddq->d_iwarns = 0;
 		ddq->d_rtbwarns = 0;
-		ddq = (xfs_disk_dquot_t *) ((xfs_dqblk_t *)ddq + 1);
+
+		if (xfs_sb_version_hascrc(&mp->m_sb)) {
+			xfs_update_cksum((char *)&dqb[j],
+					 sizeof(struct xfs_dqblk),
+					 XFS_DQUOT_CRC_OFF);
+		}
 	}
 }
 

@@ -907,19 +917,29 @@ xfs_qm_dqiter_bufs(
 			      XFS_FSB_TO_DADDR(mp, bno),
 			      mp->m_quotainfo->qi_dqchunklen, 0, &bp,
 			      &xfs_dquot_buf_ops);
-		if (error)
-			break;
 
 		/*
-		 * XXX(hch): need to figure out if it makes sense to validate
-		 *	     the CRC here.
+		 * CRC and validation errors will return a EFSCORRUPTED here. If
+		 * this occurs, re-read without CRC validation so that we can
+		 * repair the damage via xfs_qm_reset_dqcounts(). This process
+		 * will leave a trace in the log indicating corruption has
+		 * been detected.
 		 */
+		if (error == EFSCORRUPTED) {
+			error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
+				      XFS_FSB_TO_DADDR(mp, bno),
+				      mp->m_quotainfo->qi_dqchunklen, 0, &bp,
+				      NULL);
+		}
+
+		if (error)
+			break;
+
 		xfs_qm_reset_dqcounts(mp, bp, firstid, type);
 		xfs_buf_delwri_queue(bp, buffer_list);
 		xfs_buf_relse(bp);
-		/*
-		 * goto the next block.
-		 */
+
+		/* goto the next block. */
 		bno++;
 		firstid += mp->m_quotainfo->qi_dqperchunk;
 	}

fs/xfs/xfs_quota.h +0 -0

index c61e31c..c38068f 100644

Quoted 28 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Calculating dquot CRCs when the backing buffer is written back just doesn't work reliably. There are several places which manipulate dquots directly in the buffers, and they don't calculate CRCs appropriately, nor do they always set the buffer up to calculate CRCs appropriately.

Firstly, if we log a dquot buffer (e.g. during allocation) it gets logged without valid CRC, and so on recovery we end up with a dquot that is not valid.

Secondly, if we recover/repair a dquot, we don't have a verifier attached to the buffer and hence CRCs arenot calculate don the way down to disk.

Thirdly, calculating the CRC after we've changed the contents means that if we re-read the dquot from the buffer, we cannot verify the contents of the dquot are valid, as the CRC is invalid.

So, to avoid all the dquot CRC errors that are being detected by the read verifier, change to using the same model as for inodes. that is, dquot CRCs are calculated and written to the backing buffer at the time the dquot is flushed to the backing buffer. If we modify the dquuot directly in the backing buffer, calculate the CRC immediately after the modification is complete. Hence the dquot in the on-disk buffer should always have a valid CRC.

Quoted 1 line

Signed-off-by Dave Chinner <[email protected]>

Reviewed-by Brian Foster <[email protected]>

Quoted 23 lines

---
 fs/xfs/xfs_dquot.c       |   37 ++++++++++++++++---------------------
 fs/xfs/xfs_log_recover.c |   10 ++++++++++
 fs/xfs/xfs_qm.c          |   40 ++++++++++++++++++++++++++++++----------
 fs/xfs/xfs_quota.h       |    2 ++
 4 files changed, 58 insertions(+), 31 deletions(-)

fs/xfs/xfs_dquot.c +4 -1

index a41f8bf..044e97a 100644
@@ -249,8 +249,11 @@ xfs_qm_init_dquot_blk(
 		d->dd_diskdq.d_version = XFS_DQUOT_VERSION;
 		d->dd_diskdq.d_id = cpu_to_be32(curid);
 		d->dd_diskdq.d_flags = type;
-		if (xfs_sb_version_hascrc(&mp->m_sb))
+		if (xfs_sb_version_hascrc(&mp->m_sb)) {
 			uuid_copy(&d->dd_uuid, &mp->m_sb.sb_uuid);
+			xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
+					 XFS_DQUOT_CRC_OFF);
+		}
 	}
 

fs/xfs/xfs_dquot.c xfs_qm_init_dquot_blk( · line 259

Quoted 1 line

fs/xfs/xfs_dquot.c +0 -0

 	xfs_trans_dquot_buf(tp, bp,

Quoted 56 lines · Brian Foster

Quoted 28 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Calculating dquot CRCs when the backing buffer is written back just doesn't work reliably. There are several places which manipulate dquots directly in the buffers, and they don't calculate CRCs appropriately, nor do they always set the buffer up to calculate CRCs appropriately.

Firstly, if we log a dquot buffer (e.g. during allocation) it gets logged without valid CRC, and so on recovery we end up with a dquot that is not valid.

Secondly, if we recover/repair a dquot, we don't have a verifier attached to the buffer and hence CRCs arenot calculate don the way down to disk.

Thirdly, calculating the CRC after we've changed the contents means that if we re-read the dquot from the buffer, we cannot verify the contents of the dquot are valid, as the CRC is invalid.

So, to avoid all the dquot CRC errors that are being detected by the read verifier, change to using the same model as for inodes. that is, dquot CRCs are calculated and written to the backing buffer at the time the dquot is flushed to the backing buffer. If we modify the dquuot directly in the backing buffer, calculate the CRC immediately after the modification is complete. Hence the dquot in the on-disk buffer should always have a valid CRC.

Quoted 1 line

Signed-off-by Dave Chinner <[email protected]>

Reviewed-by Brian Foster <[email protected]>

Quoted 23 lines

---
 fs/xfs/xfs_dquot.c       |   37 ++++++++++++++++---------------------
 fs/xfs/xfs_log_recover.c |   10 ++++++++++
 fs/xfs/xfs_qm.c          |   40 ++++++++++++++++++++++++++++++----------
 fs/xfs/xfs_quota.h       |    2 ++
 4 files changed, 58 insertions(+), 31 deletions(-)

fs/xfs/xfs_dquot.c +4 -1

index a41f8bf..044e97a 100644
@@ -249,8 +249,11 @@ xfs_qm_init_dquot_blk(
 		d->dd_diskdq.d_version = XFS_DQUOT_VERSION;
 		d->dd_diskdq.d_id = cpu_to_be32(curid);
 		d->dd_diskdq.d_flags = type;
-		if (xfs_sb_version_hascrc(&mp->m_sb))
+		if (xfs_sb_version_hascrc(&mp->m_sb)) {
 			uuid_copy(&d->dd_uuid, &mp->m_sb.sb_uuid);
+			xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
+					 XFS_DQUOT_CRC_OFF);
+		}
 	}
 

Quoted 1 line

fs/xfs/xfs_dquot.c xfs_qm_init_dquot_blk( · line 259

Quoted 1 line

fs/xfs/xfs_dquot.c +0 -0

 	xfs_trans_dquot_buf(tp, bp,

Huh. Looks like this buffer will not be recovered. See xlog_recover_do_reg_buffer... I think he'll be skipped in recovery. Goto next.

That's not an issue with this patch though.

Reviewed-by Ben Myers <[email protected]>

From: Dave Chinner <[email protected]>

The inode unlinked list manipulations operate directly on the inode buffer, and so bypass the inode CRC calculation mechanisms. Hence an inode on the unlinked list has an invalid CRC. Fix this by recalculating the CRC whenever we modify an unlinked list pointer in an inode, ncluding during log recovery. This is trivial to do and results in unlinked list operations always leaving a consistent inode in the buffer.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_inode.c       |   16 ++++++++++++++++
 fs/xfs/xfs_log_recover.c |    9 +++++++++
 2 files changed, 25 insertions(+)

fs/xfs/xfs_inode.c +12 -0

index efbe1ac..c50e785 100644
@@ -1638,6 +1638,10 @@ xfs_iunlink(
 		dip->di_next_unlinked = agi->agi_unlinked[bucket_index];
 		offset = ip->i_imap.im_boffset +
 			offsetof(xfs_dinode_t, di_next_unlinked);
+
+		/* need to recalc the inode CRC if appropriate */
+		xfs_dinode_calc_crc(mp, dip);
+
 		xfs_trans_inode_buf(tp, ibp);
 		xfs_trans_log_buf(tp, ibp, offset,
 				  (offset + sizeof(xfs_agino_t) - 1));

@@ -1723,6 +1727,10 @@ xfs_iunlink_remove(
 			dip->di_next_unlinked = cpu_to_be32(NULLAGINO);
 			offset = ip->i_imap.im_boffset +
 				offsetof(xfs_dinode_t, di_next_unlinked);
+
+			/* need to recalc the inode CRC if appropriate */
+			xfs_dinode_calc_crc(mp, dip);
+
 			xfs_trans_inode_buf(tp, ibp);
 			xfs_trans_log_buf(tp, ibp, offset,
 					  (offset + sizeof(xfs_agino_t) - 1));

@@ -1796,6 +1804,10 @@ xfs_iunlink_remove(
 			dip->di_next_unlinked = cpu_to_be32(NULLAGINO);
 			offset = ip->i_imap.im_boffset +
 				offsetof(xfs_dinode_t, di_next_unlinked);
+
+			/* need to recalc the inode CRC if appropriate */
+			xfs_dinode_calc_crc(mp, dip);
+
 			xfs_trans_inode_buf(tp, ibp);
 			xfs_trans_log_buf(tp, ibp, offset,
 					  (offset + sizeof(xfs_agino_t) - 1));

fs/xfs/xfs_inode.c +0 -0

 		xfs_trans_inode_buf(tp, last_ibp);
 		xfs_trans_log_buf(tp, last_ibp, offset,
 				  (offset + sizeof(xfs_agino_t) - 1));

fs/xfs/xfs_log_recover.c +2 -0

index 83088d9..45a85ff 100644
@@ -1912,6 +1912,15 @@ xlog_recover_do_inode_buffer(
 		buffer_nextp = (xfs_agino_t *)xfs_buf_offset(bp,
 					      next_unlinked_offset);
 		*buffer_nextp = *logged_nextp;
+
+		/*

Quoted 11 lines · Dave Chinner

From: Dave Chinner <[email protected]>

The inode unlinked list manipulations operate directly on the inode buffer, and so bypass the inode CRC calculation mechanisms. Hence an inode on the unlinked list has an invalid CRC. Fix this by recalculating the CRC whenever we modify an unlinked list pointer in an inode, ncluding during log recovery. This is trivial to do and results in unlinked list operations always leaving a consistent inode in the buffer.

Signed-off-by Dave Chinner <[email protected]>

fs/xfs/xfs_inode.c xfs_iunlink_remove( · line 1827

Quoted 8 lines

fs/xfs/xfs_inode.c +4 -0

@@ -1809,6 +1821,10 @@ xfs_iunlink_remove(
 		last_dip->di_next_unlinked = cpu_to_be32(next_agino);
 		ASSERT(next_agino != 0);
 		offset = last_offset + offsetof(xfs_dinode_t, di_next_unlinked);
+
+		/* need to recalc the inode CRC if appropriate */
+		xfs_dinode_calc_crc(mp, dip);
+

Ugh, sorry I didn't notice this last time around, but this one looks like it should recalculate the crc on last_dip instead of dip.

From: Dave Chinner <[email protected]>

When invalidating an attribute leaf block block, there might be remote attributes that it points to. With the recent rework of the remote attribute format, we have to make sure we calculate the length of the attribute correctly. We aren't doing that in xfs_attr3_leaf_inactive(), so fix it.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_attr_leaf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_attr_leaf.c +0 -0

index d788302..31d3cd1 100644

From: Dave Chinner<[email protected]>

When invalidating an attribute leaf block block, there might be remote attributes that it points to. With the recent rework of the remote attribute format, we have to make sure we calculate the length of the attribute correctly. We aren't doing that in xfs_attr3_leaf_inactive(), so fix it.

Signed-off-by Dave Chinner<[email protected]>

Quoted 9 lines · Dave Chinner

From: Dave Chinner<[email protected]>

When invalidating an attribute leaf block block, there might be remote attributes that it points to. With the recent rework of the remote attribute format, we have to make sure we calculate the length of the attribute correctly. We aren't doing that in xfs_attr3_leaf_inactive(), so fix it.

Signed-off-by Dave Chinner<[email protected]>

I scratched my head reading:

in xfs_attr_leaf.h:
/*
* Used to keep a list of "remote value" extents when unlinking an inode. */

typedef struct xfs_attr_inactive_list {
	xfs_dablk_t	valueblk;	/* block number of value bytes */
	int		valuelen;	/* number of bytes in value */
						     ^^^^^
						     |||||
} xfs_attr_inactive_list_t;

Where "valuelen" is clearly being used as blocks.

A more obvious name is
the former "valueblk". Blame commit d7929ff6 for the confusion. Should change
the comment and/or variable one of these days ...

From: Dave Chinner <[email protected]>

When invalidating an attribute leaf block block, there might be remote attributes that it points to. With the recent rework of the remote attribute format, we have to make sure we calculate the length of the attribute correctly. We aren't doing that in xfs_attr3_leaf_inactive(), so fix it.

Signed-off-by Dave Chinner <[email protected]>

From: Dave Chinner <[email protected]>

attr2 format is always enabled for v5 superblock filesystems, so the mount options to enable or disable it need to be cause mount errors.

Signed-off-by Dave Chinner <[email protected]>

---
 Documentation/filesystems/xfs.txt |    3 +++
 fs/xfs/xfs_super.c                |   11 +++++++++++
 2 files changed, 14 insertions(+)

Documentation/filesystems/xfs.txt +3 -0

index 3e4b3dd..83577f0 100644
@@ -33,6 +33,9 @@ When mounting an XFS filesystem, the following options are accepted.
 	removing extended attributes) the on-disk superblock feature
 	bit field will be updated to reflect this format being in use.
 
+	CRC enabled filesystems always use the attr2 format, and so
+	will reject the noattr2 mount option if it is set.
+
   barrier
 	Enables the use of block layer write barriers for writes into
 	the journal and unwritten extent conversion.  This allows for

fs/xfs/xfs_super.c +0 -0

index ea341ce..f62abb2 100644

From: Dave Chinner<[email protected]>

attr2 format is always enabled for v5 superblock filesystems, so the mount options to enable or disable it need to be cause mount errors.

Signed-off-by Dave Chinner<[email protected]>

---
  Documentation/filesystems/xfs.txt |    3 +++
  fs/xfs/xfs_super.c                |   11 +++++++++++
  2 files changed, 14 insertions(+)

Documentation/filesystems/xfs.txt +0 -0

index 3e4b3dd..83577f0 100644

Quoted 19 lines · Dave Chinner

From: Dave Chinner<[email protected]>

attr2 format is always enabled for v5 superblock filesystems, so the mount options to enable or disable it need to be cause mount errors.

Signed-off-by Dave Chinner<[email protected]>

---
 Documentation/filesystems/xfs.txt |    3 +++
 fs/xfs/xfs_super.c                |   11 +++++++++++
 2 files changed, 14 insertions(+)

Documentation/filesystems/xfs.txt +0 -0

index 3e4b3dd..83577f0 100644
@@ -33,6 +33,9 @@ When mounting an XFS filesystem, the following options are accepted.
 	removing extended attributes) the on-disk superblock feature
 	bit field will be updated to reflect this format being in use.
 

Documentation/filesystems/xfs.txt When mounting an XFS filesystem, the following options are accepted. · line 36

Quoted 1 line

Documentation/filesystems/xfs.txt +1 -0

+	CRC enabled filesystems always use the attr2 format, and so

Filesystems using the version 5 superblock always use the attr2 format, and so

Documentation/filesystems/xfs.txt When mounting an XFS filesystem, the following options are accepted. · line 37

Quoted 1 line

Documentation/filesystems/xfs.txt +1 -0

+	will reject the noattr2 mount option if it is set.

reject the noattr2 mount option if it is set.

Quoted 1 line

Documentation/filesystems/xfs.txt +1 -0

+

From: Dave Chinner<[email protected]>

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<[email protected]>

Reviewed-by Brian Foster<[email protected]>

---

fs/xfs/xfs_acl.c +0 -0

  	if (type == ACL_TYPE_ACCESS) {

fs/xfs/xfs_acl.h +9 -7

index 39632d9..0da8725 100644
@@ -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];
  };

Quoted 17 lines · Dave Chinner

From: Dave Chinner<[email protected]>

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<[email protected]>

Reviewed-by Brian Foster<[email protected]>

---

fs/xfs/xfs_acl.c +1 -1

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

Review xfs_set_acl(struct inode *inode, int type, struct posix_acl *acl) · line 198

Quoted 2 lines

(quoted diff) +1 -1

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

Quoted 30 lines

fs/xfs/xfs_acl.c +0 -0

 	if (type == ACL_TYPE_ACCESS) {

fs/xfs/xfs_acl.h +9 -7

index 39632d9..0da8725 100644
@@ -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];
 };

fs/xfs/xfs_acl.h struct inode; · line 48

Quoted 10 lines

fs/xfs/xfs_acl.h +9 -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

fs/xfs/xfs_acl.h struct inode; · line 52

Quoted 4 lines

fs/xfs/xfs_acl.h +4 -0

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

Quoted 10 lines · Mark Tinguely

Quoted 9 lines · Dave Chinner

From: Dave Chinner<[email protected]>

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<[email protected]>

Reviewed-by Brian Foster<[email protected]>

---

Quoted 39 lines

Quoted 18 lines

/* On-disk XFS access control list structure */

fs/xfs/xfs_acl.h +9 -6

+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];
 };

fs/xfs/xfs_acl.h struct inode; · line 48

Quoted 10 lines

fs/xfs/xfs_acl.h +9 -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

fs/xfs/xfs_acl.h struct inode; · line 52

Quoted 4 lines

fs/xfs/xfs_acl.h +4 -0

+
+#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

Quoted 3 lines

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.

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Brian Foster <[email protected]>

Quoted 8 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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 <[email protected]>

Reviewed-by Brian Foster <[email protected]>

Do we have any means of testing that many acls?

Quoted 7 lines · Ben Myers

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Dave Chinner <[email protected]>

Reviewed-by Brian Foster <[email protected]>

Do we have any means of testing that many acls?

Sure, just modify xfs/099. It fails with this patch because it successfuly creates 26 acls....