Re: [PATCH 03/16] xfs: rework attr2 feature and mount options

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 03/16] xfs: rework attr2 feature and mount options
Date: Wed, 18 Aug 2021 11:18:07 +1000	[thread overview]
Message-ID: <20210818011807.GN3657114@dread.disaster.area> (raw)
In-Reply-To: <20210811230405.GL3601443@magnolia>

On Wed, Aug 11, 2021 at 04:04:05PM -0700, Darrick J. Wong wrote:
> On Tue, Aug 10, 2021 at 03:24:38PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The attr2 feature is somewhat unique in that it has both a superblock
> > feature bit to enable it and mount options to enable and disable it.
> > 
> > Back when it was first introduced in 2005, attr2 was disabled unless
> > either the attr2 superblock feature bit was set, or the attr2 mount
> > option was set. If the superblock feature bit was not set but the
> > mount option was set, then when the first attr2 format inode fork
> > was created, it would set the superblock feature bit. This is as it
> > should be - the superblock feature bit indicated the presence of the
> > attr2 on disk format.
......
> 
> I see the following regression on xfs/187 with this patch applied:
> 
> --- /tmp/fstests/tests/xfs/187.out      2021-05-13 11:47:55.849859833 -0700
> +++ /var/tmp/fstests/xfs/187.out.bad    2021-08-11 15:59:15.692618610 -0700
> @@ -9,6 +9,8 @@
>  
>  noattr2 fs
>  
> +MOREBITS
> +ATTR2
>  
>  *** 2. test attr2 mkfs and then noattr2 mount with 1 EA ***
>  
> @@ -23,6 +25,8 @@
>  user.test
>  
>  ATTR
> +MOREBITS
> +ATTR2
>  
>  *** 3. test noattr2 mount and lazy sb ***
>  
> @@ -36,4 +40,5 @@
>  noattr2 fs
>  
>  MOREBITS
> +ATTR2
>  LAZYSBCOUNT
> 
> I am pretty sure this is a direct result of "This will not remove the
> superblock feature bit", correct?  Do you have an adjustment to xfs/187
> to avoid regressing QA?

I've been looking at 187, and the tests that are failing are testing
the specific behaviour of "mkfs w/attr2 enabled; mount noattr2 and
ensure the attr2 version bits are not set".

Basically, the premise of the noattr2 verification tests are not
valid anymore, but adding filters to mask out the addition of
MOREBITS and ATTR2 basically renders the test useless for validating
the old behaviour is still correct.

IOWs, there's no way to detect if a kernel is going to behave like
the old code and remove the bits, or whether it is going to just
leave the attr2 fields set on disk. Hence I've got no idea how we'd
change xfs/187 to validate both the old and new behaviour because we
have no way of knowing which behaviour to expect.

So I'd vote for just throwing away xfs/187 because a) it looks
unfixable to me, and b) it's trying to validate what should be
considered broken behaviour (i.e. on-disk feature bit gets removed
even though the feature is still present on disk)...

Your thoughts?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com