| Message ID | 1473149039-30487-1-git-send-email-guaneryu@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, Sep 06, 2016 at 04:03:59PM +0800, Eryu Guan wrote: > "blocks" should be added back to fdblocks at undo time, not taken > away, i.e. the minus sign should not be used. You've described the code change you made, not about the problem you hit and are fixing. i.e. I've got no idea how you found this, or even how to identify a system that is tripping over this problem. By describing how you found it and the symptoms being displayed, I'll learn from you how to identify the problem and hence, in future, be able to identify systems that are tripping over the problem, too. > Fixes: 0d485ada404b ("xfs: use generic percpu counters for free block counter") I really don't like this sort of "annotation". It wrongly implies the commit was broken (it wasn't) and there's no scope for stating the problem context. i.e. that the problem is a minor regression in a rarely travelled corner case that is unlikely to affect production machines in any significant way. It's better to describe things with all the relevant context: "This is a regression introduced in commit ... and only occurs when .... " > Signed-off-by: Eryu Guan <guaneryu@gmail.com> Not @redhat? > --- > fs/xfs/xfs_trans.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > index 5f3d33d..011dace 100644 > --- a/fs/xfs/xfs_trans.c > +++ b/fs/xfs/xfs_trans.c > @@ -217,7 +217,7 @@ undo_log: > > undo_blocks: > if (blocks > 0) { > - xfs_mod_fdblocks(tp->t_mountp, -((int64_t)blocks), rsvd); > + xfs_mod_fdblocks(tp->t_mountp, ((int64_t)blocks), rsvd); Outer () can be dropped, too. Cheers, Dave.
On Tue, Sep 06, 2016 at 06:48:28PM +1000, Dave Chinner wrote: > On Tue, Sep 06, 2016 at 04:03:59PM +0800, Eryu Guan wrote: > > "blocks" should be added back to fdblocks at undo time, not taken > > away, i.e. the minus sign should not be used. > > You've described the code change you made, not about the problem you > hit and are fixing. > > i.e. I've got no idea how you found this, or even how to identify a > system that is tripping over this problem. By describing how you > found it and the symptoms being displayed, I'll learn from you how > to identify the problem and hence, in future, be able to identify > systems that are tripping over the problem, too. Usually I will describe the symptoms, how I hit the problem and the reproducer in commit log in details, but this time I found this bug by code inspection, I don't have these information. I should have mentioned this info too. > > > Fixes: 0d485ada404b ("xfs: use generic percpu counters for free block counter") > > I really don't like this sort of "annotation". It wrongly implies > the commit was broken (it wasn't) and there's no scope for stating > the problem context. i.e. that the problem is a minor regression in > a rarely travelled corner case that is unlikely to affect production > machines in any significant way. It's better to describe things with > all the relevant context: > > "This is a regression introduced in commit ... and only occurs when > .... " Makes sense, will do so. > > > Signed-off-by: Eryu Guan <guaneryu@gmail.com> > > Not @redhat? I thought that I'm employed by Red Hat as a QE not a filesystem developer, all filesystem patches I send reflect my own opinions not my employer's, so all silly mistakes I made in the patches are under my personal email too :) > > > --- > > fs/xfs/xfs_trans.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > > index 5f3d33d..011dace 100644 > > --- a/fs/xfs/xfs_trans.c > > +++ b/fs/xfs/xfs_trans.c > > @@ -217,7 +217,7 @@ undo_log: > > > > undo_blocks: > > if (blocks > 0) { > > - xfs_mod_fdblocks(tp->t_mountp, -((int64_t)blocks), rsvd); > > + xfs_mod_fdblocks(tp->t_mountp, ((int64_t)blocks), rsvd); > > Outer () can be dropped, too. OK. Thanks for the review! Eryu
On Tue, Sep 06, 2016 at 07:50:45PM +0800, Eryu Guan wrote: > On Tue, Sep 06, 2016 at 06:48:28PM +1000, Dave Chinner wrote: > > On Tue, Sep 06, 2016 at 04:03:59PM +0800, Eryu Guan wrote: > > > Fixes: 0d485ada404b ("xfs: use generic percpu counters for free block counter") > > > > I really don't like this sort of "annotation". It wrongly implies > > the commit was broken (it wasn't) and there's no scope for stating > > the problem context. i.e. that the problem is a minor regression in > > a rarely travelled corner case that is unlikely to affect production > > machines in any significant way. It's better to describe things with > > all the relevant context: > > > > "This is a regression introduced in commit ... and only occurs when > > .... " > > Makes sense, will do so. > > > > > > Signed-off-by: Eryu Guan <guaneryu@gmail.com> > > > > Not @redhat? > > I thought that I'm employed by Red Hat as a QE not a filesystem > developer, all filesystem patches I send reflect my own opinions not my > employer's, so all silly mistakes I made in the patches are under my > personal email too :) Copyright assignments rarely work like that. It's a big grey area, though, so I think you should check with your legal department as to what you should use here. Cheers, Dave.
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c index 5f3d33d..011dace 100644 --- a/fs/xfs/xfs_trans.c +++ b/fs/xfs/xfs_trans.c @@ -217,7 +217,7 @@ undo_log: undo_blocks: if (blocks > 0) { - xfs_mod_fdblocks(tp->t_mountp, -((int64_t)blocks), rsvd); + xfs_mod_fdblocks(tp->t_mountp, ((int64_t)blocks), rsvd); tp->t_blk_res = 0; }
"blocks" should be added back to fdblocks at undo time, not taken away, i.e. the minus sign should not be used. Fixes: 0d485ada404b ("xfs: use generic percpu counters for free block counter") Signed-off-by: Eryu Guan <guaneryu@gmail.com> --- fs/xfs/xfs_trans.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)