[PATCHSET v6 00/16] xfs: minor cleanups of the quota functions

[PATCHSET v6 00/16] xfs: minor cleanups of the quota functions

[Darrick J. Wong]

Hi all,

This series reworks some of the internal quota APIs and cleans up some
of the function calls so that we have a clean(er) place to start the
space reclamation patchset.  The first five patches clean up the
existing quota transaction helpers.  The next five patches create a
common helper to allocate space, quota, and transaction to handle a file
modification.  The final three patches of the series create common
helpers to do more or less the same thing for file creation and chown
operations.  The goal of these changes is to reduce open-coded idioms,
which makes the job of the space reclamation patchset easier since we
can now (mostly) hide the retry loops within single functions.

v2: rework the xfs_quota_reserve_blkres calling conventions per hch
v3: create new xfs_trans_alloc* helpers that will take care of free
    space and quota reservation all at once for block allocations, inode
    creation, and chown operations, to simplify the subsequent patches.
v4: fix some jump labels, improve commit messages, call out a quota
    accounting fix on dax files, fix some locking conventions with
    reflink
v5: refactor the chown transaction allocation into a helper function,
    fix a longstanding quota bug where incore delalloc reservations were
    lost if fssetxattr failed, other cleanups
v6: streamline the chown quota reservation code by moving it to
    xfs_trans_alloc_ichange.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=quota-function-cleanups-5.12
---
 fs/xfs/libxfs/xfs_attr.c |   15 ----
 fs/xfs/libxfs/xfs_bmap.c |   23 ++-----
 fs/xfs/xfs_bmap_util.c   |   60 ++++--------------
 fs/xfs/xfs_inode.c       |   30 +++------
 fs/xfs/xfs_ioctl.c       |   67 ++++++++------------
 fs/xfs/xfs_iomap.c       |   54 +++++-----------
 fs/xfs/xfs_iops.c        |   26 +-------
 fs/xfs/xfs_qm.c          |  115 ++++++----------------------------
 fs/xfs/xfs_quota.h       |   49 ++++++++++----
 fs/xfs/xfs_reflink.c     |   71 ++++++++-------------
 fs/xfs/xfs_symlink.c     |   15 +---
 fs/xfs/xfs_trans.c       |  157 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.h       |   13 ++++
 fs/xfs/xfs_trans_dquot.c |   73 ++++++++++++++++-----
 14 files changed, 392 insertions(+), 376 deletions(-)

[PATCH 01/16] xfs: fix chown leaking delalloc quota blocks when fssetxattr fails

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

While refactoring the quota code to create a function to allocate inode
change transactions, I noticed that xfs_qm_vop_chown_reserve does more
than just make reservations: it also *modifies* the incore counts
directly to handle the owner id change for the delalloc blocks.

I then observed that the fssetxattr code continues validating input
arguments after making the quota reservation but before dirtying the
transaction.  If the routine decides to error out, it fails to undo the
accounting switch!  This leads to incorrect quota reservation and
failure down the line.

We can fix this by making the reservation function do only that -- for
the new dquot, it reserves ondisk and delalloc blocks to the
transaction, and the old dquot hangs on to its incore reservation for
now.  Once we actually switch the dquots, we can then update the incore
reservations because we've dirtied the transaction and it's too late to
turn back now.

No fixes tag because this has been broken since the start of git.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm.c |   91 +++++++++++++++++++++----------------------------------
 1 file changed, 34 insertions(+), 57 deletions(-)


diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index c134eb4aeaa8..322d337b5dca 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1785,6 +1785,28 @@ xfs_qm_vop_chown(
 	xfs_trans_mod_dquot(tp, newdq, bfield, ip->i_d.di_nblocks);
 	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_ICOUNT, 1);
 
+	/*
+	 * Back when we made quota reservations for the chown, we reserved the
+	 * ondisk blocks + delalloc blocks with the new dquot.  Now that we've
+	 * switched the dquots, decrease the new dquot's block reservation
+	 * (having already bumped up the real counter) so that we don't have
+	 * any reservation to give back when we commit.
+	 */
+	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_RES_BLKS,
+			-ip->i_delayed_blks);
+
+	/*
+	 * Give the incore reservation for delalloc blocks back to the old
+	 * dquot.  We don't normally handle delalloc quota reservations
+	 * transactionally, so just lock the dquot and subtract from the
+	 * reservation.  We've dirtied the transaction, so it's too late to
+	 * turn back now.
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	xfs_dqlock(prevdq);
+	prevdq->q_blk.reserved -= ip->i_delayed_blks;
+	xfs_dqunlock(prevdq);
+
 	/*
 	 * Take an extra reference, because the inode is going to keep
 	 * this dquot pointer even after the trans_commit.
@@ -1807,84 +1829,39 @@ xfs_qm_vop_chown_reserve(
 	uint			flags)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	uint64_t		delblks;
 	unsigned int		blkflags;
-	struct xfs_dquot	*udq_unres = NULL;
-	struct xfs_dquot	*gdq_unres = NULL;
-	struct xfs_dquot	*pdq_unres = NULL;
 	struct xfs_dquot	*udq_delblks = NULL;
 	struct xfs_dquot	*gdq_delblks = NULL;
 	struct xfs_dquot	*pdq_delblks = NULL;
-	int			error;
-
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
-	delblks = ip->i_delayed_blks;
 	blkflags = XFS_IS_REALTIME_INODE(ip) ?
 			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    i_uid_read(VFS_I(ip)) != udqp->q_id) {
+	    i_uid_read(VFS_I(ip)) != udqp->q_id)
 		udq_delblks = udqp;
-		/*
-		 * If there are delayed allocation blocks, then we have to
-		 * unreserve those from the old dquot, and add them to the
-		 * new dquot.
-		 */
-		if (delblks) {
-			ASSERT(ip->i_udquot);
-			udq_unres = ip->i_udquot;
-		}
-	}
+
 	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
-	    i_gid_read(VFS_I(ip)) != gdqp->q_id) {
+	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
 		gdq_delblks = gdqp;
-		if (delblks) {
-			ASSERT(ip->i_gdquot);
-			gdq_unres = ip->i_gdquot;
-		}
-	}
 
 	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
-	    ip->i_d.di_projid != pdqp->q_id) {
+	    ip->i_d.di_projid != pdqp->q_id)
 		pdq_delblks = pdqp;
-		if (delblks) {
-			ASSERT(ip->i_pdquot);
-			pdq_unres = ip->i_pdquot;
-		}
-	}
-
-	error = xfs_trans_reserve_quota_bydquots(tp, ip->i_mount,
-				udq_delblks, gdq_delblks, pdq_delblks,
-				ip->i_d.di_nblocks, 1, flags | blkflags);
-	if (error)
-		return error;
 
 	/*
-	 * Do the delayed blks reservations/unreservations now. Since, these
-	 * are done without the help of a transaction, if a reservation fails
-	 * its previous reservations won't be automatically undone by trans
-	 * code. So, we have to do it manually here.
+	 * Reserve enough quota to handle blocks on disk and reserved for a
+	 * delayed allocation.  We'll actually transfer the delalloc
+	 * reservation between dquots at chown time, even though that part is
+	 * only semi-transactional.
 	 */
-	if (delblks) {
-		/*
-		 * Do the reservations first. Unreservation can't fail.
-		 */
-		ASSERT(udq_delblks || gdq_delblks || pdq_delblks);
-		ASSERT(udq_unres || gdq_unres || pdq_unres);
-		error = xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
-			    udq_delblks, gdq_delblks, pdq_delblks,
-			    (xfs_qcnt_t)delblks, 0, flags | blkflags);
-		if (error)
-			return error;
-		xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
-				udq_unres, gdq_unres, pdq_unres,
-				-((xfs_qcnt_t)delblks), 0, blkflags);
-	}
-
-	return 0;
+	return xfs_trans_reserve_quota_bydquots(tp, ip->i_mount, udq_delblks,
+			gdq_delblks, pdq_delblks,
+			ip->i_d.di_nblocks + ip->i_delayed_blks,
+			1, blkflags | flags);
 }
 
 int

