Re: [PATCH RFC] xfs: warn instead of fail verifier on empty attr3 leaf block

[Brian Foster <bfoster@redhat.com>]

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@redhat.com>
> > > ---
> > > 
> > > 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
>