| Message ID | 1600765442-12146-2-git-send-email-kaixuxia@tencent.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: random fixes for disk quota | expand |
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 >
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 >
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 >> >
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.
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 >
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 > > >
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; }