From: Christoph Hellwig <hch@lst.de>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: cem@kernel.org, hch@lst.de, stable@vger.kernel.org,
linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/8] xfs: don't replace the wrong part of the cow fork
Date: Fri, 10 Jul 2026 07:37:01 +0200 [thread overview]
Message-ID: <20260710053701.GA7392@lst.de> (raw)
In-Reply-To: <178366081014.1173468.1923355584342845630.stgit@frogsfrogsfrogs>
On Thu, Jul 09, 2026 at 10:21:56PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> LOLLM points out that xfs_iext_lookup_extent can return a @got where
> got->br_startoff < startoff. In this case, xrep_cow_replace_range
> replaces the entire mapping instead of just the part that had been
> marked bad in the bitmap, but advances the bitmap cursor in
> xrep_cow_replace by the amount replaced. As a result, we fail to
> replace the end of the bad range, and replace part of the good range.
>
> Fix this by rewriting the replace method to handle replacing the middle
> of a cow fork mapping. This we do by returning both the current mapping
> as @got, and the subset of the mapping that we want to replace as @rep,
> using @rep to store the results of the new allocation, and comparing
> @rep to @got to figure out the exact transformations needed.
>
> Cc: <stable@vger.kernel.org> # v6.8
> Fixes: dbbdbd0086320a ("xfs: repair problems in CoW forks")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> Assisted-by: LOLLM # finding obvious bugs
> ---
> fs/xfs/scrub/trace.h | 28 ++++--
> fs/xfs/scrub/cow_repair.c | 203 +++++++++++++++++++++++++++++----------------
> 2 files changed, 148 insertions(+), 83 deletions(-)
>
>
> diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> index 1b7d9e07a27d3d..d48a6db5b5f308 100644
> --- a/fs/xfs/scrub/trace.h
> +++ b/fs/xfs/scrub/trace.h
> @@ -2672,9 +2672,9 @@ TRACE_EVENT(xrep_cow_mark_file_range,
> );
>
> TRACE_EVENT(xrep_cow_replace_mapping,
> - TP_PROTO(struct xfs_inode *ip, const struct xfs_bmbt_irec *irec,
> - xfs_fsblock_t new_startblock, xfs_extlen_t new_blockcount),
> - TP_ARGS(ip, irec, new_startblock, new_blockcount),
> + TP_PROTO(struct xfs_inode *ip, const struct xfs_bmbt_irec *got,
> + const struct xfs_bmbt_irec *rep),
> + TP_ARGS(ip, got, rep),
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(xfs_ino_t, ino)
> @@ -2682,28 +2682,34 @@ TRACE_EVENT(xrep_cow_replace_mapping,
> __field(xfs_fileoff_t, startoff)
> __field(xfs_filblks_t, blockcount)
> __field(xfs_exntst_t, state)
> + __field(xfs_fileoff_t, new_startoff)
> __field(xfs_fsblock_t, new_startblock)
> __field(xfs_extlen_t, new_blockcount)
> + __field(xfs_exntst_t, new_state)
> ),
> TP_fast_assign(
> __entry->dev = ip->i_mount->m_super->s_dev;
> __entry->ino = I_INO(ip);
> - __entry->startoff = irec->br_startoff;
> - __entry->startblock = irec->br_startblock;
> - __entry->blockcount = irec->br_blockcount;
> - __entry->state = irec->br_state;
> - __entry->new_startblock = new_startblock;
> - __entry->new_blockcount = new_blockcount;
> + __entry->startoff = got->br_startoff;
> + __entry->startblock = got->br_startblock;
> + __entry->blockcount = got->br_blockcount;
> + __entry->state = got->br_state;
> + __entry->new_startoff = rep->br_startoff;
> + __entry->new_startblock = rep->br_startblock;
> + __entry->new_blockcount = rep->br_blockcount;
> + __entry->new_state = rep->br_state;
> ),
> - TP_printk("dev %d:%d ino 0x%llx startoff 0x%llx startblock 0x%llx fsbcount 0x%llx state 0x%x new_startblock 0x%llx new_fsbcount 0x%x",
> + TP_printk("dev %d:%d ino 0x%llx startoff 0x%llx startblock 0x%llx fsbcount 0x%llx state 0x%x new_startoff 0x%llx new_startblock 0x%llx new_fsbcount 0x%x new_state 0x%x",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> __entry->ino,
> __entry->startoff,
> __entry->startblock,
> __entry->blockcount,
> __entry->state,
> + __entry->new_startoff,
> __entry->new_startblock,
> - __entry->new_blockcount)
> + __entry->new_blockcount,
> + __entry->new_state)
> );
>
> TRACE_EVENT(xrep_cow_free_staging,
> diff --git a/fs/xfs/scrub/cow_repair.c b/fs/xfs/scrub/cow_repair.c
> index 0075b6d5a1b5ff..ca3405a26b641c 100644
> --- a/fs/xfs/scrub/cow_repair.c
> +++ b/fs/xfs/scrub/cow_repair.c
> @@ -80,12 +80,6 @@ struct xrep_cow {
> unsigned int next_bno;
> };
>
> -/* CoW staging extent. */
> -struct xrep_cow_extent {
> - xfs_fsblock_t fsbno;
> - xfs_extlen_t len;
> -};
> -
> /*
> * Mark the part of the file range that corresponds to the given physical
> * space. Caller must ensure that the physical range is within xc->irec.
> @@ -401,22 +395,21 @@ xrep_cow_find_bad_rt(
> STATIC int
> xrep_cow_alloc(
> struct xfs_scrub *sc,
> - xfs_extlen_t maxlen,
> - struct xrep_cow_extent *repl)
> + struct xfs_bmbt_irec *del)
> {
> struct xfs_alloc_arg args = {
> .tp = sc->tp,
> .mp = sc->mp,
> .oinfo = XFS_RMAP_OINFO_SKIP_UPDATE,
> .minlen = 1,
> - .maxlen = maxlen,
> + .maxlen = del->br_blockcount,
> .prod = 1,
> .resv = XFS_AG_RESV_NONE,
> .datatype = XFS_ALLOC_USERDATA,
> };
> int error;
>
> - error = xfs_trans_reserve_more(sc->tp, maxlen, 0);
> + error = xfs_trans_reserve_more(sc->tp, del->br_blockcount, 0);
> if (error)
> return error;
>
> @@ -428,8 +421,8 @@ xrep_cow_alloc(
>
> xfs_refcount_alloc_cow_extent(sc->tp, false, args.fsbno, args.len);
>
> - repl->fsbno = args.fsbno;
> - repl->len = args.len;
> + del->br_startblock = args.fsbno;
> + del->br_blockcount = args.len;
> return 0;
> }
>
> @@ -440,10 +433,12 @@ xrep_cow_alloc(
> STATIC int
> xrep_cow_alloc_rt(
> struct xfs_scrub *sc,
> - xfs_extlen_t maxlen,
> - struct xrep_cow_extent *repl)
> + struct xfs_bmbt_irec *del)
> {
> - xfs_rtxlen_t maxrtx = xfs_rtb_to_rtx(sc->mp, maxlen);
> + xfs_fsblock_t fsbno;
> + xfs_rtxlen_t maxrtx =
> + min(U32_MAX, xfs_blen_to_rtbxlen(sc->mp, del->br_blockcount));
> + xfs_extlen_t len;
> int error;
>
> error = xfs_trans_reserve_more(sc->tp, 0, maxrtx);
> @@ -451,11 +446,14 @@ xrep_cow_alloc_rt(
> return error;
>
> error = xfs_rtallocate_rtgs(sc->tp, NULLRTBLOCK, 1, maxrtx, 1, false,
> - false, &repl->fsbno, &repl->len);
> + false, &fsbno, &len);
> if (error)
> return error;
>
> - xfs_refcount_alloc_cow_extent(sc->tp, true, repl->fsbno, repl->len);
> + xfs_refcount_alloc_cow_extent(sc->tp, true, fsbno, len);
> +
> + del->br_startblock = fsbno;
> + del->br_blockcount = len;
> return 0;
> }
>
> @@ -469,19 +467,19 @@ static inline int
> xrep_cow_find_mapping(
> struct xrep_cow *xc,
> struct xfs_iext_cursor *icur,
> - xfs_fileoff_t startoff,
> - struct xfs_bmbt_irec *got)
> + xfs_fileoff_t badoff,
> + xfs_extlen_t badlen,
> + struct xfs_bmbt_irec *got,
> + struct xfs_bmbt_irec *rep)
> {
> struct xfs_inode *ip = xc->sc->ip;
> struct xfs_ifork *ifp = xfs_ifork_ptr(ip, XFS_COW_FORK);
>
> - if (!xfs_iext_lookup_extent(ip, ifp, startoff, icur, got))
> + if (!xfs_iext_lookup_extent(ip, ifp, badoff, icur, got))
> goto bad;
> + memcpy(rep, got, sizeof(*rep));
>
> - if (got->br_startoff > startoff)
> - goto bad;
> -
> - if (got->br_blockcount == 0)
> + if (got->br_startoff > badoff)
> goto bad;
>
> if (isnullstartblock(got->br_startblock))
> @@ -490,6 +488,24 @@ xrep_cow_find_mapping(
> if (xfs_bmap_is_written_extent(got))
> goto bad;
>
> + if (got->br_startoff < badoff) {
> + const int64_t delta = badoff - got->br_startoff;
> +
> + rep->br_blockcount -= delta;
> + rep->br_startoff += delta;
> + rep->br_startblock += delta;
> + }
> +
> + if (got->br_startoff + got->br_blockcount > badoff + badlen) {
> + const int64_t delta = (got->br_startoff + got->br_blockcount) -
> + (badoff + badlen);
> +
> + rep->br_blockcount -= delta;
Overly long line here. Although I'm not even sure what the point is in
this local delta variable that is only used once anyay.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
next prev parent reply other threads:[~2026-07-10 5:37 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-10 5:21 [PATCHSET v2] xfs: LOLLM-inspired bug fixes, part 1 Darrick J. Wong
2026-07-10 5:21 ` [PATCH 1/8] xfs: don't replace the wrong part of the cow fork Darrick J. Wong
2026-07-10 5:37 ` Christoph Hellwig [this message]
2026-07-10 5:22 ` [PATCH 2/8] xfs: make cow repair somewhat flaky when debugging knob enabled Darrick J. Wong
2026-07-10 5:37 ` Christoph Hellwig
2026-07-10 5:22 ` [PATCH 3/8] xfs: move cow_replace_mapping to xfs_bmap_util.c Darrick J. Wong
2026-07-10 5:37 ` Christoph Hellwig
2026-07-10 5:22 ` [PATCH 4/8] xfs: don't wrap around quota ids in dqiterate Darrick J. Wong
2026-07-10 5:22 ` [PATCH 5/8] xfs: use rtrefcount btree cursor in xchk_xref_is_rt_cow_staging Darrick J. Wong
2026-07-10 5:23 ` [PATCH 6/8] xfs: use the rt version of the cow staging checker Darrick J. Wong
2026-07-10 5:23 ` [PATCH 7/8] xfs: write the rg superblock when fixing it Darrick J. Wong
2026-07-10 5:23 ` [PATCH 8/8] xfs: grab rtrmap btree when checking rgsuper Darrick J. Wong
-- strict thread matches above, loose matches on Subject: below --
2026-07-14 6:03 [PATCHSET v3 1/2] xfs: LLM-inspired bug fixes, part 1 Darrick J. Wong
2026-07-14 6:04 ` [PATCH 1/8] xfs: don't replace the wrong part of the cow fork Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260710053701.GA7392@lst.de \
--to=hch@lst.de \
--cc=cem@kernel.org \
--cc=djwong@kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox