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

[Dave Chinner <david@fromorbit.com>]

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?

....

> -#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?

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. 

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....


> +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.

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