[PATCH 02/16] xfs: clean up quota reservation callsites

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Convert a few xfs_trans_*reserve* callsites that are open-coding other
convenience functions.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c |    3 +--
 fs/xfs/xfs_bmap_util.c   |    4 ++--
 fs/xfs/xfs_reflink.c     |    4 ++--
 3 files changed, 5 insertions(+), 6 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 7ea1dbbe3d0b..c730288b5981 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4937,8 +4937,7 @@ xfs_bmap_del_extent_delay(
 	 * sb counters as we might have to borrow some blocks for the
 	 * indirect block accounting.
 	 */
-	error = xfs_trans_reserve_quota_nblks(NULL, ip,
-			-((long)del->br_blockcount), 0,
+	error = xfs_trans_unreserve_quota_nblks(NULL, ip, del->br_blockcount, 0,
 			isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
 	if (error)
 		return error;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index f3f8c48ff5bf..792809debaaa 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -884,8 +884,8 @@ xfs_unmap_extent(
 	}
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = xfs_trans_reserve_quota(tp, mp, ip->i_udquot, ip->i_gdquot,
-			ip->i_pdquot, resblks, 0, XFS_QMOPT_RES_REGBLKS);
+	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
+			XFS_QMOPT_RES_REGBLKS);
 	if (error)
 		goto out_trans_cancel;
 
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index e1c98dbf79e4..183142fd0961 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -508,8 +508,8 @@ xfs_reflink_cancel_cow_blocks(
 			xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
 
 			/* Remove the quota reservation */
-			error = xfs_trans_reserve_quota_nblks(NULL, ip,
-					-(long)del.br_blockcount, 0,
+			error = xfs_trans_unreserve_quota_nblks(NULL, ip,
+					del.br_blockcount, 0,
 					XFS_QMOPT_RES_REGBLKS);
 			if (error)
 				break;

[PATCH 03/16] xfs: create convenience wrappers for incore quota block reservations

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Create a couple of convenience wrappers for creating and deleting quota
block reservations against future changes.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c |   10 ++++------
 fs/xfs/xfs_quota.h       |   19 +++++++++++++++++++
 fs/xfs/xfs_reflink.c     |    5 ++---
 3 files changed, 25 insertions(+), 9 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index c730288b5981..94d582a9587d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4097,8 +4097,7 @@ xfs_bmapi_reserve_delalloc(
 	 * blocks.  This number gets adjusted later.  We return if we haven't
 	 * allocated blocks already inside this loop.
 	 */
-	error = xfs_trans_reserve_quota_nblks(NULL, ip, (long)alen, 0,
-						XFS_QMOPT_RES_REGBLKS);
+	error = xfs_quota_reserve_blkres(ip, alen);
 	if (error)
 		return error;
 
@@ -4144,8 +4143,7 @@ xfs_bmapi_reserve_delalloc(
 	xfs_mod_fdblocks(mp, alen, false);
 out_unreserve_quota:
 	if (XFS_IS_QUOTA_ON(mp))
-		xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0,
-						XFS_QMOPT_RES_REGBLKS);
+		xfs_quota_unreserve_blkres(ip, alen);
 	return error;
 }
 
@@ -4937,8 +4935,8 @@ xfs_bmap_del_extent_delay(
 	 * sb counters as we might have to borrow some blocks for the
 	 * indirect block accounting.
 	 */
-	error = xfs_trans_unreserve_quota_nblks(NULL, ip, del->br_blockcount, 0,
-			isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
+	ASSERT(!isrt);
+	error = xfs_quota_unreserve_blkres(ip, del->br_blockcount);
 	if (error)
 		return error;
 	ip->i_delayed_blks -= del->br_blockcount;
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 5a62398940d0..1d1a1634ea29 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -108,6 +108,12 @@ extern void xfs_qm_mount_quotas(struct xfs_mount *);
 extern void xfs_qm_unmount(struct xfs_mount *);
 extern void xfs_qm_unmount_quotas(struct xfs_mount *);
 
+static inline int
+xfs_quota_reserve_blkres(struct xfs_inode *ip, int64_t blocks)
+{
+	return xfs_trans_reserve_quota_nblks(NULL, ip, blocks, 0,
+			XFS_QMOPT_RES_REGBLKS);
+}
 #else
 static inline int
 xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
@@ -136,6 +142,13 @@ static inline int xfs_trans_reserve_quota_bydquots(struct xfs_trans *tp,
 {
 	return 0;
 }
+
+static inline int
+xfs_quota_reserve_blkres(struct xfs_inode *ip, int64_t blocks)
+{
+	return 0;
+}
+
 #define xfs_qm_vop_create_dqattach(tp, ip, u, g, p)
 #define xfs_qm_vop_rename_dqattach(it)					(0)
 #define xfs_qm_vop_chown(tp, ip, old, new)				(NULL)
@@ -157,6 +170,12 @@ static inline int xfs_trans_reserve_quota_bydquots(struct xfs_trans *tp,
 	xfs_trans_reserve_quota_bydquots(tp, mp, ud, gd, pd, nb, ni, \
 				f | XFS_QMOPT_RES_REGBLKS)
 
+static inline int
+xfs_quota_unreserve_blkres(struct xfs_inode *ip, int64_t blocks)
+{
+	return xfs_quota_reserve_blkres(ip, -blocks);
+}
+
 extern int xfs_mount_reset_sbqflags(struct xfs_mount *);
 
 #endif	/* __XFS_QUOTA_H__ */
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 183142fd0961..bea64ed5a57f 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -508,9 +508,8 @@ xfs_reflink_cancel_cow_blocks(
 			xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
 
 			/* Remove the quota reservation */
-			error = xfs_trans_unreserve_quota_nblks(NULL, ip,
-					del.br_blockcount, 0,
-					XFS_QMOPT_RES_REGBLKS);
+			error = xfs_quota_unreserve_blkres(ip,
+					del.br_blockcount);
 			if (error)
 				break;
 		} else {

[PATCH 04/16] xfs: remove xfs_trans_unreserve_quota_nblks completely

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

xfs_trans_cancel will release all the quota resources that were reserved
on behalf of the transaction, so get rid of the explicit unreserve step.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_bmap_util.c |   11 ++++-------
 fs/xfs/xfs_iomap.c     |    6 ++----
 fs/xfs/xfs_quota.h     |    2 --
 fs/xfs/xfs_reflink.c   |    5 +----
 4 files changed, 7 insertions(+), 17 deletions(-)


diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 792809debaaa..ae2d98af693c 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -820,12 +820,12 @@ xfs_alloc_file_space(
 		error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks,
 						      0, quota_flag);
 		if (error)
-			goto error1;
+			goto error;
 
 		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
 				XFS_IEXT_ADD_NOSPLIT_CNT);
 		if (error)
-			goto error0;
+			goto error;
 
 		xfs_trans_ijoin(tp, ip, 0);
 
@@ -833,7 +833,7 @@ xfs_alloc_file_space(
 					allocatesize_fsb, alloc_type, 0, imapp,
 					&nimaps);
 		if (error)
-			goto error0;
+			goto error;
 
 		/*
 		 * Complete the transaction
@@ -856,10 +856,7 @@ xfs_alloc_file_space(
 
 	return error;
 
-error0:	/* unlock inode, unreserve quota blocks, cancel trans */
-	xfs_trans_unreserve_quota_nblks(tp, ip, (long)qblocks, 0, quota_flag);
-
-error1:	/* Just cancel transaction */
+error:
 	xfs_trans_cancel(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 8f4b27cded20..ea2f71e09b41 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -253,7 +253,7 @@ xfs_iomap_write_direct(
 	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
 			XFS_IEXT_ADD_NOSPLIT_CNT);
 	if (error)
-		goto out_res_cancel;
+		goto out_trans_cancel;
 
 	xfs_trans_ijoin(tp, ip, 0);
 
@@ -265,7 +265,7 @@ xfs_iomap_write_direct(
 	error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb, bmapi_flags, 0,
 				imap, &nimaps);
 	if (error)
-		goto out_res_cancel;
+		goto out_trans_cancel;
 
 	/*
 	 * Complete the transaction
@@ -289,8 +289,6 @@ xfs_iomap_write_direct(
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
 
-out_res_cancel:
-	xfs_trans_unreserve_quota_nblks(tp, ip, (long)qblocks, 0, quota_flag);
 out_trans_cancel:
 	xfs_trans_cancel(tp);
 	goto out_unlock;
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 1d1a1634ea29..31d0de899cc4 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -164,8 +164,6 @@ xfs_quota_reserve_blkres(struct xfs_inode *ip, int64_t blocks)
 #define xfs_qm_unmount_quotas(mp)
 #endif /* CONFIG_XFS_QUOTA */
 
-#define xfs_trans_unreserve_quota_nblks(tp, ip, nblks, ninos, flags) \
-	xfs_trans_reserve_quota_nblks(tp, ip, -(nblks), -(ninos), flags)
 #define xfs_trans_reserve_quota(tp, mp, ud, gd, pd, nb, ni, f) \
 	xfs_trans_reserve_quota_bydquots(tp, mp, ud, gd, pd, nb, ni, \
 				f | XFS_QMOPT_RES_REGBLKS)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index bea64ed5a57f..15435229bc1f 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -411,7 +411,7 @@ xfs_reflink_allocate_cow(
 			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, 0, cmap,
 			&nimaps);
 	if (error)
-		goto out_unreserve;
+		goto out_trans_cancel;
 
 	xfs_inode_set_cowblocks_tag(ip);
 	error = xfs_trans_commit(tp);
@@ -436,9 +436,6 @@ xfs_reflink_allocate_cow(
 	trace_xfs_reflink_convert_cow(ip, cmap);
 	return xfs_reflink_convert_cow_locked(ip, offset_fsb, count_fsb);
 
-out_unreserve:
-	xfs_trans_unreserve_quota_nblks(tp, ip, (long)resblks, 0,
-			XFS_QMOPT_RES_REGBLKS);
 out_trans_cancel:
 	xfs_trans_cancel(tp);
 	return error;

[PATCH 05/16] xfs: clean up icreate quota reservation calls

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Create a proper helper so that inode creation calls can reserve quota
with a dedicated function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 fs/xfs/xfs_inode.c       |    6 ++----
 fs/xfs/xfs_quota.h       |   14 ++++++++++----
 fs/xfs/xfs_symlink.c     |    3 +--
 fs/xfs/xfs_trans_dquot.c |   18 ++++++++++++++++++
 4 files changed, 31 insertions(+), 10 deletions(-)


diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e2a1db4cee43..4bbd2fb628f7 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1037,8 +1037,7 @@ xfs_create(
 	/*
 	 * Reserve disk quota and the inode.
 	 */
-	error = xfs_trans_reserve_quota(tp, mp, udqp, gdqp,
-						pdqp, resblks, 1, 0);
+	error = xfs_trans_reserve_quota_icreate(tp, udqp, gdqp, pdqp, resblks);
 	if (error)
 		goto out_trans_cancel;
 
@@ -1169,8 +1168,7 @@ xfs_create_tmpfile(
 	if (error)
 		goto out_release_inode;
 
-	error = xfs_trans_reserve_quota(tp, mp, udqp, gdqp,
-						pdqp, resblks, 1, 0);
+	error = xfs_trans_reserve_quota_icreate(tp, udqp, gdqp, pdqp, resblks);
 	if (error)
 		goto out_trans_cancel;
 
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 31d0de899cc4..919c3a924821 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -86,6 +86,9 @@ extern int xfs_trans_reserve_quota_nblks(struct xfs_trans *,
 extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
 		struct xfs_mount *, struct xfs_dquot *,
 		struct xfs_dquot *, struct xfs_dquot *, int64_t, long, uint);
+int xfs_trans_reserve_quota_icreate(struct xfs_trans *tp,
+		struct xfs_dquot *udqp, struct xfs_dquot *gdqp,
+		struct xfs_dquot *pdqp, int64_t dblocks);
 
 extern int xfs_qm_vop_dqalloc(struct xfs_inode *, kuid_t, kgid_t,
 		prid_t, uint, struct xfs_dquot **, struct xfs_dquot **,
@@ -149,6 +152,13 @@ xfs_quota_reserve_blkres(struct xfs_inode *ip, int64_t blocks)
 	return 0;
 }
 
+static inline int
+xfs_trans_reserve_quota_icreate(struct xfs_trans *tp, struct xfs_dquot *udqp,
+		struct xfs_dquot *gdqp, struct xfs_dquot *pdqp, int64_t dblocks)
+{
+	return 0;
+}
+
 #define xfs_qm_vop_create_dqattach(tp, ip, u, g, p)
 #define xfs_qm_vop_rename_dqattach(it)					(0)
 #define xfs_qm_vop_chown(tp, ip, old, new)				(NULL)
@@ -164,10 +174,6 @@ xfs_quota_reserve_blkres(struct xfs_inode *ip, int64_t blocks)
 #define xfs_qm_unmount_quotas(mp)
 #endif /* CONFIG_XFS_QUOTA */
 
-#define xfs_trans_reserve_quota(tp, mp, ud, gd, pd, nb, ni, f) \
-	xfs_trans_reserve_quota_bydquots(tp, mp, ud, gd, pd, nb, ni, \
-				f | XFS_QMOPT_RES_REGBLKS)
-
 static inline int
 xfs_quota_unreserve_blkres(struct xfs_inode *ip, int64_t blocks)
 {
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 7f96649e918a..d5dee8f409b2 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -215,8 +215,7 @@ xfs_symlink(
 	/*
 	 * Reserve disk quota : blocks and inode.
 	 */
-	error = xfs_trans_reserve_quota(tp, mp, udqp, gdqp,
-						pdqp, resblks, 1, 0);
+	error = xfs_trans_reserve_quota_icreate(tp, udqp, gdqp, pdqp, resblks);
 	if (error)
 		goto out_trans_cancel;
 
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 28b8ac701919..22aa875b84f7 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -804,6 +804,24 @@ xfs_trans_reserve_quota_nblks(
 						nblks, ninos, flags);
 }
 
+/* Change the quota reservations for an inode creation activity. */
+int
+xfs_trans_reserve_quota_icreate(
+	struct xfs_trans	*tp,
+	struct xfs_dquot	*udqp,
+	struct xfs_dquot	*gdqp,
+	struct xfs_dquot	*pdqp,
+	int64_t			dblocks)
+{
+	struct xfs_mount	*mp = tp->t_mountp;
+
+	if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
+		return 0;
+
+	return xfs_trans_reserve_quota_bydquots(tp, mp, udqp, gdqp, pdqp,
+			dblocks, 1, XFS_QMOPT_RES_REGBLKS);
+}
+
 /*
  * This routine is called to allocate a quotaoff log item.
  */

[PATCH 06/16] xfs: fix up build warnings when quotas are disabled

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Fix some build warnings on gcc 10.2 when quotas are disabled.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_quota.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 919c3a924821..03235c184aab 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -130,7 +130,7 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
 }
 #define xfs_trans_dup_dqinfo(tp, tp2)
 #define xfs_trans_free_dqinfo(tp)
-#define xfs_trans_mod_dquot_byino(tp, ip, fields, delta)
+#define xfs_trans_mod_dquot_byino(tp, ip, fields, delta) do { } while (0)
 #define xfs_trans_apply_dquot_deltas(tp)
 #define xfs_trans_unreserve_and_mod_dquots(tp)
 static inline int xfs_trans_reserve_quota_nblks(struct xfs_trans *tp,
@@ -166,8 +166,8 @@ xfs_trans_reserve_quota_icreate(struct xfs_trans *tp, struct xfs_dquot *udqp,
 #define xfs_qm_dqattach(ip)						(0)
 #define xfs_qm_dqattach_locked(ip, fl)					(0)
 #define xfs_qm_dqdetach(ip)
-#define xfs_qm_dqrele(d)
-#define xfs_qm_statvfs(ip, s)
+#define xfs_qm_dqrele(d)			do { (d) = (d); } while(0)
+#define xfs_qm_statvfs(ip, s)			do { } while(0)
 #define xfs_qm_newmount(mp, a, b)					(0)
 #define xfs_qm_mount_quotas(mp)
 #define xfs_qm_unmount(mp)

[PATCH 07/16] xfs: reduce quota reservation when doing a dax unwritten extent conversion

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In commit 3b0fe47805802, we reduced the free space requirement to
perform a pre-write unwritten extent conversion on an S_DAX file.  Since
we're not actually allocating any space, the logic goes, we only need
enough reservation to handle shape changes in the bmbt.

The same logic should have been applied to quota -- we're not allocating
any space, so we only need to reserve enough quota to handle the bmbt
shape changes.

Fixes: 3b0fe4780580 ("xfs: Don't use reserved blocks for data blocks with DAX")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_iomap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index ea2f71e09b41..b046eadd3b21 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -236,7 +236,7 @@ xfs_iomap_write_direct(
 		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
 		if (imap->br_state == XFS_EXT_UNWRITTEN) {
 			tflags |= XFS_TRANS_RESERVE;
-			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
+			resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
 		}
 	}
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, resrtextents,

[PATCH 08/16] xfs: reserve data and rt quota at the same time

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Modify xfs_trans_reserve_quota_nblks so that we can reserve data and
realtime blocks from the dquot at the same time.  This change has the
theoretical side effect that for allocations to realtime files we will
reserve from the dquot both the number of rtblocks being allocated and
the number of bmbt blocks that might be needed to add the mapping.
However, since the mount code disables quota if it finds a realtime
device, this should not result in any behavior changes.

Now that we've moved the inode creation callers away from using the
_nblks function, we can repurpose the (now unused) ninos argument for
realtime blocks, so make that change.  This also replaces the flags
argument with a boolean parameter to force the reservation since we
don't need to distinguish between data and rt quota reservations any
more, and the only flag being passed in was FORCE_RES.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c |    6 +-----
 fs/xfs/libxfs/xfs_bmap.c |    4 +---
 fs/xfs/xfs_bmap_util.c   |   24 +++++++++++-------------
 fs/xfs/xfs_iomap.c       |   26 +++++++++++++-------------
 fs/xfs/xfs_quota.h       |   10 +++++-----
 fs/xfs/xfs_reflink.c     |    6 ++----
 fs/xfs/xfs_trans_dquot.c |   42 ++++++++++++++++++++++++++++--------------
 7 files changed, 61 insertions(+), 57 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index be51e7068dcd..e05dc0bc4a8f 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -474,12 +474,8 @@ xfs_attr_set(
 	}
 
 	if (args->value) {
-		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
-
-		if (rsvd)
-			quota_flags |= XFS_QMOPT_FORCE_RES;
 		error = xfs_trans_reserve_quota_nblks(args->trans, dp,
-				args->total, 0, quota_flags);
+				args->total, 0, rsvd);
 		if (error)
 			goto out_trans_cancel;
 
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 94d582a9587d..6e6734398f0d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1085,9 +1085,7 @@ xfs_bmap_add_attrfork(
 		return error;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = xfs_trans_reserve_quota_nblks(tp, ip, blks, 0, rsvd ?
-			XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-			XFS_QMOPT_RES_REGBLKS);
+	error = xfs_trans_reserve_quota_nblks(tp, ip, blks, 0, rsvd);
 	if (error)
 		goto trans_cancel;
 	if (XFS_IFORK_Q(ip))
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index ae2d98af693c..ef8f7055af77 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -727,11 +727,10 @@ xfs_alloc_file_space(
 	xfs_fileoff_t		startoffset_fsb;
 	xfs_fileoff_t		endoffset_fsb;
 	int			nimaps;
-	int			quota_flag;
 	int			rt;
 	xfs_trans_t		*tp;
 	xfs_bmbt_irec_t		imaps[1], *imapp;
-	uint			qblocks, resblks, resrtextents;
+	uint			resblks, resrtextents;
 	int			error;
 
 	trace_xfs_alloc_file_space(ip);
@@ -761,6 +760,7 @@ xfs_alloc_file_space(
 	 */
 	while (allocatesize_fsb && !error) {
 		xfs_fileoff_t	s, e;
+		unsigned int	dblocks, rblocks;
 
 		/*
 		 * Determine space reservations for data/realtime.
@@ -790,20 +790,19 @@ xfs_alloc_file_space(
 		 */
 		resblks = min_t(xfs_fileoff_t, (e - s), (MAXEXTLEN * nimaps));
 		if (unlikely(rt)) {
-			resrtextents = qblocks = resblks;
+			resrtextents = resblks;
 			resrtextents /= mp->m_sb.sb_rextsize;
-			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
-			quota_flag = XFS_QMOPT_RES_RTBLKS;
+			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
+			rblocks = resblks;
 		} else {
-			resrtextents = 0;
-			resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, resblks);
-			quota_flag = XFS_QMOPT_RES_REGBLKS;
+			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resblks);
+			rblocks = 0;
 		}
 
 		/*
 		 * Allocate and setup the transaction.
 		 */
-		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks,
+		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, dblocks,
 				resrtextents, 0, &tp);
 
 		/*
@@ -817,8 +816,8 @@ xfs_alloc_file_space(
 			break;
 		}
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks,
-						      0, quota_flag);
+		error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks,
+				false);
 		if (error)
 			goto error;
 
@@ -881,8 +880,7 @@ xfs_unmap_extent(
 	}
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
-			XFS_QMOPT_RES_REGBLKS);
+	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, false);
 	if (error)
 		goto out_trans_cancel;
 
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index b046eadd3b21..fba7303d8ed6 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -194,25 +194,25 @@ xfs_iomap_write_direct(
 	struct xfs_trans	*tp;
 	xfs_filblks_t		resaligned;
 	int			nimaps;
-	int			quota_flag;
-	uint			qblocks, resblks;
+	unsigned int		dblocks, rblocks;
 	unsigned int		resrtextents = 0;
 	int			error;
 	int			bmapi_flags = XFS_BMAPI_PREALLOC;
-	uint			tflags = 0;
+	int			tflags = 0;
+	bool			force = false;
 
 	ASSERT(count_fsb > 0);
 
 	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb,
 					   xfs_get_extsz_hint(ip));
 	if (unlikely(XFS_IS_REALTIME_INODE(ip))) {
-		resrtextents = qblocks = resaligned;
+		resrtextents = resaligned;
 		resrtextents /= mp->m_sb.sb_rextsize;
-		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
-		quota_flag = XFS_QMOPT_RES_RTBLKS;
+		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
+		rblocks = resaligned;
 	} else {
-		resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
-		quota_flag = XFS_QMOPT_RES_REGBLKS;
+		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
+		rblocks = 0;
 	}
 
 	error = xfs_qm_dqattach(ip);
@@ -235,18 +235,19 @@ xfs_iomap_write_direct(
 	if (IS_DAX(VFS_I(ip))) {
 		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
 		if (imap->br_state == XFS_EXT_UNWRITTEN) {
+			force = true;
 			tflags |= XFS_TRANS_RESERVE;
-			resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
+			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
 		}
 	}
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, resrtextents,
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, dblocks, resrtextents,
 			tflags, &tp);
 	if (error)
 		return error;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 
-	error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
+	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
 	if (error)
 		goto out_trans_cancel;
 
@@ -559,8 +560,7 @@ xfs_iomap_write_unwritten(
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		xfs_trans_ijoin(tp, ip, 0);
 
-		error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
-				XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES);
+		error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, true);
 		if (error)
 			goto error_on_bmapi_transaction;
 
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 03235c184aab..6ddc4b358ede 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -81,8 +81,8 @@ extern void xfs_trans_mod_dquot_byino(struct xfs_trans *, struct xfs_inode *,
 		uint, int64_t);
 extern void xfs_trans_apply_dquot_deltas(struct xfs_trans *);
 extern void xfs_trans_unreserve_and_mod_dquots(struct xfs_trans *);
-extern int xfs_trans_reserve_quota_nblks(struct xfs_trans *,
-		struct xfs_inode *, int64_t, long, uint);
+int xfs_trans_reserve_quota_nblks(struct xfs_trans *tp, struct xfs_inode *ip,
+		int64_t dblocks, int64_t rblocks, bool force);
 extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
 		struct xfs_mount *, struct xfs_dquot *,
 		struct xfs_dquot *, struct xfs_dquot *, int64_t, long, uint);
@@ -114,8 +114,7 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
 static inline int
 xfs_quota_reserve_blkres(struct xfs_inode *ip, int64_t blocks)
 {
-	return xfs_trans_reserve_quota_nblks(NULL, ip, blocks, 0,
-			XFS_QMOPT_RES_REGBLKS);
+	return xfs_trans_reserve_quota_nblks(NULL, ip, blocks, 0, false);
 }
 #else
 static inline int
@@ -134,7 +133,8 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
 #define xfs_trans_apply_dquot_deltas(tp)
 #define xfs_trans_unreserve_and_mod_dquots(tp)
 static inline int xfs_trans_reserve_quota_nblks(struct xfs_trans *tp,
-		struct xfs_inode *ip, int64_t nblks, long ninos, uint flags)
+		struct xfs_inode *ip, int64_t dblocks, int64_t rblocks,
+		bool force)
 {
 	return 0;
 }
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 15435229bc1f..0778b5810c26 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -398,8 +398,7 @@ xfs_reflink_allocate_cow(
 		goto convert;
 	}
 
-	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
-			XFS_QMOPT_RES_REGBLKS);
+	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, false);
 	if (error)
 		goto out_trans_cancel;
 
@@ -1090,8 +1089,7 @@ xfs_reflink_remap_extent(
 	if (!smap_real && dmap_written)
 		qres += dmap->br_blockcount;
 	if (qres > 0) {
-		error = xfs_trans_reserve_quota_nblks(tp, ip, qres, 0,
-				XFS_QMOPT_RES_REGBLKS);
+		error = xfs_trans_reserve_quota_nblks(tp, ip, qres, 0, false);
 		if (error)
 			goto out_cancel;
 	}
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 22aa875b84f7..a1a72b7900c5 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -780,28 +780,42 @@ int
 xfs_trans_reserve_quota_nblks(
 	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
-	int64_t			nblks,
-	long			ninos,
-	uint			flags)
+	int64_t			dblocks,
+	int64_t			rblocks,
+	bool			force)
 {
 	struct xfs_mount	*mp = ip->i_mount;
+	unsigned int		qflags = 0;
+	int			error;
 
 	if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
 		return 0;
 
 	ASSERT(!xfs_is_quota_inode(&mp->m_sb, ip->i_ino));
-
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
-	ASSERT((flags & ~(XFS_QMOPT_FORCE_RES)) == XFS_TRANS_DQ_RES_RTBLKS ||
-	       (flags & ~(XFS_QMOPT_FORCE_RES)) == XFS_TRANS_DQ_RES_BLKS);
-
-	/*
-	 * Reserve nblks against these dquots, with trans as the mediator.
-	 */
-	return xfs_trans_reserve_quota_bydquots(tp, mp,
-						ip->i_udquot, ip->i_gdquot,
-						ip->i_pdquot,
-						nblks, ninos, flags);
+
+	if (force)
+		qflags |= XFS_QMOPT_FORCE_RES;
+
+	/* Reserve data device quota against the inode's dquots. */
+	error = xfs_trans_reserve_quota_bydquots(tp, mp, ip->i_udquot,
+			ip->i_gdquot, ip->i_pdquot, dblocks, 0,
+			XFS_QMOPT_RES_REGBLKS | qflags);
+	if (error)
+		return error;
+
+	/* Do the same but for realtime blocks. */
+	error = xfs_trans_reserve_quota_bydquots(tp, mp, ip->i_udquot,
+			ip->i_gdquot, ip->i_pdquot, rblocks, 0,
+			XFS_QMOPT_RES_RTBLKS | qflags);
+	if (error) {
+		xfs_trans_reserve_quota_bydquots(tp, mp, ip->i_udquot,
+				ip->i_gdquot, ip->i_pdquot, -dblocks, 0,
+				XFS_QMOPT_RES_REGBLKS);
+		return error;
+	}
+
+	return 0;
 }
 
 /* Change the quota reservations for an inode creation activity. */

[PATCH 09/16] xfs: refactor common transaction/inode/quota allocation idiom

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Create a new helper xfs_trans_alloc_inode that allocates a transaction,
locks and joins an inode to it, and then reserves the appropriate amount
of quota against that transction.  Then replace all the open-coded
idioms with a single call to this helper.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c |   11 +----------
 fs/xfs/libxfs/xfs_bmap.c |   10 ++--------
 fs/xfs/xfs_bmap_util.c   |   14 +++----------
 fs/xfs/xfs_iomap.c       |   11 ++---------
 fs/xfs/xfs_trans.c       |   48 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.h       |    3 +++
 6 files changed, 59 insertions(+), 38 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index e05dc0bc4a8f..cb95bc77fe59 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -458,14 +458,10 @@ xfs_attr_set(
 	 * Root fork attributes can use reserved data blocks for this
 	 * operation if necessary
 	 */
-	error = xfs_trans_alloc(mp, &tres, total, 0,
-			rsvd ? XFS_TRANS_RESERVE : 0, &args->trans);
+	error = xfs_trans_alloc_inode(dp, &tres, total, rsvd, &args->trans);
 	if (error)
 		return error;
 
-	xfs_ilock(dp, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(args->trans, dp, 0);
-
 	if (args->value || xfs_inode_hasattr(dp)) {
 		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
 				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
@@ -474,11 +470,6 @@ xfs_attr_set(
 	}
 
 	if (args->value) {
-		error = xfs_trans_reserve_quota_nblks(args->trans, dp,
-				args->total, 0, rsvd);
-		if (error)
-			goto out_trans_cancel;
-
 		error = xfs_has_attr(args);
 		if (error == -EEXIST && (args->attr_flags & XATTR_CREATE))
 			goto out_trans_cancel;
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 6e6734398f0d..be6661645b59 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1079,19 +1079,13 @@ xfs_bmap_add_attrfork(
 
 	blks = XFS_ADDAFORK_SPACE_RES(mp);
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_addafork, blks, 0,
-			rsvd ? XFS_TRANS_RESERVE : 0, &tp);
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_addafork, blks,
+			rsvd, &tp);
 	if (error)
 		return error;
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = xfs_trans_reserve_quota_nblks(tp, ip, blks, 0, rsvd);
-	if (error)
-		goto trans_cancel;
 	if (XFS_IFORK_Q(ip))
 		goto trans_cancel;
 
-	xfs_trans_ijoin(tp, ip, 0);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	error = xfs_bmap_set_attrforkoff(ip, size, &version);
 	if (error)
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index ef8f7055af77..c5687ae437dc 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -873,18 +873,10 @@ xfs_unmap_extent(
 	uint			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 	int			error;
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
-	if (error) {
-		ASSERT(error == -ENOSPC || XFS_FORCED_SHUTDOWN(mp));
-		return error;
-	}
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, false);
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks,
+			false, &tp);
 	if (error)
-		goto out_trans_cancel;
-
-	xfs_trans_ijoin(tp, ip, 0);
+		return error;
 
 	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
 			XFS_IEXT_PUNCH_HOLE_CNT);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index fba7303d8ed6..ac91c971342d 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -552,18 +552,11 @@ xfs_iomap_write_unwritten(
 		 * here as we might be asked to write out the same inode that we
 		 * complete here and might deadlock on the iolock.
 		 */
-		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
-				XFS_TRANS_RESERVE, &tp);
+		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks,
+				true, &tp);
 		if (error)
 			return error;
 
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		xfs_trans_ijoin(tp, ip, 0);
-
-		error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, true);
-		if (error)
-			goto error_on_bmapi_transaction;
-
 		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
 				XFS_IEXT_WRITE_UNWRITTEN_CNT);
 		if (error)
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index e72730f85af1..156b9ed8534f 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -20,6 +20,7 @@
 #include "xfs_trace.h"
 #include "xfs_error.h"
 #include "xfs_defer.h"
+#include "xfs_inode.h"
 
 kmem_zone_t	*xfs_trans_zone;
 
@@ -1024,3 +1025,50 @@ xfs_trans_roll(
 	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
 	return xfs_trans_reserve(*tpp, &tres, 0, 0);
 }
+
+/*
+ * Allocate an transaction, lock and join the inode to it, and reserve quota.
+ *
+ * The caller must ensure that the on-disk dquots attached to this inode have
+ * already been allocated and initialized.  The caller is responsible for
+ * releasing ILOCK_EXCL if a new transaction is returned.
+ */
+int
+xfs_trans_alloc_inode(
+	struct xfs_inode	*ip,
+	struct xfs_trans_res	*resv,
+	unsigned int		dblocks,
+	bool			force,
+	struct xfs_trans	**tpp)
+{
+	struct xfs_trans	*tp;
+	struct xfs_mount	*mp = ip->i_mount;
+	int			error;
+
+	error = xfs_trans_alloc(mp, resv, dblocks, 0,
+			force ? XFS_TRANS_RESERVE : 0, &tp);
+	if (error)
+		return error;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
+	error = xfs_qm_dqattach_locked(ip, false);
+	if (error) {
+		/* Caller should have allocated the dquots! */
+		ASSERT(error != -ENOENT);
+		goto out_cancel;
+	}
+
+	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, 0, force);
+	if (error)
+		goto out_cancel;
+
+	*tpp = tp;
+	return 0;
+
+out_cancel:
+	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
+}
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 084658946cc8..aa50be244432 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -268,4 +268,7 @@ xfs_trans_item_relog(
 	return lip->li_ops->iop_relog(lip, tp);
 }
 
+int xfs_trans_alloc_inode(struct xfs_inode *ip, struct xfs_trans_res *resv,
+		unsigned int dblocks, bool force, struct xfs_trans **tpp);
+
 #endif	/* __XFS_TRANS_H__ */

[PATCH 10/16] xfs: allow reservation of rtblocks with xfs_trans_alloc_inode

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Make it so that we can reserve rt blocks with the xfs_trans_alloc_inode
wrapper function, then convert a few more callsites.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c |    2 +-
 fs/xfs/libxfs/xfs_bmap.c |    2 +-
 fs/xfs/xfs_bmap_util.c   |   29 +++++------------------------
 fs/xfs/xfs_iomap.c       |   22 +++++-----------------
 fs/xfs/xfs_trans.c       |    6 ++++--
 fs/xfs/xfs_trans.h       |    3 ++-
 6 files changed, 18 insertions(+), 46 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index cb95bc77fe59..472b3039eabb 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -458,7 +458,7 @@ xfs_attr_set(
 	 * Root fork attributes can use reserved data blocks for this
 	 * operation if necessary
 	 */
-	error = xfs_trans_alloc_inode(dp, &tres, total, rsvd, &args->trans);
+	error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index be6661645b59..e0905ad171f0 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1079,7 +1079,7 @@ xfs_bmap_add_attrfork(
 
 	blks = XFS_ADDAFORK_SPACE_RES(mp);
 
-	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_addafork, blks,
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_addafork, blks, 0,
 			rsvd, &tp);
 	if (error)
 		return error;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index c5687ae437dc..e7d68318e6a5 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -730,7 +730,6 @@ xfs_alloc_file_space(
 	int			rt;
 	xfs_trans_t		*tp;
 	xfs_bmbt_irec_t		imaps[1], *imapp;
-	uint			resblks, resrtextents;
 	int			error;
 
 	trace_xfs_alloc_file_space(ip);
@@ -760,7 +759,7 @@ xfs_alloc_file_space(
 	 */
 	while (allocatesize_fsb && !error) {
 		xfs_fileoff_t	s, e;
-		unsigned int	dblocks, rblocks;
+		unsigned int	dblocks, rblocks, resblks;
 
 		/*
 		 * Determine space reservations for data/realtime.
@@ -790,8 +789,6 @@ xfs_alloc_file_space(
 		 */
 		resblks = min_t(xfs_fileoff_t, (e - s), (MAXEXTLEN * nimaps));
 		if (unlikely(rt)) {
-			resrtextents = resblks;
-			resrtextents /= mp->m_sb.sb_rextsize;
 			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 			rblocks = resblks;
 		} else {
@@ -802,32 +799,16 @@ xfs_alloc_file_space(
 		/*
 		 * Allocate and setup the transaction.
 		 */
-		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, dblocks,
-				resrtextents, 0, &tp);
-
-		/*
-		 * Check for running out of space
-		 */
-		if (error) {
-			/*
-			 * Free the transaction structure.
-			 */
-			ASSERT(error == -ENOSPC || XFS_FORCED_SHUTDOWN(mp));
-			break;
-		}
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks,
-				false);
+		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
+				dblocks, rblocks, false, &tp);
 		if (error)
-			goto error;
+			break;
 
 		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
 				XFS_IEXT_ADD_NOSPLIT_CNT);
 		if (error)
 			goto error;
 
-		xfs_trans_ijoin(tp, ip, 0);
-
 		error = xfs_bmapi_write(tp, ip, startoffset_fsb,
 					allocatesize_fsb, alloc_type, 0, imapp,
 					&nimaps);
@@ -873,7 +854,7 @@ xfs_unmap_extent(
 	uint			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 	int			error;
 
-	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks,
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
 			false, &tp);
 	if (error)
 		return error;
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index ac91c971342d..fe2bbd9b6fdb 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -195,19 +195,15 @@ xfs_iomap_write_direct(
 	xfs_filblks_t		resaligned;
 	int			nimaps;
 	unsigned int		dblocks, rblocks;
-	unsigned int		resrtextents = 0;
+	bool			force = false;
 	int			error;
 	int			bmapi_flags = XFS_BMAPI_PREALLOC;
-	int			tflags = 0;
-	bool			force = false;
 
 	ASSERT(count_fsb > 0);
 
 	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb,
 					   xfs_get_extsz_hint(ip));
 	if (unlikely(XFS_IS_REALTIME_INODE(ip))) {
-		resrtextents = resaligned;
-		resrtextents /= mp->m_sb.sb_rextsize;
 		dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 		rblocks = resaligned;
 	} else {
@@ -236,28 +232,20 @@ xfs_iomap_write_direct(
 		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
 		if (imap->br_state == XFS_EXT_UNWRITTEN) {
 			force = true;
-			tflags |= XFS_TRANS_RESERVE;
 			dblocks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
 		}
 	}
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, dblocks, resrtextents,
-			tflags, &tp);
+
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks,
+			rblocks, force, &tp);
 	if (error)
 		return error;
 
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-
-	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
-	if (error)
-		goto out_trans_cancel;
-
 	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
 			XFS_IEXT_ADD_NOSPLIT_CNT);
 	if (error)
 		goto out_trans_cancel;
 
-	xfs_trans_ijoin(tp, ip, 0);
-
 	/*
 	 * From this point onwards we overwrite the imap pointer that the
 	 * caller gave to us.
@@ -553,7 +541,7 @@ xfs_iomap_write_unwritten(
 		 * complete here and might deadlock on the iolock.
 		 */
 		error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks,
-				true, &tp);
+				0, true, &tp);
 		if (error)
 			return error;
 
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 156b9ed8534f..151f274eee43 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1038,6 +1038,7 @@ xfs_trans_alloc_inode(
 	struct xfs_inode	*ip,
 	struct xfs_trans_res	*resv,
 	unsigned int		dblocks,
+	unsigned int		rblocks,
 	bool			force,
 	struct xfs_trans	**tpp)
 {
@@ -1045,7 +1046,8 @@ xfs_trans_alloc_inode(
 	struct xfs_mount	*mp = ip->i_mount;
 	int			error;
 
-	error = xfs_trans_alloc(mp, resv, dblocks, 0,
+	error = xfs_trans_alloc(mp, resv, dblocks,
+			rblocks / mp->m_sb.sb_rextsize,
 			force ? XFS_TRANS_RESERVE : 0, &tp);
 	if (error)
 		return error;
@@ -1060,7 +1062,7 @@ xfs_trans_alloc_inode(
 		goto out_cancel;
 	}
 
-	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, 0, force);
+	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
 	if (error)
 		goto out_cancel;
 
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index aa50be244432..52bbd7e6a552 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -269,6 +269,7 @@ xfs_trans_item_relog(
 }
 
 int xfs_trans_alloc_inode(struct xfs_inode *ip, struct xfs_trans_res *resv,
-		unsigned int dblocks, bool force, struct xfs_trans **tpp);
+		unsigned int dblocks, unsigned int rblocks, bool force,
+		struct xfs_trans **tpp);
 
 #endif	/* __XFS_TRANS_H__ */

[PATCH 11/16] xfs: refactor reflink functions to use xfs_trans_alloc_inode

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The two remaining callers of xfs_trans_reserve_quota_nblks are in the
reflink code.  These conversions aren't as uniform as the previous
conversions, so call that out in a separate patch.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c   |    3 ++-
 fs/xfs/xfs_reflink.c |   59 ++++++++++++++++++++------------------------------
 2 files changed, 26 insertions(+), 36 deletions(-)


diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index fe2bbd9b6fdb..70c341658c01 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -831,7 +831,8 @@ xfs_direct_write_iomap_begin(
 	return xfs_bmbt_to_iomap(ip, iomap, &cmap, IOMAP_F_SHARED);
 
 out_unlock:
-	xfs_iunlock(ip, lockmode);
+	if (lockmode)
+		xfs_iunlock(ip, lockmode);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 0778b5810c26..27f875fa7a0d 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -376,16 +376,14 @@ xfs_reflink_allocate_cow(
 	resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
 
 	xfs_iunlock(ip, *lockmode);
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
+	*lockmode = 0;
+
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
+			false, &tp);
+	if (error)
+		return error;
+
 	*lockmode = XFS_ILOCK_EXCL;
-	xfs_ilock(ip, *lockmode);
-
-	if (error)
-		return error;
-
-	error = xfs_qm_dqattach_locked(ip, false);
-	if (error)
-		goto out_trans_cancel;
 
 	/*
 	 * Check for an overlapping extent again now that we dropped the ilock.
@@ -398,12 +396,6 @@ xfs_reflink_allocate_cow(
 		goto convert;
 	}
 
-	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, false);
-	if (error)
-		goto out_trans_cancel;
-
-	xfs_trans_ijoin(tp, ip, 0);
-
 	/* Allocate the entire reservation as unwritten blocks. */
 	nimaps = 1;
 	error = xfs_bmapi_write(tp, ip, imap->br_startoff, imap->br_blockcount,
@@ -997,7 +989,7 @@ xfs_reflink_remap_extent(
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
 	xfs_off_t		newlen;
-	int64_t			qres, qdelta;
+	int64_t			qdelta = 0;
 	unsigned int		resblks;
 	bool			smap_real;
 	bool			dmap_written = xfs_bmap_is_written_extent(dmap);
@@ -1005,15 +997,22 @@ xfs_reflink_remap_extent(
 	int			nimaps;
 	int			error;
 
-	/* Start a rolling transaction to switch the mappings */
+	/*
+	 * Start a rolling transaction to switch the mappings.
+	 *
+	 * Adding a written extent to the extent map can cause a bmbt split,
+	 * and removing a mapped extent from the extent can cause a bmbt split.
+	 * The two operations cannot both cause a split since they operate on
+	 * the same index in the bmap btree, so we only need a reservation for
+	 * one bmbt split if either thing is happening.  However, we haven't
+	 * locked the inode yet, so we reserve assuming this is the case.
+	 */
 	resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
+			false, &tp);
 	if (error)
 		goto out;
 
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, 0);
-
 	/*
 	 * Read what's currently mapped in the destination file into smap.
 	 * If smap isn't a hole, we will have to remove it before we can add
@@ -1061,15 +1060,9 @@ xfs_reflink_remap_extent(
 	}
 
 	/*
-	 * Compute quota reservation if we think the quota block counter for
+	 * Increase quota reservation if we think the quota block counter for
 	 * this file could increase.
 	 *
-	 * Adding a written extent to the extent map can cause a bmbt split,
-	 * and removing a mapped extent from the extent can cause a bmbt split.
-	 * The two operations cannot both cause a split since they operate on
-	 * the same index in the bmap btree, so we only need a reservation for
-	 * one bmbt split if either thing is happening.
-	 *
 	 * If we are mapping a written extent into the file, we need to have
 	 * enough quota block count reservation to handle the blocks in that
 	 * extent.  We log only the delta to the quota block counts, so if the
@@ -1083,13 +1076,9 @@ xfs_reflink_remap_extent(
 	 * before we started.  That should have removed all the delalloc
 	 * reservations, but we code defensively.
 	 */
-	qres = qdelta = 0;
-	if (smap_real || dmap_written)
-		qres = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
-	if (!smap_real && dmap_written)
-		qres += dmap->br_blockcount;
-	if (qres > 0) {
-		error = xfs_trans_reserve_quota_nblks(tp, ip, qres, 0, false);
+	if (!smap_real && dmap_written) {
+		error = xfs_trans_reserve_quota_nblks(tp, ip,
+				dmap->br_blockcount, 0, false);
 		if (error)
 			goto out_cancel;
 	}

[PATCH 12/16] xfs: refactor inode creation transaction/inode/quota allocation idiom

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

For file creation, create a new helper xfs_trans_alloc_icreate that
allocates a transaction and reserves the appropriate amount of quota
against that transction.  Replace all the open-coded idioms with a
single call to this helper so that we can contain the retry loops in the
next patchset.

This changes the locking behavior for non-tempfile creation slightly, in
that we now make the quota reservation without holding the directory
ILOCK.  While the dquots chosen for inode creation are based on the
directory state at a given point in time, the directory ILOCK was
released as soon as the dquot references are picked up.  Hence it was
never necessary to hold the directory ILOCK for the quota reservation.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode.c   |   28 ++++++++++------------------
 fs/xfs/xfs_symlink.c |   14 ++++----------
 fs/xfs/xfs_trans.c   |   33 +++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.h   |    6 ++++++
 4 files changed, 53 insertions(+), 28 deletions(-)


diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 4bbd2fb628f7..636ac13b1df2 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1022,25 +1022,20 @@ xfs_create(
 	 * the case we'll drop the one we have and get a more
 	 * appropriate transaction later.
 	 */
-	error = xfs_trans_alloc(mp, tres, resblks, 0, 0, &tp);
+	error = xfs_trans_alloc_icreate(mp, tres, udqp, gdqp, pdqp, resblks,
+			&tp);
 	if (error == -ENOSPC) {
 		/* flush outstanding delalloc blocks and retry */
 		xfs_flush_inodes(mp);
-		error = xfs_trans_alloc(mp, tres, resblks, 0, 0, &tp);
+		error = xfs_trans_alloc_icreate(mp, tres, udqp, gdqp, pdqp,
+				resblks, &tp);
 	}
 	if (error)
-		goto out_release_inode;
+		goto out_release_dquots;
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT);
 	unlock_dp_on_error = true;
 
-	/*
-	 * Reserve disk quota and the inode.
-	 */
-	error = xfs_trans_reserve_quota_icreate(tp, udqp, gdqp, pdqp, resblks);
-	if (error)
-		goto out_trans_cancel;
-
 	error = xfs_iext_count_may_overflow(dp, XFS_DATA_FORK,
 			XFS_IEXT_DIR_MANIP_CNT(mp));
 	if (error)
@@ -1120,7 +1115,7 @@ xfs_create(
 		xfs_finish_inode_setup(ip);
 		xfs_irele(ip);
 	}
-
+ out_release_dquots:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);
 	xfs_qm_dqrele(pdqp);
@@ -1164,13 +1159,10 @@ xfs_create_tmpfile(
 	resblks = XFS_IALLOC_SPACE_RES(mp);
 	tres = &M_RES(mp)->tr_create_tmpfile;
 
-	error = xfs_trans_alloc(mp, tres, resblks, 0, 0, &tp);
+	error = xfs_trans_alloc_icreate(mp, tres, udqp, gdqp, pdqp, resblks,
+			&tp);
 	if (error)
-		goto out_release_inode;
-
-	error = xfs_trans_reserve_quota_icreate(tp, udqp, gdqp, pdqp, resblks);
-	if (error)
-		goto out_trans_cancel;
+		goto out_release_dquots;
 
 	error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, &ip);
 	if (error)
@@ -1213,7 +1205,7 @@ xfs_create_tmpfile(
 		xfs_finish_inode_setup(ip);
 		xfs_irele(ip);
 	}
-
+ out_release_dquots:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);
 	xfs_qm_dqrele(pdqp);
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index d5dee8f409b2..8565663b16cd 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -197,9 +197,10 @@ xfs_symlink(
 		fs_blocks = xfs_symlink_blocks(mp, pathlen);
 	resblks = XFS_SYMLINK_SPACE_RES(mp, link_name->len, fs_blocks);
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_symlink, resblks, 0, 0, &tp);
+	error = xfs_trans_alloc_icreate(mp, &M_RES(mp)->tr_symlink, udqp, gdqp,
+			pdqp, resblks, &tp);
 	if (error)
-		goto out_release_inode;
+		goto out_release_dquots;
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT);
 	unlock_dp_on_error = true;
@@ -212,13 +213,6 @@ xfs_symlink(
 		goto out_trans_cancel;
 	}
 
-	/*
-	 * Reserve disk quota : blocks and inode.
-	 */
-	error = xfs_trans_reserve_quota_icreate(tp, udqp, gdqp, pdqp, resblks);
-	if (error)
-		goto out_trans_cancel;
-
 	error = xfs_iext_count_may_overflow(dp, XFS_DATA_FORK,
 			XFS_IEXT_DIR_MANIP_CNT(mp));
 	if (error)
@@ -347,7 +341,7 @@ xfs_symlink(
 		xfs_finish_inode_setup(ip);
 		xfs_irele(ip);
 	}
-
+out_release_dquots:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);
 	xfs_qm_dqrele(pdqp);
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 151f274eee43..6c68635cc6ac 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -21,6 +21,8 @@
 #include "xfs_error.h"
 #include "xfs_defer.h"
 #include "xfs_inode.h"
+#include "xfs_dquot_item.h"
+#include "xfs_dquot.h"
 
 kmem_zone_t	*xfs_trans_zone;
 
@@ -1074,3 +1076,34 @@ xfs_trans_alloc_inode(
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
 }
+
+/*
+ * Allocate an transaction in preparation for inode creation by reserving quota
+ * against the given dquots.  Callers are not required to hold any inode locks.
+ */
+int
+xfs_trans_alloc_icreate(
+	struct xfs_mount	*mp,
+	struct xfs_trans_res	*resv,
+	struct xfs_dquot	*udqp,
+	struct xfs_dquot	*gdqp,
+	struct xfs_dquot	*pdqp,
+	unsigned int		dblocks,
+	struct xfs_trans	**tpp)
+{
+	struct xfs_trans	*tp;
+	int			error;
+
+	error = xfs_trans_alloc(mp, resv, dblocks, 0, 0, &tp);
+	if (error)
+		return error;
+
+	error = xfs_trans_reserve_quota_icreate(tp, udqp, gdqp, pdqp, dblocks);
+	if (error) {
+		xfs_trans_cancel(tp);
+		return error;
+	}
+
+	*tpp = tp;
+	return 0;
+}
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 52bbd7e6a552..04c132c55e9b 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -268,8 +268,14 @@ xfs_trans_item_relog(
 	return lip->li_ops->iop_relog(lip, tp);
 }
 
+struct xfs_dquot;
+
 int xfs_trans_alloc_inode(struct xfs_inode *ip, struct xfs_trans_res *resv,
 		unsigned int dblocks, unsigned int rblocks, bool force,
 		struct xfs_trans **tpp);
+int xfs_trans_alloc_icreate(struct xfs_mount *mp, struct xfs_trans_res *resv,
+		struct xfs_dquot *udqp, struct xfs_dquot *gdqp,
+		struct xfs_dquot *pdqp, unsigned int dblocks,
+		struct xfs_trans **tpp);
 
 #endif	/* __XFS_TRANS_H__ */

[PATCH 13/16] xfs: refactor inode ownership change transaction/inode/quota allocation idiom

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

For file ownership (uid, gid, prid) changes, create a new helper
xfs_trans_alloc_ichange that allocates a transaction and reserves the
appropriate amount of quota against that transction in preparation for a
change of user, group, or project id.  Replace all the open-coded idioms
with a single call to this helper so that we can contain the retry loops
in the next patchset.

This changes the locking behavior for ichange transactions slightly.
Since tr_ichange does not have a permanent reservation and cannot roll,
we pass XFS_ILOCK_EXCL to ijoin so that the inode will be unlocked
automatically at commit time.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_ioctl.c |   29 ++++++++----------------
 fs/xfs/xfs_iops.c  |   26 ++--------------------
 fs/xfs/xfs_trans.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.h |    3 +++
 4 files changed, 77 insertions(+), 43 deletions(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 3fbd98f61ea5..78ee201eb7cb 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1275,24 +1275,23 @@ xfs_ioctl_setattr_prepare_dax(
  */
 static struct xfs_trans *
 xfs_ioctl_setattr_get_trans(
-	struct xfs_inode	*ip)
+	struct xfs_inode	*ip,
+	struct xfs_dquot	*pdqp)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
 	int			error = -EROFS;
 
 	if (mp->m_flags & XFS_MOUNT_RDONLY)
-		goto out_unlock;
+		goto out_error;
 	error = -EIO;
 	if (XFS_FORCED_SHUTDOWN(mp))
-		goto out_unlock;
+		goto out_error;
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
+	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
+			capable(CAP_FOWNER), &tp);
 	if (error)
-		goto out_unlock;
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+		goto out_error;
 
 	/*
 	 * CAP_FOWNER overrides the following restrictions:
@@ -1312,7 +1311,7 @@ xfs_ioctl_setattr_get_trans(
 
 out_cancel:
 	xfs_trans_cancel(tp);
-out_unlock:
+out_error:
 	return ERR_PTR(error);
 }
 
@@ -1462,20 +1461,12 @@ xfs_ioctl_setattr(
 
 	xfs_ioctl_setattr_prepare_dax(ip, fa);
 
-	tp = xfs_ioctl_setattr_get_trans(ip);
+	tp = xfs_ioctl_setattr_get_trans(ip, pdqp);
 	if (IS_ERR(tp)) {
 		code = PTR_ERR(tp);
 		goto error_free_dquots;
 	}
 
-	if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) &&
-	    ip->i_d.di_projid != fa->fsx_projid) {
-		code = xfs_qm_vop_chown_reserve(tp, ip, NULL, NULL, pdqp,
-				capable(CAP_FOWNER) ?  XFS_QMOPT_FORCE_RES : 0);
-		if (code)	/* out of quota */
-			goto error_trans_cancel;
-	}
-
 	xfs_fill_fsxattr(ip, false, &old_fa);
 	code = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
 	if (code)
@@ -1608,7 +1599,7 @@ xfs_ioc_setxflags(
 
 	xfs_ioctl_setattr_prepare_dax(ip, &fa);
 
-	tp = xfs_ioctl_setattr_get_trans(ip);
+	tp = xfs_ioctl_setattr_get_trans(ip, NULL);
 	if (IS_ERR(tp)) {
 		error = PTR_ERR(tp);
 		goto out_drop_write;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index f1e21b6cfa48..00369502fe25 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -700,13 +700,11 @@ xfs_setattr_nonsize(
 			return error;
 	}
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
+	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
+			capable(CAP_FOWNER), &tp);
 	if (error)
 		goto out_dqrele;
 
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, 0);
-
 	/*
 	 * Change file ownership.  Must be the owner or privileged.
 	 */
@@ -722,21 +720,6 @@ xfs_setattr_nonsize(
 		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 
-		/*
-		 * Do a quota reservation only if uid/gid is actually
-		 * going to change.
-		 */
-		if (XFS_IS_QUOTA_RUNNING(mp) &&
-		    ((XFS_IS_UQUOTA_ON(mp) && !uid_eq(iuid, uid)) ||
-		     (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
-			ASSERT(tp);
-			error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
-						NULL, capable(CAP_FOWNER) ?
-						XFS_QMOPT_FORCE_RES : 0);
-			if (error)	/* out of quota */
-				goto out_cancel;
-		}
-
 		/*
 		 * CAP_FSETID overrides the following restrictions:
 		 *
@@ -786,8 +769,6 @@ xfs_setattr_nonsize(
 		xfs_trans_set_sync(tp);
 	error = xfs_trans_commit(tp);
 
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-
 	/*
 	 * Release any dquot(s) the inode had kept before chown.
 	 */
@@ -814,9 +795,6 @@ xfs_setattr_nonsize(
 
 	return 0;
 
-out_cancel:
-	xfs_trans_cancel(tp);
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out_dqrele:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 6c68635cc6ac..60672b5545c9 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1107,3 +1107,65 @@ xfs_trans_alloc_icreate(
 	*tpp = tp;
 	return 0;
 }
+
+/*
+ * Allocate an transaction, lock and join the inode to it, and reserve quota
+ * in preparation for inode attribute changes that include uid, gid, or prid
+ * changes.
+ *
+ * The caller must ensure that the on-disk dquots attached to this inode have
+ * already been allocated and initialized.  The ILOCK will be dropped when the
+ * transaction is committed or cancelled.
+ */
+int
+xfs_trans_alloc_ichange(
+	struct xfs_inode	*ip,
+	struct xfs_dquot	*udqp,
+	struct xfs_dquot	*gdqp,
+	struct xfs_dquot	*pdqp,
+	bool			force,
+	struct xfs_trans	**tpp)
+{
+	struct xfs_trans	*tp;
+	struct xfs_mount	*mp = ip->i_mount;
+	int			error;
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
+	if (error)
+		return error;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+
+	error = xfs_qm_dqattach_locked(ip, false);
+	if (error) {
+		/* Caller should have allocated the dquots! */
+		ASSERT(error != -ENOENT);
+		goto out_cancel;
+	}
+
+	/*
+	 * For each quota type, skip quota reservations if the inode's dquots
+	 * now match the ones that came from the caller, or the caller didn't
+	 * pass one in.
+	 */
+	if (udqp == ip->i_udquot)
+		udqp = NULL;
+	if (gdqp == ip->i_gdquot)
+		gdqp = NULL;
+	if (pdqp == ip->i_pdquot)
+		pdqp = NULL;
+	if (udqp || gdqp || pdqp) {
+		error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp, pdqp,
+				force ? XFS_QMOPT_FORCE_RES : 0);
+		if (error)
+			goto out_cancel;
+	}
+
+	*tpp = tp;
+	return 0;
+
+out_cancel:
+	xfs_trans_cancel(tp);
+	return error;
+}
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 04c132c55e9b..8b03fbfe9a1b 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -277,5 +277,8 @@ int xfs_trans_alloc_icreate(struct xfs_mount *mp, struct xfs_trans_res *resv,
 		struct xfs_dquot *udqp, struct xfs_dquot *gdqp,
 		struct xfs_dquot *pdqp, unsigned int dblocks,
 		struct xfs_trans **tpp);
+int xfs_trans_alloc_ichange(struct xfs_inode *ip, struct xfs_dquot *udqp,
+		struct xfs_dquot *gdqp, struct xfs_dquot *pdqp, bool force,
+		struct xfs_trans **tpp);
 
 #endif	/* __XFS_TRANS_H__ */

[PATCH 14/16] xfs: remove xfs_qm_vop_chown_reserve

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Now that the only caller of this function is xfs_trans_alloc_ichange,
just open-code the meat of _chown_reserve in that caller.  Drop the
(redundant) [ugp]id checks because xfs has a 1:1 relationship between
quota ids and incore dquots.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_qm.c    |   48 ------------------------------------------------
 fs/xfs/xfs_quota.h |    4 ----
 fs/xfs/xfs_trans.c |   16 ++++++++++++++--
 3 files changed, 14 insertions(+), 54 deletions(-)


diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 322d337b5dca..275cf5d7a178 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1816,54 +1816,6 @@ xfs_qm_vop_chown(
 	return prevdq;
 }
 
-/*
- * Quota reservations for setattr(AT_UID|AT_GID|AT_PROJID).
- */
-int
-xfs_qm_vop_chown_reserve(
-	struct xfs_trans	*tp,
-	struct xfs_inode	*ip,
-	struct xfs_dquot	*udqp,
-	struct xfs_dquot	*gdqp,
-	struct xfs_dquot	*pdqp,
-	uint			flags)
-{
-	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;
-
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
-	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
-
-	blkflags = XFS_IS_REALTIME_INODE(ip) ?
-			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
-
-	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    i_uid_read(VFS_I(ip)) != udqp->q_id)
-		udq_delblks = udqp;
-
-	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
-	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
-		gdq_delblks = gdqp;
-
-	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
-	    ip->i_d.di_projid != pdqp->q_id)
-		pdq_delblks = pdqp;
-
-	/*
-	 * Reserve enough quota to handle blocks on disk and reserved for a
-	 * delayed allocation.  We'll actually transfer the delalloc
-	 * 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,
-			ip->i_d.di_nblocks + ip->i_delayed_blks,
-			1, blkflags | flags);
-}
-
 int
 xfs_qm_vop_rename_dqattach(
 	struct xfs_inode	**i_tab)
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 6ddc4b358ede..d00d01302545 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -98,9 +98,6 @@ extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
 extern int xfs_qm_vop_rename_dqattach(struct xfs_inode **);
 extern struct xfs_dquot *xfs_qm_vop_chown(struct xfs_trans *,
 		struct xfs_inode *, struct xfs_dquot **, struct xfs_dquot *);
-extern int xfs_qm_vop_chown_reserve(struct xfs_trans *, struct xfs_inode *,
-		struct xfs_dquot *, struct xfs_dquot *,
-		struct xfs_dquot *, uint);
 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 *);
@@ -162,7 +159,6 @@ xfs_trans_reserve_quota_icreate(struct xfs_trans *tp, struct xfs_dquot *udqp,
 #define xfs_qm_vop_create_dqattach(tp, ip, u, g, p)
 #define xfs_qm_vop_rename_dqattach(it)					(0)
 #define xfs_qm_vop_chown(tp, ip, old, new)				(NULL)
-#define xfs_qm_vop_chown_reserve(tp, ip, u, g, p, fl)			(0)
 #define xfs_qm_dqattach(ip)						(0)
 #define xfs_qm_dqattach_locked(ip, fl)					(0)
 #define xfs_qm_dqdetach(ip)
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 60672b5545c9..29dca1bc4c1a 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1156,8 +1156,20 @@ xfs_trans_alloc_ichange(
 	if (pdqp == ip->i_pdquot)
 		pdqp = NULL;
 	if (udqp || gdqp || pdqp) {
-		error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp, pdqp,
-				force ? XFS_QMOPT_FORCE_RES : 0);
+		unsigned int	qflags = XFS_QMOPT_RES_REGBLKS;
+
+		if (force)
+			qflags |= XFS_QMOPT_FORCE_RES;
+
+		/*
+		 * Reserve enough quota to handle blocks on disk and reserved
+		 * for a delayed allocation.  We'll actually transfer the
+		 * delalloc reservation between dquots at chown time, even
+		 * though that part is only semi-transactional.
+		 */
+		error = xfs_trans_reserve_quota_bydquots(tp, mp, udqp, gdqp,
+				pdqp, ip->i_d.di_nblocks + ip->i_delayed_blks,
+				1, qflags);
 		if (error)
 			goto out_cancel;
 	}

[PATCH 15/16] xfs: rename code to error in xfs_ioctl_setattr

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Rename the 'code' variable to 'error' to follow the naming convention of
most other functions in xfs.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c |   38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 78ee201eb7cb..38ee66d999d8 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1435,13 +1435,13 @@ xfs_ioctl_setattr(
 	struct xfs_trans	*tp;
 	struct xfs_dquot	*pdqp = NULL;
 	struct xfs_dquot	*olddquot = NULL;
-	int			code;
+	int			error;
 
 	trace_xfs_ioctl_setattr(ip);
 
-	code = xfs_ioctl_setattr_check_projid(ip, fa);
-	if (code)
-		return code;
+	error = xfs_ioctl_setattr_check_projid(ip, fa);
+	if (error)
+		return error;
 
 	/*
 	 * If disk quotas is on, we make sure that the dquots do exist on disk,
@@ -1452,36 +1452,36 @@ xfs_ioctl_setattr(
 	 * because the i_*dquot fields will get updated anyway.
 	 */
 	if (XFS_IS_QUOTA_ON(mp)) {
-		code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
+		error = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
 				VFS_I(ip)->i_gid, fa->fsx_projid,
 				XFS_QMOPT_PQUOTA, NULL, NULL, &pdqp);
-		if (code)
-			return code;
+		if (error)
+			return error;
 	}
 
 	xfs_ioctl_setattr_prepare_dax(ip, fa);
 
 	tp = xfs_ioctl_setattr_get_trans(ip, pdqp);
 	if (IS_ERR(tp)) {
-		code = PTR_ERR(tp);
+		error = PTR_ERR(tp);
 		goto error_free_dquots;
 	}
 
 	xfs_fill_fsxattr(ip, false, &old_fa);
-	code = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
-	if (code)
+	error = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
+	if (error)
 		goto error_trans_cancel;
 
-	code = xfs_ioctl_setattr_check_extsize(ip, fa);
-	if (code)
+	error = xfs_ioctl_setattr_check_extsize(ip, fa);
+	if (error)
 		goto error_trans_cancel;
 
-	code = xfs_ioctl_setattr_check_cowextsize(ip, fa);
-	if (code)
+	error = xfs_ioctl_setattr_check_cowextsize(ip, fa);
+	if (error)
 		goto error_trans_cancel;
 
-	code = xfs_ioctl_setattr_xflags(tp, ip, fa);
-	if (code)
+	error = xfs_ioctl_setattr_xflags(tp, ip, fa);
+	if (error)
 		goto error_trans_cancel;
 
 	/*
@@ -1521,7 +1521,7 @@ xfs_ioctl_setattr(
 	else
 		ip->i_d.di_cowextsize = 0;
 
-	code = xfs_trans_commit(tp);
+	error = xfs_trans_commit(tp);
 
 	/*
 	 * Release any dquot(s) the inode had kept before chown.
@@ -1529,13 +1529,13 @@ xfs_ioctl_setattr(
 	xfs_qm_dqrele(olddquot);
 	xfs_qm_dqrele(pdqp);
 
-	return code;
+	return error;
 
 error_trans_cancel:
 	xfs_trans_cancel(tp);
 error_free_dquots:
 	xfs_qm_dqrele(pdqp);
-	return code;
+	return error;
 }
 
 STATIC int

[PATCH 16/16] xfs: shut down the filesystem if we screw up quota errors

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If we ever screw up the quota reservations enough to trip the
assertions, something's wrong with the quota code.  Shut down the
filesystem when this happens, because this is corruption.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_trans_dquot.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index a1a72b7900c5..48e09ea30ee5 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -16,6 +16,7 @@
 #include "xfs_quota.h"
 #include "xfs_qm.h"
 #include "xfs_trace.h"
+#include "xfs_error.h"
 
 STATIC void	xfs_trans_alloc_dqinfo(xfs_trans_t *);
 
@@ -691,9 +692,11 @@ xfs_trans_dqresv(
 				    nblks);
 		xfs_trans_mod_dquot(tp, dqp, XFS_TRANS_DQ_RES_INOS, ninos);
 	}
-	ASSERT(dqp->q_blk.reserved >= dqp->q_blk.count);
-	ASSERT(dqp->q_rtb.reserved >= dqp->q_rtb.count);
-	ASSERT(dqp->q_ino.reserved >= dqp->q_ino.count);
+
+	if (XFS_IS_CORRUPT(mp, dqp->q_blk.reserved < dqp->q_blk.count) ||
+	    XFS_IS_CORRUPT(mp, dqp->q_rtb.reserved < dqp->q_rtb.count) ||
+	    XFS_IS_CORRUPT(mp, dqp->q_ino.reserved < dqp->q_ino.count))
+		goto error_corrupt;
 
 	xfs_dqunlock(dqp);
 	return 0;
@@ -703,6 +706,10 @@ xfs_trans_dqresv(
 	if (xfs_dquot_type(dqp) == XFS_DQTYPE_PROJ)
 		return -ENOSPC;
 	return -EDQUOT;
+error_corrupt:
+	xfs_dqunlock(dqp);
+	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+	return -EFSCORRUPTED;
 }
 
 

Re: [PATCH 13/16] xfs: refactor inode ownership change transaction/inode/quota allocation idiom

[Christoph Hellwig]

On Mon, Feb 01, 2021 at 06:04:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> For file ownership (uid, gid, prid) changes, create a new helper
> xfs_trans_alloc_ichange that allocates a transaction and reserves the
> appropriate amount of quota against that transction in preparation for a
> change of user, group, or project id.  Replace all the open-coded idioms
> with a single call to this helper so that we can contain the retry loops
> in the next patchset.
> 
> This changes the locking behavior for ichange transactions slightly.
> Since tr_ichange does not have a permanent reservation and cannot roll,
> we pass XFS_ILOCK_EXCL to ijoin so that the inode will be unlocked
> automatically at commit time.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

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

Re: [PATCH 14/16] xfs: remove xfs_qm_vop_chown_reserve

[Christoph Hellwig]

On Mon, Feb 01, 2021 at 06:04:37PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that the only caller of this function is xfs_trans_alloc_ichange,
> just open-code the meat of _chown_reserve in that caller.  Drop the
> (redundant) [ugp]id checks because xfs has a 1:1 relationship between
> quota ids and incore dquots.

Awesome, this is so much better than what we had before..

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

Re: [PATCH 01/16] xfs: fix chown leaking delalloc quota blocks when fssetxattr fails

[Brian Foster]

On Mon, Feb 01, 2021 at 06:03:23PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> While refactoring the quota code to create a function to allocate inode
> change transactions, I noticed that xfs_qm_vop_chown_reserve does more
> than just make reservations: it also *modifies* the incore counts
> directly to handle the owner id change for the delalloc blocks.
> 
> I then observed that the fssetxattr code continues validating input
> arguments after making the quota reservation but before dirtying the
> transaction.  If the routine decides to error out, it fails to undo the
> accounting switch!  This leads to incorrect quota reservation and
> failure down the line.
> 
> We can fix this by making the reservation function do only that -- for
> the new dquot, it reserves ondisk and delalloc blocks to the
> transaction, and the old dquot hangs on to its incore reservation for
> now.  Once we actually switch the dquots, we can then update the incore
> reservations because we've dirtied the transaction and it's too late to
> turn back now.
> 
> No fixes tag because this has been broken since the start of git.
> 

Do we have any test coverage for this problem?

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_qm.c |   91 +++++++++++++++++++++----------------------------------
>  1 file changed, 34 insertions(+), 57 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index c134eb4aeaa8..322d337b5dca 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1785,6 +1785,28 @@ xfs_qm_vop_chown(
>  	xfs_trans_mod_dquot(tp, newdq, bfield, ip->i_d.di_nblocks);
>  	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_ICOUNT, 1);
>  
> +	/*
> +	 * Back when we made quota reservations for the chown, we reserved the
> +	 * ondisk blocks + delalloc blocks with the new dquot.  Now that we've
> +	 * switched the dquots, decrease the new dquot's block reservation
> +	 * (having already bumped up the real counter) so that we don't have
> +	 * any reservation to give back when we commit.
> +	 */
> +	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_RES_BLKS,
> +			-ip->i_delayed_blks);
> +

Ok, so the reservation code code below continues to reserve di_blocks
and delalloc blocks as it did before, but associates it with the
transaction and no longer reduces delalloc reservation from the old
dquots. Instead, this function increases the consumption of reserved
blocks for di_blocks and then removes the additional quota reservation
from the transaction, but not the new dquots. Thus when the transaction
commits, this has the side effect of leaving additional reservation on
the new dquots that correspond to the delalloc blocks on the inode. Am I
following that correctly?

> +	/*
> +	 * Give the incore reservation for delalloc blocks back to the old
> +	 * dquot.  We don't normally handle delalloc quota reservations
> +	 * transactionally, so just lock the dquot and subtract from the
> +	 * reservation.  We've dirtied the transaction, so it's too late to
> +	 * turn back now.
> +	 */
> +	tp->t_flags |= XFS_TRANS_DIRTY;
> +	xfs_dqlock(prevdq);
> +	prevdq->q_blk.reserved -= ip->i_delayed_blks;
> +	xfs_dqunlock(prevdq);
> +

What's the reason for not using xfs_trans_reserve_quota_bydquots(NULL,
...) here like the original code?

Brian

>  	/*
>  	 * Take an extra reference, because the inode is going to keep
>  	 * this dquot pointer even after the trans_commit.
> @@ -1807,84 +1829,39 @@ xfs_qm_vop_chown_reserve(
>  	uint			flags)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> -	uint64_t		delblks;
>  	unsigned int		blkflags;
> -	struct xfs_dquot	*udq_unres = NULL;
> -	struct xfs_dquot	*gdq_unres = NULL;
> -	struct xfs_dquot	*pdq_unres = NULL;
>  	struct xfs_dquot	*udq_delblks = NULL;
>  	struct xfs_dquot	*gdq_delblks = NULL;
>  	struct xfs_dquot	*pdq_delblks = NULL;
> -	int			error;
> -
>  
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
>  	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
>  
> -	delblks = ip->i_delayed_blks;
>  	blkflags = XFS_IS_REALTIME_INODE(ip) ?
>  			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
>  
>  	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> -	    i_uid_read(VFS_I(ip)) != udqp->q_id) {
> +	    i_uid_read(VFS_I(ip)) != udqp->q_id)
>  		udq_delblks = udqp;
> -		/*
> -		 * If there are delayed allocation blocks, then we have to
> -		 * unreserve those from the old dquot, and add them to the
> -		 * new dquot.
> -		 */
> -		if (delblks) {
> -			ASSERT(ip->i_udquot);
> -			udq_unres = ip->i_udquot;
> -		}
> -	}
> +
>  	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> -	    i_gid_read(VFS_I(ip)) != gdqp->q_id) {
> +	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
>  		gdq_delblks = gdqp;
> -		if (delblks) {
> -			ASSERT(ip->i_gdquot);
> -			gdq_unres = ip->i_gdquot;
> -		}
> -	}
>  
>  	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
> -	    ip->i_d.di_projid != pdqp->q_id) {
> +	    ip->i_d.di_projid != pdqp->q_id)
>  		pdq_delblks = pdqp;
> -		if (delblks) {
> -			ASSERT(ip->i_pdquot);
> -			pdq_unres = ip->i_pdquot;
> -		}
> -	}
> -
> -	error = xfs_trans_reserve_quota_bydquots(tp, ip->i_mount,
> -				udq_delblks, gdq_delblks, pdq_delblks,
> -				ip->i_d.di_nblocks, 1, flags | blkflags);
> -	if (error)
> -		return error;
>  
>  	/*
> -	 * Do the delayed blks reservations/unreservations now. Since, these
> -	 * are done without the help of a transaction, if a reservation fails
> -	 * its previous reservations won't be automatically undone by trans
> -	 * code. So, we have to do it manually here.
> +	 * Reserve enough quota to handle blocks on disk and reserved for a
> +	 * delayed allocation.  We'll actually transfer the delalloc
> +	 * reservation between dquots at chown time, even though that part is
> +	 * only semi-transactional.
>  	 */
> -	if (delblks) {
> -		/*
> -		 * Do the reservations first. Unreservation can't fail.
> -		 */
> -		ASSERT(udq_delblks || gdq_delblks || pdq_delblks);
> -		ASSERT(udq_unres || gdq_unres || pdq_unres);
> -		error = xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> -			    udq_delblks, gdq_delblks, pdq_delblks,
> -			    (xfs_qcnt_t)delblks, 0, flags | blkflags);
> -		if (error)
> -			return error;
> -		xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> -				udq_unres, gdq_unres, pdq_unres,
> -				-((xfs_qcnt_t)delblks), 0, blkflags);
> -	}
> -
> -	return 0;
> +	return xfs_trans_reserve_quota_bydquots(tp, ip->i_mount, udq_delblks,
> +			gdq_delblks, pdq_delblks,
> +			ip->i_d.di_nblocks + ip->i_delayed_blks,
> +			1, blkflags | flags);
>  }
>  
>  int
> 

