* [PATCH 0/2] xfs: Clear a couple of W=1 warnings
@ 2024-04-25 12:08 John Garry
2024-04-25 12:08 ` [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks(): John Garry
2024-04-25 12:08 ` [PATCH 2/2] xfs: Clear W=1 warning in xfs_trans_unreserve_and_mod_sb(): John Garry
0 siblings, 2 replies; 14+ messages in thread
From: John Garry @ 2024-04-25 12:08 UTC (permalink / raw
To: chandan.babu, djwong; +Cc: linux-xfs, 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 12:08 [PATCH 0/2] xfs: Clear a couple of W=1 warnings John Garry
@ 2024-04-25 12:08 ` John Garry
2024-04-25 12:17 ` Christoph Hellwig
2024-04-25 12:08 ` [PATCH 2/2] xfs: Clear W=1 warning in xfs_trans_unreserve_and_mod_sb(): John Garry
1 sibling, 1 reply; 14+ messages in thread
From: John Garry @ 2024-04-25 12:08 UTC (permalink / raw
To: chandan.babu, djwong; +Cc: linux-xfs, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] xfs: Clear W=1 warning in xfs_trans_unreserve_and_mod_sb():
2024-04-25 12:08 [PATCH 0/2] xfs: Clear a couple of W=1 warnings John Garry
2024-04-25 12:08 ` [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks(): John Garry
@ 2024-04-25 12:08 ` John Garry
2024-04-25 12:18 ` Christoph Hellwig
1 sibling, 1 reply; 14+ messages in thread
From: John Garry @ 2024-04-25 12:08 UTC (permalink / raw
To: chandan.babu, djwong; +Cc: linux-xfs, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 12:08 ` [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks(): John Garry
@ 2024-04-25 12:17 ` Christoph Hellwig
2024-04-25 13:24 ` John Garry
0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2024-04-25 12:17 UTC (permalink / raw
To: John Garry; +Cc: chandan.babu, djwong, linux-xfs
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);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xfs: Clear W=1 warning in xfs_trans_unreserve_and_mod_sb():
2024-04-25 12:08 ` [PATCH 2/2] xfs: Clear W=1 warning in xfs_trans_unreserve_and_mod_sb(): John Garry
@ 2024-04-25 12:18 ` Christoph Hellwig
2024-04-25 13:35 ` John Garry
0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2024-04-25 12:18 UTC (permalink / raw
To: John Garry; +Cc: chandan.babu, djwong, linux-xfs
> + int __maybe_unused error;
error is gone in for-next.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 12:17 ` Christoph Hellwig
@ 2024-04-25 13:24 ` John Garry
2024-04-25 13:30 ` Christoph Hellwig
0 siblings, 1 reply; 14+ messages in thread
From: John Garry @ 2024-04-25 13:24 UTC (permalink / raw
To: Christoph Hellwig; +Cc: chandan.babu, djwong, linux-xfs
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);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 13:24 ` John Garry
@ 2024-04-25 13:30 ` Christoph Hellwig
2024-04-25 13:33 ` John Garry
0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2024-04-25 13:30 UTC (permalink / raw
To: John Garry; +Cc: Christoph Hellwig, chandan.babu, djwong, linux-xfs
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 13:30 ` Christoph Hellwig
@ 2024-04-25 13:33 ` John Garry
2024-04-25 15:37 ` John Garry
0 siblings, 1 reply; 14+ messages in thread
From: John Garry @ 2024-04-25 13:33 UTC (permalink / raw
To: Christoph Hellwig; +Cc: chandan.babu, djwong, linux-xfs
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..
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xfs: Clear W=1 warning in xfs_trans_unreserve_and_mod_sb():
2024-04-25 12:18 ` Christoph Hellwig
@ 2024-04-25 13:35 ` John Garry
0 siblings, 0 replies; 14+ messages in thread
From: John Garry @ 2024-04-25 13:35 UTC (permalink / raw
To: Christoph Hellwig; +Cc: chandan.babu, djwong, linux-xfs
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 13:33 ` John Garry
@ 2024-04-25 15:37 ` John Garry
2024-04-25 16:17 ` Darrick J. Wong
2024-04-25 23:30 ` Dave Chinner
0 siblings, 2 replies; 14+ messages in thread
From: John Garry @ 2024-04-25 15:37 UTC (permalink / raw
To: Christoph Hellwig; +Cc: chandan.babu, djwong, linux-xfs
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.
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 15:37 ` John Garry
@ 2024-04-25 16:17 ` Darrick J. Wong
2024-04-25 23:30 ` Dave Chinner
1 sibling, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2024-04-25 16:17 UTC (permalink / raw
To: John Garry; +Cc: Christoph Hellwig, chandan.babu, linux-xfs
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.
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 15:37 ` John Garry
2024-04-25 16:17 ` Darrick J. Wong
@ 2024-04-25 23:30 ` Dave Chinner
2024-04-26 6:09 ` Christoph Hellwig
2024-05-01 8:10 ` John Garry
1 sibling, 2 replies; 14+ messages in thread
From: Dave Chinner @ 2024-04-25 23:30 UTC (permalink / raw
To: John Garry; +Cc: Christoph Hellwig, chandan.babu, djwong, linux-xfs
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 23:30 ` Dave Chinner
@ 2024-04-26 6:09 ` Christoph Hellwig
2024-05-01 8:10 ` John Garry
1 sibling, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2024-04-26 6:09 UTC (permalink / raw
To: Dave Chinner
Cc: John Garry, Christoph Hellwig, chandan.babu, djwong, linux-xfs
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks():
2024-04-25 23:30 ` Dave Chinner
2024-04-26 6:09 ` Christoph Hellwig
@ 2024-05-01 8:10 ` John Garry
1 sibling, 0 replies; 14+ messages in thread
From: John Garry @ 2024-05-01 8:10 UTC (permalink / raw
To: Dave Chinner; +Cc: Christoph Hellwig, chandan.babu, djwong, linux-xfs
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-05-01 8:10 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-25 12:08 [PATCH 0/2] xfs: Clear a couple of W=1 warnings John Garry
2024-04-25 12:08 ` [PATCH 1/2] xfs: Clear W=1 warning in xfs_iwalk_run_callbacks(): John Garry
2024-04-25 12:17 ` Christoph Hellwig
2024-04-25 13:24 ` John Garry
2024-04-25 13:30 ` Christoph Hellwig
2024-04-25 13:33 ` John Garry
2024-04-25 15:37 ` John Garry
2024-04-25 16:17 ` Darrick J. Wong
2024-04-25 23:30 ` Dave Chinner
2024-04-26 6:09 ` Christoph Hellwig
2024-05-01 8:10 ` John Garry
2024-04-25 12:08 ` [PATCH 2/2] xfs: Clear W=1 warning in xfs_trans_unreserve_and_mod_sb(): John Garry
2024-04-25 12:18 ` Christoph Hellwig
2024-04-25 13:35 ` John Garry
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).