Re: [PATCH 9/9] spaceman/defrag: warn on extsize

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jul 09, 2024 at 12:10:28PM -0700, Wengang Wang wrote:
> According to current kernel implemenation, non-zero extsize might affect
> the result of defragmentation.
> Just print a warning on that if non-zero extsize is set on file.

I'm not sure what's the point of warning vaguely about extent size
hints?  I'd have thought that would help reduce the number of extents;
is that not the case?

> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  spaceman/defrag.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/spaceman/defrag.c b/spaceman/defrag.c
> index ab8508bb..b6b89dd9 100644
> --- a/spaceman/defrag.c
> +++ b/spaceman/defrag.c
> @@ -526,6 +526,18 @@ defrag_xfs_defrag(char *file_path) {
>  		goto out;
>  	}
>  
> +       if (ioctl(defrag_fd, FS_IOC_FSGETXATTR, &fsx) < 0) {
> +               fprintf(stderr, "FSGETXATTR failed %s\n",
> +                       strerror(errno));

Also we usually indent continuations by two tabs (not one) so that the
continuation is more obvious:

		fprintf(stderr, "FSGETXATTR failed %s\n",
				strerror(errno));

> +               ret = 1;
> +               goto out;
> +       }
> +
> +       if (fsx.fsx_extsize != 0)
> +               fprintf(stderr, "%s has extsize set %d. That might affect defrag "
> +                       "according to kernel implementation\n",

Format strings in userspace printf calls should be wrapped so that
gettext can provide translated versions:

	fprintf(stderr, _("%s has extsize...\n"), file_path...);

(I know, xfsprogs isn't as consistent as it probably ought to be...)

--D

> +                       file_path, fsx.fsx_extsize);
> +
>  	clone.src_fd = defrag_fd;
>  
>  	defrag_dir = dirname(file_path);
> -- 
> 2.39.3 (Apple Git-146)
> 
>