Re: [PATCH 13/16] xfs: refactor inode ownership change transaction/inode/quota allocation idiom

[Brian Foster]

On Mon, Feb 01, 2021 at 06:04:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> For file ownership (uid, gid, prid) changes, create a new helper
> xfs_trans_alloc_ichange that allocates a transaction and reserves the
> appropriate amount of quota against that transction in preparation for a
> change of user, group, or project id.  Replace all the open-coded idioms
> with a single call to this helper so that we can contain the retry loops
> in the next patchset.
> 
> This changes the locking behavior for ichange transactions slightly.
> Since tr_ichange does not have a permanent reservation and cannot roll,
> we pass XFS_ILOCK_EXCL to ijoin so that the inode will be unlocked
> automatically at commit time.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

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

>  fs/xfs/xfs_ioctl.c |   29 ++++++++----------------
>  fs/xfs/xfs_iops.c  |   26 ++--------------------
>  fs/xfs/xfs_trans.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_trans.h |    3 +++
>  4 files changed, 77 insertions(+), 43 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 3fbd98f61ea5..78ee201eb7cb 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1275,24 +1275,23 @@ xfs_ioctl_setattr_prepare_dax(
>   */
>  static struct xfs_trans *
>  xfs_ioctl_setattr_get_trans(
> -	struct xfs_inode	*ip)
> +	struct xfs_inode	*ip,
> +	struct xfs_dquot	*pdqp)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_trans	*tp;
>  	int			error = -EROFS;
>  
>  	if (mp->m_flags & XFS_MOUNT_RDONLY)
> -		goto out_unlock;
> +		goto out_error;
>  	error = -EIO;
>  	if (XFS_FORCED_SHUTDOWN(mp))
> -		goto out_unlock;
> +		goto out_error;
>  
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> +			capable(CAP_FOWNER), &tp);
>  	if (error)
> -		goto out_unlock;
> -
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +		goto out_error;
>  
>  	/*
>  	 * CAP_FOWNER overrides the following restrictions:
> @@ -1312,7 +1311,7 @@ xfs_ioctl_setattr_get_trans(
>  
>  out_cancel:
>  	xfs_trans_cancel(tp);
> -out_unlock:
> +out_error:
>  	return ERR_PTR(error);
>  }
>  
> @@ -1462,20 +1461,12 @@ xfs_ioctl_setattr(
>  
>  	xfs_ioctl_setattr_prepare_dax(ip, fa);
>  
> -	tp = xfs_ioctl_setattr_get_trans(ip);
> +	tp = xfs_ioctl_setattr_get_trans(ip, pdqp);
>  	if (IS_ERR(tp)) {
>  		code = PTR_ERR(tp);
>  		goto error_free_dquots;
>  	}
>  
> -	if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) &&
> -	    ip->i_d.di_projid != fa->fsx_projid) {
> -		code = xfs_qm_vop_chown_reserve(tp, ip, NULL, NULL, pdqp,
> -				capable(CAP_FOWNER) ?  XFS_QMOPT_FORCE_RES : 0);
> -		if (code)	/* out of quota */
> -			goto error_trans_cancel;
> -	}
> -
>  	xfs_fill_fsxattr(ip, false, &old_fa);
>  	code = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
>  	if (code)
> @@ -1608,7 +1599,7 @@ xfs_ioc_setxflags(
>  
>  	xfs_ioctl_setattr_prepare_dax(ip, &fa);
>  
> -	tp = xfs_ioctl_setattr_get_trans(ip);
> +	tp = xfs_ioctl_setattr_get_trans(ip, NULL);
>  	if (IS_ERR(tp)) {
>  		error = PTR_ERR(tp);
>  		goto out_drop_write;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index f1e21b6cfa48..00369502fe25 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -700,13 +700,11 @@ xfs_setattr_nonsize(
>  			return error;
>  	}
>  
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> +			capable(CAP_FOWNER), &tp);
>  	if (error)
>  		goto out_dqrele;
>  
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	xfs_trans_ijoin(tp, ip, 0);
> -
>  	/*
>  	 * Change file ownership.  Must be the owner or privileged.
>  	 */
> @@ -722,21 +720,6 @@ xfs_setattr_nonsize(
>  		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
>  		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
>  
> -		/*
> -		 * Do a quota reservation only if uid/gid is actually
> -		 * going to change.
> -		 */
> -		if (XFS_IS_QUOTA_RUNNING(mp) &&
> -		    ((XFS_IS_UQUOTA_ON(mp) && !uid_eq(iuid, uid)) ||
> -		     (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
> -			ASSERT(tp);
> -			error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
> -						NULL, capable(CAP_FOWNER) ?
> -						XFS_QMOPT_FORCE_RES : 0);
> -			if (error)	/* out of quota */
> -				goto out_cancel;
> -		}
> -
>  		/*
>  		 * CAP_FSETID overrides the following restrictions:
>  		 *
> @@ -786,8 +769,6 @@ xfs_setattr_nonsize(
>  		xfs_trans_set_sync(tp);
>  	error = xfs_trans_commit(tp);
>  
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -
>  	/*
>  	 * Release any dquot(s) the inode had kept before chown.
>  	 */
> @@ -814,9 +795,6 @@ xfs_setattr_nonsize(
>  
>  	return 0;
>  
> -out_cancel:
> -	xfs_trans_cancel(tp);
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  out_dqrele:
>  	xfs_qm_dqrele(udqp);
>  	xfs_qm_dqrele(gdqp);
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 6c68635cc6ac..60672b5545c9 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -1107,3 +1107,65 @@ xfs_trans_alloc_icreate(
>  	*tpp = tp;
>  	return 0;
>  }
> +
> +/*
> + * Allocate an transaction, lock and join the inode to it, and reserve quota
> + * in preparation for inode attribute changes that include uid, gid, or prid
> + * changes.
> + *
> + * The caller must ensure that the on-disk dquots attached to this inode have
> + * already been allocated and initialized.  The ILOCK will be dropped when the
> + * transaction is committed or cancelled.
> + */
> +int
> +xfs_trans_alloc_ichange(
> +	struct xfs_inode	*ip,
> +	struct xfs_dquot	*udqp,
> +	struct xfs_dquot	*gdqp,
> +	struct xfs_dquot	*pdqp,
> +	bool			force,
> +	struct xfs_trans	**tpp)
> +{
> +	struct xfs_trans	*tp;
> +	struct xfs_mount	*mp = ip->i_mount;
> +	int			error;
> +
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	if (error)
> +		return error;
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +
> +	error = xfs_qm_dqattach_locked(ip, false);
> +	if (error) {
> +		/* Caller should have allocated the dquots! */
> +		ASSERT(error != -ENOENT);
> +		goto out_cancel;
> +	}
> +
> +	/*
> +	 * For each quota type, skip quota reservations if the inode's dquots
> +	 * now match the ones that came from the caller, or the caller didn't
> +	 * pass one in.
> +	 */
> +	if (udqp == ip->i_udquot)
> +		udqp = NULL;
> +	if (gdqp == ip->i_gdquot)
> +		gdqp = NULL;
> +	if (pdqp == ip->i_pdquot)
> +		pdqp = NULL;
> +	if (udqp || gdqp || pdqp) {
> +		error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp, pdqp,
> +				force ? XFS_QMOPT_FORCE_RES : 0);
> +		if (error)
> +			goto out_cancel;
> +	}
> +
> +	*tpp = tp;
> +	return 0;
> +
> +out_cancel:
> +	xfs_trans_cancel(tp);
> +	return error;
> +}
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 04c132c55e9b..8b03fbfe9a1b 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -277,5 +277,8 @@ int xfs_trans_alloc_icreate(struct xfs_mount *mp, struct xfs_trans_res *resv,
>  		struct xfs_dquot *udqp, struct xfs_dquot *gdqp,
>  		struct xfs_dquot *pdqp, unsigned int dblocks,
>  		struct xfs_trans **tpp);
> +int xfs_trans_alloc_ichange(struct xfs_inode *ip, struct xfs_dquot *udqp,
> +		struct xfs_dquot *gdqp, struct xfs_dquot *pdqp, bool force,
> +		struct xfs_trans **tpp);
>  
>  #endif	/* __XFS_TRANS_H__ */
> 

Re: [PATCH 14/16] xfs: remove xfs_qm_vop_chown_reserve

[Brian Foster]

On Mon, Feb 01, 2021 at 06:04:37PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that the only caller of this function is xfs_trans_alloc_ichange,
> just open-code the meat of _chown_reserve in that caller.  Drop the
> (redundant) [ugp]id checks because xfs has a 1:1 relationship between
> quota ids and incore dquots.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

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

>  fs/xfs/xfs_qm.c    |   48 ------------------------------------------------
>  fs/xfs/xfs_quota.h |    4 ----
>  fs/xfs/xfs_trans.c |   16 ++++++++++++++--
>  3 files changed, 14 insertions(+), 54 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 322d337b5dca..275cf5d7a178 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1816,54 +1816,6 @@ xfs_qm_vop_chown(
>  	return prevdq;
>  }
>  
> -/*
> - * Quota reservations for setattr(AT_UID|AT_GID|AT_PROJID).
> - */
> -int
> -xfs_qm_vop_chown_reserve(
> -	struct xfs_trans	*tp,
> -	struct xfs_inode	*ip,
> -	struct xfs_dquot	*udqp,
> -	struct xfs_dquot	*gdqp,
> -	struct xfs_dquot	*pdqp,
> -	uint			flags)
> -{
> -	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;
> -
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> -	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> -
> -	blkflags = XFS_IS_REALTIME_INODE(ip) ?
> -			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
> -
> -	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> -	    i_uid_read(VFS_I(ip)) != udqp->q_id)
> -		udq_delblks = udqp;
> -
> -	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> -	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
> -		gdq_delblks = gdqp;
> -
> -	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
> -	    ip->i_d.di_projid != pdqp->q_id)
> -		pdq_delblks = pdqp;
> -
> -	/*
> -	 * Reserve enough quota to handle blocks on disk and reserved for a
> -	 * delayed allocation.  We'll actually transfer the delalloc
> -	 * 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,
> -			ip->i_d.di_nblocks + ip->i_delayed_blks,
> -			1, blkflags | flags);
> -}
> -
>  int
>  xfs_qm_vop_rename_dqattach(
>  	struct xfs_inode	**i_tab)
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index 6ddc4b358ede..d00d01302545 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -98,9 +98,6 @@ extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
>  extern int xfs_qm_vop_rename_dqattach(struct xfs_inode **);
>  extern struct xfs_dquot *xfs_qm_vop_chown(struct xfs_trans *,
>  		struct xfs_inode *, struct xfs_dquot **, struct xfs_dquot *);
> -extern int xfs_qm_vop_chown_reserve(struct xfs_trans *, struct xfs_inode *,
> -		struct xfs_dquot *, struct xfs_dquot *,
> -		struct xfs_dquot *, uint);
>  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 *);
> @@ -162,7 +159,6 @@ xfs_trans_reserve_quota_icreate(struct xfs_trans *tp, struct xfs_dquot *udqp,
>  #define xfs_qm_vop_create_dqattach(tp, ip, u, g, p)
>  #define xfs_qm_vop_rename_dqattach(it)					(0)
>  #define xfs_qm_vop_chown(tp, ip, old, new)				(NULL)
> -#define xfs_qm_vop_chown_reserve(tp, ip, u, g, p, fl)			(0)
>  #define xfs_qm_dqattach(ip)						(0)
>  #define xfs_qm_dqattach_locked(ip, fl)					(0)
>  #define xfs_qm_dqdetach(ip)
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 60672b5545c9..29dca1bc4c1a 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -1156,8 +1156,20 @@ xfs_trans_alloc_ichange(
>  	if (pdqp == ip->i_pdquot)
>  		pdqp = NULL;
>  	if (udqp || gdqp || pdqp) {
> -		error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp, pdqp,
> -				force ? XFS_QMOPT_FORCE_RES : 0);
> +		unsigned int	qflags = XFS_QMOPT_RES_REGBLKS;
> +
> +		if (force)
> +			qflags |= XFS_QMOPT_FORCE_RES;
> +
> +		/*
> +		 * Reserve enough quota to handle blocks on disk and reserved
> +		 * for a delayed allocation.  We'll actually transfer the
> +		 * delalloc reservation between dquots at chown time, even
> +		 * though that part is only semi-transactional.
> +		 */
> +		error = xfs_trans_reserve_quota_bydquots(tp, mp, udqp, gdqp,
> +				pdqp, ip->i_d.di_nblocks + ip->i_delayed_blks,
> +				1, qflags);
>  		if (error)
>  			goto out_cancel;
>  	}
> 

Re: [PATCH 15/16] xfs: rename code to error in xfs_ioctl_setattr

[Brian Foster]

On Mon, Feb 01, 2021 at 06:04:43PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Rename the 'code' variable to 'error' to follow the naming convention of
> most other functions in xfs.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_ioctl.c |   38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 78ee201eb7cb..38ee66d999d8 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1435,13 +1435,13 @@ xfs_ioctl_setattr(
>  	struct xfs_trans	*tp;
>  	struct xfs_dquot	*pdqp = NULL;
>  	struct xfs_dquot	*olddquot = NULL;
> -	int			code;
> +	int			error;
>  
>  	trace_xfs_ioctl_setattr(ip);
>  
> -	code = xfs_ioctl_setattr_check_projid(ip, fa);
> -	if (code)
> -		return code;
> +	error = xfs_ioctl_setattr_check_projid(ip, fa);
> +	if (error)
> +		return error;
>  
>  	/*
>  	 * If disk quotas is on, we make sure that the dquots do exist on disk,
> @@ -1452,36 +1452,36 @@ xfs_ioctl_setattr(
>  	 * because the i_*dquot fields will get updated anyway.
>  	 */
>  	if (XFS_IS_QUOTA_ON(mp)) {
> -		code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
> +		error = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
>  				VFS_I(ip)->i_gid, fa->fsx_projid,
>  				XFS_QMOPT_PQUOTA, NULL, NULL, &pdqp);
> -		if (code)
> -			return code;
> +		if (error)
> +			return error;
>  	}
>  
>  	xfs_ioctl_setattr_prepare_dax(ip, fa);
>  
>  	tp = xfs_ioctl_setattr_get_trans(ip, pdqp);
>  	if (IS_ERR(tp)) {
> -		code = PTR_ERR(tp);
> +		error = PTR_ERR(tp);
>  		goto error_free_dquots;
>  	}
>  
>  	xfs_fill_fsxattr(ip, false, &old_fa);
> -	code = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
> -	if (code)
> +	error = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
> +	if (error)
>  		goto error_trans_cancel;
>  
> -	code = xfs_ioctl_setattr_check_extsize(ip, fa);
> -	if (code)
> +	error = xfs_ioctl_setattr_check_extsize(ip, fa);
> +	if (error)
>  		goto error_trans_cancel;
>  
> -	code = xfs_ioctl_setattr_check_cowextsize(ip, fa);
> -	if (code)
> +	error = xfs_ioctl_setattr_check_cowextsize(ip, fa);
> +	if (error)
>  		goto error_trans_cancel;
>  
> -	code = xfs_ioctl_setattr_xflags(tp, ip, fa);
> -	if (code)
> +	error = xfs_ioctl_setattr_xflags(tp, ip, fa);
> +	if (error)
>  		goto error_trans_cancel;
>  
>  	/*
> @@ -1521,7 +1521,7 @@ xfs_ioctl_setattr(
>  	else
>  		ip->i_d.di_cowextsize = 0;
>  
> -	code = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp);
>  
>  	/*
>  	 * Release any dquot(s) the inode had kept before chown.
> @@ -1529,13 +1529,13 @@ xfs_ioctl_setattr(
>  	xfs_qm_dqrele(olddquot);
>  	xfs_qm_dqrele(pdqp);
>  
> -	return code;
> +	return error;
>  
>  error_trans_cancel:
>  	xfs_trans_cancel(tp);
>  error_free_dquots:
>  	xfs_qm_dqrele(pdqp);
> -	return code;
> +	return error;
>  }
>  
>  STATIC int
> 

Re: [PATCH 16/16] xfs: shut down the filesystem if we screw up quota errors

[Brian Foster]

On Mon, Feb 01, 2021 at 06:04:48PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If we ever screw up the quota reservations enough to trip the
> assertions, something's wrong with the quota code.  Shut down the
> filesystem when this happens, because this is corruption.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_trans_dquot.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index a1a72b7900c5..48e09ea30ee5 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -16,6 +16,7 @@
>  #include "xfs_quota.h"
>  #include "xfs_qm.h"
>  #include "xfs_trace.h"
> +#include "xfs_error.h"
>  
>  STATIC void	xfs_trans_alloc_dqinfo(xfs_trans_t *);
>  
> @@ -691,9 +692,11 @@ xfs_trans_dqresv(
>  				    nblks);
>  		xfs_trans_mod_dquot(tp, dqp, XFS_TRANS_DQ_RES_INOS, ninos);
>  	}
> -	ASSERT(dqp->q_blk.reserved >= dqp->q_blk.count);
> -	ASSERT(dqp->q_rtb.reserved >= dqp->q_rtb.count);
> -	ASSERT(dqp->q_ino.reserved >= dqp->q_ino.count);
> +
> +	if (XFS_IS_CORRUPT(mp, dqp->q_blk.reserved < dqp->q_blk.count) ||
> +	    XFS_IS_CORRUPT(mp, dqp->q_rtb.reserved < dqp->q_rtb.count) ||
> +	    XFS_IS_CORRUPT(mp, dqp->q_ino.reserved < dqp->q_ino.count))
> +		goto error_corrupt;
>  
>  	xfs_dqunlock(dqp);
>  	return 0;
> @@ -703,6 +706,10 @@ xfs_trans_dqresv(
>  	if (xfs_dquot_type(dqp) == XFS_DQTYPE_PROJ)
>  		return -ENOSPC;
>  	return -EDQUOT;
> +error_corrupt:
> +	xfs_dqunlock(dqp);
> +	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> +	return -EFSCORRUPTED;
>  }
>  
>  
> 

Re: [PATCH 01/16] xfs: fix chown leaking delalloc quota blocks when fssetxattr fails

[Darrick J. Wong]

On Tue, Feb 02, 2021 at 08:13:15AM -0500, Brian Foster wrote:
> On Mon, Feb 01, 2021 at 06:03:23PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > While refactoring the quota code to create a function to allocate inode
> > change transactions, I noticed that xfs_qm_vop_chown_reserve does more
> > than just make reservations: it also *modifies* the incore counts
> > directly to handle the owner id change for the delalloc blocks.
> > 
> > I then observed that the fssetxattr code continues validating input
> > arguments after making the quota reservation but before dirtying the
> > transaction.  If the routine decides to error out, it fails to undo the
> > accounting switch!  This leads to incorrect quota reservation and
> > failure down the line.
> > 
> > We can fix this by making the reservation function do only that -- for
> > the new dquot, it reserves ondisk and delalloc blocks to the
> > transaction, and the old dquot hangs on to its incore reservation for
> > now.  Once we actually switch the dquots, we can then update the incore
> > reservations because we've dirtied the transaction and it's too late to
> > turn back now.
> > 
> > No fixes tag because this has been broken since the start of git.
> > 
> 
> Do we have any test coverage for this problem?

Working on it, yeah.  The tricky part is finding a combination of
FSSETXATTR values that will reliably trigger an error return.  I think
it can be done by allocating delalloc blocks and then trying to set an
extent size hint and change the project id in a single call.

> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/xfs_qm.c |   91 +++++++++++++++++++++----------------------------------
> >  1 file changed, 34 insertions(+), 57 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > index c134eb4aeaa8..322d337b5dca 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -1785,6 +1785,28 @@ xfs_qm_vop_chown(
> >  	xfs_trans_mod_dquot(tp, newdq, bfield, ip->i_d.di_nblocks);
> >  	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_ICOUNT, 1);
> >  
> > +	/*
> > +	 * Back when we made quota reservations for the chown, we reserved the
> > +	 * ondisk blocks + delalloc blocks with the new dquot.  Now that we've
> > +	 * switched the dquots, decrease the new dquot's block reservation
> > +	 * (having already bumped up the real counter) so that we don't have
> > +	 * any reservation to give back when we commit.
> > +	 */
> > +	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_RES_BLKS,
> > +			-ip->i_delayed_blks);
> > +
> 
> Ok, so the reservation code code below continues to reserve di_blocks
> and delalloc blocks as it did before, but associates it with the
> transaction and no longer reduces delalloc reservation from the old
> dquots. Instead, this function increases the consumption of reserved
> blocks for di_blocks and then removes the additional quota reservation
> from the transaction, but not the new dquots. Thus when the transaction
> commits, this has the side effect of leaving additional reservation on
> the new dquots that correspond to the delalloc blocks on the inode. Am I
> following that correctly?

