[PATCH 0/6] xfs: quota fixes and enhancements

[PATCH 0/6] xfs: quota fixes and enhancements

[Eric Sandeen]

A semi-random smattering of quota stuff.  First three seem quite
good to go, the rest are more along the lines of a suggestion
or conversation-starter.  ;)

(the first patch is just removing an unused arg).

xfs_repair doesn't look at quota blocks.  At all.  It relies
on quotacheck in the kernel to fix them up as needed.

But quotacheck doesn't check crc or UUID, so those errors
never get fixed, even though verifiers flag them.  So,
fix (some of) that by validating the uuid during quotacheck.
That's patches 2 & 3.

Also, when we are initializing quota, it's running quota
blocks through the full verifiers, and complaining loudly
about running xfs_repair (which won't do anything) even though
it's about to run the in-kernel repair.  The rest of the
patches take a stab at making that cleaner.

(One oddity is that we still don't catch a quota block which
has nothing but the crc wrong, I think.  I need to think harder
about that because crc handling for quotas seems special...)

Thanks,
-Eric

[PATCH 1/6] xfs: remove unused flags arg from xfs_dquot_verify

[Eric Sandeen]

Long ago the flags argument was used to determine whether
to issue warnings about corruptions, but that's done elsewhere
now and the flag is unused here, so remove it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_dquot_buf.c  | 5 ++---
 fs/xfs/libxfs/xfs_quota_defs.h | 3 +--
 fs/xfs/xfs_dquot.c             | 2 +-
 fs/xfs/xfs_log_recover.c       | 4 ++--
 fs/xfs/xfs_qm.c                | 2 +-
 5 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index 8b7a6c3..a926058 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -47,8 +47,7 @@
 	struct xfs_mount *mp,
 	xfs_disk_dquot_t *ddq,
 	xfs_dqid_t	 id,
-	uint		 type,	  /* used only when IO_dorepair is true */
-	uint		 flags)
+	uint		 type)	  /* used only when IO_dorepair is true */
 {
 	/*
 	 * We can encounter an uninitialized dquot buffer for 2 reasons:
@@ -200,7 +199,7 @@
 		if (i == 0)
 			id = be32_to_cpu(ddq->d_id);
 
-		fa = xfs_dquot_verify(mp, ddq, id + i, 0, 0);
+		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
 		if (fa)
 			return fa;
 	}
diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
index bb1b13a..8433656 100644
--- a/fs/xfs/libxfs/xfs_quota_defs.h
+++ b/fs/xfs/libxfs/xfs_quota_defs.h
@@ -152,8 +152,7 @@
 #define XFS_QMOPT_RESBLK_MASK	(XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_RES_RTBLKS)
 
 extern xfs_failaddr_t xfs_dquot_verify(struct xfs_mount *mp,
-		struct xfs_disk_dquot *ddq, xfs_dqid_t id, uint type,
-		uint flags);
+		struct xfs_disk_dquot *ddq, xfs_dqid_t id, uint type);
 extern int xfs_calc_dquots_per_chunk(unsigned int nbblks);
 extern int xfs_dquot_repair(struct xfs_mount *mp, struct xfs_disk_dquot *ddq,
 		xfs_dqid_t id, uint type);
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 43572f8..7e203f9 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1003,7 +1003,7 @@
 	/*
 	 * A simple sanity check in case we got a corrupted dquot..
 	 */
-	fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0, 0);
+	fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0);
 	if (fa) {
 		xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
 				be32_to_cpu(ddqp->d_id), fa);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 00240c9..3b35feb 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2702,7 +2702,7 @@ struct xfs_buf_cancel {
 				goto next;
 			}
 			fa = xfs_dquot_verify(mp, item->ri_buf[i].i_addr,
-					       -1, 0, 0);
+					       -1, 0);
 			if (fa) {
 				xfs_alert(mp,
 	"dquot corrupt at %pS trying to replay into block 0x%llx",
@@ -3353,7 +3353,7 @@ struct xfs_buf_cancel {
 	 */
 	dq_f = item->ri_buf[0].i_addr;
 	ASSERT(dq_f);
-	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id, 0, 0);
+	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id, 0);
 	if (fa) {
 		xfs_alert(mp, "corrupt dquot ID 0x%x in log at %pS",
 				dq_f->qlf_id, fa);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 5b848f4..64c22c32 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -867,7 +867,7 @@ struct xfs_qm_isolate {
 		 * find uninitialised dquot blks. See comment in
 		 * xfs_dquot_verify.
 		 */
-		fa = xfs_dquot_verify(mp, ddq, id + j, type, 0);
+		fa = xfs_dquot_verify(mp, ddq, id + j, type);
 		if (fa)
 			xfs_dquot_repair(mp, ddq, id + j, type);
 
-- 
1.8.3.1

Re: [PATCH 1/6] xfs: remove unused flags arg from xfs_dquot_verify

[Christoph Hellwig]

On Wed, Apr 04, 2018 at 01:49:09PM -0500, Eric Sandeen wrote:
> Long ago the flags argument was used to determine whether
> to issue warnings about corruptions, but that's done elsewhere
> now and the flag is unused here, so remove it.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/6] xfs: remove unused flags arg from xfs_dquot_verify

[Darrick J. Wong]

On Wed, Apr 04, 2018 at 01:49:09PM -0500, Eric Sandeen wrote:
> Long ago the flags argument was used to determine whether
> to issue warnings about corruptions, but that's done elsewhere
> now and the flag is unused here, so remove it.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_dquot_buf.c  | 5 ++---
>  fs/xfs/libxfs/xfs_quota_defs.h | 3 +--
>  fs/xfs/xfs_dquot.c             | 2 +-
>  fs/xfs/xfs_log_recover.c       | 4 ++--
>  fs/xfs/xfs_qm.c                | 2 +-
>  5 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index 8b7a6c3..a926058 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -47,8 +47,7 @@
>  	struct xfs_mount *mp,
>  	xfs_disk_dquot_t *ddq,
>  	xfs_dqid_t	 id,
> -	uint		 type,	  /* used only when IO_dorepair is true */
> -	uint		 flags)
> +	uint		 type)	  /* used only when IO_dorepair is true */
>  {
>  	/*
>  	 * We can encounter an uninitialized dquot buffer for 2 reasons:
> @@ -200,7 +199,7 @@
>  		if (i == 0)
>  			id = be32_to_cpu(ddq->d_id);
>  
> -		fa = xfs_dquot_verify(mp, ddq, id + i, 0, 0);
> +		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
>  		if (fa)
>  			return fa;
>  	}
> diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
> index bb1b13a..8433656 100644
> --- a/fs/xfs/libxfs/xfs_quota_defs.h
> +++ b/fs/xfs/libxfs/xfs_quota_defs.h
> @@ -152,8 +152,7 @@
>  #define XFS_QMOPT_RESBLK_MASK	(XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_RES_RTBLKS)
>  
>  extern xfs_failaddr_t xfs_dquot_verify(struct xfs_mount *mp,
> -		struct xfs_disk_dquot *ddq, xfs_dqid_t id, uint type,
> -		uint flags);
> +		struct xfs_disk_dquot *ddq, xfs_dqid_t id, uint type);
>  extern int xfs_calc_dquots_per_chunk(unsigned int nbblks);
>  extern int xfs_dquot_repair(struct xfs_mount *mp, struct xfs_disk_dquot *ddq,
>  		xfs_dqid_t id, uint type);
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 43572f8..7e203f9 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1003,7 +1003,7 @@
>  	/*
>  	 * A simple sanity check in case we got a corrupted dquot..
>  	 */
> -	fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0, 0);
> +	fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0);
>  	if (fa) {
>  		xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
>  				be32_to_cpu(ddqp->d_id), fa);
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 00240c9..3b35feb 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2702,7 +2702,7 @@ struct xfs_buf_cancel {
>  				goto next;
>  			}
>  			fa = xfs_dquot_verify(mp, item->ri_buf[i].i_addr,
> -					       -1, 0, 0);
> +					       -1, 0);
>  			if (fa) {
>  				xfs_alert(mp,
>  	"dquot corrupt at %pS trying to replay into block 0x%llx",
> @@ -3353,7 +3353,7 @@ struct xfs_buf_cancel {
>  	 */
>  	dq_f = item->ri_buf[0].i_addr;
>  	ASSERT(dq_f);
> -	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id, 0, 0);
> +	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id, 0);
>  	if (fa) {
>  		xfs_alert(mp, "corrupt dquot ID 0x%x in log at %pS",
>  				dq_f->qlf_id, fa);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 5b848f4..64c22c32 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -867,7 +867,7 @@ struct xfs_qm_isolate {
>  		 * find uninitialised dquot blks. See comment in
>  		 * xfs_dquot_verify.
>  		 */
> -		fa = xfs_dquot_verify(mp, ddq, id + j, type, 0);
> +		fa = xfs_dquot_verify(mp, ddq, id + j, type);
>  		if (fa)
>  			xfs_dquot_repair(mp, ddq, id + j, type);
>  
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Eric Sandeen]

In order to validate the UUID in xfs_dquot_verify, we need
the full xfs_qblk, not just the xfs_disk_dquot_t (which is
a subset).

Do the same for xfs_dquot_repair, for the same reasons.
Casting a xfs_disk_dquot to a xfs_qblk is risky if the source
pointer wasn't a full xfs_dqblk, so enforce that by changing
the arguments to these functions.

In xfs_qm_dqflush we move the memcpy up so that we have
a full (and updated) xfs_dqblk to test.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_dquot_buf.c  | 23 +++++++++--------------
 fs/xfs/libxfs/xfs_quota_defs.h |  4 ++--
 fs/xfs/xfs_dquot.c             | 12 +++++++-----
 fs/xfs/xfs_log_recover.c       |  6 ++++--
 fs/xfs/xfs_qm.c                |  6 +++---
 5 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index a926058..f94e8c2 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -44,11 +44,13 @@
  */
 xfs_failaddr_t
 xfs_dquot_verify(
-	struct xfs_mount *mp,
-	xfs_disk_dquot_t *ddq,
-	xfs_dqid_t	 id,
-	uint		 type)	  /* used only when IO_dorepair is true */
+	struct xfs_mount	*mp,
+	struct xfs_dqblk	*dqb,
+	xfs_dqid_t		id,
+	uint			type)	  /* used only during quota rebuild */
 {
+	struct xfs_disk_dquot	*ddq = &dqb->dd_diskdq;
+
 	/*
 	 * We can encounter an uninitialized dquot buffer for 2 reasons:
 	 * 1. If we crash while deleting the quotainode(s), and those blks got
@@ -104,13 +106,10 @@
 int
 xfs_dquot_repair(
 	struct xfs_mount	*mp,
-	struct xfs_disk_dquot	*ddq,
+	struct xfs_dqblk	*d,
 	xfs_dqid_t		id,
 	uint			type)
 {
-	struct xfs_dqblk	*d = (struct xfs_dqblk *)ddq;
-
-
 	/*
 	 * Typically, a repair is only requested by quotacheck.
 	 */
@@ -192,14 +191,10 @@
 	 * buffer so corruptions could point to the wrong dquot in this case.
 	 */
 	for (i = 0; i < ndquots; i++) {
-		struct xfs_disk_dquot	*ddq;
-
-		ddq = &d[i].dd_diskdq;
-
 		if (i == 0)
-			id = be32_to_cpu(ddq->d_id);
+			id = be32_to_cpu(d[i].dd_diskdq.d_id);
 
-		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
+		fa = xfs_dquot_verify(mp, &d[i], id + i, 0);
 		if (fa)
 			return fa;
 	}
diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
index 8433656..424526a 100644
--- a/fs/xfs/libxfs/xfs_quota_defs.h
+++ b/fs/xfs/libxfs/xfs_quota_defs.h
@@ -152,9 +152,9 @@
 #define XFS_QMOPT_RESBLK_MASK	(XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_RES_RTBLKS)
 
 extern xfs_failaddr_t xfs_dquot_verify(struct xfs_mount *mp,
-		struct xfs_disk_dquot *ddq, xfs_dqid_t id, uint type);
+		struct xfs_dqblk *dqb, xfs_dqid_t id, uint type);
 extern int xfs_calc_dquots_per_chunk(unsigned int nbblks);
-extern int xfs_dquot_repair(struct xfs_mount *mp, struct xfs_disk_dquot *ddq,
+extern int xfs_dquot_repair(struct xfs_mount *mp, struct xfs_dqblk *dqb,
 		xfs_dqid_t id, uint type);
 
 #endif	/* __XFS_QUOTA_H__ */
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 7e203f9..f7bed3c 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -955,6 +955,7 @@
 {
 	struct xfs_mount	*mp = dqp->q_mount;
 	struct xfs_buf		*bp;
+	struct xfs_dqblk	*dqb;
 	struct xfs_disk_dquot	*ddqp;
 	xfs_failaddr_t		fa;
 	int			error;
@@ -998,12 +999,16 @@
 	/*
 	 * Calculate the location of the dquot inside the buffer.
 	 */
-	ddqp = bp->b_addr + dqp->q_bufoffset;
+	dqb = bp->b_addr + dqp->q_bufoffset;
+	ddqp = &dqb->dd_diskdq;
+
+	/* This is the only portion of data that needs to persist */
+	memcpy(ddqp, &dqp->q_core, sizeof(xfs_disk_dquot_t));
 
 	/*
 	 * A simple sanity check in case we got a corrupted dquot..
 	 */
-	fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0);
+	fa = xfs_dquot_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0);
 	if (fa) {
 		xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
 				be32_to_cpu(ddqp->d_id), fa);
@@ -1013,9 +1018,6 @@
 		return -EIO;
 	}
 
