On Fri, May 15, 2020 at 12:06:48PM -0400, Brian Foster wrote: > The attr fork can transition from shortform to leaf format while > empty if the first xattr doesn't fit in shortform. While this empty > leaf block state is intended to be transient, it is technically not > due to the transactional implementation of the xattr set operation. > > We historically have a couple of bandaids to work around this > problem. The first is to hold the buffer after the format conversion > to prevent premature writeback of the empty leaf buffer and the > second is to bypass the xattr count check in the verifier during > recovery. The latter assumes that the xattr set is also in the log > and will be recovered into the buffer soon after the empty leaf > buffer is reconstructed. This is not guaranteed, however. > > If the filesystem crashes after the format conversion but before the > xattr set that induced it, only the format conversion may exist in > the log. When recovered, this creates a latent corrupted state on > the inode as any subsequent attempts to read the buffer fail due to > verifier failure. This includes further attempts to set xattrs on > the inode or attempts to destroy the attr fork, which prevents the > inode from ever being removed from the unlinked list. > > To avoid this condition, accept that an empty attr leaf block is a > valid state and remove the count check from the verifier. This means > that on rare occasions an attr fork might exist in an unexpected > state, but is otherwise consistent and functional. Note that we > retain the logic to avoid racing with metadata writeback to reduce > the window where this can occur. > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> --D > --- > > v2: > - Add comment. > v1: https://lore.kernel.org/linux-xfs/20200513145343.45855-1-bfoster@xxxxxxxxxx/ > - Remove the verifier check instead of warn. > rfc: https://lore.kernel.org/linux-xfs/20200511185016.33684-1-bfoster@xxxxxxxxxx/ > > fs/xfs/libxfs/xfs_attr_leaf.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 863444e2dda7..6d18e86bb9c7 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -308,14 +308,6 @@ xfs_attr3_leaf_verify( > if (fa) > return fa; > > - /* > - * In recovery there is a transient state where count == 0 is valid > - * because we may have transitioned an empty shortform attr to a leaf > - * if the attr didn't fit in shortform. > - */ > - if (!xfs_log_in_recovery(mp) && ichdr.count == 0) > - return __this_address; > - > /* > * firstused is the block offset of the first name info structure. > * Make sure it doesn't go off the block or crash into the header. > @@ -331,6 +323,13 @@ xfs_attr3_leaf_verify( > (char *)bp->b_addr + ichdr.firstused) > return __this_address; > > + /* > + * NOTE: This verifier historically failed empty leaf buffers because > + * we expect the fork to be in another format. Empty attr fork format > + * conversions are possible during xattr set, however, and format > + * conversion is not atomic with the xattr set that triggers it. We > + * cannot assume leaf blocks are non-empty until that is addressed. > + */ > buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize; > for (i = 0, ent = entries; i < ichdr.count; ent++, i++) { > fa = xfs_attr3_leaf_verify_entry(mp, buf_end, leaf, &ichdr, > -- > 2.21.1 >