Correct.

> > +	/*
> > +	 * Give the incore reservation for delalloc blocks back to the old
> > +	 * dquot.  We don't normally handle delalloc quota reservations
> > +	 * transactionally, so just lock the dquot and subtract from the
> > +	 * reservation.  We've dirtied the transaction, so it's too late to
> > +	 * turn back now.
> > +	 */
> > +	tp->t_flags |= XFS_TRANS_DIRTY;
> > +	xfs_dqlock(prevdq);
> > +	prevdq->q_blk.reserved -= ip->i_delayed_blks;
> > +	xfs_dqunlock(prevdq);
> > +
> 
> What's the reason for not using xfs_trans_reserve_quota_bydquots(NULL,
> ...) here like the original code?

xfs_trans_reserve_quota_bydquots() makes the caller pass in user, group,
and project dquots.  It's not difficult to add more code to declare and
route parameters, but that just felt overdone.

Given that this is the only place in the codebase where we want to
change the incore quota reservation on a single dquot, I also didn't
think it was worth making a whole new function.

FWIW I don't really mind doing it, it just seemed like more work.
Alternately I suppose I could expose xfs_trans_dqresv.

--D

> Brian
> 
> >  	/*
> >  	 * Take an extra reference, because the inode is going to keep
> >  	 * this dquot pointer even after the trans_commit.
> > @@ -1807,84 +1829,39 @@ xfs_qm_vop_chown_reserve(
> >  	uint			flags)
> >  {
> >  	struct xfs_mount	*mp = ip->i_mount;
> > -	uint64_t		delblks;
> >  	unsigned int		blkflags;
> > -	struct xfs_dquot	*udq_unres = NULL;
> > -	struct xfs_dquot	*gdq_unres = NULL;
> > -	struct xfs_dquot	*pdq_unres = NULL;
> >  	struct xfs_dquot	*udq_delblks = NULL;
> >  	struct xfs_dquot	*gdq_delblks = NULL;
> >  	struct xfs_dquot	*pdq_delblks = NULL;
> > -	int			error;
> > -
> >  
> >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> >  	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> >  
> > -	delblks = ip->i_delayed_blks;
> >  	blkflags = XFS_IS_REALTIME_INODE(ip) ?
> >  			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
> >  
> >  	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> > -	    i_uid_read(VFS_I(ip)) != udqp->q_id) {
> > +	    i_uid_read(VFS_I(ip)) != udqp->q_id)
> >  		udq_delblks = udqp;
> > -		/*
> > -		 * If there are delayed allocation blocks, then we have to
> > -		 * unreserve those from the old dquot, and add them to the
> > -		 * new dquot.
> > -		 */
> > -		if (delblks) {
> > -			ASSERT(ip->i_udquot);
> > -			udq_unres = ip->i_udquot;
> > -		}
> > -	}
> > +
> >  	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> > -	    i_gid_read(VFS_I(ip)) != gdqp->q_id) {
> > +	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
> >  		gdq_delblks = gdqp;
> > -		if (delblks) {
> > -			ASSERT(ip->i_gdquot);
> > -			gdq_unres = ip->i_gdquot;
> > -		}
> > -	}
> >  
> >  	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
> > -	    ip->i_d.di_projid != pdqp->q_id) {
> > +	    ip->i_d.di_projid != pdqp->q_id)
> >  		pdq_delblks = pdqp;
> > -		if (delblks) {
> > -			ASSERT(ip->i_pdquot);
> > -			pdq_unres = ip->i_pdquot;
> > -		}
> > -	}
> > -
> > -	error = xfs_trans_reserve_quota_bydquots(tp, ip->i_mount,
> > -				udq_delblks, gdq_delblks, pdq_delblks,
> > -				ip->i_d.di_nblocks, 1, flags | blkflags);
> > -	if (error)
> > -		return error;
> >  
> >  	/*
> > -	 * Do the delayed blks reservations/unreservations now. Since, these
> > -	 * are done without the help of a transaction, if a reservation fails
> > -	 * its previous reservations won't be automatically undone by trans
> > -	 * code. So, we have to do it manually here.
> > +	 * Reserve enough quota to handle blocks on disk and reserved for a
> > +	 * delayed allocation.  We'll actually transfer the delalloc
> > +	 * reservation between dquots at chown time, even though that part is
> > +	 * only semi-transactional.
> >  	 */
> > -	if (delblks) {
> > -		/*
> > -		 * Do the reservations first. Unreservation can't fail.
> > -		 */
> > -		ASSERT(udq_delblks || gdq_delblks || pdq_delblks);
> > -		ASSERT(udq_unres || gdq_unres || pdq_unres);
> > -		error = xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> > -			    udq_delblks, gdq_delblks, pdq_delblks,
> > -			    (xfs_qcnt_t)delblks, 0, flags | blkflags);
> > -		if (error)
> > -			return error;
> > -		xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> > -				udq_unres, gdq_unres, pdq_unres,
> > -				-((xfs_qcnt_t)delblks), 0, blkflags);
> > -	}
> > -
> > -	return 0;
> > +	return xfs_trans_reserve_quota_bydquots(tp, ip->i_mount, udq_delblks,
> > +			gdq_delblks, pdq_delblks,
> > +			ip->i_d.di_nblocks + ip->i_delayed_blks,
> > +			1, blkflags | flags);
> >  }
> >  
> >  int
> > 
> 

Re: [PATCH 01/16] xfs: fix chown leaking delalloc quota blocks when fssetxattr fails

[Christoph Hellwig]

On Tue, Feb 02, 2021 at 09:47:26AM -0800, Darrick J. Wong wrote:
> > > +	prevdq->q_blk.reserved -= ip->i_delayed_blks;
> > > +	xfs_dqunlock(prevdq);
> > > +
> > 
> > What's the reason for not using xfs_trans_reserve_quota_bydquots(NULL,
> > ...) here like the original code?
> 
> xfs_trans_reserve_quota_bydquots() makes the caller pass in user, group,
> and project dquots.  It's not difficult to add more code to declare and
> route parameters, but that just felt overdone.
> 
> Given that this is the only place in the codebase where we want to
> change the incore quota reservation on a single dquot, I also didn't
> think it was worth making a whole new function.
> 
> FWIW I don't really mind doing it, it just seemed like more work.
> Alternately I suppose I could expose xfs_trans_dqresv.

xfs_trans_dqresv sounds way better than
xfs_trans_reserve_quota_bydquots. But I'm also perfectly fine with
the current open coded version.  If we insist on layering my preference
would be a new custom helper just for this case instead of going through
much more heavyweight functions.

Re: [PATCH 01/16] xfs: fix chown leaking delalloc quota blocks when fssetxattr fails