-	/* This is the only portion of data that needs to persist */
-	memcpy(ddqp, &dqp->q_core, sizeof(xfs_disk_dquot_t));
-
 	/*
 	 * Clear the dirty field and remember the flush lsn for later use.
 	 */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 3b35feb..546df8e1 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3309,6 +3309,7 @@ struct xfs_buf_cancel {
 {
 	xfs_mount_t		*mp = log->l_mp;
 	xfs_buf_t		*bp;
+	struct xfs_dqblk	*dqb;
 	struct xfs_disk_dquot	*ddq, *recddq;
 	xfs_failaddr_t		fa;
 	int			error;
@@ -3322,7 +3323,8 @@ struct xfs_buf_cancel {
 	if (mp->m_qflags == 0)
 		return 0;
 
-	recddq = item->ri_buf[1].i_addr;
+	dqb = item->ri_buf[1].i_addr;
+	recddq = &dqb->dd_diskdq;
 	if (recddq == NULL) {
 		xfs_alert(log->l_mp, "NULL dquot in %s.", __func__);
 		return -EIO;
@@ -3353,7 +3355,7 @@ struct xfs_buf_cancel {
 	 */
 	dq_f = item->ri_buf[0].i_addr;
 	ASSERT(dq_f);
-	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id, 0);
+	fa = xfs_dquot_verify(mp, dqb, dq_f->qlf_id, 0);
 	if (fa) {
 		xfs_alert(mp, "corrupt dquot ID 0x%x in log at %pS",
 				dq_f->qlf_id, fa);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 64c22c32..b422382 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -859,7 +859,7 @@ struct xfs_qm_isolate {
 	for (j = 0; j < mp->m_quotainfo->qi_dqperchunk; j++) {
 		struct xfs_disk_dquot	*ddq;
 
-		ddq = (struct xfs_disk_dquot *)&dqb[j];
+		ddq = &dqb[j].dd_diskdq;
 
 		/*
 		 * Do a sanity check, and if needed, repair the dqblk. Don't
@@ -867,9 +867,9 @@ struct xfs_qm_isolate {
 		 * find uninitialised dquot blks. See comment in
 		 * xfs_dquot_verify.
 		 */
-		fa = xfs_dquot_verify(mp, ddq, id + j, type);
+		fa = xfs_dquot_verify(mp, &dqb[j], id + j, type);
 		if (fa)
-			xfs_dquot_repair(mp, ddq, id + j, type);
+			xfs_dquot_repair(mp, &dqb[j], id + j, type);
 
 		/*
 		 * Reset type in case we are reusing group quota file for
-- 
1.8.3.1

Re: [PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Darrick J. Wong]

On Wed, Apr 04, 2018 at 01:54:26PM -0500, Eric Sandeen wrote:
> In order to validate the UUID in xfs_dquot_verify, we need
> the full xfs_qblk, not just the xfs_disk_dquot_t (which is

          ^^^^^^^^^ xfs_dqblk, right?

> a subset).
> 
> Do the same for xfs_dquot_repair, for the same reasons.
> Casting a xfs_disk_dquot to a xfs_qblk is risky if the source
> pointer wasn't a full xfs_dqblk, so enforce that by changing
> the arguments to these functions.
> 
> In xfs_qm_dqflush we move the memcpy up so that we have
> a full (and updated) xfs_dqblk to test.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_dquot_buf.c  | 23 +++++++++--------------
>  fs/xfs/libxfs/xfs_quota_defs.h |  4 ++--
>  fs/xfs/xfs_dquot.c             | 12 +++++++-----
>  fs/xfs/xfs_log_recover.c       |  6 ++++--
>  fs/xfs/xfs_qm.c                |  6 +++---
>  5 files changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index a926058..f94e8c2 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -44,11 +44,13 @@
>   */
>  xfs_failaddr_t
>  xfs_dquot_verify(
> -	struct xfs_mount *mp,
> -	xfs_disk_dquot_t *ddq,
> -	xfs_dqid_t	 id,
> -	uint		 type)	  /* used only when IO_dorepair is true */
> +	struct xfs_mount	*mp,
> +	struct xfs_dqblk	*dqb,
> +	xfs_dqid_t		id,
> +	uint			type)	  /* used only during quota rebuild */
>  {
> +	struct xfs_disk_dquot	*ddq = &dqb->dd_diskdq;
> +
>  	/*
>  	 * We can encounter an uninitialized dquot buffer for 2 reasons:
>  	 * 1. If we crash while deleting the quotainode(s), and those blks got
> @@ -104,13 +106,10 @@
>  int
>  xfs_dquot_repair(
>  	struct xfs_mount	*mp,
> -	struct xfs_disk_dquot	*ddq,
> +	struct xfs_dqblk	*d,
>  	xfs_dqid_t		id,
>  	uint			type)
>  {
> -	struct xfs_dqblk	*d = (struct xfs_dqblk *)ddq;
> -
> -
>  	/*
>  	 * Typically, a repair is only requested by quotacheck.
>  	 */
> @@ -192,14 +191,10 @@

Any way you can get your diff generator to add -p to spit out the
alleged function this chunk is supposed to land in?  It makes reviewing
patches somewhat easier for me. :)

>  	 * buffer so corruptions could point to the wrong dquot in this case.
>  	 */
>  	for (i = 0; i < ndquots; i++) {
> -		struct xfs_disk_dquot	*ddq;
> -
> -		ddq = &d[i].dd_diskdq;
> -
>  		if (i == 0)
> -			id = be32_to_cpu(ddq->d_id);
> +			id = be32_to_cpu(d[i].dd_diskdq.d_id);
>  
> -		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
> +		fa = xfs_dquot_verify(mp, &d[i], id + i, 0);
>  		if (fa)
>  			return fa;
>  	}
> diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
> index 8433656..424526a 100644
> --- a/fs/xfs/libxfs/xfs_quota_defs.h
> +++ b/fs/xfs/libxfs/xfs_quota_defs.h
> @@ -152,9 +152,9 @@
>  #define XFS_QMOPT_RESBLK_MASK	(XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_RES_RTBLKS)
>  
>  extern xfs_failaddr_t xfs_dquot_verify(struct xfs_mount *mp,
> -		struct xfs_disk_dquot *ddq, xfs_dqid_t id, uint type);
> +		struct xfs_dqblk *dqb, xfs_dqid_t id, uint type);
>  extern int xfs_calc_dquots_per_chunk(unsigned int nbblks);
> -extern int xfs_dquot_repair(struct xfs_mount *mp, struct xfs_disk_dquot *ddq,
> +extern int xfs_dquot_repair(struct xfs_mount *mp, struct xfs_dqblk *dqb,
>  		xfs_dqid_t id, uint type);
>  
>  #endif	/* __XFS_QUOTA_H__ */
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 7e203f9..f7bed3c 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -955,6 +955,7 @@
>  {
>  	struct xfs_mount	*mp = dqp->q_mount;
>  	struct xfs_buf		*bp;
> +	struct xfs_dqblk	*dqb;
>  	struct xfs_disk_dquot	*ddqp;
>  	xfs_failaddr_t		fa;
>  	int			error;
> @@ -998,12 +999,16 @@
>  	/*
>  	 * Calculate the location of the dquot inside the buffer.
>  	 */
> -	ddqp = bp->b_addr + dqp->q_bufoffset;
> +	dqb = bp->b_addr + dqp->q_bufoffset;
> +	ddqp = &dqb->dd_diskdq;
> +
> +	/* This is the only portion of data that needs to persist */
> +	memcpy(ddqp, &dqp->q_core, sizeof(xfs_disk_dquot_t));
>  
>  	/*
>  	 * A simple sanity check in case we got a corrupted dquot..
>  	 */
> -	fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0);
> +	fa = xfs_dquot_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0);
>  	if (fa) {
>  		xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
>  				be32_to_cpu(ddqp->d_id), fa);
> @@ -1013,9 +1018,6 @@
>  		return -EIO;
>  	}
>  
> -	/* This is the only portion of data that needs to persist */
> -	memcpy(ddqp, &dqp->q_core, sizeof(xfs_disk_dquot_t));

About this memcpy() -- isn't the point of this function that we verify
the contents of the in-core q_core and only memcpy the contents into the
xfs_buf if it actually passes validation?  I guess the _dquot_verify
function needs the entire on-disk buffer so that it can validate the crc
and the uuid on a read, but we update the crc on dqflush and
(presumably) can set the uuid on write (quotacheck) or fail the dquot
read everywhere else, right?

Put another way, why not have xfs_dquot_buf_verify check the uuid?
xfs_dquot_repair seems to reset the uuid if it's garbage.

--D

> -
>  	/*
>  	 * Clear the dirty field and remember the flush lsn for later use.
>  	 */
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 3b35feb..546df8e1 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3309,6 +3309,7 @@ struct xfs_buf_cancel {
>  {
>  	xfs_mount_t		*mp = log->l_mp;
>  	xfs_buf_t		*bp;
> +	struct xfs_dqblk	*dqb;
>  	struct xfs_disk_dquot	*ddq, *recddq;
>  	xfs_failaddr_t		fa;
>  	int			error;
> @@ -3322,7 +3323,8 @@ struct xfs_buf_cancel {
>  	if (mp->m_qflags == 0)
>  		return 0;
>  
> -	recddq = item->ri_buf[1].i_addr;
> +	dqb = item->ri_buf[1].i_addr;
> +	recddq = &dqb->dd_diskdq;
>  	if (recddq == NULL) {
>  		xfs_alert(log->l_mp, "NULL dquot in %s.", __func__);
>  		return -EIO;
> @@ -3353,7 +3355,7 @@ struct xfs_buf_cancel {
>  	 */
>  	dq_f = item->ri_buf[0].i_addr;
>  	ASSERT(dq_f);
> -	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id, 0);
> +	fa = xfs_dquot_verify(mp, dqb, dq_f->qlf_id, 0);
>  	if (fa) {
>  		xfs_alert(mp, "corrupt dquot ID 0x%x in log at %pS",
>  				dq_f->qlf_id, fa);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 64c22c32..b422382 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -859,7 +859,7 @@ struct xfs_qm_isolate {
>  	for (j = 0; j < mp->m_quotainfo->qi_dqperchunk; j++) {
>  		struct xfs_disk_dquot	*ddq;
>  
> -		ddq = (struct xfs_disk_dquot *)&dqb[j];
> +		ddq = &dqb[j].dd_diskdq;
>  
>  		/*
>  		 * Do a sanity check, and if needed, repair the dqblk. Don't
> @@ -867,9 +867,9 @@ struct xfs_qm_isolate {
>  		 * find uninitialised dquot blks. See comment in
>  		 * xfs_dquot_verify.
>  		 */
> -		fa = xfs_dquot_verify(mp, ddq, id + j, type);
> +		fa = xfs_dquot_verify(mp, &dqb[j], id + j, type);
>  		if (fa)
> -			xfs_dquot_repair(mp, ddq, id + j, type);
> +			xfs_dquot_repair(mp, &dqb[j], id + j, type);
>  
>  		/*
>  		 * Reset type in case we are reusing group quota file for
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Eric Sandeen]

On 4/4/18 10:52 PM, Darrick J. Wong wrote:
> On Wed, Apr 04, 2018 at 01:54:26PM -0500, Eric Sandeen wrote:
>> In order to validate the UUID in xfs_dquot_verify, we need
>> the full xfs_qblk, not just the xfs_disk_dquot_t (which is
> 
>           ^^^^^^^^^ xfs_dqblk, right?

yup

...

>> @@ -192,14 +191,10 @@
> 
> Any way you can get your diff generator to add -p to spit out the
> alleged function this chunk is supposed to land in?  It makes reviewing
> patches somewhat easier for me. :)

No doubt ... I don't know why it doesn't do so, sorry.  :/  Will try
to figure that out.  Sorry about that.

>>  	 * buffer so corruptions could point to the wrong dquot in this case.
>>  	 */
>>  	for (i = 0; i < ndquots; i++) {
>> -		struct xfs_disk_dquot	*ddq;
>> -
>> -		ddq = &d[i].dd_diskdq;
>> -
>>  		if (i == 0)
>> -			id = be32_to_cpu(ddq->d_id);
>> +			id = be32_to_cpu(d[i].dd_diskdq.d_id);
>>  
>> -		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
>> +		fa = xfs_dquot_verify(mp, &d[i], id + i, 0);
>>  		if (fa)
>>  			return fa;
>>  	}

...

>> @@ -1013,9 +1018,6 @@
>>  		return -EIO;
>>  	}
>>  
>> -	/* This is the only portion of data that needs to persist */
>> -	memcpy(ddqp, &dqp->q_core, sizeof(xfs_disk_dquot_t));
> 
> About this memcpy() -- isn't the point of this function that we verify
> the contents of the in-core q_core and only memcpy the contents into the
> xfs_buf if it actually passes validation?

yeah, but if it fails here we release the buffer & shut down the filesystem ;)

> I guess the _dquot_verify
> function needs the entire on-disk buffer so that it can validate the crc

narrator:  xfs_dquot_verify doesn't verify the crc ;)

> and the uuid on a read, but we update the crc on dqflush and
> (presumably) can set the uuid on write (quotacheck) or fail the dquot
> read everywhere else, right?
> 
> Put another way, why not have xfs_dquot_buf_verify check the uuid?
> xfs_dquot_repair seems to reset the uuid if it's garbage.

Well, the above path (xfs_qm_dqflush) isn't going to do repair...

But OK, xfs_dquot_buf_verify does the entire dqblk; it iterates over
each dquot calling xfs_dquot_verify.  I figured the easiest way to
get uuid validation was to put it into xfs_dquot_verify.

But I guess you're suggesting a separate uuid check in xfs_dquot_buf_verify 
to validate the uuid, given that it has the full on-disk dquot?  Ok that
might make sense...

Thanks,
-Eric

Re: [PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Dave Chinner]

On Wed, Apr 04, 2018 at 11:13:03PM -0500, Eric Sandeen wrote:
> On 4/4/18 10:52 PM, Darrick J. Wong wrote:
> > On Wed, Apr 04, 2018 at 01:54:26PM -0500, Eric Sandeen wrote:
> >> In order to validate the UUID in xfs_dquot_verify, we need
> >> the full xfs_qblk, not just the xfs_disk_dquot_t (which is
> > 
> >           ^^^^^^^^^ xfs_dqblk, right?
> 
> yup
> 
> ...
> 
> >> @@ -192,14 +191,10 @@
> > 
> > Any way you can get your diff generator to add -p to spit out the
> > alleged function this chunk is supposed to land in?  It makes reviewing
> > patches somewhat easier for me. :)
> 
> No doubt ... I don't know why it doesn't do so, sorry.  :/  Will try
> to figure that out.  Sorry about that.

diff -up

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Eric Sandeen]

On 4/5/18 5:40 PM, Dave Chinner wrote:
> On Wed, Apr 04, 2018 at 11:13:03PM -0500, Eric Sandeen wrote:
>> On 4/4/18 10:52 PM, Darrick J. Wong wrote:
>>> On Wed, Apr 04, 2018 at 01:54:26PM -0500, Eric Sandeen wrote:
>>>> In order to validate the UUID in xfs_dquot_verify, we need
>>>> the full xfs_qblk, not just the xfs_disk_dquot_t (which is
>>>
>>>           ^^^^^^^^^ xfs_dqblk, right?
>>
>> yup
>>
>> ...
>>
>>>> @@ -192,14 +191,10 @@
>>>
>>> Any way you can get your diff generator to add -p to spit out the
>>> alleged function this chunk is supposed to land in?  It makes reviewing
>>> patches somewhat easier for me. :)
>>
>> No doubt ... I don't know why it doesn't do so, sorry.  :/  Will try
>> to figure that out.  Sorry about that.
> 
> diff -up

I am actually familiar with diff, thanks ;)  I couldn't figure out why
git & guilt weren't giving me the expected results.

For some reason I had this in this particular repo:

$ cat .gitattributes
*.c   diff=cpp
*.h   diff=cpp

Getting rid of that fixes it, I don't actually know why it was there.

-Eric

Re: [PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Dave Chinner]

On Thu, Apr 05, 2018 at 09:50:01PM -0500, Eric Sandeen wrote:
> 
> 
> On 4/5/18 5:40 PM, Dave Chinner wrote:
> > On Wed, Apr 04, 2018 at 11:13:03PM -0500, Eric Sandeen wrote:
> >> On 4/4/18 10:52 PM, Darrick J. Wong wrote:
> >>> On Wed, Apr 04, 2018 at 01:54:26PM -0500, Eric Sandeen wrote:
> >>>> In order to validate the UUID in xfs_dquot_verify, we need
> >>>> the full xfs_qblk, not just the xfs_disk_dquot_t (which is
> >>>
> >>>           ^^^^^^^^^ xfs_dqblk, right?
> >>
> >> yup
> >>
> >> ...
> >>
> >>>> @@ -192,14 +191,10 @@
> >>>
> >>> Any way you can get your diff generator to add -p to spit out the
> >>> alleged function this chunk is supposed to land in?  It makes reviewing
> >>> patches somewhat easier for me. :)
> >>
> >> No doubt ... I don't know why it doesn't do so, sorry.  :/  Will try
> >> to figure that out.  Sorry about that.
> > 
> > diff -up
> 
> I am actually familiar with diff, thanks ;)  I couldn't figure out why
> git & guilt weren't giving me the expected results.
> 
> For some reason I had this in this particular repo:
> 
> $ cat .gitattributes
> *.c   diff=cpp
> *.h   diff=cpp
> 
> Getting rid of that fixes it, I don't actually know why it was there.

That's in all my kernel repos, but not in xfs repos. It works like
.gitignore apparently, so it's something that comes along with the
repo.

$ gl .gitattributes 
commit 218dd85887da3d7d08119de18e9d325fcf30d7a4
Author: Jean Delvare <jdelvare@suse.de>
Date:   Fri Oct 7 17:03:04 2016 -0700

    .gitattributes: set git diff driver for C source code files
    
    Git can be told to apply language-specific rules when generating diffs.
    Enable this for C source code files (*.c and *.h) so that function names
    are printed right.  Specifically, doing so prevents "git diff" from
    mistakenly considering unindented goto labels as function names.
    
    Link: http://lkml.kernel.org/r/20160907143403.1449324f@endymion
    Signed-off-by: Jean Delvare <jdelvare@suse.de>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Joe Perches <joe@perches.com>
    Cc: Jonathan Corbet <corbet@lwn.net>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>


$ man 5 gitattributes
....
Defining a custom hunk-header
	  Each group of changes (called a "hunk") in the textual
	  diff output is prefixed with a line of the form:

		 @@ -k,l +n,m @@ TEXT
.....
	.... The following built in patterns are available:
.....
		- cpp suitable for source code in the C and C++ languages.


It doesn't seem to make any difference when I copy it into my local
xfs repos, except:

$ git status
On branch guilt/working
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	.gitattributes

nothing added to commit but untracked files present (use "git add" to track)
$

/me shrugs.

Git became far too complex for mere mortals to understand a long
time ago....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Darrick J. Wong]

On Wed, Apr 04, 2018 at 11:13:03PM -0500, Eric Sandeen wrote:
> On 4/4/18 10:52 PM, Darrick J. Wong wrote:
> > On Wed, Apr 04, 2018 at 01:54:26PM -0500, Eric Sandeen wrote:
> >> In order to validate the UUID in xfs_dquot_verify, we need
> >> the full xfs_qblk, not just the xfs_disk_dquot_t (which is
> > 
> >           ^^^^^^^^^ xfs_dqblk, right?
> 
> yup
> 
> ...
> 
> >> @@ -192,14 +191,10 @@
> > 
> > Any way you can get your diff generator to add -p to spit out the
> > alleged function this chunk is supposed to land in?  It makes reviewing
> > patches somewhat easier for me. :)
> 
> No doubt ... I don't know why it doesn't do so, sorry.  :/  Will try
> to figure that out.  Sorry about that.
> 
> >>  	 * buffer so corruptions could point to the wrong dquot in this case.
> >>  	 */
> >>  	for (i = 0; i < ndquots; i++) {
> >> -		struct xfs_disk_dquot	*ddq;
> >> -
> >> -		ddq = &d[i].dd_diskdq;
> >> -
> >>  		if (i == 0)
> >> -			id = be32_to_cpu(ddq->d_id);
> >> +			id = be32_to_cpu(d[i].dd_diskdq.d_id);
> >>  
> >> -		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
> >> +		fa = xfs_dquot_verify(mp, &d[i], id + i, 0);
> >>  		if (fa)
> >>  			return fa;
> >>  	}
> 
> ...
> 
> >> @@ -1013,9 +1018,6 @@
> >>  		return -EIO;
> >>  	}
> >>  
> >> -	/* This is the only portion of data that needs to persist */
> >> -	memcpy(ddqp, &dqp->q_core, sizeof(xfs_disk_dquot_t));
> > 
> > About this memcpy() -- isn't the point of this function that we verify
> > the contents of the in-core q_core and only memcpy the contents into the
> > xfs_buf if it actually passes validation?
> 
> yeah, but if it fails here we release the buffer & shut down the filesystem ;)
> 
> > I guess the _dquot_verify
> > function needs the entire on-disk buffer so that it can validate the crc
> 
> narrator:  xfs_dquot_verify doesn't verify the crc ;)

Sorry, my brain was all discombobulated last week. :(

> > and the uuid on a read, but we update the crc on dqflush and
> > (presumably) can set the uuid on write (quotacheck) or fail the dquot
> > read everywhere else, right?
> > 
> > Put another way, why not have xfs_dquot_buf_verify check the uuid?
> > xfs_dquot_repair seems to reset the uuid if it's garbage.
> 
> Well, the above path (xfs_qm_dqflush) isn't going to do repair...
> 
> But OK, xfs_dquot_buf_verify does the entire dqblk; it iterates over
> each dquot calling xfs_dquot_verify.  I figured the easiest way to
> get uuid validation was to put it into xfs_dquot_verify.
> 
> But I guess you're suggesting a separate uuid check in xfs_dquot_buf_verify 
> to validate the uuid, given that it has the full on-disk dquot?  Ok that
> might make sense...

Eh, now that I've figured out what all these patches are trying to do
(and figured out what hunks are modifying which functions) this is a lot
clearer to me.  The UUID check should be in the structure verifier, not
the crc verifier, as you point out.

So the more I reread this series the more I think they're ok, though you
might want to fix the things Christoph pointed out on this patch.

(I'll review the repair side of things whenever you get to that.)

--D

> Thanks,
> -Eric
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Christoph Hellwig]

On Wed, Apr 04, 2018 at 01:54:26PM -0500, Eric Sandeen wrote:
> In order to validate the UUID in xfs_dquot_verify, we need
> the full xfs_qblk, not just the xfs_disk_dquot_t (which is
> a subset).
> 
> Do the same for xfs_dquot_repair, for the same reasons.
> Casting a xfs_disk_dquot to a xfs_qblk is risky if the source
> pointer wasn't a full xfs_dqblk, so enforce that by changing
> the arguments to these functions.
> 
> In xfs_qm_dqflush we move the memcpy up so that we have
> a full (and updated) xfs_dqblk to test.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_dquot_buf.c  | 23 +++++++++--------------
>  fs/xfs/libxfs/xfs_quota_defs.h |  4 ++--
>  fs/xfs/xfs_dquot.c             | 12 +++++++-----
>  fs/xfs/xfs_log_recover.c       |  6 ++++--
>  fs/xfs/xfs_qm.c                |  6 +++---
>  5 files changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index a926058..f94e8c2 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -44,11 +44,13 @@
>   */
>  xfs_failaddr_t
>  xfs_dquot_verify(
> -	struct xfs_mount *mp,
> -	xfs_disk_dquot_t *ddq,
> -	xfs_dqid_t	 id,
> -	uint		 type)	  /* used only when IO_dorepair is true */
> +	struct xfs_mount	*mp,
> +	struct xfs_dqblk	*dqb,
> +	xfs_dqid_t		id,
> +	uint			type)	  /* used only during quota rebuild */
>  {
> +	struct xfs_disk_dquot	*ddq = &dqb->dd_diskdq;
> +
>  	/*
>  	 * We can encounter an uninitialized dquot buffer for 2 reasons:
>  	 * 1. If we crash while deleting the quotainode(s), and those blks got
> @@ -104,13 +106,10 @@
>  int
>  xfs_dquot_repair(
>  	struct xfs_mount	*mp,
> -	struct xfs_disk_dquot	*ddq,
> +	struct xfs_dqblk	*d,

Rename this to dqblks to make it clear that it is an array?

>  		if (i == 0)
> -			id = be32_to_cpu(ddq->d_id);
> +			id = be32_to_cpu(d[i].dd_diskdq.d_id);
>  
> -		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
> +		fa = xfs_dquot_verify(mp, &d[i], id + i, 0);

Can we either use only array indices or only pointer arithmetics and
not mix the two?  (personall I prefer the pointer arithmetics).

Functionally the patch looks fine to me.

Re: [PATCH 2/6] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Darrick J. Wong]

On Thu, Apr 05, 2018 at 12:14:12AM -0700, Christoph Hellwig wrote:
> On Wed, Apr 04, 2018 at 01:54:26PM -0500, Eric Sandeen wrote:
> > In order to validate the UUID in xfs_dquot_verify, we need
> > the full xfs_qblk, not just the xfs_disk_dquot_t (which is
> > a subset).
> > 
> > Do the same for xfs_dquot_repair, for the same reasons.
> > Casting a xfs_disk_dquot to a xfs_qblk is risky if the source
> > pointer wasn't a full xfs_dqblk, so enforce that by changing
> > the arguments to these functions.
> > 
> > In xfs_qm_dqflush we move the memcpy up so that we have
> > a full (and updated) xfs_dqblk to test.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_dquot_buf.c  | 23 +++++++++--------------
> >  fs/xfs/libxfs/xfs_quota_defs.h |  4 ++--
> >  fs/xfs/xfs_dquot.c             | 12 +++++++-----
> >  fs/xfs/xfs_log_recover.c       |  6 ++++--
> >  fs/xfs/xfs_qm.c                |  6 +++---
> >  5 files changed, 25 insertions(+), 26 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> > index a926058..f94e8c2 100644
> > --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> > +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> > @@ -44,11 +44,13 @@
> >   */
> >  xfs_failaddr_t
> >  xfs_dquot_verify(
> > -	struct xfs_mount *mp,
> > -	xfs_disk_dquot_t *ddq,
> > -	xfs_dqid_t	 id,
> > -	uint		 type)	  /* used only when IO_dorepair is true */
> > +	struct xfs_mount	*mp,
> > +	struct xfs_dqblk	*dqb,
> > +	xfs_dqid_t		id,
> > +	uint			type)	  /* used only during quota rebuild */
> >  {
> > +	struct xfs_disk_dquot	*ddq = &dqb->dd_diskdq;
> > +
> >  	/*
> >  	 * We can encounter an uninitialized dquot buffer for 2 reasons:
> >  	 * 1. If we crash while deleting the quotainode(s), and those blks got
> > @@ -104,13 +106,10 @@
> >  int
> >  xfs_dquot_repair(
> >  	struct xfs_mount	*mp,
> > -	struct xfs_disk_dquot	*ddq,
> > +	struct xfs_dqblk	*d,
> 
> Rename this to dqblks to make it clear that it is an array?
> 
> >  		if (i == 0)
> > -			id = be32_to_cpu(ddq->d_id);
> > +			id = be32_to_cpu(d[i].dd_diskdq.d_id);
> >  
> > -		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
> > +		fa = xfs_dquot_verify(mp, &d[i], id + i, 0);
> 
> Can we either use only array indices or only pointer arithmetics and
> not mix the two?  (personall I prefer the pointer arithmetics).
> 
> Functionally the patch looks fine to me.

The patch looks fine to me too, though I think Christoph's comments need
some kind of response.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6 V2] xfs: pass xfs_dqblk to xfs_dquot_verify/xfs_dquot_repair

[Eric Sandeen]

In order to validate the UUID in xfs_dquot_verify, we need the full
xfs_qblk, not just the xfs_disk_dquot_t (which is a subset).
    
Do the same for xfs_dquot_repair, for the same reasons.  Casting a
xfs_disk_dquot to a xfs_qblk is risky if the source pointer wasn't a full
xfs_dqblk, so enforce that by changing the arguments to these functions.
    
In xfs_qm_dqflush we move the memcpy up so that we have a full
(and updated) xfs_dqblk to test.
    
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: rename some variables for clarity, get rid of pointer math in
xfs_dquot_buf_verify_crc

diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index a926058..b5c0f3c 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -44,11 +44,13 @@ xfs_calc_dquots_per_chunk(
  */
 xfs_failaddr_t
 xfs_dquot_verify(
-	struct xfs_mount *mp,
-	xfs_disk_dquot_t *ddq,
-	xfs_dqid_t	 id,
-	uint		 type)	  /* used only when IO_dorepair is true */
+	struct xfs_mount	*mp,
+	struct xfs_dqblk	*dqb,
+	xfs_dqid_t		id,
+	uint			type)	  /* used only during quota rebuild */
 {
+	struct xfs_disk_dquot	*ddq = &dqb->dd_diskdq;
+
 	/*
 	 * We can encounter an uninitialized dquot buffer for 2 reasons:
 	 * 1. If we crash while deleting the quotainode(s), and those blks got
@@ -104,27 +106,24 @@ xfs_dquot_verify(
 int
 xfs_dquot_repair(
 	struct xfs_mount	*mp,
-	struct xfs_disk_dquot	*ddq,
+	struct xfs_dqblk	*dqblk,
 	xfs_dqid_t		id,
 	uint			type)
 {
-	struct xfs_dqblk	*d = (struct xfs_dqblk *)ddq;
-
-
 	/*
 	 * Typically, a repair is only requested by quotacheck.
 	 */
 	ASSERT(id != -1);
-	memset(d, 0, sizeof(xfs_dqblk_t));
+	memset(dqblk, 0, sizeof(xfs_dqblk_t));
 
-	d->dd_diskdq.d_magic = cpu_to_be16(XFS_DQUOT_MAGIC);
-	d->dd_diskdq.d_version = XFS_DQUOT_VERSION;
-	d->dd_diskdq.d_flags = type;
-	d->dd_diskdq.d_id = cpu_to_be32(id);
+	dqblk->dd_diskdq.d_magic = cpu_to_be16(XFS_DQUOT_MAGIC);
+	dqblk->dd_diskdq.d_version = XFS_DQUOT_VERSION;
+	dqblk->dd_diskdq.d_flags = type;
+	dqblk->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_meta_uuid);
-		xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
+		uuid_copy(&dqblk->dd_uuid, &mp->m_sb.sb_meta_uuid);
+		xfs_update_cksum((char *)dqblk, sizeof(struct xfs_dqblk),
 				 XFS_DQUOT_CRC_OFF);
 	}
 
@@ -136,7 +135,7 @@ xfs_dquot_buf_verify_crc(
 	struct xfs_mount	*mp,
 	struct xfs_buf		*bp)
 {
-	struct xfs_dqblk	*d = (struct xfs_dqblk *)bp->b_addr;
+	struct xfs_dqblk	*dqblks = (struct xfs_dqblk *)bp->b_addr;
 	int			ndquots;
 	int			i;
 
@@ -153,11 +152,11 @@ xfs_dquot_buf_verify_crc(
 	else
 		ndquots = xfs_calc_dquots_per_chunk(bp->b_length);
 
-	for (i = 0; i < ndquots; i++, d++) {
-		if (!xfs_verify_cksum((char *)d, sizeof(struct xfs_dqblk),
-				 XFS_DQUOT_CRC_OFF))
+	for (i = 0; i < ndquots; i++) {
+		if (!xfs_verify_cksum((char *)&dqblks[i],
+				sizeof(struct xfs_dqblk), XFS_DQUOT_CRC_OFF))
 			return false;
-		if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_meta_uuid))
+		if (!uuid_equal(&dqblks[i].dd_uuid, &mp->m_sb.sb_meta_uuid))
 			return false;
 	}
 	return true;
@@ -192,14 +191,10 @@ xfs_dquot_buf_verify(
 	 * buffer so corruptions could point to the wrong dquot in this case.
 	 */
 	for (i = 0; i < ndquots; i++) {
-		struct xfs_disk_dquot	*ddq;
-
-		ddq = &d[i].dd_diskdq;
-
 		if (i == 0)
-			id = be32_to_cpu(ddq->d_id);
+			id = be32_to_cpu(d[i].dd_diskdq.d_id);
 
-		fa = xfs_dquot_verify(mp, ddq, id + i, 0);
+		fa = xfs_dquot_verify(mp, &d[i], id + i, 0);
 		if (fa)
 			return fa;
 	}
diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
index 8433656..424526a 100644
--- a/fs/xfs/libxfs/xfs_quota_defs.h
+++ b/fs/xfs/libxfs/xfs_quota_defs.h
@@ -152,9 +152,9 @@ typedef uint16_t	xfs_qwarncnt_t;
 #define XFS_QMOPT_RESBLK_MASK	(XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_RES_RTBLKS)
 
 extern xfs_failaddr_t xfs_dquot_verify(struct xfs_mount *mp,
-		struct xfs_disk_dquot *ddq, xfs_dqid_t id, uint type);
+		struct xfs_dqblk *dqb, xfs_dqid_t id, uint type);
 extern int xfs_calc_dquots_per_chunk(unsigned int nbblks);
-extern int xfs_dquot_repair(struct xfs_mount *mp, struct xfs_disk_dquot *ddq,
+extern int xfs_dquot_repair(struct xfs_mount *mp, struct xfs_dqblk *dqb,
 		xfs_dqid_t id, uint type);
 
 #endif	/* __XFS_QUOTA_H__ */
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 8d378f4..613fbe6 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -953,6 +953,7 @@ xfs_qm_dqflush(
 {
 	struct xfs_mount	*mp = dqp->q_mount;
 	struct xfs_buf		*bp;
+	struct xfs_dqblk	*dqb;
 	struct xfs_disk_dquot	*ddqp;
 	xfs_failaddr_t		fa;
 	int			error;
@@ -996,12 +997,16 @@ xfs_qm_dqflush(
 	/*
 	 * Calculate the location of the dquot inside the buffer.
 	 */
-	ddqp = bp->b_addr + dqp->q_bufoffset;
+	dqb = bp->b_addr + dqp->q_bufoffset;
+	ddqp = &dqb->dd_diskdq;
+
+	/* This is the only portion of data that needs to persist */
+	memcpy(ddqp, &dqp->q_core, sizeof(xfs_disk_dquot_t));
 
 	/*
 	 * A simple sanity check in case we got a corrupted dquot..
 	 */
-	fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0);
+	fa = xfs_dquot_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0);
 	if (fa) {
 		xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
 				be32_to_cpu(ddqp->d_id), fa);
@@ -1011,9 +1016,6 @@ xfs_qm_dqflush(
 		return -EIO;
 	}
 
-	/* This is the only portion of data that needs to persist */
-	memcpy(ddqp, &dqp->q_core, sizeof(xfs_disk_dquot_t));
-
 	/*
 	 * Clear the dirty field and remember the flush lsn for later use.
 	 */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 06a09cb..d866a59 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3304,6 +3304,7 @@ xlog_recover_dquot_pass2(
 {
 	xfs_mount_t		*mp = log->l_mp;
 	xfs_buf_t		*bp;
+	struct xfs_dqblk	*dqb;
 	struct xfs_disk_dquot	*ddq, *recddq;
 	xfs_failaddr_t		fa;
 	int			error;
@@ -3317,7 +3318,8 @@ xlog_recover_dquot_pass2(
 	if (mp->m_qflags == 0)
 		return 0;
 
-	recddq = item->ri_buf[1].i_addr;
+	dqb = item->ri_buf[1].i_addr;
+	recddq = &dqb->dd_diskdq;
 	if (recddq == NULL) {
 		xfs_alert(log->l_mp, "NULL dquot in %s.", __func__);
 		return -EIO;
@@ -3348,7 +3350,7 @@ xlog_recover_dquot_pass2(
 	 */
 	dq_f = item->ri_buf[0].i_addr;
 	ASSERT(dq_f);
-	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id, 0);
+	fa = xfs_dquot_verify(mp, dqb, dq_f->qlf_id, 0);
 	if (fa) {
 		xfs_alert(mp, "corrupt dquot ID 0x%x in log at %pS",
 				dq_f->qlf_id, fa);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index c71ad79..7d784d6 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -857,7 +857,7 @@ xfs_qm_reset_dqcounts(
 	for (j = 0; j < mp->m_quotainfo->qi_dqperchunk; j++) {
 		struct xfs_disk_dquot	*ddq;
 
-		ddq = (struct xfs_disk_dquot *)&dqb[j];
+		ddq = &dqb[j].dd_diskdq;
 
 		/*
 		 * Do a sanity check, and if needed, repair the dqblk. Don't
@@ -865,9 +865,9 @@ xfs_qm_reset_dqcounts(
 		 * find uninitialised dquot blks. See comment in
 		 * xfs_dquot_verify.
 		 */
-		fa = xfs_dquot_verify(mp, ddq, id + j, type);
+		fa = xfs_dquot_verify(mp, &dqb[j], id + j, type);
 		if (fa)
-			xfs_dquot_repair(mp, ddq, id + j, type);
+			xfs_dquot_repair(mp, &dqb[j], id + j, type);
 
 		/*
 		 * Reset type in case we are reusing group quota file for

[PATCH 3/6] xfs: validate UUID and type in xfs_dquot_verify

[Eric Sandeen]

Today the xfs_dqblk UUID is only checked in
xfs_dquot_buf_verify_crc, which is a bit odd in itself; normally
a CRC is checked on its own, separate from other metadata.

And by not checking the UUID in xfs_dquot_verify, this means
that future read/write verifiers will continue to choke on
mismatched UUID errors which are never seen or repaired when
quotacheck calls xfs_dquot_verify to validate a disk dquot.

Move the uuid check from xfs_dquot_buf_verify_crc to
xfs_dquot_verify so that this piece of metadata is more consistently
checked.  If we have a type sent in as well, validate that too -
it was unused until now.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_dquot_buf.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index f94e8c2..9f8b2c5 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -66,11 +66,20 @@
 	 * This is all fine; things are still consistent, and we haven't lost
 	 * any quota information. Just don't complain about bad dquot blks.
 	 */
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		struct xfs_dqblk *dqb = (struct xfs_dqblk *)ddq;
+
+		if (!uuid_equal(&dqb->dd_uuid, &mp->m_sb.sb_meta_uuid))
+			return __this_address;
+	}
+
 	if (ddq->d_magic != cpu_to_be16(XFS_DQUOT_MAGIC))
 		return __this_address;
 	if (ddq->d_version != XFS_DQUOT_VERSION)
 		return __this_address;
 
+	if (type && ddq->d_flags != type)
+		return __this_address;
 	if (ddq->d_flags != XFS_DQ_USER &&
 	    ddq->d_flags != XFS_DQ_PROJ &&
 	    ddq->d_flags != XFS_DQ_GROUP)
@@ -156,8 +165,6 @@
 		if (!xfs_verify_cksum((char *)d, sizeof(struct xfs_dqblk),
 				 XFS_DQUOT_CRC_OFF))
 			return false;
-		if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_meta_uuid))
-			return false;
 	}
 	return true;
 }
-- 
1.8.3.1

Re: [PATCH 3/6] xfs: validate UUID and type in xfs_dquot_verify

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/6] xfs: validate UUID and type in xfs_dquot_verify

[Darrick J. Wong]

On Wed, Apr 04, 2018 at 02:00:22PM -0500, Eric Sandeen wrote:
> Today the xfs_dqblk UUID is only checked in
> xfs_dquot_buf_verify_crc, which is a bit odd in itself; normally
> a CRC is checked on its own, separate from other metadata.
> 
> And by not checking the UUID in xfs_dquot_verify, this means
> that future read/write verifiers will continue to choke on
> mismatched UUID errors which are never seen or repaired when
> quotacheck calls xfs_dquot_verify to validate a disk dquot.
> 
> Move the uuid check from xfs_dquot_buf_verify_crc to
> xfs_dquot_verify so that this piece of metadata is more consistently
> checked.  If we have a type sent in as well, validate that too -
> it was unused until now.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_dquot_buf.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index f94e8c2..9f8b2c5 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -66,11 +66,20 @@
>  	 * This is all fine; things are still consistent, and we haven't lost
>  	 * any quota information. Just don't complain about bad dquot blks.
>  	 */
> +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +		struct xfs_dqblk *dqb = (struct xfs_dqblk *)ddq;
> +
> +		if (!uuid_equal(&dqb->dd_uuid, &mp->m_sb.sb_meta_uuid))
> +			return __this_address;
> +	}
> +
>  	if (ddq->d_magic != cpu_to_be16(XFS_DQUOT_MAGIC))
>  		return __this_address;
>  	if (ddq->d_version != XFS_DQUOT_VERSION)
>  		return __this_address;
>  
> +	if (type && ddq->d_flags != type)
> +		return __this_address;
>  	if (ddq->d_flags != XFS_DQ_USER &&
>  	    ddq->d_flags != XFS_DQ_PROJ &&
>  	    ddq->d_flags != XFS_DQ_GROUP)
> @@ -156,8 +165,6 @@
>  		if (!xfs_verify_cksum((char *)d, sizeof(struct xfs_dqblk),
>  				 XFS_DQUOT_CRC_OFF))
>  			return false;
> -		if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_meta_uuid))
> -			return false;
>  	}
>  	return true;
>  }
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] xfs: validate UUID and type in xfs_dquot_verify

