* [PATCH 0/2] xfs: discard every free extent in a whole-AG FITRIM
@ 2026-10-26 1:54 ` Daejun Park via B4 Relay
0 siblings, 0 replies; 8+ messages in thread
From: Daejun Park via B4 Relay @ 2026-10-26 1:54 UTC (permalink / raw)
To: Carlos Maiolino, linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, linux-kernel,
stable, Daejun Park
A whole-device fstrim walks each AG but the last by length. Two bugs
in that walk leave free extents undiscarded, while fstrim reports the
whole range as trimmed. Patch 2 depends on patch 1.
The new fstests case xfs/842, sent separately, fails without this
series and passes with it. ./check -g trim shows no regressions
(generic/038 and 500 were not run).
---
Daejun Park (2):
xfs: don't skip free extents in later batches of a whole-AG FITRIM
xfs: don't skip free extents of the longest length in a whole-AG FITRIM
fs/xfs/xfs_discard.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
---
base-commit: ec8f62cc109a3f596b3c65dc0f1011c209239051
change-id: 20261023-xfs-fitrim-skip-7ff1cf0364c5
Best regards,
--
Daejun Park <daejun7.park@samsung.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] xfs: don't skip free extents in later batches of a whole-AG FITRIM
2026-10-26 1:54 ` Daejun Park via B4 Relay
@ 2026-10-26 1:54 ` Daejun Park via B4 Relay
-1 siblings, 0 replies; 8+ messages in thread
From: Daejun Park @ 2026-10-26 1:54 UTC (permalink / raw)
To: Carlos Maiolino, linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, linux-kernel,
stable, Daejun Park
A whole-device FITRIM walks every AG but the last by length, in
batches. xfs_trim_gather_extents() saves the record to restart from in
tcur->start. But tcur->start is also the start of the range to trim.
Later batches therefore never discard the free extents below that
block.
Save the restart record in a new restart_bno field.
Cc: stable@vger.kernel.org # v6.10
Fixes: b0ffe661fab4 ("xfs: fix performance problems when fstrimming a subset of a fragmented AG")
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
In the first layout of the new fstests case xfs/842, 409 of 512 free
extents that held data were not discarded without this patch, and none
with it.
---
fs/xfs/xfs_discard.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 906221ffe9..8120f136af 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -155,6 +155,8 @@ struct xfs_trim_cur {
xfs_agblock_t end;
xfs_extlen_t minlen;
bool by_bno;
+ /* restart point (with count) for the by-length walk */
+ xfs_agblock_t restart_bno;
};
static int
@@ -198,14 +200,15 @@ xfs_trim_gather_extents(
error = xfs_alloc_lookup_le(cur, tcur->start, 0, &i);
if (!error && !i)
error = xfs_alloc_lookup_ge(cur, tcur->start, 0, &i);
- } else if (tcur->start == 0) {
+ } else if (tcur->restart_bno == NULLAGBLOCK) {
/* first time through a by-len starts with max length */
cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
error = xfs_alloc_lookup_ge(cur, 0, tcur->count, &i);
} else {
/* nth time through a by-len starts where we left off */
cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
- error = xfs_alloc_lookup_le(cur, tcur->start, tcur->count, &i);
+ error = xfs_alloc_lookup_le(cur, tcur->restart_bno,
+ tcur->count, &i);
}
if (error)
goto out_del_cursor;
@@ -235,9 +238,15 @@ xfs_trim_gather_extents(
if (--batch <= 0) {
/*
* Update the cursor to point at this extent so we
- * restart the next batch from this extent.
+ * restart the next batch from this extent. The
+ * by-length walk does not visit extents in block
+ * order, so it must keep the start of the range and
+ * restart from restart_bno instead.
*/
- tcur->start = fbno;
+ if (tcur->by_bno)
+ tcur->start = fbno;
+ else
+ tcur->restart_bno = fbno;
tcur->count = flen;
break;
}
@@ -340,6 +349,7 @@ xfs_trim_perag_extents(
.start = start,
.end = end,
.minlen = minlen,
+ .restart_bno = NULLAGBLOCK,
};
int error = 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 1/2] xfs: don't skip free extents in later batches of a whole-AG FITRIM
@ 2026-10-26 1:54 ` Daejun Park via B4 Relay
0 siblings, 0 replies; 8+ messages in thread
From: Daejun Park via B4 Relay @ 2026-10-26 1:54 UTC (permalink / raw)
To: Carlos Maiolino, linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, linux-kernel,
stable, Daejun Park
From: Daejun Park <daejun7.park@samsung.com>
A whole-device FITRIM walks every AG but the last by length, in
batches. xfs_trim_gather_extents() saves the record to restart from in
tcur->start. But tcur->start is also the start of the range to trim.
Later batches therefore never discard the free extents below that
block.
Save the restart record in a new restart_bno field.
Cc: stable@vger.kernel.org # v6.10
Fixes: b0ffe661fab4 ("xfs: fix performance problems when fstrimming a subset of a fragmented AG")
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
In the first layout of the new fstests case xfs/842, 409 of 512 free
extents that held data were not discarded without this patch, and none
with it.
---
fs/xfs/xfs_discard.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 906221ffe9..8120f136af 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -155,6 +155,8 @@ struct xfs_trim_cur {
xfs_agblock_t end;
xfs_extlen_t minlen;
bool by_bno;
+ /* restart point (with count) for the by-length walk */
+ xfs_agblock_t restart_bno;
};
static int
@@ -198,14 +200,15 @@ xfs_trim_gather_extents(
error = xfs_alloc_lookup_le(cur, tcur->start, 0, &i);
if (!error && !i)
error = xfs_alloc_lookup_ge(cur, tcur->start, 0, &i);
- } else if (tcur->start == 0) {
+ } else if (tcur->restart_bno == NULLAGBLOCK) {
/* first time through a by-len starts with max length */
cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
error = xfs_alloc_lookup_ge(cur, 0, tcur->count, &i);
} else {
/* nth time through a by-len starts where we left off */
cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
- error = xfs_alloc_lookup_le(cur, tcur->start, tcur->count, &i);
+ error = xfs_alloc_lookup_le(cur, tcur->restart_bno,
+ tcur->count, &i);
}
if (error)
goto out_del_cursor;
@@ -235,9 +238,15 @@ xfs_trim_gather_extents(
if (--batch <= 0) {
/*
* Update the cursor to point at this extent so we
- * restart the next batch from this extent.
+ * restart the next batch from this extent. The
+ * by-length walk does not visit extents in block
+ * order, so it must keep the start of the range and
+ * restart from restart_bno instead.
*/
- tcur->start = fbno;
+ if (tcur->by_bno)
+ tcur->start = fbno;
+ else
+ tcur->restart_bno = fbno;
tcur->count = flen;
break;
}
@@ -340,6 +349,7 @@ xfs_trim_perag_extents(
.start = start,
.end = end,
.minlen = minlen,
+ .restart_bno = NULLAGBLOCK,
};
int error = 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] xfs: don't skip free extents in later batches of a whole-AG FITRIM
2026-10-26 1:54 ` Daejun Park via B4 Relay
(?)
@ 2026-10-28 15:03 ` Christoph Hellwig
-1 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2026-10-28 15:03 UTC (permalink / raw)
To: daejun7.park
Cc: Carlos Maiolino, linux-xfs, Darrick J. Wong, Dave Chinner,
Christoph Hellwig, linux-kernel, stable
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xfs: don't skip free extents of the longest length in a whole-AG FITRIM
2026-10-26 1:54 ` Daejun Park via B4 Relay
@ 2026-10-26 1:54 ` Daejun Park via B4 Relay
-1 siblings, 0 replies; 8+ messages in thread
From: Daejun Park @ 2026-10-26 1:54 UTC (permalink / raw)
To: Carlos Maiolino, linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, linux-kernel,
stable, Daejun Park
Since commit a66d636385d6 ("xfs: fix fstrim offset calculations"), the
by-length walk of an AG in FITRIM starts with
xfs_alloc_lookup_ge(cur, 0, pagf_longest) and walks down. The lookup
lands on the longest free extent with the lowest block number. The
other free extents of that length sort after it and are never
discarded.
Start every by-length batch with xfs_alloc_lookup_le() at restart_bno.
Suggested-by: Dave Chinner <dgc@kernel.org>
Link: https://lore.kernel.org/r/ZgiJWxVcVwag0fIP@dread.disaster.area
Cc: stable@vger.kernel.org
Fixes: a66d636385d6 ("xfs: fix fstrim offset calculations")
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
In xfs/842, 6 of 15 free extents that held data were not discarded
without this patch, and none with it.
On v6.10 and later this needs the previous patch. Older stable trees
need a small backport, which I can send:
- 6.6.y: always use xfs_alloc_lookup_le(cur, tcur->ar_startblock,
tcur->ar_blockcount, &i), and backport 23be716b1c4f ("xfs: don't
assume perags are initialised when trimming AGs") with it.
- 5.10.y to 6.1.y: replace xfs_alloc_lookup_ge(cur, 0, agf_longest)
with xfs_alloc_lookup_le(cur, NULLAGBLOCK, agf_longest).
---
fs/xfs/xfs_discard.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 8120f136af..cdf660344c 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -200,12 +200,12 @@ xfs_trim_gather_extents(
error = xfs_alloc_lookup_le(cur, tcur->start, 0, &i);
if (!error && !i)
error = xfs_alloc_lookup_ge(cur, tcur->start, 0, &i);
- } else if (tcur->restart_bno == NULLAGBLOCK) {
- /* first time through a by-len starts with max length */
- cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
- error = xfs_alloc_lookup_ge(cur, 0, tcur->count, &i);
} else {
- /* nth time through a by-len starts where we left off */
+ /*
+ * A by-len walk starts at the last record of the longest
+ * length (restart_bno is NULLAGBLOCK until the first batch
+ * ends) and then where the previous batch stopped.
+ */
cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
error = xfs_alloc_lookup_le(cur, tcur->restart_bno,
tcur->count, &i);
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/2] xfs: don't skip free extents of the longest length in a whole-AG FITRIM
@ 2026-10-26 1:54 ` Daejun Park via B4 Relay
0 siblings, 0 replies; 8+ messages in thread
From: Daejun Park via B4 Relay @ 2026-10-26 1:54 UTC (permalink / raw)
To: Carlos Maiolino, linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, linux-kernel,
stable, Daejun Park
From: Daejun Park <daejun7.park@samsung.com>
Since commit a66d636385d6 ("xfs: fix fstrim offset calculations"), the
by-length walk of an AG in FITRIM starts with
xfs_alloc_lookup_ge(cur, 0, pagf_longest) and walks down. The lookup
lands on the longest free extent with the lowest block number. The
other free extents of that length sort after it and are never
discarded.
Start every by-length batch with xfs_alloc_lookup_le() at restart_bno.
Suggested-by: Dave Chinner <dgc@kernel.org>
Link: https://lore.kernel.org/r/ZgiJWxVcVwag0fIP@dread.disaster.area
Cc: stable@vger.kernel.org
Fixes: a66d636385d6 ("xfs: fix fstrim offset calculations")
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
In xfs/842, 6 of 15 free extents that held data were not discarded
without this patch, and none with it.
On v6.10 and later this needs the previous patch. Older stable trees
need a small backport, which I can send:
- 6.6.y: always use xfs_alloc_lookup_le(cur, tcur->ar_startblock,
tcur->ar_blockcount, &i), and backport 23be716b1c4f ("xfs: don't
assume perags are initialised when trimming AGs") with it.
- 5.10.y to 6.1.y: replace xfs_alloc_lookup_ge(cur, 0, agf_longest)
with xfs_alloc_lookup_le(cur, NULLAGBLOCK, agf_longest).
---
fs/xfs/xfs_discard.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 8120f136af..cdf660344c 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -200,12 +200,12 @@ xfs_trim_gather_extents(
error = xfs_alloc_lookup_le(cur, tcur->start, 0, &i);
if (!error && !i)
error = xfs_alloc_lookup_ge(cur, tcur->start, 0, &i);
- } else if (tcur->restart_bno == NULLAGBLOCK) {
- /* first time through a by-len starts with max length */
- cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
- error = xfs_alloc_lookup_ge(cur, 0, tcur->count, &i);
} else {
- /* nth time through a by-len starts where we left off */
+ /*
+ * A by-len walk starts at the last record of the longest
+ * length (restart_bno is NULLAGBLOCK until the first batch
+ * ends) and then where the previous batch stopped.
+ */
cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
error = xfs_alloc_lookup_le(cur, tcur->restart_bno,
tcur->count, &i);
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] xfs: don't skip free extents of the longest length in a whole-AG FITRIM
2026-10-26 1:54 ` Daejun Park via B4 Relay
(?)
@ 2026-10-28 15:04 ` Christoph Hellwig
-1 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2026-10-28 15:04 UTC (permalink / raw)
To: daejun7.park
Cc: Carlos Maiolino, linux-xfs, Darrick J. Wong, Dave Chinner,
Christoph Hellwig, linux-kernel, stable
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 8+ messages in thread