[PATCH 0/6] xfs: fixes for 3.10-rc4

[PATCH 0/6] xfs: fixes for 3.10-rc4

[Dave Chinner]

Hi Ben,

He's an up-to-date set of patches remaining for 3.10. It fixes the
dquot crc calc that Brain pointed out, as well are removes the
unnecessary xfs_iunlink_dinode_calc_crc() function.

There's also a new patch that deals with rejecting the noattr2 mount
option for CRC enabled filesystems. attr2 is always enabled for
them, so the noattr2 mount option needs to be rejected as invalid.

Cheers,

Dave.

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

[PATCH 1/6] xfs: rework dquot CRCs

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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 <dchinner@redhat.com>
---
 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(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index a41f8bf..044e97a 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -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);
 	}
 
 	/*
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index d9e4d3c..d6204d1 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -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);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index f41702b..b75c9bb 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -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;
 	}
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index c61e31c..c38068f 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -87,6 +87,8 @@ typedef struct xfs_dqblk {
 	uuid_t		  dd_uuid;	/* location information */
 } xfs_dqblk_t;
 
+#define XFS_DQUOT_CRC_OFF	offsetof(struct xfs_dqblk, dd_crc)
+
 /*
  * flags for q_flags field in the dquot.
  */
-- 
1.7.10.4

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

Re: [PATCH 1/6] xfs: rework dquot CRCs

[Brian Foster]

On 06/03/2013 01:28 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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 <dchinner@redhat.com>

Reviewed-by: Brian Foster <bfoster@redhat.com>

> ---
>  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(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index a41f8bf..044e97a 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -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);
>  	}
>  
>  	/*
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index d9e4d3c..d6204d1 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -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);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index f41702b..b75c9bb 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -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;
>  	}
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index c61e31c..c38068f 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -87,6 +87,8 @@ typedef struct xfs_dqblk {
>  	uuid_t		  dd_uuid;	/* location information */
>  } xfs_dqblk_t;
>  
> +#define XFS_DQUOT_CRC_OFF	offsetof(struct xfs_dqblk, dd_crc)
> +
>  /*
>   * flags for q_flags field in the dquot.
>   */
> 

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

Re: [PATCH 1/6] xfs: rework dquot CRCs

[Ben Myers]

On Mon, Jun 03, 2013 at 02:18:33PM -0400, Brian Foster wrote:
> On 06/03/2013 01:28 AM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > 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 <dchinner@redhat.com>
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > ---
> >  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(-)
> > 
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index a41f8bf..044e97a 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -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,

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 <bpm@sgi.com>

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

Re: [PATCH 1/6] xfs: rework dquot CRCs

[Ben Myers]

On Tue, Jun 04, 2013 at 04:46:12PM -0500, Ben Myers wrote:
> On Mon, Jun 03, 2013 at 02:18:33PM -0400, Brian Foster wrote:
> > On 06/03/2013 01:28 AM, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > 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 <dchinner@redhat.com>
> > 
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > 
> > > ---
> > >  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(-)
> > > 
> > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > > index a41f8bf..044e97a 100644
> > > --- a/fs/xfs/xfs_dquot.c
> > > +++ b/fs/xfs/xfs_dquot.c
> > > @@ -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,
> 
> 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 <bpm@sgi.com>

No Ben that is only on error.  ;)

Applied.

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

[PATCH 2/6] xfs: fix log recovery transaction item reordering

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

There are several constraints that inode allocation and unlink
logging impose on log recovery. These all stem from the fact that
inode alloc/unlink are logged in buffers, but all other inode
changes are logged in inode items. Hence there are ordering
constraints that recovery must follow to ensure the correct result
occurs.

As it turns out, this ordering has been working mostly by chance
than good management. The existing code moves all buffers except
cancelled buffers to the head of the list, and everything else to
the tail of the list. The problem with this is that is interleaves
inode items with the buffer cancellation items, and hence whether
the inode item in an cancelled buffer gets replayed is essentially
left to chance.