[Darrick J. Wong]

On Tue, May 01, 2018 at 09:13:37AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 04, 2018 at 02:00:22PM -0500, Eric Sandeen wrote:
> > Today the xfs_dqblk UUID is only checked in
> > xfs_dquot_buf_verify_crc, which is a bit odd in itself; normally
> > a CRC is checked on its own, separate from other metadata.
> > 
> > And by not checking the UUID in xfs_dquot_verify, this means
> > that future read/write verifiers will continue to choke on
> > mismatched UUID errors which are never seen or repaired when
> > quotacheck calls xfs_dquot_verify to validate a disk dquot.
> > 
> > Move the uuid check from xfs_dquot_buf_verify_crc to
> > xfs_dquot_verify so that this piece of metadata is more consistently
> > checked.  If we have a type sent in as well, validate that too -
> > it was unused until now.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Looks ok,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

I take this back...

> --D
> 
> > ---
> >  fs/xfs/libxfs/xfs_dquot_buf.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> > index f94e8c2..9f8b2c5 100644
> > --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> > +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> > @@ -66,11 +66,20 @@
> >  	 * This is all fine; things are still consistent, and we haven't lost
> >  	 * any quota information. Just don't complain about bad dquot blks.
> >  	 */
> > +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > +		struct xfs_dqblk *dqb = (struct xfs_dqblk *)ddq;
> > +
> > +		if (!uuid_equal(&dqb->dd_uuid, &mp->m_sb.sb_meta_uuid))
> > +			return __this_address;
> > +	}

