Re: [PATCH 08/47] xfs: support btrees with overlapping intervals for keys

[Christoph Hellwig <hch@infradead.org>]

On Mon, Aug 01, 2016 at 12:11:26PM -0700, Darrick J. Wong wrote:
> > > +/*
> > > + * In-core key that holds both low and high keys for overlapped btrees.
> > > + * The two keys are packed next to each other on disk, so do the same
> > > + * in memory.  Preserve the existing xfs_btree_key as a single key to
> > > + * avoid the mental model breakage that would happen if we passed a
> > > + * bigkey into a function that operates on a single key.
> > > + */
> > > +union xfs_btree_bigkey {
> > > +	struct xfs_bmbt_key		bmbt;
> > > +	xfs_bmdr_key_t			bmbr;	/* bmbt root block */
> > > +	xfs_alloc_key_t			alloc;
> > > +	struct xfs_inobt_key		inobt;
> > > +};
> > 
> > I don't understand the purpose of this union at all, and the comment
> > seems misleading.  Compared to union xfs_btree_key the only difference
> > seems to be that xfs_btree_bigkey is missing the
> > 'struct xfs_rmap_key rmap' member.  How does that enable us to holds
> 
> I think you might be missing a later patch, wherein we add the rmap
> stuff to the btree structures, which expands bigkey to look like this:

Yeah, I was stuck in the middle of tree.  I still think the bigkey
is a very bad idea.  There are only 7 place left that actually
allocate storage for a "union xfs_btree_key".  Everything else uses
fancy pointer arithmetics to get them out of a disk buffer:

 - xfs_btree_lookup
 - xfs_btree_get_leaf_keys_overlapped
 - xfs_btree_update_keys
 - xfs_btree_lshift
 - xfs_btree_rshift
 - xfs_btree_simple_query_range
 - xfs_btree_overlapped_query_range

So just adding the rmap to union xfs_btree_key would simplify things
and remove a potential pitfall at the cost of just a little bit
more stack usage.  And at least of the
init_high_key_from_rec/init_key_from_rec we could probably replace
two on-stack xfs_btree_keys with a single new, bigger xfs_btree_key.

> union xfs_btree_key {
> 	struct xfs_bmbt_key		bmbt;
> 	xfs_bmdr_key_t			bmbr;	/* bmbt root block */
> 	xfs_alloc_key_t			alloc;
> 	struct xfs_inobt_key		inobt;
> 	struct xfs_rmap_key		rmap[2];
> 	struct xfs_refcount_key		refc;
> };
> 
> This gives us the storage we want and avoids casts, but it still
> doesn't fix the problem that sometimes we want to create a key pointer
> to just the high fields and treat that as a pointer.

Where does that problem occur?  I don't quite understand how having
the bigger structure is a problem if we don't want to initialize all
of it.