<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Exchange Server"> <style></style> </head> <body> <div> <div style="font-family:Calibri,sans-serif"> <div dir="ltr">So, actually the original comparison in the assert (dodgy scaling factor not withstanding) was probably correct in that we don't ever want to remove all blocks from the inode as one of them is used for the inode itself? Or do we still think it should allow for change to be the negative of current blocks?</div> <div dir="ltr">Mark</div> <hr> From: Andreas Gruenbacher <agruenba@redhat.com> Sent: Wednesday, 9 January 2019 17:24 To: Tim Smith <tim.smith@citrix.com> CC: Mark Syms <Mark.Syms@citrix.com>,cluster-devel <cluster-devel@redhat.com>,Igor Druzhinin <igor.druzhinin@citrix.com> Subject: Re: [Cluster-devel] kernel BUG at fs/gfs2/inode.h:64 </div> </div> <div class="PlainText">On Wed, 9 Jan 2019 at 18:14, Tim Smith <tim.smith@citrix.com> wrote: > > On Wednesday, 9 January 2019 15:35:05 GMT Andreas Gruenbacher wrote: > > On Wed, 9 Jan 2019 at 14:43, Mark Syms <Mark.Syms@citrix.com> wrote: > > > 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. > > > > The following change has at least survived xfstests: > > > > --- a/fs/gfs2/inode.h > > +++ b/fs/gfs2/inode.h > > @@ -61,8 +61,8 @@ static inline u64 gfs2_get_inode_blocks(const struct > > inode *inode) > > > > 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); > > + change <<= inode->i_blkbits - 9; > > + gfs2_assert(GFS2_SB(inode), change >= 0 || inode->i_blocks >= -change); > > inode->i_blocks += change; > > } > > > > Andreas > > I'll use > > change <<= (GFS2_SB(inode)->sd_sb.sb_bsize_shift - GFS2_BASIC_BLOCK_SHIFT); > > for consistency with the gfs2_get/set_inode_blocks(). I'll send the patch in a > bit. > > Given what it was like before, either i_blocks was already 0 or -change > somehow became stupidly huge. Anything else looks like it would be hard to > reproduce without mkfs.gfs2 -b 512 (so that GFS2_SB(inode)->sd_sb.sb_bsize == > GFS2_BASIC_BLOCK) which we don't do. > > I'll try to work out what could have caused it and see if we can provoke it > again. > > Out of curiosity I did a few tests where I created a file on GFS2, copied / > dev/null on top of it, and then ran stat on the file. It seems like GFS2 never > frees the last allocation on truncate; stat always reports 1, 2, 4 or 8 blocks > in use for a zero-length file depending on the underlying filesystem block > size, unlike (say) ext3/4 where it reports 0. I presume this is intentional so > maybe some corner case where it *is* trying to do that is the root of the > problem. Yes, that's intentional. An empty file without extended attributes consumes one block. Extended attributes consume at least one additional block. Andreas </div> </body> </html>