generic/055 regresses when quotas are enabled and log recovery has to
process a dquot:

xfs_dquot_verify+0x48/0x120 [xfs]
xlog_recover_dquot_pass2.isra.40+0x98/0x340 [xfs]
xlog_recover_items_pass2+0x3c/0x60 [xfs]
xlog_recover_commit_trans+0x178/0x2a0 [xfs]
xlog_recovery_process_trans+0x84/0xd0 [xfs]
xlog_recover_process_data+0x93/0x230 [xfs]
xlog_do_recovery_pass+0x453/0x6d0 [xfs]
xlog_do_log_recovery+0x8c/0x140 [xfs]
xlog_do_recover+0x43/0x2b0 [xfs]
xlog_recover+0xac/0x140 [xfs]
xfs_log_mount+0x1bb/0x2f0 [xfs]
xfs_mountfs+0x520/0xa60 [xfs]
? xfs_filestream_get_parent+0x70/0x70 [xfs]
? xfs_mru_cache_create+0x18b/0x1f0 [xfs]
xfs_fs_fill_super+0x4ce/0x650 [xfs]
? xfs_test_remount_options+0x60/0x60 [xfs]
mount_bdev+0x17b/0x1b0
mount_fs+0x15/0x80
vfs_kern_mount.part.34+0x54/0x150
do_mount+0x21f/0xd30
? rcu_read_lock_sched_held+0x6b/0x80
ksys_mount+0x80/0xd0
__x64_sys_mount+0x21/0x30
do_syscall_64+0x56/0x180
entry_SYSCALL_64_after_hwframe+0x49/0xbe

