[PATCH 0/2] xfs: Clear a couple of W=1 warnings

[PATCH 0/2] xfs: Clear a couple of W=1 warnings

[John Garry]

For configs XFS_DEBUG and XFS_WARN disabled, there are a couple of W=1
warnings for unused variables. I have attempted to clear these here by
simply marking the unused variables as __maybe_unused.

I suppose that another solution would be to change the ASSERT() statement
relevant to this config from:
#define ASSERT(expr) ((void)0)
to:
#define ASSERT(expr) ((void)(expr))

Then the problem is that some local variables need to lose the
#ifdef XFS_DEBUG guards, as those variables are now referenced in
ASSERT(). This means that the objcode grows ever so slightly. I
assume that this has all been considered previously...

Based on v6.9-rc5

John Garry (2):
  xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
  xfs: Clear W=1 warning in xfs_trans_unreserve_and_mod_sb():

 fs/xfs/xfs_iwalk.c | 2 +-
 fs/xfs/xfs_trans.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.31.1

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

[John Garry]

For CONFIG_XFS_DEBUG unset, xfs_iwalk_run_callbacks() generates the
following warning for when building with W=1:

fs/xfs/xfs_iwalk.c: In function ‘xfs_iwalk_run_callbacks’:
fs/xfs/xfs_iwalk.c:354:42: error: variable ‘irec’ set but not used [-Werror=unused-but-set-variable]
  354 |         struct xfs_inobt_rec_incore     *irec;
      |                                          ^~~~
cc1: all warnings being treated as errors

Mark irec as __maybe_unused to clear the warning.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_iwalk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
index 01b55f03a102..a92030611ff4 100644
--- a/fs/xfs/xfs_iwalk.c
+++ b/fs/xfs/xfs_iwalk.c
@@ -351,7 +351,7 @@ xfs_iwalk_run_callbacks(
 	int				*has_more)
 {
 	struct xfs_mount		*mp = iwag->mp;
-	struct xfs_inobt_rec_incore	*irec;
+	struct xfs_inobt_rec_incore __maybe_unused	*irec;
 	xfs_agino_t			next_agino;
 	int				error;
 
-- 
2.31.1

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

[Christoph Hellwig]

On Thu, Apr 25, 2024 at 12:08:45PM +0000, John Garry wrote:
> +	struct xfs_inobt_rec_incore __maybe_unused	*irec;

I've never seen code where __maybe_unused is the right answer, and this
is no exception.

Just remove this instance of irec, which also removes the variable
shadowing by the one inside the loop below.

diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
index 730c8d48da2827..86f14ec7c31fed 100644
--- a/fs/xfs/xfs_iwalk.c
+++ b/fs/xfs/xfs_iwalk.c
@@ -351,7 +351,6 @@ xfs_iwalk_run_callbacks(
 	int				*has_more)
 {
 	struct xfs_mount		*mp = iwag->mp;
-	struct xfs_inobt_rec_incore	*irec;
 	xfs_agino_t			next_agino;
 	int				error;
 
@@ -361,8 +360,8 @@ xfs_iwalk_run_callbacks(
 
 	/* Delete cursor but remember the last record we cached... */
 	xfs_iwalk_del_inobt(iwag->tp, curpp, agi_bpp, 0);
-	irec = &iwag->recs[iwag->nr_recs - 1];
-	ASSERT(next_agino >= irec->ir_startino + XFS_INODES_PER_CHUNK);
+	ASSERT(next_agino >= iwag->recs[iwag->nr_recs - 1].ir_startino +
+			XFS_INODES_PER_CHUNK);
 
 	if (iwag->drop_trans) {
 		xfs_trans_cancel(iwag->tp);

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

[John Garry]

On 25/04/2024 13:17, Christoph Hellwig wrote:
> On Thu, Apr 25, 2024 at 12:08:45PM +0000, John Garry wrote:
>> +	struct xfs_inobt_rec_incore __maybe_unused	*irec;
> 
> I've never seen code where __maybe_unused is the right answer, and this
> is no exception.

Then what about 9798f615ad2be?

> 
> Just remove this instance of irec, which also removes the variable
> shadowing by the one inside the loop below.
> 
> diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
> index 730c8d48da2827..86f14ec7c31fed 100644
> --- a/fs/xfs/xfs_iwalk.c
> +++ b/fs/xfs/xfs_iwalk.c
> @@ -351,7 +351,6 @@ xfs_iwalk_run_callbacks(
>   	int				*has_more)
>   {
>   	struct xfs_mount		*mp = iwag->mp;
> -	struct xfs_inobt_rec_incore	*irec;
>   	xfs_agino_t			next_agino;
>   	int				error;
>   
> @@ -361,8 +360,8 @@ xfs_iwalk_run_callbacks(
>   
>   	/* Delete cursor but remember the last record we cached... */
>   	xfs_iwalk_del_inobt(iwag->tp, curpp, agi_bpp, 0);
> -	irec = &iwag->recs[iwag->nr_recs - 1];
> -	ASSERT(next_agino >= irec->ir_startino + XFS_INODES_PER_CHUNK);
> +	ASSERT(next_agino >= iwag->recs[iwag->nr_recs - 1].ir_startino +
> +			XFS_INODES_PER_CHUNK);
>   

ok, that seems better.

>   	if (iwag->drop_trans) {
>   		xfs_trans_cancel(iwag->tp);

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

[Christoph Hellwig]

On Thu, Apr 25, 2024 at 02:24:28PM +0100, John Garry wrote:
> On 25/04/2024 13:17, Christoph Hellwig wrote:
> > On Thu, Apr 25, 2024 at 12:08:45PM +0000, John Garry wrote:
> > > +	struct xfs_inobt_rec_incore __maybe_unused	*irec;
> > 
> > I've never seen code where __maybe_unused is the right answer, and this
> > is no exception.
> 
> Then what about 9798f615ad2be?

Also not great and should be fixed.

(it also wasn't in the original patch and only got added working around
some debug warnings)

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

[John Garry]

On 25/04/2024 14:30, Christoph Hellwig wrote:
>>> I've never seen code where __maybe_unused is the right answer, and this
>>> is no exception.
>> Then what about 9798f615ad2be?
> Also not great and should be fixed.
> 
> (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.

However, there seems to be many more in -next now, but not for local 
variables..

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

[John Garry]

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

  	error = xfs_alloc_fixup_trees(acur->cnt, acur->bnolt, acur->rec_bno,
  				      acur->rec_len, acur->bno, acur->len, 0);
@@ -1217,7 +1217,7 @@ STATIC int			/* error */
  xfs_alloc_ag_vextent_exact(
  	xfs_alloc_arg_t	*args)	/* allocation argument structure */
  {
-	struct xfs_agf __maybe_unused *agf = args->agbp->b_addr;
+	struct xfs_buf	*agbp = args->agbp;
  	struct xfs_btree_cur *bno_cur;/* by block-number btree cursor */
  	struct xfs_btree_cur *cnt_cur;/* by count btree cursor */
  	int		error;
@@ -1234,8 +1234,7 @@ xfs_alloc_ag_vextent_exact(
  	/*
  	 * Allocate/initialize a cursor for the by-number freespace btree.
  	 */
-	bno_cur = xfs_bnobt_init_cursor(args->mp, args->tp, args->agbp,
-					  args->pag);
+	bno_cur = xfs_bnobt_init_cursor(args->mp, args->tp, agbp, args->pag);

  	/*
  	 * Lookup bno and minlen in the btree (minlen is irrelevant, really).
@@ -1295,9 +1294,9 @@ xfs_alloc_ag_vextent_exact(
  	 * We are allocating agbno for args->len
  	 * Allocate/initialize a cursor for the by-size btree.
  	 */
-	cnt_cur = xfs_cntbt_init_cursor(args->mp, args->tp, args->agbp,
-					args->pag);
-	ASSERT(args->agbno + args->len <= be32_to_cpu(agf->agf_length));
+	cnt_cur = xfs_cntbt_init_cursor(args->mp, args->tp, agbp, args->pag);
+	ASSERT(args->agbno + args->len <=
+		be32_to_cpu(((struct xfs_agf *)agbp->b_addr)->agf_length));
  	error = xfs_alloc_fixup_trees(cnt_cur, bno_cur, fbno, flen, args->agbno,
  				      args->len, XFSA_FIXUP_BNO_OK);
  	if (error) {
-- 
2.35.3


---

There's a few ways to improve this, like make xfs_buf.b_addr a union, 
but I am not sure if it is worth it.

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

[Darrick J. Wong]

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;

If you surround this declaration with #ifdef DEBUG, will the warning go
away...

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

...without the need for this?

--D

> 
>  	error = xfs_alloc_fixup_trees(acur->cnt, acur->bnolt, acur->rec_bno,
>  				      acur->rec_len, acur->bno, acur->len, 0);
> @@ -1217,7 +1217,7 @@ STATIC int			/* error */
>  xfs_alloc_ag_vextent_exact(
>  	xfs_alloc_arg_t	*args)	/* allocation argument structure */
>  {
> -	struct xfs_agf __maybe_unused *agf = args->agbp->b_addr;
> +	struct xfs_buf	*agbp = args->agbp;
>  	struct xfs_btree_cur *bno_cur;/* by block-number btree cursor */
>  	struct xfs_btree_cur *cnt_cur;/* by count btree cursor */
>  	int		error;
> @@ -1234,8 +1234,7 @@ xfs_alloc_ag_vextent_exact(
>  	/*
>  	 * Allocate/initialize a cursor for the by-number freespace btree.
>  	 */
> -	bno_cur = xfs_bnobt_init_cursor(args->mp, args->tp, args->agbp,
> -					  args->pag);
> +	bno_cur = xfs_bnobt_init_cursor(args->mp, args->tp, agbp, args->pag);
> 
>  	/*
>  	 * Lookup bno and minlen in the btree (minlen is irrelevant, really).
> @@ -1295,9 +1294,9 @@ xfs_alloc_ag_vextent_exact(
>  	 * We are allocating agbno for args->len
>  	 * Allocate/initialize a cursor for the by-size btree.
>  	 */
> -	cnt_cur = xfs_cntbt_init_cursor(args->mp, args->tp, args->agbp,
> -					args->pag);
> -	ASSERT(args->agbno + args->len <= be32_to_cpu(agf->agf_length));
> +	cnt_cur = xfs_cntbt_init_cursor(args->mp, args->tp, agbp, args->pag);
> +	ASSERT(args->agbno + args->len <=
> +		be32_to_cpu(((struct xfs_agf *)agbp->b_addr)->agf_length));
>  	error = xfs_alloc_fixup_trees(cnt_cur, bno_cur, fbno, flen, args->agbno,
>  				      args->len, XFSA_FIXUP_BNO_OK);
>  	if (error) {
> -- 
> 2.35.3
> 
> 
> ---
> 
> There's a few ways to improve this, like make xfs_buf.b_addr a union, but I
> am not sure if it is worth it.
> 
> 

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

[Dave Chinner]

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

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

[Christoph Hellwig]

On Fri, Apr 26, 2024 at 09:30:05AM +1000, Dave Chinner wrote:
> 	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...

Yupp.  That's what I mean with my original comment - once you start
thinking a little bigger there is usually a much better option than
these __maybe_unused hacks.

Using perag fields and helpers instead of raw buffer access would
also benefit a few other places in the allocator as well.

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

[John Garry]

On 26/04/2024 00:30, Dave Chinner wrote:
> 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...

Fine.

I'll send a single patch to convert both instances in xfs_alloc.c and 
tag you as "Originally-by" in the patch. Let me know if not ok.

Thanks,
John

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

[John Garry]

For CONFIG_XFS_DEBUG unset, xfs_trans_unreserve_and_mod_sb() generates the
following warning for when building with W=1:

  CC      fs/xfs/xfs_trans.o
fs/xfs/xfs_trans.c: In function ‘xfs_trans_unreserve_and_mod_sb’:
fs/xfs/xfs_trans.c:601:17: error: variable ‘error’ set but not used [-Werror=unused-but-set-variable]
  601 |         int     error;
      |                 ^~~~~
cc1: all warnings being treated as errors

Mark ret as __maybe_unused to clear the warning.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_trans.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 7350640059cc..46674a9b1c97 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -598,7 +598,7 @@ xfs_trans_unreserve_and_mod_sb(
 	int64_t			rtxdelta = 0;
 	int64_t			idelta = 0;
 	int64_t			ifreedelta = 0;
-	int			error;
+	int __maybe_unused	error;
 
 	/* calculate deltas */
 	if (tp->t_blk_res > 0)
-- 
2.31.1

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

[Christoph Hellwig]

> +	int __maybe_unused	error;

error is gone in for-next.

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

[John Garry]

On 25/04/2024 13:18, Christoph Hellwig wrote:
>> +	int __maybe_unused	error;
> 
> error is gone in for-next.
> 

ok, as long as this warning is acceptable for 6.9 - I assume what you 
reference is for 6.10