Re: support for RT data checksums

[Dave Chinner <dgc@kernel.org>]

On Mon, Sep 28, 2026 at 07:24:36AM +0200, Christoph Hellwig wrote:
> On Mon, Sep 28, 2026 at 08:59:31AM +1000, Dave Chinner wrote:
> > 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".
> 
> No, I call your meandering writing style slop.

And you think that makes what you said any better? Talk about not
knowing when to stop digging...

How hard is it to say "I don't really understand your concern - can
you clarify what you are concerned about?" instead of calling it
slop?

That's would have been a constructive response, and the discussion
then goes an entirely different (and far more pleasant) way from
there.

There are three lines in the commit message and two in your reply
repeating the same thing: "it's for issuing the checksum block read
in parallel with the data read".

I know that - it's just async readahead of the checksum block
followed by a blocking operation that waits for the readahead to
complete.

What I said above is that this async read IO pattern already exists
in XFS - btree traversals do this same readahead/blocking read
pattern to pull sibling nodes into memory ahead of time as we search
sideways across levels. And they do it within existing transaction
APIs, too.

That is, we already have an API that allows async_read+blocking_read
pairs that wait for async readahead to complete.  It also gathers
errors - if readahead IO fails, the blocking read will reissue the
read IO and gather the error if it fails again.

Hence I'm wanting to know why you chose to duplicate that code and
place it behind a slightly different API with slightly different
semantics instead of just using the existing code with a couple of
small tweaks? Why is this new code better than reusing the existing
code?

My concerns about the checksum buffer transactionsi are about not
using existing APIs. By open coding them, you've skipped all the
transaction/BLI recursion detection. i.e. you've encoded an
assumption that a buffer will never get relogged within a given
transaction. You've also encoded that external values bound
memcpy()s in to the buffer and logging ranges to within the buffer
length, but the buffer length itself is never checked. 

I don't know enough about the design to validate these assumptions,
and there is no documentation (code, comments or commit messages)
describing why the code is safe the way it is written...

We also use wrapper functions to set BLFT type specific BLI flags
(inode bufs, dquot bufs, ordered bufs, etc) along with the BLFT
type. These also include asserts to ensure that we call those
functions appropriately. The csum logging code open codes these
flags/types, and there are no asserts anywhere to indicate incorrect
usage of the new BLI flags that csum buffers use. Why deviate from
the existing BLI patterns and APIs?

IMO, if you're going to implement new transaction and BLI
interactions, you need to document and explain how it all works.
BLI life cycle bugs are still an ongoing source of crashes and UAFs
and I'd really like to make sure that these changes don't make it
impossible to fix the life cycle issues and UAFs they result in.

> > 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.
> 
> Stop this bullshit. Replay to technical details in the patches if you
> want, or wait for the requested document, but don't write weirdly
> halluscinated high-level concerns.

Clearly you haven't understood why I'm disappointed in you.  If you
don't want to deal with "this BS", then -don't be an asshole-. End
of story. 

> > 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!
> 
> No, as explained before and clearly visible even from the full diff
> it does not use any new BLF flag.   See why this discussion is so
> hard?

I meant "BLFT", not BLF - it's just a simple typo. There's no need
to be an asshole over a simple typo, especially as the typo doesn't
materially change what I said (i.e. BFLTs are BLF flags...)

> > Documenting the design helps -everyone-, not just now, but well into
> > the future as well.
> 
> And I've not disagree with this.

But you also haven't agreed to write a design doc yet, either.

Your previous response was pretty negative towards my request -
saying "I think I explained it pretty well" is a fair indication
that you aren't going to write one.

So, are you going to write a design doc or not?

> But next time you think you need one
> just request it, and don't generate pages full of rambling and incorrect
> text.

Four paragraphs to request a design doc and it's scope it is hardly
"pages full of rambling".  Why are you being so obnoxious about
being asked for a design doc?

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