Re: [PATCH] generic/347: Fix sporadic test failures

On Mon, Aug 03, 2026 at 01:20:22PM +0200, Jan Kara wrote:
> Hi Zorro!
> 
> On Mon 03-08-26 16:27:15, Zorro Lang wrote:
> > On Thu, Jul 30, 2026 at 05:18:17PM +0200, Jan Kara wrote:
> > > generic/347 was occasionally failing on ext4 in our QA due to ext4
> > > aborting its journal when the filesystem on thinp device was overfilled.
> > > I have tracked the problem down to journal checkpointing failing to
> > > write a metadata block to its final location due to ENOSPC failure from
> > > the thinp device. Modify the test to first preallocate blocks for the
> > > files and remount the filesystem which practically makes sure all
> > > involved metadata blocks were written and so their further modifications
> > > will not fail.
> > > 
> > > Signed-off-by: Jan Kara <[email protected]>
> > > ---
> > >  tests/generic/347 | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/generic/347 b/tests/generic/347
> > > index 06df0cf9eddc..56538c160392 100755
> > > --- a/tests/generic/347
> > > +++ b/tests/generic/347
> > > @@ -38,7 +38,17 @@ _setup_thin()
> > >  
> > >  _workout()
> > >  {
> > > -	# Overfill it by a bit
> > > +	# Preallocate space to avoid failure for metadata writeback
> > > +	for I in `seq 1 500`; do
> > > +		$XFS_IO_PROG -f -c "falloc 0 1M" $SCRATCH_MNT/file$I &>/dev/null
> > 
> > Thanks for this fix! Adding fallocate introduces an extra dependency via
> > _require_xfs_io_command "falloc", which will limit some filesystems can
> > run this test.
> 
> I agree the fix has some downsides. OTOH when I was thinking about it I've
> concluded that filesystems where you realistically care about behavior on
> thinp storage also do support fallocate. So I don't think this it's a
> serious test coverage limitation but it's up for discussion.

Sure, if we accept this patch, we'll need the _require_xfs_io_command "falloc"
restriction. This is a basic test for filesystems on thinp, let's try to get
more review points, if there are no objections, we can merge it at first.

>
> > Additionally, since the underlying storage is a thinp device, I doubt
> > `falloc 0 1M` actually triggers physical block allocation on the thin
> > pool (correct me if I'm wrong). If so, I suspect this 1M * 500 preallocation
> > might exhaust the 500M BACKING_SIZE prematurely (along with file system
> > metadata overhead), similar to the pwrite loop below.
> 
> We do *not* want fallocate to trigger the data block allocation. It will

Sorry, I previously thought that fallocate would trigger actual physical block
allocation on thinp under certain filesystems or mount options. However,
I just tested it out (including exFAT, and Btrfs with nodatacow/compress=zstd),
and found that none of them actually allocate physical space.

Thanks,
Zorro

> however modify all the relevant metadata blocks and subsequent unmount will
> writeout these modified metadata which forces the physical block allocation
> for the metadata which is what we need. Physical space for data blocks is
> not reserved from thinp during fallocate so we should not run out of
> backing device space during this loop.
> 
> 								Honza
> 
> -- 
> Jan Kara <[email protected]>
> SUSE Labs, CR