On Tue, May 12, 2020 at 08:53:20AM -0700, Darrick J. Wong wrote: > On Tue, May 12, 2020 at 01:10:37AM -0700, Christoph Hellwig wrote: > > On Mon, May 11, 2020 at 02:50:16PM -0400, Brian Foster wrote: > > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx> > > > --- > > > > > > What do folks think of something like this? We have a user report of a > > > corresponding read verifier failure while processing unlinked inodes. > > > This presumably means the attr fork was put in this state because the > > > format conversion and xattr set are not atomic. For example, the > > > filesystem crashed after the format conversion transaction hit the log > > > but before the xattr set transaction. The subsequent recovery succeeds > > > according to the logic below, but if the attr didn't hit the log the > > > leaf block remains empty and sets a landmine for the next read attempt. > > > This either prevents further xattr operations on the inode or prevents > > > the inode from being removed from the unlinked list due to xattr > > > inactivation failure. > > > > > > I've not confirmed that this is how the user got into this state, but > > > I've confirmed that it's possible. We have a couple band aids now (this > > > and the writeback variant) that intend to deal with this problem and > > > still haven't quite got it right, so personally I'm inclined to accept > > > the reality that an empty attr leaf block is an expected state based on > > > our current xattr implementation and just remove the check from the > > > verifier (at least until we have atomic sets). I turned it into a > > > warning/comment for the purpose of discussion. Thoughts? > > > > If the transaction is not atomic I don't think we should even > > warn in this case, even if it is unlikely to happen.. > > I was gonna say, I think we've messed this up enough that I think we > just have to accept empty attr leaf blocks. :/ > That makes at least 3 votes (including me) to drop the check so I'll send a real patch after some regression testing. Thanks. Brian > I also think we should improve the ability to scan for and invalidate > incore buffers so that we can invalidate and truncate the attr fork > extents directly from an extent walk loop. It seems a little silly that > we have to walk the dabtree just to find out where multiblock remote > attr value structures might be hiding. > > --D >
