[PATCH v2 00/13] xfs-4.18: quota refactor

[PATCH v2 00/13] xfs-4.18: quota refactor

[Darrick J. Wong]

Hi all,

Here's a series to refactor and clean out the incohesive mess that
xfs_qm_dqget has become.

I've distinguished three contexts in which dqget can be called, and
converted them into separate externally visible interfaces --
_qm_dqget_next, which returns the first initialized dquot with an id at
least as high as the id passed in; _qm_dqget_inode, which returns the
dquot for a given (inode, type); and so _qm_dqget now only returns the
dquot for a given (id, type).  I removed unused parameters from the
dqattach functions, renamed _qm_dqiterate (since it now only knows how
to zap dquot counters during quotacheck), and added a real function to
iterate all dquots of a given type.

This means that all _dqget variants no longer take QMOPT flags, and
QMOPT_NEXT is gone.  XFS_QMOPT_DQALLOC is now a boolean and can
only be specified for functions where it makes sense.

If you're going to start using this mess, you probably ought to just
pull from my git trees.  The kernel patches[1] should apply against
4.17-rc1.  xfsprogs[2] and xfstests[3] can be found in their usual
places.  The git trees contain all four series' worth of changes.

This is an extraordinary way to eat your data.  Enjoy!
Comments and questions are, as always, welcome.

--D

[1] https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=djwong-devel
[2] https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=djwong-devel
[3] https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=djwong-devel

[PATCH 01/13] xfs: refactor XFS_QMOPT_DQNEXT out of existence

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

There's only one caller of DQNEXT and its semantics can be moved into a
separate function, so create the function and get rid of the flag.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_quota_defs.h |    1 
 fs/xfs/scrub/quota.c           |    3 -
 fs/xfs/xfs_dquot.c             |   63 ++++++++++++++------------
 fs/xfs/xfs_dquot.h             |    2 +
 fs/xfs/xfs_qm.h                |    6 ++-
 fs/xfs/xfs_qm_syscalls.c       |   96 +++++++++++++++++++++++++++-------------
 fs/xfs/xfs_quotaops.c          |    8 +--
 7 files changed, 108 insertions(+), 71 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
index bb1b13a9b5f4..c80b22629c78 100644
--- a/fs/xfs/libxfs/xfs_quota_defs.h
+++ b/fs/xfs/libxfs/xfs_quota_defs.h
@@ -114,7 +114,6 @@ typedef uint16_t	xfs_qwarncnt_t;
 #define XFS_QMOPT_SBVERSION	0x0000040 /* change superblock version num */
 #define XFS_QMOPT_GQUOTA	0x0002000 /* group dquot requested */
 #define XFS_QMOPT_ENOSPC	0x0004000 /* enospc instead of edquot (prj) */
-#define XFS_QMOPT_DQNEXT	0x0008000 /* return next dquot >= this ID */
 
 /*
  * flags to xfs_trans_mod_dquot to indicate which field needs to be
diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index 6ba465e6c885..50415e8e5dd1 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -268,8 +268,7 @@ xfs_scrub_quota(
 		if (xfs_scrub_should_terminate(sc, &error))
 			break;
 
-		error = xfs_qm_dqget(mp, NULL, id, dqtype, XFS_QMOPT_DQNEXT,
-				&dq);
+		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
 		if (error == -ENOENT)
 			break;
 		if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index a7daef9e16bf..79a9df691d6c 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -728,18 +728,6 @@ xfs_qm_dqget(
 			goto restart;
 		}
 
-		/* uninit / unused quota found in radix tree, keep looking  */
-		if (flags & XFS_QMOPT_DQNEXT) {
-			if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
-				xfs_dqunlock(dqp);
-				mutex_unlock(&qi->qi_tree_lock);
-				error = xfs_dq_get_next_id(mp, type, &id);
-				if (error)
-					return error;
-				goto restart;
-			}
-		}
-
 		dqp->q_nrefs++;
 		mutex_unlock(&qi->qi_tree_lock);
 
