Re: [PATCH] xfs: test mkfs.xfs sizing of internal logs that

[Dave Chinner <david@fromorbit.com>]

On Wed, May 25, 2022 at 06:35:20PM -0700, Darrick J. Wong wrote:
> On Thu, May 26, 2022 at 10:11:49AM +1000, Dave Chinner wrote:
> > On Wed, May 25, 2022 at 04:21:36PM -0700, Darrick J. Wong wrote:
> > > On Wed, May 25, 2022 at 10:24:13AM +1000, Dave Chinner wrote:
> > > > On Tue, May 24, 2022 at 04:49:30PM -0700, Darrick J. Wong wrote:
> > > > > On Wed, May 25, 2022 at 09:44:26AM +1000, Dave Chinner wrote:
> > > > > > On Tue, May 24, 2022 at 12:52:57PM -0700, Darrick J. Wong wrote:
> > > > > > > +# First we try various small filesystems and stripe sizes.
> > > > > > > +for M in `seq 298 302` `seq 490 520`; do
> > > > > > > +	for S in `seq 32 4 64`; do
> > > > > > > +		test_format "M=$M S=$S" -dsu=${S}k,sw=1,size=${M}m
> > > > > > > +	done
> > > > > > > +done
> > > > > > > +
> > > > > > > +# log so large it pushes the root dir into AG 1
> > > > > > > +test_format "log pushes rootdir into AG 1" -d agcount=3200,size=6366g -lagnum=0
> > > > > 
> > > > > ...this particular check in mkfs only happens after we allocate the root
> > > > > directory, which an -N invocation doesn't do.
> > > > 
> > > > Ok, so for this test can we drop the -N? We don't need to do 30 IOs
> > > > and write 64MB logs for every config we test - I think there's ~35 *
> > > > 8 invocations of test_format in the loop above before we get here...
> > > > 
> > > > Also, why do we need a 6.3TB filesystem with 2.1GB AGs and a 2GB log
> > > > to trigger this? That means we have to write 2GB to disk, plus
> > > > ~20,000 write IOs for the AG headers and btree root blocks before we
> > > > get to the failure case, yes?
> > > > 
> > > > Why not just exercise the failure case with something like this:
> > > > 
> > > > # mkfs.xfs -d agcount=2,size=64M -l size=8180b,agnum=0 -d file,name=test.img
> > > > meta-data=test.img               isize=512    agcount=2, agsize=8192 blks
> > > >          =                       sectsz=512   attr=2, projid32bit=1
> > > >          =                       crc=1        finobt=1, sparse=1, rmapbt=0
> > > >          =                       reflink=1    bigtime=0 inobtcount=0
> > > > data     =                       bsize=4096   blocks=16384, imaxpct=25
> > > >          =                       sunit=0      swidth=0 blks
> > > > naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
> > > > log      =internal log           bsize=4096   blocks=8180, version=2
> > > >          =                       sectsz=512   sunit=0 blks, lazy-count=1
> > > > realtime =none                   extsz=4096   blocks=0, rtextents=0
> > > > mkfs.xfs: root inode created in AG 1, not AG 0
> > > 
> > > Welll... a better reason for why this test can't do that -- one of my
> > > fixes also made mkfs reject -l size=XXX when XXX is not possible.
> > 
> > So you make other changes to mkfs that aren't yet upstream that
> > check whether the log causes the root inode to change position?
> 
> It's in for-next, is that not good enough?

Sorry, I didn't realise Eric had pulled it in. I'm not keep a close
eye on exactly what non-libxfs-sync stuff going into xfsprogs right
now - I've had to filter that stuff out to concentrate on the kernel
cycle stuff...

As it is, it's fine however we reject this case, isn't? it doesn't
have to be the root inode error message that causes it to abort, as
long as we get a non-zero exit value. Hence the above method would
fail with both old and new mkfs binaries, even if they are detecting
the issue differently.

> > I mean, '-l size=8180b' results in a log that fits perfectly fine in
> > an empty AG, and so you must have added more checks to detect this
> > moves the root inode somehow?
> > 
> > > That said... -d agcount=3200,size=6366g -lagnum=0 -N seems to work?
> > 
> > On a new mkfs binary with those checks that you mention above?  If
> > so, it seems to me that those new checks trigger for this case, too,
> > before we write anything?
> >
> > IOWs, this won't fail on older mkfs.xfs binaries that don't have
> > these new log size checks because we never get around to allocating
> > the root inode and checking it's location, right?  Yet what we
> > really want here is for it to fail on an old mkfs binary?
> 
> Yeah, something like that.  It's been so long since Eric and I were
> actively poking on this that I don't remember so well anymore.

Worth checking, because ISTM that if it doesn't fail on old mkfs,
then we'd still the non "-N" version or the targetted log size
variant like above. Of course, I could still be misunderstanding
what you're trying to exercise here...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com