[PATCH] generic/347: Fix sporadic test failures

[PATCH] generic/347: Fix sporadic test failures

[Jan Kara]

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 <jack@suse.cz>
---
 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
+	done
+
+	# Unmount and check the device to make sure all metadata is written
+	_dmthin_check_fs
+	_dmthin_mount
+
+	# Write the data blocks to force thinp space allocation.
+	# Overfill it by a bit.
 	for I in `seq 1 500`; do
 		$XFS_IO_PROG -f -c "pwrite -W 0 1M" $SCRATCH_MNT/file$I &>/dev/null
 	done
-- 
2.51.0

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

[Zorro Lang]

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 <jack@suse.cz>
> ---
>  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

Hi Jan,

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.

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.

Do you think changing it to something smaller, like fallocate 0 64k, would
be better to populate the metadata structures without prematurely running
out of thin pool space?

Thanks,
Zorro

> +	done
> +
> +	# Unmount and check the device to make sure all metadata is written
> +	_dmthin_check_fs
> +	_dmthin_mount
> +
> +	# Write the data blocks to force thinp space allocation.
> +	# Overfill it by a bit.
>  	for I in `seq 1 500`; do
>  		$XFS_IO_PROG -f -c "pwrite -W 0 1M" $SCRATCH_MNT/file$I &>/dev/null
>  	done
> -- 
> 2.51.0
> 

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

[Jan Kara]

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 <jack@suse.cz>
> > ---
> >  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.

> 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
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 <jack@suse.com>
SUSE Labs, CR

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

[Zorro Lang]

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 <jack@suse.cz>
> > > ---
> > >  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 <jack@suse.com>
> SUSE Labs, CR