Re: [PATCH 09/21] xfs: add support for per-RTG csum files

[Christoph Hellwig <hch@lst.de>]

On Thu, Sep 24, 2026 at 03:24:42PM -0700, Darrick J. Wong wrote:
> > +	if (XFS_IS_CORRUPT(mp, ifp->if_nextents != 1))
> > +		goto sick;
> > +
> > +	/*
> > +	 * We can do an unlocked lookup here because the bmap btree for the
> > +	 * csum files is immutable once created.
> > +	 */
> 
> This might be problematic if we ever want online repair to be able to
> rebuild the checksum file data fork at runtime.  That would probably
> cause catastrophic loss of file data integrity guarantees, but that
> might be better than the filesystem dying.

repair is still on my todo list, but the plan would be to use a static_call
to introduce looking if/when we have to online repair to add locking.

> > +/* Convert an rgbno to the csum byte position in the csum file. */
> > +static inline xfs_off_t
> > +xfs_rgb_to_rtcsumpos(
> 
> rgbno_to_ ?

Yeah.

> 
> > +	struct xfs_mount	*mp,
> > +	xfs_rtblock_t		rgbno)
> 
> Shouldn't ^^^^ this be an xfs_rgblock_t given the name and comment?

Yes.

> I don't think you support checksums on !rtgroups filesystems, so you'll
> never have to deal with a rt block address larger than 2^32.

Yes, this is zoned only and zoned requires rtgroups.  Even if for some
reason we want to support non-zoned, requiring rtgroups is required for
the per-group file infrastucture.

> 
> > +{
> > +	return (xfs_off_t)rgbno << mp->m_rtcsum_shift;
> 
> Hrm.  What's the difference between this and +xfs_extlen_to_rtcsum_len?
> I guess one describes a quantity of fsblocks, whereas another describes
> an address?

Yes, which implies 32 vs 64bit values.
> > +static inline xfs_filblks_t
> > +xfs_rtcsum_max_len(
> > +	struct xfs_mount	*mp,
> > +	xfs_fsblock_t		fsbno)
> > +{
> > +	return xfs_rtcsum_len_to_extlen(mp,
> > +			mp->m_rtcsum_bsize - xfs_rtb_to_rtcsumoff(mp, fsbno));
> > +}
> 
> What does this do?  Does it compute the number of checksums you can
> write to the rest of a single csum file block given a fsblock address?

Yes, although the limiting factor is reads where the code wants to deal
with a single buffer per read.  For writes the data is logged where
we can trivially loop over buffers.

> > +union xfs_csum {
> > +	uint32_t		crc32c;
> > +	uint64_t		crc64;
> > +};
> > +
> > +static __always_inline void
> > +xfs_csum_seed(
> > +	struct xfs_mount	*mp,
> > +	union xfs_csum		*csum)
> > +{
> > +	if (mp->m_sb.sb_rtcsum_type == XFS_CSUM_TYPE_CRC32C)
> > +		csum->crc32c = XFS_CRC_SEED;
> > +	else
> > +		csum->crc64 = XFS_CRC64_SEED;
> 
> I was expecting a switch() here. ;)

That would require coming up with something for the impossible case,
so I'd rather side-step it.