[RFC PATCH 0/3]xfs: random fixes for disk quota

[RFC PATCH 0/3]xfs: random fixes for disk quota

[xiakaixu1987]

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.

Kaixu Xia (3):
  xfs: directly return if the delta equal to zero
  xfs: remove the unused parameter id from xfs_qm_dqattach_one
  xfs: only do dqget or dqhold for the specified dquots

 fs/xfs/xfs_qm.c          | 24 +++++++++---------------
 fs/xfs/xfs_trans_dquot.c | 12 ++++++------
 2 files changed, 15 insertions(+), 21 deletions(-)

-- 
2.20.0

[PATCH 1/3] xfs: directly return if the delta equal to zero

[xiakaixu1987]

From: Kaixu Xia <kaixuxia@tencent.com>

It is useless to go on when the variable delta equal to zero in
xfs_trans_mod_dquot(), so just return if the value equal to 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 133fc6fc3edd..23c34af71825 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -215,10 +215,11 @@ 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);
-	}
+	if (!delta)
+		return;
+
+	trace_xfs_trans_mod_dquot_before(qtrx);
+	trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
 
 	switch (field) {
 
@@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
 		ASSERT(0);
 	}
 
-	if (delta)
-		trace_xfs_trans_mod_dquot_after(qtrx);
+	trace_xfs_trans_mod_dquot_after(qtrx);
 
 	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
 }
-- 
2.20.0

[PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one

[xiakaixu1987]

From: Kaixu Xia <kaixuxia@tencent.com>

Since we never use the second parameter id, so remove it from
xfs_qm_dqattach_one() function.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
 fs/xfs/xfs_qm.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 41a459ffd1f2..44509decb4cd 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -249,7 +249,6 @@ xfs_qm_unmount_quotas(
 STATIC int
 xfs_qm_dqattach_one(
 	struct xfs_inode	*ip,
-	xfs_dqid_t		id,
 	xfs_dqtype_t		type,
 	bool			doalloc,
 	struct xfs_dquot	**IO_idqpp)
@@ -330,23 +329,23 @@ xfs_qm_dqattach_locked(
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
-		error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
-				XFS_DQTYPE_USER, doalloc, &ip->i_udquot);
+		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_USER,
+				doalloc, &ip->i_udquot);
 		if (error)
 			goto done;
 		ASSERT(ip->i_udquot);
 	}
 
 	if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
