Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():

[Dave Chinner <david@fromorbit.com>]

On Thu, Apr 25, 2024 at 04:37:25PM +0100, John Garry wrote:
> On 25/04/2024 14:33, John Garry wrote:
> > > 
> > > (it also wasn't in the original patch and only got added working around
> > > some debug warnings)
> > 
> > Fine, I'll look to remove those ones as well, which I think is possible
> > with the same method you suggest.
> 
> It's a bit messy, as xfs_buf.b_addr is a void *:
> 
> From 1181afdac3d61b79813381d308b9ab2ebe30abca Mon Sep 17 00:00:00 2001
> From: John Garry <john.g.garry@oracle.com>
> Date: Thu, 25 Apr 2024 16:23:49 +0100
> Subject: [PATCH] xfs: Stop using __maybe_unused in xfs_alloc.c
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 9da52e92172a..5d84a97b4971 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -1008,13 +1008,13 @@ xfs_alloc_cur_finish(
>  	struct xfs_alloc_arg	*args,
>  	struct xfs_alloc_cur	*acur)
>  {
> -	struct xfs_agf __maybe_unused *agf = args->agbp->b_addr;
>  	int			error;
> 
>  	ASSERT(acur->cnt && acur->bnolt);
>  	ASSERT(acur->bno >= acur->rec_bno);
>  	ASSERT(acur->bno + acur->len <= acur->rec_bno + acur->rec_len);
> -	ASSERT(acur->rec_bno + acur->rec_len <= be32_to_cpu(agf->agf_length));
> +	ASSERT(acur->rec_bno + acur->rec_len <=
> +		be32_to_cpu(((struct xfs_agf *)args->agbp->b_addr)->agf_length));

Please think about what the code is actually doing and our data
structures a little more deeply - this is can be fixed in a much
better way than doing a mechanical code change.

agf->agf_length is what, exactly?

	It's an on-disk constant for the AG size held in the AGF.

What is this ASSERT check doing?

	It is verifying the agbno of the end of the extent is
	within valid bounds.

Do we have a pre-computed in memory constant for this on disk
value?

	Yes, we do: pag->block_count

Do we have a function to verify an agbno is within valid bounds of
the AG using these in-memory constants?

	Yes, we do: xfs_verify_agbno().

Do we have a function to verify an extent is within the valid bounds
of the AG using these in-memory constants?

	Yes, we do: xfs_verify_agbext()

Can this be written differently that has no need to access the
on-disk AGF at all?

	Yes, it can:

	ASSERT(xfs_verify_agbno(args->pag, acur->rec_bno + acur->rec_len));

	or:

	ASSERT(xfs_verify_agbext(args->pag, acur->rec_bno, acur->rec_len));

The latter is better, as it verifies both the start and the end of
the extent are within the bounds of the AG and catches overflows...

-Dave.
-- 
Dave Chinner
david@fromorbit.com