> On Jul 9, 2024, at 1:21 PM, Darrick J. Wong <djwong@xxxxxxxxxx> wrote: > > 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? Not exactly. Same 1G file with about 54K extents, The one with 16K extsize, after defrag, it’s extents drops to 13K. And the one with 0 extsize, after defrag, it’s extents dropped to 22. Above is tested with UEK6 (5.4 kernel). I can get the numbers on mainline if you want. > >> Signed-off-by: Wengang Wang <wen.gang.wang@xxxxxxxxxx> >> --- >> 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)); OK. > >> + 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...) > OK. Thanks, Wengang > --D > >> + file_path, fsx.fsx_extsize); >> + >> clone.src_fd = defrag_fd; >> >> defrag_dir = dirname(file_path); >> -- >> 2.39.3 (Apple Git-146)