| Message ID | 20240425120846.707829-2-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | xfs: Clear a couple of W=1 warnings | expand |
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);
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);
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)
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..
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) {
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. > >
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.
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.
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
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;
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(-)