[RFC PATCH 0/2] Improve user experience with xfs_fsr

[RFC PATCH 0/2] Improve user experience with xfs_fsr

[cem]

From: Carlos Maiolino <cem@kernel.org>

Those are a couple patches referring to a user report in IRC. I didn't
properly test them yet, reason why RFC tag is added, also, I'm
interested on others POV on these changes

Carlos Maiolino (2):
  libfrog: make xfrog_defragrange return a positive valued
  fsr: Always print error messages from xfrog_defragrange()

 fsr/xfs_fsr.c           | 10 +++-------
 libfrog/file_exchange.c |  4 ++--
 2 files changed, 5 insertions(+), 9 deletions(-)

-- 
2.52.0

[RFC PATCH 1/2] libfrog: make xfrog_defragrange return a positive valued

[cem]

From: Carlos Maiolino <cem@kernel.org>

Currently, the only user for xfrog_defragrange is xfs_fsr's packfile(),
which expects error to be a positive value.

Whenever xfrog_defragrange fails, the switch case always falls into the
default clausule, making the error message pointless.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 libfrog/file_exchange.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfrog/file_exchange.c b/libfrog/file_exchange.c
index e6c3f486b0ff..31bbc6da60c3 100644
--- a/libfrog/file_exchange.c
+++ b/libfrog/file_exchange.c
@@ -232,7 +232,7 @@ xfrog_defragrange(
 	if (ret) {
 		if (errno == EOPNOTSUPP || errno != ENOTTY)
 			goto legacy_fallback;
-		return -errno;
+		return errno;
 	}
 
 	return 0;
@@ -240,7 +240,7 @@ xfrog_defragrange(
 legacy_fallback:
 	ret = xfrog_ioc_swapext(file2_fd, xdf);
 	if (ret)
-		return -errno;
+		return errno;
 
 	return 0;
 }
-- 
2.52.0

Re: [RFC PATCH 1/2] libfrog: make xfrog_defragrange return a positive valued

[Darrick J. Wong]

On Mon, Jan 19, 2026 at 03:26:50PM +0100, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> Currently, the only user for xfrog_defragrange is xfs_fsr's packfile(),
> which expects error to be a positive value.
> 
> Whenever xfrog_defragrange fails, the switch case always falls into the
> default clausule, making the error message pointless.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  libfrog/file_exchange.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libfrog/file_exchange.c b/libfrog/file_exchange.c
> index e6c3f486b0ff..31bbc6da60c3 100644
> --- a/libfrog/file_exchange.c
> +++ b/libfrog/file_exchange.c
> @@ -232,7 +232,7 @@ xfrog_defragrange(
>  	if (ret) {
>  		if (errno == EOPNOTSUPP || errno != ENOTTY)
>  			goto legacy_fallback;
> -		return -errno;
> +		return errno;

Hrmm.  If you're going to change the polarity of the error numbers (e.g.
negative to positive) then please update the comments.

That said, I'd prefer to keep the errno polarity the same at least
within a .c file ... even though libfrog is a mess of different error
number return strategies.  What if the callsite changed to:

	/* Swap the extents */
	error = -xfrog_defragrange(...);

and

	/* Snapshot file_fd before we start copying data... */
	error = -xfrog_defragrange_prep(...);

(and I guess io/exchrange.c also needs a fix)

	/* Snapshot the original file metadata in anticipation... */
	ret = -xfrog_commitrange_prep(...);

Hrm?

--D

>  	}
>  
>  	return 0;
> @@ -240,7 +240,7 @@ xfrog_defragrange(
>  legacy_fallback:
>  	ret = xfrog_ioc_swapext(file2_fd, xdf);
>  	if (ret)
> -		return -errno;
> +		return errno;
>  
>  	return 0;
>  }
> -- 
> 2.52.0
> 
> 

Re: [RFC PATCH 1/2] libfrog: make xfrog_defragrange return a positive valued

[Carlos Maiolino]

On Tue, Jan 20, 2026 at 09:20:39AM -0800, Darrick J. Wong wrote:
> On Mon, Jan 19, 2026 at 03:26:50PM +0100, cem@kernel.org wrote:
> > From: Carlos Maiolino <cem@kernel.org>
> >
> > Currently, the only user for xfrog_defragrange is xfs_fsr's packfile(),
> > which expects error to be a positive value.
> >
> > Whenever xfrog_defragrange fails, the switch case always falls into the
> > default clausule, making the error message pointless.
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  libfrog/file_exchange.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libfrog/file_exchange.c b/libfrog/file_exchange.c
> > index e6c3f486b0ff..31bbc6da60c3 100644
> > --- a/libfrog/file_exchange.c
> > +++ b/libfrog/file_exchange.c
> > @@ -232,7 +232,7 @@ xfrog_defragrange(
> >  	if (ret) {
> >  		if (errno == EOPNOTSUPP || errno != ENOTTY)
> >  			goto legacy_fallback;
> > -		return -errno;
> > +		return errno;
> 
> Hrmm.  If you're going to change the polarity of the error numbers (e.g.
> negative to positive) then please update the comments.

Sorry, I did it quickly and didn't even pay attention there were
comments :)

> 
> That said, I'd prefer to keep the errno polarity the same at least
> within a .c file ... even though libfrog is a mess of different error
> number return strategies.  What if the callsite changed to:
> 
> 	/* Swap the extents */
> 	error = -xfrog_defragrange(...);

This looks to just add more confusion to it IMHO, it's not 'easy' for me
at least to notice the minus sign in front of it.

What about just changing the switch case to catch for -ERROR instead of
their positive counterparts? At least we stop playing the error sign
change and just catch what we already have.


> 
> and
> 
> 	/* Snapshot file_fd before we start copying data... */
> 	error = -xfrog_defragrange_prep(...);
> 
> (and I guess io/exchrange.c also needs a fix)
> 
> 	/* Snapshot the original file metadata in anticipation... */
> 	ret = -xfrog_commitrange_prep(...);
> 
> Hrm?
> 
> --D
> 
> >  	}
> >
> >  	return 0;
> > @@ -240,7 +240,7 @@ xfrog_defragrange(
> >  legacy_fallback:
> >  	ret = xfrog_ioc_swapext(file2_fd, xdf);
> >  	if (ret)
> > -		return -errno;
> > +		return errno;
> >
> >  	return 0;
> >  }
> > --
> > 2.52.0
> >
> >
> 

