Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary()

["Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com>]

On 1/29/26 18:59, Carlos Maiolino wrote:
> On Thu, Jan 29, 2026 at 06:04:20PM +0530, Nirjhar Roy (IBM) wrote:
>> On 1/29/26 14:27, Carlos Maiolino wrote:
>>> On Thu, Jan 29, 2026 at 09:52:02AM +0100, Carlos Maiolino wrote:
>>>> On Thu, Jan 29, 2026 at 12:14:41AM +0530, Nirjhar Roy (IBM) wrote:
>>>>> We should ASSERT on a variable before using it, so that we
>>>>> don't end up using an illegal value.
>>>>>
>>>>> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
>>>>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>>>>> ---
>>>>>    fs/xfs/xfs_rtalloc.c | 6 +++++-
>>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
>>>>> index a12ffed12391..9fb975171bf8 100644
>>>>> --- a/fs/xfs/xfs_rtalloc.c
>>>>> +++ b/fs/xfs/xfs_rtalloc.c
>>>>> @@ -112,6 +112,11 @@ xfs_rtcopy_summary(
>>>>>    			error = xfs_rtget_summary(oargs, log, bbno, &sum);
>>>>>    			if (error)
>>>>>    				goto out;
>>>>> +			if (sum < 0) {
>>>>> +				ASSERT(sum >= 0);
>>>>> +				error = -EFSCORRUPTED;
>>>>> +				goto out;
>>>>> +			}
>>>> What am I missing here? This looks weird...
>>>> We execute the block if sum is lower than 0, and then we assert it's
>>>> greater or equal than zero? This looks the assert will never fire as it
>>>> will only be checked when sum is always negative.
>>> Ugh, nvm, I'll grab more coffee. On the other hand, this still looks
>>> confusing, it would be better if we just ASSERT(0) there.
>> Well, the idea (as discussed in [1] and [2]) was that we should log that sum
>> has been assigned an illegal negative value (using an ASSERT) and then bail
>> out.
>> [1] https://lore.kernel.org/all/20260122181148.GE5945@frogsfrogsfrogs/
>>
>> [2] https://lore.kernel.org/all/20260128161447.GV5945@frogsfrogsfrogs/
> I see. I honestly think this is really ugly, pointless, and confusing at
> a first glance (at least for me). The assert location is logged anyway
> when it fire.
>
> If I'm the only one who finds this confusing, then fine, otherwise I'd
> rather see ASSERT(0) in there.

Sure, Carlos. ASSERT(0); sounds okay to me. Darrick, do you have any 
hard preferences between ASSERT(0); and ASSERT(sum < 0); ? If not, then 
I can go ahead, make the change and send a revision with the suggested 
change here.

--NR

>
> Darrick, hch, thoughts?
>
>> --NR
>>
>>>> What am I missing from this patch?
>>>>
>>>>>    			if (sum == 0)
>>>>>    				continue;
>>>>>    			error = xfs_rtmodify_summary(oargs, log, bbno, -sum);
>>>>> @@ -120,7 +125,6 @@ xfs_rtcopy_summary(
>>>>>    			error = xfs_rtmodify_summary(nargs, log, bbno, sum);
>>>>>    			if (error)
>>>>>    				goto out;
>>>>> -			ASSERT(sum > 0);
>>>>>    		}
>>>>>    	}
>>>>>    	error = 0;
>>>>> -- 
>>>>> 2.43.5
>>>>>
>>>>>
>> -- 
>> Nirjhar Roy
>> Linux Kernel Developer
>> IBM, Bangalore
>>
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore