Re: [PATCH 6/8] xfs: use accessor functions for bitmap words

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Oct 12, 2023 at 08:19:16AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 11, 2023 at 11:07:48AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Create get and set functions for rtbitmap words so that we can redefine
> > the ondisk format with a specific endianness.  Note that this requires
> > the definition of a distinct type for ondisk rtbitmap words so that the
> > compiler can perform proper typechecking as we go back and forth.
> > 
> > In the upcoming rtgroups feature, we're going to fix the problem that
> > rtwords are written in host endian order, which means we'll need the
> > distinct rtword/rtword_raw types.
> 
> I've been looking over this and I have to say I kinda hate the
> abstraction level.
> 
> Having to deal with both the union xfs_rtword_ondisk, and the
> normal in-memory rtword just feels cumbersome.
> 
> I'd go for an API that gets/sets the values based on [bp, word] indices
> instead.  That would also need helpers for logging the buffer ranges
> based on indices, which seems helpful for the code quality anyway.
> 
> I don't really want to burden that on you and would offer to do that
> work myself after we work before this merged.

Hmm, so you want to go from:

	union xfs_rtword_ondisk *start, *end, *b;

	start = b = xfs_rbmblock_wordptr(bp, startword);
	end = xfs_rbmblock_wordptr(bp, endword);

	while (b < end) {
		somevalue = xfs_rtbitmap_getword(mp, b);
		somevalue |= somemask;
		xfs_rtbitmap_setword(mp, b, somevalue);
		b++;
	}

	xfs_trans_log_buf(tp, bp, start - bp->b_addr, b - bp->b_addr);

to something like:

	for (word = startword; word <= endword; word++) {
		somevalue = xfs_rtbitmap_getword(mp, b);
		somevalue |= somemask;
		xfs_rtbitmap_setword(mp, bp, word, somevalue);
	}
	xfs_rtbitmap_log_buf(tp, bp, startword, endword);

I think that could be done with relatively little churn, though it's
unfortunate that the second version does 2x(shift + addition) each time
it goes through the loop instead of the pointer increment that the first
version does.

--D