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

Darrick J. Wong
2024-02-28T19:55:33Z

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@lst.de>

Thanks!

--D