diff mbox series

[09/17] generic/562: handle ENOSPC while cloning gracefully

Message ID 173229420148.358248.4652252209849197144.stgit@frogsfrogsfrogs (mailing list archive)
State New
Headers show
Series [01/17] generic/757: fix various bugs in this test | expand

Commit Message

Darrick J. Wong Nov. 22, 2024, 4:52 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

This test creates a couple of patterned files on a tiny filesystem,
fragments the free space, clones one patterned file to the other, and
checks that the entire file was cloned.

However, this test doesn't work on a 64k fsblock filesystem because
we've used up all the free space reservation for the rmapbt, and that
causes the FICLONE to error out with ENOSPC partway through.  Hence we
need to detect the ENOSPC and _notrun the test.

That said, it turns out that XFS has been silently dropping error codes
if we managed to make some progress cloning extents.  That's ok if the
operation has REMAP_FILE_CAN_SHORTEN like copy_file_range does, but
FICLONE/FICLONERANGE do not permit partial results, so the dropped error
codes is actually an error.

Therefore, this testcase now becomes a regression test for the patch to
fix that.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 tests/generic/562 |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Nov. 25, 2024, 5:14 a.m. UTC | #1 
On Fri, Nov 22, 2024 at 08:52:48AM -0800, Darrick J. Wong wrote:
> +# with ENOSPC for example.  However, XFS will sometimes run out of space.
> +_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
> +cat $tmp.err
> +test "$FSTYP" = "xfs" && grep -q 'No space left on device' $tmp.err && \
> +	_notrun "ran out of space while cloning"

Should this simply be unconditional instead of depend on XFS?
Darrick J. Wong Nov. 25, 2024, 5:16 a.m. UTC | #2 
On Sun, Nov 24, 2024 at 09:14:43PM -0800, Christoph Hellwig wrote:
> On Fri, Nov 22, 2024 at 08:52:48AM -0800, Darrick J. Wong wrote:
> > +# with ENOSPC for example.  However, XFS will sometimes run out of space.
> > +_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
> > +cat $tmp.err
> > +test "$FSTYP" = "xfs" && grep -q 'No space left on device' $tmp.err && \
> > +	_notrun "ran out of space while cloning"
> 
> Should this simply be unconditional instead of depend on XFS?

Felipe said no:
https://lore.kernel.org/fstests/CAL3q7H5KjvXsXzt4n0XP1FTUt=A5cKom7p+dGD6GG-iL7CyDXQ@mail.gmail.com/

(which I should reply to)

--D
Christoph Hellwig Nov. 25, 2024, 5:20 a.m. UTC | #3 
On Sun, Nov 24, 2024 at 09:16:39PM -0800, Darrick J. Wong wrote:
> On Sun, Nov 24, 2024 at 09:14:43PM -0800, Christoph Hellwig wrote:
> > On Fri, Nov 22, 2024 at 08:52:48AM -0800, Darrick J. Wong wrote:
> > > +# with ENOSPC for example.  However, XFS will sometimes run out of space.
> > > +_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
> > > +cat $tmp.err
> > > +test "$FSTYP" = "xfs" && grep -q 'No space left on device' $tmp.err && \
> > > +	_notrun "ran out of space while cloning"
> > 
> > Should this simply be unconditional instead of depend on XFS?
> 
> Felipe said no:
> https://lore.kernel.org/fstests/CAL3q7H5KjvXsXzt4n0XP1FTUt=A5cKom7p+dGD6GG-iL7CyDXQ@mail.gmail.com/

Hmm.   Being able to totally fill the fs without ENOSPC seems odd.
Maybe we need to figure out a way to scale down the size for the generic
test and have a separate one for the XFS ENOSPC case?  Not a huge fan
of that, but the current version also seems odd.
Darrick J. Wong Nov. 26, 2024, 1:26 a.m. UTC | #4 
On Sun, Nov 24, 2024 at 09:20:29PM -0800, Christoph Hellwig wrote:
> On Sun, Nov 24, 2024 at 09:16:39PM -0800, Darrick J. Wong wrote:
> > On Sun, Nov 24, 2024 at 09:14:43PM -0800, Christoph Hellwig wrote:
> > > On Fri, Nov 22, 2024 at 08:52:48AM -0800, Darrick J. Wong wrote:
> > > > +# with ENOSPC for example.  However, XFS will sometimes run out of space.
> > > > +_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
> > > > +cat $tmp.err
> > > > +test "$FSTYP" = "xfs" && grep -q 'No space left on device' $tmp.err && \
> > > > +	_notrun "ran out of space while cloning"
> > > 
> > > Should this simply be unconditional instead of depend on XFS?
> > 
> > Felipe said no:
> > https://lore.kernel.org/fstests/CAL3q7H5KjvXsXzt4n0XP1FTUt=A5cKom7p+dGD6GG-iL7CyDXQ@mail.gmail.com/
> 
> Hmm.   Being able to totally fill the fs without ENOSPC seems odd.
> Maybe we need to figure out a way to scale down the size for the generic
> test and have a separate one for the XFS ENOSPC case?  Not a huge fan
> of that, but the current version also seems odd.

Yeah, I definitely need to write a fstest that can trip this bug on
smaller fsblock filesystems.  In the meantime, this one should not fail
just because xfs runs out of space before the point where this test
would have thought that would happen; and then xfs_io spews an error
message into the golden output.

Though if this is really a test that computes when *btrfs* would run out
of space and drives towards that point just to see if ENOSPC does /not/
come out of the kernel, then maybe this belongs in tests/btrfs/ ?

--D
diff mbox series

Patch

diff --git a/tests/generic/562 b/tests/generic/562
index 91360c4154a6a2..36bd02911c96b8 100755
--- a/tests/generic/562
+++ b/tests/generic/562
@@ -15,6 +15,9 @@  _begin_fstest auto clone punch
 . ./common/filter
 . ./common/reflink
 
+test "$FSTYP" = "xfs" && \
+	_fixed_by_kernel_commit XXXXXXXXXX "xfs: don't drop errno values when we fail to ficlone the entire range"
+
 _require_scratch_reflink
 _require_test_program "punch-alternating"
 _require_xfs_io_command "fpunch"
@@ -48,8 +51,11 @@  while true; do
 done
 
 # Now clone file bar into file foo. This is supposed to succeed and not fail
-# with ENOSPC for example.
-_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full
+# with ENOSPC for example.  However, XFS will sometimes run out of space.
+_reflink $SCRATCH_MNT/bar $SCRATCH_MNT/foo >>$seqres.full 2> $tmp.err
+cat $tmp.err
+test "$FSTYP" = "xfs" && grep -q 'No space left on device' $tmp.err && \
+	_notrun "ran out of space while cloning"
 
 # Unmount and mount the filesystem again to verify the operation was durably
 # persisted.