All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] xfs: random fixes for disk quota
@ 2020-10-08  4:19 xiakaixu1987
  2020-10-08  4:19 ` [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: xiakaixu1987 @ 2020-10-08  4:19 UTC (permalink / raw)
  To: linux-xfs; +Cc: darrick.wong, Kaixu Xia

From: Kaixu Xia <kaixuxia@tencent.com>

Hi all,

This patchset include random fixes and code cleanups for disk quota.
In order to make it easier to track, I bundle them up and put all
the scattered patches into a single patchset.

Changes for v4:
 -delete duplicated tp->t_dqinfo null check and allocation in one
  patch.
 -drop the first two patches that have been applied.

Changes for v3:
 -add a separate patch to delete duplicated tp->t_dqinfo null check
  and allocation.

Changes for v2: 
 -add the ASSERT for the arguments O_{u,g,p}dqpp.
 -fix the strange indent.
 -remove the XFS_TRANS_DQ_DIRTY flag.
 -add more commit log description for delta judgement.

Kaixu Xia (3):
  xfs: delete duplicated tp->t_dqinfo null check and allocation
  xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
  xfs: directly return if the delta equal to zero

 fs/xfs/libxfs/xfs_shared.h |  1 -
 fs/xfs/xfs_inode.c         |  8 +-------
 fs/xfs/xfs_trans_dquot.c   | 32 ++++++++------------------------
 3 files changed, 9 insertions(+), 32 deletions(-)

-- 
2.20.0


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation
  2020-10-08  4:19 [PATCH v4 0/3] xfs: random fixes for disk quota xiakaixu1987
@ 2020-10-08  4:19 ` xiakaixu1987
  2020-10-09 11:32   ` Brian Foster
  2020-10-26 22:51   ` Darrick J. Wong
  2020-10-08  4:19 ` [PATCH v4 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
  2020-10-08  4:19 ` [PATCH v4 3/3] xfs: directly return if the delta equal to zero xiakaixu1987
  2 siblings, 2 replies; 10+ messages in thread
From: xiakaixu1987 @ 2020-10-08  4:19 UTC (permalink / raw)
  To: linux-xfs; +Cc: darrick.wong, Kaixu Xia

From: Kaixu Xia <kaixuxia@tencent.com>

The function xfs_trans_mod_dquot_byino() wrap around xfs_trans_mod_dquot()
to account for quotas, and also there is the function call chain
xfs_trans_reserve_quota_bydquots -> xfs_trans_dqresv -> xfs_trans_mod_dquot,
both of them do the duplicated null check and allocation. Thus
we can delete the duplicated operation from them.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
 fs/xfs/xfs_trans_dquot.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index fe45b0c3970c..67f1e275b34d 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -143,9 +143,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)