...because we only log struct xfs_disk_dquot, not struct xfs_dqblk.
The logged buffer isn't long enough to contain dd_uuid so we trip over
the uuid check in xfs_dquot_verify and log recovery fails.

XFS (pmem1): corrupt dquot ID 0x0 in log at xfs_dquot_verify+0x43/0x120 [xfs]
00000000411604c1: 44 51 01 01 00 00 00 00 00 00 00 00 00 00 00 00  DQ..............
00000000f11219a7: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
00000000a8451ea9: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
0000000060077c63: 00 00 00 00 00 00 00 f5 00 00 00 00 00 00 00 00  ................
00000000efc4e1e2: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
000000000b057484: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
000000005fffbba2: 00 00 00 00 00 00 00 00 00 01 00 00 00 00 ad de  ................
00000000960b6207: 00 02 00 00 00 00 ad de 00 00 00 00 00 00 00 00  ................
00000000c16a5600: 41 42 33 43 00 00 00 01                          AB3C....

(Yikes, we've overrun into a cntbt block header!)

--D

> > +
> >  	if (ddq->d_magic != cpu_to_be16(XFS_DQUOT_MAGIC))
> >  		return __this_address;
> >  	if (ddq->d_version != XFS_DQUOT_VERSION)
> >  		return __this_address;
> >  
> > +	if (type && ddq->d_flags != type)
> > +		return __this_address;
> >  	if (ddq->d_flags != XFS_DQ_USER &&
> >  	    ddq->d_flags != XFS_DQ_PROJ &&
> >  	    ddq->d_flags != XFS_DQ_GROUP)
> > @@ -156,8 +165,6 @@
> >  		if (!xfs_verify_cksum((char *)d, sizeof(struct xfs_dqblk),
> >  				 XFS_DQUOT_CRC_OFF))
> >  			return false;
> > -		if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_meta_uuid))
> > -			return false;
> >  	}
> >  	return true;
> >  }
> > -- 
> > 1.8.3.1
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] xfs: quieter quota initialization with bad dquots

