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