@@ -698,7 +695,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 +748,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] 10+ messages in thread
* [PATCH v4 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
  2020-10-08  4:19 [PATCH v4 0/3] xfs: random fixes for disk quota xiakaixu1987
  2020-10-08  4:19 ` [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
@ 2020-10-08  4:19 ` xiakaixu1987
  2020-10-09 11:32   ` Brian Foster
  2020-10-26 22:51   ` Darrick J. Wong
  2020-10-08  4:19 ` [PATCH v4 3/3] xfs: directly return if the delta equal to zero xiakaixu1987
  2 siblings, 2 replies; 10+ messages in thread
From: xiakaixu1987 @ 2020-10-08  4:19 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   | 13 ++-----------
 3 files changed, 3 insertions(+), 19 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 2bfbcf28b1bd..4d2cebaa3637 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -959,7 +959,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);
@@ -1018,12 +1017,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);
@@ -1031,10 +1027,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 67f1e275b34d..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];
@@ -270,8 +263,6 @@ xfs_trans_mod_dquot(
 
 	if (delta)
 		trace_xfs_trans_mod_dquot_after(qtrx);
-
-	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
 }
 
 
@@ -348,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);
@@ -490,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++) {
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v4 3/3] xfs: directly return if the delta equal to zero
  2020-10-08  4:19 [PATCH v4 0/3] xfs: random fixes for disk quota xiakaixu1987
  2020-10-08  4:19 ` [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
  2020-10-08  4:19 ` [PATCH v4 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
@ 2020-10-08  4:19 ` xiakaixu1987
  2020-10-09 11:32   ` Brian Foster
  2 siblings, 1 reply; 10+ messages in thread
From: xiakaixu1987 @ 2020-10-08  4:19 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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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] 10+ messages in thread
* Re: [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation
  2020-10-08  4:19 ` [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
@ 2020-10-09 11:32   ` Brian Foster
  2020-10-26 22:51   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Brian Foster @ 2020-10-09 11:32 UTC (permalink / raw)
  To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia

On Thu, Oct 08, 2020 at 12:19:07PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> The function xfs_trans_mod_dquot_byino() wrap around xfs_trans_mod_dquot()
> to account for quotas, and also there is the function call chain
> xfs_trans_reserve_quota_bydquots -> xfs_trans_dqresv -> xfs_trans_mod_dquot,
> both of them do the duplicated null check and allocation. Thus
> we can delete the duplicated operation from them.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---

Seems reasonable:

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

>  fs/xfs/xfs_trans_dquot.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index fe45b0c3970c..67f1e275b34d 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -143,9 +143,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)
> @@ -698,7 +695,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 +748,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	[flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
  2020-10-08  4:19 ` [PATCH v4 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
@ 2020-10-09 11:32   ` Brian Foster
  2020-10-26 22:51   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Brian Foster @ 2020-10-09 11:32 UTC (permalink / raw)
  To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia

On Thu, Oct 08, 2020 at 12:19:08PM +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>
> ---

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

>  fs/xfs/libxfs/xfs_shared.h |  1 -
>  fs/xfs/xfs_inode.c         |  8 +-------
>  fs/xfs/xfs_trans_dquot.c   | 13 ++-----------
>  3 files changed, 3 insertions(+), 19 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 2bfbcf28b1bd..4d2cebaa3637 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -959,7 +959,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);
> @@ -1018,12 +1017,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);
> @@ -1031,10 +1027,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 67f1e275b34d..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];
> @@ -270,8 +263,6 @@ xfs_trans_mod_dquot(
>  
>  	if (delta)
>  		trace_xfs_trans_mod_dquot_after(qtrx);
> -
> -	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
>  }
>  
>  
> @@ -348,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);
> @@ -490,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++) {
> -- 
> 2.20.0
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v4 3/3] xfs: directly return if the delta equal to zero
  2020-10-08  4:19 ` [PATCH v4 3/3] xfs: directly return if the delta equal to zero xiakaixu1987
@ 2020-10-09 11:32   ` Brian Foster
  2020-10-10  1:50     ` kaixuxia
  0 siblings, 1 reply; 10+ messages in thread
From: Brian Foster @ 2020-10-09 11:32 UTC (permalink / raw)
  To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia

On Thu, Oct 08, 2020 at 12:19:09PM +0800, xiakaixu1987@gmail.com wrote:
> 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>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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;
> +

Note that the calls in xfs_trans_dqresv() also check for delta != 0, so
that could be removed with this patch. That aside:

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

>  	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	[flat|nested] 10+ messages in thread
* Re: [PATCH v4 3/3] xfs: directly return if the delta equal to zero
  2020-10-09 11:32   ` Brian Foster
@ 2020-10-10  1:50     ` kaixuxia
  0 siblings, 0 replies; 10+ messages in thread
From: kaixuxia @ 2020-10-10  1:50 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs, darrick.wong, Kaixu Xia



On 2020/10/9 19:32, Brian Foster wrote:
> On Thu, Oct 08, 2020 at 12:19:09PM +0800, xiakaixu1987@gmail.com wrote:
>> 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>
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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;
>> +
> 
> Note that the calls in xfs_trans_dqresv() also check for delta != 0, so
> that could be removed with this patch. That aside:

Yeah, I'll do that in the next version.

Thanks,
Kaixu
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
>>  	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
>>
> 

-- 
kaixuxia

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation
  2020-10-08  4:19 ` [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
  2020-10-09 11:32   ` Brian Foster
@ 2020-10-26 22:51   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2020-10-26 22:51 UTC (permalink / raw)
  To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia

On Thu, Oct 08, 2020 at 12:19:07PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> The function xfs_trans_mod_dquot_byino() wrap around xfs_trans_mod_dquot()
> to account for quotas, and also there is the function call chain
> xfs_trans_reserve_quota_bydquots -> xfs_trans_dqresv -> xfs_trans_mod_dquot,
> both of them do the duplicated null check and allocation. Thus
> we can delete the duplicated operation from them.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

Looks good to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_trans_dquot.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index fe45b0c3970c..67f1e275b34d 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -143,9 +143,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)
> @@ -698,7 +695,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 +748,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	[flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
  2020-10-08  4:19 ` [PATCH v4 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
  2020-10-09 11:32   ` Brian Foster
@ 2020-10-26 22:51   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2020-10-26 22:51 UTC (permalink / raw)
  To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia

On Thu, Oct 08, 2020 at 12:19:08PM +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>

Woooooo!!
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_shared.h |  1 -
>  fs/xfs/xfs_inode.c         |  8 +-------
>  fs/xfs/xfs_trans_dquot.c   | 13 ++-----------
>  3 files changed, 3 insertions(+), 19 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 2bfbcf28b1bd..4d2cebaa3637 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -959,7 +959,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);
> @@ -1018,12 +1017,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);
> @@ -1031,10 +1027,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 67f1e275b34d..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];
> @@ -270,8 +263,6 @@ xfs_trans_mod_dquot(
>  
>  	if (delta)
>  		trace_xfs_trans_mod_dquot_after(qtrx);
> -
> -	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
>  }
>  
>  
> @@ -348,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);
> @@ -490,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++) {
> -- 
> 2.20.0
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-26 22:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-08  4:19 [PATCH v4 0/3] xfs: random fixes for disk quota xiakaixu1987
2020-10-08  4:19 ` [PATCH v4 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
2020-10-09 11:32   ` Brian Foster
2020-10-26 22:51   ` Darrick J. Wong
2020-10-08  4:19 ` [PATCH v4 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
2020-10-09 11:32   ` Brian Foster
2020-10-26 22:51   ` Darrick J. Wong
2020-10-08  4:19 ` [PATCH v4 3/3] xfs: directly return if the delta equal to zero xiakaixu1987
2020-10-09 11:32   ` Brian Foster
2020-10-10  1:50     ` kaixuxia

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.