Re: [PATCH 04/10] xfs: replace shouty XFS_BM{BT,DR} macros

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

On Thu, Aug 29, 2024 at 12:00:43PM +1000, Dave Chinner wrote:
> On Tue, Aug 27, 2024 at 04:34:45PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Replace all the shouty bmap btree and bmap disk root macros with actual
> > functions, and fix a type handling error in the xattr code that the
> > macros previously didn't care about.
> 
> I don't see a type handling fix in the xattr code in the patch.
> 
> If there is one, can you please split it out to make it obvious?

No, I think that's just debris from an old iteration of this years-old
patch.  The xattr fixes got merged in the xattr state machine series
that Allison merged a long time ago.

> ....
> 
> > -#define XFS_BMDR_REC_ADDR(block, index) \
> > -	((xfs_bmdr_rec_t *) \
> > -		((char *)(block) + \
> > -		 sizeof(struct xfs_bmdr_block) + \
> > -	         ((index) - 1) * sizeof(xfs_bmdr_rec_t)))
> 
> > +
> > +static inline struct xfs_bmbt_rec *
> > +xfs_bmdr_rec_addr(
> > +	struct xfs_bmdr_block	*block,
> > +	unsigned int		index)
> > +{
> > +	return (struct xfs_bmbt_rec *)
> > +		((char *)(block + 1) +
> > +		 (index - 1) * sizeof(struct xfs_bmbt_rec));
> > +}
> 
> There's a logic change in these BMDR conversions - why does the new
> version use (block + 1) and the old one use a (block + sizeof())
> calculation?

Pointer arithmetic works great inside C functions where you know the
type of the operand.  That's why I used it.

That doesn't work at all in a macro where someone could pass in any
random pointer and then the math is wrong.  That's why the macro casts
to a byte-sized pointer type and then adds exactly the sizeof() bytes,
or at least I assume that's the motivation of the authors.

> I *think* they are equivalent, but now as I read the code I have to
> think about casts and pointer arithmetic and work out what structure
> we are taking the size of in my head rather than it being straight
> forward and obvious from the code.

The function argument declaration is four lines up.

> It doesn't change the code that is generated, so I think that the
> existing "+ sizeof()" variants is better than this mechanism because
> everyone is familiar with the existing definitions....

I disagree.  With your change, to validate this function, everyone must
to check that the argument type matches the sizeof argument to confirm
that the pointer arithmetic is correct.

static inline struct xfs_bmbt_rec *
xfs_bmdr_rec_addr(
	struct xfs_bmdr_block	*block,
	unsigned int		index)
{
	return (struct xfs_bmbt_rec *)
		((char *)(block) +
		 sizeof(struct xfs_btree_block) +
		 (index - 1) * sizeof(struct xfs_bmbt_rec));
}

Oops, this function is broken, when we could have trusted the compiler
to get the types and the math correct for us.

> > +static inline struct xfs_bmbt_key *
> > +xfs_bmdr_key_addr(
> > +	struct xfs_bmdr_block	*block,
> > +	unsigned int		index)
> > +{
> > +	return (struct xfs_bmbt_key *)
> > +		((char *)(block + 1) +
> > +		 (index - 1) * sizeof(struct xfs_bmbt_key));
> > +}
> > +
> > +static inline xfs_bmbt_ptr_t *
> > +xfs_bmdr_ptr_addr(
> > +	struct xfs_bmdr_block	*block,
> > +	unsigned int		index,
> > +	unsigned int		maxrecs)
> > +{
> > +	return (xfs_bmbt_ptr_t *)
> > +		((char *)(block + 1) +
> > +		 maxrecs * sizeof(struct xfs_bmbt_key) +
> > +		 (index - 1) * sizeof(xfs_bmbt_ptr_t));
> > +}
> 
> Same for these.
> 
> > +/*
> > + * Compute the space required for the incore btree root containing the given
> > + * number of records.
> > + */
> > +static inline size_t
> > +xfs_bmap_broot_space_calc(
> > +	struct xfs_mount	*mp,
> > +	unsigned int		nrecs)
> > +{
> > +	return xfs_bmbt_block_len(mp) + \
> > +	       (nrecs * (sizeof(struct xfs_bmbt_key) + sizeof(xfs_bmbt_ptr_t)));
> > +}
> 
> stray '\' remains in that conversion.

Will fix, thanks.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>