Re: [PATCH 05/26] xfs: split the incore dquot type into a separate field

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jul 15, 2020 at 06:43:40PM +0100, Christoph Hellwig wrote:
> On Tue, Jul 14, 2020 at 11:05:02AM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 14, 2020 at 08:57:56AM +0100, Christoph Hellwig wrote:
> > > On Mon, Jul 13, 2020 at 06:32:00PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Create a new type (xfs_dqtype_t) to represent the type of an incore
> > > > dquot.  Break the type field out from the dq_flags field of the incore
> > > > dquot.
> > > 
> > > I don't understand why we need separate in-core vs on-disk values for
> > > the type.  Why not something like this on top of the whole series:
> > 
> > I want to keep the ondisk d_type values separate from the incore q_type
> > values because they don't describe exactly the same concepts:
> > 
> > First, the incore qtype has a NONE value that we can pass to the dquot
> > core verifier when we don't actually know if this is a user, group, or
> > project dquot.  This should never end up on disk.
> 
> Which we can trivially verify.  Or just get rid of NONE, which actually
> cleans things up a fair bit (patch on top of my previous one below)

Ok, I'll get rid of that usage.

> > Second, xfs_dqtype_t is a (barely concealed) enumeration type for quota
> > callers to tell us that they want to perform an action on behalf of
> > user, group, or project quotas.  The incore q_flags and the ondisk
> > d_type contain internal state that should not be exposed to quota
> > callers.
> 
> I don't think that is an argument, as we do the same elsewhere.
> 
> > 
> > I feel a need to reiterate that I'm about to start adding more flags to
> > d_type (for y2038+ time support), for which it will be very important to
> > keep d_type and q_{type,flags} separate.
> 
> Why?  We'll just OR the bigtime flag in before writing to disk.

Ugh, fine, I'll rework the whole series yet again, since it doesn't look
like anyone else is going to have the time to review a 27 patch cleanup
series.

--D