[Eric Sandeen]

As of now, when we start quotacheck we read quotas with verifiers,
then reread without them, if we saw corruption.  This is so the
corruption is noted in the logs; this is all well and good, except that
the verifier errors instruct the user to run xfs_repair, which won't
actually do anything for corrupt dqblks.

We can quiet this down by doing the initial read without verifiers,
knowing that we're going to validate the dqblks manually in later
stages, and repair them if needed.

This adds new (more terse) messages stating whether a corrupted
dquot was found and fixed.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

I'm a bit on the fence about this one; we lose the hexdump too so we won't
see what was wrong (I could add that back in, I suppose).

 fs/xfs/libxfs/xfs_dquot_buf.c |  4 ++++
 fs/xfs/xfs_qm.c               | 30 +++++++++++++-----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index 9f8b2c5..c13d440 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -136,6 +136,10 @@
 				 XFS_DQUOT_CRC_OFF);
 	}
 
+	xfs_alert(mp, "Repaired %s quota block for id %d.",
+		  type == XFS_DQ_USER ? "user" :
+		    (type == XFS_DQ_GROUP ? "group" : "project"), id);
+
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index b422382..328d770 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -868,8 +868,12 @@ struct xfs_qm_isolate {
 		 * xfs_dquot_verify.
 		 */
 		fa = xfs_dquot_verify(mp, &dqb[j], id + j, type);
-		if (fa)
+		if (fa) {
+			xfs_alert(mp,
+"Metadata corruption error detected at %pS, xfs_dquot block 0x%llx.",
+				  __this_address, bp->b_bn);
 			xfs_dquot_repair(mp, &dqb[j], id + j, type);
+		}
 
 		/*
 		 * Reset type in case we are reusing group quota file for
@@ -922,24 +926,16 @@ struct xfs_qm_isolate {
 	 * everything if we were to crash in the middle of this loop.
 	 */
 	while (blkcnt--) {
-		error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
-			      XFS_FSB_TO_DADDR(mp, bno),
-			      mp->m_quotainfo->qi_dqchunklen, 0, &bp,
-			      &xfs_dquot_buf_ops);
-
 		/*
-		 * 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.
+		 * CRC and validation errors are possible here.  Because
+		 * this may occur, we read without CRC validation so that we can
+		 * repair the damage via xfs_qm_reset_dqcounts().
+		 * Errors will be detected, declared, and repaired later in the
+		 * quotacheck process.
 		 */
-		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);
-		}
+		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;
-- 
1.8.3.1

Re: [PATCH 4/6] xfs: quieter quota initialization with bad dquots

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/6] xfs: quieter quota initialization with bad dquots

[Darrick J. Wong]

On Wed, Apr 04, 2018 at 02:06:17PM -0500, Eric Sandeen wrote:
> As of now, when we start quotacheck we read quotas with verifiers,
> then reread without them, if we saw corruption.  This is so the
> corruption is noted in the logs; this is all well and good, except that
> the verifier errors instruct the user to run xfs_repair, which won't
> actually do anything for corrupt dqblks.
> 
> We can quiet this down by doing the initial read without verifiers,
> knowing that we're going to validate the dqblks manually in later
> stages, and repair them if needed.
> 
> This adds new (more terse) messages stating whether a corrupted
> dquot was found and fixed.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> I'm a bit on the fence about this one; we lose the hexdump too so we won't
> see what was wrong (I could add that back in, I suppose).
> 
>  fs/xfs/libxfs/xfs_dquot_buf.c |  4 ++++
>  fs/xfs/xfs_qm.c               | 30 +++++++++++++-----------------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index 9f8b2c5..c13d440 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -136,6 +136,10 @@
>  				 XFS_DQUOT_CRC_OFF);
>  	}
>  
> +	xfs_alert(mp, "Repaired %s quota block for id %d.",
> +		  type == XFS_DQ_USER ? "user" :
> +		    (type == XFS_DQ_GROUP ? "group" : "project"), id);
> +
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index b422382..328d770 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -868,8 +868,12 @@ struct xfs_qm_isolate {
>  		 * xfs_dquot_verify.
>  		 */
>  		fa = xfs_dquot_verify(mp, &dqb[j], id + j, type);
> -		if (fa)
> +		if (fa) {
> +			xfs_alert(mp,
> +"Metadata corruption error detected at %pS, xfs_dquot block 0x%llx.",
> +				  __this_address, bp->b_bn);
>  			xfs_dquot_repair(mp, &dqb[j], id + j, type);
> +		}
>  
>  		/*
>  		 * Reset type in case we are reusing group quota file for
> @@ -922,24 +926,16 @@ struct xfs_qm_isolate {
>  	 * everything if we were to crash in the middle of this loop.
>  	 */
>  	while (blkcnt--) {
> -		error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
> -			      XFS_FSB_TO_DADDR(mp, bno),
> -			      mp->m_quotainfo->qi_dqchunklen, 0, &bp,
> -			      &xfs_dquot_buf_ops);
> -
>  		/*
> -		 * 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.
> +		 * CRC and validation errors are possible here.  Because
> +		 * this may occur, we read without CRC validation so that we can
> +		 * repair the damage via xfs_qm_reset_dqcounts().
> +		 * Errors will be detected, declared, and repaired later in the
> +		 * quotacheck process.
>  		 */
> -		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);
> -		}
> +		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;
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] xfs: factor out quota time limit initialization

[Eric Sandeen]

Factor out the time limit initialization from
xfs_qm_init_quotainfo so that we can delay it until
after quotacheck - it requires reading the default disk
quotas, which may need repair.

No functional changes in this patch.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

nb: the patch itself looks weird, what it really does is move the
timelimit setting /up/ in the file.  You just have to squint
at it.

 fs/xfs/xfs_qm.c | 91 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 40 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 328d770..a4da46c 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -594,47 +594,13 @@ struct xfs_qm_isolate {
 	}
 }
 
