[PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[Zizhi Wo]

From: Zizhi Wo <wozizhi@huaweicloud.com>

In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
to check the result of query_fn(), because there won't be another iteration
of the loop anyway. Also, both before and after the change, info->group
will eventually be set to NULL and the reference count of xfs_group will
also be decremented before exiting the iteration.

The same logic applies to other similar functions as well, so related
cleanup operations are performed together.

Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/xfs/xfs_fsmap.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 414b27a86458..792282aa8a29 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
 		if (pag_agno(pag) == end_ag) {
 			info->last = true;
 			error = query_fn(tp, info, &bt_cur, priv);
-			if (error)
-				break;
 		}
 		info->group = NULL;
 	}
@@ -813,8 +811,6 @@ xfs_getfsmap_rtdev_rtbitmap(
 			info->last = true;
 			error = xfs_getfsmap_rtdev_rtbitmap_helper(rtg, tp,
 					&ahigh, info);
-			if (error)
-				break;
 		}
 
 		xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_BITMAP_SHARED);
@@ -1018,8 +1014,6 @@ xfs_getfsmap_rtdev_rmapbt(
 			info->last = true;
 			error = xfs_getfsmap_rtdev_rmapbt_helper(bt_cur,
 					&info->high, info);
-			if (error)
-				break;
 		}
 		info->group = NULL;
 	}
-- 
2.39.2

Re: [PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[Nirjhar Roy (IBM)]

On Sat, 2025-05-17 at 15:43 +0800, Zizhi Wo wrote:
> From: Zizhi Wo <wozizhi@huaweicloud.com>
> 
> In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
> to check the result of query_fn(), because there won't be another iteration
Yes, this makes sense to me. Once pag_agno(pag) == end_ag, that will be the last iteration. "error"
is used later after coming out of the while loop. This looks good to me. 
Feel free to add

Reviewed-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>

> of the loop anyway. Also, both before and after the change, info->group
> will eventually be set to NULL and the reference count of xfs_group will
> also be decremented before exiting the iteration.
> 
> The same logic applies to other similar functions as well, so related
> cleanup operations are performed together.
> 
> Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>  fs/xfs/xfs_fsmap.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 414b27a86458..792282aa8a29 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
>  		if (pag_agno(pag) == end_ag) {
>  			info->last = true;
>  			error = query_fn(tp, info, &bt_cur, priv);
> -			if (error)
> -				break;
>  		}
>  		info->group = NULL;
>  	}
> @@ -813,8 +811,6 @@ xfs_getfsmap_rtdev_rtbitmap(
>  			info->last = true;
>  			error = xfs_getfsmap_rtdev_rtbitmap_helper(rtg, tp,
>  					&ahigh, info);
> -			if (error)
> -				break;
>  		}
>  
>  		xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_BITMAP_SHARED);
> @@ -1018,8 +1014,6 @@ xfs_getfsmap_rtdev_rmapbt(
>  			info->last = true;
>  			error = xfs_getfsmap_rtdev_rmapbt_helper(bt_cur,
>  					&info->high, info);
> -			if (error)
> -				break;
>  		}
>  		info->group = NULL;
>  	}

