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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.