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

[kaixuxia <xiakaixu1987@xxxxxxxxx>]

On 2020/9/23 1:43, Brian Foster wrote:
> On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@xxxxxxxxx wrote:
>> From: Kaixu Xia <kaixuxia@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> ---
>>  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