Re: [PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[Darrick J. Wong]

On Sat, May 17, 2025 at 03:43:41PM +0800, Zizhi Wo wrote:
> From: Zizhi Wo <wozizhi@huaweicloud.com>
> 
> In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
> to check the result of query_fn(), because there won't be another iteration
> of the loop anyway. Also, both before and after the change, info->group
> will eventually be set to NULL and the reference count of xfs_group will
> also be decremented before exiting the iteration.
> 
> The same logic applies to other similar functions as well, so related
> cleanup operations are performed together.
> 
> Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>  fs/xfs/xfs_fsmap.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 414b27a86458..792282aa8a29 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
>  		if (pag_agno(pag) == end_ag) {
>  			info->last = true;
>  			error = query_fn(tp, info, &bt_cur, priv);
> -			if (error)
> -				break;

Removing these statements make the error path harder to follow.  Before,
it was explicit that an error breaks out of the loop body.  Now you have
to look upwards to the while loop conditional and reason about what
xfs_perag_next_range does when pag-> agno == end_ag to determine that
the loop terminates.

This also leaves a tripping point for anyone who wants to add another
statement into this here if body because now they have to recognize that
they need to re-add the "if (error) break;" statements that you're now
taking out.

You also don't claim any reduction in generated machine code size or
execution speed, which means all the programmers end up having to
perform extra reasoning when reading this code for ... what?  Zero gain?

Please stop sending overly narrowly focused "optimizations" that make
life harder for all of us.

NAK.

--D

>  		}
>  		info->group = NULL;
>  	}
> @@ -813,8 +811,6 @@ xfs_getfsmap_rtdev_rtbitmap(
>  			info->last = true;
>  			error = xfs_getfsmap_rtdev_rtbitmap_helper(rtg, tp,
>  					&ahigh, info);
> -			if (error)
> -				break;
>  		}
>  
>  		xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_BITMAP_SHARED);
> @@ -1018,8 +1014,6 @@ xfs_getfsmap_rtdev_rmapbt(
>  			info->last = true;
>  			error = xfs_getfsmap_rtdev_rmapbt_helper(bt_cur,
>  					&info->high, info);
> -			if (error)
> -				break;
>  		}
>  		info->group = NULL;
>  	}
> -- 
> 2.39.2
> 
> 

Re: [PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[Carlos Maiolino]

On Mon, May 19, 2025 at 08:08:54AM -0700, Darrick J. Wong wrote:
> On Sat, May 17, 2025 at 03:43:41PM +0800, Zizhi Wo wrote:
> > From: Zizhi Wo <wozizhi@huaweicloud.com>
> >
> > In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
> > to check the result of query_fn(), because there won't be another iteration
> > of the loop anyway. Also, both before and after the change, info->group
> > will eventually be set to NULL and the reference count of xfs_group will
> > also be decremented before exiting the iteration.
> >
> > The same logic applies to other similar functions as well, so related
> > cleanup operations are performed together.
> >
> > Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
> > Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> > ---
> >  fs/xfs/xfs_fsmap.c | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > index 414b27a86458..792282aa8a29 100644
> > --- a/fs/xfs/xfs_fsmap.c
> > +++ b/fs/xfs/xfs_fsmap.c
> > @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
> >  		if (pag_agno(pag) == end_ag) {
> >  			info->last = true;
> >  			error = query_fn(tp, info, &bt_cur, priv);
> > -			if (error)
> > -				break;
> 
> Removing these statements make the error path harder to follow.  Before,
> it was explicit that an error breaks out of the loop body.  Now you have
> to look upwards to the while loop conditional and reason about what
> xfs_perag_next_range does when pag-> agno == end_ag to determine that
> the loop terminates.
> 
> This also leaves a tripping point for anyone who wants to add another
> statement into this here if body because now they have to recognize that
> they need to re-add the "if (error) break;" statements that you're now
> taking out.
> 
> You also don't claim any reduction in generated machine code size or
> execution speed, which means all the programmers end up having to
> perform extra reasoning when reading this code for ... what?  Zero gain?
> 
> Please stop sending overly narrowly focused "optimizations" that make
> life harder for all of us.

I do agree with Darrick. What's the point of this patch other than making code
harder to understand? This gets rid of less than 10 machine instructions at the
final module, and such cod is not even a hot path. making these extra instructions
virtually negligible IMO (looking at x86 architecture). The checks are unneeded
logically, but make the code easier to read, which is also important.
Did you actually see any improvement on anything by applying this patch? Or was
it crafted merely as a result of code inspection? Where are the results that make
this change worth the extra complexity while reading it?

Cheers,
Carlos

> 
> NAK.
> 
> --D
> 
> >  		}
> >  		info->group = NULL;
> >  	}
> > @@ -813,8 +811,6 @@ xfs_getfsmap_rtdev_rtbitmap(
> >  			info->last = true;
> >  			error = xfs_getfsmap_rtdev_rtbitmap_helper(rtg, tp,
> >  					&ahigh, info);
> > -			if (error)
> > -				break;
> >  		}
> >
> >  		xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_BITMAP_SHARED);
> > @@ -1018,8 +1014,6 @@ xfs_getfsmap_rtdev_rmapbt(
> >  			info->last = true;
> >  			error = xfs_getfsmap_rtdev_rmapbt_helper(bt_cur,
> >  					&info->high, info);
> > -			if (error)
> > -				break;
> >  		}
> >  		info->group = NULL;
> >  	}
> > --
> > 2.39.2
> >
> >
> 

