[14/17] xfs: clean up xfs_trans_reserve_quota_chown a bit

Commit Message

Darrick J. Wong Feb. 1, 2021, 2:05 a.m. UTC

From: Darrick J. Wong <[e-mail]>

Clean up the calling conventions of xfs_trans_reserve_quota_chown a
little bit -- we can pass in a boolean force parameter since that's the
only qmopt that caller care about, and make the obvious deficiencies
more obvious for anyone who someday wants to clean up rt quota.

Signed-off-by: Darrick J. Wong <[e-mail]>
---
 fs/xfs/xfs_ioctl.c       |    2 +-
 fs/xfs/xfs_iops.c        |    3 +--
 fs/xfs/xfs_quota.h       |    4 ++--
 fs/xfs/xfs_trans_dquot.c |   37 ++++++++++++++++++++-----------------
 4 files changed, 24 insertions(+), 22 deletions(-)

Comments

Christoph Hellwig Feb. 1, 2021, 12:18 p.m. UTC | #1

>  
>  	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
>  	    i_uid_read(VFS_I(ip)) != udqp->q_id)
> -		udq_delblks = udqp;
> +		new_udqp = udqp;

We don't even need two variables for each type, instead we can just
clear the original pointer to NULL if the conditions aren't met.

Darrick J. Wong Feb. 1, 2021, 6:27 p.m. UTC | #2

On Mon, Feb 01, 2021 at 12:18:34PM +0000, Christoph Hellwig wrote:
> >  
> >  	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> >  	    i_uid_read(VFS_I(ip)) != udqp->q_id)
> > -		udq_delblks = udqp;
> > +		new_udqp = udqp;
> 
> We don't even need two variables for each type, instead we can just
> clear the original pointer to NULL if the conditions aren't met.

Once I've made xfs_trans_alloc_ichange the only caller of
xfs_trans_reserve_quota_chown, I think we could get rid of these checks
entirely, since there ought to be a 1:1 correspondence between (i_udquot
== new_udqp) and (i_uid_read(VFS_I(ip)) == new_udqp->q_id), if the
caller even passed in a new_udqp.

Though... if I take your (later) suggestion to move the call to
xfs_trans_reserve_quota_bydquots into xfs_trans_alloc_ichange, then it
makes more sense to drop all these patches.

--D

Patch

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index e299fbd9ef13..73cfee8007a8 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1471,7 +1471,7 @@  xfs_ioctl_setattr(
 	if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) &&
 	    ip->i_d.di_projid != fa->fsx_projid) {
 		code = xfs_trans_reserve_quota_chown(tp, ip, NULL, NULL, pdqp,
-				capable(CAP_FOWNER) ?  XFS_QMOPT_FORCE_RES : 0);
+				capable(CAP_FOWNER));
 		if (code)	/* out of quota */
 			goto error_trans_cancel;
 	}
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index cb68be87e0a4..51c877ce90bc 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -731,8 +731,7 @@  xfs_setattr_nonsize(
 		     (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
 			ASSERT(tp);
 			error = xfs_trans_reserve_quota_chown(tp, ip, udqp,
-					gdqp, NULL, capable(CAP_FOWNER) ?
-					XFS_QMOPT_FORCE_RES : 0);
+					gdqp, NULL, capable(CAP_FOWNER));
 			if (error)	/* out of quota */
 				goto out_cancel;
 		}
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 02f88670c2d9..f95a92d7ceaf 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -100,7 +100,7 @@  extern struct xfs_dquot *xfs_qm_vop_chown(struct xfs_trans *,
 		struct xfs_inode *, struct xfs_dquot **, struct xfs_dquot *);
 int xfs_trans_reserve_quota_chown(struct xfs_trans *tp, struct xfs_inode *ip,
 		struct xfs_dquot *udqp, struct xfs_dquot *gdqp,
-		struct xfs_dquot *pdqp, uint flags);
+		struct xfs_dquot *pdqp, bool force);
 extern int xfs_qm_dqattach(struct xfs_inode *);
 extern int xfs_qm_dqattach_locked(struct xfs_inode *ip, bool doalloc);
 extern void xfs_qm_dqdetach(struct xfs_inode *);
@@ -165,7 +165,7 @@  xfs_trans_reserve_quota_icreate(struct xfs_trans *tp, struct xfs_dquot *udqp,
 static inline int
 xfs_trans_reserve_quota_chown(struct xfs_trans *tp, struct xfs_inode *ip,
 		struct xfs_dquot *udqp, struct xfs_dquot *gdqp,
-		struct xfs_dquot *pdqp, uint flags)
+		struct xfs_dquot *pdqp, bool force)
 {
 	return 0;
 }
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 3595c779f5d3..a02311e8be25 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -836,9 +836,7 @@  xfs_trans_reserve_quota_icreate(
 			dblocks, 1, XFS_QMOPT_RES_REGBLKS);
 }
 
-/*
- * Quota reservations for setattr(AT_UID|AT_GID|AT_PROJID).
- */
+/* Change quota reservations for a change in user, group, or project id. */
 int
 xfs_trans_reserve_quota_chown(
 	struct xfs_trans	*tp,
@@ -846,31 +844,36 @@  xfs_trans_reserve_quota_chown(
 	struct xfs_dquot	*udqp,
 	struct xfs_dquot	*gdqp,
 	struct xfs_dquot	*pdqp,
-	uint			flags)
+	bool			force)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	unsigned int		blkflags;
-	struct xfs_dquot	*udq_delblks = NULL;
-	struct xfs_dquot	*gdq_delblks = NULL;
-	struct xfs_dquot	*pdq_delblks = NULL;
+	struct xfs_dquot	*new_udqp = NULL;
+	struct xfs_dquot	*new_gdqp = NULL;
+	struct xfs_dquot	*new_pdqp = NULL;
+	unsigned int		qflags = XFS_QMOPT_RES_REGBLKS;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	/*
+	 * XXX: This function doesn't handle rt quota counts correctly.  We
+	 * don't support mounting with rt+quota so leave this breadcrumb.
+	 */
+	ASSERT(!XFS_IS_REALTIME_INODE(ip));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
-	blkflags = XFS_IS_REALTIME_INODE(ip) ?
-			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
+	if (force)
+		qflags |= XFS_QMOPT_FORCE_RES;
 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
 	    i_uid_read(VFS_I(ip)) != udqp->q_id)
-		udq_delblks = udqp;
+		new_udqp = udqp;
 
 	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
 	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
-		gdq_delblks = gdqp;
+		new_gdqp = gdqp;
 
 	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
 	    ip->i_d.di_projid != pdqp->q_id)
-		pdq_delblks = pdqp;
+		new_pdqp = pdqp;
 
 	/*
 	 * Reserve enough quota to handle blocks on disk and reserved for a
@@ -878,10 +881,10 @@  xfs_trans_reserve_quota_chown(
 	 * reservation between dquots at chown time, even though that part is
 	 * only semi-transactional.
 	 */
-	return xfs_trans_reserve_quota_bydquots(tp, ip->i_mount, udq_delblks,
-			gdq_delblks, pdq_delblks,
+	return xfs_trans_reserve_quota_bydquots(tp, ip->i_mount, new_udqp,
+			new_gdqp, new_pdqp,
 			ip->i_d.di_nblocks + ip->i_delayed_blks,
-			1, blkflags | flags);
+			1, qflags);
 }
 
 /*