| Message ID | 20210125095809.219833-1-chandanrlinux@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/2] xfsprogs: xfs_fsr: Interpret arguments of qsort's compare function correctly | expand |
On 1/25/21 3:58 AM, Chandan Babu R wrote: > The first argument passed to qsort() in fsrfs() is an array of "struct > xfs_bulkstat". Hence the two arguments to the cmp() function must be > interpreted as being of type "struct xfs_bulkstat *" as against "struct > xfs_bstat *" that is being used to currently typecast them. > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> Yikes. Broken since 5.3.0, and the structures have different sizes and different bs_extents offsets. :( Fixes: 4cca629d6 ("misc: convert xfrog_bulkstat functions to have v5 semantics") Reviewed-by: Eric Sandeen <sandeen@redhat.com> At least it's only affecting the whole-fs defragment which is generally not recommended, but is still available and does get used. I wonder if it explains this bug report: Jan 07 20:52:44 <Tharn> hey, quick question... the first time I ran xfs_fsr last night, it ran for 2 hours and looking at the console log, it ended with a lot of "start inode=0" repeating > --- > fsr/xfs_fsr.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c > index 77a10a1d..635e4c70 100644 > --- a/fsr/xfs_fsr.c > +++ b/fsr/xfs_fsr.c > @@ -702,9 +702,8 @@ out0: > int > cmp(const void *s1, const void *s2) > { > - return( ((struct xfs_bstat *)s2)->bs_extents - > - ((struct xfs_bstat *)s1)->bs_extents); > - > + return( ((struct xfs_bulkstat *)s2)->bs_extents - > + ((struct xfs_bulkstat *)s1)->bs_extents); > } > > /* >
On 25 Jan 2021 at 19:47, Eric Sandeen wrote: > On 1/25/21 3:58 AM, Chandan Babu R wrote: >> The first argument passed to qsort() in fsrfs() is an array of "struct >> xfs_bulkstat". Hence the two arguments to the cmp() function must be >> interpreted as being of type "struct xfs_bulkstat *" as against "struct >> xfs_bstat *" that is being used to currently typecast them. >> >> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> > > Yikes. Broken since 5.3.0, and the structures have different sizes and > different bs_extents offsets. :( > > Fixes: 4cca629d6 ("misc: convert xfrog_bulkstat functions to have v5 semantics") > Reviewed-by: Eric Sandeen <sandeen@redhat.com> > > At least it's only affecting the whole-fs defragment which is generally not > recommended, but is still available and does get used. > > I wonder if it explains this bug report: > > Jan 07 20:52:44 <Tharn> hey, quick question... the first time I ran xfs_fsr last night, it ran for 2 hours and looking at the console log, it ended with a lot of "start inode=0" repeating With my limited understanding of xfs_fsr code, looks like the bug reporter saw the logs corresponding to the default (i.e. 10) number of passes that xfs_fsr executes on the inodes of the filesystem. Sorry, I couldn't be of much help in this case. > >> --- >> fsr/xfs_fsr.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c >> index 77a10a1d..635e4c70 100644 >> --- a/fsr/xfs_fsr.c >> +++ b/fsr/xfs_fsr.c >> @@ -702,9 +702,8 @@ out0: >> int >> cmp(const void *s1, const void *s2) >> { >> - return( ((struct xfs_bstat *)s2)->bs_extents - >> - ((struct xfs_bstat *)s1)->bs_extents); >> - >> + return( ((struct xfs_bulkstat *)s2)->bs_extents - >> + ((struct xfs_bulkstat *)s1)->bs_extents); >> } >> >> /* >> -- chandan
On Mon, Jan 25, 2021 at 03:28:08PM +0530, Chandan Babu R wrote: > The first argument passed to qsort() in fsrfs() is an array of "struct > xfs_bulkstat". Hence the two arguments to the cmp() function must be > interpreted as being of type "struct xfs_bulkstat *" as against "struct > xfs_bstat *" that is being used to currently typecast them. > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> > --- > fsr/xfs_fsr.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c > index 77a10a1d..635e4c70 100644 > --- a/fsr/xfs_fsr.c > +++ b/fsr/xfs_fsr.c > @@ -702,9 +702,8 @@ out0: > int > cmp(const void *s1, const void *s2) > { > - return( ((struct xfs_bstat *)s2)->bs_extents - > - ((struct xfs_bstat *)s1)->bs_extents); > - > + return( ((struct xfs_bulkstat *)s2)->bs_extents - > + ((struct xfs_bulkstat *)s1)->bs_extents); It might be a good idea to check bs_version here to avoid future maintainer screwups <coughs this maintainer> Thanks for catching this, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > } > > /* > -- > 2.29.2 >
On 26 Jan 2021 at 23:28, Darrick J. Wong wrote: > On Mon, Jan 25, 2021 at 03:28:08PM +0530, Chandan Babu R wrote: >> The first argument passed to qsort() in fsrfs() is an array of "struct >> xfs_bulkstat". Hence the two arguments to the cmp() function must be >> interpreted as being of type "struct xfs_bulkstat *" as against "struct >> xfs_bstat *" that is being used to currently typecast them. >> >> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> >> --- >> fsr/xfs_fsr.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c >> index 77a10a1d..635e4c70 100644 >> --- a/fsr/xfs_fsr.c >> +++ b/fsr/xfs_fsr.c >> @@ -702,9 +702,8 @@ out0: >> int >> cmp(const void *s1, const void *s2) >> { >> - return( ((struct xfs_bstat *)s2)->bs_extents - >> - ((struct xfs_bstat *)s1)->bs_extents); >> - >> + return( ((struct xfs_bulkstat *)s2)->bs_extents - >> + ((struct xfs_bulkstat *)s1)->bs_extents); > > It might be a good idea to check bs_version here to avoid future > maintainer screwups <coughs this maintainer> > Sure, I will write a new patch to add that. > Thanks for catching this, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> >
diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c index 77a10a1d..635e4c70 100644 --- a/fsr/xfs_fsr.c +++ b/fsr/xfs_fsr.c @@ -702,9 +702,8 @@ out0: int cmp(const void *s1, const void *s2) { - return( ((struct xfs_bstat *)s2)->bs_extents - - ((struct xfs_bstat *)s1)->bs_extents); - + return( ((struct xfs_bulkstat *)s2)->bs_extents - + ((struct xfs_bulkstat *)s1)->bs_extents); } /*
The first argument passed to qsort() in fsrfs() is an array of "struct xfs_bulkstat". Hence the two arguments to the cmp() function must be interpreted as being of type "struct xfs_bulkstat *" as against "struct xfs_bstat *" that is being used to currently typecast them. Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> --- fsr/xfs_fsr.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)