[PATCH] xfs: correct calculation for blockcount

[PATCH] xfs: correct calculation for blockcount

[Shiyang Ruan]

The blockcount, which means length, should be "end + 1 - start".  So,
add the missing "+1" here.

Fixes: 5cf32f63b0f4 ("xfs: fix the calculation for "end" and "length"")
Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
 fs/xfs/xfs_notify_failure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
index 4a9bbd3fe120..459fc8a39635 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -151,7 +151,7 @@ xfs_dax_notify_ddev_failure(
 		agend = min(be32_to_cpu(agf->agf_length),
 				ri_high.rm_startblock);
 		notify.startblock = ri_low.rm_startblock;
-		notify.blockcount = agend - ri_low.rm_startblock;
+		notify.blockcount = agend + 1 - ri_low.rm_startblock;
 
 		error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
 				xfs_dax_failure_fn, &notify);
-- 
2.41.0

Re: [PATCH] xfs: correct calculation for blockcount

[Shiyang Ruan]

Ping~

在 2023/8/28 15:24, Shiyang Ruan 写道:
> The blockcount, which means length, should be "end + 1 - start".  So,
> add the missing "+1" here.
> 
> Fixes: 5cf32f63b0f4 ("xfs: fix the calculation for "end" and "length"")
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
>   fs/xfs/xfs_notify_failure.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
> index 4a9bbd3fe120..459fc8a39635 100644
> --- a/fs/xfs/xfs_notify_failure.c
> +++ b/fs/xfs/xfs_notify_failure.c
> @@ -151,7 +151,7 @@ xfs_dax_notify_ddev_failure(
>   		agend = min(be32_to_cpu(agf->agf_length),
>   				ri_high.rm_startblock);
>   		notify.startblock = ri_low.rm_startblock;
> -		notify.blockcount = agend - ri_low.rm_startblock;
> +		notify.blockcount = agend + 1 - ri_low.rm_startblock;
>   
>   		error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
>   				xfs_dax_failure_fn, &notify);

Re: [PATCH] xfs: correct calculation for blockcount

[Darrick J. Wong]

On Fri, Sep 08, 2023 at 06:18:52PM +0800, Shiyang Ruan wrote:
> Ping~
> 
> 在 2023/8/28 15:24, Shiyang Ruan 写道:
> > The blockcount, which means length, should be "end + 1 - start".  So,
> > add the missing "+1" here.
> > 
> > Fixes: 5cf32f63b0f4 ("xfs: fix the calculation for "end" and "length"")
> > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> > ---
> >   fs/xfs/xfs_notify_failure.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
> > index 4a9bbd3fe120..459fc8a39635 100644
> > --- a/fs/xfs/xfs_notify_failure.c
> > +++ b/fs/xfs/xfs_notify_failure.c
> > @@ -151,7 +151,7 @@ xfs_dax_notify_ddev_failure(
> >   		agend = min(be32_to_cpu(agf->agf_length),
> >   				ri_high.rm_startblock);

I don't understand this.  ri_high.rm_startblock should be the last agbno
for which we want rmapbt mappings.  If agf_length is 100, then don't we
want to be clamping agend to 99, not 100?  Block 99 is the last block in
an AG.

	agend = min(be32_to_cpu(agf->agf_length) - 1,
		    ri_high.rm_startblock);

If we do the above...

> >   		notify.startblock = ri_low.rm_startblock;
> > -		notify.blockcount = agend - ri_low.rm_startblock;
> > +		notify.blockcount = agend + 1 - ri_low.rm_startblock;

...then this actually makes sense.

> >   		error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
> >   				xfs_dax_failure_fn, &notify);

Sorry I've been kinda slow to respond.

--D

[PATCH v2] xfs: correct calculation for agend and blockcount

[Shiyang Ruan]

The agend should be "start + length - 1", then, blockcount should be
"end + 1 - start".  Correct 2 calculation mistakes.

Fixes: 5cf32f63b0f4 ("xfs: fix the calculation for "end" and "length"")
Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
 fs/xfs/xfs_notify_failure.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
