[PATCH v2 0/2] Misc fixes in XFS realtime

[PATCH v2 0/2] Misc fixes in XFS realtime

[Nirjhar Roy (IBM)]

This patchset has 2 fixes in some XFS realtime code. Details are
in the commit messages.

[v1] --> v2

1. Added RB from Darrick in patch 1/2.
2. Added Cc, Fixes tag and RB from Darrck in patch 2/2.

[v1]- https://lore.kernel.org/all/cover.1769613182.git.nirjhar.roy.lists@gmail.com/

Nirjhar Roy (IBM) (2):
  xfs: Move ASSERTion location in xfs_rtcopy_summary()
  xfs: Fix in xfs_rtalloc_query_range()

 fs/xfs/libxfs/xfs_rtbitmap.c | 2 +-
 fs/xfs/xfs_rtalloc.c         | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.43.5

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

[Nirjhar Roy (IBM)]

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;
+			}
 			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

[PATCH v2 2/2] xfs: Fix in xfs_rtalloc_query_range()

[Nirjhar Roy (IBM)]

xfs_rtalloc_query_range() should not return 0 by doing a NOP when
start == end i.e, when the rtgroup size is 1. This causes incorrect
calculation of free rtextents i.e, the count is reduced by 1 since
the last rtgroup's rtextent count is not taken and hence xfs_scrub
throws false summary counter report (from xchk_fscounters()).

A simple way to reproduce the above bug:

$ mkfs.xfs -f -m metadir=1 \
	-r rtdev=/dev/loop2,extsize=4096,rgcount=4,size=1G \
	-d size=1G /dev/loop1
meta-data=/dev/loop1             isize=512    agcount=4, agsize=65536 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=1
         =                       reflink=0    bigtime=1 inobtcount=1 nrext64=1
         =                       exchange=1   metadir=1
data     =                       bsize=4096   blocks=262144, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1, parent=0
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =/dev/loop2             extsz=4096   blocks=262144, rtextents=262144
         =                       rgcount=4    rgsize=65536 extents
         =                       zoned=0      start=0 reserved=0
Discarding blocks...Done.
Discarding blocks...Done.
$ mount -o rtdev=/dev/loop2 /dev/loop1 /mnt1/scratch
$ xfs_growfs -R $(( 65536 * 4 + 1 ))  /mnt1/scratch
meta-data=/dev/loop1             isize=512    agcount=4, agsize=65536 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=1
         =                       reflink=0    bigtime=1 inobtcount=1 nrext64=1
         =                       exchange=1   metadir=1
data     =                       bsize=4096   blocks=262144, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1, parent=0
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =/dev/loop2             extsz=4096   blocks=262144, rtextents=262144
         =                       rgcount=4    rgsize=65536 extents
         =                       zoned=0      start=0 reserved=0
calling xfsctl with in.newblocks = 262145
realtime blocks changed from 262144 to 262145
$ xfs_scrub -n   -v /mnt1/scratch
Phase 1: Find filesystem geometry.
/mnt1/scratch: using 2 threads to scrub.
Phase 2: Check internal metadata.
Corruption: rtgroup 4 realtime summary: Repairs are required.
Phase 3: Scan all inodes.
Phase 5: Check directory tree.
Info: /mnt1/scratch: Filesystem has errors, skipping connectivity checks.
Phase 7: Check summary counters.
Corruption: filesystem summary counters: Repairs are required.
125.0MiB data used;  8.0KiB realtime data used;  15 inodes used.
64.3MiB data found; 4.0KiB realtime data found; 18 inodes found.
18 inodes counted; 18 inodes checked.
Phase 8: Trim filesystem storage.
/mnt1/scratch: corruptions found: 2
/mnt1/scratch: Re-run xfs_scrub without -n.

Cc: <stable@vger.kernel.org> # v6.13
Fixes: e3088ae2dcae3c ("xfs: move RT bitmap and summary information to the rtgroup")

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 fs/xfs/libxfs/xfs_rtbitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 618061d898d4..8f552129ffcc 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -1170,7 +1170,7 @@ xfs_rtalloc_query_range(
 
 	if (start > end)
 		return -EINVAL;
-	if (start == end || start >= rtg->rtg_extents)
+	if (start >= rtg->rtg_extents)
 		return 0;
 
 	end = min(end, rtg->rtg_extents - 1);
-- 
2.43.5

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

[Carlos Maiolino]

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.

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
> 
> 

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

[Carlos Maiolino]

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.

> 
> 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
> > 
> > 
> 

Re: [PATCH v2 2/2] xfs: Fix in xfs_rtalloc_query_range()

[Carlos Maiolino]

On Thu, Jan 29, 2026 at 12:14:42AM +0530, Nirjhar Roy (IBM) wrote:
> xfs_rtalloc_query_range() should not return 0 by doing a NOP when
> start == end i.e, when the rtgroup size is 1. This causes incorrect
> calculation of free rtextents i.e, the count is reduced by 1 since
> the last rtgroup's rtextent count is not taken and hence xfs_scrub
> throws false summary counter report (from xchk_fscounters()).
> 
> A simple way to reproduce the above bug:
> 
> $ mkfs.xfs -f -m metadir=1 \
> 	-r rtdev=/dev/loop2,extsize=4096,rgcount=4,size=1G \
> 	-d size=1G /dev/loop1
> meta-data=/dev/loop1             isize=512    agcount=4, agsize=65536 blks
>          =                       sectsz=512   attr=2, projid32bit=1
>          =                       crc=1        finobt=1, sparse=1, rmapbt=1
>          =                       reflink=0    bigtime=1 inobtcount=1 nrext64=1
>          =                       exchange=1   metadir=1
> data     =                       bsize=4096   blocks=262144, imaxpct=25
>          =                       sunit=0      swidth=0 blks
> naming   =version 2              bsize=4096   ascii-ci=0, ftype=1, parent=0
> log      =internal log           bsize=4096   blocks=16384, version=2
>          =                       sectsz=512   sunit=0 blks, lazy-count=1
> realtime =/dev/loop2             extsz=4096   blocks=262144, rtextents=262144
>          =                       rgcount=4    rgsize=65536 extents
>          =                       zoned=0      start=0 reserved=0
> Discarding blocks...Done.
> Discarding blocks...Done.
> $ mount -o rtdev=/dev/loop2 /dev/loop1 /mnt1/scratch
> $ xfs_growfs -R $(( 65536 * 4 + 1 ))  /mnt1/scratch
> meta-data=/dev/loop1             isize=512    agcount=4, agsize=65536 blks
>          =                       sectsz=512   attr=2, projid32bit=1
>          =                       crc=1        finobt=1, sparse=1, rmapbt=1
>          =                       reflink=0    bigtime=1 inobtcount=1 nrext64=1
>          =                       exchange=1   metadir=1
> data     =                       bsize=4096   blocks=262144, imaxpct=25
>          =                       sunit=0      swidth=0 blks
> naming   =version 2              bsize=4096   ascii-ci=0, ftype=1, parent=0
> log      =internal log           bsize=4096   blocks=16384, version=2
>          =                       sectsz=512   sunit=0 blks, lazy-count=1
> realtime =/dev/loop2             extsz=4096   blocks=262144, rtextents=262144
>          =                       rgcount=4    rgsize=65536 extents
>          =                       zoned=0      start=0 reserved=0
> calling xfsctl with in.newblocks = 262145
> realtime blocks changed from 262144 to 262145
> $ xfs_scrub -n   -v /mnt1/scratch
> Phase 1: Find filesystem geometry.
> /mnt1/scratch: using 2 threads to scrub.
> Phase 2: Check internal metadata.
> Corruption: rtgroup 4 realtime summary: Repairs are required.
> Phase 3: Scan all inodes.
> Phase 5: Check directory tree.
> Info: /mnt1/scratch: Filesystem has errors, skipping connectivity checks.
> Phase 7: Check summary counters.
> Corruption: filesystem summary counters: Repairs are required.
> 125.0MiB data used;  8.0KiB realtime data used;  15 inodes used.
> 64.3MiB data found; 4.0KiB realtime data found; 18 inodes found.
> 18 inodes counted; 18 inodes checked.
> Phase 8: Trim filesystem storage.
> /mnt1/scratch: corruptions found: 2
> /mnt1/scratch: Re-run xfs_scrub without -n.
> 
> Cc: <stable@vger.kernel.org> # v6.13
> Fixes: e3088ae2dcae3c ("xfs: move RT bitmap and summary information to the rtgroup")
> 
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

>  fs/xfs/libxfs/xfs_rtbitmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 618061d898d4..8f552129ffcc 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1170,7 +1170,7 @@ xfs_rtalloc_query_range(
>  
>  	if (start > end)
>  		return -EINVAL;
> -	if (start == end || start >= rtg->rtg_extents)
> +	if (start >= rtg->rtg_extents)
>  		return 0;
>  
>  	end = min(end, rtg->rtg_extents - 1);
> -- 
> 2.43.5
> 
> 

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

[Nirjhar Roy (IBM)]

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/

--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

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

[Carlos Maiolino]

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.

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
> 
> 

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

[Alan Huang]

On Jan 29, 2026, at 21:29, Carlos Maiolino <cem@kernel.org> 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.

I had the same thought before, and I think ASSERT(0) would be less confusing.

> 
> 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

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

[Nirjhar Roy (IBM)]

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

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

[Carlos Maiolino]

> > > > > 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.
> 

Thanks!

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

[Darrick J. Wong]

On Mon, Feb 02, 2026 at 05:51:39PM +0100, Carlos Maiolino wrote:
> > > > > > 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.

Personally I think it's kinda dumb to log debugging messages that tell
you very little:

XFS: Assertion failed: 0, file: <whatever>

when you could actually say what the problem is:

XFS: Assertion failed: sum >= 0, file: <whatever>

but cem already said yes so I'm really just talking into the internet
here.

--D

> 
> Thanks!
>