-		error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
-				XFS_DQTYPE_GROUP, doalloc, &ip->i_gdquot);
+		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_GROUP,
+				doalloc, &ip->i_gdquot);
 		if (error)
 			goto done;
 		ASSERT(ip->i_gdquot);
 	}
 
 	if (XFS_IS_PQUOTA_ON(mp) && !ip->i_pdquot) {
-		error = xfs_qm_dqattach_one(ip, ip->i_d.di_projid, XFS_DQTYPE_PROJ,
+		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_PROJ,
 				doalloc, &ip->i_pdquot);
 		if (error)
 			goto done;
-- 
2.20.0

[PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots

[xiakaixu1987]

From: Kaixu Xia <kaixuxia@tencent.com>

We attach the dquot(s) to the given inode and return udquot, gdquot
or pdquot with references taken by dqget or dqhold in xfs_qm_vop_dqalloc()
funciton. Actually, we only need to do dqget or dqhold for the specified
dquots, it is unnecessary if the passed-in O_{u,g,p}dqpp value is NULL.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
 fs/xfs/xfs_qm.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 44509decb4cd..38380fc29b4d 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1661,7 +1661,7 @@ xfs_qm_vop_dqalloc(
 		}
 	}
 
-	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
+	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp) && O_udqpp) {
 		if (!uid_eq(inode->i_uid, uid)) {
 			/*
 			 * What we need is the dquot that has this uid, and
@@ -1694,7 +1694,7 @@ xfs_qm_vop_dqalloc(
 			uq = xfs_qm_dqhold(ip->i_udquot);
 		}
 	}
-	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
+	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp) && O_gdqpp) {
 		if (!gid_eq(inode->i_gid, gid)) {
 			xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
@@ -1711,7 +1711,7 @@ xfs_qm_vop_dqalloc(
 			gq = xfs_qm_dqhold(ip->i_gdquot);
 		}
 	}
-	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
+	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp) && O_pdqpp) {
 		if (ip->i_d.di_projid != prid) {
 			xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, prid,
@@ -1733,16 +1733,11 @@ xfs_qm_vop_dqalloc(
 	xfs_iunlock(ip, lockflags);
 	if (O_udqpp)
 		*O_udqpp = uq;
-	else
-		xfs_qm_dqrele(uq);
 	if (O_gdqpp)
 		*O_gdqpp = gq;
-	else
-		xfs_qm_dqrele(gq);
 	if (O_pdqpp)
 		*O_pdqpp = pq;
-	else
-		xfs_qm_dqrele(pq);
+
 	return 0;
 
 error_rele:
-- 
2.20.0

Re: [PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots

[Darrick J. Wong]

On Tue, Sep 22, 2020 at 05:04:02PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> We attach the dquot(s) to the given inode and return udquot, gdquot
> or pdquot with references taken by dqget or dqhold in xfs_qm_vop_dqalloc()
> funciton. Actually, we only need to do dqget or dqhold for the specified
> dquots, it is unnecessary if the passed-in O_{u,g,p}dqpp value is NULL.

When would a caller pass in (for example) XFS_QMOPT_UQUOTA, a different
uid than the one currently associated with the inode, but a null
O_udqpp?  It doesn't make sense to say "Prepare to change this file's
uid, but don't give me the dquot of the new uid."

None of the callers do that today, so I don't see the point of this
patch.  Perhaps the function could ASSERT the arguments a little more
closely?

--D

> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
>  fs/xfs/xfs_qm.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 44509decb4cd..38380fc29b4d 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1661,7 +1661,7 @@ xfs_qm_vop_dqalloc(
>  		}
>  	}
>  
> -	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
> +	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp) && O_udqpp) {
>  		if (!uid_eq(inode->i_uid, uid)) {
>  			/*
>  			 * What we need is the dquot that has this uid, and
> @@ -1694,7 +1694,7 @@ xfs_qm_vop_dqalloc(
>  			uq = xfs_qm_dqhold(ip->i_udquot);
>  		}
>  	}
> -	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
> +	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp) && O_gdqpp) {
>  		if (!gid_eq(inode->i_gid, gid)) {
>  			xfs_iunlock(ip, lockflags);
>  			error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
> @@ -1711,7 +1711,7 @@ xfs_qm_vop_dqalloc(
>  			gq = xfs_qm_dqhold(ip->i_gdquot);
>  		}
>  	}
> -	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
> +	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp) && O_pdqpp) {
>  		if (ip->i_d.di_projid != prid) {
>  			xfs_iunlock(ip, lockflags);
>  			error = xfs_qm_dqget(mp, prid,
> @@ -1733,16 +1733,11 @@ xfs_qm_vop_dqalloc(
>  	xfs_iunlock(ip, lockflags);
>  	if (O_udqpp)
>  		*O_udqpp = uq;
> -	else
> -		xfs_qm_dqrele(uq);
>  	if (O_gdqpp)
>  		*O_gdqpp = gq;
> -	else
> -		xfs_qm_dqrele(gq);
>  	if (O_pdqpp)
>  		*O_pdqpp = pq;
> -	else
> -		xfs_qm_dqrele(pq);
> +
>  	return 0;
>  
>  error_rele:
> -- 
> 2.20.0
> 

Re: [PATCH 1/3] xfs: directly return if the delta equal to zero

[Darrick J. Wong]

On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> It is useless to go on when the variable delta equal to zero in
> xfs_trans_mod_dquot(), so just return if the value equal to zero.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

Yeah, that makes sense.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  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 133fc6fc3edd..23c34af71825 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -215,10 +215,11 @@ 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);
> -	}
> +	if (!delta)
> +		return;
> +
> +	trace_xfs_trans_mod_dquot_before(qtrx);
> +	trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
>  
>  	switch (field) {
>  
> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
>  		ASSERT(0);
>  	}
>  
> -	if (delta)
> -		trace_xfs_trans_mod_dquot_after(qtrx);
> +	trace_xfs_trans_mod_dquot_after(qtrx);
>  
>  	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
>  }
> -- 
> 2.20.0
> 

Re: [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one

[Darrick J. Wong]

On Tue, Sep 22, 2020 at 05:04:01PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> Since we never use the second parameter id, so remove it from
> xfs_qm_dqattach_one() function.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

Heh, yep.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_qm.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 41a459ffd1f2..44509decb4cd 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -249,7 +249,6 @@ xfs_qm_unmount_quotas(
>  STATIC int
>  xfs_qm_dqattach_one(
>  	struct xfs_inode	*ip,
> -	xfs_dqid_t		id,
>  	xfs_dqtype_t		type,
>  	bool			doalloc,
>  	struct xfs_dquot	**IO_idqpp)
> @@ -330,23 +329,23 @@ xfs_qm_dqattach_locked(
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
>  	if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> -		error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
> -				XFS_DQTYPE_USER, doalloc, &ip->i_udquot);
> +		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_USER,
> +				doalloc, &ip->i_udquot);
>  		if (error)
>  			goto done;
>  		ASSERT(ip->i_udquot);
>  	}
>  
>  	if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> -		error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
> -				XFS_DQTYPE_GROUP, doalloc, &ip->i_gdquot);
> +		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_GROUP,
> +				doalloc, &ip->i_gdquot);
>  		if (error)
>  			goto done;
>  		ASSERT(ip->i_gdquot);
>  	}
>  
>  	if (XFS_IS_PQUOTA_ON(mp) && !ip->i_pdquot) {
> -		error = xfs_qm_dqattach_one(ip, ip->i_d.di_projid, XFS_DQTYPE_PROJ,
> +		error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_PROJ,
>  				doalloc, &ip->i_pdquot);
>  		if (error)
>  			goto done;
> -- 
> 2.20.0
> 

Re: [PATCH 1/3] xfs: directly return if the delta equal to zero

[Brian Foster]

On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> It is useless to go on when the variable delta equal to zero in
> xfs_trans_mod_dquot(), so just return if the value equal to 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 133fc6fc3edd..23c34af71825 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -215,10 +215,11 @@ 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);
> -	}
> +	if (!delta)
> +		return;
> +


This does slightly change behavior in that this function currently
unconditionally results in logging the associated dquot in the
transaction. I'm not sure anything really depends on that with a delta
== 0, but it might be worth documenting in the commit log.

Also, it does seem a little odd to bail out after we've potentially
allocated ->t_dqinfo as well as assigned the current dquot a slot in the
transaction. I think that means the effect of this change is lost if
another dquot happens to be modified (with delta != 0) in the same
transaction (which might also be an odd thing to do).

Brian

> +	trace_xfs_trans_mod_dquot_before(qtrx);
> +	trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
>  
>  	switch (field) {
>  
> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
>  		ASSERT(0);
>  	}
>  
> -	if (delta)
> -		trace_xfs_trans_mod_dquot_after(qtrx);
> +	trace_xfs_trans_mod_dquot_after(qtrx);
>  
>  	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
>  }
> -- 
> 2.20.0
> 

Re: [PATCH 1/3] xfs: directly return if the delta equal to zero

[kaixuxia]

On 2020/9/23 1:43, Brian Foster wrote:
> On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> It is useless to go on when the variable delta equal to zero in
>> xfs_trans_mod_dquot(), so just return if the value equal to 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 133fc6fc3edd..23c34af71825 100644
>> --- a/fs/xfs/xfs_trans_dquot.c
>> +++ b/fs/xfs/xfs_trans_dquot.c
>> @@ -215,10 +215,11 @@ 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);
>> -	}
>> +	if (!delta)
>> +		return;
>> +
> 
> 
> This does slightly change behavior in that this function currently
> unconditionally results in logging the associated dquot in the
> transaction. I'm not sure anything really depends on that with a delta
> == 0, but it might be worth documenting in the commit log.
>> Also, it does seem a little odd to bail out after we've potentially
> allocated ->t_dqinfo as well as assigned the current dquot a slot in the
> transaction. I think that means the effect of this change is lost if
> another dquot happens to be modified (with delta != 0) in the same
> transaction (which might also be an odd thing to do).
>
Since the dquot value doesn't changes if the delta == 0, we shouldn't
set the XFS_TRANS_DQ_DIRTY flag to current transaction. Maybe we should
do the judgement at the beginning of the function, we will do nothing if
the delta == 0. Just like this,

 xfs_trans_mod_dquot(
 {
   ...
   if (!delta)
     return;
   if (tp->t_dqinfo == NULL)
     xfs_trans_alloc_dqinfo(tp);
   ...
 }

I'm not sure...What's your opinion about that?

Thanks,
Kaixu

> Brian
> 
>> +	trace_xfs_trans_mod_dquot_before(qtrx);
>> +	trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
>>  
>>  	switch (field) {
>>  
>> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
>>  		ASSERT(0);
>>  	}
>>  
>> -	if (delta)
>> -		trace_xfs_trans_mod_dquot_after(qtrx);
>> +	trace_xfs_trans_mod_dquot_after(qtrx);
>>  
>>  	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
>>  }
>> -- 
>> 2.20.0
>>
> 

-- 
kaixuxia

Re: [PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots

[kaixuxia]

On 2020/9/23 0:18, Darrick J. Wong wrote:
> On Tue, Sep 22, 2020 at 05:04:02PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> We attach the dquot(s) to the given inode and return udquot, gdquot
>> or pdquot with references taken by dqget or dqhold in xfs_qm_vop_dqalloc()
>> funciton. Actually, we only need to do dqget or dqhold for the specified
>> dquots, it is unnecessary if the passed-in O_{u,g,p}dqpp value is NULL.
> 
> When would a caller pass in (for example) XFS_QMOPT_UQUOTA, a different
> uid than the one currently associated with the inode, but a null
> O_udqpp?  It doesn't make sense to say "Prepare to change this file's
> uid, but don't give me the dquot of the new uid."
> 
> None of the callers do that today, so I don't see the point of this
> patch.  Perhaps the function could ASSERT the arguments a little more
> closely?

Yeah, ASSERT the arguments is better, will do that in the next version.

Thanks,
Kaixu
 
> 
> --D
> 
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>>  fs/xfs/xfs_qm.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
>> index 44509decb4cd..38380fc29b4d 100644
>> --- a/fs/xfs/xfs_qm.c
>> +++ b/fs/xfs/xfs_qm.c
>> @@ -1661,7 +1661,7 @@ xfs_qm_vop_dqalloc(
>>  		}
>>  	}
>>  
>> -	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
>> +	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp) && O_udqpp) {
>>  		if (!uid_eq(inode->i_uid, uid)) {
>>  			/*
>>  			 * What we need is the dquot that has this uid, and
>> @@ -1694,7 +1694,7 @@ xfs_qm_vop_dqalloc(
>>  			uq = xfs_qm_dqhold(ip->i_udquot);
>>  		}
>>  	}
>> -	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
>> +	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp) && O_gdqpp) {
>>  		if (!gid_eq(inode->i_gid, gid)) {
>>  			xfs_iunlock(ip, lockflags);
>>  			error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
>> @@ -1711,7 +1711,7 @@ xfs_qm_vop_dqalloc(
>>  			gq = xfs_qm_dqhold(ip->i_gdquot);
>>  		}
>>  	}
>> -	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
>> +	if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp) && O_pdqpp) {
>>  		if (ip->i_d.di_projid != prid) {
>>  			xfs_iunlock(ip, lockflags);
>>  			error = xfs_qm_dqget(mp, prid,
>> @@ -1733,16 +1733,11 @@ xfs_qm_vop_dqalloc(
>>  	xfs_iunlock(ip, lockflags);
>>  	if (O_udqpp)
>>  		*O_udqpp = uq;
>> -	else
>> -		xfs_qm_dqrele(uq);
>>  	if (O_gdqpp)
>>  		*O_gdqpp = gq;
>> -	else
>> -		xfs_qm_dqrele(gq);
>>  	if (O_pdqpp)
>>  		*O_pdqpp = pq;
>> -	else
>> -		xfs_qm_dqrele(pq);
>> +
>>  	return 0;
>>  
>>  error_rele:
>> -- 
>> 2.20.0
>>

-- 
kaixuxia

Re: [PATCH 1/3] xfs: directly return if the delta equal to zero

[Christoph Hellwig]

On Tue, Sep 22, 2020 at 01:43:47PM -0400, Brian Foster wrote:
> This does slightly change behavior in that this function currently
> unconditionally results in logging the associated dquot in the
> transaction. I'm not sure anything really depends on that with a delta
> == 0, but it might be worth documenting in the commit log.
> 
> Also, it does seem a little odd to bail out after we've potentially
> allocated ->t_dqinfo as well as assigned the current dquot a slot in the
> transaction. I think that means the effect of this change is lost if
> another dquot happens to be modified (with delta != 0) in the same
> transaction (which might also be an odd thing to do).

Yes, it seems like we should probably bail out at the very beginning for
delta == 0, and document what kind of changes this theoretically causes,
and why they don't matter.

Btw, the function could really use a reindent, the formatting is very
strange.

Re: [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one

[Christoph Hellwig]

On Tue, Sep 22, 2020 at 05:04:01PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> Since we never use the second parameter id, so remove it from
> xfs_qm_dqattach_one() function.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

Looks good:

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

Re: [PATCH 1/3] xfs: directly return if the delta equal to zero

[Brian Foster]

On Wed, Sep 23, 2020 at 10:42:10AM +0800, kaixuxia wrote:
> 
> 
> On 2020/9/23 1:43, Brian Foster wrote:
> > On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
> >> From: Kaixu Xia <kaixuxia@tencent.com>
> >>
> >> It is useless to go on when the variable delta equal to zero in
> >> xfs_trans_mod_dquot(), so just return if the value equal to 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 133fc6fc3edd..23c34af71825 100644
> >> --- a/fs/xfs/xfs_trans_dquot.c
> >> +++ b/fs/xfs/xfs_trans_dquot.c
> >> @@ -215,10 +215,11 @@ 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);
> >> -	}
> >> +	if (!delta)
> >> +		return;
> >> +
> > 
> > 
> > This does slightly change behavior in that this function currently
> > unconditionally results in logging the associated dquot in the
> > transaction. I'm not sure anything really depends on that with a delta
> > == 0, but it might be worth documenting in the commit log.
> >> Also, it does seem a little odd to bail out after we've potentially
> > allocated ->t_dqinfo as well as assigned the current dquot a slot in the
> > transaction. I think that means the effect of this change is lost if
> > another dquot happens to be modified (with delta != 0) in the same
> > transaction (which might also be an odd thing to do).
> >
> Since the dquot value doesn't changes if the delta == 0, we shouldn't
> set the XFS_TRANS_DQ_DIRTY flag to current transaction. Maybe we should
> do the judgement at the beginning of the function, we will do nothing if
> the delta == 0. Just like this,
> 
>  xfs_trans_mod_dquot(
>  {
>    ...
>    if (!delta)
>      return;
>    if (tp->t_dqinfo == NULL)
>      xfs_trans_alloc_dqinfo(tp);
>    ...
>  }
> 
> I'm not sure...What's your opinion about that?
> 

Yes, I think that makes more sense than bailing out after at least.
Otherwise if some other path sets XFS_TRANS_DQ_DIRTY, then this dquot is
still associated with the transaction. I'm not sure that's currently
possible, but it's an odd wart where the current code is at least
readable/predictable. That said, note again that this changes behavior,
so it's not quite sufficient for the commit log description to just say
bail out early since delta is zero. That much is obvious from the code
change. We need to audit the behavior change and provide a few sentences
in the commit log description to explain why it is safe.

Brian

> Thanks,
> Kaixu
> 
> > Brian
> > 
> >> +	trace_xfs_trans_mod_dquot_before(qtrx);
> >> +	trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
> >>  
> >>  	switch (field) {
> >>  
> >> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
> >>  		ASSERT(0);
> >>  	}
> >>  
> >> -	if (delta)
> >> -		trace_xfs_trans_mod_dquot_after(qtrx);
> >> +	trace_xfs_trans_mod_dquot_after(qtrx);
> >>  
> >>  	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
> >>  }
> >> -- 
> >> 2.20.0
> >>
> > 
> 
> -- 
> kaixuxia
> 

Re: [PATCH 1/3] xfs: directly return if the delta equal to zero

[Darrick J. Wong]

On Wed, Sep 23, 2020 at 09:27:45AM -0400, Brian Foster wrote:
> On Wed, Sep 23, 2020 at 10:42:10AM +0800, kaixuxia wrote:
> > 
> > 
> > On 2020/9/23 1:43, Brian Foster wrote:
> > > On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
> > >> From: Kaixu Xia <kaixuxia@tencent.com>
> > >>
> > >> It is useless to go on when the variable delta equal to zero in
> > >> xfs_trans_mod_dquot(), so just return if the value equal to 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 133fc6fc3edd..23c34af71825 100644
> > >> --- a/fs/xfs/xfs_trans_dquot.c
> > >> +++ b/fs/xfs/xfs_trans_dquot.c
> > >> @@ -215,10 +215,11 @@ 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);
> > >> -	}
> > >> +	if (!delta)
> > >> +		return;
> > >> +
> > > 
> > > 
> > > This does slightly change behavior in that this function currently
> > > unconditionally results in logging the associated dquot in the
> > > transaction. I'm not sure anything really depends on that with a delta
> > > == 0, but it might be worth documenting in the commit log.
> > >> Also, it does seem a little odd to bail out after we've potentially
> > > allocated ->t_dqinfo as well as assigned the current dquot a slot in the
> > > transaction. I think that means the effect of this change is lost if
> > > another dquot happens to be modified (with delta != 0) in the same
> > > transaction (which might also be an odd thing to do).
> > >
> > Since the dquot value doesn't changes if the delta == 0, we shouldn't
> > set the XFS_TRANS_DQ_DIRTY flag to current transaction. Maybe we should
> > do the judgement at the beginning of the function, we will do nothing if
> > the delta == 0. Just like this,
> > 
> >  xfs_trans_mod_dquot(
> >  {
> >    ...
> >    if (!delta)
> >      return;
> >    if (tp->t_dqinfo == NULL)
> >      xfs_trans_alloc_dqinfo(tp);
> >    ...
> >  }
> > 
> > I'm not sure...What's your opinion about that?
> > 
> 
> Yes, I think that makes more sense than bailing out after at least.
> Otherwise if some other path sets XFS_TRANS_DQ_DIRTY, then this dquot is
> still associated with the transaction. I'm not sure that's currently
> possible, but it's an odd wart where the current code is at least
> readable/predictable. That said, note again that this changes behavior,
> so it's not quite sufficient for the commit log description to just say
> bail out early since delta is zero. That much is obvious from the code
> change. We need to audit the behavior change and provide a few sentences
> in the commit log description to explain why it is safe.

Agreed.  Sorry I didn't notice the TRANS_DQ_DIRTY thing earlier. :/

TBH I wonder if we even need that flag, since the only thing it seems to
do nowadays is shortcut checking if tp->t_dqinfo == NULL in
xfs_trans_apply_dquot_deltas and its unreserve variant.

--D

> 
> Brian
> 
> > Thanks,
> > Kaixu
> > 
> > > Brian
> > > 
> > >> +	trace_xfs_trans_mod_dquot_before(qtrx);
> > >> +	trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
> > >>  
> > >>  	switch (field) {
> > >>  
> > >> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
> > >>  		ASSERT(0);
> > >>  	}
> > >>  
> > >> -	if (delta)
> > >> -		trace_xfs_trans_mod_dquot_after(qtrx);
> > >> +	trace_xfs_trans_mod_dquot_after(qtrx);
> > >>  
> > >>  	tp->t_flags |= XFS_TRANS_DQ_DIRTY;
> > >>  }
> > >> -- 
> > >> 2.20.0
> > >>
> > > 
> > 
> > -- 
> > kaixuxia
> > 
> 

Re: [RFC PATCH 0/3]xfs: random fixes for disk quota

[Darrick J. Wong]

On Tue, Sep 22, 2020 at 05:03:59PM +0800, xiakaixu1987@gmail.com wrote:
> 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.

FWIW I've applied the second patch, so you can drop it from the next
resend.

--D

> Kaixu Xia (3):
>   xfs: directly return if the delta equal to zero
>   xfs: remove the unused parameter id from xfs_qm_dqattach_one
>   xfs: only do dqget or dqhold for the specified dquots
> 
>  fs/xfs/xfs_qm.c          | 24 +++++++++---------------
>  fs/xfs/xfs_trans_dquot.c | 12 ++++++------
>  2 files changed, 15 insertions(+), 21 deletions(-)
> 
> -- 
> 2.20.0
>