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

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

[Eryu Guan]

"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(-)

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;
 	}
 
-- 
2.7.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eryu Guan]

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs