* [PATCH v2 1/4] xfs: do the ASSERT for the arguments O_{u,g,p}dqpp
2020-09-26 13:14 [PATCH v2 0/4] xfs: random fixes for disk quota xiakaixu1987
@ 2020-09-26 13:14 ` xiakaixu1987
2020-10-06 4:35 ` Darrick J. Wong
2020-09-26 13:14 ` [PATCH v2 2/4] xfs: fix the indent in xfs_trans_mod_dquot xiakaixu1987
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: xiakaixu1987 @ 2020-09-26 13:14 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
If we pass in XFS_QMOPT_{U,G,P}QUOTA flags and different uid/gid/prid
than them currently associated with the inode, the arguments
O_{u,g,p}dqpp shouldn't be NULL, so add the ASSERT for them.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_qm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 44509decb4cd..b2a9abee8b2b 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1662,6 +1662,7 @@ xfs_qm_vop_dqalloc(
}
if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
+ ASSERT(O_udqpp);
if (!uid_eq(inode->i_uid, uid)) {
/*
* What we need is the dquot that has this uid, and
@@ -1695,6 +1696,7 @@ xfs_qm_vop_dqalloc(
}
}
if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
+ ASSERT(O_gdqpp);
if (!gid_eq(inode->i_gid, gid)) {
xfs_iunlock(ip, lockflags);
error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
@@ -1712,6 +1714,7 @@ xfs_qm_vop_dqalloc(
}
}
if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
+ ASSERT(O_pdqpp);
if (ip->i_d.di_projid != prid) {
xfs_iunlock(ip, lockflags);
error = xfs_qm_dqget(mp, prid,
--
2.20.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 1/4] xfs: do the ASSERT for the arguments O_{u,g,p}dqpp
2020-09-26 13:14 ` [PATCH v2 1/4] xfs: do the ASSERT for the arguments O_{u,g,p}dqpp xiakaixu1987
@ 2020-10-06 4:35 ` Darrick J. Wong
0 siblings, 0 replies; 11+ messages in thread
From: Darrick J. Wong @ 2020-10-06 4:35 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Sat, Sep 26, 2020 at 09:14:30PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> If we pass in XFS_QMOPT_{U,G,P}QUOTA flags and different uid/gid/prid
> than them currently associated with the inode, the arguments
> O_{u,g,p}dqpp shouldn't be NULL, so add the ASSERT for them.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Ok, seems fine to me...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_qm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 44509decb4cd..b2a9abee8b2b 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1662,6 +1662,7 @@ xfs_qm_vop_dqalloc(
> }
>
> if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
> + ASSERT(O_udqpp);
> if (!uid_eq(inode->i_uid, uid)) {
> /*
> * What we need is the dquot that has this uid, and
> @@ -1695,6 +1696,7 @@ xfs_qm_vop_dqalloc(
> }
> }
> if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
> + ASSERT(O_gdqpp);
> if (!gid_eq(inode->i_gid, gid)) {
> xfs_iunlock(ip, lockflags);
> error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
> @@ -1712,6 +1714,7 @@ xfs_qm_vop_dqalloc(
> }
> }
> if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
> + ASSERT(O_pdqpp);
> if (ip->i_d.di_projid != prid) {
> xfs_iunlock(ip, lockflags);
> error = xfs_qm_dqget(mp, prid,
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] xfs: fix the indent in xfs_trans_mod_dquot
2020-09-26 13:14 [PATCH v2 0/4] xfs: random fixes for disk quota xiakaixu1987
2020-09-26 13:14 ` [PATCH v2 1/4] xfs: do the ASSERT for the arguments O_{u,g,p}dqpp xiakaixu1987
@ 2020-09-26 13:14 ` xiakaixu1987
2020-10-06 4:36 ` Darrick J. Wong
2020-09-26 13:14 ` [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
2020-09-26 13:14 ` [PATCH v2 4/4] xfs: directly return if the delta equal to zero xiakaixu1987
3 siblings, 1 reply; 11+ messages in thread
From: xiakaixu1987 @ 2020-09-26 13:14 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
The formatting is strange in xfs_trans_mod_dquot, so do a reindent.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_trans_dquot.c | 43 ++++++++++++++--------------------------
1 file changed, 15 insertions(+), 28 deletions(-)
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 133fc6fc3edd..fe45b0c3970c 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -221,36 +221,27 @@ xfs_trans_mod_dquot(
}
switch (field) {
-
- /*
- * regular disk blk reservation
- */
- case XFS_TRANS_DQ_RES_BLKS:
+ /* regular disk blk reservation */
+ case XFS_TRANS_DQ_RES_BLKS:
qtrx->qt_blk_res += delta;
break;
- /*
- * inode reservation
- */
- case XFS_TRANS_DQ_RES_INOS:
+ /* inode reservation */
+ case XFS_TRANS_DQ_RES_INOS:
qtrx->qt_ino_res += delta;
break;
- /*
- * disk blocks used.
- */
- case XFS_TRANS_DQ_BCOUNT:
+ /* disk blocks used. */
+ case XFS_TRANS_DQ_BCOUNT:
qtrx->qt_bcount_delta += delta;
break;
- case XFS_TRANS_DQ_DELBCOUNT:
+ case XFS_TRANS_DQ_DELBCOUNT:
qtrx->qt_delbcnt_delta += delta;
break;
- /*
- * Inode Count
- */
- case XFS_TRANS_DQ_ICOUNT:
+ /* Inode Count */
+ case XFS_TRANS_DQ_ICOUNT:
if (qtrx->qt_ino_res && delta > 0) {
qtrx->qt_ino_res_used += delta;
ASSERT(qtrx->qt_ino_res >= qtrx->qt_ino_res_used);
@@ -258,17 +249,13 @@ xfs_trans_mod_dquot(
qtrx->qt_icount_delta += delta;
break;
- /*
- * rtblk reservation
- */
- case XFS_TRANS_DQ_RES_RTBLKS:
+ /* rtblk reservation */
+ case XFS_TRANS_DQ_RES_RTBLKS:
qtrx->qt_rtblk_res += delta;
break;
- /*
- * rtblk count
- */
- case XFS_TRANS_DQ_RTBCOUNT:
+ /* rtblk count */
+ case XFS_TRANS_DQ_RTBCOUNT:
if (qtrx->qt_rtblk_res && delta > 0) {
qtrx->qt_rtblk_res_used += delta;
ASSERT(qtrx->qt_rtblk_res >= qtrx->qt_rtblk_res_used);
@@ -276,11 +263,11 @@ xfs_trans_mod_dquot(
qtrx->qt_rtbcount_delta += delta;
break;
- case XFS_TRANS_DQ_DELRTBCOUNT:
+ case XFS_TRANS_DQ_DELRTBCOUNT:
qtrx->qt_delrtb_delta += delta;
break;
- default:
+ default:
ASSERT(0);
}
--
2.20.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/4] xfs: fix the indent in xfs_trans_mod_dquot
2020-09-26 13:14 ` [PATCH v2 2/4] xfs: fix the indent in xfs_trans_mod_dquot xiakaixu1987
@ 2020-10-06 4:36 ` Darrick J. Wong
0 siblings, 0 replies; 11+ messages in thread
From: Darrick J. Wong @ 2020-10-06 4:36 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Sat, Sep 26, 2020 at 09:14:31PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> The formatting is strange in xfs_trans_mod_dquot, so do a reindent.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
"strange". I can just picture the Papyrus font now... :)
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_trans_dquot.c | 43 ++++++++++++++--------------------------
> 1 file changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index 133fc6fc3edd..fe45b0c3970c 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -221,36 +221,27 @@ xfs_trans_mod_dquot(
> }
>
> switch (field) {
> -
> - /*
> - * regular disk blk reservation
> - */
> - case XFS_TRANS_DQ_RES_BLKS:
> + /* regular disk blk reservation */
> + case XFS_TRANS_DQ_RES_BLKS:
> qtrx->qt_blk_res += delta;
> break;
>
> - /*
> - * inode reservation
> - */
> - case XFS_TRANS_DQ_RES_INOS:
> + /* inode reservation */
> + case XFS_TRANS_DQ_RES_INOS:
> qtrx->qt_ino_res += delta;
> break;
>
> - /*
> - * disk blocks used.
> - */
> - case XFS_TRANS_DQ_BCOUNT:
> + /* disk blocks used. */
> + case XFS_TRANS_DQ_BCOUNT:
> qtrx->qt_bcount_delta += delta;
> break;
>
> - case XFS_TRANS_DQ_DELBCOUNT:
> + case XFS_TRANS_DQ_DELBCOUNT:
> qtrx->qt_delbcnt_delta += delta;
> break;
>
> - /*
> - * Inode Count
> - */
> - case XFS_TRANS_DQ_ICOUNT:
> + /* Inode Count */
> + case XFS_TRANS_DQ_ICOUNT:
> if (qtrx->qt_ino_res && delta > 0) {
> qtrx->qt_ino_res_used += delta;
> ASSERT(qtrx->qt_ino_res >= qtrx->qt_ino_res_used);
> @@ -258,17 +249,13 @@ xfs_trans_mod_dquot(
> qtrx->qt_icount_delta += delta;
> break;
>
> - /*
> - * rtblk reservation
> - */
> - case XFS_TRANS_DQ_RES_RTBLKS:
> + /* rtblk reservation */
> + case XFS_TRANS_DQ_RES_RTBLKS:
> qtrx->qt_rtblk_res += delta;
> break;
>
> - /*
> - * rtblk count
> - */
> - case XFS_TRANS_DQ_RTBCOUNT:
> + /* rtblk count */
> + case XFS_TRANS_DQ_RTBCOUNT:
> if (qtrx->qt_rtblk_res && delta > 0) {
> qtrx->qt_rtblk_res_used += delta;
> ASSERT(qtrx->qt_rtblk_res >= qtrx->qt_rtblk_res_used);
> @@ -276,11 +263,11 @@ xfs_trans_mod_dquot(
> qtrx->qt_rtbcount_delta += delta;
> break;
>
> - case XFS_TRANS_DQ_DELRTBCOUNT:
> + case XFS_TRANS_DQ_DELRTBCOUNT:
> qtrx->qt_delrtb_delta += delta;
> break;
>
> - default:
> + default:
> ASSERT(0);
> }
>
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
2020-09-26 13:14 [PATCH v2 0/4] xfs: random fixes for disk quota xiakaixu1987
2020-09-26 13:14 ` [PATCH v2 1/4] xfs: do the ASSERT for the arguments O_{u,g,p}dqpp xiakaixu1987
2020-09-26 13:14 ` [PATCH v2 2/4] xfs: fix the indent in xfs_trans_mod_dquot xiakaixu1987
@ 2020-09-26 13:14 ` xiakaixu1987
2020-09-26 15:36 ` Eric Sandeen
2020-10-06 4:42 ` Darrick J. Wong
2020-09-26 13:14 ` [PATCH v2 4/4] xfs: directly return if the delta equal to zero xiakaixu1987
3 siblings, 2 replies; 11+ messages in thread
From: xiakaixu1987 @ 2020-09-26 13:14 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
Nowadays the only things that the XFS_TRANS_DQ_DIRTY flag seems to do
are indicates the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values
changed and check in xfs_trans_apply_dquot_deltas() and the unreserve
variant xfs_trans_unreserve_and_mod_dquots(). Actually, we also can
use the tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag, that
is to say, we allocate the new tp->t_dqinfo only when the qtrx values
changed, so the tp->t_dqinfo value isn't NULL equals the XFS_TRANS_DQ_DIRTY
flag is set, we only need to check if tp->t_dqinfo == NULL in
xfs_trans_apply_dquot_deltas() and its unreserve variant to determine
whether lock all of the dquots and join them to the transaction.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/libxfs/xfs_shared.h | 1 -
fs/xfs/xfs_inode.c | 8 +-------
fs/xfs/xfs_trans_dquot.c | 20 ++------------------
3 files changed, 3 insertions(+), 26 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index c795ae47b3c9..8c61a461bf7b 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -62,7 +62,6 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
#define XFS_TRANS_SB_DIRTY 0x02 /* superblock is modified */
#define XFS_TRANS_PERM_LOG_RES 0x04 /* xact took a permanent log res */
#define XFS_TRANS_SYNC 0x08 /* make commit synchronous */
-#define XFS_TRANS_DQ_DIRTY 0x10 /* at least one dquot in trx dirty */
#define XFS_TRANS_RESERVE 0x20 /* OK to use reserved data blocks */
#define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
#define XFS_TRANS_RES_FDBLKS 0x80 /* reserve newly freed blocks */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 49624973eecc..9108eed0ea45 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -941,7 +941,6 @@ xfs_dir_ialloc(
xfs_buf_t *ialloc_context = NULL;
int code;
void *dqinfo;
- uint tflags;
tp = *tpp;
ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
@@ -1000,12 +999,9 @@ xfs_dir_ialloc(
* and attach it to the next transaction.
*/
dqinfo = NULL;
- tflags = 0;
if (tp->t_dqinfo) {
dqinfo = (void *)tp->t_dqinfo;
tp->t_dqinfo = NULL;
- tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
- tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
}
code = xfs_trans_roll(&tp);
@@ -1013,10 +1009,8 @@ xfs_dir_ialloc(
/*
* Re-attach the quota info that we detached from prev trx.
*/
- if (dqinfo) {
+ if (dqinfo)
tp->t_dqinfo = dqinfo;
- tp->t_flags |= tflags;
- }
if (code) {
xfs_buf_relse(ialloc_context);
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index fe45b0c3970c..0ebfd7930382 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -84,13 +84,6 @@ xfs_trans_dup_dqinfo(
xfs_trans_alloc_dqinfo(ntp);
- /*
- * Because the quota blk reservation is carried forward,
- * it is also necessary to carry forward the DQ_DIRTY flag.
- */
- if (otp->t_flags & XFS_TRANS_DQ_DIRTY)
- ntp->t_flags |= XFS_TRANS_DQ_DIRTY;
-
for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
oqa = otp->t_dqinfo->dqs[j];
nqa = ntp->t_dqinfo->dqs[j];
@@ -143,9 +136,6 @@ xfs_trans_mod_dquot_byino(
xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
return;
- if (tp->t_dqinfo == NULL)
- xfs_trans_alloc_dqinfo(tp);
-
if (XFS_IS_UQUOTA_ON(mp) && ip->i_udquot)
(void) xfs_trans_mod_dquot(tp, ip->i_udquot, field, delta);
if (XFS_IS_GQUOTA_ON(mp) && ip->i_gdquot)
@@ -273,8 +263,6 @@ xfs_trans_mod_dquot(
if (delta)
trace_xfs_trans_mod_dquot_after(qtrx);
-
- tp->t_flags |= XFS_TRANS_DQ_DIRTY;
}
@@ -351,7 +339,7 @@ xfs_trans_apply_dquot_deltas(
int64_t totalbdelta;
int64_t totalrtbdelta;
- if (!(tp->t_flags & XFS_TRANS_DQ_DIRTY))
+ if (!tp->t_dqinfo)
return;
ASSERT(tp->t_dqinfo);
@@ -493,7 +481,7 @@ xfs_trans_unreserve_and_mod_dquots(
struct xfs_dqtrx *qtrx, *qa;
bool locked;
- if (!tp->t_dqinfo || !(tp->t_flags & XFS_TRANS_DQ_DIRTY))
+ if (!tp->t_dqinfo)
return;
for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
@@ -698,7 +686,6 @@ xfs_trans_dqresv(
* because we don't have the luxury of a transaction envelope then.
*/
if (tp) {
- ASSERT(tp->t_dqinfo);
ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
if (nblks != 0)
xfs_trans_mod_dquot(tp, dqp,
@@ -752,9 +739,6 @@ xfs_trans_reserve_quota_bydquots(
if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
return 0;
- if (tp && tp->t_dqinfo == NULL)
- xfs_trans_alloc_dqinfo(tp);
-
ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
if (udqp) {
--
2.20.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
2020-09-26 13:14 ` [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
@ 2020-09-26 15:36 ` Eric Sandeen
2020-10-02 3:31 ` kaixuxia
2020-10-06 4:42 ` Darrick J. Wong
1 sibling, 1 reply; 11+ messages in thread
From: Eric Sandeen @ 2020-09-26 15:36 UTC (permalink / raw)
To: xiakaixu1987, linux-xfs; +Cc: darrick.wong, Kaixu Xia
On 9/26/20 8:14 AM, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Nowadays the only things that the XFS_TRANS_DQ_DIRTY flag seems to do
> are indicates the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values
> changed and check in xfs_trans_apply_dquot_deltas() and the unreserve
> variant xfs_trans_unreserve_and_mod_dquots(). Actually, we also can
> use the tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag, that
> is to say, we allocate the new tp->t_dqinfo only when the qtrx values
> changed, so the tp->t_dqinfo value isn't NULL equals the XFS_TRANS_DQ_DIRTY
> flag is set, we only need to check if tp->t_dqinfo == NULL in
> xfs_trans_apply_dquot_deltas() and its unreserve variant to determine
> whether lock all of the dquots and join them to the transaction.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
> fs/xfs/libxfs/xfs_shared.h | 1 -
> fs/xfs/xfs_inode.c | 8 +-------
> fs/xfs/xfs_trans_dquot.c | 20 ++------------------
> 3 files changed, 3 insertions(+), 26 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index c795ae47b3c9..8c61a461bf7b 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -62,7 +62,6 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
> #define XFS_TRANS_SB_DIRTY 0x02 /* superblock is modified */
> #define XFS_TRANS_PERM_LOG_RES 0x04 /* xact took a permanent log res */
> #define XFS_TRANS_SYNC 0x08 /* make commit synchronous */
> -#define XFS_TRANS_DQ_DIRTY 0x10 /* at least one dquot in trx dirty */
> #define XFS_TRANS_RESERVE 0x20 /* OK to use reserved data blocks */
> #define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
> #define XFS_TRANS_RES_FDBLKS 0x80 /* reserve newly freed blocks */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 49624973eecc..9108eed0ea45 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -941,7 +941,6 @@ xfs_dir_ialloc(
> xfs_buf_t *ialloc_context = NULL;
> int code;
> void *dqinfo;
> - uint tflags;
>
> tp = *tpp;
> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> @@ -1000,12 +999,9 @@ xfs_dir_ialloc(
> * and attach it to the next transaction.
> */
> dqinfo = NULL;
> - tflags = 0;
> if (tp->t_dqinfo) {
> dqinfo = (void *)tp->t_dqinfo;
> tp->t_dqinfo = NULL;
> - tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
> - tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
> }
>
> code = xfs_trans_roll(&tp);
> @@ -1013,10 +1009,8 @@ xfs_dir_ialloc(
> /*
> * Re-attach the quota info that we detached from prev trx.
> */
> - if (dqinfo) {
> + if (dqinfo)
> tp->t_dqinfo = dqinfo;
> - tp->t_flags |= tflags;
> - }
>
> if (code) {
> xfs_buf_relse(ialloc_context);
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index fe45b0c3970c..0ebfd7930382 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -84,13 +84,6 @@ xfs_trans_dup_dqinfo(
>
> xfs_trans_alloc_dqinfo(ntp);
>
> - /*
> - * Because the quota blk reservation is carried forward,
> - * it is also necessary to carry forward the DQ_DIRTY flag.
> - */
> - if (otp->t_flags & XFS_TRANS_DQ_DIRTY)
> - ntp->t_flags |= XFS_TRANS_DQ_DIRTY;
> -
> for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
> oqa = otp->t_dqinfo->dqs[j];
> nqa = ntp->t_dqinfo->dqs[j];
> @@ -143,9 +136,6 @@ xfs_trans_mod_dquot_byino(
> xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
> return;
>
> - if (tp->t_dqinfo == NULL)
> - xfs_trans_alloc_dqinfo(tp);
> -
I can't tell from the commit log or from a very quick read of the code why these
allocations are being removed. Can we not get here with a NULL t_dqinfo?
If not, why not? This seems like a change unrelated to the proposed
"t_dqinfo set == XFS_TRANS_DQ_DIRTY" change.
Also, while it seems clear to say that !t_dqinfo == !XFS_TRANS_DQ_DIRTY, is the
converse true? Is it possible to have t_dqinfo set, but it's not dirty?
I think the answer is that when we free the transaction we set t_dqinfo to
NULL again, but I'm not certain, and it's not obvious from the changelog...
-Eric
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
2020-09-26 15:36 ` Eric Sandeen
@ 2020-10-02 3:31 ` kaixuxia
0 siblings, 0 replies; 11+ messages in thread
From: kaixuxia @ 2020-10-02 3:31 UTC (permalink / raw)
To: Eric Sandeen, linux-xfs; +Cc: darrick.wong, Kaixu Xia
On 2020/9/26 23:36, Eric Sandeen wrote:
> On 9/26/20 8:14 AM, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> Nowadays the only things that the XFS_TRANS_DQ_DIRTY flag seems to do
>> are indicates the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values
>> changed and check in xfs_trans_apply_dquot_deltas() and the unreserve
>> variant xfs_trans_unreserve_and_mod_dquots(). Actually, we also can
>> use the tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag, that
>> is to say, we allocate the new tp->t_dqinfo only when the qtrx values
>> changed, so the tp->t_dqinfo value isn't NULL equals the XFS_TRANS_DQ_DIRTY
>> flag is set, we only need to check if tp->t_dqinfo == NULL in
>> xfs_trans_apply_dquot_deltas() and its unreserve variant to determine
>> whether lock all of the dquots and join them to the transaction.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>> fs/xfs/libxfs/xfs_shared.h | 1 -
>> fs/xfs/xfs_inode.c | 8 +-------
>> fs/xfs/xfs_trans_dquot.c | 20 ++------------------
>> 3 files changed, 3 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
>> index c795ae47b3c9..8c61a461bf7b 100644
>> --- a/fs/xfs/libxfs/xfs_shared.h
>> +++ b/fs/xfs/libxfs/xfs_shared.h
>> @@ -62,7 +62,6 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
>> #define XFS_TRANS_SB_DIRTY 0x02 /* superblock is modified */
>> #define XFS_TRANS_PERM_LOG_RES 0x04 /* xact took a permanent log res */
>> #define XFS_TRANS_SYNC 0x08 /* make commit synchronous */
>> -#define XFS_TRANS_DQ_DIRTY 0x10 /* at least one dquot in trx dirty */
>> #define XFS_TRANS_RESERVE 0x20 /* OK to use reserved data blocks */
>> #define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
>> #define XFS_TRANS_RES_FDBLKS 0x80 /* reserve newly freed blocks */
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 49624973eecc..9108eed0ea45 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -941,7 +941,6 @@ xfs_dir_ialloc(
>> xfs_buf_t *ialloc_context = NULL;
>> int code;
>> void *dqinfo;
>> - uint tflags;
>>
>> tp = *tpp;
>> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
>> @@ -1000,12 +999,9 @@ xfs_dir_ialloc(
>> * and attach it to the next transaction.
>> */
>> dqinfo = NULL;
>> - tflags = 0;
>> if (tp->t_dqinfo) {
>> dqinfo = (void *)tp->t_dqinfo;
>> tp->t_dqinfo = NULL;
>> - tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
>> - tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
>> }
>>
>> code = xfs_trans_roll(&tp);
>> @@ -1013,10 +1009,8 @@ xfs_dir_ialloc(
>> /*
>> * Re-attach the quota info that we detached from prev trx.
>> */
>> - if (dqinfo) {
>> + if (dqinfo)
>> tp->t_dqinfo = dqinfo;
>> - tp->t_flags |= tflags;
>> - }
>>
>> if (code) {
>> xfs_buf_relse(ialloc_context);
>> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
>> index fe45b0c3970c..0ebfd7930382 100644
>> --- a/fs/xfs/xfs_trans_dquot.c
>> +++ b/fs/xfs/xfs_trans_dquot.c
>> @@ -84,13 +84,6 @@ xfs_trans_dup_dqinfo(
>>
>> xfs_trans_alloc_dqinfo(ntp);
>>
>> - /*
>> - * Because the quota blk reservation is carried forward,
>> - * it is also necessary to carry forward the DQ_DIRTY flag.
>> - */
>> - if (otp->t_flags & XFS_TRANS_DQ_DIRTY)
>> - ntp->t_flags |= XFS_TRANS_DQ_DIRTY;
>> -
>> for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
>> oqa = otp->t_dqinfo->dqs[j];
>> nqa = ntp->t_dqinfo->dqs[j];
>> @@ -143,9 +136,6 @@ xfs_trans_mod_dquot_byino(
>> xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
>> return;
>>
>> - if (tp->t_dqinfo == NULL)
>> - xfs_trans_alloc_dqinfo(tp);
>> -
>
> I can't tell from the commit log or from a very quick read of the code why these
> allocations are being removed. Can we not get here with a NULL t_dqinfo?
> If not, why not? This seems like a change unrelated to the proposed
> "t_dqinfo set == XFS_TRANS_DQ_DIRTY" change.
Yeah, remove these allocations because I want to allocate the t_dqinfo only
when the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values changed, that
is to say, only do the allocation in xfs_trans_mod_dquot() function. Actually,
original these allocations are repeated, for example, the xfs_trans_mod_dquot_byino()
function call the xfs_trans_mod_dquot(), but both of them do the allocation,
so remove one of them may be reasonable.
>
> Also, while it seems clear to say that !t_dqinfo == !XFS_TRANS_DQ_DIRTY, is the
> converse true? Is it possible to have t_dqinfo set, but it's not dirty?>
> I think the answer is that when we free the transaction we set t_dqinfo to
> NULL again, but I'm not certain, and it's not obvious from the changelog...
Now we do the allocation in xfs_trans_mod_dquot() function and only have t_dqinfo
set when it is dirty.
Thanks,
Kaixu
>
> -Eric
>
--
kaixuxia
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
2020-09-26 13:14 ` [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
2020-09-26 15:36 ` Eric Sandeen
@ 2020-10-06 4:42 ` Darrick J. Wong
2020-10-06 11:33 ` kaixuxia
1 sibling, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2020-10-06 4:42 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Sat, Sep 26, 2020 at 09:14:32PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Nowadays the only things that the XFS_TRANS_DQ_DIRTY flag seems to do
> are indicates the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values
> changed and check in xfs_trans_apply_dquot_deltas() and the unreserve
> variant xfs_trans_unreserve_and_mod_dquots(). Actually, we also can
> use the tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag, that
> is to say, we allocate the new tp->t_dqinfo only when the qtrx values
> changed, so the tp->t_dqinfo value isn't NULL equals the XFS_TRANS_DQ_DIRTY
> flag is set, we only need to check if tp->t_dqinfo == NULL in
> xfs_trans_apply_dquot_deltas() and its unreserve variant to determine
> whether lock all of the dquots and join them to the transaction.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
> fs/xfs/libxfs/xfs_shared.h | 1 -
> fs/xfs/xfs_inode.c | 8 +-------
> fs/xfs/xfs_trans_dquot.c | 20 ++------------------
> 3 files changed, 3 insertions(+), 26 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index c795ae47b3c9..8c61a461bf7b 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -62,7 +62,6 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
> #define XFS_TRANS_SB_DIRTY 0x02 /* superblock is modified */
> #define XFS_TRANS_PERM_LOG_RES 0x04 /* xact took a permanent log res */
> #define XFS_TRANS_SYNC 0x08 /* make commit synchronous */
> -#define XFS_TRANS_DQ_DIRTY 0x10 /* at least one dquot in trx dirty */
> #define XFS_TRANS_RESERVE 0x20 /* OK to use reserved data blocks */
> #define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
> #define XFS_TRANS_RES_FDBLKS 0x80 /* reserve newly freed blocks */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 49624973eecc..9108eed0ea45 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -941,7 +941,6 @@ xfs_dir_ialloc(
> xfs_buf_t *ialloc_context = NULL;
> int code;
> void *dqinfo;
> - uint tflags;
>
> tp = *tpp;
> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> @@ -1000,12 +999,9 @@ xfs_dir_ialloc(
> * and attach it to the next transaction.
> */
> dqinfo = NULL;
> - tflags = 0;
> if (tp->t_dqinfo) {
> dqinfo = (void *)tp->t_dqinfo;
> tp->t_dqinfo = NULL;
> - tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
> - tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
> }
>
> code = xfs_trans_roll(&tp);
> @@ -1013,10 +1009,8 @@ xfs_dir_ialloc(
> /*
> * Re-attach the quota info that we detached from prev trx.
> */
> - if (dqinfo) {
> + if (dqinfo)
> tp->t_dqinfo = dqinfo;
> - tp->t_flags |= tflags;
> - }
>
> if (code) {
> xfs_buf_relse(ialloc_context);
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index fe45b0c3970c..0ebfd7930382 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -84,13 +84,6 @@ xfs_trans_dup_dqinfo(
>
> xfs_trans_alloc_dqinfo(ntp);
>
> - /*
> - * Because the quota blk reservation is carried forward,
> - * it is also necessary to carry forward the DQ_DIRTY flag.
> - */
> - if (otp->t_flags & XFS_TRANS_DQ_DIRTY)
> - ntp->t_flags |= XFS_TRANS_DQ_DIRTY;
> -
> for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
> oqa = otp->t_dqinfo->dqs[j];
> nqa = ntp->t_dqinfo->dqs[j];
> @@ -143,9 +136,6 @@ xfs_trans_mod_dquot_byino(
> xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
> return;
>
> - if (tp->t_dqinfo == NULL)
> - xfs_trans_alloc_dqinfo(tp);
This change ought to be a separate clean patch.
> -
> if (XFS_IS_UQUOTA_ON(mp) && ip->i_udquot)
> (void) xfs_trans_mod_dquot(tp, ip->i_udquot, field, delta);
> if (XFS_IS_GQUOTA_ON(mp) && ip->i_gdquot)
> @@ -273,8 +263,6 @@ xfs_trans_mod_dquot(
>
> if (delta)
> trace_xfs_trans_mod_dquot_after(qtrx);
> -
> - tp->t_flags |= XFS_TRANS_DQ_DIRTY;
> }
>
>
> @@ -351,7 +339,7 @@ xfs_trans_apply_dquot_deltas(
> int64_t totalbdelta;
> int64_t totalrtbdelta;
>
> - if (!(tp->t_flags & XFS_TRANS_DQ_DIRTY))
> + if (!tp->t_dqinfo)
> return;
>
> ASSERT(tp->t_dqinfo);
> @@ -493,7 +481,7 @@ xfs_trans_unreserve_and_mod_dquots(
> struct xfs_dqtrx *qtrx, *qa;
> bool locked;
>
> - if (!tp->t_dqinfo || !(tp->t_flags & XFS_TRANS_DQ_DIRTY))
> + if (!tp->t_dqinfo)
> return;
>
> for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
> @@ -698,7 +686,6 @@ xfs_trans_dqresv(
> * because we don't have the luxury of a transaction envelope then.
> */
> if (tp) {
> - ASSERT(tp->t_dqinfo);
> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
> if (nblks != 0)
> xfs_trans_mod_dquot(tp, dqp,
> @@ -752,9 +739,6 @@ xfs_trans_reserve_quota_bydquots(
> if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
> return 0;
>
> - if (tp && tp->t_dqinfo == NULL)
> - xfs_trans_alloc_dqinfo(tp);
Um, who allocates the dqinfo in this case?
--D
> -
> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>
> if (udqp) {
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
2020-10-06 4:42 ` Darrick J. Wong
@ 2020-10-06 11:33 ` kaixuxia
0 siblings, 0 replies; 11+ messages in thread
From: kaixuxia @ 2020-10-06 11:33 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Kaixu Xia
On 2020/10/6 12:42, Darrick J. Wong wrote:
> On Sat, Sep 26, 2020 at 09:14:32PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> Nowadays the only things that the XFS_TRANS_DQ_DIRTY flag seems to do
>> are indicates the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values
>> changed and check in xfs_trans_apply_dquot_deltas() and the unreserve
>> variant xfs_trans_unreserve_and_mod_dquots(). Actually, we also can
>> use the tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag, that
>> is to say, we allocate the new tp->t_dqinfo only when the qtrx values
>> changed, so the tp->t_dqinfo value isn't NULL equals the XFS_TRANS_DQ_DIRTY
>> flag is set, we only need to check if tp->t_dqinfo == NULL in
>> xfs_trans_apply_dquot_deltas() and its unreserve variant to determine
>> whether lock all of the dquots and join them to the transaction.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>> fs/xfs/libxfs/xfs_shared.h | 1 -
>> fs/xfs/xfs_inode.c | 8 +-------
>> fs/xfs/xfs_trans_dquot.c | 20 ++------------------
>> 3 files changed, 3 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
>> index c795ae47b3c9..8c61a461bf7b 100644
>> --- a/fs/xfs/libxfs/xfs_shared.h
>> +++ b/fs/xfs/libxfs/xfs_shared.h
>> @@ -62,7 +62,6 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
>> #define XFS_TRANS_SB_DIRTY 0x02 /* superblock is modified */
>> #define XFS_TRANS_PERM_LOG_RES 0x04 /* xact took a permanent log res */
>> #define XFS_TRANS_SYNC 0x08 /* make commit synchronous */
>> -#define XFS_TRANS_DQ_DIRTY 0x10 /* at least one dquot in trx dirty */
>> #define XFS_TRANS_RESERVE 0x20 /* OK to use reserved data blocks */
>> #define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
>> #define XFS_TRANS_RES_FDBLKS 0x80 /* reserve newly freed blocks */
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 49624973eecc..9108eed0ea45 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -941,7 +941,6 @@ xfs_dir_ialloc(
>> xfs_buf_t *ialloc_context = NULL;
>> int code;
>> void *dqinfo;
>> - uint tflags;
>>
>> tp = *tpp;
>> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
>> @@ -1000,12 +999,9 @@ xfs_dir_ialloc(
>> * and attach it to the next transaction.
>> */
>> dqinfo = NULL;
>> - tflags = 0;
>> if (tp->t_dqinfo) {
>> dqinfo = (void *)tp->t_dqinfo;
>> tp->t_dqinfo = NULL;
>> - tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
>> - tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
>> }
>>
>> code = xfs_trans_roll(&tp);
>> @@ -1013,10 +1009,8 @@ xfs_dir_ialloc(
>> /*
>> * Re-attach the quota info that we detached from prev trx.
>> */
>> - if (dqinfo) {
>> + if (dqinfo)
>> tp->t_dqinfo = dqinfo;
>> - tp->t_flags |= tflags;
>> - }
>>
>> if (code) {
>> xfs_buf_relse(ialloc_context);
>> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
>> index fe45b0c3970c..0ebfd7930382 100644
>> --- a/fs/xfs/xfs_trans_dquot.c
>> +++ b/fs/xfs/xfs_trans_dquot.c
>> @@ -84,13 +84,6 @@ xfs_trans_dup_dqinfo(
>>
>> xfs_trans_alloc_dqinfo(ntp);
>>
>> - /*
>> - * Because the quota blk reservation is carried forward,
>> - * it is also necessary to carry forward the DQ_DIRTY flag.
>> - */
>> - if (otp->t_flags & XFS_TRANS_DQ_DIRTY)
>> - ntp->t_flags |= XFS_TRANS_DQ_DIRTY;
>> -
>> for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
>> oqa = otp->t_dqinfo->dqs[j];
>> nqa = ntp->t_dqinfo->dqs[j];
>> @@ -143,9 +136,6 @@ xfs_trans_mod_dquot_byino(
>> xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
>> return;
>>
>> - if (tp->t_dqinfo == NULL)
>> - xfs_trans_alloc_dqinfo(tp);
>
> This change ought to be a separate clean patch.
Yeah, I'll do that in the next version.
>
>> -
>> if (XFS_IS_UQUOTA_ON(mp) && ip->i_udquot)
>> (void) xfs_trans_mod_dquot(tp, ip->i_udquot, field, delta);
>> if (XFS_IS_GQUOTA_ON(mp) && ip->i_gdquot)
>> @@ -273,8 +263,6 @@ xfs_trans_mod_dquot(
>>
>> if (delta)
>> trace_xfs_trans_mod_dquot_after(qtrx);
>> -
>> - tp->t_flags |= XFS_TRANS_DQ_DIRTY;
>> }
>>
>>
>> @@ -351,7 +339,7 @@ xfs_trans_apply_dquot_deltas(
>> int64_t totalbdelta;
>> int64_t totalrtbdelta;
>>
>> - if (!(tp->t_flags & XFS_TRANS_DQ_DIRTY))
>> + if (!tp->t_dqinfo)
>> return;
>>
>> ASSERT(tp->t_dqinfo);
>> @@ -493,7 +481,7 @@ xfs_trans_unreserve_and_mod_dquots(
>> struct xfs_dqtrx *qtrx, *qa;
>> bool locked;
>>
>> - if (!tp->t_dqinfo || !(tp->t_flags & XFS_TRANS_DQ_DIRTY))
>> + if (!tp->t_dqinfo)
>> return;
>>
>> for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
>> @@ -698,7 +686,6 @@ xfs_trans_dqresv(
>> * because we don't have the luxury of a transaction envelope then.
>> */
>> if (tp) {
>> - ASSERT(tp->t_dqinfo);
>> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>> if (nblks != 0)
>> xfs_trans_mod_dquot(tp, dqp,
>> @@ -752,9 +739,6 @@ xfs_trans_reserve_quota_bydquots(
>> if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
>> return 0;
>>
>> - if (tp && tp->t_dqinfo == NULL)
>> - xfs_trans_alloc_dqinfo(tp);
>
> Um, who allocates the dqinfo in this case?
The function call chain here is xfs_trans_reserve_quota_bydquots()->
xfs_trans_dqresv()->xfs_trans_mod_dquot()(tp is non-null), thus the
dqinfo will be allocated in xfs_trans_mod_dquot() function. Actually,
now we do the allocation only in xfs_trans_mod_dquot() function when
the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values changed.
Thanks,
Kaixu
>
> --D
>
>> -
>> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>>
>> if (udqp) {
>> --
>> 2.20.0
>>
--
kaixuxia
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] xfs: directly return if the delta equal to zero
2020-09-26 13:14 [PATCH v2 0/4] xfs: random fixes for disk quota xiakaixu1987
` (2 preceding siblings ...)
2020-09-26 13:14 ` [PATCH v2 3/4] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
@ 2020-09-26 13:14 ` xiakaixu1987
3 siblings, 0 replies; 11+ messages in thread
From: xiakaixu1987 @ 2020-09-26 13:14 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
The xfs_trans_mod_dquot() function will allocate new tp->t_dqinfo if it is
NULL and make the changes in the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}].
Nowadays seems none of the callers want to join the dquots to the
transaction and push them to device when the delta is zero. Actually,
most of time the caller would check the delta and go on only when the
delta value is not zero, so we should bail out when it is zero.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_trans_dquot.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 0ebfd7930382..3e37501791bf 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -194,6 +194,9 @@ xfs_trans_mod_dquot(
ASSERT(XFS_IS_QUOTA_RUNNING(tp->t_mountp));
qtrx = NULL;
+ if (!delta)
+ return;
+
if (tp->t_dqinfo == NULL)
xfs_trans_alloc_dqinfo(tp);
/*
@@ -205,10 +208,8 @@ xfs_trans_mod_dquot(
if (qtrx->qt_dquot == NULL)
qtrx->qt_dquot = dqp;
- if (delta) {
- trace_xfs_trans_mod_dquot_before(qtrx);
- trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
- }
+ trace_xfs_trans_mod_dquot_before(qtrx);
+ trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
switch (field) {
/* regular disk blk reservation */
@@ -261,8 +262,7 @@ xfs_trans_mod_dquot(
ASSERT(0);
}
- if (delta)
- trace_xfs_trans_mod_dquot_after(qtrx);
+ trace_xfs_trans_mod_dquot_after(qtrx);
}
--
2.20.0
^ permalink raw reply related [flat|nested] 11+ messages in thread