[PATCH 1/2] xfsprogs: xfs_fsr: Interpret arguments of qsort's compare function correctly

[PATCH 1/2] xfsprogs: xfs_fsr: Interpret arguments of qsort's compare function correctly

[Chandan Babu R]

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);
 }
 
 /*
-- 
2.29.2

[PATCH 2/2] xfsprogs: xfs_fsr: Limit the scope of cmp()

[Chandan Babu R]

cmp() function is being referred to by only from within fsr/xfs_fsr.c. Hence
this commit limits its scope to the current file.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fsr/xfs_fsr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 635e4c70..2d070320 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -81,7 +81,7 @@ char * gettmpname(char *fname);
 char * getparent(char *fname);
 int fsrprintf(const char *fmt, ...);
 int read_fd_bmap(int, struct xfs_bstat *, int *);
-int cmp(const void *, const void *);
+static int cmp(const void *, const void *);
 static void tmp_init(char *mnt);
 static char * tmp_next(char *mnt);
 static void tmp_close(char *mnt);
@@ -699,7 +699,7 @@ out0:
 /*
  * To compare bstat structs for qsort.
  */
-int
+static int
 cmp(const void *s1, const void *s2)
 {
 	return( ((struct xfs_bulkstat *)s2)->bs_extents -
-- 
2.29.2

Re: [PATCH 1/2] xfsprogs: xfs_fsr: Interpret arguments of qsort's compare function correctly

[Eric Sandeen]

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);
>  }
>  
>  /*
> 

Re: [PATCH 2/2] xfsprogs: xfs_fsr: Limit the scope of cmp()

[Eric Sandeen]

On 1/25/21 3:58 AM, Chandan Babu R wrote:
> cmp() function is being referred to by only from within fsr/xfs_fsr.c. Hence
> this commit limits its scope to the current file.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>

Might be nice to move cmp just ahead of fsrfs() so we don't need the
forward declaration but *shrug*

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fsr/xfs_fsr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 635e4c70..2d070320 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -81,7 +81,7 @@ char * gettmpname(char *fname);
>  char * getparent(char *fname);
>  int fsrprintf(const char *fmt, ...);
>  int read_fd_bmap(int, struct xfs_bstat *, int *);
> -int cmp(const void *, const void *);
> +static int cmp(const void *, const void *);
>  static void tmp_init(char *mnt);
>  static char * tmp_next(char *mnt);
>  static void tmp_close(char *mnt);
> @@ -699,7 +699,7 @@ out0:
>  /*
>   * To compare bstat structs for qsort.
>   */
> -int
> +static int
>  cmp(const void *s1, const void *s2)
>  {
>  	return( ((struct xfs_bulkstat *)s2)->bs_extents -
> 

[PATCH V1.1] xfsprogs: xfs_fsr: Limit the scope of cmp()

[Chandan Babu R]

cmp() function is being referred to from within fsr/xfs_fsr.c. Hence
this commit limits its scope to the current file.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fsr/xfs_fsr.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 635e4c70..b885395e 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -81,7 +81,6 @@ char * gettmpname(char *fname);
 char * getparent(char *fname);
 int fsrprintf(const char *fmt, ...);
 int read_fd_bmap(int, struct xfs_bstat *, int *);
-int cmp(const void *, const void *);
 static void tmp_init(char *mnt);
 static char * tmp_next(char *mnt);
 static void tmp_close(char *mnt);
@@ -577,6 +576,16 @@ fsrall_cleanup(int timeout)
 	}
 }
 
+/*
+ * To compare bstat structs for qsort.
+ */
+static int
+cmp(const void *s1, const void *s2)
+{
+	return( ((struct xfs_bulkstat *)s2)->bs_extents -
+	        ((struct xfs_bulkstat *)s1)->bs_extents);
+}
+
 /*
  * fsrfs -- reorganize a file system
  */
@@ -696,16 +705,6 @@ out0:
 	return 0;
 }
 
-/*
- * To compare bstat structs for qsort.
- */
-int
-cmp(const void *s1, const void *s2)
-{
-	return( ((struct xfs_bulkstat *)s2)->bs_extents -
-	        ((struct xfs_bulkstat *)s1)->bs_extents);
-}
-
 /*
  * reorganize by directory hierarchy.
  * Stay in dev (a restriction based on structure of this program -- either
-- 
2.29.2

Re: [PATCH 1/2] xfsprogs: xfs_fsr: Interpret arguments of qsort's compare function correctly

[Chandan Babu R]

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

Re: [PATCH 1/2] xfsprogs: xfs_fsr: Interpret arguments of qsort's compare function correctly

[Darrick J. Wong]

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
> 

Re: [PATCH 1/2] xfsprogs: xfs_fsr: Interpret arguments of qsort's compare function correctly

[Chandan Babu R]

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>
>

-- 
chandan

Re: [PATCH V1.1] xfsprogs: xfs_fsr: Limit the scope of cmp()

[Eric Sandeen]

On 1/25/21 10:42 PM, Chandan Babu R wrote:
> cmp() function is being referred to from within fsr/xfs_fsr.c. Hence
> this commit limits its scope to the current file.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>

Thanks,

Reviewed-by: Eric Sandeen <sandeen@redhat.com>