Re: [PATCH 2/9] xfs: encode the default bc_flags in the btree ops structure

Darrick J. Wong
2024-01-03T20:00:51Z

On Wed, Jan 03, 2024 at 11:58:26AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 02, 2024 at 05:15:11PM -0800, Darrick J. Wong wrote:
> > On Tue, Jan 02, 2024 at 02:33:34AM -0800, Christoph Hellwig wrote:
> > > On Sun, Dec 31, 2023 at 12:17:28PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > >
> > > > Certain btree flags never change for the life of a btree cursor because
> > > > they describe the geometry of the btree itself. Encode these in the
> > > > btree ops structure and reduce the amount of code required in each btree
> > > > type's init_cursor functions.
> > >
> > > I like the idea, but why are the geom_flags mirrored into bc_flags
> > > instead of beeing kept entirely separate and accessed as
> > > cur->bc_ops->geom_flags which would be a lot easier to follow?
> >
> > Oh! That hadn't occurred to me. Let me take a look at that.
>
> Eeeeyugh, this became kind of a mess. These XFS_BTREE_ flags describe
> btree geometry, are set in the bc_ops->geom_flags, and never change:
>
> 1. XFS_BTREE_LONG_PTRS
> 2. XFS_BTREE_ROOT_IN_INODE
> 3. XFS_BTREE_IROOT_RECORDS /* rt rmap patchset */
> 4. XFS_BTREE_IN_XFILE
> 5. XFS_BTREE_OVERLAPPING
>
> This one flag describes geometry but is set dynamically by
> xfs_btree_alloc_cursor. Some of the geom_flags (rmap, refcount) can set
> it directly too, since they don't exist in V4 filesystems:
>
> 6. XFS_BTREE_CRC_BLOCKS
>
> This one flag doesn't describe btree geometry but never changes and
> could be set in bc_ops->geom_flags:
>
> 7. XFS_BTREE_LASTREC_UPDATE
>
> The remaining flag actually describes per-cursor state:
>
> 8. XFS_BTREE_STAGING
>
> Flags 1-5 can be referenced directly from geom_flags.
>
> Flag 6 could be replaced by an xfs_has_crc call, though I'd bet it's
> cheaper to test a cursor variable than to walk to the xfs_mount and
> test_bit. But this feels weird.
>
> Flag 7 is set in geom_flags as it should be.
>
> Flag 8 is really a runtime flag, so it can stay in bc_flags.
>
> *or* I could rename geom_flags to default_bcflags and make it clearer
> that it's used to seed cur->bc_flags?

*or* I could define a separate struct xfs_btree_ops for the
bnobt/cntbt/inobt/bmbt for V4 filesystems.

--D

>
> --D
>