Further, this ordering causes problems for log recovery when inode
CRCs are enabled. It typically replays the inode unlink buffer long before
it replays the inode core changes, and so the CRC recorded in an
unlink buffer is going to be invalid and hence any attempt to
validate the inode in the buffer is going to fail. Hence we really
need to enforce the ordering that the inode alloc/unlink code has
expected log recovery to have since inode chunk de-allocation was
introduced back in 2003...

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_recover.c |   65 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index d6204d1..83088d9 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1599,10 +1599,43 @@ xlog_recover_add_to_trans(
 }
 
 /*
- * Sort the log items in the transaction. Cancelled buffers need
- * to be put first so they are processed before any items that might
- * modify the buffers. If they are cancelled, then the modifications
- * don't need to be replayed.
+ * Sort the log items in the transaction.
+ *
+ * The ordering constraints are defined by the inode allocation and unlink
+ * behaviour. The rules are:
+ *
+ *	1. Every item is only logged once in a given transaction. Hence it
+ *	   represents the last logged state of the item. Hence ordering is
+ *	   dependent on the order in which operations need to be performed so
+ *	   required initial conditions are always met.
+ *
+ *	2. Cancelled buffers are recorded in pass 1 in a separate table and
+ *	   there's nothing to replay from them so we can simply cull them
+ *	   from the transaction. However, we can't do that until after we've
+ *	   replayed all the other items because they may be dependent on the
+ *	   cancelled buffer and replaying the cancelled buffer can remove it
+ *	   form the cancelled buffer table. Hence they have tobe done last.
+ *
+ *	3. Inode allocation buffers must be replayed before inode items that
+ *	   read the buffer and replay changes into it.
+ *
+ *	4. Inode unlink buffers must be replayed after inode items are replayed.
+ *	   This ensures that inodes are completely flushed to the inode buffer
+ *	   in a "free" state before we remove the unlinked inode list pointer.
+ *
+ * Hence the ordering needs to be inode allocation buffers first, inode items
+ * second, inode unlink buffers third and cancelled buffers last.
+ *
+ * But there's a problem with that - we can't tell an inode allocation buffer
+ * apart from a regular buffer, so we can't separate them. We can, however,
+ * tell an inode unlink buffer from the others, and so we can separate them out
+ * from all the other buffers and move them to last.
+ *
+ * Hence, 4 lists, in order from head to tail:
+ * 	- buffer_list for all buffers except cancelled/inode unlink buffers
+ * 	- item_list for all non-buffer items
+ * 	- inode_buffer_list for inode unlink buffers
+ * 	- cancel_list for the cancelled buffers
  */
 STATIC int
 xlog_recover_reorder_trans(
@@ -1612,6 +1645,10 @@ xlog_recover_reorder_trans(
 {
 	xlog_recover_item_t	*item, *n;
 	LIST_HEAD(sort_list);
+	LIST_HEAD(cancel_list);
+	LIST_HEAD(buffer_list);
+	LIST_HEAD(inode_buffer_list);
+	LIST_HEAD(inode_list);
 
 	list_splice_init(&trans->r_itemq, &sort_list);
 	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
@@ -1619,12 +1656,18 @@ xlog_recover_reorder_trans(
 
 		switch (ITEM_TYPE(item)) {
 		case XFS_LI_BUF:
-			if (!(buf_f->blf_flags & XFS_BLF_CANCEL)) {
+			if (buf_f->blf_flags & XFS_BLF_CANCEL) {
 				trace_xfs_log_recover_item_reorder_head(log,
 							trans, item, pass);
-				list_move(&item->ri_list, &trans->r_itemq);
+				list_move(&item->ri_list, &cancel_list);
 				break;
 			}
+			if (buf_f->blf_flags & XFS_BLF_INODE_BUF) {
+				list_move(&item->ri_list, &inode_buffer_list);
+				break;
+			}
+			list_move_tail(&item->ri_list, &buffer_list);
+			break;
 		case XFS_LI_INODE:
 		case XFS_LI_DQUOT:
 		case XFS_LI_QUOTAOFF:
@@ -1632,7 +1675,7 @@ xlog_recover_reorder_trans(
 		case XFS_LI_EFI:
 			trace_xfs_log_recover_item_reorder_tail(log,
 							trans, item, pass);
-			list_move_tail(&item->ri_list, &trans->r_itemq);
+			list_move_tail(&item->ri_list, &inode_list);
 			break;
 		default:
 			xfs_warn(log->l_mp,
@@ -1643,6 +1686,14 @@ xlog_recover_reorder_trans(
 		}
 	}
 	ASSERT(list_empty(&sort_list));
+	if (!list_empty(&buffer_list))
+		list_splice(&buffer_list, &trans->r_itemq);
+	if (!list_empty(&inode_list))
+		list_splice_tail(&inode_list, &trans->r_itemq);
+	if (!list_empty(&inode_buffer_list))
+		list_splice_tail(&inode_buffer_list, &trans->r_itemq);
+	if (!list_empty(&cancel_list))
+		list_splice_tail(&cancel_list, &trans->r_itemq);
 	return 0;
 }
 
-- 
1.7.10.4

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

[PATCH 3/6] xfs: inode unlinked list needs to recalculate the inode CRC

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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 <dchinner@redhat.com>
---
 fs/xfs/xfs_inode.c       |   16 ++++++++++++++++
 fs/xfs/xfs_log_recover.c |    9 +++++++++
 2 files changed, 25 insertions(+)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index efbe1ac..c50e785 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -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));
@@ -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);
+
 		xfs_trans_inode_buf(tp, last_ibp);
 		xfs_trans_log_buf(tp, last_ibp, offset,
 				  (offset + sizeof(xfs_agino_t) - 1));
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 83088d9..45a85ff 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -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;
+
+		/*
+		 * If necessary, recalculate the CRC in the on-disk inode. We
+		 * have to leave the inode in a consistent state for whoever
+		 * reads it next....
+		 */
+		xfs_dinode_calc_crc(mp, (struct xfs_dinode *)
+				xfs_buf_offset(bp, i * mp->m_sb.sb_inodesize));
+
 	}
 
 	return 0;
-- 
1.7.10.4

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

Re: [PATCH 3/6] xfs: inode unlinked list needs to recalculate the inode CRC

[Brian Foster]

On 06/03/2013 01:28 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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 <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode.c       |   16 ++++++++++++++++
>  fs/xfs/xfs_log_recover.c |    9 +++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index efbe1ac..c50e785 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -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));
> @@ -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.

