* [PATCH 01/13] xfs: refactor XFS_QMOPT_DQNEXT out of existence
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
@ 2018-04-30 5:43 ` Darrick J. Wong
2018-04-30 5:43 ` [PATCH 02/13] xfs: refactor dquot cache handling Darrick J. Wong
` (13 subsequent siblings)
14 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:43 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 02/13] xfs: refactor dquot cache handling
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
2018-04-30 5:43 ` [PATCH 01/13] xfs: refactor XFS_QMOPT_DQNEXT out of existence Darrick J. Wong
@ 2018-04-30 5:43 ` Darrick J. Wong
2018-04-30 5:43 ` [PATCH 03/13] xfs: delegate dqget input checks to helper function Darrick J. Wong
` (12 subsequent siblings)
14 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:43 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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);
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 03/13] xfs: delegate dqget input checks to helper function
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
2018-04-30 5:43 ` [PATCH 01/13] xfs: refactor XFS_QMOPT_DQNEXT out of existence Darrick J. Wong
2018-04-30 5:43 ` [PATCH 02/13] xfs: refactor dquot cache handling Darrick J. Wong
@ 2018-04-30 5:43 ` Darrick J. Wong
2018-04-30 5:43 ` [PATCH 04/13] xfs: remove unnecessary xfs_qm_dqattach parameter Darrick J. Wong
` (11 subsequent siblings)
14 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:43 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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);
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 04/13] xfs: remove unnecessary xfs_qm_dqattach parameter
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (2 preceding siblings ...)
2018-04-30 5:43 ` [PATCH 03/13] xfs: delegate dqget input checks to helper function Darrick J. Wong
@ 2018-04-30 5:43 ` Darrick J. Wong
2018-04-30 5:43 ` [PATCH 05/13] xfs: split out dqget for inodes from regular dqget Darrick J. Wong
` (10 subsequent siblings)
14 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:43 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 05/13] xfs: split out dqget for inodes from regular dqget
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (3 preceding siblings ...)
2018-04-30 5:43 ` [PATCH 04/13] xfs: remove unnecessary xfs_qm_dqattach parameter Darrick J. Wong
@ 2018-04-30 5:43 ` Darrick J. Wong
2018-04-30 5:43 ` [PATCH 06/13] xfs: fetch dquots directly during quotacheck Darrick J. Wong
` (9 subsequent siblings)
14 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:43 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 06/13] xfs: fetch dquots directly during quotacheck
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (4 preceding siblings ...)
2018-04-30 5:43 ` [PATCH 05/13] xfs: split out dqget for inodes from regular dqget Darrick J. Wong
@ 2018-04-30 5:43 ` Darrick J. Wong
2018-04-30 5:43 ` [PATCH 07/13] xfs: refactor incore dquot initialization functions Darrick J. Wong
` (8 subsequent siblings)
14 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:43 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 07/13] xfs: refactor incore dquot initialization functions
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (5 preceding siblings ...)
2018-04-30 5:43 ` [PATCH 06/13] xfs: fetch dquots directly during quotacheck Darrick J. Wong
@ 2018-04-30 5:43 ` Darrick J. Wong
2018-04-30 5:44 ` [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc Darrick J. Wong
` (7 subsequent siblings)
14 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:43 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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);
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (6 preceding siblings ...)
2018-04-30 5:43 ` [PATCH 07/13] xfs: refactor incore dquot initialization functions Darrick J. Wong
@ 2018-04-30 5:44 ` Darrick J. Wong
2018-05-01 13:44 ` Brian Foster
` (2 more replies)
2018-04-30 5:44 ` [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument Darrick J. Wong
` (6 subsequent siblings)
14 siblings, 3 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:44 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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;
}
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc
2018-04-30 5:44 ` [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc Darrick J. Wong
@ 2018-05-01 13:44 ` Brian Foster
2018-05-02 16:32 ` Christoph Hellwig
2018-05-03 0:10 ` Darrick J. Wong
2 siblings, 0 replies; 51+ messages in thread
From: Brian Foster @ 2018-05-01 13:44 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc
2018-04-30 5:44 ` [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc Darrick J. Wong
2018-05-01 13:44 ` Brian Foster
@ 2018-05-02 16:32 ` Christoph Hellwig
2018-05-03 0:10 ` Darrick J. Wong
2 siblings, 0 replies; 51+ messages in thread
From: Christoph Hellwig @ 2018-05-02 16:32 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, bfoster, hch
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc
2018-04-30 5:44 ` [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc Darrick J. Wong
2018-05-01 13:44 ` Brian Foster
2018-05-02 16:32 ` Christoph Hellwig
@ 2018-05-03 0:10 ` Darrick J. Wong
2 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-03 0:10 UTC (permalink / raw)
To: linux-xfs, bfoster, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (7 preceding siblings ...)
2018-04-30 5:44 ` [PATCH 08/13] xfs: refactor xfs_qm_dqtobp and xfs_qm_dqalloc Darrick J. Wong
@ 2018-04-30 5:44 ` Darrick J. Wong
2018-05-01 13:44 ` Brian Foster
` (2 more replies)
2018-04-30 5:44 ` [PATCH 10/13] xfs: replace XFS_QMOPT_DQALLOC with XFS_DQGET_{ALLOC, EXISTS} Darrick J. Wong
` (5 subsequent siblings)
14 siblings, 3 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:44 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument
2018-04-30 5:44 ` [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument Darrick J. Wong
@ 2018-05-01 13:44 ` Brian Foster
2018-05-02 16:34 ` Christoph Hellwig
2018-05-08 0:05 ` [PATCH 09/13] xfs: remove direct calls to _qm_dqread Darrick J. Wong
2 siblings, 0 replies; 51+ messages in thread
From: Brian Foster @ 2018-05-01 13:44 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument
2018-04-30 5:44 ` [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument Darrick J. Wong
2018-05-01 13:44 ` Brian Foster
@ 2018-05-02 16:34 ` Christoph Hellwig
2018-05-02 16:58 ` Darrick J. Wong
2018-05-08 0:05 ` [PATCH 09/13] xfs: remove direct calls to _qm_dqread Darrick J. Wong
2 siblings, 1 reply; 51+ messages in thread
From: Christoph Hellwig @ 2018-05-02 16:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, bfoster, hch
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.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument
2018-05-02 16:34 ` Christoph Hellwig
@ 2018-05-02 16:58 ` Darrick J. Wong
2018-05-07 14:41 ` Christoph Hellwig
0 siblings, 1 reply; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-02 16:58 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, bfoster
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument
2018-05-02 16:58 ` Darrick J. Wong
@ 2018-05-07 14:41 ` Christoph Hellwig
2018-05-08 0:04 ` Darrick J. Wong
0 siblings, 1 reply; 51+ messages in thread
From: Christoph Hellwig @ 2018-05-07 14:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs, bfoster
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..
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument
2018-05-07 14:41 ` Christoph Hellwig
@ 2018-05-08 0:04 ` Darrick J. Wong
0 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-08 0:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, bfoster
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 09/13] xfs: remove direct calls to _qm_dqread
2018-04-30 5:44 ` [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument Darrick J. Wong
2018-05-01 13:44 ` Brian Foster
2018-05-02 16:34 ` Christoph Hellwig
@ 2018-05-08 0:05 ` Darrick J. Wong
2018-05-09 16:40 ` Brian Foster
2018-05-10 8:26 ` Christoph Hellwig
2 siblings, 2 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-08 0:05 UTC (permalink / raw)
To: linux-xfs, bfoster, hch, Dave Chinner
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 09/13] xfs: remove direct calls to _qm_dqread
2018-05-08 0:05 ` [PATCH 09/13] xfs: remove direct calls to _qm_dqread Darrick J. Wong
@ 2018-05-09 16:40 ` Brian Foster
2018-05-10 8:26 ` Christoph Hellwig
1 sibling, 0 replies; 51+ messages in thread
From: Brian Foster @ 2018-05-09 16:40 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch, Dave Chinner
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 09/13] xfs: remove direct calls to _qm_dqread
2018-05-08 0:05 ` [PATCH 09/13] xfs: remove direct calls to _qm_dqread Darrick J. Wong
2018-05-09 16:40 ` Brian Foster
@ 2018-05-10 8:26 ` Christoph Hellwig
2018-05-10 15:20 ` Darrick J. Wong
1 sibling, 1 reply; 51+ messages in thread
From: Christoph Hellwig @ 2018-05-10 8:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, bfoster, hch, Dave Chinner
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..
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 09/13] xfs: remove direct calls to _qm_dqread
2018-05-10 8:26 ` Christoph Hellwig
@ 2018-05-10 15:20 ` Darrick J. Wong
0 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-10 15:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, bfoster, Dave Chinner
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 10/13] xfs: replace XFS_QMOPT_DQALLOC with XFS_DQGET_{ALLOC, EXISTS}
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (8 preceding siblings ...)
2018-04-30 5:44 ` [PATCH 09/13] xfs: remove xfs_qm_dqread flags argument Darrick J. Wong
@ 2018-04-30 5:44 ` Darrick J. Wong
2018-04-30 5:47 ` [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean Darrick J. Wong
2018-04-30 5:44 ` [PATCH 11/13] xfs: report failing address when dquot verifier fails Darrick J. Wong
` (4 subsequent siblings)
14 siblings, 1 reply; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:44 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean
2018-04-30 5:44 ` [PATCH 10/13] xfs: replace XFS_QMOPT_DQALLOC with XFS_DQGET_{ALLOC, EXISTS} Darrick J. Wong
@ 2018-04-30 5:47 ` Darrick J. Wong
2018-05-01 13:45 ` Brian Foster
2018-05-02 16:35 ` Christoph Hellwig
0 siblings, 2 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:47 UTC (permalink / raw)
To: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean
2018-04-30 5:47 ` [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean Darrick J. Wong
@ 2018-05-01 13:45 ` Brian Foster
2018-05-01 15:52 ` Darrick J. Wong
2018-05-02 16:35 ` Christoph Hellwig
1 sibling, 1 reply; 51+ messages in thread
From: Brian Foster @ 2018-05-01 13:45 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean
2018-05-01 13:45 ` Brian Foster
@ 2018-05-01 15:52 ` Darrick J. Wong
0 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-01 15:52 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean
2018-04-30 5:47 ` [PATCH v2 10/13] xfs: replace XFS_QMOPT_DQALLOC with boolean Darrick J. Wong
2018-05-01 13:45 ` Brian Foster
@ 2018-05-02 16:35 ` Christoph Hellwig
1 sibling, 0 replies; 51+ messages in thread
From: Christoph Hellwig @ 2018-05-02 16:35 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, bfoster, hch
Looks fine:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 11/13] xfs: report failing address when dquot verifier fails
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (9 preceding siblings ...)
2018-04-30 5:44 ` [PATCH 10/13] xfs: replace XFS_QMOPT_DQALLOC with XFS_DQGET_{ALLOC, EXISTS} Darrick J. Wong
@ 2018-04-30 5:44 ` Darrick J. Wong
2018-04-30 5:44 ` [PATCH 12/13] xfs: rename on-disk dquot counter zap functions Darrick J. Wong
` (3 subsequent siblings)
14 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:44 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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 = {
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 12/13] xfs: rename on-disk dquot counter zap functions
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (10 preceding siblings ...)
2018-04-30 5:44 ` [PATCH 11/13] xfs: report failing address when dquot verifier fails Darrick J. Wong
@ 2018-04-30 5:44 ` Darrick J. Wong
2018-05-01 13:45 ` Brian Foster
2018-05-02 16:35 ` Christoph Hellwig
2018-04-30 5:44 ` [PATCH 13/13] xfs: refactor dquot iteration Darrick J. Wong
` (2 subsequent siblings)
14 siblings, 2 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:44 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 12/13] xfs: rename on-disk dquot counter zap functions
2018-04-30 5:44 ` [PATCH 12/13] xfs: rename on-disk dquot counter zap functions Darrick J. Wong
@ 2018-05-01 13:45 ` Brian Foster
2018-05-02 16:35 ` Christoph Hellwig
1 sibling, 0 replies; 51+ messages in thread
From: Brian Foster @ 2018-05-01 13:45 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 12/13] xfs: rename on-disk dquot counter zap functions
2018-04-30 5:44 ` [PATCH 12/13] xfs: rename on-disk dquot counter zap functions Darrick J. Wong
2018-05-01 13:45 ` Brian Foster
@ 2018-05-02 16:35 ` Christoph Hellwig
1 sibling, 0 replies; 51+ messages in thread
From: Christoph Hellwig @ 2018-05-02 16:35 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, bfoster, hch
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 13/13] xfs: refactor dquot iteration
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (11 preceding siblings ...)
2018-04-30 5:44 ` [PATCH 12/13] xfs: rename on-disk dquot counter zap functions Darrick J. Wong
@ 2018-04-30 5:44 ` Darrick J. Wong
2018-05-01 13:45 ` Brian Foster
2018-05-02 16:43 ` [PATCH v2 " Darrick J. Wong
2018-05-03 17:53 ` [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error Darrick J. Wong
2018-05-03 17:54 ` [PATCH 0.2/13] xfs: don't spray logs when dquot flush/purge fail Darrick J. Wong
14 siblings, 2 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-04-30 5:44 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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__ */
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 13/13] xfs: refactor dquot iteration
2018-04-30 5:44 ` [PATCH 13/13] xfs: refactor dquot iteration Darrick J. Wong
@ 2018-05-01 13:45 ` Brian Foster
2018-05-01 15:53 ` Darrick J. Wong
2018-05-02 16:43 ` [PATCH v2 " Darrick J. Wong
1 sibling, 1 reply; 51+ messages in thread
From: Brian Foster @ 2018-05-01 13:45 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 13/13] xfs: refactor dquot iteration
2018-05-01 13:45 ` Brian Foster
@ 2018-05-01 15:53 ` Darrick J. Wong
2018-05-02 16:37 ` Christoph Hellwig
0 siblings, 1 reply; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-01 15:53 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 13/13] xfs: refactor dquot iteration
2018-04-30 5:44 ` [PATCH 13/13] xfs: refactor dquot iteration Darrick J. Wong
2018-05-01 13:45 ` Brian Foster
@ 2018-05-02 16:43 ` Darrick J. Wong
1 sibling, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-02 16:43 UTC (permalink / raw)
To: linux-xfs, bfoster, hch
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__ */
^ permalink raw reply related [flat|nested] 51+ messages in thread
* [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (12 preceding siblings ...)
2018-04-30 5:44 ` [PATCH 13/13] xfs: refactor dquot iteration Darrick J. Wong
@ 2018-05-03 17:53 ` Darrick J. Wong
2018-05-04 11:31 ` Brian Foster
2018-05-04 21:19 ` [PATCH v2 " Darrick J. Wong
2018-05-03 17:54 ` [PATCH 0.2/13] xfs: don't spray logs when dquot flush/purge fail Darrick J. Wong
14 siblings, 2 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-03 17:53 UTC (permalink / raw)
To: linux-xfs, bfoster, hch
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:
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-05-03 17:53 ` [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error Darrick J. Wong
@ 2018-05-04 11:31 ` Brian Foster
2018-05-04 15:12 ` Darrick J. Wong
2018-05-04 21:19 ` [PATCH v2 " Darrick J. Wong
1 sibling, 1 reply; 51+ messages in thread
From: Brian Foster @ 2018-05-04 11:31 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-05-04 11:31 ` Brian Foster
@ 2018-05-04 15:12 ` Darrick J. Wong
2018-05-04 15:41 ` Brian Foster
0 siblings, 1 reply; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-04 15:12 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-05-04 15:12 ` Darrick J. Wong
@ 2018-05-04 15:41 ` Brian Foster
2018-05-04 15:52 ` Darrick J. Wong
0 siblings, 1 reply; 51+ messages in thread
From: Brian Foster @ 2018-05-04 15:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-05-04 15:41 ` Brian Foster
@ 2018-05-04 15:52 ` Darrick J. Wong
2018-05-04 16:03 ` Brian Foster
0 siblings, 1 reply; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-04 15:52 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-05-04 15:52 ` Darrick J. Wong
@ 2018-05-04 16:03 ` Brian Foster
2018-05-04 20:05 ` Darrick J. Wong
0 siblings, 1 reply; 51+ messages in thread
From: Brian Foster @ 2018-05-04 16:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-05-04 16:03 ` Brian Foster
@ 2018-05-04 20:05 ` Darrick J. Wong
0 siblings, 0 replies; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-04 20:05 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-05-03 17:53 ` [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error Darrick J. Wong
2018-05-04 11:31 ` Brian Foster
@ 2018-05-04 21:19 ` Darrick J. Wong
2018-05-07 11:03 ` Brian Foster
1 sibling, 1 reply; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-04 21:19 UTC (permalink / raw)
To: linux-xfs, bfoster, hch
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;
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v2 0.1/13] xfs: release new dquot buffer on defer_finish error
2018-05-04 21:19 ` [PATCH v2 " Darrick J. Wong
@ 2018-05-07 11:03 ` Brian Foster
0 siblings, 0 replies; 51+ messages in thread
From: Brian Foster @ 2018-05-07 11:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 0.2/13] xfs: don't spray logs when dquot flush/purge fail
2018-04-30 5:43 [PATCH v2 00/13] xfs-4.18: quota refactor Darrick J. Wong
` (13 preceding siblings ...)
2018-05-03 17:53 ` [PATCH 0.1/13] xfs: release new dquot buffer on defer_finish error Darrick J. Wong
@ 2018-05-03 17:54 ` Darrick J. Wong
2018-05-04 11:32 ` Brian Foster
14 siblings, 1 reply; 51+ messages in thread
From: Darrick J. Wong @ 2018-05-03 17:54 UTC (permalink / raw)
To: linux-xfs, bfoster, hch
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);
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 0.2/13] xfs: don't spray logs when dquot flush/purge fail
2018-05-03 17:54 ` [PATCH 0.2/13] xfs: don't spray logs when dquot flush/purge fail Darrick J. Wong
@ 2018-05-04 11:32 ` Brian Foster
0 siblings, 0 replies; 51+ messages in thread
From: Brian Foster @ 2018-05-04 11:32 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
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
^ permalink raw reply [flat|nested] 51+ messages in thread