Re: [PATCH 05/14] xfs: create deferred log items for file mapping exchanges

["Darrick J. Wong" <djwong@xxxxxxxxxx>]

On Wed, Feb 28, 2024 at 07:49:05AM -0800, Christoph Hellwig wrote:
> > -static inline bool xfs_bmap_is_written_extent(struct xfs_bmbt_irec *irec)
> > +static inline bool xfs_bmap_is_written_extent(const struct xfs_bmbt_irec *irec)
> 
> This seems entirely unrelated, can you split it into a cleanup patch?

Done.

> > +		state |= CRIGHT_CONTIG;
> > +	if ((state & CBOTH_CONTIG) == CBOTH_CONTIG &&
> > +	    left->br_startblock + curr->br_startblock +
> > +					right->br_startblock > XFS_MAX_BMBT_EXTLEN)

Oh yikes, that should be br_blockcount, not br_startblock.

> Overly long line here (and pretty weird formatting causing it..)

I'll change it to this helper:

static inline bool
xmi_can_merge_all(
	const struct xfs_bmbt_irec	*l,
	const struct xfs_bmbt_irec	*m,
	const struct xfs_bmbt_irec	*r)
{
	xfs_filblks_t			new_len;

	new_len = l->br_blockcount + m->br_blockcount + r->br_blockcount;
	return new_len <= XFS_MAX_BMBT_EXTLEN;
}

Then the call sites become:

	if ((state & CBOTH_CONTIG) == CBOTH_CONTIG &&
	    !xmi_can_merge_all(left, curr, right))
		state &= ~CRIGHT_CONTIG;

> > +	if ((state & NBOTH_CONTIG) == NBOTH_CONTIG &&
> > +	    left->br_startblock + new->br_startblock +
> > +					right->br_startblock > XFS_MAX_BMBT_EXTLEN)
> 
> Same here.

Here too.

> > +/* XFS-specific parts of file exchanges */
> 
> Well, everything really is XFS-specific :)  I'd drop this comment.

Ok.

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>

Thanks!

--D