index 4a9bbd3fe120..8b8ef776bdc3 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -148,10 +148,10 @@ xfs_dax_notify_ddev_failure(
 			ri_high.rm_startblock = XFS_FSB_TO_AGBNO(mp, end_fsbno);
 
 		agf = agf_bp->b_addr;
-		agend = min(be32_to_cpu(agf->agf_length),
+		agend = min(be32_to_cpu(agf->agf_length) - 1,
 				ri_high.rm_startblock);
 		notify.startblock = ri_low.rm_startblock;
-		notify.blockcount = agend - ri_low.rm_startblock;
+		notify.blockcount = agend + 1 - ri_low.rm_startblock;
 
 		error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
 				xfs_dax_failure_fn, &notify);
-- 
2.42.0

Re: [PATCH] xfs: correct calculation for blockcount

[Shiyang Ruan]

在 2023/9/9 7:55, Darrick J. Wong 写道:
> On Fri, Sep 08, 2023 at 06:18:52PM +0800, Shiyang Ruan wrote:
>> Ping~
>>
>> 在 2023/8/28 15:24, Shiyang Ruan 写道:
>>> The blockcount, which means length, should be "end + 1 - start".  So,
>>> add the missing "+1" here.
>>>
>>> Fixes: 5cf32f63b0f4 ("xfs: fix the calculation for "end" and "length"")
>>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
>>> ---
>>>    fs/xfs/xfs_notify_failure.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
>>> index 4a9bbd3fe120..459fc8a39635 100644
>>> --- a/fs/xfs/xfs_notify_failure.c
>>> +++ b/fs/xfs/xfs_notify_failure.c
>>> @@ -151,7 +151,7 @@ xfs_dax_notify_ddev_failure(
>>>    		agend = min(be32_to_cpu(agf->agf_length),
>>>    				ri_high.rm_startblock);
> 
> I don't understand this.  ri_high.rm_startblock should be the last agbno
> for which we want rmapbt mappings.  If agf_length is 100, then don't we
> want to be clamping agend to 99, not 100?  Block 99 is the last block in
> an AG.
> 
> 	agend = min(be32_to_cpu(agf->agf_length) - 1,
> 		    ri_high.rm_startblock);

This is right.  Will fix this too.

> 
> If we do the above...
> 
>>>    		notify.startblock = ri_low.rm_startblock;
>>> -		notify.blockcount = agend - ri_low.rm_startblock;
>>> +		notify.blockcount = agend + 1 - ri_low.rm_startblock;
> 
> ...then this actually makes sense.
> 
>>>    		error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
>>>    				xfs_dax_failure_fn, &notify);
> 
> Sorry I've been kinda slow to respond.

No problem :)


--
Thanks,
Ruan.

> 
> --D

Re: [PATCH v2] xfs: correct calculation for agend and blockcount

[Darrick J. Wong]

On Mon, Sep 11, 2023 at 06:46:41PM +0800, Shiyang Ruan wrote:
> The agend should be "start + length - 1", then, blockcount should be
> "end + 1 - start".  Correct 2 calculation mistakes.
> 
> Fixes: 5cf32f63b0f4 ("xfs: fix the calculation for "end" and "length"")
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>

/me notes that "agend" would be better named "range_agend" since it's
not the end of the AG per se; it's the end of the dead region *within*
an AG's agblock space.

With that changed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_notify_failure.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
> index 4a9bbd3fe120..8b8ef776bdc3 100644
> --- a/fs/xfs/xfs_notify_failure.c
> +++ b/fs/xfs/xfs_notify_failure.c
> @@ -148,10 +148,10 @@ xfs_dax_notify_ddev_failure(
>  			ri_high.rm_startblock = XFS_FSB_TO_AGBNO(mp, end_fsbno);
>  
>  		agf = agf_bp->b_addr;
> -		agend = min(be32_to_cpu(agf->agf_length),
> +		agend = min(be32_to_cpu(agf->agf_length) - 1,
>  				ri_high.rm_startblock);
>  		notify.startblock = ri_low.rm_startblock;
> -		notify.blockcount = agend - ri_low.rm_startblock;
> +		notify.blockcount = agend + 1 - ri_low.rm_startblock;
>  
>  		error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
>  				xfs_dax_failure_fn, &notify);
> -- 
> 2.42.0
> 

[PATCH v3] xfs: correct calculation for agend and blockcount

[Shiyang Ruan]

The agend should be "start + length - 1", then, blockcount should be
"end + 1 - start".  Correct 2 calculation mistakes.

Also, rename "agend" to "range_agend" because it's not the end of the AG
per se; it's the end of the dead region within an AG's agblock space.

Fixes: 5cf32f63b0f4 ("xfs: fix the calculation for "end" and "length"")
Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_notify_failure.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
index 4a9bbd3fe120..a7daa522e00f 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -126,8 +126,8 @@ xfs_dax_notify_ddev_failure(
 		struct xfs_rmap_irec	ri_low = { };
 		struct xfs_rmap_irec	ri_high;
 		struct xfs_agf		*agf;
-		xfs_agblock_t		agend;
 		struct xfs_perag	*pag;
+		xfs_agblock_t		range_agend;
 
 		pag = xfs_perag_get(mp, agno);
 		error = xfs_alloc_read_agf(pag, tp, 0, &agf_bp);
@@ -148,10 +148,10 @@ xfs_dax_notify_ddev_failure(
 			ri_high.rm_startblock = XFS_FSB_TO_AGBNO(mp, end_fsbno);
 
 		agf = agf_bp->b_addr;
-		agend = min(be32_to_cpu(agf->agf_length),
+		range_agend = min(be32_to_cpu(agf->agf_length) - 1,
 				ri_high.rm_startblock);
 		notify.startblock = ri_low.rm_startblock;
-		notify.blockcount = agend - ri_low.rm_startblock;
+		notify.blockcount = range_agend + 1 - ri_low.rm_startblock;
 
 		error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
 				xfs_dax_failure_fn, &notify);
-- 
2.42.0