Re: [PATCH] xfs: undo block reservation correctly in xfs_trans_reserve()

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 <[email protected]>
> 
> 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

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs