diff mbox series

[2/3] xfs: No need for inode number error injection in __xfs_dir3_data_check

Message ID 20210223054748.3292734-3-david@fromorbit.com (mailing list archive)
State Superseded
Headers show
Series xfs: 64kb directory block verification hurts | expand

Commit Message

Dave Chinner Feb. 23, 2021, 5:47 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

We call xfs_dir_ino_validate() for every dir entry in a directory
when doing validity checking of the directory. It calls
xfs_verify_dir_ino() then emits a corruption report if bad or does
error injection if good. It is extremely costly:

  43.27%  [kernel]  [k] xfs_dir3_leaf_check_int
  10.28%  [kernel]  [k] __xfs_dir3_data_check
   6.61%  [kernel]  [k] xfs_verify_dir_ino
   4.16%  [kernel]  [k] xfs_errortag_test
   4.00%  [kernel]  [k] memcpy
   3.48%  [kernel]  [k] xfs_dir_ino_validate

7% of the cpu usage in this directory traversal workload is
xfs_dir_ino_validate() doing absolutely nothing.

We don't need error injection to simulate a bad inode numbers in the
directory structure because we can do that by fuzzing the structure
on disk.

And we don't need a corruption report, because the
__xfs_dir3_data_check() will emit one if the inode number is bad.

So just call xfs_verify_dir_ino() directly here, and get rid of all
this unnecessary overhead:

  40.30%  [kernel]  [k] xfs_dir3_leaf_check_int
  10.98%  [kernel]  [k] __xfs_dir3_data_check
   8.10%  [kernel]  [k] xfs_verify_dir_ino
   4.42%  [kernel]  [k] memcpy
   2.22%  [kernel]  [k] xfs_dir2_data_get_ftype
   1.52%  [kernel]  [k] do_raw_spin_lock

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_dir2_data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Darrick J. Wong Feb. 24, 2021, 9:47 p.m. UTC | #1
On Tue, Feb 23, 2021 at 04:47:47PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We call xfs_dir_ino_validate() for every dir entry in a directory
> when doing validity checking of the directory. It calls
> xfs_verify_dir_ino() then emits a corruption report if bad or does
> error injection if good. It is extremely costly:
> 
>   43.27%  [kernel]  [k] xfs_dir3_leaf_check_int
>   10.28%  [kernel]  [k] __xfs_dir3_data_check
>    6.61%  [kernel]  [k] xfs_verify_dir_ino
>    4.16%  [kernel]  [k] xfs_errortag_test
>    4.00%  [kernel]  [k] memcpy
>    3.48%  [kernel]  [k] xfs_dir_ino_validate
> 
> 7% of the cpu usage in this directory traversal workload is
> xfs_dir_ino_validate() doing absolutely nothing.
> 
> We don't need error injection to simulate a bad inode numbers in the
> directory structure because we can do that by fuzzing the structure
> on disk.
> 
> And we don't need a corruption report, because the
> __xfs_dir3_data_check() will emit one if the inode number is bad.
> 
> So just call xfs_verify_dir_ino() directly here, and get rid of all
> this unnecessary overhead:
> 
>   40.30%  [kernel]  [k] xfs_dir3_leaf_check_int
>   10.98%  [kernel]  [k] __xfs_dir3_data_check
>    8.10%  [kernel]  [k] xfs_verify_dir_ino
>    4.42%  [kernel]  [k] memcpy
>    2.22%  [kernel]  [k] xfs_dir2_data_get_ftype
>    1.52%  [kernel]  [k] do_raw_spin_lock
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Heh, yeah, this one has filled up the fuzzer test results for a while...

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

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_data.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index 375b3edb2ad2..e67fa086f2c1 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -218,7 +218,7 @@ __xfs_dir3_data_check(
>  		 */
>  		if (dep->namelen == 0)
>  			return __this_address;
> -		if (xfs_dir_ino_validate(mp, be64_to_cpu(dep->inumber)))
> +		if (!xfs_verify_dir_ino(mp, be64_to_cpu(dep->inumber)))
>  			return __this_address;
>  		if (offset + xfs_dir2_data_entsize(mp, dep->namelen) > end)
>  			return __this_address;
> -- 
> 2.28.0
>
Christoph Hellwig Feb. 25, 2021, 9:06 a.m. UTC | #2
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

On Tue, Feb 23, 2021 at 04:47:47PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We call xfs_dir_ino_validate() for every dir entry in a directory
> when doing validity checking of the directory. It calls
> xfs_verify_dir_ino() then emits a corruption report if bad or does
> error injection if good. It is extremely costly:
> 
>   43.27%  [kernel]  [k] xfs_dir3_leaf_check_int
>   10.28%  [kernel]  [k] __xfs_dir3_data_check
>    6.61%  [kernel]  [k] xfs_verify_dir_ino
>    4.16%  [kernel]  [k] xfs_errortag_test
>    4.00%  [kernel]  [k] memcpy
>    3.48%  [kernel]  [k] xfs_dir_ino_validate
> 
> 7% of the cpu usage in this directory traversal workload is
> xfs_dir_ino_validate() doing absolutely nothing.
> 
> We don't need error injection to simulate a bad inode numbers in the
> directory structure because we can do that by fuzzing the structure
> on disk.
> 
> And we don't need a corruption report, because the
> __xfs_dir3_data_check() will emit one if the inode number is bad.
> 
> So just call xfs_verify_dir_ino() directly here, and get rid of all
> this unnecessary overhead:
> 
>   40.30%  [kernel]  [k] xfs_dir3_leaf_check_int
>   10.98%  [kernel]  [k] __xfs_dir3_data_check
>    8.10%  [kernel]  [k] xfs_verify_dir_ino
>    4.42%  [kernel]  [k] memcpy
>    2.22%  [kernel]  [k] xfs_dir2_data_get_ftype
>    1.52%  [kernel]  [k] do_raw_spin_lock
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_dir2_data.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index 375b3edb2ad2..e67fa086f2c1 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -218,7 +218,7 @@ __xfs_dir3_data_check(
>  		 */
>  		if (dep->namelen == 0)
>  			return __this_address;
> -		if (xfs_dir_ino_validate(mp, be64_to_cpu(dep->inumber)))
> +		if (!xfs_verify_dir_ino(mp, be64_to_cpu(dep->inumber)))
>  			return __this_address;
>  		if (offset + xfs_dir2_data_entsize(mp, dep->namelen) > end)
>  			return __this_address;
> -- 
> 2.28.0
> 
---end quoted text---
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index 375b3edb2ad2..e67fa086f2c1 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -218,7 +218,7 @@  __xfs_dir3_data_check(
 		 */
 		if (dep->namelen == 0)
 			return __this_address;
-		if (xfs_dir_ino_validate(mp, be64_to_cpu(dep->inumber)))
+		if (!xfs_verify_dir_ino(mp, be64_to_cpu(dep->inumber)))
 			return __this_address;
 		if (offset + xfs_dir2_data_entsize(mp, dep->namelen) > end)
 			return __this_address;