On Fri, Apr 28, 2023 at 12:11:25PM +1000, Dave Chinner wrote:
> On Thu, Apr 27, 2023 at 03:49:05PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > Through generic/300, I discovered that mkfs.xfs creates corrupt
> > filesystems when given these parameters:
> >
> > # mkfs.xfs -d size=512M /dev/sda -f -d su=128k,sw=4 --unsupported
> > Filesystems formatted with --unsupported are not supported!!
> > meta-data=/dev/sda isize=512 agcount=8, agsize=16352 blks
> > = sectsz=512 attr=2, projid32bit=1
> > = crc=1 finobt=1, sparse=1, rmapbt=1
> > = reflink=1 bigtime=1 inobtcount=1 nrext64=1
> > data = bsize=4096 blocks=130816, imaxpct=25
> > = sunit=32 swidth=128 blks
> > naming =version 2 bsize=4096 ascii-ci=0, ftype=1
> > log =internal log bsize=4096 blocks=8192, version=2
> > = sectsz=512 sunit=32 blks, lazy-count=1
> > realtime =none extsz=4096 blocks=0, rtextents=0
> > = rgcount=0 rgsize=0 blks
> > Discarding blocks...Done.
> > # xfs_repair -n /dev/sda
> > Phase 1 - find and verify superblock...
> > - reporting progress in intervals of 15 minutes
> > Phase 2 - using internal log
> > - zero log...
> > - 16:30:50: zeroing log - 16320 of 16320 blocks done
> > - scan filesystem freespace and inode maps...
> > agf_freeblks 25, counted 0 in ag 4
> > sb_fdblocks 8823, counted 8798
> >
> > The root cause of this problem is the numrecs handling in
> > xfs_freesp_init_recs, which is used to initialize a new AG. Prior to
> > calling the function, we set up the new bnobt block with numrecs == 1
> > and rely on _freesp_init_recs to format that new record. If the last
> > record created has a blockcount of zero, then it sets numrecs = 0.
> >
> > That last bit isn't correct if the AG contains the log, the start of the
> > log is not immediately after the initial blocks due to stripe alignment,
> > and the end of the log is perfectly aligned with the end of the AG. For
> > this case, we actually formatted a single bnobt record to handle the
> > free space before the start of the (stripe aligned) log, and incremented
> > arec to try to format a second record. That second record turned out to
> > be unnecessary, so what we really want is to leave numrecs at 1.
> >
> > The numrecs handling itself is overly complicated because a different
> > function sets numrecs == 1. Change the bnobt creation code to start
> > with numrecs set to zero and only increment it after successfully
> > formatting a free space extent into the btree block.
> >
> > Fixes: f327a00745ff ("xfs: account for log space when formatting new AGs")
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_ag.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index 1b078bbbf225..4481ce8ead9d 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -499,6 +499,7 @@ xfs_freesp_init_recs(
> > */
> > arec->ar_blockcount = cpu_to_be32(start -
> > mp->m_ag_prealloc_blocks);
> > + be16_add_cpu(&block->bb_numrecs, 1);
> > nrec = arec + 1;
> >
> > /*
> > @@ -509,7 +510,6 @@ xfs_freesp_init_recs(
> > be32_to_cpu(arec->ar_startblock) +
> > be32_to_cpu(arec->ar_blockcount));
> > arec = nrec;
> > - be16_add_cpu(&block->bb_numrecs, 1);
> > }
> > /*
> > * Change record start to after the internal log
> > @@ -525,8 +525,8 @@ xfs_freesp_init_recs(
> > */
> > arec->ar_blockcount = cpu_to_be32(id->agsize -
> > be32_to_cpu(arec->ar_startblock));
> > - if (!arec->ar_blockcount)
> > - block->bb_numrecs = 0;
> > + if (arec->ar_blockcount)
> > + be16_add_cpu(&block->bb_numrecs, 1);
>
> Ok, but I think the comment above this about resetting the count
> back to zero needs to be updated as we aren't resetting anything
> back to zero anymore.
Done:
/*
* Calculate the block count of this record; if it is nonzero,
* increment the record count.
*/
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
