linux-kernel - Re: [PATCH] xfs: Remove unnecessary checks in functions related to xfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250519150854.GB9705@frogsfrogsfrogs>
Date: Mon, 19 May 2025 08:08:54 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Zizhi Wo <wozizhi@...wei.com>
Cc: cem@...nel.org, dchinner@...hat.com, osandov@...com,
	john.g.garry@...cle.com, linux-xfs@...r.kernel.org,
	linux-kernel@...r.kernel.org, yangerkun@...wei.com,
	leo.lilong@...wei.com
Subject: Re: [PATCH] xfs: Remove unnecessary checks in functions related to
 xfs_fsmap

On Sat, May 17, 2025 at 03:43:41PM +0800, Zizhi Wo wrote:
> From: Zizhi Wo <wozizhi@...weicloud.com>
> 
> In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
> to check the result of query_fn(), because there won't be another iteration
> of the loop anyway. Also, both before and after the change, info->group
> will eventually be set to NULL and the reference count of xfs_group will
> also be decremented before exiting the iteration.
> 
> The same logic applies to other similar functions as well, so related
> cleanup operations are performed together.
> 
> Signed-off-by: Zizhi Wo <wozizhi@...weicloud.com>
> Signed-off-by: Zizhi Wo <wozizhi@...wei.com>
> ---
>  fs/xfs/xfs_fsmap.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 414b27a86458..792282aa8a29 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
>  		if (pag_agno(pag) == end_ag) {
>  			info->last = true;
>  			error = query_fn(tp, info, &bt_cur, priv);
> -			if (error)
> -				break;

Removing these statements make the error path harder to follow.  Before,
it was explicit that an error breaks out of the loop body.  Now you have
to look upwards to the while loop conditional and reason about what
xfs_perag_next_range does when pag-> agno == end_ag to determine that
the loop terminates.

This also leaves a tripping point for anyone who wants to add another
statement into this here if body because now they have to recognize that
they need to re-add the "if (error) break;" statements that you're now
taking out.

You also don't claim any reduction in generated machine code size or
execution speed, which means all the programmers end up having to
perform extra reasoning when reading this code for ... what?  Zero gain?

Please stop sending overly narrowly focused "optimizations" that make
life harder for all of us.

NAK.

--D

>  		}
>  		info->group = NULL;
>  	}
> @@ -813,8 +811,6 @@ xfs_getfsmap_rtdev_rtbitmap(
>  			info->last = true;
>  			error = xfs_getfsmap_rtdev_rtbitmap_helper(rtg, tp,
>  					&ahigh, info);
> -			if (error)
> -				break;
>  		}
>  
>  		xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_BITMAP_SHARED);
> @@ -1018,8 +1014,6 @@ xfs_getfsmap_rtdev_rmapbt(
>  			info->last = true;
>  			error = xfs_getfsmap_rtdev_rmapbt_helper(bt_cur,
>  					&info->high, info);
> -			if (error)
> -				break;
>  		}
>  		info->group = NULL;
>  	}
> -- 
> 2.39.2
> 
>