-/*
- * This initializes all the quota information that's kept in the
- * mount structure
- */
-STATIC int
-xfs_qm_init_quotainfo(
-	xfs_mount_t	*mp)
+STATIC void
+xfs_qm_set_timelimits(
+	struct xfs_mount	*mp,
+	struct xfs_quotainfo	*qinf)
 {
-	xfs_quotainfo_t *qinf;
-	int		error;
-	xfs_dquot_t	*dqp;
-
-	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
-
-	qinf = mp->m_quotainfo = kmem_zalloc(sizeof(xfs_quotainfo_t), KM_SLEEP);
-
-	error = list_lru_init(&qinf->qi_lru);
-	if (error)
-		goto out_free_qinf;
-
-	/*
-	 * See if quotainodes are setup, and if not, allocate them,
-	 * and change the superblock accordingly.
-	 */
-	error = xfs_qm_init_quotainos(mp);
-	if (error)
-		goto out_free_lru;
-
-	INIT_RADIX_TREE(&qinf->qi_uquota_tree, GFP_NOFS);
-	INIT_RADIX_TREE(&qinf->qi_gquota_tree, GFP_NOFS);
-	INIT_RADIX_TREE(&qinf->qi_pquota_tree, GFP_NOFS);
-	mutex_init(&qinf->qi_tree_lock);
-
-	/* mutex used to serialize quotaoffs */
-	mutex_init(&qinf->qi_quotaofflock);
-
-	/* Precalc some constants */
-	qinf->qi_dqchunklen = XFS_FSB_TO_BB(mp, XFS_DQUOT_CLUSTER_SIZE_FSB);
-	qinf->qi_dqperchunk = xfs_calc_dquots_per_chunk(qinf->qi_dqchunklen);
-
-	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
+	int			error;
+	xfs_dquot_t		*dqp;
 
 	/*
 	 * We try to get the limits from the superuser's limits fields.
@@ -691,6 +657,51 @@ struct xfs_qm_isolate {
 		xfs_qm_set_defquota(mp, XFS_DQ_PROJ, qinf);
 
 	qinf->qi_shrinker.count_objects = xfs_qm_shrink_count;
+}
+
+/*
+ * This initializes all the quota information that's kept in the
+ * mount structure
+ */
+STATIC int
+xfs_qm_init_quotainfo(
+	xfs_mount_t	*mp)
+{
+	xfs_quotainfo_t *qinf;
+	int		error;
+
+	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
+
+	qinf = mp->m_quotainfo = kmem_zalloc(sizeof(xfs_quotainfo_t), KM_SLEEP);
+
+	error = list_lru_init(&qinf->qi_lru);
+	if (error)
+		goto out_free_qinf;
+
+	/*
+	 * See if quotainodes are setup, and if not, allocate them,
+	 * and change the superblock accordingly.
+	 */
+	error = xfs_qm_init_quotainos(mp);
+	if (error)
+		goto out_free_lru;
+
+	INIT_RADIX_TREE(&qinf->qi_uquota_tree, GFP_NOFS);
+	INIT_RADIX_TREE(&qinf->qi_gquota_tree, GFP_NOFS);
+	INIT_RADIX_TREE(&qinf->qi_pquota_tree, GFP_NOFS);
+	mutex_init(&qinf->qi_tree_lock);
+
+	/* mutex used to serialize quotaoffs */
+	mutex_init(&qinf->qi_quotaofflock);
+
+	/* Precalc some constants */
+	qinf->qi_dqchunklen = XFS_FSB_TO_BB(mp, XFS_DQUOT_CLUSTER_SIZE_FSB);
+	qinf->qi_dqperchunk = xfs_calc_dquots_per_chunk(qinf->qi_dqchunklen);
+
+	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
+
+	xfs_qm_set_timelimits(mp, qinf);
+
 	qinf->qi_shrinker.scan_objects = xfs_qm_shrink_scan;
 	qinf->qi_shrinker.seeks = DEFAULT_SEEKS;
 	qinf->qi_shrinker.flags = SHRINKER_NUMA_AWARE;
-- 
1.8.3.1

Re: [PATCH 5/6] xfs: factor out quota time limit initialization

[Christoph Hellwig]

On Wed, Apr 04, 2018 at 02:10:20PM -0500, Eric Sandeen wrote:
> Factor out the time limit initialization from
> xfs_qm_init_quotainfo so that we can delay it until
> after quotacheck - it requires reading the default disk
> quotas, which may need repair.
> 
> No functional changes in this patch.

You have 75 characters per line for your changelog, use them!

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/6] xfs: factor out quota time limit initialization

[Eric Sandeen]

On 4/5/18 2:15 AM, Christoph Hellwig wrote:
> On Wed, Apr 04, 2018 at 02:10:20PM -0500, Eric Sandeen wrote:
>> Factor out the time limit initialization from
>> xfs_qm_init_quotainfo so that we can delay it until
>> after quotacheck - it requires reading the default disk
>> quotas, which may need repair.
>>
>> No functional changes in this patch.
> 
> You have 75 characters per line for your changelog, use them!

Sorry, bad habit.  I never remember exactly how many and sometimes I just
keep shortening...
 
The public shaming should help, I'll try to do better.  :)

-Eric

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/6] xfs: factor out quota time limit initialization

[Dave Chinner]

On Thu, Apr 05, 2018 at 07:36:58AM -0500, Eric Sandeen wrote:
> 
> 
> On 4/5/18 2:15 AM, Christoph Hellwig wrote:
> > On Wed, Apr 04, 2018 at 02:10:20PM -0500, Eric Sandeen wrote:
> >> Factor out the time limit initialization from
> >> xfs_qm_init_quotainfo so that we can delay it until
> >> after quotacheck - it requires reading the default disk
> >> quotas, which may need repair.
> >>
> >> No functional changes in this patch.
> > 
> > You have 75 characters per line for your changelog, use them!
> 
> Sorry, bad habit.  I never remember exactly how many and sometimes I just
> keep shortening...

If you use vim, set this up so you get automatic setting of the text
width (with highlighting) depending on the editor context you are
running:

" set default text width and shortcut to set custom widths
set     textwidth=80
map	#tw :set textwidth=<C-R>=col(".")<C-M>

" set the textwidth to 68 characters for commit messages
au	BufNewFile,BufRead guilt.msg.*,.gitsendemail.*,git.*,*/.git/* set tw=68

" set the textwidth to 68 characters for replies (email&usenet)
au	BufNewFile,BufRead .letter,mutt*,nn.*,snd.* set tw=68

" highlight textwidth
set	cc=+1


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

Re: [PATCH 5/6] xfs: factor out quota time limit initialization

[Darrick J. Wong]

On Wed, Apr 04, 2018 at 02:10:20PM -0500, Eric Sandeen wrote:
> Factor out the time limit initialization from
> xfs_qm_init_quotainfo so that we can delay it until
> after quotacheck - it requires reading the default disk
> quotas, which may need repair.
> 
> No functional changes in this patch.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> nb: the patch itself looks weird, what it really does is move the
> timelimit setting /up/ in the file.  You just have to squint
> at it.
> 
>  fs/xfs/xfs_qm.c | 91 ++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 51 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 328d770..a4da46c 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -594,47 +594,13 @@ struct xfs_qm_isolate {
>  	}
>  }
>  
> -/*
> - * This initializes all the quota information that's kept in the
> - * mount structure
> - */
> -STATIC int
> -xfs_qm_init_quotainfo(
> -	xfs_mount_t	*mp)
> +STATIC void
> +xfs_qm_set_timelimits(
> +	struct xfs_mount	*mp,
> +	struct xfs_quotainfo	*qinf)
>  {
> -	xfs_quotainfo_t *qinf;
> -	int		error;
> -	xfs_dquot_t	*dqp;
> -
> -	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> -
> -	qinf = mp->m_quotainfo = kmem_zalloc(sizeof(xfs_quotainfo_t), KM_SLEEP);
> -
> -	error = list_lru_init(&qinf->qi_lru);
> -	if (error)
> -		goto out_free_qinf;
> -
> -	/*
> -	 * See if quotainodes are setup, and if not, allocate them,
> -	 * and change the superblock accordingly.
> -	 */
> -	error = xfs_qm_init_quotainos(mp);
> -	if (error)
> -		goto out_free_lru;
> -
> -	INIT_RADIX_TREE(&qinf->qi_uquota_tree, GFP_NOFS);
> -	INIT_RADIX_TREE(&qinf->qi_gquota_tree, GFP_NOFS);
> -	INIT_RADIX_TREE(&qinf->qi_pquota_tree, GFP_NOFS);
> -	mutex_init(&qinf->qi_tree_lock);
> -
> -	/* mutex used to serialize quotaoffs */
> -	mutex_init(&qinf->qi_quotaofflock);
> -
> -	/* Precalc some constants */
> -	qinf->qi_dqchunklen = XFS_FSB_TO_BB(mp, XFS_DQUOT_CLUSTER_SIZE_FSB);
> -	qinf->qi_dqperchunk = xfs_calc_dquots_per_chunk(qinf->qi_dqchunklen);
> -
> -	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
> +	int			error;
> +	xfs_dquot_t		*dqp;
>  
>  	/*
>  	 * We try to get the limits from the superuser's limits fields.
> @@ -691,6 +657,51 @@ struct xfs_qm_isolate {
>  		xfs_qm_set_defquota(mp, XFS_DQ_PROJ, qinf);
>  
>  	qinf->qi_shrinker.count_objects = xfs_qm_shrink_count;
> +}
> +
> +/*
> + * This initializes all the quota information that's kept in the
> + * mount structure
> + */
> +STATIC int
> +xfs_qm_init_quotainfo(
> +	xfs_mount_t	*mp)
> +{
> +	xfs_quotainfo_t *qinf;
> +	int		error;
> +
> +	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> +
> +	qinf = mp->m_quotainfo = kmem_zalloc(sizeof(xfs_quotainfo_t), KM_SLEEP);
> +
> +	error = list_lru_init(&qinf->qi_lru);
> +	if (error)
> +		goto out_free_qinf;
> +
> +	/*
> +	 * See if quotainodes are setup, and if not, allocate them,
> +	 * and change the superblock accordingly.
> +	 */
> +	error = xfs_qm_init_quotainos(mp);
> +	if (error)
> +		goto out_free_lru;
> +
> +	INIT_RADIX_TREE(&qinf->qi_uquota_tree, GFP_NOFS);
> +	INIT_RADIX_TREE(&qinf->qi_gquota_tree, GFP_NOFS);
> +	INIT_RADIX_TREE(&qinf->qi_pquota_tree, GFP_NOFS);
> +	mutex_init(&qinf->qi_tree_lock);
> +
> +	/* mutex used to serialize quotaoffs */
> +	mutex_init(&qinf->qi_quotaofflock);
> +
> +	/* Precalc some constants */
> +	qinf->qi_dqchunklen = XFS_FSB_TO_BB(mp, XFS_DQUOT_CLUSTER_SIZE_FSB);
> +	qinf->qi_dqperchunk = xfs_calc_dquots_per_chunk(qinf->qi_dqchunklen);
> +
> +	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
> +
> +	xfs_qm_set_timelimits(mp, qinf);
> +
>  	qinf->qi_shrinker.scan_objects = xfs_qm_shrink_scan;
>  	qinf->qi_shrinker.seeks = DEFAULT_SEEKS;
>  	qinf->qi_shrinker.flags = SHRINKER_NUMA_AWARE;
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6 V2] xfs: factor out quota time limit initialization

[Eric Sandeen]

Factor out the time limit initialization from xfs_qm_init_quotainfo
so that we can delay it until after quotacheck - it requires reading
the default disk quotas, which may need repair.

No functional changes in this patch.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: nice wide changelog :D  No patch changes.

nb: the patch itself looks weird, what it really does is move the
timelimit setting /up/ in the file.  You just have to squint
at it.

 fs/xfs/xfs_qm.c | 91 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 40 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 328d770..a4da46c 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -594,47 +594,13 @@ struct xfs_qm_isolate {
 	}
 }
 