Brian

>  		xfs_trans_inode_buf(tp, last_ibp);
>  		xfs_trans_log_buf(tp, last_ibp, offset,
>  				  (offset + sizeof(xfs_agino_t) - 1));
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 83088d9..45a85ff 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -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;
> +
> +		/*
> +		 * If necessary, recalculate the CRC in the on-disk inode. We
> +		 * have to leave the inode in a consistent state for whoever
> +		 * reads it next....
> +		 */
> +		xfs_dinode_calc_crc(mp, (struct xfs_dinode *)
> +				xfs_buf_offset(bp, i * mp->m_sb.sb_inodesize));
> +
>  	}
>  
>  	return 0;
> 

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

Re: [PATCH 3/6] xfs: inode unlinked list needs to recalculate the inode CRC

[Dave Chinner]

On Mon, Jun 03, 2013 at 02:18:43PM -0400, Brian Foster wrote:
> On 06/03/2013 01:28 AM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > 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 <dchinner@redhat.com>
.....
> > @@ -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.

Yup, it should - good catch. This just highlights how hard it is to
actaully catch a filesystem in the state with a corrupt CRC on the
unlinked list...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 4/6] xfs: fix remote attribute invalidation for a leaf

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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 <dchinner@redhat.com>
---
 fs/xfs/xfs_attr_leaf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index d788302..31d3cd1 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -3258,7 +3258,7 @@ xfs_attr3_leaf_inactive(
 			name_rmt = xfs_attr3_leaf_name_remote(leaf, i);
 			if (name_rmt->valueblk) {
 				lp->valueblk = be32_to_cpu(name_rmt->valueblk);
-				lp->valuelen = XFS_B_TO_FSB(dp->i_mount,
+				lp->valuelen = xfs_attr3_rmt_blocks(dp->i_mount,
 						    be32_to_cpu(name_rmt->valuelen));
 				lp++;
 			}
-- 
1.7.10.4

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

Re: [PATCH 4/6] xfs: fix remote attribute invalidation for a leaf

[Brian Foster]

On 06/03/2013 01:28 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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 <dchinner@redhat.com>

Reviewed-by: Brian Foster <bfoster@redhat.com>

> ---
>  fs/xfs/xfs_attr_leaf.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
> index d788302..31d3cd1 100644
> --- a/fs/xfs/xfs_attr_leaf.c
> +++ b/fs/xfs/xfs_attr_leaf.c
> @@ -3258,7 +3258,7 @@ xfs_attr3_leaf_inactive(
>  			name_rmt = xfs_attr3_leaf_name_remote(leaf, i);
>  			if (name_rmt->valueblk) {
>  				lp->valueblk = be32_to_cpu(name_rmt->valueblk);
> -				lp->valuelen = XFS_B_TO_FSB(dp->i_mount,
> +				lp->valuelen = xfs_attr3_rmt_blocks(dp->i_mount,
>  						    be32_to_cpu(name_rmt->valuelen));
>  				lp++;
>  			}
> 

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

Re: [PATCH 4/6] xfs: fix remote attribute invalidation for a leaf

[Mark Tinguely]

On 06/03/13 00:28, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> 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<dchinner@redhat.com>

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

Reviewed-by: Mark Tinguely <tinuguely@sgi.com>

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

Re: [PATCH 4/6] xfs: fix remote attribute invalidation for a leaf

[Dave Chinner]

On Mon, Jun 03, 2013 at 02:09:48PM -0500, Mark Tinguely wrote:
> On 06/03/13 00:28, Dave Chinner wrote:
> >From: Dave Chinner<dchinner@redhat.com>
> >
> >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<dchinner@redhat.com>
> 
> 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.

Yeah, good point.

This is one of the reasons why I dislike comments explaining what
variables in structures mean. I didn't even look at the definition
of the structure, because it's meaning is obvious from the name of
the varaible of the code that uses it.  ;)

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

Actaully, a structure that is used once and local to a single
function shouldn't be declared in a header file - if should be local
to the function. I'll fix this in a separate patch for 3.11.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 4/6] xfs: fix remote attribute invalidation for a leaf

[Ben Myers]

On Mon, Jun 03, 2013 at 03:28:49PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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 <dchinner@redhat.com>

Applied.  Looks like the rest need to be reposted due to various review
comments and, #2 still needs review.

-Ben

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

[PATCH 5/6] xfs: disable noattr2/attr2 mount options for CRC enabled filesystems

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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 <dchinner@redhat.com>
---
 Documentation/filesystems/xfs.txt |    3 +++
 fs/xfs/xfs_super.c                |   11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
index 3e4b3dd..83577f0 100644
--- a/Documentation/filesystems/xfs.txt
+++ b/Documentation/filesystems/xfs.txt
@@ -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
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ea341ce..f62abb2 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1373,6 +1373,17 @@ xfs_finish_flags(
 	}
 
 	/*
+	 * CRC enabled filesystems always use attr2 format for attributes.
+	 */
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	    (mp->m_flags & XFS_MOUNT_NOATTR2)) {
+		xfs_warn(mp,
+"Cannot mount a V5 filesystems as %s. %s is always enabled for v5 filesystems.",
+			MNTOPT_NOATTR2, MNTOPT_ATTR2);
+		return XFS_ERROR(EINVAL);
+	}
+
+	/*
 	 * mkfs'ed attr2 will turn on attr2 mount unless explicitly
 	 * told by noattr2 to turn it off
 	 */
-- 
1.7.10.4

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

Re: [PATCH 5/6] xfs: disable noattr2/attr2 mount options for CRC enabled filesystems

[Brian Foster]

On 06/03/2013 01:28 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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 <dchinner@redhat.com>
> ---
>  Documentation/filesystems/xfs.txt |    3 +++
>  fs/xfs/xfs_super.c                |   11 +++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
> index 3e4b3dd..83577f0 100644
> --- a/Documentation/filesystems/xfs.txt
> +++ b/Documentation/filesystems/xfs.txt
> @@ -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
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ea341ce..f62abb2 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1373,6 +1373,17 @@ xfs_finish_flags(
>  	}
>  
>  	/*
> +	 * CRC enabled filesystems always use attr2 format for attributes.
> +	 */
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	    (mp->m_flags & XFS_MOUNT_NOATTR2)) {
> +		xfs_warn(mp,
> +"Cannot mount a V5 filesystems as %s. %s is always enabled for v5 filesystems.",
		      filesystem

(or alternatively remove the 'a').

... and V5 and v5 is inconsistent, fwiw. :P Nits, nonetheless...

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +			MNTOPT_NOATTR2, MNTOPT_ATTR2);
> +		return XFS_ERROR(EINVAL);
> +	}
> +
> +	/*
>  	 * mkfs'ed attr2 will turn on attr2 mount unless explicitly
>  	 * told by noattr2 to turn it off
>  	 */
> 

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

Re: [PATCH 5/6] xfs: disable noattr2/attr2 mount options for CRC enabled filesystems

[Mark Tinguely]

On 06/03/13 00:28, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> 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<dchinner@redhat.com>
> ---
>   Documentation/filesystems/xfs.txt |    3 +++
>   fs/xfs/xfs_super.c                |   11 +++++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
> index 3e4b3dd..83577f0 100644
> --- a/Documentation/filesystems/xfs.txt
> +++ b/Documentation/filesystems/xfs.txt
> @@ -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
Filesystems using the version 5 superblock always use the attr2 format, 
and so
> +	will reject the noattr2 mount option if it is set.
  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
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ea341ce..f62abb2 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1373,6 +1373,17 @@ xfs_finish_flags(
>   	}
>
>   	/*
> +	 * CRC enabled filesystems always use attr2 format for attributes.

             V5 filesystems always use attr2 format for attributes.

> +	 */
> +	if (xfs_sb_version_hascrc(&mp->m_sb)&&

...

--Mark.

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

Re: [PATCH 5/6] xfs: disable noattr2/attr2 mount options for CRC enabled filesystems

[Dave Chinner]

On Mon, Jun 03, 2013 at 04:38:30PM -0500, Mark Tinguely wrote:
> On 06/03/13 00:28, Dave Chinner wrote:
> >From: Dave Chinner<dchinner@redhat.com>
> >
> >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<dchinner@redhat.com>
> >---
> >  Documentation/filesystems/xfs.txt |    3 +++
> >  fs/xfs/xfs_super.c                |   11 +++++++++++
> >  2 files changed, 14 insertions(+)
> >
> >diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
> >index 3e4b3dd..83577f0 100644
> >--- a/Documentation/filesystems/xfs.txt
> >+++ b/Documentation/filesystems/xfs.txt
> >@@ -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
> Filesystems using the version 5 superblock always use the attr2
> format, and so
> >+	will reject the noattr2 mount option if it is set.
>  reject the noattr2 mount option if it is set.
> >+

I'm going to leave that as "CRC enabled" as the mkfs option that
turns on V5 superblocks is "-m crc=1". IOWs, users aren't going to
know that V5 superblocks == CRC enabled, so the docco reflects
that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The limit of 25 ACL entries is arbitrary, but baked into the on-disk
format.  For version 5 superblocks, increase it to the maximum nuber
of ACLs that can fit into a single xattr.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_acl.c |   31 ++++++++++++++++++-------------
 fs/xfs/xfs_acl.h |   30 +++++++++++++++++++++++-------
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 1d32f1d..58b6da3 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -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,9 +112,9 @@ 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);
 	unsigned char *ea_name;
 	int error;
+	int len;
 
 	acl = get_cached_acl(inode, type);
 	if (acl != ACL_NOT_CACHED)
@@ -133,8 +137,8 @@ xfs_get_acl(struct inode *inode, int type)
 	 * If we have a cached ACLs value just return it, not need to
 	 * go out to the disk.
 	 */
-
-	xfs_acl = kzalloc(sizeof(struct xfs_acl), GFP_KERNEL);
+	len = XFS_ACL_SIZE(ip->i_mount);
+	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) {
diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
index 39632d9..0da8725 100644
--- a/fs/xfs/xfs_acl.h
+++ b/fs/xfs/xfs_acl.h
@@ -22,19 +22,35 @@ struct inode;
 struct posix_acl;
 struct xfs_inode;
 
-#define XFS_ACL_MAX_ENTRIES 25
 #define XFS_ACL_NOT_PRESENT (-1)
 
 /* On-disk XFS access control list structure */
+struct xfs_acl_entry {
+	__be32	ae_tag;
+	__be32	ae_id;
+	__be16	ae_perm;
+	__be16	ae_pad;		/* fill the implicit hole in the structure */
+};
+
 struct xfs_acl {
-	__be32		acl_cnt;
-	struct xfs_acl_entry {
-		__be32	ae_tag;
-		__be32	ae_id;
-		__be16	ae_perm;
-	} acl_entry[XFS_ACL_MAX_ENTRIES];
+	__be32			acl_cnt;
+	struct xfs_acl_entry	acl_entry[0];
 };
 
+/*
+ * The number of ACL entries allowed is defined by the on-disk format.
+ * For v4 superblocks, that is limited to 25 entries. For v5 superblocks, it is
+ * limited only by the maximum size of the xattr that stores the information.
+ */
+#define XFS_ACL_MAX_ENTRIES(mp)	\
+	(xfs_sb_version_hascrc(&mp->m_sb) \
+	   ?  (XATTR_SIZE_MAX - sizeof(__be32)) / sizeof(struct xfs_acl_entry) \
+	   : 25)
+
+#define XFS_ACL_SIZE(mp) \
+	(sizeof(struct xfs_acl) + \
+		sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))
+
 /* On-disk XFS extended attribute names */
 #define SGI_ACL_FILE		(unsigned char *)"SGI_ACL_FILE"
 #define SGI_ACL_DEFAULT		(unsigned char *)"SGI_ACL_DEFAULT"
