[Cluster-devel] kernel BUG at fs/gfs2/inode.h:64

[Mark Syms <Mark.Syms@citrix.com>]

We don't yet know how the assert got triggered as we've only seen it once and in the original form it looks like it would be very hard to trigger in any normal case (given that in default usage i_blocks should be at least 8 times what any putative value for change could be). So, for the assert to have triggered we've been asked to remove at least 8 times the number of blocks currently allocated to the inode. Possible causes could be a double release or some other higher level bug that will require further investigation to uncover.

	Mark.

-----Original Message-----
From: Andreas Gruenbacher <agruenba@redhat.com> 
Sent: 09 January 2019 13:31
To: Tim Smith <tim.smith@citrix.com>
Cc: cluster-devel <cluster-devel@redhat.com>; Igor Druzhinin <igor.druzhinin@citrix.com>; Mark Syms <Mark.Syms@citrix.com>
Subject: Re: [Cluster-devel] kernel BUG at fs/gfs2/inode.h:64

Mark and Tim,

On Wed, 9 Jan 2019 at 13:05, Tim Smith <tim.smith@citrix.com> wrote:
> On Tuesday, 8 January 2019 13:32:20 GMT Mark Syms wrote:
> > Hi,
> >
> > We've seen this in testing with 4.19.
> >
> > Full trace is at the bottom.
> >
> > Looking at the code though it looks like it will assert if the value 
> > of change is equal to the number of blocks currently allocated to the inode.
> > Is this expected or should the assert be using >= instead of > ?
>
> It's weirder. Looking at
>
> static inline void gfs2_add_inode_blocks(struct inode *inode, s64 
> change) {
>         gfs2_assert(GFS2_SB(inode), (change >= 0 || inode->i_blocks > -change));
>         change *= (GFS2_SB(inode)->sd_sb.sb_bsize/GFS2_BASIC_BLOCK);
>         inode->i_blocks += change;
> }
>
> where the BUG is happening, "inode->i_blocks" is counting of 512b 
> blocks and "change" is in units of whatever-the-superblock-said which 
> will probably be counting 4096b blocks, so the comparison makes no sense.

indeed. I wonder why this hasn't been discovered long ago.

> It should probably read
>
> static inline void gfs2_add_inode_blocks(struct inode *inode, s64 
> change) {
>         change *= (GFS2_SB(inode)->sd_sb.sb_bsize/GFS2_BASIC_BLOCK);
>         gfs2_assert(GFS2_SB(inode), (change >= 0 || inode->i_blocks >= -change));
>         inode->i_blocks += change;
> }
>
> so I'll make a patch for that unless someone wants to correct me.

You can just shift by (inode->i_blkbits - 9).

Can you still trigger the assert with this fix?

Thanks,
Andreas