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

[Dave Chinner <david@fromorbit.com>]

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