[Brian Foster]

On Tue, Feb 02, 2021 at 09:47:26AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 02, 2021 at 08:13:15AM -0500, Brian Foster wrote:
> > On Mon, Feb 01, 2021 at 06:03:23PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > While refactoring the quota code to create a function to allocate inode
> > > change transactions, I noticed that xfs_qm_vop_chown_reserve does more
> > > than just make reservations: it also *modifies* the incore counts
> > > directly to handle the owner id change for the delalloc blocks.
> > > 
> > > I then observed that the fssetxattr code continues validating input
> > > arguments after making the quota reservation but before dirtying the
> > > transaction.  If the routine decides to error out, it fails to undo the
> > > accounting switch!  This leads to incorrect quota reservation and
> > > failure down the line.
> > > 
> > > We can fix this by making the reservation function do only that -- for
> > > the new dquot, it reserves ondisk and delalloc blocks to the
> > > transaction, and the old dquot hangs on to its incore reservation for
> > > now.  Once we actually switch the dquots, we can then update the incore
> > > reservations because we've dirtied the transaction and it's too late to
> > > turn back now.
> > > 
> > > No fixes tag because this has been broken since the start of git.
> > > 
> > 
> > Do we have any test coverage for this problem?
> 
> Working on it, yeah.  The tricky part is finding a combination of
> FSSETXATTR values that will reliably trigger an error return.  I think
> it can be done by allocating delalloc blocks and then trying to set an
> extent size hint and change the project id in a single call.
> 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_qm.c |   91 +++++++++++++++++++++----------------------------------
> > >  1 file changed, 34 insertions(+), 57 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > > index c134eb4aeaa8..322d337b5dca 100644
> > > --- a/fs/xfs/xfs_qm.c
> > > +++ b/fs/xfs/xfs_qm.c
> > > @@ -1785,6 +1785,28 @@ xfs_qm_vop_chown(
> > >  	xfs_trans_mod_dquot(tp, newdq, bfield, ip->i_d.di_nblocks);
> > >  	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_ICOUNT, 1);
> > >  
> > > +	/*
> > > +	 * Back when we made quota reservations for the chown, we reserved the
> > > +	 * ondisk blocks + delalloc blocks with the new dquot.  Now that we've
> > > +	 * switched the dquots, decrease the new dquot's block reservation
> > > +	 * (having already bumped up the real counter) so that we don't have
> > > +	 * any reservation to give back when we commit.
> > > +	 */
> > > +	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_RES_BLKS,
> > > +			-ip->i_delayed_blks);
> > > +
> > 
> > Ok, so the reservation code code below continues to reserve di_blocks
> > and delalloc blocks as it did before, but associates it with the
> > transaction and no longer reduces delalloc reservation from the old
> > dquots. Instead, this function increases the consumption of reserved
> > blocks for di_blocks and then removes the additional quota reservation
> > from the transaction, but not the new dquots. Thus when the transaction
> > commits, this has the side effect of leaving additional reservation on
> > the new dquots that correspond to the delalloc blocks on the inode. Am I
> > following that correctly?
> 
> Correct.
> 

