Re: [PATCH 05/12] generic/562: handle ENOSPC while cloning gracefully

Re: [PATCH 05/12] generic/562: handle ENOSPC while cloning gracefully

[Filipe Manana]

On Mon, Nov 18, 2024 at 11:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> 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(-)
>
>
> diff --git a/tests/generic/562 b/tests/generic/562
> index 91360c4154a6a2..62899945003513 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
> +grep -q 'No space left on device' $tmp.err && \
> +       _notrun "ran out of space while cloning"

This defeats the original purpose of the test, which was to verify
btrfs didn't fail with -ENOSPC (or any other error).

If XFS has an ENOSPC issue in some cases, and it's not fixable, why
not make it _notrun only if it's XFS that is being tested?

Thanks.


>
>  # Unmount and mount the filesystem again to verify the operation was durably
>  # persisted.
>
>

Re: [PATCH 05/12] generic/562: handle ENOSPC while cloning gracefully

[Darrick J. Wong]

On Tue, Nov 19, 2024 at 12:17:56AM +0000, Filipe Manana wrote:
> On Mon, Nov 18, 2024 at 11:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > 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(-)
> >
> >
> > diff --git a/tests/generic/562 b/tests/generic/562
> > index 91360c4154a6a2..62899945003513 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
> > +grep -q 'No space left on device' $tmp.err && \
> > +       _notrun "ran out of space while cloning"
> 
> This defeats the original purpose of the test, which was to verify
> btrfs didn't fail with -ENOSPC (or any other error).
> 
> If XFS has an ENOSPC issue in some cases, and it's not fixable, why
> not make it _notrun only if it's XFS that is being tested?

Ok, will do.  In the case of xfs, we don't let you share data if the
allocation group it's in is more than about 90% full.

--D

> Thanks.
> 
> 
> >
> >  # Unmount and mount the filesystem again to verify the operation was durably
> >  # persisted.
> >
> >
> 

Re: [PATCH 05/12] generic/562: handle ENOSPC while cloning gracefully

[Darrick J. Wong]

On Mon, Nov 18, 2024 at 04:31:31PM -0800, Darrick J. Wong wrote:
> On Tue, Nov 19, 2024 at 12:17:56AM +0000, Filipe Manana wrote:
> > On Mon, Nov 18, 2024 at 11:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > 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(-)
> > >
> > >
> > > diff --git a/tests/generic/562 b/tests/generic/562
> > > index 91360c4154a6a2..62899945003513 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
> > > +grep -q 'No space left on device' $tmp.err && \
> > > +       _notrun "ran out of space while cloning"
> > 
> > This defeats the original purpose of the test, which was to verify
> > btrfs didn't fail with -ENOSPC (or any other error).
> > 
> > If XFS has an ENOSPC issue in some cases, and it's not fixable, why
> > not make it _notrun only if it's XFS that is being tested?
> 
> Ok, will do.  In the case of xfs, we don't let you share data if the
> allocation group it's in is more than about 90% full.

Er... scratch that response :)

It's true that XFS doesn't let you share data if the AG is more than 90%
full, but this test exposed a data corruption vector in 4.20-6.12 if you
did run out of space.  Granted you'd only hit that if you had 64k block
size and a fairly small disk (~33GB) but still, that's reason enough to
keep running on XFS even if it runs out of space.  ENOSPC is better than
data corruption.

--D

> --D
> 
> > Thanks.
> > 
> > 
> > >
> > >  # Unmount and mount the filesystem again to verify the operation was durably
> > >  # persisted.
> > >
> > >
> > 
>