Mark and Tim,
On Wed, 9 Jan 2019 at 13:05, Tim Smith <tim.sm...@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