Thanks.

> > > +	/*
> > > +	 * Give the incore reservation for delalloc blocks back to the old
> > > +	 * dquot.  We don't normally handle delalloc quota reservations
> > > +	 * transactionally, so just lock the dquot and subtract from the
> > > +	 * reservation.  We've dirtied the transaction, so it's too late to
> > > +	 * turn back now.
> > > +	 */
> > > +	tp->t_flags |= XFS_TRANS_DIRTY;
> > > +	xfs_dqlock(prevdq);
> > > +	prevdq->q_blk.reserved -= ip->i_delayed_blks;
> > > +	xfs_dqunlock(prevdq);
> > > +
> > 
> > What's the reason for not using xfs_trans_reserve_quota_bydquots(NULL,
> > ...) here like the original code?
> 
> xfs_trans_reserve_quota_bydquots() makes the caller pass in user, group,
> and project dquots.  It's not difficult to add more code to declare and
> route parameters, but that just felt overdone.
> 

Ah, Ok.

> Given that this is the only place in the codebase where we want to
> change the incore quota reservation on a single dquot, I also didn't
> think it was worth making a whole new function.
> 
> FWIW I don't really mind doing it, it just seemed like more work.
> Alternately I suppose I could expose xfs_trans_dqresv.
> 

I have no strong preference. It seems fine to me as it is, the reason
for the change just wasn't clear to me at first glance:

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

