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

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Brian Foster <bfoster@redhat.com>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH RFC] xfs: warn instead of fail verifier on empty attr3 leaf block
Date: Tue, 12 May 2020 08:53:20 -0700	[thread overview]
Message-ID: <20200512155320.GD6714@magnolia> (raw)
In-Reply-To: <20200512081037.GB28206@infradead.org>

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. :/

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