Re: [PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[Zizhi Wo]

在 2025/5/20 18:38, Carlos Maiolino 写道:
> On Mon, May 19, 2025 at 08:08:54AM -0700, Darrick J. Wong wrote:
>> On Sat, May 17, 2025 at 03:43:41PM +0800, Zizhi Wo wrote:
>>> From: Zizhi Wo <wozizhi@huaweicloud.com>
>>>
>>> In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
>>> to check the result of query_fn(), because there won't be another iteration
>>> of the loop anyway. Also, both before and after the change, info->group
>>> will eventually be set to NULL and the reference count of xfs_group will
>>> also be decremented before exiting the iteration.
>>>
>>> The same logic applies to other similar functions as well, so related
>>> cleanup operations are performed together.
>>>
>>> Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
>>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>>> ---
>>>   fs/xfs/xfs_fsmap.c | 6 ------
>>>   1 file changed, 6 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
>>> index 414b27a86458..792282aa8a29 100644
>>> --- a/fs/xfs/xfs_fsmap.c
>>> +++ b/fs/xfs/xfs_fsmap.c
>>> @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
>>>   		if (pag_agno(pag) == end_ag) {
>>>   			info->last = true;
>>>   			error = query_fn(tp, info, &bt_cur, priv);
>>> -			if (error)
>>> -				break;
>>
>> Removing these statements make the error path harder to follow.  Before,
>> it was explicit that an error breaks out of the loop body.  Now you have
>> to look upwards to the while loop conditional and reason about what
>> xfs_perag_next_range does when pag-> agno == end_ag to determine that
>> the loop terminates.
>>
>> This also leaves a tripping point for anyone who wants to add another
>> statement into this here if body because now they have to recognize that
>> they need to re-add the "if (error) break;" statements that you're now
>> taking out.
>>
>> You also don't claim any reduction in generated machine code size or
>> execution speed, which means all the programmers end up having to
>> perform extra reasoning when reading this code for ... what?  Zero gain?
>>
>> Please stop sending overly narrowly focused "optimizations" that make
>> life harder for all of us.
> 
> I do agree with Darrick. What's the point of this patch other than making code
> harder to understand? This gets rid of less than 10 machine instructions at the
> final module, and such cod is not even a hot path. making these extra instructions
> virtually negligible IMO (looking at x86 architecture). The checks are unneeded
> logically, but make the code easier to read, which is also important.
> Did you actually see any improvement on anything by applying this patch? Or was
> it crafted merely as a result of code inspection? Where are the results that make
> this change worth the extra complexity while reading it?
> 

Yes, I was simply thinking about this while looking at the fsmap-related
code. Since the current for loop always exits after iterating to the
last AG, I thought off the top of my head that the error check for
last_ag might be unnecessary. As you and Darrick mentioned, though,
removing it would increase the difficulty of reading the code and could
also affect future developers.:(

If doing so could bring significant performance benefits, then it might
be worth considering — perhaps with some added comments to clarify? But
in this case, the performance gain is indeed negligible, I admit. Thank
you for pointing that out.

Thanks,
Zizhi Wo

> Cheers,
> Carlos
> 
>>
>> NAK.
>>
>> --D
>>
>>>   		}
>>>   		info->group = NULL;
>>>   	}
>>> @@ -813,8 +811,6 @@ xfs_getfsmap_rtdev_rtbitmap(
>>>   			info->last = true;
>>>   			error = xfs_getfsmap_rtdev_rtbitmap_helper(rtg, tp,
>>>   					&ahigh, info);
>>> -			if (error)
>>> -				break;
>>>   		}
>>>
>>>   		xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_BITMAP_SHARED);
>>> @@ -1018,8 +1014,6 @@ xfs_getfsmap_rtdev_rmapbt(
>>>   			info->last = true;
>>>   			error = xfs_getfsmap_rtdev_rmapbt_helper(bt_cur,
>>>   					&info->high, info);
>>> -			if (error)
>>> -				break;
>>>   		}
>>>   		info->group = NULL;
>>>   	}
>>> --
>>> 2.39.2
>>>
>>>
>>
> 
> 

Re: [PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[Carlos Maiolino]

On Tue, May 20, 2025 at 07:57:19PM +0800, Zizhi Wo wrote:
> 
> 
> 在 2025/5/20 18:38, Carlos Maiolino 写道:
> > On Mon, May 19, 2025 at 08:08:54AM -0700, Darrick J. Wong wrote:
> >> On Sat, May 17, 2025 at 03:43:41PM +0800, Zizhi Wo wrote:
> >>> From: Zizhi Wo <wozizhi@huaweicloud.com>
> >>>
> >>> In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
> >>> to check the result of query_fn(), because there won't be another iteration
> >>> of the loop anyway. Also, both before and after the change, info->group
> >>> will eventually be set to NULL and the reference count of xfs_group will
> >>> also be decremented before exiting the iteration.
> >>>
> >>> The same logic applies to other similar functions as well, so related
> >>> cleanup operations are performed together.
> >>>
> >>> Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
> >>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> >>> ---
> >>>   fs/xfs/xfs_fsmap.c | 6 ------
> >>>   1 file changed, 6 deletions(-)
> >>>
> >>> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> >>> index 414b27a86458..792282aa8a29 100644
> >>> --- a/fs/xfs/xfs_fsmap.c
> >>> +++ b/fs/xfs/xfs_fsmap.c
> >>> @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
> >>>   		if (pag_agno(pag) == end_ag) {
> >>>   			info->last = true;
> >>>   			error = query_fn(tp, info, &bt_cur, priv);
> >>> -			if (error)
> >>> -				break;
> >>
> >> Removing these statements make the error path harder to follow.  Before,
> >> it was explicit that an error breaks out of the loop body.  Now you have
> >> to look upwards to the while loop conditional and reason about what
> >> xfs_perag_next_range does when pag-> agno == end_ag to determine that
> >> the loop terminates.
> >>
> >> This also leaves a tripping point for anyone who wants to add another
> >> statement into this here if body because now they have to recognize that
> >> they need to re-add the "if (error) break;" statements that you're now
> >> taking out.
> >>
> >> You also don't claim any reduction in generated machine code size or
> >> execution speed, which means all the programmers end up having to
> >> perform extra reasoning when reading this code for ... what?  Zero gain?
> >>
> >> Please stop sending overly narrowly focused "optimizations" that make
> >> life harder for all of us.
> >
> > I do agree with Darrick. What's the point of this patch other than making code
> > harder to understand? This gets rid of less than 10 machine instructions at the
> > final module, and such cod is not even a hot path. making these extra instructions
> > virtually negligible IMO (looking at x86 architecture). The checks are unneeded
> > logically, but make the code easier to read, which is also important.
> > Did you actually see any improvement on anything by applying this patch? Or was
> > it crafted merely as a result of code inspection? Where are the results that make
> > this change worth the extra complexity while reading it?
> >
> 
> Yes, I was simply thinking about this while looking at the fsmap-related
> code. Since the current for loop always exits after iterating to the
> last AG, I thought off the top of my head that the error check for
> last_ag might be unnecessary. As you and Darrick mentioned, though,
> removing it would increase the difficulty of reading the code and could
> also affect future developers.:(
> 
> If doing so could bring significant performance benefits, then it might
> be worth considering — perhaps with some added comments to clarify? But
> in this case, the performance gain is indeed negligible, I admit. Thank
> you for pointing that out.

As anything, it's always a trade-off.
The key thing to take away here is your patch make the whole code it touches
harder to understand without bringing any real benefit. Even if you could
prove this adds small performance gain to the fsmap interface, perhaps it
would still not be worth the performance gain in lieu of poor code reliability,
giving the interface you changed. So, it's always about discussing the
change's trade-off between complexity and performance. In your case, there
seems to be no real trade-off.

Re: [PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[Zizhi Wo]

在 2025/5/20 20:47, Carlos Maiolino 写道:
> On Tue, May 20, 2025 at 07:57:19PM +0800, Zizhi Wo wrote:
>>
>>
>> 在 2025/5/20 18:38, Carlos Maiolino 写道:
>>> On Mon, May 19, 2025 at 08:08:54AM -0700, Darrick J. Wong wrote:
>>>> On Sat, May 17, 2025 at 03:43:41PM +0800, Zizhi Wo wrote:
>>>>> From: Zizhi Wo <wozizhi@huaweicloud.com>
>>>>>
>>>>> In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
>>>>> to check the result of query_fn(), because there won't be another iteration
>>>>> of the loop anyway. Also, both before and after the change, info->group
>>>>> will eventually be set to NULL and the reference count of xfs_group will
>>>>> also be decremented before exiting the iteration.
>>>>>
>>>>> The same logic applies to other similar functions as well, so related
>>>>> cleanup operations are performed together.
>>>>>
>>>>> Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
>>>>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>>>>> ---
>>>>>    fs/xfs/xfs_fsmap.c | 6 ------
>>>>>    1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
>>>>> index 414b27a86458..792282aa8a29 100644
>>>>> --- a/fs/xfs/xfs_fsmap.c
>>>>> +++ b/fs/xfs/xfs_fsmap.c
>>>>> @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
>>>>>    		if (pag_agno(pag) == end_ag) {
>>>>>    			info->last = true;
>>>>>    			error = query_fn(tp, info, &bt_cur, priv);
>>>>> -			if (error)
>>>>> -				break;
>>>>
>>>> Removing these statements make the error path harder to follow.  Before,
>>>> it was explicit that an error breaks out of the loop body.  Now you have
>>>> to look upwards to the while loop conditional and reason about what
>>>> xfs_perag_next_range does when pag-> agno == end_ag to determine that
>>>> the loop terminates.
>>>>
>>>> This also leaves a tripping point for anyone who wants to add another
>>>> statement into this here if body because now they have to recognize that
>>>> they need to re-add the "if (error) break;" statements that you're now
>>>> taking out.
>>>>
>>>> You also don't claim any reduction in generated machine code size or
>>>> execution speed, which means all the programmers end up having to
>>>> perform extra reasoning when reading this code for ... what?  Zero gain?
>>>>
>>>> Please stop sending overly narrowly focused "optimizations" that make
>>>> life harder for all of us.
>>>
>>> I do agree with Darrick. What's the point of this patch other than making code
>>> harder to understand? This gets rid of less than 10 machine instructions at the
>>> final module, and such cod is not even a hot path. making these extra instructions
>>> virtually negligible IMO (looking at x86 architecture). The checks are unneeded
>>> logically, but make the code easier to read, which is also important.
>>> Did you actually see any improvement on anything by applying this patch? Or was
>>> it crafted merely as a result of code inspection? Where are the results that make
>>> this change worth the extra complexity while reading it?
>>>
>>
>> Yes, I was simply thinking about this while looking at the fsmap-related
>> code. Since the current for loop always exits after iterating to the
>> last AG, I thought off the top of my head that the error check for
>> last_ag might be unnecessary. As you and Darrick mentioned, though,
>> removing it would increase the difficulty of reading the code and could
>> also affect future developers.:(
>>
>> If doing so could bring significant performance benefits, then it might
>> be worth considering — perhaps with some added comments to clarify? But
>> in this case, the performance gain is indeed negligible, I admit. Thank
>> you for pointing that out.
> 
> As anything, it's always a trade-off.
> The key thing to take away here is your patch make the whole code it touches
> harder to understand without bringing any real benefit. Even if you could
> prove this adds small performance gain to the fsmap interface, perhaps it
> would still not be worth the performance gain in lieu of poor code reliability,
> giving the interface you changed. So, it's always about discussing the
> change's trade-off between complexity and performance. In your case, there
> seems to be no real trade-off.
> 

Yes, unfortunately, the change I made here isn't substantial enough to
warrant any serious consideration or trade-off... Anyway, It was a good
learning experience for me as well.

Re: [PATCH] xfs: Remove unnecessary checks in functions related to xfs_fsmap

[Nirjhar Roy (IBM)]

On Tue, 2025-05-20 at 12:38 +0200, Carlos Maiolino wrote:
> On Mon, May 19, 2025 at 08:08:54AM -0700, Darrick J. Wong wrote:
> > On Sat, May 17, 2025 at 03:43:41PM +0800, Zizhi Wo wrote:
> > > From: Zizhi Wo <wozizhi@huaweicloud.com>
> > > 
> > > In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
> > > to check the result of query_fn(), because there won't be another iteration
> > > of the loop anyway. Also, both before and after the change, info->group
> > > will eventually be set to NULL and the reference count of xfs_group will
> > > also be decremented before exiting the iteration.
> > > 
> > > The same logic applies to other similar functions as well, so related
> > > cleanup operations are performed together.
> > > 
> > > Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com>
> > > Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> > > ---
> > >  fs/xfs/xfs_fsmap.c | 6 ------
> > >  1 file changed, 6 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > > index 414b27a86458..792282aa8a29 100644
> > > --- a/fs/xfs/xfs_fsmap.c
> > > +++ b/fs/xfs/xfs_fsmap.c
> > > @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
> > >  		if (pag_agno(pag) == end_ag) {
> > >  			info->last = true;
> > >  			error = query_fn(tp, info, &bt_cur, priv);
> > > -			if (error)
> > > -				break;
> > 
> > Removing these statements make the error path harder to follow.  Before,
> > it was explicit that an error breaks out of the loop body.  Now you have
> > to look upwards to the while loop conditional and reason about what
> > xfs_perag_next_range does when pag-> agno == end_ag to determine that
> > the loop terminates.
> > 
> > This also leaves a tripping point for anyone who wants to add another
> > statement into this here if body because now they have to recognize that
> > they need to re-add the "if (error) break;" statements that you're now
> > taking out.
> > 
> > You also don't claim any reduction in generated machine code size or
> > execution speed, which means all the programmers end up having to
> > perform extra reasoning when reading this code for ... what?  Zero gain?
> > 
> > Please stop sending overly narrowly focused "optimizations" that make
> > life harder for all of us.
> 
> I do agree with Darrick. What's the point of this patch other than making code
> harder to understand? This gets rid of less than 10 machine instructions at the
> final module, and such cod is not even a hot path. making these extra instructions
> virtually negligible IMO (looking at x86 architecture). The checks are unneeded
> logically, but make the code easier to read, which is also important.
> Did you actually see any improvement on anything by applying this patch? Or was
> it crafted merely as a result of code inspection? Where are the results that make
> this change worth the extra complexity while reading it?
I too agree with Darrick and Carlos. I initially gave my RB for the change, however I didn't
consider the above points mentioned by Carlos and Darrick. Thank you for the above pointers. It was
a good learning for me too - I will keep these in mind in my future reviews and patches too. 
--NR
> 
> Cheers,
> Carlos
> 
> > NAK.
> > 
> > --D
> > 
> > >  		}
> > >  		info->group = NULL;
> > >  	}
> > > @@ -813,8 +811,6 @@ xfs_getfsmap_rtdev_rtbitmap(
> > >  			info->last = true;
> > >  			error = xfs_getfsmap_rtdev_rtbitmap_helper(rtg, tp,
> > >  					&ahigh, info);
> > > -			if (error)
> > > -				break;
> > >  		}
> > > 
> > >  		xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_BITMAP_SHARED);
> > > @@ -1018,8 +1014,6 @@ xfs_getfsmap_rtdev_rmapbt(
> > >  			info->last = true;
> > >  			error = xfs_getfsmap_rtdev_rmapbt_helper(bt_cur,
> > >  					&info->high, info);
> > > -			if (error)
> > > -				break;
> > >  		}
> > >  		info->group = NULL;
> > >  	}
> > > --
> > > 2.39.2
> > > 
> > >