linux-kernel - Re: [PATCH v2] xfs: Use for_each_perag_from() to iterate all available AGs

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230410163029.GC360889@frogsfrogsfrogs>
Date:   Mon, 10 Apr 2023 09:30:29 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     Ryosuke Yasuoka <ryasuoka@...hat.com>
Cc:     linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        sandeen@...deen.net, david@...morbit.com
Subject: Re: [PATCH v2] xfs: Use for_each_perag_from() to iterate all
 available AGs

On Tue, Apr 11, 2023 at 01:07:27AM +0900, Ryosuke Yasuoka wrote:
> xfs_filestream_pick_ag() iterates all the available AGs when no
> unassociated AGs are available by using for_each_perag_wrap().
> To iterate all the available AGs, just use for_each_perag_from() instead.
> 
> 
> This patch cleans up a code where xfs_filestream_pick_ag() iterates 
> all the available AGs when no unassociated AGs are available.
> Current implementation is using a for_each_perag_wrap() macro which
> iterates all AGs from start_agno through wrap_agno, wraps to
> restart_agno, and then iterates again toward to (start_agno - 1).
> In this case, xfs_filestream_pick_ag() start to iterate from 0 and
> does't need to wrap. Although passing 0 as start_agno to
> for_each_perag_wrap() 
> is not problematic, we have already a for_each_perag() macro family
> which just iterates all AGs from 0 and doesn't wrap. Hense, I propose
> to use for_each_perag() family simply.
> 
> 
> Changes since v1 [1]:
> Use for_each_perag_from() instead of for_each_perag() to clarify
> where we are iterating from.
> 
> [1]:
> https://lore.kernel.org/linux-xfs/CAHpthZrvhqh8O1HO7U_jVnaq9R9Ur=Yq2eWzjWfNx3ryDbnGPA@mail.gmail.com/T/#m5704d0409bec1ce5273be0d3860e8ad60e9886fd
> 
> Signed-off-by: Ryosuke Yasuoka <ryasuoka@...hat.com>
> ---
>  fs/xfs/xfs_filestream.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index 22c13933c8f8..29acd9f7d422 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -151,7 +151,8 @@ xfs_filestream_pick_ag(
>  		 * grab.
>  		 */
>  		if (!max_pag) {
> -			for_each_perag_wrap(args->mp, 0, start_agno, args->pag)
> +			start_agno = 0;
> +			for_each_perag_from(args->mp, start_agno, args->pag)

IDGI.  for_each_perag initializes the loop variable and calls
for_each_perag_from, so this is open-coding an existing macro.

If people are confused by the reuse of the function call parameter
variable for the second loop, then either declare a new variable and let
the compiler notice that we never use start_agno ever again and reuse
a cpu register:

	if (!max_pag) {
		xfs_agnumber_t	agno;

		for_each_perag(args->mp, agno, args->pag)
			break;
		...
	}

Or reuse it explicitly and leave a comment:

	if (!max_pag) {
		/*
		 * Use any AG that we can grab.  start_agno is no longer
		 * pertinent here so we can reuse the variable.
		 */
		for_each_perag(args->mp, start_agno, args->pag)
			break;
		...
	}

As a third alternative, I suppose you could encapsulate all of that into
a dorky helper since I bet this isn't the first or the last time we're
going to need something like this:

static inline struct xfs_perag *
xfs_perag_get_first_avail(
	struct xfs_mount	*mp)
{
	struct xfs_perag	*pag;
	xfs_agnumber_t		agno;

	for_each_perag(mp, agno, pag)
		return pag;

	ASSERT(0);
	return NULL;
}

	if (!max_pag) {
		args->pag = xfs_perag_get_first_avail(mp);
		...
	}

--D

>  				break;
>  			atomic_inc(&args->pag->pagf_fstrms);
>  			*longest = 0;
> -- 
> 2.39.2
>