-/*
- * This initializes all the quota information that's kept in the
- * mount structure
- */
-STATIC int
-xfs_qm_init_quotainfo(
-	xfs_mount_t	*mp)
+STATIC void
+xfs_qm_set_timelimits(
+	struct xfs_mount	*mp,
+	struct xfs_quotainfo	*qinf)
 {
-	xfs_quotainfo_t *qinf;
-	int		error;
-	xfs_dquot_t	*dqp;
-
-	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
-
-	qinf = mp->m_quotainfo = kmem_zalloc(sizeof(xfs_quotainfo_t), KM_SLEEP);
-
-	error = list_lru_init(&qinf->qi_lru);
-	if (error)
-		goto out_free_qinf;
-
-	/*
-	 * See if quotainodes are setup, and if not, allocate them,
-	 * and change the superblock accordingly.
-	 */
-	error = xfs_qm_init_quotainos(mp);
-	if (error)
-		goto out_free_lru;
-
-	INIT_RADIX_TREE(&qinf->qi_uquota_tree, GFP_NOFS);
-	INIT_RADIX_TREE(&qinf->qi_gquota_tree, GFP_NOFS);
-	INIT_RADIX_TREE(&qinf->qi_pquota_tree, GFP_NOFS);
-	mutex_init(&qinf->qi_tree_lock);
-
-	/* mutex used to serialize quotaoffs */
-	mutex_init(&qinf->qi_quotaofflock);
-
-	/* Precalc some constants */
-	qinf->qi_dqchunklen = XFS_FSB_TO_BB(mp, XFS_DQUOT_CLUSTER_SIZE_FSB);
-	qinf->qi_dqperchunk = xfs_calc_dquots_per_chunk(qinf->qi_dqchunklen);
-
-	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
+	int			error;
+	xfs_dquot_t		*dqp;
 
 	/*
 	 * We try to get the limits from the superuser's limits fields.
@@ -691,6 +657,51 @@ struct xfs_qm_isolate {
 		xfs_qm_set_defquota(mp, XFS_DQ_PROJ, qinf);
 
 	qinf->qi_shrinker.count_objects = xfs_qm_shrink_count;
+}
+
+/*
+ * This initializes all the quota information that's kept in the
+ * mount structure
+ */
+STATIC int
+xfs_qm_init_quotainfo(
+	xfs_mount_t	*mp)
+{
+	xfs_quotainfo_t *qinf;
+	int		error;
+
+	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
+
+	qinf = mp->m_quotainfo = kmem_zalloc(sizeof(xfs_quotainfo_t), KM_SLEEP);
+
+	error = list_lru_init(&qinf->qi_lru);
+	if (error)
+		goto out_free_qinf;
+
+	/*
+	 * See if quotainodes are setup, and if not, allocate them,
+	 * and change the superblock accordingly.
+	 */
+	error = xfs_qm_init_quotainos(mp);
+	if (error)
+		goto out_free_lru;
+
+	INIT_RADIX_TREE(&qinf->qi_uquota_tree, GFP_NOFS);
+	INIT_RADIX_TREE(&qinf->qi_gquota_tree, GFP_NOFS);
+	INIT_RADIX_TREE(&qinf->qi_pquota_tree, GFP_NOFS);
+	mutex_init(&qinf->qi_tree_lock);
+
+	/* mutex used to serialize quotaoffs */
+	mutex_init(&qinf->qi_quotaofflock);
+
+	/* Precalc some constants */
+	qinf->qi_dqchunklen = XFS_FSB_TO_BB(mp, XFS_DQUOT_CLUSTER_SIZE_FSB);
+	qinf->qi_dqperchunk = xfs_calc_dquots_per_chunk(qinf->qi_dqchunklen);
+
+	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
+
+	xfs_qm_set_timelimits(mp, qinf);
+
 	qinf->qi_shrinker.scan_objects = xfs_qm_shrink_scan;
 	qinf->qi_shrinker.seeks = DEFAULT_SEEKS;
 	qinf->qi_shrinker.flags = SHRINKER_NUMA_AWARE;
-- 
1.8.3.1

[PATCH 6/6] xfs: delay quota timelimit init until after quotacheck

[Eric Sandeen]

Initializing the time limits in the quota info requires reading
the default quota block.  If it's corrupt, this yields even more
dmesg spew before kernelspace gets around to properly detecting
and repairing the corruption.

If we move the read and initialization until post-quotacheck,
we can avoid the noisy read if it's corrupted.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_qm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index a4da46c..95487cb4 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -698,9 +698,9 @@ struct xfs_qm_isolate {
 	qinf->qi_dqchunklen = XFS_FSB_TO_BB(mp, XFS_DQUOT_CLUSTER_SIZE_FSB);
 	qinf->qi_dqperchunk = xfs_calc_dquots_per_chunk(qinf->qi_dqchunklen);
 
-	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
+	/* Default quota will be read post-quotacheck to set timelimits */
 
-	xfs_qm_set_timelimits(mp, qinf);
+	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
 
 	qinf->qi_shrinker.scan_objects = xfs_qm_shrink_scan;
 	qinf->qi_shrinker.seeks = DEFAULT_SEEKS;
@@ -1469,6 +1469,10 @@ struct xfs_qm_isolate {
 			return;
 		}
 	}
+
+	/* Now that quotacheck is done, set time limits */
+	xfs_qm_set_timelimits(mp, mp->m_quotainfo);
+
 	/*
 	 * If one type of quotas is off, then it will lose its
 	 * quotachecked status, since we won't be doing accounting for
-- 
1.8.3.1

Re: [PATCH 6/6] xfs: delay quota timelimit init until after quotacheck

[Christoph Hellwig]

On Wed, Apr 04, 2018 at 02:12:40PM -0500, Eric Sandeen wrote:
> Initializing the time limits in the quota info requires reading
> the default quota block.  If it's corrupt, this yields even more
> dmesg spew before kernelspace gets around to properly detecting
> and repairing the corruption.
> 
> If we move the read and initialization until post-quotacheck,
> we can avoid the noisy read if it's corrupted.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 6/6] xfs: delay quota timelimit init until after quotacheck

[Darrick J. Wong]

On Wed, Apr 04, 2018 at 02:12:40PM -0500, Eric Sandeen wrote:
> Initializing the time limits in the quota info requires reading
> the default quota block.  If it's corrupt, this yields even more
> dmesg spew before kernelspace gets around to properly detecting
> and repairing the corruption.
> 
> If we move the read and initialization until post-quotacheck,
> we can avoid the noisy read if it's corrupted.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_qm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index a4da46c..95487cb4 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -698,9 +698,9 @@ struct xfs_qm_isolate {
>  	qinf->qi_dqchunklen = XFS_FSB_TO_BB(mp, XFS_DQUOT_CLUSTER_SIZE_FSB);
>  	qinf->qi_dqperchunk = xfs_calc_dquots_per_chunk(qinf->qi_dqchunklen);
>  
> -	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
> +	/* Default quota will be read post-quotacheck to set timelimits */
>  
> -	xfs_qm_set_timelimits(mp, qinf);
> +	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
>  
>  	qinf->qi_shrinker.scan_objects = xfs_qm_shrink_scan;
>  	qinf->qi_shrinker.seeks = DEFAULT_SEEKS;
> @@ -1469,6 +1469,10 @@ struct xfs_qm_isolate {
>  			return;
>  		}
>  	}
> +
> +	/* Now that quotacheck is done, set time limits */
> +	xfs_qm_set_timelimits(mp, mp->m_quotainfo);
> +
>  	/*
>  	 * If one type of quotas is off, then it will lose its
>  	 * quotachecked status, since we won't be doing accounting for
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] xfs: quota fixes and enhancements

[Eric Sandeen]

On 4/4/18 1:47 PM, Eric Sandeen wrote:
> A semi-random smattering of quota stuff.  First three seem quite
> good to go, the rest are more along the lines of a suggestion
> or conversation-starter.  ;)
> 
> (the first patch is just removing an unused arg).
> 
> xfs_repair doesn't look at quota blocks.  At all.  It relies
> on quotacheck in the kernel to fix them up as needed.

I'm starting to rethink a lot of this hackery.  Why doesn't xfs_repair
just fix things up?  (leave quotacheck to the next mount, but the
"repair" stuff in the kernel seems like a really strange wart.)

I think I'll look at teaching repair to sanity check the quota
inodes, but if anyone knows why that's a bad idea please let me
know.  ;)

Thanks,
-Eric

Re: [PATCH 0/6] xfs: quota fixes and enhancements

[Darrick J. Wong]

On Sat, Apr 07, 2018 at 05:00:13PM -0500, Eric Sandeen wrote:
> On 4/4/18 1:47 PM, Eric Sandeen wrote:
> > A semi-random smattering of quota stuff.  First three seem quite
> > good to go, the rest are more along the lines of a suggestion
> > or conversation-starter.  ;)
> > 
> > (the first patch is just removing an unused arg).
> > 
> > xfs_repair doesn't look at quota blocks.  At all.  It relies
> > on quotacheck in the kernel to fix them up as needed.
> 
> I'm starting to rethink a lot of this hackery.  Why doesn't xfs_repair
> just fix things up?  (leave quotacheck to the next mount, but the
> "repair" stuff in the kernel seems like a really strange wart.)
> 
> I think I'll look at teaching repair to sanity check the quota
> inodes, but if anyone knows why that's a bad idea please let me
> know.  ;)

/me shrugs, we still need to fix the kernel's quota verifiers to check
the uuid and all that, right?  Which means that both are going to need
patches, afaict.

--D

> Thanks,
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] xfs: quota fixes and enhancements

[Eric Sandeen]

On 4/7/18 8:37 PM, Darrick J. Wong wrote:
> On Sat, Apr 07, 2018 at 05:00:13PM -0500, Eric Sandeen wrote:
>> On 4/4/18 1:47 PM, Eric Sandeen wrote:
>>> A semi-random smattering of quota stuff.  First three seem quite
>>> good to go, the rest are more along the lines of a suggestion
>>> or conversation-starter.  ;)
>>>
>>> (the first patch is just removing an unused arg).
>>>
>>> xfs_repair doesn't look at quota blocks.  At all.  It relies
>>> on quotacheck in the kernel to fix them up as needed.
>>
>> I'm starting to rethink a lot of this hackery.  Why doesn't xfs_repair
>> just fix things up?  (leave quotacheck to the next mount, but the
>> "repair" stuff in the kernel seems like a really strange wart.)
>>
>> I think I'll look at teaching repair to sanity check the quota
>> inodes, but if anyone knows why that's a bad idea please let me
>> know.  ;)
> 
> /me shrugs, we still need to fix the kernel's quota verifiers to check
> the uuid and all that, right?  Which means that both are going to need
> patches, afaict.

The verifiers do check UUID:

xfs_dquot_buf_read_verify
	xfs_dquot_buf_verify_crc
                if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_meta_uuid))
                        return false;

the issue is that neither the kernel nor userspace repairs this error
if it's detected.

-Eric