-- 
1.7.10.4

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

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

[Mark Tinguely]

On 06/03/13 00:28, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> The limit of 25 ACL entries is arbitrary, but baked into the on-disk
> format.  For version 5 superblocks, increase it to the maximum nuber
> of ACLs that can fit into a single xattr.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> Reviewed-by: Brian Foster<bfoster@redhat.com>
> ---

> @@ -189,16 +193,17 @@ xfs_set_acl(struct inode *inode, int type, struct posix_acl *acl)
>
>   	if (acl) {
>   		struct xfs_acl *xfs_acl;
> -		int len;
> +		int len = XFS_ACL_SIZE(ip->i_mount);
>
> -		xfs_acl = kzalloc(sizeof(struct xfs_acl), GFP_KERNEL);
> +		xfs_acl = kzalloc(len, GFP_KERNEL);

Isn't that physical contiguous allocator? wouldn't a virtual contiguous 
be good enough for the acl?

...

>   	if (type == ACL_TYPE_ACCESS) {
> diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
> index 39632d9..0da8725 100644
> --- a/fs/xfs/xfs_acl.h
> +++ b/fs/xfs/xfs_acl.h
> @@ -22,19 +22,35 @@ struct inode;
>   struct posix_acl;
>   struct xfs_inode;
>
> -#define XFS_ACL_MAX_ENTRIES 25
>   #define XFS_ACL_NOT_PRESENT (-1)
>
>   /* On-disk XFS access control list structure */
> +struct xfs_acl_entry {
> +	__be32	ae_tag;
> +	__be32	ae_id;
> +	__be16	ae_perm;
> +	__be16	ae_pad;		/* fill the implicit hole in the structure */
> +};
> +
>   struct xfs_acl {
> -	__be32		acl_cnt;
> -	struct xfs_acl_entry {
> -		__be32	ae_tag;
> -		__be32	ae_id;
> -		__be16	ae_perm;
> -	} acl_entry[XFS_ACL_MAX_ENTRIES];
> +	__be32			acl_cnt;
> +	struct xfs_acl_entry	acl_entry[0];
>   };
>
> +/*
> + * The number of ACL entries allowed is defined by the on-disk format.
> + * For v4 superblocks, that is limited to 25 entries. For v5 superblocks, it is
> + * limited only by the maximum size of the xattr that stores the information.
> + */
> +#define XFS_ACL_MAX_ENTRIES(mp)	\
> +	(xfs_sb_version_hascrc(&mp->m_sb) \
> +	   ?  (XATTR_SIZE_MAX - sizeof(__be32)) / sizeof(struct xfs_acl_entry) \
> +	   : 25)


XFS_ACL_MAX_ENTRIES(mp)  == (65536 - 4) / 12 == 5461

> +
> +#define XFS_ACL_SIZE(mp) \
> +	(sizeof(struct xfs_acl) + \
> +		sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))

XFS_ACL_SIZE(mp) == (4 + 12) + 12 * ((64K - 4) / 12) == 65548

Did you want to add in the sizeof(struct xfs_acl) to the first term or 
the sizeof(__be32)? I would think the acl_entry[0] is the start of the 
array.

> +
>   /* On-disk XFS extended attribute names */
>   #define SGI_ACL_FILE		(unsigned char *)"SGI_ACL_FILE"
>   #define SGI_ACL_DEFAULT		(unsigned char *)"SGI_ACL_DEFAULT"

--Mark.

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

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

[Dave Chinner]

On Mon, Jun 03, 2013 at 05:08:38PM -0500, Mark Tinguely wrote:
> On 06/03/13 00:28, Dave Chinner wrote:
> >From: Dave Chinner<dchinner@redhat.com>
> >
> >The limit of 25 ACL entries is arbitrary, but baked into the on-disk
> >format.  For version 5 superblocks, increase it to the maximum nuber
> >of ACLs that can fit into a single xattr.
> >
> >Signed-off-by: Dave Chinner<dchinner@redhat.com>
> >Reviewed-by: Brian Foster<bfoster@redhat.com>
> >---
> 
> >@@ -189,16 +193,17 @@ xfs_set_acl(struct inode *inode, int type, struct posix_acl *acl)
> >
> >  	if (acl) {
> >  		struct xfs_acl *xfs_acl;
> >-		int len;
> >+		int len = XFS_ACL_SIZE(ip->i_mount);
> >
> >-		xfs_acl = kzalloc(sizeof(struct xfs_acl), GFP_KERNEL);
> >+		xfs_acl = kzalloc(len, GFP_KERNEL);
> 
> Isn't that physical contiguous allocator? wouldn't a virtual
> contiguous be good enough for the acl?

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

> >  	if (type == ACL_TYPE_ACCESS) {
> >diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
> >index 39632d9..0da8725 100644
> >--- a/fs/xfs/xfs_acl.h
> >+++ b/fs/xfs/xfs_acl.h
> >@@ -22,19 +22,35 @@ struct inode;
> >  struct posix_acl;
> >  struct xfs_inode;
> >
> >-#define XFS_ACL_MAX_ENTRIES 25
> >  #define XFS_ACL_NOT_PRESENT (-1)
> >
> >  /* On-disk XFS access control list structure */
> >+struct xfs_acl_entry {
> >+	__be32	ae_tag;
> >+	__be32	ae_id;
> >+	__be16	ae_perm;
> >+	__be16	ae_pad;		/* fill the implicit hole in the structure */
> >+};
> >+
> >  struct xfs_acl {
> >-	__be32		acl_cnt;
> >-	struct xfs_acl_entry {
> >-		__be32	ae_tag;
> >-		__be32	ae_id;
> >-		__be16	ae_perm;
> >-	} acl_entry[XFS_ACL_MAX_ENTRIES];
> >+	__be32			acl_cnt;
> >+	struct xfs_acl_entry	acl_entry[0];
> >  };
> >
> >+/*
> >+ * The number of ACL entries allowed is defined by the on-disk format.
> >+ * For v4 superblocks, that is limited to 25 entries. For v5 superblocks, it is
> >+ * limited only by the maximum size of the xattr that stores the information.
> >+ */
> >+#define XFS_ACL_MAX_ENTRIES(mp)	\
> >+	(xfs_sb_version_hascrc(&mp->m_sb) \
> >+	   ?  (XATTR_SIZE_MAX - sizeof(__be32)) / sizeof(struct xfs_acl_entry) \
> >+	   : 25)
> 
> 
> XFS_ACL_MAX_ENTRIES(mp)  == (65536 - 4) / 12 == 5461
> 
> >+
> >+#define XFS_ACL_SIZE(mp) \
> >+	(sizeof(struct xfs_acl) + \
> >+		sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))
> 
> XFS_ACL_SIZE(mp) == (4 + 12) + 12 * ((64K - 4) / 12) == 65548
> 
> Did you want to add in the sizeof(struct xfs_acl) to the first term
> or the sizeof(__be32)? I would think the acl_entry[0] is the start
> of the array.

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Dave Chinner]

On Tue, Jun 04, 2013 at 01:26:40PM +1000, Dave Chinner wrote:
> On Mon, Jun 03, 2013 at 05:08:38PM -0500, Mark Tinguely wrote:
> > On 06/03/13 00:28, Dave Chinner wrote:
> > >From: Dave Chinner<dchinner@redhat.com>
> > >
> > >The limit of 25 ACL entries is arbitrary, but baked into the on-disk
> > >format.  For version 5 superblocks, increase it to the maximum nuber
> > >of ACLs that can fit into a single xattr.
> > >
> > >Signed-off-by: Dave Chinner<dchinner@redhat.com>
> > >Reviewed-by: Brian Foster<bfoster@redhat.com>
> > >---
....
> > >  /* On-disk XFS access control list structure */
> > >+struct xfs_acl_entry {
> > >+	__be32	ae_tag;
> > >+	__be32	ae_id;
> > >+	__be16	ae_perm;
> > >+	__be16	ae_pad;		/* fill the implicit hole in the structure */
> > >+};
> > >+
> > >  struct xfs_acl {
> > >-	__be32		acl_cnt;
> > >-	struct xfs_acl_entry {
> > >-		__be32	ae_tag;
> > >-		__be32	ae_id;
> > >-		__be16	ae_perm;
> > >-	} acl_entry[XFS_ACL_MAX_ENTRIES];
> > >+	__be32			acl_cnt;
> > >+	struct xfs_acl_entry	acl_entry[0];
> > >  };
> > >
> > >+/*
> > >+ * The number of ACL entries allowed is defined by the on-disk format.
> > >+ * For v4 superblocks, that is limited to 25 entries. For v5 superblocks, it is
> > >+ * limited only by the maximum size of the xattr that stores the information.
> > >+ */
> > >+#define XFS_ACL_MAX_ENTRIES(mp)	\
> > >+	(xfs_sb_version_hascrc(&mp->m_sb) \
> > >+	   ?  (XATTR_SIZE_MAX - sizeof(__be32)) / sizeof(struct xfs_acl_entry) \
> > >+	   : 25)
> > 
> > 
> > XFS_ACL_MAX_ENTRIES(mp)  == (65536 - 4) / 12 == 5461
> > 
> > >+
> > >+#define XFS_ACL_SIZE(mp) \
> > >+	(sizeof(struct xfs_acl) + \
> > >+		sizeof(struct xfs_acl_entry) * XFS_ACL_MAX_ENTRIES((mp)))
> > 
> > XFS_ACL_SIZE(mp) == (4 + 12) + 12 * ((64K - 4) / 12) == 65548
> > 
> > Did you want to add in the sizeof(struct xfs_acl) to the first term
> > or the sizeof(__be32)? I would think the acl_entry[0] is the start
> > of the array.
> 
> Ugh, I lost that in translation somewhere. Good catch.

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

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

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

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

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

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Ben Myers]

On Mon, Jun 03, 2013 at 03:28:51PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The limit of 25 ACL entries is arbitrary, but baked into the on-disk
> format.  For version 5 superblocks, increase it to the maximum nuber
> of ACLs that can fit into a single xattr.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Do we have any means of testing that many acls?

-Ben

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

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

[Dave Chinner]

On Tue, Jun 04, 2013 at 10:34:11AM -0500, Ben Myers wrote:
> On Mon, Jun 03, 2013 at 03:28:51PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The limit of 25 ACL entries is arbitrary, but baked into the on-disk
> > format.  For version 5 superblocks, increase it to the maximum nuber
> > of ACLs that can fit into a single xattr.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> 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....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Ben Myers]

On Wed, Jun 05, 2013 at 08:29:51AM +1000, Dave Chinner wrote:
> On Tue, Jun 04, 2013 at 10:34:11AM -0500, Ben Myers wrote:
> > On Mon, Jun 03, 2013 at 03:28:51PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > The limit of 25 ACL entries is arbitrary, but baked into the on-disk
> > > format.  For version 5 superblocks, increase it to the maximum nuber
> > > of ACLs that can fit into a single xattr.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > 
> > 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....

Nice. ;)

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