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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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
> > 
>