@@ -766,13 +754,6 @@ xfs_qm_dqget(
 	if (ip)
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
 
-	/* If we are asked to find next active id, keep looking */
-	if (error == -ENOENT && (flags & XFS_QMOPT_DQNEXT)) {
-		error = xfs_dq_get_next_id(mp, type, &id);
-		if (!error)
-			goto restart;
-	}
-
 	if (error)
 		return error;
 
@@ -823,17 +804,6 @@ xfs_qm_dqget(
 	qi->qi_dquots++;
 	mutex_unlock(&qi->qi_tree_lock);
 
-	/* If we are asked to find next active id, keep looking */
-	if (flags & XFS_QMOPT_DQNEXT) {
-		if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
-			xfs_qm_dqput(dqp);
-			error = xfs_dq_get_next_id(mp, type, &id);
-			if (error)
-				return error;
-			goto restart;
-		}
-	}
-
  dqret:
 	ASSERT((ip == NULL) || xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	trace_xfs_dqget_miss(dqp);
@@ -841,6 +811,39 @@ xfs_qm_dqget(
 	return 0;
 }
 
+/*
+ * Starting at @id and progressing upwards, look for an initialized incore
+ * dquot, lock it, and return it.
+ */
+int
+xfs_qm_dqget_next(
+	struct xfs_mount	*mp,
+	xfs_dqid_t		id,
+	uint			type,
+	struct xfs_dquot	**dqpp)
+{
+	struct xfs_dquot	*dqp;
+	int			error = 0;
+
+	*dqpp = NULL;
+	for (; !error; error = xfs_dq_get_next_id(mp, type, &id)) {
+		error = xfs_qm_dqget(mp, NULL, id, type, 0, &dqp);
+		if (error == -ENOENT)
+			continue;
+		else if (error != 0)
+			break;
+
+		if (!XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
+			*dqpp = dqp;
+			return 0;
+		}
+
+		xfs_qm_dqput(dqp);
+	}
+
+	return error;
+}
+
 /*
  * Release a reference to the dquot (decrement ref-count) and unlock it.
  *
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 2f536f33cd26..303e71dfcf70 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -171,6 +171,8 @@ extern void		xfs_qm_adjust_dqlimits(struct xfs_mount *,
 					       struct xfs_dquot *);
 extern int		xfs_qm_dqget(xfs_mount_t *, xfs_inode_t *,
 					xfs_dqid_t, uint, uint, xfs_dquot_t **);
+extern int		xfs_qm_dqget_next(struct xfs_mount *mp, xfs_dqid_t id,
+					uint type, struct xfs_dquot **dqpp);
 extern void		xfs_qm_dqput(xfs_dquot_t *);
 
 extern void		xfs_dqlock2(struct xfs_dquot *, struct xfs_dquot *);
diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
index 2975a822e9f0..e3129b280423 100644
--- a/fs/xfs/xfs_qm.h
+++ b/fs/xfs/xfs_qm.h
@@ -170,8 +170,10 @@ extern void		xfs_qm_dqrele_all_inodes(struct xfs_mount *, uint);
 
 /* quota ops */
 extern int		xfs_qm_scall_trunc_qfiles(struct xfs_mount *, uint);
-extern int		xfs_qm_scall_getquota(struct xfs_mount *, xfs_dqid_t *,
-					uint, struct qc_dqblk *, uint);
+extern int		xfs_qm_scall_getquota(struct xfs_mount *, xfs_dqid_t,
+					uint, struct qc_dqblk *);
+extern int		xfs_qm_scall_getquota_next(struct xfs_mount *,
+					xfs_dqid_t *, uint, struct qc_dqblk *);
 extern int		xfs_qm_scall_setqlim(struct xfs_mount *, xfs_dqid_t, uint,
 					struct qc_dqblk *);
 extern int		xfs_qm_scall_quotaon(struct xfs_mount *, uint);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 9cb5c381b01c..0234cfc4d445 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -622,39 +622,14 @@ xfs_qm_log_quotaoff(
 	return error;
 }
 
-
-int
-xfs_qm_scall_getquota(
+/* Fill out the quota context. */
+static void
+xfs_qm_scall_getquota_fill_qc(
 	struct xfs_mount	*mp,
-	xfs_dqid_t		*id,
 	uint			type,
-	struct qc_dqblk		*dst,
-	uint			dqget_flags)
+	const struct xfs_dquot	*dqp,
+	struct qc_dqblk		*dst)
 {
-	struct xfs_dquot	*dqp;
-	int			error;
-
-	/*
-	 * Try to get the dquot. We don't want it allocated on disk, so
-	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
-	 * exist, we'll get ENOENT back.
-	 */
-	error = xfs_qm_dqget(mp, NULL, *id, type, dqget_flags, &dqp);
-	if (error)
-		return error;
-
-	/*
-	 * If everything's NULL, this dquot doesn't quite exist as far as
-	 * our utility programs are concerned.
-	 */
-	if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
-		error = -ENOENT;
-		goto out_put;
-	}
-
-	/* Fill in the ID we actually read from disk */
-	*id = be32_to_cpu(dqp->q_core.d_id);
-
 	memset(dst, 0, sizeof(*dst));
 	dst->d_spc_hardlimit =
 		XFS_FSB_TO_B(mp, be64_to_cpu(dqp->q_core.d_blk_hardlimit));
@@ -696,7 +671,7 @@ xfs_qm_scall_getquota(
 	if (((XFS_IS_UQUOTA_ENFORCED(mp) && type == XFS_DQ_USER) ||
 	     (XFS_IS_GQUOTA_ENFORCED(mp) && type == XFS_DQ_GROUP) ||
 	     (XFS_IS_PQUOTA_ENFORCED(mp) && type == XFS_DQ_PROJ)) &&
-	    *id != 0) {
+	    dqp->q_core.d_id != 0) {
 		if ((dst->d_space > dst->d_spc_softlimit) &&
 		    (dst->d_spc_softlimit > 0)) {
 			ASSERT(dst->d_spc_timer != 0);
@@ -707,11 +682,70 @@ xfs_qm_scall_getquota(
 		}
 	}
 #endif
+}
+
+/* Return the quota information for the dquot matching id. */
+int
+xfs_qm_scall_getquota(
+	struct xfs_mount	*mp,
+	xfs_dqid_t		id,
+	uint			type,
+	struct qc_dqblk		*dst)
+{
+	struct xfs_dquot	*dqp;
+	int			error;
+
+	/*
+	 * Try to get the dquot. We don't want it allocated on disk, so
+	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
+	 * exist, we'll get ENOENT back.
+	 */
+	error = xfs_qm_dqget(mp, NULL, id, type, 0, &dqp);
+	if (error)
+		return error;
+
+	/*
+	 * If everything's NULL, this dquot doesn't quite exist as far as
+	 * our utility programs are concerned.
+	 */
+	if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) {
+		error = -ENOENT;
+		goto out_put;
+	}
+
+	xfs_qm_scall_getquota_fill_qc(mp, type, dqp, dst);
+
 out_put:
 	xfs_qm_dqput(dqp);
 	return error;
 }
 
+/*
+ * Return the quota information for the first initialized dquot whose id
+ * is at least as high as id.
+ */
+int
+xfs_qm_scall_getquota_next(
+	struct xfs_mount	*mp,
+	xfs_dqid_t		*id,
+	uint			type,
+	struct qc_dqblk		*dst)
+{
+	struct xfs_dquot	*dqp;
+	int			error;
+
+	error = xfs_qm_dqget_next(mp, *id, type, &dqp);
+	if (error)
+		return error;
+
+	/* Fill in the ID we actually read from disk */
+	*id = be32_to_cpu(dqp->q_core.d_id);
+
+	xfs_qm_scall_getquota_fill_qc(mp, type, dqp, dst);
+
+	xfs_qm_dqput(dqp);
+	return error;
+}
 
 STATIC int
 xfs_dqrele_inode(
diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index a65108594a07..c93fc913dffb 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -239,8 +239,7 @@ xfs_fs_get_dqblk(
 		return -ESRCH;
 
 	id = from_kqid(&init_user_ns, qid);
-	return xfs_qm_scall_getquota(mp, &id,
-				      xfs_quota_type(qid.type), qdq, 0);
+	return xfs_qm_scall_getquota(mp, id, xfs_quota_type(qid.type), qdq);
 }
 
 /* Return quota info for active quota >= this qid */
@@ -260,9 +259,8 @@ xfs_fs_get_nextdqblk(
 		return -ESRCH;
 
 	id = from_kqid(&init_user_ns, *qid);
-	ret = xfs_qm_scall_getquota(mp, &id,
-				    xfs_quota_type(qid->type), qdq,
-				    XFS_QMOPT_DQNEXT);
+	ret = xfs_qm_scall_getquota_next(mp, &id, xfs_quota_type(qid->type),
+			qdq);
 	if (ret)
 		return ret;
 

[PATCH 02/13] xfs: refactor dquot cache handling

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Delegate the dquot cache handling (radix tree lookup and insertion) to
separate helper functions so that we can continue to simplify the body
of dqget.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot.c |  112 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 78 insertions(+), 34 deletions(-)


diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 79a9df691d6c..43b0b32effd4 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -678,6 +678,81 @@ xfs_dq_get_next_id(
 	return error;
 }
 
+/*
+ * Look up the dquot in the in-core cache.  If found, the dquot is returned
+ * locked and ready to go.
+ */
+static struct xfs_dquot *
+xfs_qm_dqget_cache_lookup(
+	struct xfs_mount	*mp,
+	struct xfs_quotainfo	*qi,
+	struct radix_tree_root	*tree,
+	xfs_dqid_t		id)
+{
+	struct xfs_dquot	*dqp;
+
+restart:
+	mutex_lock(&qi->qi_tree_lock);
+	dqp = radix_tree_lookup(tree, id);
+	if (!dqp) {
+		mutex_unlock(&qi->qi_tree_lock);
+		XFS_STATS_INC(mp, xs_qm_dqcachemisses);
+		return NULL;
+	}
+
+	xfs_dqlock(dqp);
+	if (dqp->dq_flags & XFS_DQ_FREEING) {
+		xfs_dqunlock(dqp);
+		mutex_unlock(&qi->qi_tree_lock);
+		trace_xfs_dqget_freeing(dqp);
+		delay(1);
+		goto restart;
+	}
+
+	dqp->q_nrefs++;
+	mutex_unlock(&qi->qi_tree_lock);
+
+	trace_xfs_dqget_hit(dqp);
+	XFS_STATS_INC(mp, xs_qm_dqcachehits);
+	return dqp;
+}
+
+/*
+ * Try to insert a new dquot into the in-core cache.  If an error occurs the
+ * caller should throw away the dquot and start over.  Otherwise, the dquot
+ * is returned locked (and held by the cache) as if there had been a cache
+ * hit.
+ */
+static int
+xfs_qm_dqget_cache_insert(
+	struct xfs_mount	*mp,
+	struct xfs_quotainfo	*qi,
+	struct radix_tree_root	*tree,
+	xfs_dqid_t		id,
+	struct xfs_dquot	*dqp)
+{
+	int			error;
+
+	mutex_lock(&qi->qi_tree_lock);
+	error = radix_tree_insert(tree, id, dqp);
+	if (unlikely(error)) {
+		/* Duplicate found!  Caller must try again. */
+		WARN_ON(error != -EEXIST);
+		mutex_unlock(&qi->qi_tree_lock);
+		trace_xfs_dqget_dup(dqp);
+		return error;
+	}
+
+	/* Return a locked dquot to the caller, with a reference taken. */
+	xfs_dqlock(dqp);
+	dqp->q_nrefs = 1;
+
+	qi->qi_dquots++;
+	mutex_unlock(&qi->qi_tree_lock);
+
+	return 0;
+}
+
 /*
  * Given the file system, inode OR id, and type (UDQUOT/GDQUOT), return a
  * a locked dquot, doing an allocation (if requested) as needed.
@@ -716,28 +791,11 @@ xfs_qm_dqget(
 	}
 
 restart:
-	mutex_lock(&qi->qi_tree_lock);
-	dqp = radix_tree_lookup(tree, id);
+	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
 	if (dqp) {
-		xfs_dqlock(dqp);
-		if (dqp->dq_flags & XFS_DQ_FREEING) {
-			xfs_dqunlock(dqp);
-			mutex_unlock(&qi->qi_tree_lock);
-			trace_xfs_dqget_freeing(dqp);
-			delay(1);
-			goto restart;
-		}
-
-		dqp->q_nrefs++;
-		mutex_unlock(&qi->qi_tree_lock);
-
-		trace_xfs_dqget_hit(dqp);
-		XFS_STATS_INC(mp, xs_qm_dqcachehits);
 		*O_dqpp = dqp;
 		return 0;
 	}
-	mutex_unlock(&qi->qi_tree_lock);
-	XFS_STATS_INC(mp, xs_qm_dqcachemisses);
 
 	/*
 	 * Dquot cache miss. We don't want to keep the inode lock across
@@ -779,31 +837,17 @@ xfs_qm_dqget(
 		}
 	}
 
-	mutex_lock(&qi->qi_tree_lock);
-	error = radix_tree_insert(tree, id, dqp);
-	if (unlikely(error)) {
-		WARN_ON(error != -EEXIST);
-
+	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
+	if (error) {
 		/*
 		 * Duplicate found. Just throw away the new dquot and start
 		 * over.
 		 */
-		mutex_unlock(&qi->qi_tree_lock);
-		trace_xfs_dqget_dup(dqp);
 		xfs_qm_dqdestroy(dqp);
 		XFS_STATS_INC(mp, xs_qm_dquot_dups);
 		goto restart;
 	}
 
-	/*
-	 * We return a locked dquot to the caller, with a reference taken
-	 */
-	xfs_dqlock(dqp);
-	dqp->q_nrefs = 1;
-
-	qi->qi_dquots++;
-	mutex_unlock(&qi->qi_tree_lock);
-
  dqret:
 	ASSERT((ip == NULL) || xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	trace_xfs_dqget_miss(dqp);

[PATCH 03/13] xfs: delegate dqget input checks to helper function

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Move the dqget input checks to a separate function in preparation for
splitting up the dqget functionality.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot.c |   40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)


diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 43b0b32effd4..028d0d513cdf 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -753,6 +753,34 @@ xfs_qm_dqget_cache_insert(
 	return 0;
 }
 
+/* Check our input parameters. */
+static int
+xfs_qm_dqget_checks(
+	struct xfs_mount	*mp,
+	uint			type)
+{
+	if (WARN_ON_ONCE(!XFS_IS_QUOTA_RUNNING(mp)))
+		return -ESRCH;
+
+	switch (type) {
+	case XFS_DQ_USER:
+		if (!XFS_IS_UQUOTA_ON(mp))
+			return -ESRCH;
+		return 0;
+	case XFS_DQ_GROUP:
+		if (!XFS_IS_GQUOTA_ON(mp))
+			return -ESRCH;
+		return 0;
+	case XFS_DQ_PROJ:
+		if (!XFS_IS_PQUOTA_ON(mp))
+			return -ESRCH;
+		return 0;
+	default:
+		WARN_ON_ONCE(0);
+		return -EINVAL;
+	}
+}
+
 /*
  * Given the file system, inode OR id, and type (UDQUOT/GDQUOT), return a
  * a locked dquot, doing an allocation (if requested) as needed.
@@ -775,16 +803,10 @@ xfs_qm_dqget(
 	struct xfs_dquot	*dqp;
 	int			error;
 
-	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
-	if ((! XFS_IS_UQUOTA_ON(mp) && type == XFS_DQ_USER) ||
-	    (! XFS_IS_PQUOTA_ON(mp) && type == XFS_DQ_PROJ) ||
-	    (! XFS_IS_GQUOTA_ON(mp) && type == XFS_DQ_GROUP)) {
-		return -ESRCH;
-	}
+	error = xfs_qm_dqget_checks(mp, type);
+	if (error)
+		return error;
 
-	ASSERT(type == XFS_DQ_USER ||
-	       type == XFS_DQ_PROJ ||
-	       type == XFS_DQ_GROUP);
 	if (ip) {
 		ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 		ASSERT(xfs_inode_dquot(ip, type) == NULL);

[PATCH 04/13] xfs: remove unnecessary xfs_qm_dqattach parameter

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The flags argument is always zero, get rid of it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr.c |    4 ++--
 fs/xfs/xfs_bmap_util.c   |    6 +++---
 fs/xfs/xfs_inode.c       |   10 +++++-----
 fs/xfs/xfs_iomap.c       |    4 ++--
 fs/xfs/xfs_iops.c        |    2 +-
 fs/xfs/xfs_qm.c          |    7 +++----
 fs/xfs/xfs_quota.h       |    4 ++--
 fs/xfs/xfs_reflink.c     |    2 +-
 8 files changed, 19 insertions(+), 20 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 35a124400d60..c3d02a66d39d 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -236,7 +236,7 @@ xfs_attr_set(
 	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
 	args.total = xfs_attr_calc_size(&args, &local);
 
-	error = xfs_qm_dqattach(dp, 0);
+	error = xfs_qm_dqattach(dp);
 	if (error)
 		return error;
 
@@ -427,7 +427,7 @@ xfs_attr_remove(
 	 */
 	args.op_flags = XFS_DA_OP_OKNOENT;
 
-	error = xfs_qm_dqattach(dp, 0);
+	error = xfs_qm_dqattach(dp);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 8cd8c412f52d..65b2cc2a69df 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -848,7 +848,7 @@ xfs_free_eofblocks(
 		/*
 		 * Attach the dquots to the inode up front.
 		 */
-		error = xfs_qm_dqattach(ip, 0);
+		error = xfs_qm_dqattach(ip);
 		if (error)
 			return error;
 
@@ -918,7 +918,7 @@ xfs_alloc_file_space(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	error = xfs_qm_dqattach(ip, 0);
+	error = xfs_qm_dqattach(ip);
 	if (error)
 		return error;
 
@@ -1169,7 +1169,7 @@ xfs_free_file_space(
 
 	trace_xfs_free_file_space(ip);
 
-	error = xfs_qm_dqattach(ip, 0);
+	error = xfs_qm_dqattach(ip);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 2b70c8b4cee2..1da986330e73 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1411,11 +1411,11 @@ xfs_link(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	error = xfs_qm_dqattach(sip, 0);
+	error = xfs_qm_dqattach(sip);
 	if (error)
 		goto std_return;
 
-	error = xfs_qm_dqattach(tdp, 0);
+	error = xfs_qm_dqattach(tdp);
 	if (error)
 		goto std_return;
 
@@ -1911,7 +1911,7 @@ xfs_inactive(
 	     ip->i_d.di_nextents > 0 || ip->i_delayed_blks > 0))
 		truncate = 1;
 
-	error = xfs_qm_dqattach(ip, 0);
+	error = xfs_qm_dqattach(ip);
 	if (error)
 		return;
 
@@ -2574,11 +2574,11 @@ xfs_remove(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	error = xfs_qm_dqattach(dp, 0);
+	error = xfs_qm_dqattach(dp);
 	if (error)
 		goto std_return;
 
-	error = xfs_qm_dqattach(ip, 0);
+	error = xfs_qm_dqattach(ip);
 	if (error)
 		goto std_return;
 
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 046469fcc1b8..58b04eace2bc 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -224,7 +224,7 @@ xfs_iomap_write_direct(
 	 * necessary and move on to transaction setup.
 	 */
 	xfs_iunlock(ip, lockmode);
-	error = xfs_qm_dqattach(ip, 0);
+	error = xfs_qm_dqattach(ip);
 	if (error)
 		return error;
 
@@ -692,7 +692,7 @@ xfs_iomap_write_allocate(
 	/*
 	 * Make sure that the dquots are there.
 	 */
-	error = xfs_qm_dqattach(ip, 0);
+	error = xfs_qm_dqattach(ip);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index a3ed3c811dfa..5afe3c2234b3 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -855,7 +855,7 @@ xfs_setattr_size(
 	/*
 	 * Make sure that the dquots are attached to the inode.
 	 */
-	error = xfs_qm_dqattach(ip, 0);
+	error = xfs_qm_dqattach(ip);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index ec39ae274c78..d919b8554bf0 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -381,8 +381,7 @@ xfs_qm_dqattach_locked(
 
 int
 xfs_qm_dqattach(
-	struct xfs_inode	*ip,
-	uint			flags)
+	struct xfs_inode	*ip)
 {
 	int			error;
 
@@ -390,7 +389,7 @@ xfs_qm_dqattach(
 		return 0;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = xfs_qm_dqattach_locked(ip, flags);
+	error = xfs_qm_dqattach_locked(ip, 0);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 
 	return error;
@@ -1933,7 +1932,7 @@ xfs_qm_vop_rename_dqattach(
 		 */
 		if (i == 0 || ip != i_tab[i-1]) {
 			if (XFS_NOT_DQATTACHED(mp, ip)) {
-				error = xfs_qm_dqattach(ip, 0);
+				error = xfs_qm_dqattach(ip);
 				if (error)
 					return error;
 			}
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index ce6506adab7b..a4d392240cf4 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -90,7 +90,7 @@ extern struct xfs_dquot *xfs_qm_vop_chown(struct xfs_trans *,
 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 *, uint);
+extern int xfs_qm_dqattach(struct xfs_inode *);
 extern int xfs_qm_dqattach_locked(struct xfs_inode *, uint);
 extern void xfs_qm_dqdetach(struct xfs_inode *);
 extern void xfs_qm_dqrele(struct xfs_dquot *);
@@ -132,7 +132,7 @@ static inline int xfs_trans_reserve_quota_bydquots(struct xfs_trans *tp,
 #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, fl)						(0)
+#define xfs_qm_dqattach(ip)						(0)
 #define xfs_qm_dqattach_locked(ip, fl)					(0)
 #define xfs_qm_dqdetach(ip)
 #define xfs_qm_dqrele(d)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cdbd342a5249..fd8e6afb8e1c 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1359,7 +1359,7 @@ xfs_reflink_remap_range(
 		goto out_unlock;
 
 	/* Attach dquots to dest inode before changing block map */
-	ret = xfs_qm_dqattach(dest, 0);
+	ret = xfs_qm_dqattach(dest);
 	if (ret)
 		goto out_unlock;
 

[PATCH 05/13] xfs: split out dqget for inodes from regular dqget

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

There are two uses of dqget here -- one is to return the dquot for a
given type and id, and the other is to return the dquot for a given type
and inode.  Those are two separate things, so split them into two
smaller functions.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot.c       |  148 ++++++++++++++++++++++++++++++++--------------
 fs/xfs/xfs_dquot.h       |   10 ++-
 fs/xfs/xfs_iomap.c       |    2 -
 fs/xfs/xfs_qm.c          |   39 +++++-------
 fs/xfs/xfs_qm_bhv.c      |    2 -
 fs/xfs/xfs_qm_syscalls.c |    4 +
 fs/xfs/xfs_quota.h       |    2 -
 fs/xfs/xfs_reflink.c     |    4 +
 8 files changed, 134 insertions(+), 77 deletions(-)


diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 028d0d513cdf..e3e51f6e2880 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -782,24 +782,19 @@ xfs_qm_dqget_checks(
 }
 
 /*
- * Given the file system, inode OR id, and type (UDQUOT/GDQUOT), return a
- * a locked dquot, doing an allocation (if requested) as needed.
- * When both an inode and an id are given, the inode's id takes precedence.
- * That is, if the id changes while we don't hold the ilock inside this
- * function, the new dquot is returned, not necessarily the one requested
- * in the id argument.
+ * Given the file system, id, and type (UDQUOT/GDQUOT), return a a locked
+ * dquot, doing an allocation (if requested) as needed.
  */
 int
 xfs_qm_dqget(
-	xfs_mount_t	*mp,
-	xfs_inode_t	*ip,	  /* locked inode (optional) */
-	xfs_dqid_t	id,	  /* uid/projid/gid depending on type */
-	uint		type,	  /* XFS_DQ_USER/XFS_DQ_PROJ/XFS_DQ_GROUP */
-	uint		flags,	  /* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
-	xfs_dquot_t	**O_dqpp) /* OUT : locked incore dquot */
+	struct xfs_mount	*mp,
+	xfs_dqid_t		id,
+	uint			type,
+	uint			flags,	  /* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
+	struct xfs_dquot	**O_dqpp)
 {
 	struct xfs_quotainfo	*qi = mp->m_quotainfo;
-	struct radix_tree_root *tree = xfs_dquot_tree(qi, type);
+	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
 	struct xfs_dquot	*dqp;
 	int			error;
 
@@ -807,10 +802,82 @@ xfs_qm_dqget(
 	if (error)
 		return error;
 
-	if (ip) {
-		ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
-		ASSERT(xfs_inode_dquot(ip, type) == NULL);
+restart:
+	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
+	if (dqp) {
+		*O_dqpp = dqp;
+		return 0;
+	}
+
+	error = xfs_qm_dqread(mp, id, type, flags, &dqp);
+	if (error)
+		return error;
+
+	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
+	if (error) {
+		/*
+		 * Duplicate found. Just throw away the new dquot and start
+		 * over.
+		 */
+		xfs_qm_dqdestroy(dqp);
+		XFS_STATS_INC(mp, xs_qm_dquot_dups);
+		goto restart;
+	}
+
+	trace_xfs_dqget_miss(dqp);
+	*O_dqpp = dqp;
+	return 0;
+}
+
+/* Return the quota id for a given inode and type. */
+xfs_dqid_t
+xfs_qm_id_for_quotatype(
+	struct xfs_inode	*ip,
+	uint			type)
+{
+	switch (type) {
+	case XFS_DQ_USER:
+		return ip->i_d.di_uid;
+	case XFS_DQ_GROUP:
+		return ip->i_d.di_gid;
+	case XFS_DQ_PROJ:
+		return xfs_get_projid(ip);
 	}
+	ASSERT(0);
+	return 0;
+}
+
+/*
+ * Return the dquot for a given inode and type.  If @can_alloc is true, then
+ * allocate blocks if needed.  The inode's ILOCK must be held and it must not
+ * have already had an inode attached.
+ */
+int
+xfs_qm_dqget_inode(
+	struct xfs_inode	*ip,
+	uint			type,
+	bool			can_alloc,
+	struct xfs_dquot	**O_dqpp)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_quotainfo	*qi = mp->m_quotainfo;
+	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
+	struct xfs_dquot	*dqp;
+	xfs_dqid_t		id;
+	uint			flags = 0;
+	int			error;
+
+	error = xfs_qm_dqget_checks(mp, type);
+	if (error)
+		return error;
+
+	if (can_alloc)
+		flags |= XFS_QMOPT_DQALLOC;
+
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_inode_dquot(ip, type) == NULL);
+
+	id = xfs_qm_id_for_quotatype(ip, type);
 
 restart:
 	dqp = xfs_qm_dqget_cache_lookup(mp, qi, tree, id);
@@ -826,37 +893,30 @@ xfs_qm_dqget(
 	 * lock here means dealing with a chown that can happen before
 	 * we re-acquire the lock.
 	 */
-	if (ip)
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	error = xfs_qm_dqread(mp, id, type, flags, &dqp);
-
-	if (ip)
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	if (error)
 		return error;
 
-	if (ip) {
-		/*
-		 * A dquot could be attached to this inode by now, since
-		 * we had dropped the ilock.
-		 */
-		if (xfs_this_quota_on(mp, type)) {
-			struct xfs_dquot	*dqp1;
-
-			dqp1 = xfs_inode_dquot(ip, type);
-			if (dqp1) {
-				xfs_qm_dqdestroy(dqp);
-				dqp = dqp1;
-				xfs_dqlock(dqp);
-				goto dqret;
-			}
-		} else {
-			/* inode stays locked on return */
+	/*
+	 * A dquot could be attached to this inode by now, since we had
+	 * dropped the ilock.
+	 */
+	if (xfs_this_quota_on(mp, type)) {
+		struct xfs_dquot	*dqp1;
+
+		dqp1 = xfs_inode_dquot(ip, type);
+		if (dqp1) {
 			xfs_qm_dqdestroy(dqp);
-			return -ESRCH;
+			dqp = dqp1;
+			xfs_dqlock(dqp);
+			goto dqret;
 		}
+	} else {
+		/* inode stays locked on return */
+		xfs_qm_dqdestroy(dqp);
+		return -ESRCH;
 	}
 
 	error = xfs_qm_dqget_cache_insert(mp, qi, tree, id, dqp);
@@ -870,8 +930,8 @@ xfs_qm_dqget(
 		goto restart;
 	}
 
- dqret:
-	ASSERT((ip == NULL) || xfs_isilocked(ip, XFS_ILOCK_EXCL));
+dqret:
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	trace_xfs_dqget_miss(dqp);
 	*O_dqpp = dqp;
 	return 0;
@@ -893,7 +953,7 @@ xfs_qm_dqget_next(
 
 	*dqpp = NULL;
 	for (; !error; error = xfs_dq_get_next_id(mp, type, &id)) {
-		error = xfs_qm_dqget(mp, NULL, id, type, 0, &dqp);
+		error = xfs_qm_dqget(mp, id, type, 0, &dqp);
 		if (error == -ENOENT)
 			continue;
 		else if (error != 0)
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 303e71dfcf70..8a5b30e952b0 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -169,8 +169,14 @@ extern void		xfs_qm_adjust_dqtimers(xfs_mount_t *,
 					xfs_disk_dquot_t *);
 extern void		xfs_qm_adjust_dqlimits(struct xfs_mount *,
 					       struct xfs_dquot *);
-extern int		xfs_qm_dqget(xfs_mount_t *, xfs_inode_t *,
-					xfs_dqid_t, uint, uint, xfs_dquot_t **);
+extern xfs_dqid_t	xfs_qm_id_for_quotatype(struct xfs_inode *ip,
+					uint type);
+extern int		xfs_qm_dqget(struct xfs_mount *mp, xfs_dqid_t id,
+					uint type, uint flags,
+					struct xfs_dquot **dqpp);
+extern int		xfs_qm_dqget_inode(struct xfs_inode *ip, uint type,
+					bool can_alloc,
+					struct xfs_dquot **dqpp);
 extern int		xfs_qm_dqget_next(struct xfs_mount *mp, xfs_dqid_t id,
 					uint type, struct xfs_dquot **dqpp);
 extern void		xfs_qm_dqput(xfs_dquot_t *);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 58b04eace2bc..7f8fc83c1555 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -576,7 +576,7 @@ xfs_file_iomap_begin_delay(
 		goto done;
 	}
 
-	error = xfs_qm_dqattach_locked(ip, 0);
+	error = xfs_qm_dqattach_locked(ip, false);
 	if (error)
 		goto out_unlock;
 
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index d919b8554bf0..25c0718e1993 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -265,7 +265,7 @@ xfs_qm_dqattach_one(
 	xfs_inode_t	*ip,
 	xfs_dqid_t	id,
 	uint		type,
-	uint		doalloc,
+	bool		doalloc,
 	xfs_dquot_t	**IO_idqpp)
 {
 	xfs_dquot_t	*dqp;
@@ -291,7 +291,7 @@ xfs_qm_dqattach_one(
 	 * exist on disk and we didn't ask it to allocate; ESRCH if quotas got
 	 * turned off suddenly.
 	 */
-	error = xfs_qm_dqget(ip->i_mount, ip, id, type, doalloc, &dqp);
+	error = xfs_qm_dqget_inode(ip, type, doalloc, &dqp);
 	if (error)
 		return error;
 
@@ -333,7 +333,7 @@ xfs_qm_need_dqattach(
 int
 xfs_qm_dqattach_locked(
 	xfs_inode_t	*ip,
-	uint		flags)
+	bool		doalloc)
 {
 	xfs_mount_t	*mp = ip->i_mount;
 	int		error = 0;
@@ -345,8 +345,7 @@ xfs_qm_dqattach_locked(
 
 	if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
 		error = xfs_qm_dqattach_one(ip, ip->i_d.di_uid, XFS_DQ_USER,
-						flags & XFS_QMOPT_DQALLOC,
-						&ip->i_udquot);
+				doalloc, &ip->i_udquot);
 		if (error)
 			goto done;
 		ASSERT(ip->i_udquot);
@@ -354,8 +353,7 @@ xfs_qm_dqattach_locked(
 
 	if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
 		error = xfs_qm_dqattach_one(ip, ip->i_d.di_gid, XFS_DQ_GROUP,
-						flags & XFS_QMOPT_DQALLOC,
-						&ip->i_gdquot);
+				doalloc, &ip->i_gdquot);
 		if (error)
 			goto done;
 		ASSERT(ip->i_gdquot);
@@ -363,8 +361,7 @@ xfs_qm_dqattach_locked(
 
 	if (XFS_IS_PQUOTA_ON(mp) && !ip->i_pdquot) {
 		error = xfs_qm_dqattach_one(ip, xfs_get_projid(ip), XFS_DQ_PROJ,
-						flags & XFS_QMOPT_DQALLOC,
-						&ip->i_pdquot);
+				doalloc, &ip->i_pdquot);
 		if (error)
 			goto done;
 		ASSERT(ip->i_pdquot);
@@ -389,7 +386,7 @@ xfs_qm_dqattach(
 		return 0;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = xfs_qm_dqattach_locked(ip, 0);
+	error = xfs_qm_dqattach_locked(ip, false);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 
 	return error;
@@ -1074,7 +1071,7 @@ xfs_qm_quotacheck_dqadjust(
 	struct xfs_dquot	*dqp;
 	int			error;
 
-	error = xfs_qm_dqget(mp, ip, id, type, XFS_QMOPT_DQALLOC, &dqp);
+	error = xfs_qm_dqget_inode(ip, type, true, &dqp);
 	if (error) {
 		/*
 		 * Shouldn't be able to turn off quotas here.
@@ -1674,7 +1671,7 @@ xfs_qm_vop_dqalloc(
 	 * if necessary. The dquot(s) will not be locked.
 	 */
 	if (XFS_NOT_DQATTACHED(mp, ip)) {
-		error = xfs_qm_dqattach_locked(ip, XFS_QMOPT_DQALLOC);
+		error = xfs_qm_dqattach_locked(ip, true);
 		if (error) {
 			xfs_iunlock(ip, lockflags);
 			return error;
@@ -1693,10 +1690,8 @@ xfs_qm_vop_dqalloc(
 			 * holding ilock.
 			 */
 			xfs_iunlock(ip, lockflags);
-			error = xfs_qm_dqget(mp, NULL, uid,
-						 XFS_DQ_USER,
-						 XFS_QMOPT_DQALLOC,
-						 &uq);
+			error = xfs_qm_dqget(mp, uid, XFS_DQ_USER,
+					XFS_QMOPT_DQALLOC, &uq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				return error;
@@ -1719,10 +1714,8 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
 		if (ip->i_d.di_gid != gid) {
 			xfs_iunlock(ip, lockflags);
-			error = xfs_qm_dqget(mp, NULL, gid,
-						 XFS_DQ_GROUP,
-						 XFS_QMOPT_DQALLOC,
-						 &gq);
+			error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP,
+					XFS_QMOPT_DQALLOC, &gq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				goto error_rele;
@@ -1738,10 +1731,8 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
 		if (xfs_get_projid(ip) != prid) {
 			xfs_iunlock(ip, lockflags);
-			error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)prid,
-						 XFS_DQ_PROJ,
-						 XFS_QMOPT_DQALLOC,
-						 &pq);
+			error = xfs_qm_dqget(mp, (xfs_dqid_t)prid, XFS_DQ_PROJ,
+					XFS_QMOPT_DQALLOC, &pq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				goto error_rele;
diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index 2be6d2735ca9..531e8224dcb6 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -72,7 +72,7 @@ xfs_qm_statvfs(
 	xfs_mount_t		*mp = ip->i_mount;
 	xfs_dquot_t		*dqp;
 
-	if (!xfs_qm_dqget(mp, NULL, xfs_get_projid(ip), XFS_DQ_PROJ, 0, &dqp)) {
+	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, 0, &dqp)) {
 		xfs_fill_statvfs_from_dquot(statp, dqp);
 		xfs_qm_dqput(dqp);
 	}
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 0234cfc4d445..b9243f554697 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -425,7 +425,7 @@ xfs_qm_scall_setqlim(
 	 * a reference to the dquot, so it's safe to do this unlock/lock without
 	 * it being reclaimed in the mean time.
 	 */
-	error = xfs_qm_dqget(mp, NULL, id, type, XFS_QMOPT_DQALLOC, &dqp);
+	error = xfs_qm_dqget(mp, id, type, XFS_QMOPT_DQALLOC, &dqp);
 	if (error) {
 		ASSERT(error != -ENOENT);
 		goto out_unlock;
@@ -700,7 +700,7 @@ xfs_qm_scall_getquota(
 	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
 	 * exist, we'll get ENOENT back.
 	 */
-	error = xfs_qm_dqget(mp, NULL, id, type, 0, &dqp);
+	error = xfs_qm_dqget(mp, id, type, 0, &dqp);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index a4d392240cf4..1c79ebbe5236 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -91,7 +91,7 @@ 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 *, uint);
+extern int xfs_qm_dqattach_locked(struct xfs_inode *ip, bool doalloc);
 extern void xfs_qm_dqdetach(struct xfs_inode *);
 extern void xfs_qm_dqrele(struct xfs_dquot *);
 extern void xfs_qm_statvfs(struct xfs_inode *, struct kstatfs *);
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index fd8e6afb8e1c..d7f251bd7acc 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -305,7 +305,7 @@ xfs_reflink_reserve_cow(
 	 * Fork all the shared blocks from our write offset until the end of
 	 * the extent.
 	 */
-	error = xfs_qm_dqattach_locked(ip, 0);
+	error = xfs_qm_dqattach_locked(ip, false);
 	if (error)
 		return error;
 
@@ -431,7 +431,7 @@ xfs_reflink_allocate_cow(
 		if (error)
 			return error;
 
-		error = xfs_qm_dqattach_locked(ip, 0);
+		error = xfs_qm_dqattach_locked(ip, false);
 		if (error)
 			goto out;
 		goto retry;

[PATCH 06/13] xfs: fetch dquots directly during quotacheck

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Quotacheck only runs during mount, which means that there are no other
processes in the system that could be doing chown or chproj.  Therefore
there's no potential for racing to attach dquots to the inode so we can
drop all the ILOCK and race detection bits from quotacheck.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm.c |   28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)


diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 25c0718e1993..fb14954610ee 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1062,16 +1062,17 @@ xfs_qm_dqiterate(
 STATIC int
 xfs_qm_quotacheck_dqadjust(
 	struct xfs_inode	*ip,
-	xfs_dqid_t		id,
 	uint			type,
 	xfs_qcnt_t		nblks,
 	xfs_qcnt_t		rtblks)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_dquot	*dqp;
+	xfs_dqid_t		id;
 	int			error;
 
-	error = xfs_qm_dqget_inode(ip, type, true, &dqp);
+	id = xfs_qm_id_for_quotatype(ip, type);
+	error = xfs_qm_dqget(mp, id, type, XFS_QMOPT_DQALLOC, &dqp);
 	if (error) {
 		/*
 		 * Shouldn't be able to turn off quotas here.
@@ -1144,13 +1145,10 @@ xfs_qm_dqusage_adjust(
 	}
 
 	/*
-	 * We don't _need_ to take the ilock EXCL. However, the xfs_qm_dqget
-	 * interface expects the inode to be exclusively locked because that's
-	 * the case in all other instances. It's OK that we do this because
-	 * quotacheck is done only at mount time.
+	 * We don't _need_ to take the ilock EXCL here because quotacheck runs
+	 * at mount time and therefore nobody will be racing chown/chproj.
 	 */
-	error = xfs_iget(mp, NULL, ino, XFS_IGET_DONTCACHE, XFS_ILOCK_EXCL,
-			 &ip);
+	error = xfs_iget(mp, NULL, ino, XFS_IGET_DONTCACHE, 0, &ip);
 	if (error) {
 		*res = BULKSTAT_RV_NOTHING;
 		return error;
@@ -1185,33 +1183,31 @@ xfs_qm_dqusage_adjust(
 	 * and quotaoffs don't race. (Quotachecks happen at mount time only).
 	 */
 	if (XFS_IS_UQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, ip->i_d.di_uid,
-						   XFS_DQ_USER, nblks, rtblks);
+		error = xfs_qm_quotacheck_dqadjust(ip, XFS_DQ_USER, nblks,
+				rtblks);
 		if (error)
 			goto error0;
 	}
 
 	if (XFS_IS_GQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, ip->i_d.di_gid,
-						   XFS_DQ_GROUP, nblks, rtblks);
+		error = xfs_qm_quotacheck_dqadjust(ip, XFS_DQ_GROUP, nblks,
+				rtblks);
 		if (error)
 			goto error0;
 	}
 
 	if (XFS_IS_PQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, xfs_get_projid(ip),
-						   XFS_DQ_PROJ, nblks, rtblks);
+		error = xfs_qm_quotacheck_dqadjust(ip, XFS_DQ_PROJ, nblks,
+				rtblks);
 		if (error)
 			goto error0;
 	}
 
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	IRELE(ip);
 	*res = BULKSTAT_RV_DIDONE;
 	return 0;
 
 error0:
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	IRELE(ip);
 	*res = BULKSTAT_RV_GIVEUP;
 	return error;

[PATCH 07/13] xfs: refactor incore dquot initialization functions

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Create two incore dquot initialization functions that will help us to
disentangle dqget and dqread.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot.c |   81 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 30 deletions(-)


diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index e3e51f6e2880..f1dc62ca54a7 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -491,26 +491,14 @@ xfs_qm_dqtobp(
 	return 0;
 }
 
-
-/*
- * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
- * and release the buffer immediately.
- *
- * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
- */
-int
-xfs_qm_dqread(
+/* Allocate and initialize everything we need for an incore dquot. */
+STATIC struct xfs_dquot *
+xfs_dquot_alloc(
 	struct xfs_mount	*mp,
 	xfs_dqid_t		id,
-	uint			type,
-	uint			flags,
-	struct xfs_dquot	**O_dqpp)
+	uint			type)
 {
 	struct xfs_dquot	*dqp;
-	struct xfs_disk_dquot	*ddqp;
-	struct xfs_buf		*bp;
-	struct xfs_trans	*tp = NULL;
-	int			error;
 
 	dqp = kmem_zone_zalloc(xfs_qm_dqzone, KM_SLEEP);
 
@@ -548,8 +536,54 @@ xfs_qm_dqread(
 		break;
 	}
 
+	xfs_qm_dquot_logitem_init(dqp);
+
 	XFS_STATS_INC(mp, xs_qm_dquot);
+	return dqp;
+}
+
+/* Copy the in-core quota fields in from the on-disk buffer. */
+STATIC void
+xfs_dquot_from_disk(
+	struct xfs_dquot	*dqp,
+	struct xfs_disk_dquot	*ddqp)
+{
+	/* copy everything from disk dquot to the incore dquot */
+	memcpy(&dqp->q_core, ddqp, sizeof(xfs_disk_dquot_t));
+
+	/*
+	 * Reservation counters are defined as reservation plus current usage
+	 * to avoid having to add every time.
+	 */
+	dqp->q_res_bcount = be64_to_cpu(ddqp->d_bcount);
+	dqp->q_res_icount = be64_to_cpu(ddqp->d_icount);
+	dqp->q_res_rtbcount = be64_to_cpu(ddqp->d_rtbcount);
 
+	/* initialize the dquot speculative prealloc thresholds */
+	xfs_dquot_set_prealloc_limits(dqp);
+}
+
+/*
+ * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
+ * and release the buffer immediately.
+ *
+ * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
+ */
+int
+xfs_qm_dqread(
+	struct xfs_mount	*mp,
+	xfs_dqid_t		id,
+	uint			type,
+	uint			flags,
+	struct xfs_dquot	**O_dqpp)
+{
+	struct xfs_dquot	*dqp;
+	struct xfs_disk_dquot	*ddqp;
+	struct xfs_buf		*bp;
+	struct xfs_trans	*tp = NULL;
+	int			error;
+
+	dqp = xfs_dquot_alloc(mp, id, type);
 	trace_xfs_dqread(dqp);
 
 	if (flags & XFS_QMOPT_DQALLOC) {
@@ -574,20 +608,7 @@ xfs_qm_dqread(
 		goto error1;
 	}
 
-	/* copy everything from disk dquot to the incore dquot */
-	memcpy(&dqp->q_core, ddqp, sizeof(xfs_disk_dquot_t));
-	xfs_qm_dquot_logitem_init(dqp);
-
-	/*
-	 * Reservation counters are defined as reservation plus current usage
-	 * to avoid having to add every time.
-	 */
-	dqp->q_res_bcount = be64_to_cpu(ddqp->d_bcount);
-	dqp->q_res_icount = be64_to_cpu(ddqp->d_icount);
-	dqp->q_res_rtbcount = be64_to_cpu(ddqp->d_rtbcount);
-
-	/* initialize the dquot speculative prealloc thresholds */
-	xfs_dquot_set_prealloc_limits(dqp);
+	xfs_dquot_from_disk(dqp, ddqp);
 
 	/* Mark the buf so that this will stay incore a little longer */
 	xfs_buf_set_ref(bp, XFS_DQUOT_REF);

[PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Separate the disk dquot read and allocation functionality into
two helper functions, then refactor dqread to call them directly.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_dquot.c |  261 ++++++++++++++++++++--------------------------------
 1 file changed, 98 insertions(+), 163 deletions(-)


diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index f1dc62ca54a7..d8730f6110b8 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -288,49 +288,43 @@ xfs_dquot_set_prealloc_limits(struct xfs_dquot *dqp)
 }
 
 /*
- * Allocate a block and fill it with dquots.
- * This is called when the bmapi finds a hole.
+ * Ensure that the given in-core dquot has a buffer on disk backing it, and
+ * return the buffer. This is called when the bmapi finds a hole.
  */
 STATIC int
-xfs_qm_dqalloc(
-	xfs_trans_t	**tpp,
-	xfs_mount_t	*mp,
-	xfs_dquot_t	*dqp,
-	xfs_inode_t	*quotip,
-	xfs_fileoff_t	offset_fsb,
-	xfs_buf_t	**O_bpp)
+xfs_dquot_disk_alloc(
+	struct xfs_trans	**tpp,
+	struct xfs_dquot	*dqp,
+	struct xfs_buf		**bpp)
 {
-	xfs_fsblock_t	firstblock;
-	struct xfs_defer_ops dfops;
-	xfs_bmbt_irec_t map;
-	int		nmaps, error;
-	xfs_buf_t	*bp;
-	xfs_trans_t	*tp = *tpp;
-
-	ASSERT(tp != NULL);
+	struct xfs_bmbt_irec	map;
+	struct xfs_defer_ops	dfops;
+	struct xfs_mount	*mp = (*tpp)->t_mountp;
+	struct xfs_buf		*bp;
+	struct xfs_inode	*quotip = xfs_quota_inode(mp, dqp->dq_flags);
+	xfs_fsblock_t		firstblock;
+	int			nmaps = 1;
+	int			error;
 
 	trace_xfs_dqalloc(dqp);
 
-	/*
-	 * Initialize the bmap freelist prior to calling bmapi code.
-	 */
 	xfs_defer_init(&dfops, &firstblock);
 	xfs_ilock(quotip, XFS_ILOCK_EXCL);
-	/*
-	 * Return if this type of quotas is turned off while we didn't
-	 * have an inode lock
-	 */
 	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
+		/*
+		 * Return if this type of quotas is turned off while we didn't
+		 * have an inode lock
+		 */
 		xfs_iunlock(quotip, XFS_ILOCK_EXCL);
 		return -ESRCH;
 	}
 
-	xfs_trans_ijoin(tp, quotip, XFS_ILOCK_EXCL);
-	nmaps = 1;
-	error = xfs_bmapi_write(tp, quotip, offset_fsb,
-				XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA,
-				&firstblock, XFS_QM_DQALLOC_SPACE_RES(mp),
-				&map, &nmaps, &dfops);
+	/* Create the block mapping. */
+	xfs_trans_ijoin(*tpp, quotip, XFS_ILOCK_EXCL);
+	error = xfs_bmapi_write(*tpp, quotip, dqp->q_fileoffset,
+			XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA,
+			&firstblock, XFS_QM_DQALLOC_SPACE_RES(mp),
+			&map, &nmaps, &dfops);
 	if (error)
 		goto error0;
 	ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB);
@@ -344,10 +338,8 @@ xfs_qm_dqalloc(
 	dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
 
 	/* now we can just get the buffer (there's nothing to read yet) */
-	bp = xfs_trans_get_buf(tp, mp->m_ddev_targp,
-			       dqp->q_blkno,
-			       mp->m_quotainfo->qi_dqchunklen,
-			       0);
+	bp = xfs_trans_get_buf(*tpp, mp->m_ddev_targp, dqp->q_blkno,
+			mp->m_quotainfo->qi_dqchunklen, 0);
 	if (!bp) {
 		error = -ENOMEM;
 		goto error1;
@@ -358,37 +350,22 @@ xfs_qm_dqalloc(
 	 * Make a chunk of dquots out of this buffer and log
 	 * the entire thing.
 	 */
-	xfs_qm_init_dquot_blk(tp, mp, be32_to_cpu(dqp->q_core.d_id),
+	xfs_qm_init_dquot_blk(*tpp, mp, be32_to_cpu(dqp->q_core.d_id),
 			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
+	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
 
 	/*
-	 * xfs_defer_finish() may commit the current transaction and
-	 * start a second transaction if the freelist is not empty.
-	 *
-	 * Since we still want to modify this buffer, we need to
-	 * ensure that the buffer is not released on commit of
-	 * the first transaction and ensure the buffer is added to the
-	 * second transaction.
-	 *
-	 * If there is only one transaction then don't stop the buffer
-	 * from being released when it commits later on.
+	 * Hold the buffer and join it to the dfops so that we'll still own
+	 * the buffer when we return to the caller.
 	 */
-
-	xfs_trans_bhold(tp, bp);
-
+	xfs_trans_bhold(*tpp, bp);
+	error = xfs_defer_bjoin(&dfops, bp);
+	if (error)
+		goto error1;
 	error = xfs_defer_finish(tpp, &dfops);
 	if (error)
 		goto error1;
-
-	/* Transaction was committed? */
-	if (*tpp != tp) {
-		tp = *tpp;
-		xfs_trans_bjoin(tp, bp);
-	} else {
-		xfs_trans_bhold_release(tp, bp);
-	}
-
-	*O_bpp = bp;
+	*bpp = bp;
 	return 0;
 
 error1:
@@ -398,32 +375,24 @@ xfs_qm_dqalloc(
 }
 
 /*
- * Maps a dquot to the buffer containing its on-disk version.
- * This returns a ptr to the buffer containing the on-disk dquot
- * in the bpp param, and a ptr to the on-disk dquot within that buffer
+ * Read in the in-core dquot's on-disk metadata and return the buffer.
+ * Returns ENOENT to signal a hole.
  */
 STATIC int
-xfs_qm_dqtobp(
-	xfs_trans_t		**tpp,
-	xfs_dquot_t		*dqp,
-	xfs_disk_dquot_t	**O_ddpp,
-	xfs_buf_t		**O_bpp,
-	uint			flags)
+xfs_dquot_disk_read(
+	struct xfs_mount	*mp,
+	struct xfs_dquot	*dqp,
+	struct xfs_buf		**bpp)
 {
 	struct xfs_bmbt_irec	map;
-	int			nmaps = 1, error;
 	struct xfs_buf		*bp;
-	struct xfs_inode	*quotip;
-	struct xfs_mount	*mp = dqp->q_mount;
-	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
-	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
+	struct xfs_inode	*quotip = xfs_quota_inode(mp, dqp->dq_flags);
 	uint			lock_mode;
-
-	quotip = xfs_quota_inode(dqp->q_mount, dqp->dq_flags);
-	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
+	int			nmaps = 1;
+	int			error;
 
 	lock_mode = xfs_ilock_data_map_shared(quotip);
-	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
+	if (!xfs_this_quota_on(mp, dqp->dq_flags)) {
 		/*
 		 * Return if this type of quotas is turned off while we
 		 * didn't have the quota inode lock.
@@ -436,57 +405,36 @@ xfs_qm_dqtobp(
 	 * Find the block map; no allocations yet
 	 */
 	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
-			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
-
+			XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
 	xfs_iunlock(quotip, lock_mode);
 	if (error)
 		return error;
 
 	ASSERT(nmaps == 1);
-	ASSERT(map.br_blockcount == 1);
+	ASSERT(map.br_blockcount >= 1);
+	ASSERT(map.br_startblock != DELAYSTARTBLOCK);
+	if (map.br_startblock == HOLESTARTBLOCK)
+		return -ENOENT;
+
+	trace_xfs_dqtobp_read(dqp);
 
 	/*
-	 * Offset of dquot in the (fixed sized) dquot chunk.
+	 * store the blkno etc so that we don't have to do the
+	 * mapping all the time
 	 */
-	dqp->q_bufoffset = (id % mp->m_quotainfo->qi_dqperchunk) *
-		sizeof(xfs_dqblk_t);
-
-	ASSERT(map.br_startblock != DELAYSTARTBLOCK);
-	if (map.br_startblock == HOLESTARTBLOCK) {
-		/*
-		 * We don't allocate unless we're asked to
-		 */
-		if (!(flags & XFS_QMOPT_DQALLOC))
-			return -ENOENT;
-
-		ASSERT(tp);
-		error = xfs_qm_dqalloc(tpp, mp, dqp, quotip,
-					dqp->q_fileoffset, &bp);
-		if (error)
-			return error;
-		tp = *tpp;
-	} else {
-		trace_xfs_dqtobp_read(dqp);
+	dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
 
-		/*
-		 * store the blkno etc so that we don't have to do the
-		 * mapping all the time
-		 */
-		dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
-
-		error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
-					   dqp->q_blkno,
-					   mp->m_quotainfo->qi_dqchunklen,
-					   0, &bp, &xfs_dquot_buf_ops);
-		if (error) {
-			ASSERT(bp == NULL);
-			return error;
-		}
+	error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp, dqp->q_blkno,
+			mp->m_quotainfo->qi_dqchunklen, 0, &bp,
+			&xfs_dquot_buf_ops);
+	if (error) {
+		ASSERT(bp == NULL);
+		return error;
 	}
 
 	ASSERT(xfs_buf_islocked(bp));
-	*O_bpp = bp;
-	*O_ddpp = bp->b_addr + dqp->q_bufoffset;
+	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
+	*bpp = bp;
 
 	return 0;
 }
@@ -508,6 +456,12 @@ xfs_dquot_alloc(
 	INIT_LIST_HEAD(&dqp->q_lru);
 	mutex_init(&dqp->q_qlock);
 	init_waitqueue_head(&dqp->q_pinwait);
+	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
+	/*
+	 * Offset of dquot in the (fixed sized) dquot chunk.
+	 */
+	dqp->q_bufoffset = (id % mp->m_quotainfo->qi_dqperchunk) *
+			sizeof(xfs_dqblk_t);
 
 	/*
 	 * Because we want to use a counting completion, complete
@@ -546,8 +500,10 @@ xfs_dquot_alloc(
 STATIC void
 xfs_dquot_from_disk(
 	struct xfs_dquot	*dqp,
-	struct xfs_disk_dquot	*ddqp)
+	struct xfs_buf		*bp)
 {
+	struct xfs_disk_dquot	*ddqp = bp->b_addr + dqp->q_bufoffset;
+
 	/* copy everything from disk dquot to the incore dquot */
 	memcpy(&dqp->q_core, ddqp, sizeof(xfs_disk_dquot_t));
 
@@ -575,74 +531,53 @@ xfs_qm_dqread(
 	xfs_dqid_t		id,
 	uint			type,
 	uint			flags,
-	struct xfs_dquot	**O_dqpp)
+	struct xfs_dquot	**dqpp)
 {
 	struct xfs_dquot	*dqp;
-	struct xfs_disk_dquot	*ddqp;
 	struct xfs_buf		*bp;
-	struct xfs_trans	*tp = NULL;
+	struct xfs_trans	*tp;
 	int			error;
 
 	dqp = xfs_dquot_alloc(mp, id, type);
 	trace_xfs_dqread(dqp);
 
-	if (flags & XFS_QMOPT_DQALLOC) {
+	/* Try to read the buffer... */
+	error = xfs_dquot_disk_read(mp, dqp, &bp);
+	if (error == -ENOENT && (flags & XFS_QMOPT_DQALLOC)) {
+		/* ...or allocate a new block and buffer. */
 		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc,
 				XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp);
 		if (error)
-			goto error0;
-	}
+			goto err;
 
-	/*
-	 * get a pointer to the on-disk dquot and the buffer containing it
-	 * dqp already knows its own type (GROUP/USER).
-	 */
-	error = xfs_qm_dqtobp(&tp, dqp, &ddqp, &bp, flags);
-	if (error) {
-		/*
-		 * This can happen if quotas got turned off (ESRCH),
-		 * or if the dquot didn't exist on disk and we ask to
-		 * allocate (ENOENT).
-		 */
-		trace_xfs_dqread_fail(dqp);
-		goto error1;
-	}
-
-	xfs_dquot_from_disk(dqp, ddqp);
+		error = xfs_dquot_disk_alloc(&tp, dqp, &bp);
+		if (error)
+			goto err_cancel;
 
-	/* Mark the buf so that this will stay incore a little longer */
-	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
+		error = xfs_trans_commit(tp);
+	}
+	if (error)
+		goto err;
 
 	/*
-	 * We got the buffer with a xfs_trans_read_buf() (in dqtobp())
-	 * So we need to release with xfs_trans_brelse().
-	 * The strategy here is identical to that of inodes; we lock
-	 * the dquot in xfs_qm_dqget() before making it accessible to
-	 * others. This is because dquots, like inodes, need a good level of
-	 * concurrency, and we don't want to take locks on the entire buffers
-	 * for dquot accesses.
-	 * Note also that the dquot buffer may even be dirty at this point, if
-	 * this particular dquot was repaired. We still aren't afraid to
-	 * brelse it because we have the changes incore.
+	 * At this point we should have a clean locked buffer.  Copy the data
+	 * to the incore dquot and release the buffer since the incore dquot
+	 * has its own locking protocol so we needn't tie up the buffer any
+	 * further.
 	 */
 	ASSERT(xfs_buf_islocked(bp));
-	xfs_trans_brelse(tp, bp);
+	xfs_dquot_from_disk(dqp, bp);
 
-	if (tp) {
-		error = xfs_trans_commit(tp);
-		if (error)
-			goto error0;
-	}
-
-	*O_dqpp = dqp;
+	xfs_buf_relse(bp);
+	*dqpp = dqp;
 	return error;
 
-error1:
-	if (tp)
-		xfs_trans_cancel(tp);
-error0:
+err_cancel:
+	xfs_trans_cancel(tp);
+err:
+	trace_xfs_dqread_fail(dqp);
 	xfs_qm_dqdestroy(dqp);
-	*O_dqpp = NULL;
+	*dqpp = NULL;
 	return error;
 }
 

[PATCH 09/13] xfs: remove xfs_qm_dqread flags argument

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Move the _qm_dqread functionality to _qm_dqensure, then remove the flags
argument from xfs_qm_dqread since the only DQALLOC users were internal
to xfs_dquot.c anyway.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_dquot.c |   37 +++++++++++++++++++++++--------------
 fs/xfs/xfs_dquot.h |    6 +++---
 fs/xfs/xfs_qm.c    |    4 ++--
 3 files changed, 28 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index d8730f6110b8..7a343b571ea9 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -521,16 +521,15 @@ xfs_dquot_from_disk(
 
 /*
  * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
- * and release the buffer immediately.
- *
- * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
+ * and release the buffer immediately.  If @can_alloc is specified, fill any
+ * holes in the on-disk metadata.
  */
-int
-xfs_qm_dqread(
+STATIC int
+xfs_dquot_setup(
 	struct xfs_mount	*mp,
 	xfs_dqid_t		id,
 	uint			type,
-	uint			flags,
+	bool			can_alloc,
 	struct xfs_dquot	**dqpp)
 {
 	struct xfs_dquot	*dqp;
@@ -543,7 +542,7 @@ xfs_qm_dqread(
 
 	/* Try to read the buffer... */
 	error = xfs_dquot_disk_read(mp, dqp, &bp);
-	if (error == -ENOENT && (flags & XFS_QMOPT_DQALLOC)) {
+	if (error == -ENOENT && can_alloc) {
 		/* ...or allocate a new block and buffer. */
 		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc,
 				XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp);
@@ -581,6 +580,20 @@ xfs_qm_dqread(
 	return error;
 }
 
+/*
+ * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
+ * and release the buffer immediately.
+ */
+int
+xfs_qm_dqread(
+	struct xfs_mount	*mp,
+	xfs_dqid_t		id,
+	uint			type,
+	struct xfs_dquot	**dqpp)
+{
+	return xfs_dquot_setup(mp, id, type, false, dqpp);
+}
+
 /*
  * Advance to the next id in the current chunk, or if at the
  * end of the chunk, skip ahead to first id in next allocated chunk
@@ -746,7 +759,7 @@ xfs_qm_dqget(
 	struct xfs_mount	*mp,
 	xfs_dqid_t		id,
 	uint			type,
-	uint			flags,	  /* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
+	uint			flags,	/* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
 	struct xfs_dquot	**O_dqpp)
 {
 	struct xfs_quotainfo	*qi = mp->m_quotainfo;
@@ -765,7 +778,7 @@ xfs_qm_dqget(
 		return 0;
 	}
 
-	error = xfs_qm_dqread(mp, id, type, flags, &dqp);
+	error = xfs_dquot_setup(mp, id, type, flags & XFS_QMOPT_DQALLOC, &dqp);
 	if (error)
 		return error;
 
@@ -820,16 +833,12 @@ xfs_qm_dqget_inode(
 	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
 	struct xfs_dquot	*dqp;
 	xfs_dqid_t		id;
-	uint			flags = 0;
 	int			error;
 
 	error = xfs_qm_dqget_checks(mp, type);
 	if (error)
 		return error;
 
-	if (can_alloc)
-		flags |= XFS_QMOPT_DQALLOC;
-
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(xfs_inode_dquot(ip, type) == NULL);
 
@@ -850,7 +859,7 @@ xfs_qm_dqget_inode(
 	 * we re-acquire the lock.
 	 */
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	error = xfs_qm_dqread(mp, id, type, flags, &dqp);
+	error = xfs_dquot_setup(mp, id, type, can_alloc, &dqp);
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	if (error)
 		return error;
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 8a5b30e952b0..486a1b475574 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -160,8 +160,8 @@ static inline bool xfs_dquot_lowsp(struct xfs_dquot *dqp)
 #define XFS_QM_ISPDQ(dqp)	((dqp)->dq_flags & XFS_DQ_PROJ)
 #define XFS_QM_ISGDQ(dqp)	((dqp)->dq_flags & XFS_DQ_GROUP)
 
-extern int		xfs_qm_dqread(struct xfs_mount *, xfs_dqid_t, uint,
-					uint, struct xfs_dquot	**);
+extern int		xfs_qm_dqread(struct xfs_mount *mp, xfs_dqid_t id,
+					uint type, struct xfs_dquot **dqpp);
 extern void		xfs_qm_dqdestroy(xfs_dquot_t *);
 extern int		xfs_qm_dqflush(struct xfs_dquot *, struct xfs_buf **);
 extern void		xfs_qm_dqunpin_wait(xfs_dquot_t *);
@@ -172,7 +172,7 @@ extern void		xfs_qm_adjust_dqlimits(struct xfs_mount *,
 extern xfs_dqid_t	xfs_qm_id_for_quotatype(struct xfs_inode *ip,
 					uint type);
 extern int		xfs_qm_dqget(struct xfs_mount *mp, xfs_dqid_t id,
-					uint type, uint flags,
+					uint type, uint can_alloc,
 					struct xfs_dquot **dqpp);
 extern int		xfs_qm_dqget_inode(struct xfs_inode *ip, uint type,
 					bool can_alloc,
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index fb14954610ee..9f646222c4ac 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -569,7 +569,7 @@ xfs_qm_set_defquota(
 	struct xfs_def_quota    *defq;
 	int			error;
 
-	error = xfs_qm_dqread(mp, 0, type, 0, &dqp);
+	error = xfs_qm_dqread(mp, 0, type, &dqp);
 
 	if (!error) {
 		xfs_disk_dquot_t        *ddqp = &dqp->q_core;
@@ -647,7 +647,7 @@ xfs_qm_init_quotainfo(
 			XFS_IS_UQUOTA_RUNNING(mp) ? XFS_DQ_USER :
 			 (XFS_IS_GQUOTA_RUNNING(mp) ? XFS_DQ_GROUP :
 			  XFS_DQ_PROJ),
-			0, &dqp);
+			&dqp);
 
 	if (!error) {
 		xfs_disk_dquot_t	*ddqp = &dqp->q_core;

[PATCH 10/13] xfs: replace XFS_QMOPT_DQALLOC with XFS_DQGET_{ALLOC, EXISTS}

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

DQALLOC is only ever used with xfs_qm_dqget*, and the only flag that the
_dqget family of functions cares about is DQALLOC.  Therefore, change
the name to make it clear that it's only a dqget flag.  Since these are
modes and no longer bit flags, define a XFS_DQGET_EXISTS flag and
convert all the callers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_quota_defs.h |    1 -
 fs/xfs/xfs_dquot.c             |    8 ++++----
 fs/xfs/xfs_dquot.h             |    2 +-
 fs/xfs/xfs_qm.c                |   12 +++++-------
 fs/xfs/xfs_qm_bhv.c            |    2 +-
 fs/xfs/xfs_qm_syscalls.c       |    9 ++++-----
 6 files changed, 15 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
index c80b22629c78..bfe03fd3aa8e 100644
--- a/fs/xfs/libxfs/xfs_quota_defs.h
+++ b/fs/xfs/libxfs/xfs_quota_defs.h
@@ -107,7 +107,6 @@ typedef uint16_t	xfs_qwarncnt_t;
  * to a single function. None of these XFS_QMOPT_* flags are meant to have
  * persistent values (ie. their values can and will change between versions)
  */
-#define XFS_QMOPT_DQALLOC	0x0000002 /* alloc dquot ondisk if needed */
 #define XFS_QMOPT_UQUOTA	0x0000004 /* user dquot requested */
 #define XFS_QMOPT_PQUOTA	0x0000008 /* project dquot requested */
 #define XFS_QMOPT_FORCE_RES	0x0000010 /* ignore quota limits */
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 7a343b571ea9..301a96d70785 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -521,7 +521,7 @@ xfs_dquot_from_disk(
 
 /*
  * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
- * and release the buffer immediately.  If @can_alloc is specified, fill any
+ * and release the buffer immediately.  If @can_alloc is true, fill any
  * holes in the on-disk metadata.
  */
 STATIC int
@@ -759,7 +759,7 @@ xfs_qm_dqget(
 	struct xfs_mount	*mp,
 	xfs_dqid_t		id,
 	uint			type,
-	uint			flags,	/* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
+	bool			can_alloc,
 	struct xfs_dquot	**O_dqpp)
 {
 	struct xfs_quotainfo	*qi = mp->m_quotainfo;
@@ -778,7 +778,7 @@ xfs_qm_dqget(
 		return 0;
 	}
 
-	error = xfs_dquot_setup(mp, id, type, flags & XFS_QMOPT_DQALLOC, &dqp);
+	error = xfs_dquot_setup(mp, id, type, can_alloc, &dqp);
 	if (error)
 		return error;
 
@@ -918,7 +918,7 @@ xfs_qm_dqget_next(
 
 	*dqpp = NULL;
 	for (; !error; error = xfs_dq_get_next_id(mp, type, &id)) {
-		error = xfs_qm_dqget(mp, id, type, 0, &dqp);
+		error = xfs_qm_dqget(mp, id, type, false, &dqp);
 		if (error == -ENOENT)
 			continue;
 		else if (error != 0)
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 486a1b475574..6959f0135f36 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -172,7 +172,7 @@ extern void		xfs_qm_adjust_dqlimits(struct xfs_mount *,
 extern xfs_dqid_t	xfs_qm_id_for_quotatype(struct xfs_inode *ip,
 					uint type);
 extern int		xfs_qm_dqget(struct xfs_mount *mp, xfs_dqid_t id,
-					uint type, uint can_alloc,
+					uint type, bool can_alloc,
 					struct xfs_dquot **dqpp);
 extern int		xfs_qm_dqget_inode(struct xfs_inode *ip, uint type,
 					bool can_alloc,
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 9f646222c4ac..c6b1e9bd7a3e 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -326,7 +326,7 @@ xfs_qm_need_dqattach(
 /*
  * Given a locked inode, attach dquot(s) to it, taking U/G/P-QUOTAON
  * into account.
- * If XFS_QMOPT_DQALLOC, the dquot(s) will be allocated if needed.
+ * If @doalloc is true, the dquot(s) will be allocated if needed.
  * Inode may get unlocked and relocked in here, and the caller must deal with
  * the consequences.
  */
@@ -1072,7 +1072,7 @@ xfs_qm_quotacheck_dqadjust(
 	int			error;
 
 	id = xfs_qm_id_for_quotatype(ip, type);
-	error = xfs_qm_dqget(mp, id, type, XFS_QMOPT_DQALLOC, &dqp);
+	error = xfs_qm_dqget(mp, id, type, true, &dqp);
 	if (error) {
 		/*
 		 * Shouldn't be able to turn off quotas here.
@@ -1686,8 +1686,7 @@ xfs_qm_vop_dqalloc(
 			 * holding ilock.
 			 */
 			xfs_iunlock(ip, lockflags);
-			error = xfs_qm_dqget(mp, uid, XFS_DQ_USER,
-					XFS_QMOPT_DQALLOC, &uq);
+			error = xfs_qm_dqget(mp, uid, XFS_DQ_USER, true, &uq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				return error;
@@ -1710,8 +1709,7 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
 		if (ip->i_d.di_gid != gid) {
 			xfs_iunlock(ip, lockflags);
-			error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP,
-					XFS_QMOPT_DQALLOC, &gq);
+			error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP, true, &gq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				goto error_rele;
@@ -1728,7 +1726,7 @@ xfs_qm_vop_dqalloc(
 		if (xfs_get_projid(ip) != prid) {
 			xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, (xfs_dqid_t)prid, XFS_DQ_PROJ,
-					XFS_QMOPT_DQALLOC, &pq);
+					true, &pq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				goto error_rele;
diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index 531e8224dcb6..1fa3d6d6db40 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -72,7 +72,7 @@ xfs_qm_statvfs(
 	xfs_mount_t		*mp = ip->i_mount;
 	xfs_dquot_t		*dqp;
 
-	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, 0, &dqp)) {
+	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, true, &dqp)) {
 		xfs_fill_statvfs_from_dquot(statp, dqp);
 		xfs_qm_dqput(dqp);
 	}
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index b9243f554697..3e05d300b14e 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -425,7 +425,7 @@ xfs_qm_scall_setqlim(
 	 * a reference to the dquot, so it's safe to do this unlock/lock without
 	 * it being reclaimed in the mean time.
 	 */
-	error = xfs_qm_dqget(mp, id, type, XFS_QMOPT_DQALLOC, &dqp);
+	error = xfs_qm_dqget(mp, id, type, true, &dqp);
 	if (error) {
 		ASSERT(error != -ENOENT);
 		goto out_unlock;
@@ -696,11 +696,10 @@ xfs_qm_scall_getquota(
 	int			error;
 
 	/*
-	 * Try to get the dquot. We don't want it allocated on disk, so
-	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
-	 * exist, we'll get ENOENT back.
+	 * Try to get the dquot. We don't want it allocated on disk, so don't
+	 * set doalloc. If it doesn't exist, we'll get ENOENT back.
 	 */
-	error = xfs_qm_dqget(mp, id, type, 0, &dqp);
+	error = xfs_qm_dqget(mp, id, type, false, &dqp);
 	if (error)
 		return error;
 

[PATCH 11/13] xfs: report failing address when dquot verifier fails

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Pass the failing address through to the corruption report when dquot
verifiers fail.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dquot_buf.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index 8b7a6c3cb599..a1e6cf15b171 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -229,7 +229,7 @@ xfs_dquot_buf_read_verify(
 	else {
 		fa = xfs_dquot_buf_verify(mp, bp);
 		if (fa)
-			xfs_verifier_error(bp, -EFSCORRUPTED, __this_address);
+			xfs_verifier_error(bp, -EFSCORRUPTED, fa);
 	}
 }
 
@@ -266,7 +266,7 @@ xfs_dquot_buf_write_verify(
 
 	fa = xfs_dquot_buf_verify(mp, bp);
 	if (fa)
-		xfs_verifier_error(bp, -EFSCORRUPTED, __this_address);
+		xfs_verifier_error(bp, -EFSCORRUPTED, fa);
 }
 
 const struct xfs_buf_ops xfs_dquot_buf_ops = {

[PATCH 12/13] xfs: rename on-disk dquot counter zap functions

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The function 'xfs_qm_dqiterate' doesn't iterate dquots at all, it
iterates all dquot blocks of a quota inode and clears the counters.
Therefore, change the name to something more descriptive so that we can
introduce a real dquot iterator later.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_qm.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)


diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index c6b1e9bd7a3e..fcbbae70748a 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -889,7 +889,7 @@ xfs_qm_reset_dqcounts(
 }
 
 STATIC int
-xfs_qm_dqiter_bufs(
+xfs_qm_reset_dqcounts_all(
 	struct xfs_mount	*mp,
 	xfs_dqid_t		firstid,
 	xfs_fsblock_t		bno,
@@ -957,11 +957,11 @@ xfs_qm_dqiter_bufs(
 }
 
 /*
- * Iterate over all allocated USR/GRP/PRJ dquots in the system, calling a
- * caller supplied function for every chunk of dquots that we find.
+ * Iterate over all allocated dquot blocks in this quota inode, zeroing all
+ * counters for every chunk of dquots that we find.
  */
 STATIC int
-xfs_qm_dqiterate(
+xfs_qm_reset_dqcounts_buf(
 	struct xfs_mount	*mp,
 	struct xfs_inode	*qip,
 	uint			flags,
@@ -1037,7 +1037,7 @@ xfs_qm_dqiterate(
 			 * Iterate thru all the blks in the extent and
 			 * reset the counters of all the dquots inside them.
 			 */
-			error = xfs_qm_dqiter_bufs(mp, firstid,
+			error = xfs_qm_reset_dqcounts_all(mp, firstid,
 						   map[i].br_startblock,
 						   map[i].br_blockcount,
 						   flags, buffer_list);
@@ -1299,7 +1299,7 @@ xfs_qm_quotacheck(
 	 * We don't log our changes till later.
 	 */
 	if (uip) {
-		error = xfs_qm_dqiterate(mp, uip, XFS_QMOPT_UQUOTA,
+		error = xfs_qm_reset_dqcounts_buf(mp, uip, XFS_QMOPT_UQUOTA,
 					 &buffer_list);
 		if (error)
 			goto error_return;
@@ -1307,7 +1307,7 @@ xfs_qm_quotacheck(
 	}
 
 	if (gip) {
-		error = xfs_qm_dqiterate(mp, gip, XFS_QMOPT_GQUOTA,
+		error = xfs_qm_reset_dqcounts_buf(mp, gip, XFS_QMOPT_GQUOTA,
 					 &buffer_list);
 		if (error)
 			goto error_return;
@@ -1315,7 +1315,7 @@ xfs_qm_quotacheck(
 	}
 
 	if (pip) {
-		error = xfs_qm_dqiterate(mp, pip, XFS_QMOPT_PQUOTA,
+		error = xfs_qm_reset_dqcounts_buf(mp, pip, XFS_QMOPT_PQUOTA,
 					 &buffer_list);
 		if (error)
 			goto error_return;

[PATCH 13/13] xfs: refactor dquot iteration

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a helper function to iterate all the dquots of a given type in
the system, and refactor the dquot scrub to use it.  This will get more
use in the quota repair code.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/quota.c |   56 +++++++++++++++++++++++---------------------------
 fs/xfs/xfs_dquot.c   |   32 +++++++++++++++++++++++++++++
 fs/xfs/xfs_dquot.h   |    5 ++++
 3 files changed, 63 insertions(+), 30 deletions(-)


diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index 50415e8e5dd1..7bf102c290b7 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -77,14 +77,20 @@ xfs_scrub_setup_quota(
 
 /* Quotas. */
 
+struct xfs_scrub_quota_info {
+	struct xfs_scrub_context	*sc;
+	xfs_dqid_t			last_id;
+};
+
 /* Scrub the fields in an individual quota item. */
-STATIC void
+STATIC int
 xfs_scrub_quota_item(
-	struct xfs_scrub_context	*sc,
-	uint				dqtype,
 	struct xfs_dquot		*dq,
-	xfs_dqid_t			id)
+	uint				dqtype,
+	void				*priv)
 {
+	struct xfs_scrub_quota_info	*sqi = priv;
+	struct xfs_scrub_context	*sc = sqi->sc;
 	struct xfs_mount		*mp = sc->mp;
 	struct xfs_disk_dquot		*d = &dq->q_core;
 	struct xfs_quotainfo		*qi = mp->m_quotainfo;
@@ -99,17 +105,18 @@ xfs_scrub_quota_item(
 	unsigned long long		icount;
 	unsigned long long		rcount;
 	xfs_ino_t			fs_icount;
-
-	offset = id / qi->qi_dqperchunk;
+	xfs_dqid_t			id = be32_to_cpu(d->d_id);
 
 	/*
-	 * We fed $id and DQNEXT into the xfs_qm_dqget call, which means
-	 * that the actual dquot we got must either have the same id or
-	 * the next higher id.
+	 * Except for the root dquot, the actual dquot we got must either have
+	 * the same or higher id as we saw before.
 	 */
-	if (id > be32_to_cpu(d->d_id))
+	if (id && id <= sqi->last_id)
 		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
 
+	sqi->last_id = id;
+	offset = id / qi->qi_dqperchunk;
+
 	/* Did we get the dquot type we wanted? */
 	if (dqtype != (d->d_flags & XFS_DQ_ALLTYPES))
 		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
@@ -183,6 +190,8 @@ xfs_scrub_quota_item(
 		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
 	if (id != 0 && rhard != 0 && rcount > rhard)
 		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
+
+	return 0;
 }
 
 /* Scrub all of a quota type's items. */
@@ -191,13 +200,12 @@ xfs_scrub_quota(
 	struct xfs_scrub_context	*sc)
 {
 	struct xfs_bmbt_irec		irec = { 0 };
+	struct xfs_scrub_quota_info	sqi;
 	struct xfs_mount		*mp = sc->mp;
 	struct xfs_inode		*ip;
 	struct xfs_quotainfo		*qi = mp->m_quotainfo;
-	struct xfs_dquot		*dq;
 	xfs_fileoff_t			max_dqid_off;
 	xfs_fileoff_t			off = 0;
-	xfs_dqid_t			id = 0;
 	uint				dqtype;
 	int				nimaps;
 	int				error = 0;
@@ -264,24 +272,12 @@ xfs_scrub_quota(
 		goto out;
 
 	/* Check all the quota items. */
-	while (id < ((xfs_dqid_t)-1ULL)) {
-		if (xfs_scrub_should_terminate(sc, &error))
-			break;
-
-		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
-		if (error == -ENOENT)
-			break;
-		if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
-				id * qi->qi_dqperchunk, &error))
-			break;
-
-		xfs_scrub_quota_item(sc, dqtype, dq, id);
-
-		id = be32_to_cpu(dq->q_core.d_id) + 1;
-		xfs_qm_dqput(dq);
-		if (!id)
-			break;
-	}
+	sqi.sc = sc;
+	sqi.last_id = 0;
+	error = xfs_qm_dqiterate(mp, dqtype, xfs_scrub_quota_item, &sqi);
+	if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
+			sqi.last_id * qi->qi_dqperchunk, &error))
+		goto out;
 
 out:
 	/* We set sc->ip earlier, so make sure we clear it now. */
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 301a96d70785..6f7200f8f445 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1213,3 +1213,35 @@ xfs_qm_exit(void)
 	kmem_zone_destroy(xfs_qm_dqtrxzone);
 	kmem_zone_destroy(xfs_qm_dqzone);
 }
+
+/*
+ * Iterate every dquot of a particular type.  The caller must ensure that the
+ * particular quota type is active.  iter_fn can return negative error codes,
+ * or XFS_BTREE_QUERY_RANGE_ABORT to indicate that it wants to stop iterating.
+ */
+int
+xfs_qm_dqiterate(
+	struct xfs_mount	*mp,
+	uint			dqtype,
+	xfs_qm_dqiterate_fn	iter_fn,
+	void			*priv)
+{
+	struct xfs_dquot	*dq;
+	xfs_dqid_t		id = 0;
+	int			error;
+
+	do {
+		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
+		if (error == -ENOENT)
+			return 0;
+		if (error)
+			return error;
+
+		error = iter_fn(dq, dqtype, priv);
+		id = be32_to_cpu(dq->q_core.d_id);
+		xfs_qm_dqput(dq);
+		id++;
+	} while (error == 0 && id != 0);
+
+	return error;
+}
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 6959f0135f36..417ac03b8d7b 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -193,4 +193,9 @@ static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
 	return dqp;
 }
 
+typedef int (*xfs_qm_dqiterate_fn)(struct xfs_dquot *dq, uint dqtype,
+		void *priv);
+int xfs_qm_dqiterate(struct xfs_mount *mp, uint dqtype,
+		xfs_qm_dqiterate_fn iter_fn, void *priv);
+
 #endif /* __XFS_DQUOT_H__ */

[PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean

[Darrick J. Wong]

Ugh, wrong commit message.  Here's an updated one now that the flag is
gone entirely.

--D

---
Subject: [PATCH] xfs: replace XFS_QMOPT_DQALLOC with a simple boolean

DQALLOC is only ever used with xfs_qm_dqget*, and the only flag that the
_dqget family of functions cares about is DQALLOC.  Therefore, change
it to a boolean 'can alloc?' flag for the dqget interfaces where that
makes sense.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_quota_defs.h |    1 -
 fs/xfs/xfs_dquot.c             |    8 ++++----
 fs/xfs/xfs_dquot.h             |    2 +-
 fs/xfs/xfs_qm.c                |   12 +++++-------
 fs/xfs/xfs_qm_bhv.c            |    2 +-
 fs/xfs/xfs_qm_syscalls.c       |    9 ++++-----
 6 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
index c80b22629c78..bfe03fd3aa8e 100644
--- a/fs/xfs/libxfs/xfs_quota_defs.h
+++ b/fs/xfs/libxfs/xfs_quota_defs.h
@@ -107,7 +107,6 @@ typedef uint16_t	xfs_qwarncnt_t;
  * to a single function. None of these XFS_QMOPT_* flags are meant to have
  * persistent values (ie. their values can and will change between versions)
  */
-#define XFS_QMOPT_DQALLOC	0x0000002 /* alloc dquot ondisk if needed */
 #define XFS_QMOPT_UQUOTA	0x0000004 /* user dquot requested */
 #define XFS_QMOPT_PQUOTA	0x0000008 /* project dquot requested */
 #define XFS_QMOPT_FORCE_RES	0x0000010 /* ignore quota limits */
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 7a343b571ea9..301a96d70785 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -521,7 +521,7 @@ xfs_dquot_from_disk(
 
 /*
  * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
- * and release the buffer immediately.  If @can_alloc is specified, fill any
+ * and release the buffer immediately.  If @can_alloc is true, fill any
  * holes in the on-disk metadata.
  */
 STATIC int
@@ -759,7 +759,7 @@ xfs_qm_dqget(
 	struct xfs_mount	*mp,
 	xfs_dqid_t		id,
 	uint			type,
-	uint			flags,	/* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
+	bool			can_alloc,
 	struct xfs_dquot	**O_dqpp)
 {
 	struct xfs_quotainfo	*qi = mp->m_quotainfo;
@@ -778,7 +778,7 @@ xfs_qm_dqget(
 		return 0;
 	}
 
-	error = xfs_dquot_setup(mp, id, type, flags & XFS_QMOPT_DQALLOC, &dqp);
+	error = xfs_dquot_setup(mp, id, type, can_alloc, &dqp);
 	if (error)
 		return error;
 
@@ -918,7 +918,7 @@ xfs_qm_dqget_next(
 
 	*dqpp = NULL;
 	for (; !error; error = xfs_dq_get_next_id(mp, type, &id)) {
-		error = xfs_qm_dqget(mp, id, type, 0, &dqp);
+		error = xfs_qm_dqget(mp, id, type, false, &dqp);
 		if (error == -ENOENT)
 			continue;
 		else if (error != 0)
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 486a1b475574..6959f0135f36 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -172,7 +172,7 @@ extern void		xfs_qm_adjust_dqlimits(struct xfs_mount *,
 extern xfs_dqid_t	xfs_qm_id_for_quotatype(struct xfs_inode *ip,
 					uint type);
 extern int		xfs_qm_dqget(struct xfs_mount *mp, xfs_dqid_t id,
-					uint type, uint can_alloc,
+					uint type, bool can_alloc,
 					struct xfs_dquot **dqpp);
 extern int		xfs_qm_dqget_inode(struct xfs_inode *ip, uint type,
 					bool can_alloc,
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 9f646222c4ac..c6b1e9bd7a3e 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -326,7 +326,7 @@ xfs_qm_need_dqattach(
 /*
  * Given a locked inode, attach dquot(s) to it, taking U/G/P-QUOTAON
  * into account.
- * If XFS_QMOPT_DQALLOC, the dquot(s) will be allocated if needed.
+ * If @doalloc is true, the dquot(s) will be allocated if needed.
  * Inode may get unlocked and relocked in here, and the caller must deal with
  * the consequences.
  */
@@ -1072,7 +1072,7 @@ xfs_qm_quotacheck_dqadjust(
 	int			error;
 
 	id = xfs_qm_id_for_quotatype(ip, type);
-	error = xfs_qm_dqget(mp, id, type, XFS_QMOPT_DQALLOC, &dqp);
+	error = xfs_qm_dqget(mp, id, type, true, &dqp);
 	if (error) {
 		/*
 		 * Shouldn't be able to turn off quotas here.
@@ -1686,8 +1686,7 @@ xfs_qm_vop_dqalloc(
 			 * holding ilock.
 			 */
 			xfs_iunlock(ip, lockflags);
-			error = xfs_qm_dqget(mp, uid, XFS_DQ_USER,
-					XFS_QMOPT_DQALLOC, &uq);
+			error = xfs_qm_dqget(mp, uid, XFS_DQ_USER, true, &uq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				return error;
@@ -1710,8 +1709,7 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
 		if (ip->i_d.di_gid != gid) {
 			xfs_iunlock(ip, lockflags);
-			error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP,
-					XFS_QMOPT_DQALLOC, &gq);
+			error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP, true, &gq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				goto error_rele;
@@ -1728,7 +1726,7 @@ xfs_qm_vop_dqalloc(
 		if (xfs_get_projid(ip) != prid) {
 			xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, (xfs_dqid_t)prid, XFS_DQ_PROJ,
-					XFS_QMOPT_DQALLOC, &pq);
+					true, &pq);
 			if (error) {
 				ASSERT(error != -ENOENT);
 				goto error_rele;
diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index 531e8224dcb6..1fa3d6d6db40 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -72,7 +72,7 @@ xfs_qm_statvfs(
 	xfs_mount_t		*mp = ip->i_mount;
 	xfs_dquot_t		*dqp;
 
-	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, 0, &dqp)) {
+	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, true, &dqp)) {
 		xfs_fill_statvfs_from_dquot(statp, dqp);
 		xfs_qm_dqput(dqp);
 	}
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index b9243f554697..3e05d300b14e 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -425,7 +425,7 @@ xfs_qm_scall_setqlim(
 	 * a reference to the dquot, so it's safe to do this unlock/lock without
 	 * it being reclaimed in the mean time.
 	 */
-	error = xfs_qm_dqget(mp, id, type, XFS_QMOPT_DQALLOC, &dqp);
+	error = xfs_qm_dqget(mp, id, type, true, &dqp);
 	if (error) {
 		ASSERT(error != -ENOENT);
 		goto out_unlock;
@@ -696,11 +696,10 @@ xfs_qm_scall_getquota(
 	int			error;
 
 	/*
-	 * Try to get the dquot. We don't want it allocated on disk, so
-	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
-	 * exist, we'll get ENOENT back.
+	 * Try to get the dquot. We don't want it allocated on disk, so don't
+	 * set doalloc. If it doesn't exist, we'll get ENOENT back.
 	 */
-	error = xfs_qm_dqget(mp, id, type, 0, &dqp);
+	error = xfs_qm_dqget(mp, id, type, false, &dqp);
 	if (error)
 		return error;
 

Re: [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc

[Brian Foster]

On Sun, Apr 29, 2018 at 10:44:04PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Separate the disk dquot read and allocation functionality into
> two helper functions, then refactor dqread to call them directly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

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

>  fs/xfs/xfs_dquot.c |  261 ++++++++++++++++++++--------------------------------
>  1 file changed, 98 insertions(+), 163 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index f1dc62ca54a7..d8730f6110b8 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -288,49 +288,43 @@ xfs_dquot_set_prealloc_limits(struct xfs_dquot *dqp)
>  }
>  
>  /*
> - * Allocate a block and fill it with dquots.
> - * This is called when the bmapi finds a hole.
> + * Ensure that the given in-core dquot has a buffer on disk backing it, and
> + * return the buffer. This is called when the bmapi finds a hole.
>   */
>  STATIC int
> -xfs_qm_dqalloc(
> -	xfs_trans_t	**tpp,
> -	xfs_mount_t	*mp,
> -	xfs_dquot_t	*dqp,
> -	xfs_inode_t	*quotip,
> -	xfs_fileoff_t	offset_fsb,
> -	xfs_buf_t	**O_bpp)
> +xfs_dquot_disk_alloc(
> +	struct xfs_trans	**tpp,
> +	struct xfs_dquot	*dqp,
> +	struct xfs_buf		**bpp)
>  {
> -	xfs_fsblock_t	firstblock;
> -	struct xfs_defer_ops dfops;
> -	xfs_bmbt_irec_t map;
> -	int		nmaps, error;
> -	xfs_buf_t	*bp;
> -	xfs_trans_t	*tp = *tpp;
> -
> -	ASSERT(tp != NULL);
> +	struct xfs_bmbt_irec	map;
> +	struct xfs_defer_ops	dfops;
> +	struct xfs_mount	*mp = (*tpp)->t_mountp;
> +	struct xfs_buf		*bp;
> +	struct xfs_inode	*quotip = xfs_quota_inode(mp, dqp->dq_flags);
> +	xfs_fsblock_t		firstblock;
> +	int			nmaps = 1;
> +	int			error;
>  
>  	trace_xfs_dqalloc(dqp);
>  
> -	/*
> -	 * Initialize the bmap freelist prior to calling bmapi code.
> -	 */
>  	xfs_defer_init(&dfops, &firstblock);
>  	xfs_ilock(quotip, XFS_ILOCK_EXCL);
> -	/*
> -	 * Return if this type of quotas is turned off while we didn't
> -	 * have an inode lock
> -	 */
>  	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
> +		/*
> +		 * Return if this type of quotas is turned off while we didn't
> +		 * have an inode lock
> +		 */
>  		xfs_iunlock(quotip, XFS_ILOCK_EXCL);
>  		return -ESRCH;
>  	}
>  
> -	xfs_trans_ijoin(tp, quotip, XFS_ILOCK_EXCL);
> -	nmaps = 1;
> -	error = xfs_bmapi_write(tp, quotip, offset_fsb,
> -				XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA,
> -				&firstblock, XFS_QM_DQALLOC_SPACE_RES(mp),
> -				&map, &nmaps, &dfops);
> +	/* Create the block mapping. */
> +	xfs_trans_ijoin(*tpp, quotip, XFS_ILOCK_EXCL);
> +	error = xfs_bmapi_write(*tpp, quotip, dqp->q_fileoffset,
> +			XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA,
> +			&firstblock, XFS_QM_DQALLOC_SPACE_RES(mp),
> +			&map, &nmaps, &dfops);
>  	if (error)
>  		goto error0;
>  	ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB);
> @@ -344,10 +338,8 @@ xfs_qm_dqalloc(
>  	dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
>  
>  	/* now we can just get the buffer (there's nothing to read yet) */
> -	bp = xfs_trans_get_buf(tp, mp->m_ddev_targp,
> -			       dqp->q_blkno,
> -			       mp->m_quotainfo->qi_dqchunklen,
> -			       0);
> +	bp = xfs_trans_get_buf(*tpp, mp->m_ddev_targp, dqp->q_blkno,
> +			mp->m_quotainfo->qi_dqchunklen, 0);
>  	if (!bp) {
>  		error = -ENOMEM;
>  		goto error1;
> @@ -358,37 +350,22 @@ xfs_qm_dqalloc(
>  	 * Make a chunk of dquots out of this buffer and log
>  	 * the entire thing.
>  	 */
> -	xfs_qm_init_dquot_blk(tp, mp, be32_to_cpu(dqp->q_core.d_id),
> +	xfs_qm_init_dquot_blk(*tpp, mp, be32_to_cpu(dqp->q_core.d_id),
>  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
> +	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
>  
>  	/*
> -	 * xfs_defer_finish() may commit the current transaction and
> -	 * start a second transaction if the freelist is not empty.
> -	 *
> -	 * Since we still want to modify this buffer, we need to
> -	 * ensure that the buffer is not released on commit of
> -	 * the first transaction and ensure the buffer is added to the
> -	 * second transaction.
> -	 *
> -	 * If there is only one transaction then don't stop the buffer
> -	 * from being released when it commits later on.
> +	 * Hold the buffer and join it to the dfops so that we'll still own
> +	 * the buffer when we return to the caller.
>  	 */
> -
> -	xfs_trans_bhold(tp, bp);
> -
> +	xfs_trans_bhold(*tpp, bp);
> +	error = xfs_defer_bjoin(&dfops, bp);
> +	if (error)
> +		goto error1;
>  	error = xfs_defer_finish(tpp, &dfops);
>  	if (error)
>  		goto error1;
> -
> -	/* Transaction was committed? */
> -	if (*tpp != tp) {
> -		tp = *tpp;
> -		xfs_trans_bjoin(tp, bp);
> -	} else {
> -		xfs_trans_bhold_release(tp, bp);
> -	}
> -
> -	*O_bpp = bp;
> +	*bpp = bp;
>  	return 0;
>  
>  error1:
> @@ -398,32 +375,24 @@ xfs_qm_dqalloc(
>  }
>  
>  /*
> - * Maps a dquot to the buffer containing its on-disk version.
> - * This returns a ptr to the buffer containing the on-disk dquot
> - * in the bpp param, and a ptr to the on-disk dquot within that buffer
> + * Read in the in-core dquot's on-disk metadata and return the buffer.
> + * Returns ENOENT to signal a hole.
>   */
>  STATIC int
> -xfs_qm_dqtobp(
> -	xfs_trans_t		**tpp,
> -	xfs_dquot_t		*dqp,
> -	xfs_disk_dquot_t	**O_ddpp,
> -	xfs_buf_t		**O_bpp,
> -	uint			flags)
> +xfs_dquot_disk_read(
> +	struct xfs_mount	*mp,
> +	struct xfs_dquot	*dqp,
> +	struct xfs_buf		**bpp)
>  {
>  	struct xfs_bmbt_irec	map;
> -	int			nmaps = 1, error;
>  	struct xfs_buf		*bp;
> -	struct xfs_inode	*quotip;
> -	struct xfs_mount	*mp = dqp->q_mount;
> -	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
> -	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
> +	struct xfs_inode	*quotip = xfs_quota_inode(mp, dqp->dq_flags);
>  	uint			lock_mode;
> -
> -	quotip = xfs_quota_inode(dqp->q_mount, dqp->dq_flags);
> -	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
> +	int			nmaps = 1;
> +	int			error;
>  
>  	lock_mode = xfs_ilock_data_map_shared(quotip);
> -	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
> +	if (!xfs_this_quota_on(mp, dqp->dq_flags)) {
>  		/*
>  		 * Return if this type of quotas is turned off while we
>  		 * didn't have the quota inode lock.
> @@ -436,57 +405,36 @@ xfs_qm_dqtobp(
>  	 * Find the block map; no allocations yet
>  	 */
>  	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
> -			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
> -
> +			XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
>  	xfs_iunlock(quotip, lock_mode);
>  	if (error)
>  		return error;
>  
>  	ASSERT(nmaps == 1);
> -	ASSERT(map.br_blockcount == 1);
> +	ASSERT(map.br_blockcount >= 1);
> +	ASSERT(map.br_startblock != DELAYSTARTBLOCK);
> +	if (map.br_startblock == HOLESTARTBLOCK)
> +		return -ENOENT;
> +
> +	trace_xfs_dqtobp_read(dqp);
>  
>  	/*
> -	 * Offset of dquot in the (fixed sized) dquot chunk.
> +	 * store the blkno etc so that we don't have to do the
> +	 * mapping all the time
>  	 */
> -	dqp->q_bufoffset = (id % mp->m_quotainfo->qi_dqperchunk) *
> -		sizeof(xfs_dqblk_t);
> -
> -	ASSERT(map.br_startblock != DELAYSTARTBLOCK);
> -	if (map.br_startblock == HOLESTARTBLOCK) {
> -		/*
> -		 * We don't allocate unless we're asked to
> -		 */
> -		if (!(flags & XFS_QMOPT_DQALLOC))
> -			return -ENOENT;
> -
> -		ASSERT(tp);
> -		error = xfs_qm_dqalloc(tpp, mp, dqp, quotip,
> -					dqp->q_fileoffset, &bp);
> -		if (error)
> -			return error;
> -		tp = *tpp;
> -	} else {
> -		trace_xfs_dqtobp_read(dqp);
> +	dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
>  
> -		/*
> -		 * store the blkno etc so that we don't have to do the
> -		 * mapping all the time
> -		 */
> -		dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
> -
> -		error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
> -					   dqp->q_blkno,
> -					   mp->m_quotainfo->qi_dqchunklen,
> -					   0, &bp, &xfs_dquot_buf_ops);
> -		if (error) {
> -			ASSERT(bp == NULL);
> -			return error;
> -		}
> +	error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp, dqp->q_blkno,
> +			mp->m_quotainfo->qi_dqchunklen, 0, &bp,
> +			&xfs_dquot_buf_ops);
> +	if (error) {
> +		ASSERT(bp == NULL);
> +		return error;
>  	}
>  
>  	ASSERT(xfs_buf_islocked(bp));
> -	*O_bpp = bp;
> -	*O_ddpp = bp->b_addr + dqp->q_bufoffset;
> +	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
> +	*bpp = bp;
>  
>  	return 0;
>  }
> @@ -508,6 +456,12 @@ xfs_dquot_alloc(
>  	INIT_LIST_HEAD(&dqp->q_lru);
>  	mutex_init(&dqp->q_qlock);
>  	init_waitqueue_head(&dqp->q_pinwait);
> +	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
> +	/*
> +	 * Offset of dquot in the (fixed sized) dquot chunk.
> +	 */
> +	dqp->q_bufoffset = (id % mp->m_quotainfo->qi_dqperchunk) *
> +			sizeof(xfs_dqblk_t);
>  
>  	/*
>  	 * Because we want to use a counting completion, complete
> @@ -546,8 +500,10 @@ xfs_dquot_alloc(
>  STATIC void
>  xfs_dquot_from_disk(
>  	struct xfs_dquot	*dqp,
> -	struct xfs_disk_dquot	*ddqp)
> +	struct xfs_buf		*bp)
>  {
> +	struct xfs_disk_dquot	*ddqp = bp->b_addr + dqp->q_bufoffset;
> +
>  	/* copy everything from disk dquot to the incore dquot */
>  	memcpy(&dqp->q_core, ddqp, sizeof(xfs_disk_dquot_t));
>  
> @@ -575,74 +531,53 @@ xfs_qm_dqread(
>  	xfs_dqid_t		id,
>  	uint			type,
>  	uint			flags,
> -	struct xfs_dquot	**O_dqpp)
> +	struct xfs_dquot	**dqpp)
>  {
>  	struct xfs_dquot	*dqp;
> -	struct xfs_disk_dquot	*ddqp;
>  	struct xfs_buf		*bp;
> -	struct xfs_trans	*tp = NULL;
> +	struct xfs_trans	*tp;
>  	int			error;
>  
>  	dqp = xfs_dquot_alloc(mp, id, type);
>  	trace_xfs_dqread(dqp);
>  
> -	if (flags & XFS_QMOPT_DQALLOC) {
> +	/* Try to read the buffer... */
> +	error = xfs_dquot_disk_read(mp, dqp, &bp);
> +	if (error == -ENOENT && (flags & XFS_QMOPT_DQALLOC)) {
> +		/* ...or allocate a new block and buffer. */
>  		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc,
>  				XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp);
>  		if (error)
> -			goto error0;
> -	}
> +			goto err;
>  
> -	/*
> -	 * get a pointer to the on-disk dquot and the buffer containing it
> -	 * dqp already knows its own type (GROUP/USER).
> -	 */
> -	error = xfs_qm_dqtobp(&tp, dqp, &ddqp, &bp, flags);
> -	if (error) {
> -		/*
> -		 * This can happen if quotas got turned off (ESRCH),
> -		 * or if the dquot didn't exist on disk and we ask to
> -		 * allocate (ENOENT).
> -		 */
> -		trace_xfs_dqread_fail(dqp);
> -		goto error1;
> -	}
> -
> -	xfs_dquot_from_disk(dqp, ddqp);
> +		error = xfs_dquot_disk_alloc(&tp, dqp, &bp);
> +		if (error)
> +			goto err_cancel;
>  
> -	/* Mark the buf so that this will stay incore a little longer */
> -	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
> +		error = xfs_trans_commit(tp);
> +	}
> +	if (error)
> +		goto err;
>  
>  	/*
> -	 * We got the buffer with a xfs_trans_read_buf() (in dqtobp())
> -	 * So we need to release with xfs_trans_brelse().
> -	 * The strategy here is identical to that of inodes; we lock
> -	 * the dquot in xfs_qm_dqget() before making it accessible to
> -	 * others. This is because dquots, like inodes, need a good level of
> -	 * concurrency, and we don't want to take locks on the entire buffers
> -	 * for dquot accesses.
> -	 * Note also that the dquot buffer may even be dirty at this point, if
> -	 * this particular dquot was repaired. We still aren't afraid to
> -	 * brelse it because we have the changes incore.
> +	 * At this point we should have a clean locked buffer.  Copy the data
> +	 * to the incore dquot and release the buffer since the incore dquot
> +	 * has its own locking protocol so we needn't tie up the buffer any
> +	 * further.
>  	 */
>  	ASSERT(xfs_buf_islocked(bp));
> -	xfs_trans_brelse(tp, bp);
> +	xfs_dquot_from_disk(dqp, bp);
>  
> -	if (tp) {
> -		error = xfs_trans_commit(tp);
> -		if (error)
> -			goto error0;
> -	}
> -
> -	*O_dqpp = dqp;
> +	xfs_buf_relse(bp);
> +	*dqpp = dqp;
>  	return error;
>  
> -error1:
> -	if (tp)
> -		xfs_trans_cancel(tp);
> -error0:
> +err_cancel:
> +	xfs_trans_cancel(tp);
> +err:
> +	trace_xfs_dqread_fail(dqp);
>  	xfs_qm_dqdestroy(dqp);
> -	*O_dqpp = NULL;
> +	*dqpp = NULL;
>  	return error;
>  }
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument

[Brian Foster]

On Sun, Apr 29, 2018 at 10:44:11PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the _qm_dqread functionality to _qm_dqensure, then remove the flags
> argument from xfs_qm_dqread since the only DQALLOC users were internal
> to xfs_dquot.c anyway.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

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

>  fs/xfs/xfs_dquot.c |   37 +++++++++++++++++++++++--------------
>  fs/xfs/xfs_dquot.h |    6 +++---
>  fs/xfs/xfs_qm.c    |    4 ++--
>  3 files changed, 28 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index d8730f6110b8..7a343b571ea9 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -521,16 +521,15 @@ xfs_dquot_from_disk(
>  
>  /*
>   * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
> - * and release the buffer immediately.
> - *
> - * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
> + * and release the buffer immediately.  If @can_alloc is specified, fill any
> + * holes in the on-disk metadata.
>   */
> -int
> -xfs_qm_dqread(
> +STATIC int
> +xfs_dquot_setup(
>  	struct xfs_mount	*mp,
>  	xfs_dqid_t		id,
>  	uint			type,
> -	uint			flags,
> +	bool			can_alloc,
>  	struct xfs_dquot	**dqpp)
>  {
>  	struct xfs_dquot	*dqp;
> @@ -543,7 +542,7 @@ xfs_qm_dqread(
>  
>  	/* Try to read the buffer... */
>  	error = xfs_dquot_disk_read(mp, dqp, &bp);
> -	if (error == -ENOENT && (flags & XFS_QMOPT_DQALLOC)) {
> +	if (error == -ENOENT && can_alloc) {
>  		/* ...or allocate a new block and buffer. */
>  		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc,
>  				XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp);
> @@ -581,6 +580,20 @@ xfs_qm_dqread(
>  	return error;
>  }
>  
> +/*
> + * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
> + * and release the buffer immediately.
> + */
> +int
> +xfs_qm_dqread(
> +	struct xfs_mount	*mp,
> +	xfs_dqid_t		id,
> +	uint			type,
> +	struct xfs_dquot	**dqpp)
> +{
> +	return xfs_dquot_setup(mp, id, type, false, dqpp);
> +}
> +
>  /*
>   * Advance to the next id in the current chunk, or if at the
>   * end of the chunk, skip ahead to first id in next allocated chunk
> @@ -746,7 +759,7 @@ xfs_qm_dqget(
>  	struct xfs_mount	*mp,
>  	xfs_dqid_t		id,
>  	uint			type,
> -	uint			flags,	  /* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
> +	uint			flags,	/* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
>  	struct xfs_dquot	**O_dqpp)
>  {
>  	struct xfs_quotainfo	*qi = mp->m_quotainfo;
> @@ -765,7 +778,7 @@ xfs_qm_dqget(
>  		return 0;
>  	}
>  
> -	error = xfs_qm_dqread(mp, id, type, flags, &dqp);
> +	error = xfs_dquot_setup(mp, id, type, flags & XFS_QMOPT_DQALLOC, &dqp);
>  	if (error)
>  		return error;
>  
> @@ -820,16 +833,12 @@ xfs_qm_dqget_inode(
>  	struct radix_tree_root	*tree = xfs_dquot_tree(qi, type);
>  	struct xfs_dquot	*dqp;
>  	xfs_dqid_t		id;
> -	uint			flags = 0;
>  	int			error;
>  
>  	error = xfs_qm_dqget_checks(mp, type);
>  	if (error)
>  		return error;
>  
> -	if (can_alloc)
> -		flags |= XFS_QMOPT_DQALLOC;
> -
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  	ASSERT(xfs_inode_dquot(ip, type) == NULL);
>  
> @@ -850,7 +859,7 @@ xfs_qm_dqget_inode(
>  	 * we re-acquire the lock.
>  	 */
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -	error = xfs_qm_dqread(mp, id, type, flags, &dqp);
> +	error = xfs_dquot_setup(mp, id, type, can_alloc, &dqp);
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	if (error)
>  		return error;
> diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
> index 8a5b30e952b0..486a1b475574 100644
> --- a/fs/xfs/xfs_dquot.h
> +++ b/fs/xfs/xfs_dquot.h
> @@ -160,8 +160,8 @@ static inline bool xfs_dquot_lowsp(struct xfs_dquot *dqp)
>  #define XFS_QM_ISPDQ(dqp)	((dqp)->dq_flags & XFS_DQ_PROJ)
>  #define XFS_QM_ISGDQ(dqp)	((dqp)->dq_flags & XFS_DQ_GROUP)
>  
> -extern int		xfs_qm_dqread(struct xfs_mount *, xfs_dqid_t, uint,
> -					uint, struct xfs_dquot	**);
> +extern int		xfs_qm_dqread(struct xfs_mount *mp, xfs_dqid_t id,
> +					uint type, struct xfs_dquot **dqpp);
>  extern void		xfs_qm_dqdestroy(xfs_dquot_t *);
>  extern int		xfs_qm_dqflush(struct xfs_dquot *, struct xfs_buf **);
>  extern void		xfs_qm_dqunpin_wait(xfs_dquot_t *);
> @@ -172,7 +172,7 @@ extern void		xfs_qm_adjust_dqlimits(struct xfs_mount *,
>  extern xfs_dqid_t	xfs_qm_id_for_quotatype(struct xfs_inode *ip,
>  					uint type);
>  extern int		xfs_qm_dqget(struct xfs_mount *mp, xfs_dqid_t id,
> -					uint type, uint flags,
> +					uint type, uint can_alloc,
>  					struct xfs_dquot **dqpp);
>  extern int		xfs_qm_dqget_inode(struct xfs_inode *ip, uint type,
>  					bool can_alloc,
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index fb14954610ee..9f646222c4ac 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -569,7 +569,7 @@ xfs_qm_set_defquota(
>  	struct xfs_def_quota    *defq;
>  	int			error;
>  
> -	error = xfs_qm_dqread(mp, 0, type, 0, &dqp);
> +	error = xfs_qm_dqread(mp, 0, type, &dqp);
>  
>  	if (!error) {
>  		xfs_disk_dquot_t        *ddqp = &dqp->q_core;
> @@ -647,7 +647,7 @@ xfs_qm_init_quotainfo(
>  			XFS_IS_UQUOTA_RUNNING(mp) ? XFS_DQ_USER :
>  			 (XFS_IS_GQUOTA_RUNNING(mp) ? XFS_DQ_GROUP :
>  			  XFS_DQ_PROJ),
> -			0, &dqp);
> +			&dqp);
>  
>  	if (!error) {
>  		xfs_disk_dquot_t	*ddqp = &dqp->q_core;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean

[Brian Foster]

On Sun, Apr 29, 2018 at 10:47:52PM -0700, Darrick J. Wong wrote:
> Ugh, wrong commit message.  Here's an updated one now that the flag is
> gone entirely.
> 

Just FYI.. git applies this with the above as the commit log message and
the text below is tossed. ;P

> --D
> 
> ---
> Subject: [PATCH] xfs: replace XFS_QMOPT_DQALLOC with a simple boolean
> 
> DQALLOC is only ever used with xfs_qm_dqget*, and the only flag that the
> _dqget family of functions cares about is DQALLOC.  Therefore, change
> it to a boolean 'can alloc?' flag for the dqget interfaces where that
> makes sense.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_quota_defs.h |    1 -
>  fs/xfs/xfs_dquot.c             |    8 ++++----
>  fs/xfs/xfs_dquot.h             |    2 +-
>  fs/xfs/xfs_qm.c                |   12 +++++-------
>  fs/xfs/xfs_qm_bhv.c            |    2 +-
>  fs/xfs/xfs_qm_syscalls.c       |    9 ++++-----
>  6 files changed, 15 insertions(+), 19 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> index 531e8224dcb6..1fa3d6d6db40 100644
> --- a/fs/xfs/xfs_qm_bhv.c
> +++ b/fs/xfs/xfs_qm_bhv.c
> @@ -72,7 +72,7 @@ xfs_qm_statvfs(
>  	xfs_mount_t		*mp = ip->i_mount;
>  	xfs_dquot_t		*dqp;
>  
> -	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, 0, &dqp)) {
> +	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, true, &dqp)) {

False? Otherwise looks fine:

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

>  		xfs_fill_statvfs_from_dquot(statp, dqp);
>  		xfs_qm_dqput(dqp);
>  	}
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index b9243f554697..3e05d300b14e 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -425,7 +425,7 @@ xfs_qm_scall_setqlim(
>  	 * a reference to the dquot, so it's safe to do this unlock/lock without
>  	 * it being reclaimed in the mean time.
>  	 */
> -	error = xfs_qm_dqget(mp, id, type, XFS_QMOPT_DQALLOC, &dqp);
> +	error = xfs_qm_dqget(mp, id, type, true, &dqp);
>  	if (error) {
>  		ASSERT(error != -ENOENT);
>  		goto out_unlock;
> @@ -696,11 +696,10 @@ xfs_qm_scall_getquota(
>  	int			error;
>  
>  	/*
> -	 * Try to get the dquot. We don't want it allocated on disk, so
> -	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
> -	 * exist, we'll get ENOENT back.
> +	 * Try to get the dquot. We don't want it allocated on disk, so don't
> +	 * set doalloc. If it doesn't exist, we'll get ENOENT back.
>  	 */
> -	error = xfs_qm_dqget(mp, id, type, 0, &dqp);
> +	error = xfs_qm_dqget(mp, id, type, false, &dqp);
>  	if (error)
>  		return error;
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/13] xfs: rename on-disk dquot counter zap functions

[Brian Foster]

On Sun, Apr 29, 2018 at 10:44:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The function 'xfs_qm_dqiterate' doesn't iterate dquots at all, it
> iterates all dquot blocks of a quota inode and clears the counters.
> Therefore, change the name to something more descriptive so that we can
> introduce a real dquot iterator later.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

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

>  fs/xfs/xfs_qm.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index c6b1e9bd7a3e..fcbbae70748a 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -889,7 +889,7 @@ xfs_qm_reset_dqcounts(
>  }
>  
>  STATIC int
> -xfs_qm_dqiter_bufs(
> +xfs_qm_reset_dqcounts_all(
>  	struct xfs_mount	*mp,
>  	xfs_dqid_t		firstid,
>  	xfs_fsblock_t		bno,
> @@ -957,11 +957,11 @@ xfs_qm_dqiter_bufs(
>  }
>  
>  /*
> - * Iterate over all allocated USR/GRP/PRJ dquots in the system, calling a
> - * caller supplied function for every chunk of dquots that we find.
> + * Iterate over all allocated dquot blocks in this quota inode, zeroing all
> + * counters for every chunk of dquots that we find.
>   */
>  STATIC int
> -xfs_qm_dqiterate(
> +xfs_qm_reset_dqcounts_buf(
>  	struct xfs_mount	*mp,
>  	struct xfs_inode	*qip,
>  	uint			flags,
> @@ -1037,7 +1037,7 @@ xfs_qm_dqiterate(
>  			 * Iterate thru all the blks in the extent and
>  			 * reset the counters of all the dquots inside them.
>  			 */
> -			error = xfs_qm_dqiter_bufs(mp, firstid,
> +			error = xfs_qm_reset_dqcounts_all(mp, firstid,
>  						   map[i].br_startblock,
>  						   map[i].br_blockcount,
>  						   flags, buffer_list);
> @@ -1299,7 +1299,7 @@ xfs_qm_quotacheck(
>  	 * We don't log our changes till later.
>  	 */
>  	if (uip) {
> -		error = xfs_qm_dqiterate(mp, uip, XFS_QMOPT_UQUOTA,
> +		error = xfs_qm_reset_dqcounts_buf(mp, uip, XFS_QMOPT_UQUOTA,
>  					 &buffer_list);
>  		if (error)
>  			goto error_return;
> @@ -1307,7 +1307,7 @@ xfs_qm_quotacheck(
>  	}
>  
>  	if (gip) {
> -		error = xfs_qm_dqiterate(mp, gip, XFS_QMOPT_GQUOTA,
> +		error = xfs_qm_reset_dqcounts_buf(mp, gip, XFS_QMOPT_GQUOTA,
>  					 &buffer_list);
>  		if (error)
>  			goto error_return;
> @@ -1315,7 +1315,7 @@ xfs_qm_quotacheck(
>  	}
>  
>  	if (pip) {
> -		error = xfs_qm_dqiterate(mp, pip, XFS_QMOPT_PQUOTA,
> +		error = xfs_qm_reset_dqcounts_buf(mp, pip, XFS_QMOPT_PQUOTA,
>  					 &buffer_list);
>  		if (error)
>  			goto error_return;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] xfs: refactor dquot iteration

[Brian Foster]

On Sun, Apr 29, 2018 at 10:44:36PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a helper function to iterate all the dquots of a given type in
> the system, and refactor the dquot scrub to use it.  This will get more
> use in the quota repair code.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

I get a compile warning from this patch:

fs/xfs//scrub/quota.c: In function ‘xfs_scrub_quota_item’:
fs/xfs//scrub/quota.c:115:3: warning: ‘offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

With that fixed:

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

>  fs/xfs/scrub/quota.c |   56 +++++++++++++++++++++++---------------------------
>  fs/xfs/xfs_dquot.c   |   32 +++++++++++++++++++++++++++++
>  fs/xfs/xfs_dquot.h   |    5 ++++
>  3 files changed, 63 insertions(+), 30 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index 50415e8e5dd1..7bf102c290b7 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -77,14 +77,20 @@ xfs_scrub_setup_quota(
>  
>  /* Quotas. */
>  
> +struct xfs_scrub_quota_info {
> +	struct xfs_scrub_context	*sc;
> +	xfs_dqid_t			last_id;
> +};
> +
>  /* Scrub the fields in an individual quota item. */
> -STATIC void
> +STATIC int
>  xfs_scrub_quota_item(
> -	struct xfs_scrub_context	*sc,
> -	uint				dqtype,
>  	struct xfs_dquot		*dq,
> -	xfs_dqid_t			id)
> +	uint				dqtype,
> +	void				*priv)
>  {
> +	struct xfs_scrub_quota_info	*sqi = priv;
> +	struct xfs_scrub_context	*sc = sqi->sc;
>  	struct xfs_mount		*mp = sc->mp;
>  	struct xfs_disk_dquot		*d = &dq->q_core;
>  	struct xfs_quotainfo		*qi = mp->m_quotainfo;
> @@ -99,17 +105,18 @@ xfs_scrub_quota_item(
>  	unsigned long long		icount;
>  	unsigned long long		rcount;
>  	xfs_ino_t			fs_icount;
> -
> -	offset = id / qi->qi_dqperchunk;
> +	xfs_dqid_t			id = be32_to_cpu(d->d_id);
>  
>  	/*
> -	 * We fed $id and DQNEXT into the xfs_qm_dqget call, which means
> -	 * that the actual dquot we got must either have the same id or
> -	 * the next higher id.
> +	 * Except for the root dquot, the actual dquot we got must either have
> +	 * the same or higher id as we saw before.
>  	 */
> -	if (id > be32_to_cpu(d->d_id))
> +	if (id && id <= sqi->last_id)
>  		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
>  
> +	sqi->last_id = id;
> +	offset = id / qi->qi_dqperchunk;
> +
>  	/* Did we get the dquot type we wanted? */
>  	if (dqtype != (d->d_flags & XFS_DQ_ALLTYPES))
>  		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
> @@ -183,6 +190,8 @@ xfs_scrub_quota_item(
>  		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
>  	if (id != 0 && rhard != 0 && rcount > rhard)
>  		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
> +
> +	return 0;
>  }
>  
>  /* Scrub all of a quota type's items. */
> @@ -191,13 +200,12 @@ xfs_scrub_quota(
>  	struct xfs_scrub_context	*sc)
>  {
>  	struct xfs_bmbt_irec		irec = { 0 };
> +	struct xfs_scrub_quota_info	sqi;
>  	struct xfs_mount		*mp = sc->mp;
>  	struct xfs_inode		*ip;
>  	struct xfs_quotainfo		*qi = mp->m_quotainfo;
> -	struct xfs_dquot		*dq;
>  	xfs_fileoff_t			max_dqid_off;
>  	xfs_fileoff_t			off = 0;
> -	xfs_dqid_t			id = 0;
>  	uint				dqtype;
>  	int				nimaps;
>  	int				error = 0;
> @@ -264,24 +272,12 @@ xfs_scrub_quota(
>  		goto out;
>  
>  	/* Check all the quota items. */
> -	while (id < ((xfs_dqid_t)-1ULL)) {
> -		if (xfs_scrub_should_terminate(sc, &error))
> -			break;
> -
> -		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
> -		if (error == -ENOENT)
> -			break;
> -		if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
> -				id * qi->qi_dqperchunk, &error))
> -			break;
> -
> -		xfs_scrub_quota_item(sc, dqtype, dq, id);
> -
> -		id = be32_to_cpu(dq->q_core.d_id) + 1;
> -		xfs_qm_dqput(dq);
> -		if (!id)
> -			break;
> -	}
> +	sqi.sc = sc;
> +	sqi.last_id = 0;
> +	error = xfs_qm_dqiterate(mp, dqtype, xfs_scrub_quota_item, &sqi);
> +	if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
> +			sqi.last_id * qi->qi_dqperchunk, &error))
> +		goto out;
>  
>  out:
>  	/* We set sc->ip earlier, so make sure we clear it now. */
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 301a96d70785..6f7200f8f445 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1213,3 +1213,35 @@ xfs_qm_exit(void)
>  	kmem_zone_destroy(xfs_qm_dqtrxzone);
>  	kmem_zone_destroy(xfs_qm_dqzone);
>  }
> +
> +/*
> + * Iterate every dquot of a particular type.  The caller must ensure that the
> + * particular quota type is active.  iter_fn can return negative error codes,
> + * or XFS_BTREE_QUERY_RANGE_ABORT to indicate that it wants to stop iterating.
> + */
> +int
> +xfs_qm_dqiterate(
> +	struct xfs_mount	*mp,
> +	uint			dqtype,
> +	xfs_qm_dqiterate_fn	iter_fn,
> +	void			*priv)
> +{
> +	struct xfs_dquot	*dq;
> +	xfs_dqid_t		id = 0;
> +	int			error;
> +
> +	do {
> +		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
> +		if (error == -ENOENT)
> +			return 0;
> +		if (error)
> +			return error;
> +
> +		error = iter_fn(dq, dqtype, priv);
> +		id = be32_to_cpu(dq->q_core.d_id);
> +		xfs_qm_dqput(dq);
> +		id++;
> +	} while (error == 0 && id != 0);
> +
> +	return error;
> +}
> diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
> index 6959f0135f36..417ac03b8d7b 100644
> --- a/fs/xfs/xfs_dquot.h
> +++ b/fs/xfs/xfs_dquot.h
> @@ -193,4 +193,9 @@ static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
>  	return dqp;
>  }
>  
> +typedef int (*xfs_qm_dqiterate_fn)(struct xfs_dquot *dq, uint dqtype,
> +		void *priv);
> +int xfs_qm_dqiterate(struct xfs_mount *mp, uint dqtype,
> +		xfs_qm_dqiterate_fn iter_fn, void *priv);
> +
>  #endif /* __XFS_DQUOT_H__ */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean

[Darrick J. Wong]

On Tue, May 01, 2018 at 09:45:04AM -0400, Brian Foster wrote:
> On Sun, Apr 29, 2018 at 10:47:52PM -0700, Darrick J. Wong wrote:
> > Ugh, wrong commit message.  Here's an updated one now that the flag is
> > gone entirely.
> > 
> 
> Just FYI.. git applies this with the above as the commit log message and
> the text below is tossed. ;P

D'oh.  Yeah, I screwed up the git-to-mail export. :(

This all will be done correctly in the git tree I swear.

> > --D
> > 
> > ---
> > Subject: [PATCH] xfs: replace XFS_QMOPT_DQALLOC with a simple boolean
> > 
> > DQALLOC is only ever used with xfs_qm_dqget*, and the only flag that the
> > _dqget family of functions cares about is DQALLOC.  Therefore, change
> > it to a boolean 'can alloc?' flag for the dqget interfaces where that
> > makes sense.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_quota_defs.h |    1 -
> >  fs/xfs/xfs_dquot.c             |    8 ++++----
> >  fs/xfs/xfs_dquot.h             |    2 +-
> >  fs/xfs/xfs_qm.c                |   12 +++++-------
> >  fs/xfs/xfs_qm_bhv.c            |    2 +-
> >  fs/xfs/xfs_qm_syscalls.c       |    9 ++++-----
> >  6 files changed, 15 insertions(+), 19 deletions(-)
> > 
> ...
> > diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> > index 531e8224dcb6..1fa3d6d6db40 100644
> > --- a/fs/xfs/xfs_qm_bhv.c
> > +++ b/fs/xfs/xfs_qm_bhv.c
> > @@ -72,7 +72,7 @@ xfs_qm_statvfs(
> >  	xfs_mount_t		*mp = ip->i_mount;
> >  	xfs_dquot_t		*dqp;
> >  
> > -	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, 0, &dqp)) {
> > +	if (!xfs_qm_dqget(mp, xfs_get_projid(ip), XFS_DQ_PROJ, true, &dqp)) {
> 
> False? Otherwise looks fine:

Yep, that should be false.  Fixed.

--D

> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> >  		xfs_fill_statvfs_from_dquot(statp, dqp);
> >  		xfs_qm_dqput(dqp);
> >  	}
> > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> > index b9243f554697..3e05d300b14e 100644
> > --- a/fs/xfs/xfs_qm_syscalls.c
> > +++ b/fs/xfs/xfs_qm_syscalls.c
> > @@ -425,7 +425,7 @@ xfs_qm_scall_setqlim(
> >  	 * a reference to the dquot, so it's safe to do this unlock/lock without
> >  	 * it being reclaimed in the mean time.
> >  	 */
> > -	error = xfs_qm_dqget(mp, id, type, XFS_QMOPT_DQALLOC, &dqp);
> > +	error = xfs_qm_dqget(mp, id, type, true, &dqp);
> >  	if (error) {
> >  		ASSERT(error != -ENOENT);
> >  		goto out_unlock;
> > @@ -696,11 +696,10 @@ xfs_qm_scall_getquota(
> >  	int			error;
> >  
> >  	/*
> > -	 * Try to get the dquot. We don't want it allocated on disk, so
> > -	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
> > -	 * exist, we'll get ENOENT back.
> > +	 * Try to get the dquot. We don't want it allocated on disk, so don't
> > +	 * set doalloc. If it doesn't exist, we'll get ENOENT back.
> >  	 */
> > -	error = xfs_qm_dqget(mp, id, type, 0, &dqp);
> > +	error = xfs_qm_dqget(mp, id, type, false, &dqp);
> >  	if (error)
> >  		return error;
> >  
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] xfs: refactor dquot iteration

[Darrick J. Wong]

On Tue, May 01, 2018 at 09:45:23AM -0400, Brian Foster wrote:
> On Sun, Apr 29, 2018 at 10:44:36PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Create a helper function to iterate all the dquots of a given type in
> > the system, and refactor the dquot scrub to use it.  This will get more
> > use in the quota repair code.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> I get a compile warning from this patch:
> 
> fs/xfs//scrub/quota.c: In function ‘xfs_scrub_quota_item’:
> fs/xfs//scrub/quota.c:115:3: warning: ‘offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>    xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> With that fixed:

Fixed by moving the offset assignment up a few lines.

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

Thank you for reviewing this series, Brian!

--D

> 
> >  fs/xfs/scrub/quota.c |   56 +++++++++++++++++++++++---------------------------
> >  fs/xfs/xfs_dquot.c   |   32 +++++++++++++++++++++++++++++
> >  fs/xfs/xfs_dquot.h   |    5 ++++
> >  3 files changed, 63 insertions(+), 30 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> > index 50415e8e5dd1..7bf102c290b7 100644
> > --- a/fs/xfs/scrub/quota.c
> > +++ b/fs/xfs/scrub/quota.c
> > @@ -77,14 +77,20 @@ xfs_scrub_setup_quota(
> >  
> >  /* Quotas. */
> >  
> > +struct xfs_scrub_quota_info {
> > +	struct xfs_scrub_context	*sc;
> > +	xfs_dqid_t			last_id;
> > +};
> > +
> >  /* Scrub the fields in an individual quota item. */
> > -STATIC void
> > +STATIC int
> >  xfs_scrub_quota_item(
> > -	struct xfs_scrub_context	*sc,
> > -	uint				dqtype,
> >  	struct xfs_dquot		*dq,
> > -	xfs_dqid_t			id)
> > +	uint				dqtype,
> > +	void				*priv)
> >  {
> > +	struct xfs_scrub_quota_info	*sqi = priv;
> > +	struct xfs_scrub_context	*sc = sqi->sc;
> >  	struct xfs_mount		*mp = sc->mp;
> >  	struct xfs_disk_dquot		*d = &dq->q_core;
> >  	struct xfs_quotainfo		*qi = mp->m_quotainfo;
> > @@ -99,17 +105,18 @@ xfs_scrub_quota_item(
> >  	unsigned long long		icount;
> >  	unsigned long long		rcount;
> >  	xfs_ino_t			fs_icount;
> > -
> > -	offset = id / qi->qi_dqperchunk;
> > +	xfs_dqid_t			id = be32_to_cpu(d->d_id);
> >  
> >  	/*
> > -	 * We fed $id and DQNEXT into the xfs_qm_dqget call, which means
> > -	 * that the actual dquot we got must either have the same id or
> > -	 * the next higher id.
> > +	 * Except for the root dquot, the actual dquot we got must either have
> > +	 * the same or higher id as we saw before.
> >  	 */
> > -	if (id > be32_to_cpu(d->d_id))
> > +	if (id && id <= sqi->last_id)
> >  		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
> >  
> > +	sqi->last_id = id;
> > +	offset = id / qi->qi_dqperchunk;
> > +
> >  	/* Did we get the dquot type we wanted? */
> >  	if (dqtype != (d->d_flags & XFS_DQ_ALLTYPES))
> >  		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
> > @@ -183,6 +190,8 @@ xfs_scrub_quota_item(
> >  		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
> >  	if (id != 0 && rhard != 0 && rcount > rhard)
> >  		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
> > +
> > +	return 0;
> >  }
> >  
> >  /* Scrub all of a quota type's items. */
> > @@ -191,13 +200,12 @@ xfs_scrub_quota(
> >  	struct xfs_scrub_context	*sc)
> >  {
> >  	struct xfs_bmbt_irec		irec = { 0 };
> > +	struct xfs_scrub_quota_info	sqi;
> >  	struct xfs_mount		*mp = sc->mp;
> >  	struct xfs_inode		*ip;
> >  	struct xfs_quotainfo		*qi = mp->m_quotainfo;
> > -	struct xfs_dquot		*dq;
> >  	xfs_fileoff_t			max_dqid_off;
> >  	xfs_fileoff_t			off = 0;
> > -	xfs_dqid_t			id = 0;
> >  	uint				dqtype;
> >  	int				nimaps;
> >  	int				error = 0;
> > @@ -264,24 +272,12 @@ xfs_scrub_quota(
> >  		goto out;
> >  
> >  	/* Check all the quota items. */
> > -	while (id < ((xfs_dqid_t)-1ULL)) {
> > -		if (xfs_scrub_should_terminate(sc, &error))
> > -			break;
> > -
> > -		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
> > -		if (error == -ENOENT)
> > -			break;
> > -		if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
> > -				id * qi->qi_dqperchunk, &error))
> > -			break;
> > -
> > -		xfs_scrub_quota_item(sc, dqtype, dq, id);
> > -
> > -		id = be32_to_cpu(dq->q_core.d_id) + 1;
> > -		xfs_qm_dqput(dq);
> > -		if (!id)
> > -			break;
> > -	}
> > +	sqi.sc = sc;
> > +	sqi.last_id = 0;
> > +	error = xfs_qm_dqiterate(mp, dqtype, xfs_scrub_quota_item, &sqi);
> > +	if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
> > +			sqi.last_id * qi->qi_dqperchunk, &error))
> > +		goto out;
> >  
> >  out:
> >  	/* We set sc->ip earlier, so make sure we clear it now. */
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index 301a96d70785..6f7200f8f445 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -1213,3 +1213,35 @@ xfs_qm_exit(void)
> >  	kmem_zone_destroy(xfs_qm_dqtrxzone);
> >  	kmem_zone_destroy(xfs_qm_dqzone);
> >  }
> > +
> > +/*
> > + * Iterate every dquot of a particular type.  The caller must ensure that the
> > + * particular quota type is active.  iter_fn can return negative error codes,
> > + * or XFS_BTREE_QUERY_RANGE_ABORT to indicate that it wants to stop iterating.
> > + */
> > +int
> > +xfs_qm_dqiterate(
> > +	struct xfs_mount	*mp,
> > +	uint			dqtype,
> > +	xfs_qm_dqiterate_fn	iter_fn,
> > +	void			*priv)
> > +{
> > +	struct xfs_dquot	*dq;
> > +	xfs_dqid_t		id = 0;
> > +	int			error;
> > +
> > +	do {
> > +		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
> > +		if (error == -ENOENT)
> > +			return 0;
> > +		if (error)
> > +			return error;
> > +
> > +		error = iter_fn(dq, dqtype, priv);
> > +		id = be32_to_cpu(dq->q_core.d_id);
> > +		xfs_qm_dqput(dq);
> > +		id++;
> > +	} while (error == 0 && id != 0);
> > +
> > +	return error;
> > +}
> > diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
> > index 6959f0135f36..417ac03b8d7b 100644
> > --- a/fs/xfs/xfs_dquot.h
> > +++ b/fs/xfs/xfs_dquot.h
> > @@ -193,4 +193,9 @@ static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
> >  	return dqp;
> >  }
> >  
> > +typedef int (*xfs_qm_dqiterate_fn)(struct xfs_dquot *dq, uint dqtype,
> > +		void *priv);
> > +int xfs_qm_dqiterate(struct xfs_mount *mp, uint dqtype,
> > +		xfs_qm_dqiterate_fn iter_fn, void *priv);
> > +
> >  #endif /* __XFS_DQUOT_H__ */
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc

[Christoph Hellwig]

Looks good,

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

Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument

[Christoph Hellwig]

On Sun, Apr 29, 2018 at 10:44:11PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the _qm_dqread functionality to _qm_dqensure, then remove the flags
> argument from xfs_qm_dqread since the only DQALLOC users were internal
> to xfs_dquot.c anyway.

I really see no point in doing the wrapper.  I'd much rather keep
xfs_qm_dqread with a the additional argument, be that a flags value
or a bool.  For example the same patch with the new xfs_dquot_setup
kept as xfs_qm_dqread and everyone calling it with an additional
false argument would seem much nicer to me.

Re: [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean

[Christoph Hellwig]

Looks fine:

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

Re: [PATCH 12/13] xfs: rename on-disk dquot counter zap functions

[Christoph Hellwig]

Looks fine,

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

Re: [PATCH 13/13] xfs: refactor dquot iteration

[Christoph Hellwig]

On Tue, May 01, 2018 at 08:53:38AM -0700, Darrick J. Wong wrote:
> > With that fixed:
> 
> Fixed by moving the offset assignment up a few lines.

Can you resend the fixed patch?

[PATCH v2 13/13] xfs: refactor dquot iteration

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a helper function to iterate all the dquots of a given type in
the system, and refactor the dquot scrub to use it.  This will get more
use in the quota repair code.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
v2: fix compiler warning about uninitialized offset
---
 fs/xfs/scrub/quota.c |   56 +++++++++++++++++++++++---------------------------
 fs/xfs/xfs_dquot.c   |   32 +++++++++++++++++++++++++++++
 fs/xfs/xfs_dquot.h   |    5 ++++
 3 files changed, 63 insertions(+), 30 deletions(-)

diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index 50415e8e5dd1..ba87c3aaa8b7 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -77,14 +77,20 @@ xfs_scrub_setup_quota(
 
 /* Quotas. */
 
+struct xfs_scrub_quota_info {
+	struct xfs_scrub_context	*sc;
+	xfs_dqid_t			last_id;
+};
+
 /* Scrub the fields in an individual quota item. */
-STATIC void
+STATIC int
 xfs_scrub_quota_item(
-	struct xfs_scrub_context	*sc,
-	uint				dqtype,
 	struct xfs_dquot		*dq,
-	xfs_dqid_t			id)
+	uint				dqtype,
+	void				*priv)
 {
+	struct xfs_scrub_quota_info	*sqi = priv;
+	struct xfs_scrub_context	*sc = sqi->sc;
 	struct xfs_mount		*mp = sc->mp;
 	struct xfs_disk_dquot		*d = &dq->q_core;
 	struct xfs_quotainfo		*qi = mp->m_quotainfo;
@@ -99,17 +105,18 @@ xfs_scrub_quota_item(
 	unsigned long long		icount;
 	unsigned long long		rcount;
 	xfs_ino_t			fs_icount;
-
-	offset = id / qi->qi_dqperchunk;
+	xfs_dqid_t			id = be32_to_cpu(d->d_id);
 
 	/*
-	 * We fed $id and DQNEXT into the xfs_qm_dqget call, which means
-	 * that the actual dquot we got must either have the same id or
-	 * the next higher id.
+	 * Except for the root dquot, the actual dquot we got must either have
+	 * the same or higher id as we saw before.
 	 */
-	if (id > be32_to_cpu(d->d_id))
+	offset = id / qi->qi_dqperchunk;
+	if (id && id <= sqi->last_id)
 		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
 
+	sqi->last_id = id;
+
 	/* Did we get the dquot type we wanted? */
 	if (dqtype != (d->d_flags & XFS_DQ_ALLTYPES))
 		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
@@ -183,6 +190,8 @@ xfs_scrub_quota_item(
 		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
 	if (id != 0 && rhard != 0 && rcount > rhard)
 		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
+
+	return 0;
 }
 
 /* Scrub all of a quota type's items. */
@@ -191,13 +200,12 @@ xfs_scrub_quota(
 	struct xfs_scrub_context	*sc)
 {
 	struct xfs_bmbt_irec		irec = { 0 };
+	struct xfs_scrub_quota_info	sqi;
 	struct xfs_mount		*mp = sc->mp;
 	struct xfs_inode		*ip;
 	struct xfs_quotainfo		*qi = mp->m_quotainfo;
-	struct xfs_dquot		*dq;
 	xfs_fileoff_t			max_dqid_off;
 	xfs_fileoff_t			off = 0;
-	xfs_dqid_t			id = 0;
 	uint				dqtype;
 	int				nimaps;
 	int				error = 0;
@@ -264,24 +272,12 @@ xfs_scrub_quota(
 		goto out;
 
 	/* Check all the quota items. */
-	while (id < ((xfs_dqid_t)-1ULL)) {
-		if (xfs_scrub_should_terminate(sc, &error))
-			break;
-
-		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
-		if (error == -ENOENT)
-			break;
-		if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
-				id * qi->qi_dqperchunk, &error))
-			break;
-
-		xfs_scrub_quota_item(sc, dqtype, dq, id);
-
-		id = be32_to_cpu(dq->q_core.d_id) + 1;
-		xfs_qm_dqput(dq);
-		if (!id)
-			break;
-	}
+	sqi.sc = sc;
+	sqi.last_id = 0;
+	error = xfs_qm_dqiterate(mp, dqtype, xfs_scrub_quota_item, &sqi);
+	if (!xfs_scrub_fblock_process_error(sc, XFS_DATA_FORK,
+			sqi.last_id * qi->qi_dqperchunk, &error))
+		goto out;
 
 out:
 	/* We set sc->ip earlier, so make sure we clear it now. */
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 4b76ae00e69b..f8b8c99e3e73 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1212,3 +1212,35 @@ xfs_qm_exit(void)
 	kmem_zone_destroy(xfs_qm_dqtrxzone);
 	kmem_zone_destroy(xfs_qm_dqzone);
 }
+
+/*
+ * Iterate every dquot of a particular type.  The caller must ensure that the
+ * particular quota type is active.  iter_fn can return negative error codes,
+ * or XFS_BTREE_QUERY_RANGE_ABORT to indicate that it wants to stop iterating.
+ */
+int
+xfs_qm_dqiterate(
+	struct xfs_mount	*mp,
+	uint			dqtype,
+	xfs_qm_dqiterate_fn	iter_fn,
+	void			*priv)
+{
+	struct xfs_dquot	*dq;
+	xfs_dqid_t		id = 0;
+	int			error;
+
+	do {
+		error = xfs_qm_dqget_next(mp, id, dqtype, &dq);
+		if (error == -ENOENT)
+			return 0;
+		if (error)
+			return error;
+
+		error = iter_fn(dq, dqtype, priv);
+		id = be32_to_cpu(dq->q_core.d_id);
+		xfs_qm_dqput(dq);
+		id++;
+	} while (error == 0 && id != 0);
+
+	return error;
+}
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 6959f0135f36..417ac03b8d7b 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -193,4 +193,9 @@ static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
 	return dqp;
 }
 
+typedef int (*xfs_qm_dqiterate_fn)(struct xfs_dquot *dq, uint dqtype,
+		void *priv);
+int xfs_qm_dqiterate(struct xfs_mount *mp, uint dqtype,
+		xfs_qm_dqiterate_fn iter_fn, void *priv);
+
 #endif /* __XFS_DQUOT_H__ */

Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument

[Darrick J. Wong]

On Wed, May 02, 2018 at 06:34:31PM +0200, Christoph Hellwig wrote:
> On Sun, Apr 29, 2018 at 10:44:11PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Move the _qm_dqread functionality to _qm_dqensure, then remove the flags
> > argument from xfs_qm_dqread since the only DQALLOC users were internal
> > to xfs_dquot.c anyway.
> 
> I really see no point in doing the wrapper.  I'd much rather keep
> xfs_qm_dqread with a the additional argument, be that a flags value
> or a bool.  For example the same patch with the new xfs_dquot_setup
> kept as xfs_qm_dqread and everyone calling it with an additional
> false argument would seem much nicer to me.

I'm trying to avoid having a can_alloc argument for the exported _dqread
interface because none of the callers outside of xfs_dquot.c set it to
true.

Later on, in online repair I'll need some kind of interface to allocate
and get a dquot with a given transaction, but we can argue about that
when we get there.

--D

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

Re: [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc

[Darrick J. Wong]

On Sun, Apr 29, 2018 at 10:44:04PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Separate the disk dquot read and allocation functionality into
> two helper functions, then refactor dqread to call them directly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_dquot.c |  261 ++++++++++++++++++++--------------------------------
>  1 file changed, 98 insertions(+), 163 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index f1dc62ca54a7..d8730f6110b8 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -288,49 +288,43 @@ xfs_dquot_set_prealloc_limits(struct xfs_dquot *dqp)
>  }
>  
>  /*
> - * Allocate a block and fill it with dquots.
> - * This is called when the bmapi finds a hole.
> + * Ensure that the given in-core dquot has a buffer on disk backing it, and
> + * return the buffer. This is called when the bmapi finds a hole.
>   */
>  STATIC int
> -xfs_qm_dqalloc(
> -	xfs_trans_t	**tpp,
> -	xfs_mount_t	*mp,
> -	xfs_dquot_t	*dqp,
> -	xfs_inode_t	*quotip,
> -	xfs_fileoff_t	offset_fsb,
> -	xfs_buf_t	**O_bpp)
> +xfs_dquot_disk_alloc(
> +	struct xfs_trans	**tpp,
> +	struct xfs_dquot	*dqp,
> +	struct xfs_buf		**bpp)
>  {
> -	xfs_fsblock_t	firstblock;
> -	struct xfs_defer_ops dfops;
> -	xfs_bmbt_irec_t map;
> -	int		nmaps, error;
> -	xfs_buf_t	*bp;
> -	xfs_trans_t	*tp = *tpp;
> -
> -	ASSERT(tp != NULL);
> +	struct xfs_bmbt_irec	map;
> +	struct xfs_defer_ops	dfops;
> +	struct xfs_mount	*mp = (*tpp)->t_mountp;
> +	struct xfs_buf		*bp;
> +	struct xfs_inode	*quotip = xfs_quota_inode(mp, dqp->dq_flags);
> +	xfs_fsblock_t		firstblock;
> +	int			nmaps = 1;
> +	int			error;
>  
>  	trace_xfs_dqalloc(dqp);
>  
> -	/*
> -	 * Initialize the bmap freelist prior to calling bmapi code.
> -	 */
>  	xfs_defer_init(&dfops, &firstblock);
>  	xfs_ilock(quotip, XFS_ILOCK_EXCL);
> -	/*
> -	 * Return if this type of quotas is turned off while we didn't
> -	 * have an inode lock
> -	 */
>  	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
> +		/*
> +		 * Return if this type of quotas is turned off while we didn't
> +		 * have an inode lock
> +		 */
>  		xfs_iunlock(quotip, XFS_ILOCK_EXCL);
>  		return -ESRCH;
>  	}
>  
> -	xfs_trans_ijoin(tp, quotip, XFS_ILOCK_EXCL);
> -	nmaps = 1;
> -	error = xfs_bmapi_write(tp, quotip, offset_fsb,
> -				XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA,
> -				&firstblock, XFS_QM_DQALLOC_SPACE_RES(mp),
> -				&map, &nmaps, &dfops);
> +	/* Create the block mapping. */
> +	xfs_trans_ijoin(*tpp, quotip, XFS_ILOCK_EXCL);
> +	error = xfs_bmapi_write(*tpp, quotip, dqp->q_fileoffset,
> +			XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA,
> +			&firstblock, XFS_QM_DQALLOC_SPACE_RES(mp),
> +			&map, &nmaps, &dfops);
>  	if (error)
>  		goto error0;
>  	ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB);
> @@ -344,10 +338,8 @@ xfs_qm_dqalloc(
>  	dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
>  
>  	/* now we can just get the buffer (there's nothing to read yet) */
> -	bp = xfs_trans_get_buf(tp, mp->m_ddev_targp,
> -			       dqp->q_blkno,
> -			       mp->m_quotainfo->qi_dqchunklen,
> -			       0);
> +	bp = xfs_trans_get_buf(*tpp, mp->m_ddev_targp, dqp->q_blkno,
> +			mp->m_quotainfo->qi_dqchunklen, 0);
>  	if (!bp) {
>  		error = -ENOMEM;
>  		goto error1;
> @@ -358,37 +350,22 @@ xfs_qm_dqalloc(
>  	 * Make a chunk of dquots out of this buffer and log
>  	 * the entire thing.
>  	 */
> -	xfs_qm_init_dquot_blk(tp, mp, be32_to_cpu(dqp->q_core.d_id),
> +	xfs_qm_init_dquot_blk(*tpp, mp, be32_to_cpu(dqp->q_core.d_id),
>  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
> +	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
>  
>  	/*
> -	 * xfs_defer_finish() may commit the current transaction and
> -	 * start a second transaction if the freelist is not empty.
> -	 *
> -	 * Since we still want to modify this buffer, we need to
> -	 * ensure that the buffer is not released on commit of
> -	 * the first transaction and ensure the buffer is added to the
> -	 * second transaction.
> -	 *
> -	 * If there is only one transaction then don't stop the buffer
> -	 * from being released when it commits later on.
> +	 * Hold the buffer and join it to the dfops so that we'll still own
> +	 * the buffer when we return to the caller.
>  	 */
> -
> -	xfs_trans_bhold(tp, bp);
> -
> +	xfs_trans_bhold(*tpp, bp);
> +	error = xfs_defer_bjoin(&dfops, bp);
> +	if (error)
> +		goto error1;
>  	error = xfs_defer_finish(tpp, &dfops);
>  	if (error)
>  		goto error1;

Sigh... this is wrong.  We allocated a dquot block and get_buf'd it to
initialize the dquot buffer.  If the defer_finish fails we error out,
leaving the locked buffer dangling.  This seems to have been introduced
in commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
allocating a new dquot record" back in 2005.  The leaked locked buffer
would seem to be the cause of the only-when-quota generic/388 hangs that
I was complaining about at LSF last week.

The _defer_bjoin compounds this mistake further by re-bhold'ing the
buffer after each transaction roll -- if the defer_finish succeeds but
the subsequent trans_commit fails, we again leak the locked buffer.

Will repost this patch with all this fixed.

--D

> -
> -	/* Transaction was committed? */
> -	if (*tpp != tp) {
> -		tp = *tpp;
> -		xfs_trans_bjoin(tp, bp);
> -	} else {
> -		xfs_trans_bhold_release(tp, bp);
> -	}
> -
> -	*O_bpp = bp;
> +	*bpp = bp;
>  	return 0;
>  
>  error1:
> @@ -398,32 +375,24 @@ xfs_qm_dqalloc(
>  }
>  
>  /*
> - * Maps a dquot to the buffer containing its on-disk version.
> - * This returns a ptr to the buffer containing the on-disk dquot
> - * in the bpp param, and a ptr to the on-disk dquot within that buffer
> + * Read in the in-core dquot's on-disk metadata and return the buffer.
> + * Returns ENOENT to signal a hole.
>   */
>  STATIC int
> -xfs_qm_dqtobp(
> -	xfs_trans_t		**tpp,
> -	xfs_dquot_t		*dqp,
> -	xfs_disk_dquot_t	**O_ddpp,
> -	xfs_buf_t		**O_bpp,
> -	uint			flags)
> +xfs_dquot_disk_read(
> +	struct xfs_mount	*mp,
> +	struct xfs_dquot	*dqp,
> +	struct xfs_buf		**bpp)
>  {
>  	struct xfs_bmbt_irec	map;
> -	int			nmaps = 1, error;
>  	struct xfs_buf		*bp;
> -	struct xfs_inode	*quotip;
> -	struct xfs_mount	*mp = dqp->q_mount;
> -	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
> -	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
> +	struct xfs_inode	*quotip = xfs_quota_inode(mp, dqp->dq_flags);
>  	uint			lock_mode;
> -
> -	quotip = xfs_quota_inode(dqp->q_mount, dqp->dq_flags);
> -	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
> +	int			nmaps = 1;
> +	int			error;
>  
>  	lock_mode = xfs_ilock_data_map_shared(quotip);
> -	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
> +	if (!xfs_this_quota_on(mp, dqp->dq_flags)) {
>  		/*
>  		 * Return if this type of quotas is turned off while we
>  		 * didn't have the quota inode lock.
> @@ -436,57 +405,36 @@ xfs_qm_dqtobp(
>  	 * Find the block map; no allocations yet
>  	 */
>  	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
> -			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
> -
> +			XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
>  	xfs_iunlock(quotip, lock_mode);
>  	if (error)
>  		return error;
>  
>  	ASSERT(nmaps == 1);
> -	ASSERT(map.br_blockcount == 1);
> +	ASSERT(map.br_blockcount >= 1);
> +	ASSERT(map.br_startblock != DELAYSTARTBLOCK);
> +	if (map.br_startblock == HOLESTARTBLOCK)
> +		return -ENOENT;
> +
> +	trace_xfs_dqtobp_read(dqp);
>  
>  	/*
> -	 * Offset of dquot in the (fixed sized) dquot chunk.
> +	 * store the blkno etc so that we don't have to do the
> +	 * mapping all the time
>  	 */
> -	dqp->q_bufoffset = (id % mp->m_quotainfo->qi_dqperchunk) *
> -		sizeof(xfs_dqblk_t);
> -
> -	ASSERT(map.br_startblock != DELAYSTARTBLOCK);
> -	if (map.br_startblock == HOLESTARTBLOCK) {
> -		/*
> -		 * We don't allocate unless we're asked to
> -		 */
> -		if (!(flags & XFS_QMOPT_DQALLOC))
> -			return -ENOENT;
> -
> -		ASSERT(tp);
> -		error = xfs_qm_dqalloc(tpp, mp, dqp, quotip,
> -					dqp->q_fileoffset, &bp);
> -		if (error)
> -			return error;
> -		tp = *tpp;
> -	} else {
> -		trace_xfs_dqtobp_read(dqp);
> +	dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
>  
> -		/*
> -		 * store the blkno etc so that we don't have to do the
> -		 * mapping all the time
> -		 */
> -		dqp->q_blkno = XFS_FSB_TO_DADDR(mp, map.br_startblock);
> -
> -		error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
> -					   dqp->q_blkno,
> -					   mp->m_quotainfo->qi_dqchunklen,
> -					   0, &bp, &xfs_dquot_buf_ops);
> -		if (error) {
> -			ASSERT(bp == NULL);
> -			return error;
> -		}
> +	error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp, dqp->q_blkno,
> +			mp->m_quotainfo->qi_dqchunklen, 0, &bp,
> +			&xfs_dquot_buf_ops);
> +	if (error) {
> +		ASSERT(bp == NULL);
> +		return error;
>  	}
>  
>  	ASSERT(xfs_buf_islocked(bp));
> -	*O_bpp = bp;
> -	*O_ddpp = bp->b_addr + dqp->q_bufoffset;
> +	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
> +	*bpp = bp;
>  
>  	return 0;
>  }
> @@ -508,6 +456,12 @@ xfs_dquot_alloc(
>  	INIT_LIST_HEAD(&dqp->q_lru);
>  	mutex_init(&dqp->q_qlock);
>  	init_waitqueue_head(&dqp->q_pinwait);
> +	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
> +	/*
> +	 * Offset of dquot in the (fixed sized) dquot chunk.
> +	 */
> +	dqp->q_bufoffset = (id % mp->m_quotainfo->qi_dqperchunk) *
> +			sizeof(xfs_dqblk_t);
>  
>  	/*
>  	 * Because we want to use a counting completion, complete
> @@ -546,8 +500,10 @@ xfs_dquot_alloc(
>  STATIC void
>  xfs_dquot_from_disk(
>  	struct xfs_dquot	*dqp,
> -	struct xfs_disk_dquot	*ddqp)
> +	struct xfs_buf		*bp)
>  {
> +	struct xfs_disk_dquot	*ddqp = bp->b_addr + dqp->q_bufoffset;
> +
>  	/* copy everything from disk dquot to the incore dquot */
>  	memcpy(&dqp->q_core, ddqp, sizeof(xfs_disk_dquot_t));
>  
> @@ -575,74 +531,53 @@ xfs_qm_dqread(
>  	xfs_dqid_t		id,
>  	uint			type,
>  	uint			flags,
> -	struct xfs_dquot	**O_dqpp)
> +	struct xfs_dquot	**dqpp)
>  {
>  	struct xfs_dquot	*dqp;
> -	struct xfs_disk_dquot	*ddqp;
>  	struct xfs_buf		*bp;
> -	struct xfs_trans	*tp = NULL;
> +	struct xfs_trans	*tp;
>  	int			error;
>  
>  	dqp = xfs_dquot_alloc(mp, id, type);
>  	trace_xfs_dqread(dqp);
>  
> -	if (flags & XFS_QMOPT_DQALLOC) {
> +	/* Try to read the buffer... */
> +	error = xfs_dquot_disk_read(mp, dqp, &bp);
> +	if (error == -ENOENT && (flags & XFS_QMOPT_DQALLOC)) {
> +		/* ...or allocate a new block and buffer. */
>  		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc,
>  				XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp);
>  		if (error)
> -			goto error0;
> -	}
> +			goto err;
>  
> -	/*
> -	 * get a pointer to the on-disk dquot and the buffer containing it
> -	 * dqp already knows its own type (GROUP/USER).
> -	 */
> -	error = xfs_qm_dqtobp(&tp, dqp, &ddqp, &bp, flags);
> -	if (error) {
> -		/*
> -		 * This can happen if quotas got turned off (ESRCH),
> -		 * or if the dquot didn't exist on disk and we ask to
> -		 * allocate (ENOENT).
> -		 */
> -		trace_xfs_dqread_fail(dqp);
> -		goto error1;
> -	}
> -
> -	xfs_dquot_from_disk(dqp, ddqp);
> +		error = xfs_dquot_disk_alloc(&tp, dqp, &bp);
> +		if (error)
> +			goto err_cancel;
>  
> -	/* Mark the buf so that this will stay incore a little longer */
> -	xfs_buf_set_ref(bp, XFS_DQUOT_REF);
> +		error = xfs_trans_commit(tp);
> +	}
> +	if (error)
> +		goto err;
>  
>  	/*
> -	 * We got the buffer with a xfs_trans_read_buf() (in dqtobp())
> -	 * So we need to release with xfs_trans_brelse().
> -	 * The strategy here is identical to that of inodes; we lock
> -	 * the dquot in xfs_qm_dqget() before making it accessible to
> -	 * others. This is because dquots, like inodes, need a good level of
> -	 * concurrency, and we don't want to take locks on the entire buffers
> -	 * for dquot accesses.
> -	 * Note also that the dquot buffer may even be dirty at this point, if
> -	 * this particular dquot was repaired. We still aren't afraid to
> -	 * brelse it because we have the changes incore.
> +	 * At this point we should have a clean locked buffer.  Copy the data
> +	 * to the incore dquot and release the buffer since the incore dquot
> +	 * has its own locking protocol so we needn't tie up the buffer any
> +	 * further.
>  	 */
>  	ASSERT(xfs_buf_islocked(bp));
> -	xfs_trans_brelse(tp, bp);
> +	xfs_dquot_from_disk(dqp, bp);
>  
> -	if (tp) {
> -		error = xfs_trans_commit(tp);
> -		if (error)
> -			goto error0;
> -	}
> -
> -	*O_dqpp = dqp;
> +	xfs_buf_relse(bp);
> +	*dqpp = dqp;
>  	return error;
>  
> -error1:
> -	if (tp)
> -		xfs_trans_cancel(tp);
> -error0:
> +err_cancel:
> +	xfs_trans_cancel(tp);
> +err:
> +	trace_xfs_dqread_fail(dqp);
>  	xfs_qm_dqdestroy(dqp);
> -	*O_dqpp = NULL;
> +	*dqpp = NULL;
>  	return error;
>  }
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
allocating a new dquot record", we allocate a new dquot block, grab a
buffer to initialize it, and return the locked initialized dquot buffer
to the caller for further in-core dquot initialization.  Unfortunately,
if the _bmap_finish errored out, _qm_dqalloc would also error out
without bothering to free the (locked) buffer.  Leaking a locked buffer
caused hangs in generic/388 when quotas are enabled.

Furthermore, the _bmap_finish -> _defer_finish conversion in
310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
xfs_defer_*") failed to observe that the buffer was held going into
_defer_finish and therefore failed to notice that the buffer lock is
/not/ maintained afterwards.  Now that we can bjoin a buffer to a
defer_ops, use this mechanism to ensure that the buffer stays locked
across the _defer_finish.  Release the holds and locks on the buffer as
appropriate if we have to error out.

There is a subtlety here for the caller in that the buffer emerges
locked and held to the transaction, so if the _trans_commit fails we
have to release the buffer explicitly.  This fixes the unmount hang
in generic/388 when quotas are enabled.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_dquot.c |   48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index a7daef9e16bf..4c39d8632230 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -362,33 +362,39 @@ xfs_qm_dqalloc(
 			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
 
 	/*
-	 * xfs_defer_finish() may commit the current transaction and
-	 * start a second transaction if the freelist is not empty.
+	 * Hold the buffer and join it to the dfops so that we'll still own
+	 * the buffer when we return to the caller.  The buffer disposal on
+	 * error must be paid attention to very carefully, as it has been
+	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
+	 * code when allocating a new dquot record" in 2005, and the later
+	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
+	 * the buffer locked across the _defer_finish call.  We can now do
+	 * this correctly with xfs_defer_bjoin.
 	 *
-	 * Since we still want to modify this buffer, we need to
-	 * ensure that the buffer is not released on commit of
-	 * the first transaction and ensure the buffer is added to the
-	 * second transaction.
+	 * Above, we allocated a disk block for the dquot information and
+	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
+	 * the buffer is still locked to *tpp, so we must _bhold_release and
+	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
+	 * transaction is gone but the new buffer is not joined or held to any
+	 * transaction, so we must _buf_relse it.
 	 *
-	 * If there is only one transaction then don't stop the buffer
-	 * from being released when it commits later on.
+	 * If everything succeeds, the caller of this function is returned a
+	 * buffer that is locked, held, and joined to the transaction.  If the
+	 * transaction commit fails (in the caller) the caller must unlock the
+	 * buffer manually.
 	 */
-
-	xfs_trans_bhold(tp, bp);
-
+	xfs_trans_bhold(*tpp, bp);
+	error = xfs_defer_bjoin(&dfops, bp);
+	if (error) {
+		xfs_trans_bhold_release(*tpp, bp);
+		xfs_trans_brelse(*tpp, bp);
+		goto error1;
+	}
 	error = xfs_defer_finish(tpp, &dfops);
-	if (error)
+	if (error) {
+		xfs_buf_relse(bp);
 		goto error1;
-
-	/* Transaction was committed? */
-	if (*tpp != tp) {
-		tp = *tpp;
-		xfs_trans_bjoin(tp, bp);
-	} else {
-		xfs_trans_bhold_release(tp, bp);
 	}
-
-	*O_bpp = bp;
 	return 0;
 
 error1:

[PATCH 0.2/13] xfs: don't spray logs when dquot flush/purge fail

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

When dquot flush or purge fail there's no need to spam the logs, we've
already logged the IO error or fs shutdown that caused the flush
failures.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_dquot_item.c |    5 +----
 fs/xfs/xfs_qm.c         |   10 ++--------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 4b331e354da7..d632ba403527 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -209,10 +209,7 @@ xfs_qm_dquot_logitem_push(
 	spin_unlock(&lip->li_ailp->ail_lock);
 
 	error = xfs_qm_dqflush(dqp, &bp);
-	if (error) {
-		xfs_warn(dqp->q_mount, "%s: push error %d on dqp "PTR_FMT,
-			__func__, error, dqp);
-	} else {
+	if (!error) {
 		if (!xfs_buf_delwri_queue(bp, buffer_list))
 			rval = XFS_ITEM_FLUSHING;
 		xfs_buf_relse(bp);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index ec39ae274c78..579de656bc25 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -161,10 +161,7 @@ xfs_qm_dqpurge(
 		 * to purge this dquot anyway, so we go ahead regardless.
 		 */
 		error = xfs_qm_dqflush(dqp, &bp);
-		if (error) {
-			xfs_warn(mp, "%s: dquot "PTR_FMT" flush failed",
-				__func__, dqp);
-		} else {
+		if (!error) {
 			error = xfs_bwrite(bp);
 			xfs_buf_relse(bp);
 		}
@@ -479,11 +476,8 @@ xfs_qm_dquot_isolate(
 		spin_unlock(lru_lock);
 
 		error = xfs_qm_dqflush(dqp, &bp);
-		if (error) {
-			xfs_warn(dqp->q_mount, "%s: dquot "PTR_FMT" flush failed",
-				 __func__, dqp);
+		if (error)
 			goto out_unlock_dirty;
-		}
 
 		xfs_buf_delwri_queue(bp, &isol->buffers);
 		xfs_buf_relse(bp);

Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error

[Brian Foster]

On Thu, May 03, 2018 at 10:53:43AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
> allocating a new dquot record", we allocate a new dquot block, grab a
> buffer to initialize it, and return the locked initialized dquot buffer
> to the caller for further in-core dquot initialization.  Unfortunately,
> if the _bmap_finish errored out, _qm_dqalloc would also error out
> without bothering to free the (locked) buffer.  Leaking a locked buffer
> caused hangs in generic/388 when quotas are enabled.
> 
> Furthermore, the _bmap_finish -> _defer_finish conversion in
> 310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
> xfs_defer_*") failed to observe that the buffer was held going into
> _defer_finish and therefore failed to notice that the buffer lock is
> /not/ maintained afterwards.  Now that we can bjoin a buffer to a
> defer_ops, use this mechanism to ensure that the buffer stays locked
> across the _defer_finish.  Release the holds and locks on the buffer as
> appropriate if we have to error out.
> 
> There is a subtlety here for the caller in that the buffer emerges
> locked and held to the transaction, so if the _trans_commit fails we
> have to release the buffer explicitly.  This fixes the unmount hang
> in generic/388 when quotas are enabled.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_dquot.c |   48 +++++++++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index a7daef9e16bf..4c39d8632230 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -362,33 +362,39 @@ xfs_qm_dqalloc(
>  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
>  
>  	/*
> -	 * xfs_defer_finish() may commit the current transaction and
> -	 * start a second transaction if the freelist is not empty.
> +	 * Hold the buffer and join it to the dfops so that we'll still own
> +	 * the buffer when we return to the caller.  The buffer disposal on
> +	 * error must be paid attention to very carefully, as it has been
> +	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
> +	 * code when allocating a new dquot record" in 2005, and the later
> +	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
> +	 * the buffer locked across the _defer_finish call.  We can now do
> +	 * this correctly with xfs_defer_bjoin.
>  	 *
> -	 * Since we still want to modify this buffer, we need to
> -	 * ensure that the buffer is not released on commit of
> -	 * the first transaction and ensure the buffer is added to the
> -	 * second transaction.
> +	 * Above, we allocated a disk block for the dquot information and
> +	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
> +	 * the buffer is still locked to *tpp, so we must _bhold_release and
> +	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
> +	 * transaction is gone but the new buffer is not joined or held to any
> +	 * transaction, so we must _buf_relse it.
>  	 *
> -	 * If there is only one transaction then don't stop the buffer
> -	 * from being released when it commits later on.
> +	 * If everything succeeds, the caller of this function is returned a
> +	 * buffer that is locked, held, and joined to the transaction.  If the
> +	 * transaction commit fails (in the caller) the caller must unlock the
> +	 * buffer manually.

If the buffer is held due to the xfs_defer_bjoin(), doesn't that mean
that the caller has to ultimately release it even after successful
transaction commit (assuming we don't roll the transaction again
somewhere)? I see we have an xfs_trans_brelse() up in xfs_qm_dqread(),
but it looks like that only clears the hold if the buffer isn't logged
in the tx. Hm?

Brian

>  	 */
> -
> -	xfs_trans_bhold(tp, bp);
> -
> +	xfs_trans_bhold(*tpp, bp);
> +	error = xfs_defer_bjoin(&dfops, bp);
> +	if (error) {
> +		xfs_trans_bhold_release(*tpp, bp);
> +		xfs_trans_brelse(*tpp, bp);
> +		goto error1;
> +	}
>  	error = xfs_defer_finish(tpp, &dfops);
> -	if (error)
> +	if (error) {
> +		xfs_buf_relse(bp);
>  		goto error1;
> -
> -	/* Transaction was committed? */
> -	if (*tpp != tp) {
> -		tp = *tpp;
> -		xfs_trans_bjoin(tp, bp);
> -	} else {
> -		xfs_trans_bhold_release(tp, bp);
>  	}
> -
> -	*O_bpp = bp;
>  	return 0;
>  
>  error1:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0.2/13] xfs: don't spray logs when dquot flush/purge fail

[Brian Foster]

On Thu, May 03, 2018 at 10:54:09AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When dquot flush or purge fail there's no need to spam the logs, we've
> already logged the IO error or fs shutdown that caused the flush
> failures.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

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

>  fs/xfs/xfs_dquot_item.c |    5 +----
>  fs/xfs/xfs_qm.c         |   10 ++--------
>  2 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 4b331e354da7..d632ba403527 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -209,10 +209,7 @@ xfs_qm_dquot_logitem_push(
>  	spin_unlock(&lip->li_ailp->ail_lock);
>  
>  	error = xfs_qm_dqflush(dqp, &bp);
> -	if (error) {
> -		xfs_warn(dqp->q_mount, "%s: push error %d on dqp "PTR_FMT,
> -			__func__, error, dqp);
> -	} else {
> +	if (!error) {
>  		if (!xfs_buf_delwri_queue(bp, buffer_list))
>  			rval = XFS_ITEM_FLUSHING;
>  		xfs_buf_relse(bp);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index ec39ae274c78..579de656bc25 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -161,10 +161,7 @@ xfs_qm_dqpurge(
>  		 * to purge this dquot anyway, so we go ahead regardless.
>  		 */
>  		error = xfs_qm_dqflush(dqp, &bp);
> -		if (error) {
> -			xfs_warn(mp, "%s: dquot "PTR_FMT" flush failed",
> -				__func__, dqp);
> -		} else {
> +		if (!error) {
>  			error = xfs_bwrite(bp);
>  			xfs_buf_relse(bp);
>  		}
> @@ -479,11 +476,8 @@ xfs_qm_dquot_isolate(
>  		spin_unlock(lru_lock);
>  
>  		error = xfs_qm_dqflush(dqp, &bp);
> -		if (error) {
> -			xfs_warn(dqp->q_mount, "%s: dquot "PTR_FMT" flush failed",
> -				 __func__, dqp);
> +		if (error)
>  			goto out_unlock_dirty;
> -		}
>  
>  		xfs_buf_delwri_queue(bp, &isol->buffers);
>  		xfs_buf_relse(bp);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error

[Darrick J. Wong]

On Fri, May 04, 2018 at 07:31:58AM -0400, Brian Foster wrote:
> On Thu, May 03, 2018 at 10:53:43AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
> > allocating a new dquot record", we allocate a new dquot block, grab a
> > buffer to initialize it, and return the locked initialized dquot buffer
> > to the caller for further in-core dquot initialization.  Unfortunately,
> > if the _bmap_finish errored out, _qm_dqalloc would also error out
> > without bothering to free the (locked) buffer.  Leaking a locked buffer
> > caused hangs in generic/388 when quotas are enabled.
> > 
> > Furthermore, the _bmap_finish -> _defer_finish conversion in
> > 310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
> > xfs_defer_*") failed to observe that the buffer was held going into
> > _defer_finish and therefore failed to notice that the buffer lock is
> > /not/ maintained afterwards.  Now that we can bjoin a buffer to a
> > defer_ops, use this mechanism to ensure that the buffer stays locked
> > across the _defer_finish.  Release the holds and locks on the buffer as
> > appropriate if we have to error out.
> > 
> > There is a subtlety here for the caller in that the buffer emerges
> > locked and held to the transaction, so if the _trans_commit fails we
> > have to release the buffer explicitly.  This fixes the unmount hang
> > in generic/388 when quotas are enabled.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_dquot.c |   48 +++++++++++++++++++++++++++---------------------
> >  1 file changed, 27 insertions(+), 21 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index a7daef9e16bf..4c39d8632230 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -362,33 +362,39 @@ xfs_qm_dqalloc(
> >  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
> >  
> >  	/*
> > -	 * xfs_defer_finish() may commit the current transaction and
> > -	 * start a second transaction if the freelist is not empty.
> > +	 * Hold the buffer and join it to the dfops so that we'll still own
> > +	 * the buffer when we return to the caller.  The buffer disposal on
> > +	 * error must be paid attention to very carefully, as it has been
> > +	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
> > +	 * code when allocating a new dquot record" in 2005, and the later
> > +	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
> > +	 * the buffer locked across the _defer_finish call.  We can now do
> > +	 * this correctly with xfs_defer_bjoin.
> >  	 *
> > -	 * Since we still want to modify this buffer, we need to
> > -	 * ensure that the buffer is not released on commit of
> > -	 * the first transaction and ensure the buffer is added to the
> > -	 * second transaction.
> > +	 * Above, we allocated a disk block for the dquot information and
> > +	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
> > +	 * the buffer is still locked to *tpp, so we must _bhold_release and
> > +	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
> > +	 * transaction is gone but the new buffer is not joined or held to any
> > +	 * transaction, so we must _buf_relse it.
> >  	 *
> > -	 * If there is only one transaction then don't stop the buffer
> > -	 * from being released when it commits later on.
> > +	 * If everything succeeds, the caller of this function is returned a
> > +	 * buffer that is locked, held, and joined to the transaction.  If the
> > +	 * transaction commit fails (in the caller) the caller must unlock the
> > +	 * buffer manually.
> 
> If the buffer is held due to the xfs_defer_bjoin(), doesn't that mean
> that the caller has to ultimately release it even after successful
> transaction commit (assuming we don't roll the transaction again
> somewhere)? I see we have an xfs_trans_brelse() up in xfs_qm_dqread(),
> but it looks like that only clears the hold if the buffer isn't logged
> in the tx. Hm?

Correct.  The buffer is initialized in the same transaction as the dquot
block allocation and committed in xfs_defer_finish.  After
initialization (which is to say when we return to xfs_qm_dqtobp), the
buffer is held, joined, and not logged to the transaction, and nothing
else is supposed to dirty the buffer.  Both buffer and transaction are
then returned in this state to _dqread, which the in-core dquot state
out of the dquot buffer and _trans_brelse's the (still clean) buffer,
which breaks the hold and unlocks the buffer.

After the refactor we guarantee that the buffer is locked, clean, and
not attached to a transaction by the time we get to calling
xfs_dquot_from_disk rather than returning transaction and buffer up the
call stack and having to reason up the stack about what state they're in.

--D

> Brian
> 
> >  	 */
> > -
> > -	xfs_trans_bhold(tp, bp);
> > -
> > +	xfs_trans_bhold(*tpp, bp);
> > +	error = xfs_defer_bjoin(&dfops, bp);
> > +	if (error) {
> > +		xfs_trans_bhold_release(*tpp, bp);
> > +		xfs_trans_brelse(*tpp, bp);
> > +		goto error1;
> > +	}
> >  	error = xfs_defer_finish(tpp, &dfops);
> > -	if (error)
> > +	if (error) {
> > +		xfs_buf_relse(bp);
> >  		goto error1;
> > -
> > -	/* Transaction was committed? */
> > -	if (*tpp != tp) {
> > -		tp = *tpp;
> > -		xfs_trans_bjoin(tp, bp);
> > -	} else {
> > -		xfs_trans_bhold_release(tp, bp);
> >  	}
> > -
> > -	*O_bpp = bp;
> >  	return 0;
> >  
> >  error1:
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error

[Brian Foster]

On Fri, May 04, 2018 at 08:12:47AM -0700, Darrick J. Wong wrote:
> On Fri, May 04, 2018 at 07:31:58AM -0400, Brian Foster wrote:
> > On Thu, May 03, 2018 at 10:53:43AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
> > > allocating a new dquot record", we allocate a new dquot block, grab a
> > > buffer to initialize it, and return the locked initialized dquot buffer
> > > to the caller for further in-core dquot initialization.  Unfortunately,
> > > if the _bmap_finish errored out, _qm_dqalloc would also error out
> > > without bothering to free the (locked) buffer.  Leaking a locked buffer
> > > caused hangs in generic/388 when quotas are enabled.
> > > 
> > > Furthermore, the _bmap_finish -> _defer_finish conversion in
> > > 310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
> > > xfs_defer_*") failed to observe that the buffer was held going into
> > > _defer_finish and therefore failed to notice that the buffer lock is
> > > /not/ maintained afterwards.  Now that we can bjoin a buffer to a
> > > defer_ops, use this mechanism to ensure that the buffer stays locked
> > > across the _defer_finish.  Release the holds and locks on the buffer as
> > > appropriate if we have to error out.
> > > 
> > > There is a subtlety here for the caller in that the buffer emerges
> > > locked and held to the transaction, so if the _trans_commit fails we
> > > have to release the buffer explicitly.  This fixes the unmount hang
> > > in generic/388 when quotas are enabled.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/xfs_dquot.c |   48 +++++++++++++++++++++++++++---------------------
> > >  1 file changed, 27 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > > index a7daef9e16bf..4c39d8632230 100644
> > > --- a/fs/xfs/xfs_dquot.c
> > > +++ b/fs/xfs/xfs_dquot.c
> > > @@ -362,33 +362,39 @@ xfs_qm_dqalloc(
> > >  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
> > >  
> > >  	/*
> > > -	 * xfs_defer_finish() may commit the current transaction and
> > > -	 * start a second transaction if the freelist is not empty.
> > > +	 * Hold the buffer and join it to the dfops so that we'll still own
> > > +	 * the buffer when we return to the caller.  The buffer disposal on
> > > +	 * error must be paid attention to very carefully, as it has been
> > > +	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
> > > +	 * code when allocating a new dquot record" in 2005, and the later
> > > +	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
> > > +	 * the buffer locked across the _defer_finish call.  We can now do
> > > +	 * this correctly with xfs_defer_bjoin.
> > >  	 *
> > > -	 * Since we still want to modify this buffer, we need to
> > > -	 * ensure that the buffer is not released on commit of
> > > -	 * the first transaction and ensure the buffer is added to the
> > > -	 * second transaction.
> > > +	 * Above, we allocated a disk block for the dquot information and
> > > +	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
> > > +	 * the buffer is still locked to *tpp, so we must _bhold_release and
> > > +	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
> > > +	 * transaction is gone but the new buffer is not joined or held to any
> > > +	 * transaction, so we must _buf_relse it.
> > >  	 *
> > > -	 * If there is only one transaction then don't stop the buffer
> > > -	 * from being released when it commits later on.
> > > +	 * If everything succeeds, the caller of this function is returned a
> > > +	 * buffer that is locked, held, and joined to the transaction.  If the
> > > +	 * transaction commit fails (in the caller) the caller must unlock the
> > > +	 * buffer manually.
> > 
> > If the buffer is held due to the xfs_defer_bjoin(), doesn't that mean
> > that the caller has to ultimately release it even after successful
> > transaction commit (assuming we don't roll the transaction again
> > somewhere)? I see we have an xfs_trans_brelse() up in xfs_qm_dqread(),
> > but it looks like that only clears the hold if the buffer isn't logged
> > in the tx. Hm?
> 
> Correct.  The buffer is initialized in the same transaction as the dquot
> block allocation and committed in xfs_defer_finish.  After
> initialization (which is to say when we return to xfs_qm_dqtobp), the
> buffer is held, joined, and not logged to the transaction, and nothing
> else is supposed to dirty the buffer.  Both buffer and transaction are
> then returned in this state to _dqread, which the in-core dquot state
> out of the dquot buffer and _trans_brelse's the (still clean) buffer,
> which breaks the hold and unlocks the buffer.
> 

Ok that makes sense, but doesn't that depend on having a deferred
operation? Is that always guaranteed here?

Brian

> After the refactor we guarantee that the buffer is locked, clean, and
> not attached to a transaction by the time we get to calling
> xfs_dquot_from_disk rather than returning transaction and buffer up the
> call stack and having to reason up the stack about what state they're in.
> 
> --D
> 
> > Brian
> > 
> > >  	 */
> > > -
> > > -	xfs_trans_bhold(tp, bp);
> > > -
> > > +	xfs_trans_bhold(*tpp, bp);
> > > +	error = xfs_defer_bjoin(&dfops, bp);
> > > +	if (error) {
> > > +		xfs_trans_bhold_release(*tpp, bp);
> > > +		xfs_trans_brelse(*tpp, bp);
> > > +		goto error1;
> > > +	}
> > >  	error = xfs_defer_finish(tpp, &dfops);
> > > -	if (error)
> > > +	if (error) {
> > > +		xfs_buf_relse(bp);
> > >  		goto error1;
> > > -
> > > -	/* Transaction was committed? */
> > > -	if (*tpp != tp) {
> > > -		tp = *tpp;
> > > -		xfs_trans_bjoin(tp, bp);
> > > -	} else {
> > > -		xfs_trans_bhold_release(tp, bp);
> > >  	}
> > > -
> > > -	*O_bpp = bp;
> > >  	return 0;
> > >  
> > >  error1:
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error

[Darrick J. Wong]

On Fri, May 04, 2018 at 11:41:21AM -0400, Brian Foster wrote:
> On Fri, May 04, 2018 at 08:12:47AM -0700, Darrick J. Wong wrote:
> > On Fri, May 04, 2018 at 07:31:58AM -0400, Brian Foster wrote:
> > > On Thu, May 03, 2018 at 10:53:43AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
> > > > allocating a new dquot record", we allocate a new dquot block, grab a
> > > > buffer to initialize it, and return the locked initialized dquot buffer
> > > > to the caller for further in-core dquot initialization.  Unfortunately,
> > > > if the _bmap_finish errored out, _qm_dqalloc would also error out
> > > > without bothering to free the (locked) buffer.  Leaking a locked buffer
> > > > caused hangs in generic/388 when quotas are enabled.
> > > > 
> > > > Furthermore, the _bmap_finish -> _defer_finish conversion in
> > > > 310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
> > > > xfs_defer_*") failed to observe that the buffer was held going into
> > > > _defer_finish and therefore failed to notice that the buffer lock is
> > > > /not/ maintained afterwards.  Now that we can bjoin a buffer to a
> > > > defer_ops, use this mechanism to ensure that the buffer stays locked
> > > > across the _defer_finish.  Release the holds and locks on the buffer as
> > > > appropriate if we have to error out.
> > > > 
> > > > There is a subtlety here for the caller in that the buffer emerges
> > > > locked and held to the transaction, so if the _trans_commit fails we
> > > > have to release the buffer explicitly.  This fixes the unmount hang
> > > > in generic/388 when quotas are enabled.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  fs/xfs/xfs_dquot.c |   48 +++++++++++++++++++++++++++---------------------
> > > >  1 file changed, 27 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > > > index a7daef9e16bf..4c39d8632230 100644
> > > > --- a/fs/xfs/xfs_dquot.c
> > > > +++ b/fs/xfs/xfs_dquot.c
> > > > @@ -362,33 +362,39 @@ xfs_qm_dqalloc(
> > > >  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
> > > >  
> > > >  	/*
> > > > -	 * xfs_defer_finish() may commit the current transaction and
> > > > -	 * start a second transaction if the freelist is not empty.
> > > > +	 * Hold the buffer and join it to the dfops so that we'll still own
> > > > +	 * the buffer when we return to the caller.  The buffer disposal on
> > > > +	 * error must be paid attention to very carefully, as it has been
> > > > +	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
> > > > +	 * code when allocating a new dquot record" in 2005, and the later
> > > > +	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
> > > > +	 * the buffer locked across the _defer_finish call.  We can now do
> > > > +	 * this correctly with xfs_defer_bjoin.
> > > >  	 *
> > > > -	 * Since we still want to modify this buffer, we need to
> > > > -	 * ensure that the buffer is not released on commit of
> > > > -	 * the first transaction and ensure the buffer is added to the
> > > > -	 * second transaction.
> > > > +	 * Above, we allocated a disk block for the dquot information and
> > > > +	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
> > > > +	 * the buffer is still locked to *tpp, so we must _bhold_release and
> > > > +	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
> > > > +	 * transaction is gone but the new buffer is not joined or held to any
> > > > +	 * transaction, so we must _buf_relse it.
> > > >  	 *
> > > > -	 * If there is only one transaction then don't stop the buffer
> > > > -	 * from being released when it commits later on.
> > > > +	 * If everything succeeds, the caller of this function is returned a
> > > > +	 * buffer that is locked, held, and joined to the transaction.  If the
> > > > +	 * transaction commit fails (in the caller) the caller must unlock the
> > > > +	 * buffer manually.
> > > 
> > > If the buffer is held due to the xfs_defer_bjoin(), doesn't that mean
> > > that the caller has to ultimately release it even after successful
> > > transaction commit (assuming we don't roll the transaction again
> > > somewhere)? I see we have an xfs_trans_brelse() up in xfs_qm_dqread(),
> > > but it looks like that only clears the hold if the buffer isn't logged
> > > in the tx. Hm?
> > 
> > Correct.  The buffer is initialized in the same transaction as the dquot
> > block allocation and committed in xfs_defer_finish.  After
> > initialization (which is to say when we return to xfs_qm_dqtobp), the
> > buffer is held, joined, and not logged to the transaction, and nothing
> > else is supposed to dirty the buffer.  Both buffer and transaction are
> > then returned in this state to _dqread, which the in-core dquot state
> > out of the dquot buffer and _trans_brelse's the (still clean) buffer,
> > which breaks the hold and unlocks the buffer.
> > 
> 
> Ok that makes sense, but doesn't that depend on having a deferred
> operation? Is that always guaranteed here?

Assuming you meant the case where we _trans_read_buf'd the dquot buffer
in from disk, we return a buffer that's clean, locked, and joined to the
transaction.  The only difference is that the buffer isn't held, but
_trans_brelse clears the hold unconditionally.

--D

> Brian
> 
> > After the refactor we guarantee that the buffer is locked, clean, and
> > not attached to a transaction by the time we get to calling
> > xfs_dquot_from_disk rather than returning transaction and buffer up the
> > call stack and having to reason up the stack about what state they're in.
> > 
> > --D
> > 
> > > Brian
> > > 
> > > >  	 */
> > > > -
> > > > -	xfs_trans_bhold(tp, bp);
> > > > -
> > > > +	xfs_trans_bhold(*tpp, bp);
> > > > +	error = xfs_defer_bjoin(&dfops, bp);
> > > > +	if (error) {
> > > > +		xfs_trans_bhold_release(*tpp, bp);
> > > > +		xfs_trans_brelse(*tpp, bp);
> > > > +		goto error1;
> > > > +	}
> > > >  	error = xfs_defer_finish(tpp, &dfops);
> > > > -	if (error)
> > > > +	if (error) {
> > > > +		xfs_buf_relse(bp);
> > > >  		goto error1;
> > > > -
> > > > -	/* Transaction was committed? */
> > > > -	if (*tpp != tp) {
> > > > -		tp = *tpp;
> > > > -		xfs_trans_bjoin(tp, bp);
> > > > -	} else {
> > > > -		xfs_trans_bhold_release(tp, bp);
> > > >  	}
> > > > -
> > > > -	*O_bpp = bp;
> > > >  	return 0;
> > > >  
> > > >  error1:
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error

[Brian Foster]

On Fri, May 04, 2018 at 08:52:36AM -0700, Darrick J. Wong wrote:
> On Fri, May 04, 2018 at 11:41:21AM -0400, Brian Foster wrote:
> > On Fri, May 04, 2018 at 08:12:47AM -0700, Darrick J. Wong wrote:
> > > On Fri, May 04, 2018 at 07:31:58AM -0400, Brian Foster wrote:
> > > > On Thu, May 03, 2018 at 10:53:43AM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > 
> > > > > In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
> > > > > allocating a new dquot record", we allocate a new dquot block, grab a
> > > > > buffer to initialize it, and return the locked initialized dquot buffer
> > > > > to the caller for further in-core dquot initialization.  Unfortunately,
> > > > > if the _bmap_finish errored out, _qm_dqalloc would also error out
> > > > > without bothering to free the (locked) buffer.  Leaking a locked buffer
> > > > > caused hangs in generic/388 when quotas are enabled.
> > > > > 
> > > > > Furthermore, the _bmap_finish -> _defer_finish conversion in
> > > > > 310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
> > > > > xfs_defer_*") failed to observe that the buffer was held going into
> > > > > _defer_finish and therefore failed to notice that the buffer lock is
> > > > > /not/ maintained afterwards.  Now that we can bjoin a buffer to a
> > > > > defer_ops, use this mechanism to ensure that the buffer stays locked
> > > > > across the _defer_finish.  Release the holds and locks on the buffer as
> > > > > appropriate if we have to error out.
> > > > > 
> > > > > There is a subtlety here for the caller in that the buffer emerges
> > > > > locked and held to the transaction, so if the _trans_commit fails we
> > > > > have to release the buffer explicitly.  This fixes the unmount hang
> > > > > in generic/388 when quotas are enabled.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > >  fs/xfs/xfs_dquot.c |   48 +++++++++++++++++++++++++++---------------------
> > > > >  1 file changed, 27 insertions(+), 21 deletions(-)
> > > > > 
> > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > > > > index a7daef9e16bf..4c39d8632230 100644
> > > > > --- a/fs/xfs/xfs_dquot.c
> > > > > +++ b/fs/xfs/xfs_dquot.c
> > > > > @@ -362,33 +362,39 @@ xfs_qm_dqalloc(
> > > > >  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
> > > > >  
> > > > >  	/*
> > > > > -	 * xfs_defer_finish() may commit the current transaction and
> > > > > -	 * start a second transaction if the freelist is not empty.
> > > > > +	 * Hold the buffer and join it to the dfops so that we'll still own
> > > > > +	 * the buffer when we return to the caller.  The buffer disposal on
> > > > > +	 * error must be paid attention to very carefully, as it has been
> > > > > +	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
> > > > > +	 * code when allocating a new dquot record" in 2005, and the later
> > > > > +	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
> > > > > +	 * the buffer locked across the _defer_finish call.  We can now do
> > > > > +	 * this correctly with xfs_defer_bjoin.
> > > > >  	 *
> > > > > -	 * Since we still want to modify this buffer, we need to
> > > > > -	 * ensure that the buffer is not released on commit of
> > > > > -	 * the first transaction and ensure the buffer is added to the
> > > > > -	 * second transaction.
> > > > > +	 * Above, we allocated a disk block for the dquot information and
> > > > > +	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
> > > > > +	 * the buffer is still locked to *tpp, so we must _bhold_release and
> > > > > +	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
> > > > > +	 * transaction is gone but the new buffer is not joined or held to any
> > > > > +	 * transaction, so we must _buf_relse it.
> > > > >  	 *
> > > > > -	 * If there is only one transaction then don't stop the buffer
> > > > > -	 * from being released when it commits later on.
> > > > > +	 * If everything succeeds, the caller of this function is returned a
> > > > > +	 * buffer that is locked, held, and joined to the transaction.  If the
> > > > > +	 * transaction commit fails (in the caller) the caller must unlock the
> > > > > +	 * buffer manually.
> > > > 
> > > > If the buffer is held due to the xfs_defer_bjoin(), doesn't that mean
> > > > that the caller has to ultimately release it even after successful
> > > > transaction commit (assuming we don't roll the transaction again
> > > > somewhere)? I see we have an xfs_trans_brelse() up in xfs_qm_dqread(),
> > > > but it looks like that only clears the hold if the buffer isn't logged
> > > > in the tx. Hm?
> > > 
> > > Correct.  The buffer is initialized in the same transaction as the dquot
> > > block allocation and committed in xfs_defer_finish.  After
> > > initialization (which is to say when we return to xfs_qm_dqtobp), the
> > > buffer is held, joined, and not logged to the transaction, and nothing
> > > else is supposed to dirty the buffer.  Both buffer and transaction are
> > > then returned in this state to _dqread, which the in-core dquot state
> > > out of the dquot buffer and _trans_brelse's the (still clean) buffer,
> > > which breaks the hold and unlocks the buffer.
> > > 
> > 
> > Ok that makes sense, but doesn't that depend on having a deferred
> > operation? Is that always guaranteed here?
> 
> Assuming you meant the case where we _trans_read_buf'd the dquot buffer
> in from disk, we return a buffer that's clean, locked, and joined to the
> transaction.  The only difference is that the buffer isn't held, but
> _trans_brelse clears the hold unconditionally.
> 

I'm referring to the xfs_qm_dqalloc() case. It looks like we
xfs_bmapi_write(), get the buffer, call xfs_qm_init_dquot_blk() (logs
the buffer) then go into the defer/exit sequence modified by this
patch...

If the defer finish doesn't do anything, are we in the right state or is
the buffer still dirty+held? If the latter, doesn't that mean the buffer
remains held after the caller commits the current transaction?

Brian

> --D
> 
> > Brian
> > 
> > > After the refactor we guarantee that the buffer is locked, clean, and
> > > not attached to a transaction by the time we get to calling
> > > xfs_dquot_from_disk rather than returning transaction and buffer up the
> > > call stack and having to reason up the stack about what state they're in.
> > > 
> > > --D
> > > 
> > > > Brian
> > > > 
> > > > >  	 */
> > > > > -
> > > > > -	xfs_trans_bhold(tp, bp);
> > > > > -
> > > > > +	xfs_trans_bhold(*tpp, bp);
> > > > > +	error = xfs_defer_bjoin(&dfops, bp);
> > > > > +	if (error) {
> > > > > +		xfs_trans_bhold_release(*tpp, bp);
> > > > > +		xfs_trans_brelse(*tpp, bp);
> > > > > +		goto error1;
> > > > > +	}
> > > > >  	error = xfs_defer_finish(tpp, &dfops);
> > > > > -	if (error)
> > > > > +	if (error) {
> > > > > +		xfs_buf_relse(bp);
> > > > >  		goto error1;
> > > > > -
> > > > > -	/* Transaction was committed? */
> > > > > -	if (*tpp != tp) {
> > > > > -		tp = *tpp;
> > > > > -		xfs_trans_bjoin(tp, bp);
> > > > > -	} else {
> > > > > -		xfs_trans_bhold_release(tp, bp);
> > > > >  	}
> > > > > -
> > > > > -	*O_bpp = bp;
> > > > >  	return 0;
> > > > >  
> > > > >  error1:
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error

[Darrick J. Wong]

On Fri, May 04, 2018 at 12:03:56PM -0400, Brian Foster wrote:
> On Fri, May 04, 2018 at 08:52:36AM -0700, Darrick J. Wong wrote:
> > On Fri, May 04, 2018 at 11:41:21AM -0400, Brian Foster wrote:
> > > On Fri, May 04, 2018 at 08:12:47AM -0700, Darrick J. Wong wrote:
> > > > On Fri, May 04, 2018 at 07:31:58AM -0400, Brian Foster wrote:
> > > > > On Thu, May 03, 2018 at 10:53:43AM -0700, Darrick J. Wong wrote:
> > > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > 
> > > > > > In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
> > > > > > allocating a new dquot record", we allocate a new dquot block, grab a
> > > > > > buffer to initialize it, and return the locked initialized dquot buffer
> > > > > > to the caller for further in-core dquot initialization.  Unfortunately,
> > > > > > if the _bmap_finish errored out, _qm_dqalloc would also error out
> > > > > > without bothering to free the (locked) buffer.  Leaking a locked buffer
> > > > > > caused hangs in generic/388 when quotas are enabled.
> > > > > > 
> > > > > > Furthermore, the _bmap_finish -> _defer_finish conversion in
> > > > > > 310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
> > > > > > xfs_defer_*") failed to observe that the buffer was held going into
> > > > > > _defer_finish and therefore failed to notice that the buffer lock is
> > > > > > /not/ maintained afterwards.  Now that we can bjoin a buffer to a
> > > > > > defer_ops, use this mechanism to ensure that the buffer stays locked
> > > > > > across the _defer_finish.  Release the holds and locks on the buffer as
> > > > > > appropriate if we have to error out.
> > > > > > 
> > > > > > There is a subtlety here for the caller in that the buffer emerges
> > > > > > locked and held to the transaction, so if the _trans_commit fails we
> > > > > > have to release the buffer explicitly.  This fixes the unmount hang
> > > > > > in generic/388 when quotas are enabled.
> > > > > > 
> > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > ---
> > > > > >  fs/xfs/xfs_dquot.c |   48 +++++++++++++++++++++++++++---------------------
> > > > > >  1 file changed, 27 insertions(+), 21 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > > > > > index a7daef9e16bf..4c39d8632230 100644
> > > > > > --- a/fs/xfs/xfs_dquot.c
> > > > > > +++ b/fs/xfs/xfs_dquot.c
> > > > > > @@ -362,33 +362,39 @@ xfs_qm_dqalloc(
> > > > > >  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
> > > > > >  
> > > > > >  	/*
> > > > > > -	 * xfs_defer_finish() may commit the current transaction and
> > > > > > -	 * start a second transaction if the freelist is not empty.
> > > > > > +	 * Hold the buffer and join it to the dfops so that we'll still own
> > > > > > +	 * the buffer when we return to the caller.  The buffer disposal on
> > > > > > +	 * error must be paid attention to very carefully, as it has been
> > > > > > +	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
> > > > > > +	 * code when allocating a new dquot record" in 2005, and the later
> > > > > > +	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
> > > > > > +	 * the buffer locked across the _defer_finish call.  We can now do
> > > > > > +	 * this correctly with xfs_defer_bjoin.
> > > > > >  	 *
> > > > > > -	 * Since we still want to modify this buffer, we need to
> > > > > > -	 * ensure that the buffer is not released on commit of
> > > > > > -	 * the first transaction and ensure the buffer is added to the
> > > > > > -	 * second transaction.
> > > > > > +	 * Above, we allocated a disk block for the dquot information and
> > > > > > +	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
> > > > > > +	 * the buffer is still locked to *tpp, so we must _bhold_release and
> > > > > > +	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
> > > > > > +	 * transaction is gone but the new buffer is not joined or held to any
> > > > > > +	 * transaction, so we must _buf_relse it.
> > > > > >  	 *
> > > > > > -	 * If there is only one transaction then don't stop the buffer
> > > > > > -	 * from being released when it commits later on.
> > > > > > +	 * If everything succeeds, the caller of this function is returned a
> > > > > > +	 * buffer that is locked, held, and joined to the transaction.  If the
> > > > > > +	 * transaction commit fails (in the caller) the caller must unlock the
> > > > > > +	 * buffer manually.
> > > > > 
> > > > > If the buffer is held due to the xfs_defer_bjoin(), doesn't that mean
> > > > > that the caller has to ultimately release it even after successful
> > > > > transaction commit (assuming we don't roll the transaction again
> > > > > somewhere)? I see we have an xfs_trans_brelse() up in xfs_qm_dqread(),
> > > > > but it looks like that only clears the hold if the buffer isn't logged
> > > > > in the tx. Hm?
> > > > 
> > > > Correct.  The buffer is initialized in the same transaction as the dquot
> > > > block allocation and committed in xfs_defer_finish.  After
> > > > initialization (which is to say when we return to xfs_qm_dqtobp), the
> > > > buffer is held, joined, and not logged to the transaction, and nothing
> > > > else is supposed to dirty the buffer.  Both buffer and transaction are
> > > > then returned in this state to _dqread, which the in-core dquot state
> > > > out of the dquot buffer and _trans_brelse's the (still clean) buffer,
> > > > which breaks the hold and unlocks the buffer.
> > > > 
> > > 
> > > Ok that makes sense, but doesn't that depend on having a deferred
> > > operation? Is that always guaranteed here?
> > 
> > Assuming you meant the case where we _trans_read_buf'd the dquot buffer
> > in from disk, we return a buffer that's clean, locked, and joined to the
> > transaction.  The only difference is that the buffer isn't held, but
> > _trans_brelse clears the hold unconditionally.
> > 
> 
> I'm referring to the xfs_qm_dqalloc() case. It looks like we
> xfs_bmapi_write(), get the buffer, call xfs_qm_init_dquot_blk() (logs
> the buffer) then go into the defer/exit sequence modified by this
> patch...
> 
> If the defer finish doesn't do anything, are we in the right state or is
> the buffer still dirty+held? If the latter, doesn't that mean the buffer
> remains held after the caller commits the current transaction?

Ah, right, now I see what you're getting at.

If the defer_finish does nothing then yes we do exit up to _dqread with
the buffer locked, dirty, joined, and held to the transaction, in which
case the xfs_trans_brelse does nothing.  The transaction commit right
after that will log the buffer and release the hold, but the buffer's
still locked.

So I guess we need to roll the transaction after the defer_finish to
guarantee that the buffer is no longer dirty.  Potentially we could
modify _defer_finish to always roll at least once if any of the
_defer_[bi]join items are dirty so that we always return with the
_defer_*join'd items clean?

For now it's probably fine to _bhold_release it after the _defer_finish
(as Brian just suggested on irc) and reconsider "_defer_finish with
defer_joined dirty stuff" separately.

The refactor fixes all this so that we always commit the transaction
before handing back a bjoin'd buffer, so the state is consistent.

--D

> Brian
> 
> > --D
> > 
> > > Brian
> > > 
> > > > After the refactor we guarantee that the buffer is locked, clean, and
> > > > not attached to a transaction by the time we get to calling
> > > > xfs_dquot_from_disk rather than returning transaction and buffer up the
> > > > call stack and having to reason up the stack about what state they're in.
> > > > 
> > > > --D
> > > > 
> > > > > Brian
> > > > > 
> > > > > >  	 */
> > > > > > -
> > > > > > -	xfs_trans_bhold(tp, bp);
> > > > > > -
> > > > > > +	xfs_trans_bhold(*tpp, bp);
> > > > > > +	error = xfs_defer_bjoin(&dfops, bp);
> > > > > > +	if (error) {
> > > > > > +		xfs_trans_bhold_release(*tpp, bp);
> > > > > > +		xfs_trans_brelse(*tpp, bp);
> > > > > > +		goto error1;
> > > > > > +	}
> > > > > >  	error = xfs_defer_finish(tpp, &dfops);
> > > > > > -	if (error)
> > > > > > +	if (error) {
> > > > > > +		xfs_buf_relse(bp);
> > > > > >  		goto error1;
> > > > > > -
> > > > > > -	/* Transaction was committed? */
> > > > > > -	if (*tpp != tp) {
> > > > > > -		tp = *tpp;
> > > > > > -		xfs_trans_bjoin(tp, bp);
> > > > > > -	} else {
> > > > > > -		xfs_trans_bhold_release(tp, bp);
> > > > > >  	}
> > > > > > -
> > > > > > -	*O_bpp = bp;
> > > > > >  	return 0;
> > > > > >  
> > > > > >  error1:
> > > > > > --
> > > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > > > the body of a message to majordomo@vger.kernel.org
> > > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0.1/13] xfs: release new dquot buffer on defer_finish error

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
allocating a new dquot record", we allocate a new dquot block, grab a
buffer to initialize it, and return the locked initialized dquot buffer
to the caller for further in-core dquot initialization.  Unfortunately,
if the _bmap_finish errored out, _qm_dqalloc would also error out
without bothering to free the (locked) buffer.  Leaking a locked buffer
caused hangs in generic/388 when quotas are enabled.

Furthermore, the _bmap_finish -> _defer_finish conversion in
310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
xfs_defer_*") failed to observe that the buffer was held going into
_defer_finish and therefore failed to notice that the buffer lock is
/not/ maintained afterwards.  Now that we can bjoin a buffer to a
defer_ops, use this mechanism to ensure that the buffer stays locked
across the _defer_finish.  Release the holds and locks on the buffer as
appropriate if we have to error out.

There is a subtlety here for the caller in that the buffer emerges
locked and held to the transaction, so if the _trans_commit fails we
have to release the buffer explicitly.  This fixes the unmount hang
in generic/388 when quotas are enabled.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: return buffer with consistent hold state
---
 fs/xfs/xfs_dquot.c |   48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index a7daef9e16bf..7c71a14d0d49 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -362,32 +362,40 @@ xfs_qm_dqalloc(
 			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
 
 	/*
-	 * xfs_defer_finish() may commit the current transaction and
-	 * start a second transaction if the freelist is not empty.
+	 * Hold the buffer and join it to the dfops so that we'll still own
+	 * the buffer when we return to the caller.  The buffer disposal on
+	 * error must be paid attention to very carefully, as it has been
+	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
+	 * code when allocating a new dquot record" in 2005, and the later
+	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
+	 * the buffer locked across the _defer_finish call.  We can now do
+	 * this correctly with xfs_defer_bjoin.
 	 *
-	 * Since we still want to modify this buffer, we need to
-	 * ensure that the buffer is not released on commit of
-	 * the first transaction and ensure the buffer is added to the
-	 * second transaction.
+	 * Above, we allocated a disk block for the dquot information and
+	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
+	 * the buffer is still locked to *tpp, so we must _bhold_release and
+	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
+	 * transaction is gone but the new buffer is not joined or held to any
+	 * transaction, so we must _buf_relse it.
 	 *
-	 * If there is only one transaction then don't stop the buffer
-	 * from being released when it commits later on.
+	 * If everything succeeds, the caller of this function is returned a
+	 * buffer that is locked and joined to the transaction.  The caller
+	 * is responsible for unlocking any buffer passed back, either
+	 * manually or by committing the transaction.
 	 */
-
-	xfs_trans_bhold(tp, bp);
-
+	xfs_trans_bhold(*tpp, bp);
+	error = xfs_defer_bjoin(&dfops, bp);
+	if (error) {
+		xfs_trans_bhold_release(*tpp, bp);
+		xfs_trans_brelse(*tpp, bp);
+		goto error1;
+	}
 	error = xfs_defer_finish(tpp, &dfops);
-	if (error)
+	if (error) {
+		xfs_buf_relse(bp);
 		goto error1;
-
-	/* Transaction was committed? */
-	if (*tpp != tp) {
-		tp = *tpp;
-		xfs_trans_bjoin(tp, bp);
-	} else {
-		xfs_trans_bhold_release(tp, bp);
 	}
-
+	xfs_trans_bhold_release(*tpp, bp);
 	*O_bpp = bp;
 	return 0;
 

Re: [PATCH v2 0.1/13] xfs: release new dquot buffer on defer_finish error

[Brian Foster]

On Fri, May 04, 2018 at 02:19:58PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when
> allocating a new dquot record", we allocate a new dquot block, grab a
> buffer to initialize it, and return the locked initialized dquot buffer
> to the caller for further in-core dquot initialization.  Unfortunately,
> if the _bmap_finish errored out, _qm_dqalloc would also error out
> without bothering to free the (locked) buffer.  Leaking a locked buffer
> caused hangs in generic/388 when quotas are enabled.
> 
> Furthermore, the _bmap_finish -> _defer_finish conversion in
> 310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} ->
> xfs_defer_*") failed to observe that the buffer was held going into
> _defer_finish and therefore failed to notice that the buffer lock is
> /not/ maintained afterwards.  Now that we can bjoin a buffer to a
> defer_ops, use this mechanism to ensure that the buffer stays locked
> across the _defer_finish.  Release the holds and locks on the buffer as
> appropriate if we have to error out.
> 
> There is a subtlety here for the caller in that the buffer emerges
> locked and held to the transaction, so if the _trans_commit fails we
> have to release the buffer explicitly.  This fixes the unmount hang
> in generic/388 when quotas are enabled.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: return buffer with consistent hold state
> ---

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

>  fs/xfs/xfs_dquot.c |   48 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index a7daef9e16bf..7c71a14d0d49 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -362,32 +362,40 @@ xfs_qm_dqalloc(
>  			      dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
>  
>  	/*
> -	 * xfs_defer_finish() may commit the current transaction and
> -	 * start a second transaction if the freelist is not empty.
> +	 * Hold the buffer and join it to the dfops so that we'll still own
> +	 * the buffer when we return to the caller.  The buffer disposal on
> +	 * error must be paid attention to very carefully, as it has been
> +	 * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
> +	 * code when allocating a new dquot record" in 2005, and the later
> +	 * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
> +	 * the buffer locked across the _defer_finish call.  We can now do
> +	 * this correctly with xfs_defer_bjoin.
>  	 *
> -	 * Since we still want to modify this buffer, we need to
> -	 * ensure that the buffer is not released on commit of
> -	 * the first transaction and ensure the buffer is added to the
> -	 * second transaction.
> +	 * Above, we allocated a disk block for the dquot information and
> +	 * used get_buf to initialize the dquot.  If the _defer_bjoin fails,
> +	 * the buffer is still locked to *tpp, so we must _bhold_release and
> +	 * then _trans_brelse the buffer.  If the _defer_finish fails, the old
> +	 * transaction is gone but the new buffer is not joined or held to any
> +	 * transaction, so we must _buf_relse it.
>  	 *
> -	 * If there is only one transaction then don't stop the buffer
> -	 * from being released when it commits later on.
> +	 * If everything succeeds, the caller of this function is returned a
> +	 * buffer that is locked and joined to the transaction.  The caller
> +	 * is responsible for unlocking any buffer passed back, either
> +	 * manually or by committing the transaction.
>  	 */
> -
> -	xfs_trans_bhold(tp, bp);
> -
> +	xfs_trans_bhold(*tpp, bp);
> +	error = xfs_defer_bjoin(&dfops, bp);
> +	if (error) {
> +		xfs_trans_bhold_release(*tpp, bp);
> +		xfs_trans_brelse(*tpp, bp);
> +		goto error1;
> +	}
>  	error = xfs_defer_finish(tpp, &dfops);
> -	if (error)
> +	if (error) {
> +		xfs_buf_relse(bp);
>  		goto error1;
> -
> -	/* Transaction was committed? */
> -	if (*tpp != tp) {
> -		tp = *tpp;
> -		xfs_trans_bjoin(tp, bp);
> -	} else {
> -		xfs_trans_bhold_release(tp, bp);
>  	}
> -
> +	xfs_trans_bhold_release(*tpp, bp);
>  	*O_bpp = bp;
>  	return 0;
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument

[Christoph Hellwig]

On Wed, May 02, 2018 at 09:58:56AM -0700, Darrick J. Wong wrote:
> > I really see no point in doing the wrapper.  I'd much rather keep
> > xfs_qm_dqread with a the additional argument, be that a flags value
> > or a bool.  For example the same patch with the new xfs_dquot_setup
> > kept as xfs_qm_dqread and everyone calling it with an additional
> > false argument would seem much nicer to me.
> 
> I'm trying to avoid having a can_alloc argument for the exported _dqread
> interface because none of the callers outside of xfs_dquot.c set it to
> true.

That's not really a good reason on its own..

Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument

[Darrick J. Wong]

On Mon, May 07, 2018 at 04:41:59PM +0200, Christoph Hellwig wrote:
> On Wed, May 02, 2018 at 09:58:56AM -0700, Darrick J. Wong wrote:
> > > I really see no point in doing the wrapper.  I'd much rather keep
> > > xfs_qm_dqread with a the additional argument, be that a flags value
> > > or a bool.  For example the same patch with the new xfs_dquot_setup
> > > kept as xfs_qm_dqread and everyone calling it with an additional
> > > false argument would seem much nicer to me.
> > 
> > I'm trying to avoid having a can_alloc argument for the exported _dqread
> > interface because none of the callers outside of xfs_dquot.c set it to
> > true.
> 
> That's not really a good reason on its own..

Hm, I talked to Dave on IRC, and after some spelunking we unearthed that
the whole point of calling _dqread directly was to obtain an uncached
dquot that isn't linked into any lists/radix trees/caches/whatever.
That way if we end up doing quotacheck we won't have any dquots in
memory that are now garbage.

>From that perspective it's not as big of a jump to re-hide dqread and
add a _dqget_uncached that has a much better comment justifying why it's
there and who uses it...

--D

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

[PATCH 09/13] xfs: remove direct calls to _qm_dqread

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The quota initialization code needs an "uncached" variant of _dqget to
read in default quota limits and timers before the dquot cache is fully
set up.  We've already split up _dqget into its component pieces so
create a fourth variant to address this need, and make dqread internal
to xfs_dquot.c again.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_dquot.c |   24 +++++++++++++++++++++++-
 fs/xfs/xfs_dquot.h |    5 +++--
 fs/xfs/xfs_qm.c    |   25 +++++++++++++------------
 3 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 54c16f76d12d..0cb4b553db28 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -586,7 +586,7 @@ xfs_qm_dqread_alloc(
  *
  * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
  */
-int
+static int
 xfs_qm_dqread(
 	struct xfs_mount	*mp,
 	xfs_dqid_t		id,
@@ -832,6 +832,28 @@ xfs_qm_dqget(
 	return 0;
 }
 
+/*
+ * Given a dquot id and type, read and initialize a dquot from the on-disk
+ * metadata.  This function is only for use during quota initialization so
+ * it ignores the dquot cache assuming that the dquot shrinker isn't set up.
+ * The caller is responsible for _qm_dqdestroy'ing the returned dquot.
+ */
+int
+xfs_qm_dqget_uncached(
+	struct xfs_mount	*mp,
+	xfs_dqid_t		id,
+	uint			type,
+	struct xfs_dquot	**dqpp)
+{
+	int			error;
+
+	error = xfs_qm_dqget_checks(mp, type);
+	if (error)
+		return error;
+
+	return xfs_qm_dqread(mp, id, type, 0, dqpp);
+}
+
 /* Return the quota id for a given inode and type. */
 xfs_dqid_t
 xfs_qm_id_for_quotatype(
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 8a5b30e952b0..38a42874a98c 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -160,8 +160,6 @@ static inline bool xfs_dquot_lowsp(struct xfs_dquot *dqp)
 #define XFS_QM_ISPDQ(dqp)	((dqp)->dq_flags & XFS_DQ_PROJ)
 #define XFS_QM_ISGDQ(dqp)	((dqp)->dq_flags & XFS_DQ_GROUP)
 
-extern int		xfs_qm_dqread(struct xfs_mount *, xfs_dqid_t, uint,
-					uint, struct xfs_dquot	**);
 extern void		xfs_qm_dqdestroy(xfs_dquot_t *);
 extern int		xfs_qm_dqflush(struct xfs_dquot *, struct xfs_buf **);
 extern void		xfs_qm_dqunpin_wait(xfs_dquot_t *);
@@ -179,6 +177,9 @@ extern int		xfs_qm_dqget_inode(struct xfs_inode *ip, uint type,
 					struct xfs_dquot **dqpp);
 extern int		xfs_qm_dqget_next(struct xfs_mount *mp, xfs_dqid_t id,
 					uint type, struct xfs_dquot **dqpp);
+extern int		xfs_qm_dqget_uncached(struct xfs_mount *mp,
+					xfs_dqid_t id, uint type,
+					struct xfs_dquot **dqpp);
 extern void		xfs_qm_dqput(xfs_dquot_t *);
 
 extern void		xfs_dqlock2(struct xfs_dquot *, struct xfs_dquot *);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index b9244b8983b3..ea06762aa6c1 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -563,8 +563,7 @@ xfs_qm_set_defquota(
 	struct xfs_def_quota    *defq;
 	int			error;
 
-	error = xfs_qm_dqread(mp, 0, type, 0, &dqp);
-
+	error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
 	if (!error) {
 		xfs_disk_dquot_t        *ddqp = &dqp->q_core;
 
@@ -590,11 +589,12 @@ xfs_qm_set_defquota(
  */
 STATIC int
 xfs_qm_init_quotainfo(
-	xfs_mount_t	*mp)
+	struct xfs_mount	*mp)
 {
-	xfs_quotainfo_t *qinf;
-	int		error;
-	xfs_dquot_t	*dqp;
+	struct xfs_quotainfo	*qinf;
+	struct xfs_dquot	*dqp;
+	uint			type;
+	int			error;
 
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
@@ -637,12 +637,13 @@ xfs_qm_init_quotainfo(
 	 * user/group/proj quota types, otherwise a default value is used.
 	 * This should be split into different fields per quota type.
 	 */
-	error = xfs_qm_dqread(mp, 0,
-			XFS_IS_UQUOTA_RUNNING(mp) ? XFS_DQ_USER :
-			 (XFS_IS_GQUOTA_RUNNING(mp) ? XFS_DQ_GROUP :
-			  XFS_DQ_PROJ),
-			0, &dqp);
-
+	if (XFS_IS_UQUOTA_RUNNING(mp))
+		type = XFS_DQ_USER;
+	else if (XFS_IS_GQUOTA_RUNNING(mp))
+		type = XFS_DQ_GROUP;
+	else
+		type = XFS_DQ_PROJ;
+	error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
 	if (!error) {
 		xfs_disk_dquot_t	*ddqp = &dqp->q_core;
 

Re: [PATCH 09/13] xfs: remove direct calls to _qm_dqread

[Brian Foster]

On Mon, May 07, 2018 at 05:05:35PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The quota initialization code needs an "uncached" variant of _dqget to
> read in default quota limits and timers before the dquot cache is fully
> set up.  We've already split up _dqget into its component pieces so
> create a fourth variant to address this need, and make dqread internal
> to xfs_dquot.c again.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Looks good to me:

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

>  fs/xfs/xfs_dquot.c |   24 +++++++++++++++++++++++-
>  fs/xfs/xfs_dquot.h |    5 +++--
>  fs/xfs/xfs_qm.c    |   25 +++++++++++++------------
>  3 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 54c16f76d12d..0cb4b553db28 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -586,7 +586,7 @@ xfs_qm_dqread_alloc(
>   *
>   * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
>   */
> -int
> +static int
>  xfs_qm_dqread(
>  	struct xfs_mount	*mp,
>  	xfs_dqid_t		id,
> @@ -832,6 +832,28 @@ xfs_qm_dqget(
>  	return 0;
>  }
>  
> +/*
> + * Given a dquot id and type, read and initialize a dquot from the on-disk
> + * metadata.  This function is only for use during quota initialization so
> + * it ignores the dquot cache assuming that the dquot shrinker isn't set up.
> + * The caller is responsible for _qm_dqdestroy'ing the returned dquot.
> + */
> +int
> +xfs_qm_dqget_uncached(
> +	struct xfs_mount	*mp,
> +	xfs_dqid_t		id,
> +	uint			type,
> +	struct xfs_dquot	**dqpp)
> +{
> +	int			error;
> +
> +	error = xfs_qm_dqget_checks(mp, type);
> +	if (error)
> +		return error;
> +
> +	return xfs_qm_dqread(mp, id, type, 0, dqpp);
> +}
> +
>  /* Return the quota id for a given inode and type. */
>  xfs_dqid_t
>  xfs_qm_id_for_quotatype(
> diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
> index 8a5b30e952b0..38a42874a98c 100644
> --- a/fs/xfs/xfs_dquot.h
> +++ b/fs/xfs/xfs_dquot.h
> @@ -160,8 +160,6 @@ static inline bool xfs_dquot_lowsp(struct xfs_dquot *dqp)
>  #define XFS_QM_ISPDQ(dqp)	((dqp)->dq_flags & XFS_DQ_PROJ)
>  #define XFS_QM_ISGDQ(dqp)	((dqp)->dq_flags & XFS_DQ_GROUP)
>  
> -extern int		xfs_qm_dqread(struct xfs_mount *, xfs_dqid_t, uint,
> -					uint, struct xfs_dquot	**);
>  extern void		xfs_qm_dqdestroy(xfs_dquot_t *);
>  extern int		xfs_qm_dqflush(struct xfs_dquot *, struct xfs_buf **);
>  extern void		xfs_qm_dqunpin_wait(xfs_dquot_t *);
> @@ -179,6 +177,9 @@ extern int		xfs_qm_dqget_inode(struct xfs_inode *ip, uint type,
>  					struct xfs_dquot **dqpp);
>  extern int		xfs_qm_dqget_next(struct xfs_mount *mp, xfs_dqid_t id,
>  					uint type, struct xfs_dquot **dqpp);
> +extern int		xfs_qm_dqget_uncached(struct xfs_mount *mp,
> +					xfs_dqid_t id, uint type,
> +					struct xfs_dquot **dqpp);
>  extern void		xfs_qm_dqput(xfs_dquot_t *);
>  
>  extern void		xfs_dqlock2(struct xfs_dquot *, struct xfs_dquot *);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index b9244b8983b3..ea06762aa6c1 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -563,8 +563,7 @@ xfs_qm_set_defquota(
>  	struct xfs_def_quota    *defq;
>  	int			error;
>  
> -	error = xfs_qm_dqread(mp, 0, type, 0, &dqp);
> -
> +	error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
>  	if (!error) {
>  		xfs_disk_dquot_t        *ddqp = &dqp->q_core;
>  
> @@ -590,11 +589,12 @@ xfs_qm_set_defquota(
>   */
>  STATIC int
>  xfs_qm_init_quotainfo(
> -	xfs_mount_t	*mp)
> +	struct xfs_mount	*mp)
>  {
> -	xfs_quotainfo_t *qinf;
> -	int		error;
> -	xfs_dquot_t	*dqp;
> +	struct xfs_quotainfo	*qinf;
> +	struct xfs_dquot	*dqp;
> +	uint			type;
> +	int			error;
>  
>  	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
>  
> @@ -637,12 +637,13 @@ xfs_qm_init_quotainfo(
>  	 * user/group/proj quota types, otherwise a default value is used.
>  	 * This should be split into different fields per quota type.
>  	 */
> -	error = xfs_qm_dqread(mp, 0,
> -			XFS_IS_UQUOTA_RUNNING(mp) ? XFS_DQ_USER :
> -			 (XFS_IS_GQUOTA_RUNNING(mp) ? XFS_DQ_GROUP :
> -			  XFS_DQ_PROJ),
> -			0, &dqp);
> -
> +	if (XFS_IS_UQUOTA_RUNNING(mp))
> +		type = XFS_DQ_USER;
> +	else if (XFS_IS_GQUOTA_RUNNING(mp))
> +		type = XFS_DQ_GROUP;
> +	else
> +		type = XFS_DQ_PROJ;
> +	error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
>  	if (!error) {
>  		xfs_disk_dquot_t	*ddqp = &dqp->q_core;
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/13] xfs: remove direct calls to _qm_dqread

[Christoph Hellwig]

On Mon, May 07, 2018 at 05:05:35PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The quota initialization code needs an "uncached" variant of _dqget to
> read in default quota limits and timers before the dquot cache is fully
> set up.  We've already split up _dqget into its component pieces so
> create a fourth variant to address this need, and make dqread internal
> to xfs_dquot.c again.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks fine,

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

But can you please resend the whole series?  I've lost track of all the
updates..

Re: [PATCH 09/13] xfs: remove direct calls to _qm_dqread

[Darrick J. Wong]

On Thu, May 10, 2018 at 10:26:00AM +0200, Christoph Hellwig wrote:
> On Mon, May 07, 2018 at 05:05:35PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The quota initialization code needs an "uncached" variant of _dqget to
> > read in default quota limits and timers before the dquot cache is fully
> > set up.  We've already split up _dqget into its component pieces so
> > create a fourth variant to address this need, and make dqread internal
> > to xfs_dquot.c again.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Looks fine,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But can you please resend the whole series?  I've lost track of all the
> updates..

Will do.  I'll also try to push all the accumulated/reviewed 4.18 stuff
to for-next in time for tomorrow's tree.

--D

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