> --D
> 
> > Brian
> > 
> > >  	/*
> > >  	 * Take an extra reference, because the inode is going to keep
> > >  	 * this dquot pointer even after the trans_commit.
> > > @@ -1807,84 +1829,39 @@ xfs_qm_vop_chown_reserve(
> > >  	uint			flags)
> > >  {
> > >  	struct xfs_mount	*mp = ip->i_mount;
> > > -	uint64_t		delblks;
> > >  	unsigned int		blkflags;
> > > -	struct xfs_dquot	*udq_unres = NULL;
> > > -	struct xfs_dquot	*gdq_unres = NULL;
> > > -	struct xfs_dquot	*pdq_unres = NULL;
> > >  	struct xfs_dquot	*udq_delblks = NULL;
> > >  	struct xfs_dquot	*gdq_delblks = NULL;
> > >  	struct xfs_dquot	*pdq_delblks = NULL;
> > > -	int			error;
> > > -
> > >  
> > >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> > >  	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> > >  
> > > -	delblks = ip->i_delayed_blks;
> > >  	blkflags = XFS_IS_REALTIME_INODE(ip) ?
> > >  			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
> > >  
> > >  	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> > > -	    i_uid_read(VFS_I(ip)) != udqp->q_id) {
> > > +	    i_uid_read(VFS_I(ip)) != udqp->q_id)
> > >  		udq_delblks = udqp;
> > > -		/*
> > > -		 * If there are delayed allocation blocks, then we have to
> > > -		 * unreserve those from the old dquot, and add them to the
> > > -		 * new dquot.
> > > -		 */
> > > -		if (delblks) {
> > > -			ASSERT(ip->i_udquot);
> > > -			udq_unres = ip->i_udquot;
> > > -		}
> > > -	}
> > > +
> > >  	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> > > -	    i_gid_read(VFS_I(ip)) != gdqp->q_id) {
> > > +	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
> > >  		gdq_delblks = gdqp;
> > > -		if (delblks) {
> > > -			ASSERT(ip->i_gdquot);
> > > -			gdq_unres = ip->i_gdquot;
> > > -		}
> > > -	}
> > >  
> > >  	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
> > > -	    ip->i_d.di_projid != pdqp->q_id) {
> > > +	    ip->i_d.di_projid != pdqp->q_id)
> > >  		pdq_delblks = pdqp;
> > > -		if (delblks) {
> > > -			ASSERT(ip->i_pdquot);
> > > -			pdq_unres = ip->i_pdquot;
> > > -		}
> > > -	}
> > > -
> > > -	error = xfs_trans_reserve_quota_bydquots(tp, ip->i_mount,
> > > -				udq_delblks, gdq_delblks, pdq_delblks,
> > > -				ip->i_d.di_nblocks, 1, flags | blkflags);
> > > -	if (error)
> > > -		return error;
> > >  
> > >  	/*
> > > -	 * Do the delayed blks reservations/unreservations now. Since, these
> > > -	 * are done without the help of a transaction, if a reservation fails
> > > -	 * its previous reservations won't be automatically undone by trans
> > > -	 * code. So, we have to do it manually here.
> > > +	 * Reserve enough quota to handle blocks on disk and reserved for a
> > > +	 * delayed allocation.  We'll actually transfer the delalloc
> > > +	 * reservation between dquots at chown time, even though that part is
> > > +	 * only semi-transactional.
> > >  	 */
> > > -	if (delblks) {
> > > -		/*
> > > -		 * Do the reservations first. Unreservation can't fail.
> > > -		 */
> > > -		ASSERT(udq_delblks || gdq_delblks || pdq_delblks);
> > > -		ASSERT(udq_unres || gdq_unres || pdq_unres);
> > > -		error = xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> > > -			    udq_delblks, gdq_delblks, pdq_delblks,
> > > -			    (xfs_qcnt_t)delblks, 0, flags | blkflags);
> > > -		if (error)
> > > -			return error;
> > > -		xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> > > -				udq_unres, gdq_unres, pdq_unres,
> > > -				-((xfs_qcnt_t)delblks), 0, blkflags);
> > > -	}
> > > -
> > > -	return 0;
> > > +	return xfs_trans_reserve_quota_bydquots(tp, ip->i_mount, udq_delblks,
> > > +			gdq_delblks, pdq_delblks,
> > > +			ip->i_d.di_nblocks + ip->i_delayed_blks,
> > > +			1, blkflags | flags);
> > >  }
> > >  
> > >  int
> > > 
> > 
> 

[PATCH v6.1 01/16] xfs: fix chown leaking delalloc quota blocks when fssetxattr fails

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

While refactoring the quota code to create a function to allocate inode
change transactions, I noticed that xfs_qm_vop_chown_reserve does more
than just make reservations: it also *modifies* the incore counts
directly to handle the owner id change for the delalloc blocks.

I then observed that the fssetxattr code continues validating input
arguments after making the quota reservation but before dirtying the
transaction.  If the routine decides to error out, it fails to undo the
accounting switch!  This leads to incorrect quota reservation and
failure down the line.

We can fix this by making the reservation function do only that -- for
the new dquot, it reserves ondisk and delalloc blocks to the
transaction, and the old dquot hangs on to its incore reservation for
now.  Once we actually switch the dquots, we can then update the incore
reservations because we've dirtied the transaction and it's too late to
turn back now.

No fixes tag because this has been broken since the start of git.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
v6.1: I added an debugging assert to make sure we're not totally fouling
up the incore reservations when we chown
---
 fs/xfs/xfs_qm.c |   92 +++++++++++++++++++++----------------------------------
 1 file changed, 35 insertions(+), 57 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index c134eb4aeaa8..c2e4d3a27469 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1785,6 +1785,29 @@ xfs_qm_vop_chown(
 	xfs_trans_mod_dquot(tp, newdq, bfield, ip->i_d.di_nblocks);
 	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_ICOUNT, 1);
 
+	/*
+	 * Back when we made quota reservations for the chown, we reserved the
+	 * ondisk blocks + delalloc blocks with the new dquot.  Now that we've
+	 * switched the dquots, decrease the new dquot's block reservation
+	 * (having already bumped up the real counter) so that we don't have
+	 * any reservation to give back when we commit.
+	 */
+	xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_RES_BLKS,
+			-ip->i_delayed_blks);
+
+	/*
+	 * Give the incore reservation for delalloc blocks back to the old
+	 * dquot.  We don't normally handle delalloc quota reservations
+	 * transactionally, so just lock the dquot and subtract from the
+	 * reservation.  Dirty the transaction because it's too late to turn
+	 * back now.
+	 */
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	xfs_dqlock(prevdq);
+	ASSERT(prevdq->q_blk.reserved >= ip->i_delayed_blks);
+	prevdq->q_blk.reserved -= ip->i_delayed_blks;
+	xfs_dqunlock(prevdq);
+
 	/*
 	 * Take an extra reference, because the inode is going to keep
 	 * this dquot pointer even after the trans_commit.
@@ -1807,84 +1830,39 @@ xfs_qm_vop_chown_reserve(
 	uint			flags)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	uint64_t		delblks;
 	unsigned int		blkflags;
-	struct xfs_dquot	*udq_unres = NULL;
-	struct xfs_dquot	*gdq_unres = NULL;
-	struct xfs_dquot	*pdq_unres = NULL;
 	struct xfs_dquot	*udq_delblks = NULL;
 	struct xfs_dquot	*gdq_delblks = NULL;
 	struct xfs_dquot	*pdq_delblks = NULL;
-	int			error;
-
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
-	delblks = ip->i_delayed_blks;
 	blkflags = XFS_IS_REALTIME_INODE(ip) ?
 			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    i_uid_read(VFS_I(ip)) != udqp->q_id) {
+	    i_uid_read(VFS_I(ip)) != udqp->q_id)
 		udq_delblks = udqp;
-		/*
-		 * If there are delayed allocation blocks, then we have to
-		 * unreserve those from the old dquot, and add them to the
-		 * new dquot.
-		 */
-		if (delblks) {
-			ASSERT(ip->i_udquot);
-			udq_unres = ip->i_udquot;
-		}
-	}
+
 	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
-	    i_gid_read(VFS_I(ip)) != gdqp->q_id) {
+	    i_gid_read(VFS_I(ip)) != gdqp->q_id)
 		gdq_delblks = gdqp;
-		if (delblks) {
-			ASSERT(ip->i_gdquot);
-			gdq_unres = ip->i_gdquot;
-		}
-	}
 
 	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
-	    ip->i_d.di_projid != pdqp->q_id) {
+	    ip->i_d.di_projid != pdqp->q_id)
 		pdq_delblks = pdqp;
-		if (delblks) {
-			ASSERT(ip->i_pdquot);
-			pdq_unres = ip->i_pdquot;
-		}
-	}
-
-	error = xfs_trans_reserve_quota_bydquots(tp, ip->i_mount,
-				udq_delblks, gdq_delblks, pdq_delblks,
-				ip->i_d.di_nblocks, 1, flags | blkflags);
-	if (error)
-		return error;
 
 	/*
-	 * Do the delayed blks reservations/unreservations now. Since, these
-	 * are done without the help of a transaction, if a reservation fails
-	 * its previous reservations won't be automatically undone by trans
-	 * code. So, we have to do it manually here.
+	 * Reserve enough quota to handle blocks on disk and reserved for a
+	 * delayed allocation.  We'll actually transfer the delalloc
+	 * reservation between dquots at chown time, even though that part is
+	 * only semi-transactional.
 	 */
-	if (delblks) {
-		/*
-		 * Do the reservations first. Unreservation can't fail.
-		 */
-		ASSERT(udq_delblks || gdq_delblks || pdq_delblks);
-		ASSERT(udq_unres || gdq_unres || pdq_unres);
-		error = xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
-			    udq_delblks, gdq_delblks, pdq_delblks,
-			    (xfs_qcnt_t)delblks, 0, flags | blkflags);
-		if (error)
-			return error;
-		xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
-				udq_unres, gdq_unres, pdq_unres,
-				-((xfs_qcnt_t)delblks), 0, blkflags);
-	}
-
-	return 0;
+	return xfs_trans_reserve_quota_bydquots(tp, ip->i_mount, udq_delblks,
+			gdq_delblks, pdq_delblks,
+			ip->i_d.di_nblocks + ip->i_delayed_blks,
+			1, blkflags | flags);
 }
 
 int