> 
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index ef9b8559ff6197..c48b77c223073e 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -37,11 +37,8 @@ xfs_failaddr_t
>  xfs_dquot_verify(
>  	struct xfs_mount	*mp,
>  	struct xfs_disk_dquot	*ddq,
> -	xfs_dqid_t		id,
> -	xfs_dqtype_t		type)	/* used only during quotacheck */
> +	xfs_dqid_t		id)
>  {
> -	uint8_t			ddq_type;
> -
>  	/*
>  	 * We can encounter an uninitialized dquot buffer for 2 reasons:
>  	 * 1. If we crash while deleting the quotainode(s), and those blks got
> @@ -64,13 +61,14 @@ xfs_dquot_verify(
>  
>  	if (ddq->d_type & ~XFS_DQTYPE_ANY)
>  		return __this_address;
> -	ddq_type = ddq->d_type & XFS_DQTYPE_REC_MASK;
> -	if (type != XFS_DQTYPE_NONE && ddq_type != type)
> -		return __this_address;
> -	if (ddq_type != XFS_DQTYPE_USER &&
> -	    ddq_type != XFS_DQTYPE_PROJ &&
> -	    ddq_type != XFS_DQTYPE_GROUP)
> +	switch (ddq->d_type & XFS_DQTYPE_REC_MASK) {
> +	case XFS_DQTYPE_USER:
> +	case XFS_DQTYPE_PROJ:
> +	case XFS_DQTYPE_GROUP:
> +		break;
> +	default:
>  		return __this_address;
> +	}
>  
>  	if (id != -1 && id != be32_to_cpu(ddq->d_id))
>  		return __this_address;
> @@ -100,14 +98,12 @@ xfs_failaddr_t
>  xfs_dqblk_verify(
>  	struct xfs_mount	*mp,
>  	struct xfs_dqblk	*dqb,
> -	xfs_dqid_t		id,
> -	xfs_dqtype_t		type)	/* used only during quotacheck */
> +	xfs_dqid_t		id)
>  {
>  	if (xfs_sb_version_hascrc(&mp->m_sb) &&
>  	    !uuid_equal(&dqb->dd_uuid, &mp->m_sb.sb_meta_uuid))
>  		return __this_address;
> -
> -	return xfs_dquot_verify(mp, &dqb->dd_diskdq, id, type);
> +	return xfs_dquot_verify(mp, &dqb->dd_diskdq, id);
>  }
>  
>  /*
> @@ -210,7 +206,7 @@ xfs_dquot_buf_verify(
>  		if (i == 0)
>  			id = be32_to_cpu(ddq->d_id);
>  
> -		fa = xfs_dqblk_verify(mp, &dqb[i], id + i, XFS_DQTYPE_NONE);
> +		fa = xfs_dqblk_verify(mp, &dqb[i], id + i);
>  		if (fa) {
>  			if (!readahead)
>  				xfs_buf_verifier_error(bp, -EFSCORRUPTED,
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index e4178c804abf06..ec472131ea4b15 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1150,13 +1150,11 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
>  #define XFS_DQUOT_MAGIC		0x4451		/* 'DQ' */
>  #define XFS_DQUOT_VERSION	(uint8_t)0x01	/* latest version number */
>  
> -#define XFS_DQTYPE_NONE		0
>  #define XFS_DQTYPE_USER		0x01		/* user dquot record */
>  #define XFS_DQTYPE_PROJ		0x02		/* project dquot record */
>  #define XFS_DQTYPE_GROUP	0x04		/* group dquot record */
>  
>  #define XFS_DQTYPE_STRINGS \
> -	{ XFS_DQTYPE_NONE,	"NONE" }, \
>  	{ XFS_DQTYPE_USER,	"USER" }, \
>  	{ XFS_DQTYPE_PROJ,	"PROJ" }, \
>  	{ XFS_DQTYPE_GROUP,	"GROUP" }
> diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
> index 6edd249fdef4ea..5e3d6b49981707 100644
> --- a/fs/xfs/libxfs/xfs_quota_defs.h
> +++ b/fs/xfs/libxfs/xfs_quota_defs.h
> @@ -129,9 +129,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, xfs_dqtype_t type);
> +		struct xfs_disk_dquot *ddq, xfs_dqid_t id);
>  extern xfs_failaddr_t xfs_dqblk_verify(struct xfs_mount *mp,
> -		struct xfs_dqblk *dqb, xfs_dqid_t id, xfs_dqtype_t type);
> +		struct xfs_dqblk *dqb, xfs_dqid_t id);
>  extern int xfs_calc_dquots_per_chunk(unsigned int nbblks);
>  extern void xfs_dqblk_repair(struct xfs_mount *mp, struct xfs_dqblk *dqb,
>  		xfs_dqid_t id, xfs_dqtype_t type);
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index f045895f28ffb1..a8eace16fdae74 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -31,7 +31,7 @@ xchk_quota_to_dqtype(
>  		return XFS_DQTYPE_PROJ;
>  	default:
>  		ASSERT(0);
> -		return XFS_DQTYPE_NONE;
> +		return 0;
>  	}
>  }
>  
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index c2f19d35e05dbd..e4f37c064497aa 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -494,8 +494,7 @@ xlog_recover_do_reg_buffer(
>  					item->ri_buf[i].i_len, __func__);
>  				goto next;
>  			}
> -			fa = xfs_dquot_verify(mp, item->ri_buf[i].i_addr,
> -					       -1, XFS_DQTYPE_NONE);
> +			fa = xfs_dquot_verify(mp, item->ri_buf[i].i_addr, -1);
>  			if (fa) {
>  				xfs_alert(mp,
>  	"dquot corrupt at %pS trying to replay into block 0x%llx",
> diff --git a/fs/xfs/xfs_dquot_item_recover.c b/fs/xfs/xfs_dquot_item_recover.c
> index b5afb9fb8cd4fd..2df6238ed19abc 100644
> --- a/fs/xfs/xfs_dquot_item_recover.c
> +++ b/fs/xfs/xfs_dquot_item_recover.c
> @@ -108,7 +108,7 @@ xlog_recover_dquot_commit_pass2(
>  	 */
>  	dq_f = item->ri_buf[0].i_addr;
>  	ASSERT(dq_f);
> -	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id, XFS_DQTYPE_NONE);
> +	fa = xfs_dquot_verify(mp, recddq, dq_f->qlf_id);
>  	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 96171f4406e978..716b91b582ff56 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -828,7 +828,6 @@ xfs_qm_reset_dqcounts(
>  {
>  	struct xfs_dqblk	*dqb;
>  	int			j;
> -	xfs_failaddr_t		fa;
>  
>  	trace_xfs_reset_dqcounts(bp, _RET_IP_);
>  
> @@ -853,8 +852,8 @@ xfs_qm_reset_dqcounts(
>  		 * find uninitialised dquot blks. See comment in
>  		 * xfs_dquot_verify.
>  		 */
> -		fa = xfs_dqblk_verify(mp, &dqb[j], id + j, type);
> -		if (fa)
> +		if (xfs_dqblk_verify(mp, &dqb[j], id + j, type) ||
> +		    dqb[j].d_type != type)
>  			xfs_dqblk_repair(mp, &dqb[j], id + j, type);
>  
>  		/*