linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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 19:41:09 +0800	[thread overview]
Message-ID: <YQaIVYSIwG5UmzS0@desktop> (raw)
In-Reply-To: <20210727183734.GD559142@magnolia>

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.

Thanks,
Eryu

> 
> --D
> 
> diff --git a/tests/xfs/530 b/tests/xfs/530
> index 4d168ac5..9c6f44d7 100755
> --- a/tests/xfs/530
> +++ b/tests/xfs/530
> @@ -60,10 +60,22 @@ echo "Format and mount rt volume"
>  
>  export USE_EXTERNAL=yes
>  export SCRATCH_RTDEV=$rtdev
> -_scratch_mkfs -d size=$((1024 * 1024 * 1024)) -b size=${dbsize} \
> +_scratch_mkfs -d size=$((1024 * 1024 * 1024)) \
>  	      -r size=${rtextsz},extsize=${rtextsz} >> $seqres.full
>  _try_scratch_mount || _notrun "Couldn't mount fs with synthetic rt volume"
>  
> +# If we didn't get the desired realtime volume and the same blocksize as the
> +# first format (which we used to compute a specific rt geometry), skip the
> +# test.  This can happen if the MKFS_OPTIONS conflict with the ones we passed
> +# to _scratch_mkfs or do not result in a valid rt fs geometry.  In this case,
> +# _scratch_mkfs will try to "succeed" at formatting by dropping MKFS_OPTIONS,
> +# giving us the wrong geometry.
> +formatted_blksz="$(_get_block_size $SCRATCH_MNT)"
> +test "$formatted_blksz" -ne "$dbsize" && \
> +	_notrun "Tried to format with $dbsize blocksize, got $formatted_blksz."
> +$XFS_INFO_PROG $SCRATCH_MNT | egrep -q 'realtime.*blocks=0' && \
> +	_notrun "Filesystem should have a realtime volume"
> +
>  echo "Consume free space"
>  fillerdir=$SCRATCH_MNT/fillerdir
>  nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
> 
> > >
> > > Eventually, the plan is to support rmap[1] and reflink[2] on the
> > > realtime device, at which point this will have to be torn out and a
> > > better solution found.
> > >
> > > --D
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-rmap
> > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-reflink
> > >
> > 
> > --
> > chandan

  parent reply	other threads:[~2021-08-01 11:41 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26  6:43 [PATCH 1/3] xfs/530: Do not pass block size argument to _scratch_mkfs Chandan Babu R
2021-07-26  6:43 ` [PATCH 2/3] common/xfs: Add helpers to obtain reflink/rmapbt status of a filesystem Chandan Babu R
2021-07-26  6:43 ` [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled Chandan Babu R
2021-07-26 17:19   ` Darrick J. Wong
2021-07-27  4:45     ` Chandan Babu R
2021-07-27 18:37       ` Darrick J. Wong
2021-07-28  2:35         ` Chandan Babu R
2021-08-01 11:41         ` Eryu Guan [this message]
2021-08-01 13:10           ` Eryu Guan
2021-07-26 17:08 ` [PATCH 1/3] xfs/530: Do not pass block size argument to _scratch_mkfs Darrick J. Wong
2021-08-01 11:18   ` Eryu Guan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YQaIVYSIwG5UmzS0@desktop \
    --to=guan@eryu.me \
    --cc=chandanrlinux@gmail.com \
    --cc=djwong@kernel.org \
    --cc=fstests@vger.kernel.org \
    --cc=guaneryu@gmail.com \
    --cc=linux-xfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).