xfs: fixes for 3.10-rc4

From: Dave Chinner <[email protected]>

We write the superblock every 30s or so which results in the verifier being called. Right now that results in this output every 30s:

XFS (vda): Version 5 superblock detected. This kernel has EXPERIMENTAL support enabled! Use of these features in this kernel is at your own risk!

And spamming the logs.

We don't need to check for whether we support v5 superblocks or whether there are feature bits we don't support set as these are only relevant when we first mount the filesytem. i.e. on superblock read. Hence for the write verification we can just skip all the checks (and hence verbose output) altogether.

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

---

 fs/xfs/xfs_mount.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

fs/xfs/xfs_mount.c +10 -6

index f6bfbd7..e8e310c 100644
@@ -314,7 +314,8 @@ STATIC int
 xfs_mount_validate_sb(
 	xfs_mount_t	*mp,
 	xfs_sb_t	*sbp,
-	bool		check_inprogress)
+	bool		check_inprogress,
+	bool		check_version)
 {
 
 	/*

@@ -337,9 +338,10 @@ xfs_mount_validate_sb(
 
 	/*
 	 * Version 5 superblock feature mask validation. Reject combinations the
-	 * kernel cannot support up front before checking anything else.
+	 * kernel cannot support up front before checking anything else. For
+	 * write validation, we don't need to check feature masks.
 	 */
-	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) {
+	if (check_version && XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) {
 		xfs_alert(mp,
 "Version 5 superblock detected. This kernel has EXPERIMENTAL support enabled!\n"
 "Use of these features in this kernel is at your own risk!");

@@ -675,7 +677,8 @@ xfs_sb_to_disk(
 
 static int
 xfs_sb_verify(
-	struct xfs_buf	*bp)
+	struct xfs_buf	*bp,
+	bool		check_version)
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
 	struct xfs_sb	sb;

@@ -686,7 +689,8 @@ xfs_sb_verify(
 	 * Only check the in progress field for the primary superblock as
 	 * mkfs.xfs doesn't clear it from secondary superblocks.
 	 */
-	return xfs_mount_validate_sb(mp, &sb, bp->b_bn == XFS_SB_DADDR);
+	return xfs_mount_validate_sb(mp, &sb, bp->b_bn == XFS_SB_DADDR,
+				     check_version);
 }
 
 /*

@@ -719,7 +723,7 @@ xfs_sb_read_verify(
 			goto out_error;
 		}
 	}
-	error = xfs_sb_verify(bp);
+	error = xfs_sb_verify(bp, true);
 
 out_error:
 	if (error) {

Quoted 17 lines · Dave Chinner

From: Dave Chinner <[email protected]>

We write the superblock every 30s or so which results in the verifier being called. Right now that results in this output every 30s:

XFS (vda): Version 5 superblock detected. This kernel has EXPERIMENTAL support enabled! Use of these features in this kernel is at your own risk!

And spamming the logs.

We don't need to check for whether we support v5 superblocks or whether there are feature bits we don't support set as these are only relevant when we first mount the filesytem. i.e. on superblock read. Hence for the write verification we can just skip all the checks (and hence verbose output) altogether.

Quoted 2 lines

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

---

Reviewed-by Brian Foster <[email protected]>

Quoted 18 lines · Brian Foster

Quoted 17 lines · Dave Chinner

From: Dave Chinner <[email protected]>

We write the superblock every 30s or so which results in the verifier being called. Right now that results in this output every 30s:

XFS (vda): Version 5 superblock detected. This kernel has EXPERIMENTAL support enabled! Use of these features in this kernel is at your own risk!

And spamming the logs.

We don't need to check for whether we support v5 superblocks or whether there are feature bits we don't support set as these are only relevant when we first mount the filesytem. i.e. on superblock read. Hence for the write verification we can just skip all the checks (and hence verbose output) altogether.

Quoted 4 lines

Quoted 2 lines

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

---

Reviewed-by Brian Foster <[email protected]>

Applied.

Quoted 22 lines · Ben Myers

Quoted 21 lines · Brian Foster

Quoted 19 lines · Dave Chinner

From: Dave Chinner <[email protected]>

We write the superblock every 30s or so which results in the verifier being called. Right now that results in this output every 30s:

XFS (vda): Version 5 superblock detected. This kernel has EXPERIMENTAL support enabled! Use of these features in this kernel is at your own risk!

And spamming the logs.

We don't need to check for whether we support v5 superblocks or whether there are feature bits we don't support set as these are only relevant when we first mount the filesytem. i.e. on superblock read. Hence for the write verification we can just skip all the checks (and hence verbose output) altogether.

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

---

Quoted 3 lines

Quoted 1 line

Reviewed-by Brian Foster <[email protected]>

Applied.

Ben, while I see this in the xfsdev tree, I don't see it in Linus' tree. Did it get missed on the recent merges? If so, can you queue it up for 3.10-rc6?

From: Dave Chinner <[email protected]>

When CRCs are enabled, the number of blocks needed to hold a remote symlink on a 1k block size filesystem may be 2 instead of 1. The transaction reservation for the allocated bloks was not taking this into account and only allocating one block. hence when trying to read or invalidate such symlinks, we are mapping a hole where there should be a block and things go bad at that point.

Fix the reservation to use the correct block count, clean up the block count calculation similar to the remote attribute calculation, and add a debug guard to detect when we don't write the entire symlink to disk.

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

---

 fs/xfs/xfs_symlink.c |   20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

fs/xfs/xfs_symlink.c +5 -14

index 5f234389..195a403 100644
@@ -56,16 +56,9 @@ xfs_symlink_blocks(
 	struct xfs_mount *mp,
 	int		pathlen)
 {
-	int		fsblocks = 0;
-	int		len = pathlen;
+	int buflen = XFS_SYMLINK_BUF_SPACE(mp, mp->m_sb.sb_blocksize);
 
-	do {
-		fsblocks++;
-		len -= XFS_SYMLINK_BUF_SPACE(mp, mp->m_sb.sb_blocksize);
-	} while (len > 0);
-
-	ASSERT(fsblocks <= XFS_SYMLINK_MAPS);
-	return fsblocks;
+	return (pathlen + buflen - 1) / buflen;
 }
 
 static int

@@ -405,7 +398,7 @@ xfs_symlink(
 	if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
 		fs_blocks = 0;
 	else
-		fs_blocks = XFS_B_TO_FSB(mp, pathlen);
+		fs_blocks = xfs_symlink_blocks(mp, pathlen);
 	resblks = XFS_SYMLINK_SPACE_RES(mp, link_name->len, fs_blocks);
 	error = xfs_trans_reserve(tp, resblks, XFS_SYMLINK_LOG_RES(mp), 0,
 			XFS_TRANS_PERM_LOG_RES, XFS_SYMLINK_LOG_COUNT);

@@ -512,7 +505,7 @@ xfs_symlink(
 		cur_chunk = target_path;
 		offset = 0;
 		for (n = 0; n < nmaps; n++) {
-			char *buf;
+			char	*buf;
 
 			d = XFS_FSB_TO_DADDR(mp, mval[n].br_startblock);
 			byte_cnt = XFS_FSB_TO_B(mp, mval[n].br_blockcount);

@@ -525,9 +518,7 @@ xfs_symlink(
 			bp->b_ops = &xfs_symlink_buf_ops;
 
 			byte_cnt = XFS_SYMLINK_BUF_SPACE(mp, byte_cnt);
-			if (pathlen < byte_cnt) {
-				byte_cnt = pathlen;
-			}
+			byte_cnt = min(byte_cnt, pathlen);
 
 			buf = bp->b_addr;
 			buf += xfs_symlink_hdr_set(mp, ip->i_ino, offset,

Quoted 14 lines · Dave Chinner

From: Dave Chinner <[email protected]>

When CRCs are enabled, the number of blocks needed to hold a remote symlink on a 1k block size filesystem may be 2 instead of 1. The transaction reservation for the allocated bloks was not taking this into account and only allocating one block. hence when trying to read or invalidate such symlinks, we are mapping a hole where there should be a block and things go bad at that point.

Fix the reservation to use the correct block count, clean up the block count calculation similar to the remote attribute calculation, and add a debug guard to detect when we don't write the entire symlink to disk.

Quoted 2 lines

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

---

Reviewed-by Brian Foster <[email protected]>

Unrelated question... I noticed we update di_size in xfs_symlink(). I presume this is safe because, even in the non-local case, we actually log the data (the path) in the buffers as well, right?

Quoted 14 lines · Dave Chinner

From: Dave Chinner <[email protected]>

When CRCs are enabled, the number of blocks needed to hold a remote symlink on a 1k block size filesystem may be 2 instead of 1. The transaction reservation for the allocated bloks was not taking this into account and only allocating one block. hence when trying to read or invalidate such symlinks, we are mapping a hole where there should be a block and things go bad at that point.

Fix the reservation to use the correct block count, clean up the block count calculation similar to the remote attribute calculation, and add a debug guard to detect when we don't write the entire symlink to disk.

Quoted 2 lines

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

---

Reviewed-by Brian Foster <[email protected]>

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

---
 fs/xfs/xfs_acl.c |   31 ++++++++++++++++++-------------
 fs/xfs/xfs_acl.h |   30 +++++++++++++++++++++++-------
 2 files changed, 41 insertions(+), 20 deletions(-)

fs/xfs/xfs_acl.c +7 -3

index 1d32f1d..b6bed9b 100644
@@ -21,6 +21,8 @@
 #include "xfs_bmap_btree.h"
 #include "xfs_inode.h"
 #include "xfs_vnodeops.h"
+#include "xfs_sb.h"
+#include "xfs_mount.h"
 #include "xfs_trace.h"
 #include <linux/slab.h>
 #include <linux/xattr.h>

@@ -34,7 +36,9 @@
  */
 
 STATIC struct posix_acl *
-xfs_acl_from_disk(struct xfs_acl *aclp)
+xfs_acl_from_disk(
+	struct xfs_acl	*aclp,
+	int		max_entries)
 {
 	struct posix_acl_entry *acl_e;
 	struct posix_acl *acl;

@@ -42,7 +46,7 @@ xfs_acl_from_disk(struct xfs_acl *aclp)
 	unsigned int count, i;
 
 	count = be32_to_cpu(aclp->acl_cnt);
-	if (count > XFS_ACL_MAX_ENTRIES)
+	if (count > max_entries)
 		return ERR_PTR(-EFSCORRUPTED);
 
 	acl = posix_acl_alloc(count, GFP_KERNEL);

@@ -108,7 +112,7 @@ xfs_get_acl(struct inode *inode, int type)
 	struct xfs_inode *ip = XFS_I(inode);
 	struct posix_acl *acl;
 	struct xfs_acl *xfs_acl;
-	int len = sizeof(struct xfs_acl);
+	int len = XFS_ACL_SIZE(ip->i_mount);
 	unsigned char *ea_name;
 	int error;
 

fs/xfs/xfs_acl.c +10 -8

+	xfs_acl = kzalloc(len, GFP_KERNEL);
 	if (!xfs_acl)
 		return ERR_PTR(-ENOMEM);
 

@@ -153,7 +157,7 @@ xfs_get_acl(struct inode *inode, int type)
 		goto out;
 	}
 
-	acl = xfs_acl_from_disk(xfs_acl);
+	acl = xfs_acl_from_disk(xfs_acl, XFS_ACL_MAX_ENTRIES(ip->i_mount));
 	if (IS_ERR(acl))
 		goto out;
 

@@ -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);
 		if (!xfs_acl)
 			return -ENOMEM;
 
 		xfs_acl_to_disk(xfs_acl, acl);
-		len = sizeof(struct xfs_acl) -
-			(sizeof(struct xfs_acl_entry) *
-			 (XFS_ACL_MAX_ENTRIES - acl->a_count));
+
+		/* subtract away the unused acl entries */
+		len -= sizeof(struct xfs_acl_entry) *
+			 (XFS_ACL_MAX_ENTRIES(ip->i_mount) - acl->a_count);
 
 		error = -xfs_attr_set(ip, ea_name, (unsigned char *)xfs_acl,
 				len, ATTR_ROOT);

@@ -243,7 +248,7 @@ xfs_set_mode(struct inode *inode, umode_t mode)
 static int
 xfs_acl_exists(struct inode *inode, unsigned char *name)
 {
-	int len = sizeof(struct xfs_acl);
+	int len = XFS_ACL_SIZE(XFS_M(inode->i_sb));
 
 	return (xfs_attr_get(XFS_I(inode), name, NULL, &len,
 			    ATTR_ROOT|ATTR_KERNOVAL) == 0);

@@ -379,7 +384,7 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
 		goto out_release;
 
 	error = -EINVAL;
-	if (acl->a_count > XFS_ACL_MAX_ENTRIES)
+	if (acl->a_count > XFS_ACL_MAX_ENTRIES(XFS_M(inode->i_sb)))
 		goto out_release;
 
 	if (type == ACL_TYPE_ACCESS) {

fs/xfs/xfs_acl.h +16 -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];
 };
 
+/*
+ * 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) \

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          |   36 ++++++++++++++++++++++++++----------
 fs/xfs/xfs_quota.h       |    2 ++
 4 files changed, 54 insertions(+), 31 deletions(-)

fs/xfs/xfs_qm.c +7 -2

index f41702b..d181542 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

fs/xfs/xfs_qm.c +17 -7

 	}
 }
 

@@ -907,19 +913,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 36 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.

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          |   36 ++++++++++++++++++++++++++----------
 fs/xfs/xfs_quota.h       |    2 ++
 4 files changed, 54 insertions(+), 31 deletions(-)

...

Quoted 40 lines

fs/xfs/xfs_qm.c +7 -3

index f41702b..d181542 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,8 @@ 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);

fs/xfs/xfs_qm.c xfs_qm_reset_dqcounts( · line 880

Quoted 2 lines

fs/xfs/xfs_qm.c +2 -0

+		xfs_update_cksum((char *)&dqb[j], sizeof(struct xfs_dqblk),
+					 XFS_DQUOT_CRC_OFF);

Nice cleanup on the cast ugliness even without the crc change. Is there a technical reason for the unconditional crc update here beyond that we're doing a reset? I'm wondering if there's any value in leaving those bits untouched for a filesystem that might have never enabled crc (quotacheck or not).

FWIW, the rest of this patch looks sane to me (I'm less familiar with the log recovery code, but the changes seem isolated and straightforward) and I couldn't locate anywhere else we modify the backing buffer for the dquot.

Quoted 78 lines · Brian Foster

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

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          |   36 ++++++++++++++++++++++++++----------
 fs/xfs/xfs_quota.h       |    2 ++
 4 files changed, 54 insertions(+), 31 deletions(-)

...

Quoted 40 lines

fs/xfs/xfs_qm.c +7 -3

index f41702b..d181542 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,8 @@ 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);

Quoted 8 lines

fs/xfs/xfs_qm.c xfs_qm_reset_dqcounts( · line 880

Quoted 2 lines

fs/xfs/xfs_qm.c +2 -0

+		xfs_update_cksum((char *)&dqb[j], sizeof(struct xfs_dqblk),
+					 XFS_DQUOT_CRC_OFF);

Nice cleanup on the cast ugliness even without the crc change. Is there a technical reason for the unconditional crc update here beyond that we're doing a reset? I'm wondering if there's any value in leaving those bits untouched for a filesystem that might have never enabled crc (quotacheck or not).

The dquot might be zeroed and unused, but the buffer it sits in is still allocated and valid. That means if we ever start using that dquot again (either by quotacheck or a new uid/gid/prid), it will be read straight out of the buffer rather than allocated, and hence the constraint that allocated but unused dquots still need to have valid CRCs.

FWIW, the dquot buffer read validates the CRC on all dquots in the buffer when it comes off disk as it has no way of knowing what dquots contain valid data or not. Same with the xfs_qm_dqcheck() call - an unused dquot still needs to be a valid dquot to pass those checks...

Quoted 4 lines

FWIW, the rest of this patch looks sane to me (I'm less familiar with the log recovery code, but the changes seem isolated and straightforward) and I couldn't locate anywhere else we modify the backing buffer for the dquot.

Right, apart from dquot allocation and flushing, there aren't any. Resetting the dquots to zero before a quota check is a special case. Doing it via the buffer avoids needing to initialise all the dquots in memory that *might* be tracked by the quota file. But we don't know what quota IDs are tracked in the quota file with first mapping the quota file and finding all the offsets that contain blocks. And, well, once we have that mapping, why would be do N xfs_qm_dqget() calls per buffer to get initialised, cached dquots from the buffer when we can simply RMW the buffers we've mapped directly?

Cheers,

Dave.

Quoted 31 lines · Dave Chinner

Quoted 30 lines · Brian Foster

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

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

Quoted 28 lines

Quoted 19 lines

Quoted 19 lines

fs/xfs/xfs_qm.c +5 -2

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

Quoted 8 lines

fs/xfs/xfs_qm.c xfs_qm_reset_dqcounts( · line 880

Quoted 2 lines

fs/xfs/xfs_qm.c +2 -0

+		xfs_update_cksum((char *)&dqb[j], sizeof(struct xfs_dqblk),
+					 XFS_DQUOT_CRC_OFF);

Nice cleanup on the cast ugliness even without the crc change. Is there a technical reason for the unconditional crc update here beyond that we're doing a reset? I'm wondering if there's any value in leaving those bits untouched for a filesystem that might have never enabled crc (quotacheck or not).

Quoted 7 lines

The dquot might be zeroed and unused, but the buffer it sits in is still allocated and valid. That means if we ever start using that dquot again (either by quotacheck or a new uid/gid/prid), it will be read straight out of the buffer rather than allocated, and hence the constraint that allocated but unused dquots still need to have valid CRCs.

The constraint makes sense when CRCs are enabled...

Quoted 6 lines

FWIW, the dquot buffer read validates the CRC on all dquots in the buffer when it comes off disk as it has no way of knowing what dquots contain valid data or not. Same with the xfs_qm_dqcheck() call - an unused dquot still needs to be a valid dquot to pass those checks...

Yeah, that part makes sense. I've followed through and grokked most of the dquot buffer read and dquot CRC validation code, I think.

My question is more why is the code above (in xfs_qm_reset_dqcounts()) not the following?

	if (xfs_sb_version_hascrc(&mp->m_sb))
		xfs_update_cksum(...);

From: Dave Chinner<[email protected]>

A long time ago in a galaxy far away....

.. the was a commit made to fix some ilinux specific "fragmented buffer" log recovery problem:

http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=commitdiff;h=b29c0bece51da72fb3ff3b61391a391ea54e1603

That problem occurred when a contiguous dirty region of a buffer was split across across two pages of an unmapped buffer. It's been a long time since that has been done in XFS, and the changes to log the entire inode buffers for CRC enabled filesystems has re-introduced that corner case.

And, of course, it turns out that the above commit didn't actually fix anything - it just ensured that log recovery is guaranteed to fail when this situation occurs. And now for the gory details.

Quoted 15 lines · Dave Chinner

From: Dave Chinner<[email protected]>

A long time ago in a galaxy far away....

.. the was a commit made to fix some ilinux specific "fragmented buffer" log recovery problem:

http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=commitdiff;h=b29c0bece51da72fb3ff3b61391a391ea54e1603

That problem occurred when a contiguous dirty region of a buffer was split across across two pages of an unmapped buffer. It's been a long time since that has been done in XFS, and the changes to log the entire inode buffers for CRC enabled filesystems has re-introduced that corner case.

Quoted 4 lines

And, of course, it turns out that the above commit didn't actually fix anything - it just ensured that log recovery is guaranteed to fail when this situation occurs. And now for the gory details.

...

Thanks for the great walk through of the problem.

Reviewed-by Mark Tinguely <[email protected]>

From: Dave Chinner <[email protected]>

Currently, swapping extents from one inode to another is a simple act of switching data and attribute forks from one inode to another. This, unfortunately in no longer so simple with CRC enabled filesystems as there is owner information embedded into the BMBT blocks that are swapped between inodes. Hence swapping the forks between inodes results in the inodes having mapping blocks that point to the wrong owner and hence are considered corrupt.

To fix this we need an extent tree block or record based swap algorithm so that the BMBT block owner information can be updated atomically in the swap transaction. This is a significant piece of new work, so for the moment simply don't allow swap extent operations to succeed on CRC enabled filesystems.

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

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Currently, swapping extents from one inode to another is a simple act of switching data and attribute forks from one inode to another. This, unfortunately in no longer so simple with CRC enabled filesystems as there is owner information embedded into the BMBT blocks that are swapped between inodes. Hence swapping the forks between inodes results in the inodes having mapping blocks that point to the wrong owner and hence are considered corrupt.

To fix this we need an extent tree block or record based swap algorithm so that the BMBT block owner information can be updated atomically in the swap transaction. This is a significant piece of new work, so for the moment simply don't allow swap extent operations to succeed on CRC enabled filesystems.

Quoted 1 line

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

I believe we do want to have functional swap extents for crc enabled filesystems.

But this is fine as long as it is temporary.

From: Dave Chinner <[email protected]>

Currently, swapping extents from one inode to another is a simple act of switching data and attribute forks from one inode to another. This, unfortunately in no longer so simple with CRC enabled filesystems as there is owner information embedded into the BMBT blocks that are swapped between inodes. Hence swapping the forks between inodes results in the inodes having mapping blocks that point to the wrong owner and hence are considered corrupt.

To fix this we need an extent tree block or record based swap algorithm so that the BMBT block owner information can be updated atomically in the swap transaction. This is a significant piece of new work, so for the moment simply don't allow swap extent operations to succeed on CRC enabled filesystems.

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

---

 fs/xfs/xfs_dfrag.c |    8 ++++++++
 1 file changed, 8 insertions(+)

fs/xfs/xfs_dfrag.c +1 -0

index f852b08..c407e1c 100644
@@ -219,6 +219,14 @@ xfs_swap_extents(
 	int		taforkblks = 0;
 	__uint64_t	tmp;
 
+	/*

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Currently, swapping extents from one inode to another is a simple act of switching data and attribute forks from one inode to another. This, unfortunately in no longer so simple with CRC enabled filesystems as there is owner information embedded into the BMBT blocks that are swapped between inodes. Hence swapping the forks between inodes results in the inodes having mapping blocks that point to the wrong owner and hence are considered corrupt.

To fix this we need an extent tree block or record based swap algorithm so that the BMBT block owner information can be updated atomically in the swap transaction. This is a significant piece of new work, so for the moment simply don't allow swap extent operations to succeed on CRC enabled filesystems.

Quoted 2 lines

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

---

Pretty straightforward...

Reviewed-by Brian Foster <[email protected]>

From: Dave Chinner <[email protected]>

XFS has failed to kill suid/sgid bits correctly when truncating files of non-zero size since commit c4ed4243 ("xfs: split xfs_setattr") introduced in the 3.1 kernel. Fix it.

Fix it.

cc stable kernel <[email protected]>

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

---
 fs/xfs/xfs_iops.c |   47 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

fs/xfs/xfs_iops.c +26 -15

index d82efaa..ca9ecaa 100644
@@ -455,6 +455,28 @@ xfs_vn_getattr(
 	return 0;
 }
 
+static void
+xfs_setattr_mode(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*ip,
+	struct iattr		*iattr)
+{
+	struct inode	*inode = VFS_I(ip);
+	umode_t		mode = iattr->ia_mode;
+
+	ASSERT(tp);
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+
+	if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
+		mode &= ~S_ISGID;
+
+	ip->i_d.di_mode &= S_IFMT;
+	ip->i_d.di_mode |= mode & ~S_IFMT;
+
+	inode->i_mode &= S_IFMT;
+	inode->i_mode |= mode & ~S_IFMT;
+}
+
 int
 xfs_setattr_nonsize(
 	struct xfs_inode	*ip,

@@ -606,18 +628,8 @@ xfs_setattr_nonsize(
 	/*
 	 * Change file access modes.
 	 */
-	if (mask & ATTR_MODE) {
-		umode_t mode = iattr->ia_mode;
-
-		if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
-			mode &= ~S_ISGID;
-
-		ip->i_d.di_mode &= S_IFMT;
-		ip->i_d.di_mode |= mode & ~S_IFMT;
-
-		inode->i_mode &= S_IFMT;
-		inode->i_mode |= mode & ~S_IFMT;
-	}
+	if (mask & ATTR_MODE)
+		xfs_setattr_mode(tp, ip, iattr);
 
 	/*
 	 * Change file access or modified times.

@@ -714,9 +726,8 @@ xfs_setattr_size(
 		return XFS_ERROR(error);
 
 	ASSERT(S_ISREG(ip->i_d.di_mode));
-	ASSERT((mask & (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
-			ATTR_MTIME_SET|ATTR_KILL_SUID|ATTR_KILL_SGID|
-			ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
+	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
+			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
 
 	if (!(flags & XFS_ATTR_NOLOCK)) {
 		lock_flags |= XFS_IOLOCK_EXCL;

@@ -860,6 +871,12 @@ xfs_setattr_size(
 		xfs_inode_clear_eofblocks_tag(ip);
 	}

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

XFS has failed to kill suid/sgid bits correctly when truncating files of non-zero size since commit c4ed4243 ("xfs: split xfs_setattr") introduced in the 3.1 kernel. Fix it.

The code makes sense and I can easily hit an assert when truncating (extending) a suid file on a debug kernel without this patch (and I see the suid dropped with the patch).

Hey Brian,

Quoted 8 lines · Brian Foster

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

XFS has failed to kill suid/sgid bits correctly when truncating files of non-zero size since commit c4ed4243 ("xfs: split xfs_setattr") introduced in the 3.1 kernel. Fix it.

Quoted 3 lines

The code makes sense and I can easily hit an assert when truncating (extending) a suid file on a debug kernel without this patch (and I see the suid dropped with the patch).

What commands did you use? It seems like this is dealing with S_ISGID, correct?

Thanks,
Ben

Quoted 11 lines · Ben Myers

Hey Brian,

Quoted 8 lines · Brian Foster

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

XFS has failed to kill suid/sgid bits correctly when truncating files of non-zero size since commit c4ed4243 ("xfs: split xfs_setattr") introduced in the 3.1 kernel. Fix it.

Quoted 6 lines

Quoted 3 lines

The code makes sense and I can easily hit an assert when truncating (extending) a suid file on a debug kernel without this patch (and I see the suid dropped with the patch).

What commands did you use? It seems like this is dealing with S_ISGID, correct?

Hi Ben,

Yeah, that confused me at first as well. I believe the vfs interprets the ATTR_KILL_SUID/SGIT bits prior to the setattr call and wipes out the associated mode bits if necessary.

What I did was basically create a zero sized file as root, chmod to a+s and a+rwx and then as a regular user, truncate that file to something larger than zero. Without the patch I hit the assert and with the patch the assert doesn't fire and the setuid bit is dropped.

From: Dave Chinner <[email protected]>

Currently userspace has no way of determining that a filesystem is CRC enabled. Add a flag to the XFS_IOC_FSGEOMETRY ioctl output to indicate that the filesystem has v5 superblock support enabled. This will allow xfs_info to correctly report the state of the filesystem.

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

---
 fs/xfs/xfs_fs.h    |    1 +
 fs/xfs/xfs_fsops.c |    4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

fs/xfs/xfs_fs.h +1 -0

index 6dda3f9..d046955 100644
@@ -236,6 +236,7 @@ typedef struct xfs_fsop_resblks {
 #define XFS_FSOP_GEOM_FLAGS_PROJID32	0x0800  /* 32-bit project IDs	*/
 #define XFS_FSOP_GEOM_FLAGS_DIRV2CI	0x1000	/* ASCII only CI names	*/
 #define XFS_FSOP_GEOM_FLAGS_LAZYSB	0x4000	/* lazy superblock counters */
+#define XFS_FSOP_GEOM_FLAGS_V5SB	0x8000	/* version 5 superblock */
 
 
 /*

fs/xfs/xfs_fsops.c +0 -0

index 87595b2..3c3644e 100644

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Currently userspace has no way of determining that a filesystem is CRC enabled. Add a flag to the XFS_IOC_FSGEOMETRY ioctl output to indicate that the filesystem has v5 superblock support enabled. This will allow xfs_info to correctly report the state of the filesystem.

Looks fine,

Reviewed-by Eric Sandeen <[email protected]>

Ben, having this in place for for the next point release will let userspace work & testing proceed w/o the need for a patched kernel... if you could consider pulling it in that'd be great.

Dave, just out of curiosity, most other features sort of match between the "_has_*" and the flag names, is there a reason for the crc <-> sbv5 difference? Just semantics, but just curious.

(i.e. xfs_sb_version_hasprojid32bit checks XFS_SB_VERSION2_PROJID32BIT, but xfs_sb_version_hascrc checks XFS_SB_VERSION_5)

Answering my own question maybe, I guess SB_VERSION_5 was conceived with crc already in place, so there's no need for a feature flag on top of the sb version, right...?

Hi Eric,

Quoted 10 lines · Eric Sandeen

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Currently userspace has no way of determining that a filesystem is CRC enabled. Add a flag to the XFS_IOC_FSGEOMETRY ioctl output to indicate that the filesystem has v5 superblock support enabled. This will allow xfs_info to correctly report the state of the filesystem.

Quoted 7 lines

Looks fine,

Reviewed-by Eric Sandeen <[email protected]>

Ben, having this in place for for the next point release will let userspace work & testing proceed w/o the need for a patched kernel... if you could consider pulling it in that'd be great.

Sounds reasonable. I'll check it out.

Quoted 10 lines

Dave, just out of curiosity, most other features sort of match between the "_has_*" and the flag names, is there a reason for the crc <-> sbv5 difference? Just semantics, but just curious.

(i.e. xfs_sb_version_hasprojid32bit checks XFS_SB_VERSION2_PROJID32BIT, but xfs_sb_version_hascrc checks XFS_SB_VERSION_5)

Answering my own question maybe, I guess SB_VERSION_5 was conceived with crc already in place, so there's no need for a feature flag on top of the sb version, right...?

Seems like we're also out of space in xfs_fsop_geom.flags.

There may even be
people who prefer to use v5 super blocks without crcs turned on, so maybe conflating the two ideas here is undesireable.

Hi Eric,

Quoted 10 lines · Eric Sandeen

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Currently userspace has no way of determining that a filesystem is CRC enabled. Add a flag to the XFS_IOC_FSGEOMETRY ioctl output to indicate that the filesystem has v5 superblock support enabled. This will allow xfs_info to correctly report the state of the filesystem.

Quoted 7 lines

Looks fine,

Reviewed-by Eric Sandeen <[email protected]>

Ben, having this in place for for the next point release will let userspace work & testing proceed w/o the need for a patched kernel... if you could consider pulling it in that'd be great.

Sounds reasonable. I'll check it out.

Quoted 4 lines

Dave, just out of curiosity, most other features sort of match between the "_has_*" and the flag names, is there a reason for the crc <-> sbv5 difference? Just semantics, but just curious.

Quoted 6 lines

(i.e. xfs_sb_version_hasprojid32bit checks XFS_SB_VERSION2_PROJID32BIT, but xfs_sb_version_hascrc checks XFS_SB_VERSION_5)

Answering my own question maybe, I guess SB_VERSION_5 was conceived with crc already in place, so there's no need for a feature flag on top of the sb version, right...?

Seems like we're also out of space in xfs_fsop_geom.flags.

There may even be
people who prefer to use v5 super blocks without crcs turned on, so maybe conflating the two ideas here is undesireable.

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Currently userspace has no way of determining that a filesystem is CRC enabled. Add a flag to the XFS_IOC_FSGEOMETRY ioctl output to indicate that the filesystem has v5 superblock support enabled. This will allow xfs_info to correctly report the state of the filesystem.

Looks fine,

Reviewed-by Eric Sandeen <[email protected]>

Ben, having this in place for for the next point release will let userspace work & testing proceed w/o the need for a patched kernel... if you could consider pulling it in that'd be great.

Dave, just out of curiosity, most other features sort of match between the "_has_*" and the flag names, is there a reason for the crc <-> sbv5 difference? Just semantics, but just curious.

(i.e. xfs_sb_version_hasprojid32bit checks XFS_SB_VERSION2_PROJID32BIT, but xfs_sb_version_hascrc checks XFS_SB_VERSION_5)

Answering my own question maybe, I guess SB_VERSION_5 was conceived with crc already in place, so there's no need for a feature flag on top of the sb version, right...?

From: Dave Chinner <[email protected]>

Currently userspace has no way of determining that a filesystem is CRC enabled. Add a flag to the XFS_IOC_FSGEOMETRY ioctl output to indicate that the filesystem has v5 superblock support enabled. This will allow xfs_info to correctly report the state of the filesystem.

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

---

 fs/xfs/xfs_fs.h    |    1 +
 fs/xfs/xfs_fsops.c |    4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

fs/xfs/xfs_fs.h +1 -0

index 6dda3f9..d046955 100644
@@ -236,6 +236,7 @@ typedef struct xfs_fsop_resblks {
 #define XFS_FSOP_GEOM_FLAGS_PROJID32	0x0800  /* 32-bit project IDs	*/
 #define XFS_FSOP_GEOM_FLAGS_DIRV2CI	0x1000	/* ASCII only CI names	*/
 #define XFS_FSOP_GEOM_FLAGS_LAZYSB	0x4000	/* lazy superblock counters */
+#define XFS_FSOP_GEOM_FLAGS_V5SB	0x8000	/* version 5 superblock */
 
 
 /*

fs/xfs/xfs_fsops.c +0 -0

index 87595b2..3c3644e 100644

Quoted 8 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Currently userspace has no way of determining that a filesystem is CRC enabled. Add a flag to the XFS_IOC_FSGEOMETRY ioctl output to indicate that the filesystem has v5 superblock support enabled. This will allow xfs_info to correctly report the state of the filesystem.

Quoted 2 lines

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

---

Reviewed-by Brian Foster <[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. This is trivial to do, and the new CRC gets logged with the inode core so on replay of the unlinked list operations this will be updated as expected and remain consistent.

Signed-off-by Dave Chinner <[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       |   42 ++++++++++++++++++++++++++++++++++++++----
 fs/xfs/xfs_log_recover.c |    9 +++++++++
 2 files changed, 47 insertions(+), 4 deletions(-)

fs/xfs/xfs_inode.c +6 -0

index efbe1ac..2d993e7 100644
@@ -1579,6 +1579,23 @@ out_bmap_cancel:
 }
 
 /*
+ * Helper function to calculate range of the inode to log and recalculate the
+ * on-disk inode crc if necessary.
+ */
+static int
+xfs_iunlink_dinode_calc_crc(
+	struct xfs_mount	*mp,

fs/xfs/xfs_inode.c +2 -0

+/*
  * This is called when the inode's link count goes to 0.
  * We place the on-disk inode on a list in the AGI.  It
  * will be pulled from this list when the inode is freed.

@@ -1638,10 +1655,15 @@ xfs_iunlink(
 		dip->di_next_unlinked = agi->agi_unlinked[bucket_index];
 		offset = ip->i_imap.im_boffset +
 			offsetof(xfs_dinode_t, di_next_unlinked);
+

fs/xfs/xfs_inode.c +15 -3

 	/*

@@ -1723,9 +1745,13 @@ 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_iunlink_dinode_calc_crc(mp, dip);
+
 			xfs_trans_inode_buf(tp, ibp);
 			xfs_trans_log_buf(tp, ibp, offset,
-					  (offset + sizeof(xfs_agino_t) - 1));
+					  offset + sizeof(xfs_agino_t) - 1);
 			xfs_inobp_check(mp, ibp);
 		} else {
 			xfs_trans_brelse(tp, ibp);

@@ -1796,9 +1822,13 @@ 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_iunlink_dinode_calc_crc(mp, dip);
+
 			xfs_trans_inode_buf(tp, ibp);
 			xfs_trans_log_buf(tp, ibp, offset,
-					  (offset + sizeof(xfs_agino_t) - 1));
+					  offset + sizeof(xfs_agino_t) - 1);
 			xfs_inobp_check(mp, ibp);
 		} else {
 			xfs_trans_brelse(tp, ibp);

@@ -1809,9 +1839,13 @@ 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_iunlink_dinode_calc_crc(mp, dip);
+
 		xfs_trans_inode_buf(tp, last_ibp);
 		xfs_trans_log_buf(tp, last_ibp, offset,
-				  (offset + sizeof(xfs_agino_t) - 1));
+				  offset + sizeof(xfs_agino_t) - 1);
 		xfs_inobp_check(mp, last_ibp);
 	}
 	return 0;

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 37 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       |   42 ++++++++++++++++++++++++++++++++++++++----
 fs/xfs/xfs_log_recover.c |    9 +++++++++
 2 files changed, 47 insertions(+), 4 deletions(-)

fs/xfs/xfs_inode.c +13 -0

index efbe1ac..2d993e7 100644
@@ -1579,6 +1579,23 @@ out_bmap_cancel:
 }
 
 /*
+ * Helper function to calculate range of the inode to log and recalculate the
+ * on-disk inode crc if necessary.
+ */
+static int
+xfs_iunlink_dinode_calc_crc(
+	struct xfs_mount	*mp,
+	struct xfs_dinode	*dip)
+{
+	if (dip->di_version < 3)
+		return sizeof(xfs_agino_t);
+
+	xfs_dinode_calc_crc(mp, dip);
+	return offsetof(struct xfs_dinode, di_changecount) -

fs/xfs/xfs_inode.c out_bmap_cancel: · line 1597

Quoted 3 lines

fs/xfs/xfs_inode.c +3 -0

+		offsetof(struct xfs_dinode, di_next_unlinked);
+}
+

So we've added a new helper, the return value for which is either the size of an inode number or an inode number + crc, depending on format. I also notice that the return value doesn't appear to be used anywhere this helper is called.

Quoted 10 lines

fs/xfs/xfs_inode.c +2 -0

+/*
  * This is called when the inode's link count goes to 0.
  * We place the on-disk inode on a list in the AGI.  It
  * will be pulled from this list when the inode is freed.

@@ -1638,10 +1655,15 @@ xfs_iunlink(
 		dip->di_next_unlinked = agi->agi_unlinked[bucket_index];
 		offset = ip->i_imap.im_boffset +
 			offsetof(xfs_dinode_t, di_next_unlinked);
+

fs/xfs/xfs_inode.c xfs_iunlink( · line 1667

Quoted 10 lines

fs/xfs/xfs_inode.c +5 -1

+		/* need to recalc the inode CRC if appropriate */
+		xfs_iunlink_dinode_calc_crc(mp, dip);
+
 		xfs_trans_inode_buf(tp, ibp);
 		xfs_trans_log_buf(tp, ibp, offset,
-				  (offset + sizeof(xfs_agino_t) - 1));
+				  offset + sizeof(xfs_agino_t) - 1);
 		xfs_inobp_check(mp, ibp);
+
 	}