Re: [RFC PATCH 1/2] libfrog: make xfrog_defragrange return a positive valued

[Darrick J. Wong]

On Wed, Jan 21, 2026 at 09:57:51AM +0100, Carlos Maiolino wrote:
> On Tue, Jan 20, 2026 at 09:20:39AM -0800, Darrick J. Wong wrote:
> > On Mon, Jan 19, 2026 at 03:26:50PM +0100, cem@kernel.org wrote:
> > > From: Carlos Maiolino <cem@kernel.org>
> > >
> > > Currently, the only user for xfrog_defragrange is xfs_fsr's packfile(),
> > > which expects error to be a positive value.
> > >
> > > Whenever xfrog_defragrange fails, the switch case always falls into the
> > > default clausule, making the error message pointless.
> > >
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > ---
> > >  libfrog/file_exchange.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libfrog/file_exchange.c b/libfrog/file_exchange.c
> > > index e6c3f486b0ff..31bbc6da60c3 100644
> > > --- a/libfrog/file_exchange.c
> > > +++ b/libfrog/file_exchange.c
> > > @@ -232,7 +232,7 @@ xfrog_defragrange(
> > >  	if (ret) {
> > >  		if (errno == EOPNOTSUPP || errno != ENOTTY)
> > >  			goto legacy_fallback;
> > > -		return -errno;
> > > +		return errno;
> > 
> > Hrmm.  If you're going to change the polarity of the error numbers (e.g.
> > negative to positive) then please update the comments.
> 
> Sorry, I did it quickly and didn't even pay attention there were
> comments :)
> 
> > 
> > That said, I'd prefer to keep the errno polarity the same at least
> > within a .c file ... even though libfrog is a mess of different error
> > number return strategies.  What if the callsite changed to:
> > 
> > 	/* Swap the extents */
> > 	error = -xfrog_defragrange(...);
> 
> This looks to just add more confusion to it IMHO, it's not 'easy' for me
> at least to notice the minus sign in front of it.
> 
> What about just changing the switch case to catch for -ERROR instead of
> their positive counterparts? At least we stop playing the error sign
> change and just catch what we already have.

