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