Re: support for RT data checksums

[Dave Chinner <dgc@kernel.org>]

On Fri, Sep 25, 2026 at 08:27:10AM +0200, Christoph Hellwig wrote:
> On Fri, Sep 25, 2026 at 08:52:58AM +1000, Dave Chinner wrote:
> > There's new buffer and inode locking in transactions,
> 
> Not sure what is new about that.  Tere is a single new transaction,
> which logs a single buffer per transaction.  Not exactly new and
> dancy.

You haven't answered any of my concerns - you're just handwaving
them away and....

> > and there's a
> > whole new buffer cache interface to "read a buffer", and that is
> > used to open code reading checksum buffers and joining them to a
> > transaction rather than using the existing xfs_trans_read_buf...()
> > interfaces.  That in itself needs careful consideration, and clear
> > justification for why it must be duplicated to stand outside all the
> > existing BLI/transaction APIs, especially given all the "use the new
> > async buf read interface to do sync buffer reads" behaviour across
> > the patchset that could just use the existing interfaces.
> 
> I'm not sure what to make of this.  The paragraph almost reads like
> AI slop to me.

... calling the concerns of an experienced engineer "AI slop".

I'm so disappointed right now.

You're better than this, Christoph. You know better than to attack
the person instead of addressing the technical concerns they've
raised. Calling the concerns of an experienced engineer "AI Slop" is
also pretty insulting.

You also know this has a chilling effect - how many people are going
to be willing to say anything negative about your code, if all they
get from it is a bunch of insults in return?

I don't tolerate people who behave like this towards me or other
team members anymore. "But I write lots of code" is just not good
enough anymore, Christoph. You need to do better.

FWIW, let's address the "AI Slop" aspect. I don't need to us an LLM
to review code - I know the XFS code base as well as you do,
Christoph.

I hadn't even considered passing your code to a LLM yet to see how
many holes I can poke in it. Everything I wrote document concerns my
own brain raised  when doing an initial high level scan of the
patchset. I always do that first so I don't waste time (or tokens!)
doing detailed review on something that needs deeper rework before
it is anywhere near ready for merge.

That scan raised lots of questions in my head about the change, and
so I documented some of my concerns and asked for more detail about
how all this new functionality is supposed to work to be documented.
Not just for me right now, but so there is a permanent record of the
design for future reference.

Performing critical analysis and asking for more detail to be
documented is not "AI Slop". A LLM would just plow on through with
whatever misunderstanding it started with and produce slop. It is
very much a human behaviour to stop and ask for more context,
detail, documentation, etc to address missing knoweldge before going
any further.

If you don't want meaningful review of this code, then just say so -
don't insult the people who spend their own time tryng to understand
the new functionality well enough to review it...

> The rationale is pretty clear and documented: it
> turns two dependent reads into two reads that work in parallel.

Maybe that is clear to you, but it certainly isn't from my initial
reading of the patches.

I don't have all the unwritten knowledge that is in your head that
you didn't document. I've never discussed this code with you, I have
no background on how it is supposed to work, etc. I am coming in
-cold-.

I have not been able to find clear, logical descriptions of
the algorithms for reading and writing checksums, the data
intregrity protocols, the crash recovery protocols, etc. At this
point, the only way for me to to understand details like this is to
-reverse engineer the design from implementation-.

That's the problem - you've presented an implementation without an
overall design being documented, and then asking people to review
the implementation without any other context. I need to understand
the design before I review the implementation, as that is the only
way I can perform a decent -implementation- review.

Indeed, people often complain that it is too hard to adequately
review complex changes, and this is one of the reasons: we are
presented with an implementation, and -zero- design/architecture
context from which to understand the implementation that has been
presented. We need to fix that.

> > I'd also like to have the format of the new on disk log item format
> > structures clearly documented (because we're going to have to
> 
> There is no new log item format, it uses the standard buffer log format.
> The buffer payload is somewhat new.  It is is the standard rt format,
> a xfs_rtbuf_blkinfo followed by the real payload, which is an array
> of checksums.

Yes, the buffer payload is new, and it uses a new BLF flag that
indicates it contains regions with some new on-disk format. And
there are interactions with fsync and data integrity requirements.
Document them!

> > validate them) at recovery time, and also have a clear explaination
> > of the data vs metadata ordering algorithms that ensures that
> > checksums are always valid in crash+recovery situations, especially
> > w.r.t. data integrity operations like fsync.
> 
> I think I explained it pretty well,

You didn't, and that's the problem I am asking you to address.

> but happy to repeat it again:
> The zoned write path writes data first, and then records bmap, rmap
> and used space tacking in the zone from the I/O completion handler.
> The rtcsum code builds on that and only logs that csum from that
> same I/O completion handler.  I.e. that data must have reached the
> device for the code to log it to be even called, and for devices
> with volatile write caches the generic cache flushing must work
> (it did not until recently, but the verification of this code found
> that bug and it is now fixed upstream).

So document the design in the first patch in the series! In detail,
in the tree alongside the code. That gives all reviewers the
necessary context to understand what comes next in the patchset.

Indeed, I ask this not just for reviewiers, but for anyone coming
along in the future that needs to understand how data checksums work
because they need to triage a bug, add new functionality, need to
optimise for performance, etc. Just because you understand how it
works and what you wrote in the commit messages, it doesn't mean how
it is supposed to work, why it works, or why it was implemented in
the way it was is obvious to anyone else.

Documenting the design helps -everyone-, not just now, but well into
the future as well.

-Dave.
-- 
Dave Chinner
dgc@kernel.org