* [PATCH v2 0/2] Misc fixes in XFS realtime @ 2026-01-28 18:44 Nirjhar Roy (IBM) 2026-01-28 18:44 ` [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() Nirjhar Roy (IBM) 2026-01-28 18:44 ` [PATCH v2 2/2] xfs: Fix in xfs_rtalloc_query_range() Nirjhar Roy (IBM) 0 siblings, 2 replies; 12+ messages in thread From: Nirjhar Roy (IBM) @ 2026-01-28 18:44 UTC (permalink / raw) To: linux-xfs; +Cc: ritesh.list, ojaswin, djwong, hch, cem, nirjhar.roy.lists 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-01-28 18:44 [PATCH v2 0/2] Misc fixes in XFS realtime Nirjhar Roy (IBM) @ 2026-01-28 18:44 ` Nirjhar Roy (IBM) 2026-01-29 8:52 ` Carlos Maiolino 2026-01-28 18:44 ` [PATCH v2 2/2] xfs: Fix in xfs_rtalloc_query_range() Nirjhar Roy (IBM) 1 sibling, 1 reply; 12+ messages in thread From: Nirjhar Roy (IBM) @ 2026-01-28 18:44 UTC (permalink / raw) To: linux-xfs; +Cc: ritesh.list, ojaswin, djwong, hch, cem, nirjhar.roy.lists 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-01-28 18:44 ` [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() Nirjhar Roy (IBM) @ 2026-01-29 8:52 ` Carlos Maiolino 2026-01-29 8:57 ` Carlos Maiolino 0 siblings, 1 reply; 12+ messages in thread From: Carlos Maiolino @ 2026-01-29 8:52 UTC (permalink / raw) To: Nirjhar Roy (IBM); +Cc: linux-xfs, ritesh.list, ojaswin, djwong, hch 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-01-29 8:52 ` Carlos Maiolino @ 2026-01-29 8:57 ` Carlos Maiolino 2026-01-29 12:34 ` Nirjhar Roy (IBM) 0 siblings, 1 reply; 12+ messages in thread From: Carlos Maiolino @ 2026-01-29 8:57 UTC (permalink / raw) To: Nirjhar Roy (IBM); +Cc: linux-xfs, ritesh.list, ojaswin, djwong, hch 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 > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-01-29 8:57 ` Carlos Maiolino @ 2026-01-29 12:34 ` Nirjhar Roy (IBM) 2026-01-29 13:29 ` Carlos Maiolino 0 siblings, 1 reply; 12+ messages in thread From: Nirjhar Roy (IBM) @ 2026-01-29 12:34 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-xfs, ritesh.list, ojaswin, djwong, hch 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-01-29 12:34 ` Nirjhar Roy (IBM) @ 2026-01-29 13:29 ` Carlos Maiolino 2026-01-29 17:10 ` Alan Huang 2026-02-02 14:08 ` Nirjhar Roy (IBM) 0 siblings, 2 replies; 12+ messages in thread From: Carlos Maiolino @ 2026-01-29 13:29 UTC (permalink / raw) To: Nirjhar Roy (IBM); +Cc: linux-xfs, ritesh.list, ojaswin, djwong, hch 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-01-29 13:29 ` Carlos Maiolino @ 2026-01-29 17:10 ` Alan Huang 2026-02-02 14:08 ` Nirjhar Roy (IBM) 1 sibling, 0 replies; 12+ messages in thread From: Alan Huang @ 2026-01-29 17:10 UTC (permalink / raw) To: Carlos Maiolino Cc: Nirjhar Roy (IBM), linux-xfs, ritesh.list, ojaswin, djwong, hch 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-01-29 13:29 ` Carlos Maiolino 2026-01-29 17:10 ` Alan Huang @ 2026-02-02 14:08 ` Nirjhar Roy (IBM) 2026-02-02 16:51 ` Carlos Maiolino 1 sibling, 1 reply; 12+ messages in thread From: Nirjhar Roy (IBM) @ 2026-02-02 14:08 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-xfs, ritesh.list, ojaswin, djwong, hch 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-02-02 14:08 ` Nirjhar Roy (IBM) @ 2026-02-02 16:51 ` Carlos Maiolino 2026-02-02 18:53 ` Darrick J. Wong 0 siblings, 1 reply; 12+ messages in thread From: Carlos Maiolino @ 2026-02-02 16:51 UTC (permalink / raw) To: Nirjhar Roy (IBM); +Cc: linux-xfs, ritesh.list, ojaswin, djwong, hch > > > > > 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! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() 2026-02-02 16:51 ` Carlos Maiolino @ 2026-02-02 18:53 ` Darrick J. Wong 0 siblings, 0 replies; 12+ messages in thread From: Darrick J. Wong @ 2026-02-02 18:53 UTC (permalink / raw) To: Carlos Maiolino; +Cc: Nirjhar Roy (IBM), linux-xfs, ritesh.list, ojaswin, hch 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! > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] xfs: Fix in xfs_rtalloc_query_range() 2026-01-28 18:44 [PATCH v2 0/2] Misc fixes in XFS realtime Nirjhar Roy (IBM) 2026-01-28 18:44 ` [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() Nirjhar Roy (IBM) @ 2026-01-28 18:44 ` Nirjhar Roy (IBM) 2026-01-29 9:03 ` Carlos Maiolino 1 sibling, 1 reply; 12+ messages in thread From: Nirjhar Roy (IBM) @ 2026-01-28 18:44 UTC (permalink / raw) To: linux-xfs; +Cc: ritesh.list, ojaswin, djwong, hch, cem, nirjhar.roy.lists 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] xfs: Fix in xfs_rtalloc_query_range() 2026-01-28 18:44 ` [PATCH v2 2/2] xfs: Fix in xfs_rtalloc_query_range() Nirjhar Roy (IBM) @ 2026-01-29 9:03 ` Carlos Maiolino 0 siblings, 0 replies; 12+ messages in thread From: Carlos Maiolino @ 2026-01-29 9:03 UTC (permalink / raw) To: Nirjhar Roy (IBM); +Cc: linux-xfs, ritesh.list, ojaswin, djwong, hch 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2026-02-02 18:53 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-01-28 18:44 [PATCH v2 0/2] Misc fixes in XFS realtime Nirjhar Roy (IBM) 2026-01-28 18:44 ` [PATCH v2 1/2] xfs: Move ASSERTion location in xfs_rtcopy_summary() Nirjhar Roy (IBM) 2026-01-29 8:52 ` Carlos Maiolino 2026-01-29 8:57 ` Carlos Maiolino 2026-01-29 12:34 ` Nirjhar Roy (IBM) 2026-01-29 13:29 ` Carlos Maiolino 2026-01-29 17:10 ` Alan Huang 2026-02-02 14:08 ` Nirjhar Roy (IBM) 2026-02-02 16:51 ` Carlos Maiolino 2026-02-02 18:53 ` Darrick J. Wong 2026-01-28 18:44 ` [PATCH v2 2/2] xfs: Fix in xfs_rtalloc_query_range() Nirjhar Roy (IBM) 2026-01-29 9:03 ` Carlos Maiolino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox