Re: [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled

From: Eryu Guan <guan@eryu.me>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: Chandan Babu R <chandanrlinux@gmail.com>,
	fstests@vger.kernel.org, guaneryu@gmail.com,
	linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled
Date: Sun, 1 Aug 2021 21:10:04 +0800	[thread overview]
Message-ID: <YQadLPYDvDknRFU2@desktop> (raw)
In-Reply-To: <YQaIVYSIwG5UmzS0@desktop>

On Sun, Aug 01, 2021 at 07:41:09PM +0800, Eryu Guan wrote:
> On Tue, Jul 27, 2021 at 11:37:34AM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 27, 2021 at 10:15:27AM +0530, Chandan Babu R wrote:
> > > On 26 Jul 2021 at 22:49, Darrick J. Wong wrote:
> > > > On Mon, Jul 26, 2021 at 12:13:13PM +0530, Chandan Babu R wrote:
> > > >> _scratch_do_mkfs constructs a mkfs command line by concatenating the values of
> > > >> 1. $mkfs_cmd
> > > >> 2. $MKFS_OPTIONS
> > > >> 3. $extra_mkfs_options
> > > >>
> > > >> The corresponding mkfs command line fails if $MKFS_OPTIONS enables either
> > > >> reflink or rmapbt feature. The failure occurs because the test tries to create
> > > >> a filesystem with realtime device enabled. In such a case, _scratch_do_mkfs()
> > > >> will construct and invoke an mkfs command line without including the value of
> > > >> $MKFS_OPTIONS.
> > > >>
> > > >> To prevent such silent failures, this commit causes the test to exit if it
> > > >> detects either reflink or rmapbt feature being enabled.
> > > >
> > > > Er, what combinations of mkfs.xfs and MKFS_OPTIONS cause this result?
> > > > What kind of fs configuration comes out of that?
> > > 
> > > With MKFS_OPTIONS set as shown below,
> > > 
> > > export MKFS_OPTIONS="-m reflink=1 -b size=1k"
> > > 
> > > _scratch_do_mkfs() invokes mkfs.xfs with both realtime and reflink options
> > > enabled. Such an invocation of mkfs.xfs fails causing _scratch_do_mkfs() to
> > > ignore the contents of $MKFS_OPTIONS while constructing and invoking mkfs.xfs
> > > once again.
> > > 
> > > This time, the fs block size will however be set to 4k (the default block
> > > size). At the beginning of the test we would have obtained the block size of
> > > the filesystem as 1k and used it to compute the size of the realtime device
> > > required to overflow realtime bitmap inode's max pseudo extent count.
> > > 
> > > Invocation of xfs_growfs (made later in the test) ends up succeeding since a
> > > 4k fs block can accommodate more bits than a 1k fs block.
> > 
> > OK, now I think I've finally put all the pieces together.  Both of these
> > patches are fixing weirdness when MKFS_OPTIONS="-m reflink=1 -b size=1k".
> > 
> > With current HEAD, we try to mkfs.xfs with double "-b size" arguments.
> > That fails with 'option respecified', so fstests tries again without
> > MKFS_OPTIONS, which means you don't get the filesystem that you want.
> > If, say, MKFS_OPTIONS also contained bigtime=1, you won't get a bigtime
> > filesystem.
> > 
> > So the first patch removes the double -bsize arguments.  But you still
> > have the problem that the reflink=1 in MKFS_OPTIONS still causes
> > mkfs.xfs to fail (because we don't do rt and reflink yet), so fstests
> > again drops MKFS_OPTIONS, and now you're testing the fs without a block
> > size option at all.  The test still regresses because the special rt
> > geometry depends on the blocksize, and we didn't get all the geometry
> > elements that we need to trip the growfs failure.
> > 
> > Does the following patch fix all that for you?
> 
> Do you have plan to post formal patch? I think both problems could be
> fixed in one patch like you did. I'll leave patch 1 for now.

Ah, I saw the patch now, thanks!

Eryu