Re: [PATCH 07/16] xfs: convert mount flags to features

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 07/16] xfs: convert mount flags to features
Date: Wed, 18 Aug 2021 12:25:19 +1000	[thread overview]
Message-ID: <20210818022519.GQ3657114@dread.disaster.area> (raw)
In-Reply-To: <20210811232856.GM3601443@magnolia>

On Wed, Aug 11, 2021 at 04:28:56PM -0700, Darrick J. Wong wrote:
> On Tue, Aug 10, 2021 at 05:38:00PM -0700, Darrick J. Wong wrote:
> > On Tue, Aug 10, 2021 at 03:24:42PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Replace m_flags feature checks with xfs_has_<feature>() calls and
> > > rework the setup code to set flags in m_features.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > 
> > AFAICT the only change since last time is in xfs_inode.c, right?
> > 
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> I'm having some second thoughts about the attr2 handling in this patch.
> xfs/186 regresses like so:
> 
> --- /tmp/fstests/tests/xfs/186.out      2021-05-13 11:47:55.849859833 -0700
> +++ /var/tmp/fstests/xfs/186.out.bad    2021-08-11 15:36:38.892735511 -0700
> @@ -195,6 +195,7 @@
>  
>  =================================
>  ATTR
> +ATTR2
>  forkoff = 47
>  u.sfdir2.hdr.count = 25
>  u.sfdir2.hdr.i8count = 0
>
> AFAICT, prior to this patch, if a V4 fs did not have attr2 set in the
> ondisk superblock and the user did not mount with -oattr2, the fs would
> continue to use attr1 format.  Indeed, xfs_sbversion_add_attr2 did:
> 
> 	if ((mp->m_flags & XFS_MOUNT_ATTR2) &&
> 	    !(xfs_sb_version_hasattr2(&mp->m_sb))) {
> 		/* try to add feature to ondisk super */
> 	}
> 
> Now, however, we mix the two together -- if the ondisk super has attr2
> set, XFS_FEAT_ATTR2 will be set, and if the mount options include
> -oattr2, XFS_FEAT_ATTR2 will also be set.  Now that function does:
> 
> 	if (xfs_has_attr2(mp))
> 		return;
> 	if (xfs_has_noattr2(mp))
> 		return;
> 
> 	/* try to add feature to ondisk super */
> 
> The behavior is not the same here -- if neither the ondisk sb nor the
> mount options have attr2, we upgrade an attr1 fs to attr2.  I think this
> is why xfs/186 has this regression.

Hmmm - I think I changed it to do that explicitly at one point, then
didn't remove it when splitting out attr2 specific stuff. e.g. the
comment now says:

/*
 * Switch on the ATTR2 superblock bit (implies also FEATURES2) by default unless
 * we've explicitly been told not to use attr2 (i.e. noattr2 mount option).
 */

Which is how it's behaving.  I'll just revert then "upgrade by
default" behaviour and go back to the way it used to work. That'll
also fix the other problem you mention, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com