Sure, do whatever you think is best.  This whole part of C is an
unfixable mess as far as I'm concerned.

--D

> 
> > 
> > and
> > 
> > 	/* Snapshot file_fd before we start copying data... */
> > 	error = -xfrog_defragrange_prep(...);
> > 
> > (and I guess io/exchrange.c also needs a fix)
> > 
> > 	/* Snapshot the original file metadata in anticipation... */
> > 	ret = -xfrog_commitrange_prep(...);
> > 
> > Hrm?
> > 
> > --D
> > 
> > >  	}
> > >
> > >  	return 0;
> > > @@ -240,7 +240,7 @@ xfrog_defragrange(
> > >  legacy_fallback:
> > >  	ret = xfrog_ioc_swapext(file2_fd, xdf);
> > >  	if (ret)
> > > -		return -errno;
> > > +		return errno;
> > >
> > >  	return 0;
> > >  }
> > > --
> > > 2.52.0
> > >
> > >
> > 
> 

[RFC PATCH 2/2] fsr: Always print error messages from xfrog_defragrange()

[cem]

From: Carlos Maiolino <cem@kernel.org>

Error messages when xfrog_defragrange() are only printed when
verbose/debug flages are used.

We had reports from users complaining it's hard to find out error
messages in the middle of dozens of other informational messages.

Particularly I think error messages are better to be printed
independently of verbose/debug flags, so unconditionally print those.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fsr/xfs_fsr.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 8845ff172fcb..fadb53af062d 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -1464,19 +1464,15 @@ packfile(
 		case 0:
 			break;
 	case ENOTSUP:
-		if (vflag || dflag)
-			fsrprintf(_("%s: file type not supported\n"), fname);
+		fsrprintf(_("%s: file type not supported\n"), fname);
 		break;
 	case EFAULT:
 		/* The file has changed since we started the copy */
-		if (vflag || dflag)
-			fsrprintf(_("%s: file modified defrag aborted\n"),
-					fname);
+		fsrprintf(_("%s: file modified defrag aborted\n"), fname);
 		break;
 	case EBUSY:
 		/* Timestamp has changed or mmap'ed file */
-		if (vflag || dflag)
-			fsrprintf(_("%s: file busy\n"), fname);
+		fsrprintf(_("%s: file busy\n"), fname);
 		break;
 	default:
 		fsrprintf(_("XFS_IOC_SWAPEXT failed: %s: %s\n"),
-- 
2.52.0

Re: [RFC PATCH 2/2] fsr: Always print error messages from xfrog_defragrange()

[Darrick J. Wong]

On Mon, Jan 19, 2026 at 03:26:51PM +0100, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> Error messages when xfrog_defragrange() are only printed when
> verbose/debug flages are used.
> 
> We had reports from users complaining it's hard to find out error
> messages in the middle of dozens of other informational messages.
> 
> Particularly I think error messages are better to be printed
> independently of verbose/debug flags, so unconditionally print those.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

That seems like a reasonable behavior to me.  On some level these
errors probably ought to be printed to stderr instead of stdout, but
nothing else in packfile() seems to do that so maybe we just leave that
alone?

Anyway silently dropping "did not actually defrag the file" errors by
default seems counterintuitive so

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fsr/xfs_fsr.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 8845ff172fcb..fadb53af062d 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -1464,19 +1464,15 @@ packfile(
>  		case 0:
>  			break;
>  	case ENOTSUP:
> -		if (vflag || dflag)
> -			fsrprintf(_("%s: file type not supported\n"), fname);
> +		fsrprintf(_("%s: file type not supported\n"), fname);
>  		break;
>  	case EFAULT:
>  		/* The file has changed since we started the copy */
> -		if (vflag || dflag)
> -			fsrprintf(_("%s: file modified defrag aborted\n"),
> -					fname);
> +		fsrprintf(_("%s: file modified defrag aborted\n"), fname);
>  		break;
>  	case EBUSY:
>  		/* Timestamp has changed or mmap'ed file */
> -		if (vflag || dflag)
> -			fsrprintf(_("%s: file busy\n"), fname);
> +		fsrprintf(_("%s: file busy\n"), fname);
>  		break;
>  	default:
>  		fsrprintf(_("XFS_IOC_SWAPEXT failed: %s: %s\n"),
> -- 
> 2.52.0
> 
>