So IIUC, offset is set to the offset of the di_next_unlinked value of this inode in the backing buffer of the inode (which we've just updated directly via dip).

We call the helper to update the CRC then call xfs_trans_log_buf() to log a range of the buffer. From the original code, I surmise that we're logging the range that represents the di_next_unlinked value and from the addition of the helper, I surmise we intend to now include the crc in that logged region.

But we haven't utilized the return value and I'm speculating on the intent here. So I see that we're updating the CRC, but is it actually logged? Perhaps I'm missing something, but if so, then why even have the xfs_iunlink_dinode_calc_crc() helper?

/me goes back to read the original 9/9 and followup:

http://oss.sgi.com/archives/xfs/2013-05/msg00867.html

OK, so in that case, perhaps the helper is now unnecessary and we could just call xfs_dinode_calc_crc()?

BTW, I was also going to ask here whether the fact that we update the CRC on recovery rather than logging it exposed items in the log to risk if they happened to become corrupted before that update occurs, but IIUC, we're still protected in that recovery itself should validate the existing on-disk CRC prior to the update. Correct?

From: Dave Chinner <[email protected]>

When the directory freespace index grows to a second block (2017 4k data blocks in the directory), the initialisation of the second new block header goes wrong. The write verifier fires a corruption error indicating that the block number in the header is zero. This was being tripped by xfs/110.

The problem is that the initialisation of the new block is done just fine in xfs_dir3_free_get_buf(), but the caller then users a dirv2 structure to zero on-disk header fields that xfs_dir3_free_get_buf() has already zeroed. These lined up with the block number in the dir v3 header format.

While looking at this, I noticed that the struct xfs_dir3_free_hdr() had 4 bytes of padding in it that wasn't defined as padding or being zeroed by the initialisation. Add a pad field declaration and fully zero the on disk and in-core headers in xfs_dir3_free_get_buf() so that this is never an issue in the future. Note that this doesn't change the on-disk layout, just makes the 32 bits of padding in the layout explicit.

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

---
 fs/xfs/xfs_dir2_format.h |    1 +
 fs/xfs/xfs_dir2_node.c   |   13 ++++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

fs/xfs/xfs_dir2_format.h +0 -0

index a3b1bd8..995f1f5 100644

Quoted 33 lines · Dave Chinner

From: Dave Chinner <[email protected]>

When the directory freespace index grows to a second block (2017 4k data blocks in the directory), the initialisation of the second new block header goes wrong. The write verifier fires a corruption error indicating that the block number in the header is zero. This was being tripped by xfs/110.

The problem is that the initialisation of the new block is done just fine in xfs_dir3_free_get_buf(), but the caller then users a dirv2 structure to zero on-disk header fields that xfs_dir3_free_get_buf() has already zeroed. These lined up with the block number in the dir v3 header format.

While looking at this, I noticed that the struct xfs_dir3_free_hdr() had 4 bytes of padding in it that wasn't defined as padding or being zeroed by the initialisation. Add a pad field declaration and fully zero the on disk and in-core headers in xfs_dir3_free_get_buf() so that this is never an issue in the future. Note that this doesn't change the on-disk layout, just makes the 32 bits of padding in the layout explicit.

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

---
 fs/xfs/xfs_dir2_format.h |    1 +
 fs/xfs/xfs_dir2_node.c   |   13 ++++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

fs/xfs/xfs_dir2_format.h +0 -0

index a3b1bd8..995f1f5 100644

fs/xfs/xfs_dir2_format.h struct xfs_dir3_free_hdr { · line 718

Quoted 4 lines

fs/xfs/xfs_dir2_format.h +1 -0

@@ -715,6 +715,7 @@ struct xfs_dir3_free_hdr {
 	__be32			firstdb;	/* db of first entry */
 	__be32			nvalid;		/* count of valid entries */
 	__be32			nused;		/* count of used entries */
+	__be32			pad;		/* 64 bit alignment. */

Yeah, my count also puts nused short of 64 bit alignment. Looks ok.

Reviewed-by Ben Myers <[email protected]>

This is an update for all the fixes I have for 3,10-rc4.

All the comments on the previous patches have been updated in this series, and there are two new patches. The first patch adds support for indicating that the filesystem has CRC enabled to userspace, and the second fixes the missing CRC updates on the unlinked inode list manipulations.

Hi Dave,

Quoted 2 lines · Dave Chinner

This is an update for all the fixes I have for 3,10-rc4.

Quoted 5 lines

All the comments on the previous patches have been updated in this series, and there are two new patches. The first patch adds support for indicating that the filesystem has CRC enabled to userspace, and the second fixes the missing CRC updates on the unlinked inode list manipulations.

This seems to be getting a bit out of hand for a release candidate pull request:

Fixes for xfs without crcs:
f648167f xfs: avoid nesting transactions in xfs_qm_scall_setqlim()
xfs: fix split buffer vector log recovery support
xfs: kill suid/sgid through the truncate path.

Fixes for xfs with crcs:
90253cf1 xfs: remote attribute allocation may be contiguous 913e96bc xfs: remote attribute read too short
4af3644c xfs: remote attribute tail zeroing does too much 6863ef84 xfs: correctly map remote attr buffers during removal 8517de2a xfs: fully initialise temp leaf in xfs_attr3_leaf_unbalance d4c712bc xfs: fully initialise temp leaf in xfs_attr3_leaf_compact

ad1858d7 xfs: rework remote attr CRCs

9a5b6dee xfs: fix incorrect remote symlink block count
xfs: rework dquot CRCs

xfs: inode unlinked list needs to recalculate the inode CRC * replaced with "2-3 patches"

xfs: fix dir3 freespace block corruption * maybe this effects filesystems w/o crcs?

Dev work for xfs with crcs:
xfs: increase number of ACL entries for V5 superblocks

xfs: don't emit v5 superblock warnings on write

xfs: add fsgeom flag for v5 superblock support.

xfs: disable swap extents ioctl on CRC enabled filesystems

We're representing 3 patches which are clearly approprate for a release candidate, 11 fixes for CRCs + 2 more for the rework of unlinked lists patch, and 4 patches which are development work.

We pulled in the CRC work with the understanding that it is an experimental feature that might not be perfect, and with a focus upon preventing regression for existing users. This did not imply that we're going after perfection for CRCs during the 3.10 release candidate series.

I'll be happy to review the 17 CRC related patches for inclusion in the master branch ASAP,

but I'm afraid 17 patches is a bit much
to ask for a release candidate, even if it were -rc2.

Here we are in 3.11/feature-bit territory.

Quoted 9 lines · Ben Myers

Quoted 7 lines · Dave Chinner

This is an update for all the fixes I have for 3,10-rc4.

All the comments on the previous patches have been updated in this series, and there are two new patches. The first patch adds support for indicating that the filesystem has CRC enabled to userspace, and the second fixes the missing CRC updates on the unlinked inode list manipulations.

Quoted 2 lines

This seems to be getting a bit out of hand for a release candidate pull request:

You don't need to send them all at once. The ones you committed from my initial -rc2 series could just be sent to Linus immediately, and that halves the current outstanding queue. Then there's only 8-9 patches remaining...

Quoted 4 lines

Fixes for xfs without crcs:
f648167f xfs: avoid nesting transactions in xfs_qm_scall_setqlim()
xfs: fix split buffer vector log recovery support
xfs: kill suid/sgid through the truncate path.

And the log recovery ordering fix I just sent out.

Quoted 7 lines

Fixes for xfs with crcs:
90253cf1 xfs: remote attribute allocation may be contiguous 913e96bc xfs: remote attribute read too short
4af3644c xfs: remote attribute tail zeroing does too much 6863ef84 xfs: correctly map remote attr buffers during removal 8517de2a xfs: fully initialise temp leaf in xfs_attr3_leaf_unbalance d4c712bc xfs: fully initialise temp leaf in xfs_attr3_leaf_compact

All of which fix filesystem corruption or - in the case of the buffer cache coherency problems - can lead to stale data exposure.

Quoted 1 line

ad1858d7 xfs: rework remote attr CRCs

Fixes cache coherency issues via a change in the on-disk format for remote attributes. We need to get that included before a full release is made, otherwise we need feature bits to distinguish between the original and the new formats.

Quoted 2 lines

9a5b6dee xfs: fix incorrect remote symlink block count
xfs: rework dquot CRCs

More filesystem corruption fixes.

Quoted 1 line

xfs: inode unlinked list needs to recalculate the inode CRC * replaced with "2-3 patches"

Another corruption fix, now with an addition log recovery ordering fix which prevents log recovery from potentially causing silent filesystem corruption.

Quoted 1 line

xfs: fix dir3 freespace block corruption * maybe this effects filesystems w/o crcs?

Again, a filesystem corruption, including fixing a problem in the on-disk format definition. While it probably doesn't affect non-crc filesystems, it's a landmine that should be removed....

Quoted 2 lines

Dev work for xfs with crcs:
xfs: increase number of ACL entries for V5 superblocks

On disk format change. We need to get that out before release, otherwise it needs a feature bit.

Quoted 1 line

xfs: don't emit v5 superblock warnings on write

Fixes a nasty log spamming problem. Certainly not development code.

Quoted 1 line

xfs: add fsgeom flag for v5 superblock support.

Needed for userspace support of CRC filesystems, trivial patch. if we expect anyone to test this kernel code, we need this patch so userspace can correctly identify crc-enabled filesystems.

Quoted 1 line

xfs: disable swap extents ioctl on CRC enabled filesystems

Filesystem corruption fix, caused simply by running xfs_fsr.

But, really, this entire series is not that much change in terms of volume:

$ git diff --stat b38958d..bf6331a -- fs/xfs
 fs/xfs/xfs_acl.c         |   31 ++--
 fs/xfs/xfs_acl.h         |   30 +++-
 fs/xfs/xfs_attr_leaf.c   |   74 ++++++---
 fs/xfs/xfs_attr_remote.c |  408 +++++++++++++++++++++++++++++-------------------
 fs/xfs/xfs_attr_remote.h |   10 ++
 fs/xfs/xfs_buf.c         |    1 +
 fs/xfs/xfs_buf_item.c    |    7 +-
 fs/xfs/xfs_dfrag.c       |    8 +
 fs/xfs/xfs_dir2_format.h |    1 +
 fs/xfs/xfs_dir2_node.c   |   13 +-
 fs/xfs/xfs_dquot.c       |   37 ++---
 fs/xfs/xfs_fs.h          |    1 +
 fs/xfs/xfs_fsops.c       |    4 +-
 fs/xfs/xfs_inode.c       |   42 ++++-
 fs/xfs/xfs_iops.c        |   47 ++++--
 fs/xfs/xfs_log_recover.c |   95 ++++++++++-
 fs/xfs/xfs_mount.c       |   18 ++-
 fs/xfs/xfs_qm.c          |   36 +++--
 fs/xfs/xfs_qm_syscalls.c |   40 +++--
 fs/xfs/xfs_quota.h       |    2 +
 fs/xfs/xfs_symlink.c     |   20 +--
 21 files changed, 612 insertions(+), 313 deletions(-)

And a large proportion of that change is the single attr CRC rework patch.

Quoted 3 lines

We're representing 3 patches which are clearly approprate for a release candidate, 11 fixes for CRCs + 2 more for the rework of unlinked lists patch, and 4 patches which are development work.

Most file filesystem corruptions. Without those fixes, CRC enabled filesystems simple cannot be used without all these patches being applied. It's not an experimental feature at this point - it's just broken.

Quoted 5 lines

We pulled in the CRC work with the understanding that it is an experimental feature that might not be perfect, and with a focus upon preventing regression for existing users. This did not imply that we're going after perfection for CRCs during the 3.10 release candidate series.

Right, but there was also the understanding that the CRC code would be usable by release time, and so there would be fixes committed throughout the -rc series to fix problems with the CRC code so that it is usable.

As it is, the result of all these patches is that xfstests is almost entirely passing on a 4k block size filesystem. The only tests that aren't passing are those that already fail or require userspace support that isn't currently available (e.g. xfs-db write support, metadump). And 1k block size filesystems mostly pass xfstests, too, though there are random assert failures from time to time.

IOWs after this patch set is applied, the CRC functionality fits the definition of the 'experimental' tag. It mostly works, but still has some lurking issues that need to be flushed out. I'm unlikely to be posting 2-3 bug fix patches a day from this point onwards....

Indeed, the last two days I've been doing 3.11 development work because xfstests on CRC enabled filesystems is now usable for regression testing. i.e. the majority of the "easy-to-hit" problems have been flushed out by this patchset.

Quoted 2 lines

I'll be happy to review the 17 CRC related patches for inclusion in the master branch ASAP,

That's a good start, but...

Quoted 2 lines

but I'm afraid 17 patches is a bit much
to ask for a release candidate, even if it were -rc2.

... I think you're being way too conservative here.

BTRFS in january for an -rc merge:

https://lkml.org/lkml/2013/1/25/11

That was 30 commits and +300/-100 lines. There was a another pull request 2 weeks later with another 9 commits. The same thing happened with the RAID5/6 integration into btrfs in 3.9 - the -rc merges following it had 13, 7 and 15 commits. And for 3.10-rc, the first post-rc1 merge was 25 commits (+350/-250 lines).

And if you want another example, the ext4 extent status cache fixes that went into 3.9-rc4:

https://lkml.org/lkml/2013/3/21/644

that was 21 commits and +540/-81 lines.

Yup, new filesystem code has bugs, and they get fixed in -rc releases. What makes new XFS code so special we can't do this?

The amount of change we are talking about here is not unreasonable even for an -rc4 release, as evidenced by other filesystems under development. Hence I don't think "it's too late" is really a viable reason for not fixing all these filesystem corruption issues in the CRC code...

Quoted 1 line

Here we are in 3.11/feature-bit territory.

I'd much prefer that we don't have to add code to 3.11 to reject any CRC-enabled filesystem without any feature bits set because we don't support a broken remote attr format that was fixed weeks before 3.10 released but was not allowed to be fixed in 3.10. That's just crazy from any release management perspective you care to look at it from.

Ben, if the problem is that you can't review all the fixes in a timely manner, then we can fix that. I'm sure that Mark, Eric and Brian can help review the code if this is the sticking point.

There are also
people like Michael who are actively testing the CRC code and reporting bugs to me all the time, so there's demand for these fixes to be integrated into the 3.10 tree...

Everyone knew there would be fixes for the CRC code coming along in the -rc period - we discussed it on the weekly concall before the the CRC code was merged into 3.10. Hence I'm a bit surprised to get what appears to be a blanket rejection for these fixes to the CRC code this early in the 3.10-rc cycle.

Let's not waste all the effort that we put into getting the CRC code into 3.10 by not fixing known corruptions and on-disk format deficiencies.

Releasing CRC support in a seriously broken state that we need to add permanent workarounds for in 3.11 is about the worst possible outcome that could occur right now....

Hi Dave,

On Wed, May 29, 2013 at 09:54:24AM +1000, Dave Chinner wrote:

Quoted 5 lines

I'd much prefer that we don't have to add code to 3.11 to reject any CRC-enabled filesystem without any feature bits set because we don't support a broken remote attr format that was fixed weeks before 3.10 released but was not allowed to be fixed in 3.10. That's just crazy from any release management perspective you care to look at it from.

So would I.

Quoted 3 lines

Ben, if the problem is that you can't review all the fixes in a timely manner, then we can fix that. I'm sure that Mark, Eric and Brian can help review the code if this is the sticking point.

Reviews are always welcome...

A worse outcome is that I pull in this code and something goes very wrong for the thousands of users of 3.10 with existing non-crc XFS filesystems. A feature bit and some inconvenience for a few XFS developers and testers is a safer choice.

Regards,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 3 lines · Ben Myers

Hi Dave,

On Wed, May 29, 2013 at 09:54:24AM +1000, Dave Chinner wrote:

<giant snip>

Quoted 8 lines

Quoted 5 lines

I'd much prefer that we don't have to add code to 3.11 to reject any CRC-enabled filesystem without any feature bits set because we don't support a broken remote attr format that was fixed weeks before 3.10 released but was not allowed to be fixed in 3.10. That's just crazy from any release management perspective you care to look at it from.

So would I.

Quoted 5 lines

Quoted 3 lines

Ben, if the problem is that you can't review all the fixes in a timely manner, then we can fix that. I'm sure that Mark, Eric and Brian can help review the code if this is the sticking point.

Reviews are always welcome...

But it won't matter for the sake of this argument, sounds like?

Quoted 4 lines

A worse outcome is that I pull in this code and something goes very wrong for the thousands of users of 3.10 with existing non-crc XFS filesystems. A feature bit and some inconvenience for a few XFS developers and testers is a safer choice.

Your concern (rightly) seems to be stability for non-crc users, so:

I'll review these patches with a special eye towards if/how they affect any non-crc codepaths. If it's wholly contained in crc code, you can merge them without fear. Sound like a deal?