On Fri, Nov 24, 2023 at 03:53:25PM -0800, Darrick J. Wong wrote:
> +static int
> +xrep_bmap_extent_cmp(
> + const void *a,
> + const void *b)
> +{
> + xfs_fileoff_t ao;
> + xfs_fileoff_t bo;
> +
> + ao = xfs_bmbt_disk_get_startoff((struct xfs_bmbt_rec *)a);
> + bo = xfs_bmbt_disk_get_startoff((struct xfs_bmbt_rec *)b);
It would be nice if we could just have local variables for the
xfs_bmbt_recs and not need casts. I guess for that
xfs_bmbt_disk_get_startoff would need to take a const argument?
Probably something for later.
> + if (whichfork == XFS_ATTR_FORK)
> + return 0;
Nit: I'd probably just split the data fork specific validation
into a separate helper to keep things nicely organized.
> + /*
> + * No need to waste time scanning for shared extents if the inode is
> + * already marked.
> + */
> + if (whichfork == XFS_DATA_FORK && xfs_is_reflink_inode(sc->ip))
> + rb->shared_extents = true;
The comment doesn't seem to match the code.
> +/*
> + * Try to reserve more blocks for a transaction.
> + *
> + * This is for callers that need to attach resources to a transaction, scan
> + * those resources to determine the space reservation requirements, and then
> + * modify the attached resources. In other words, online repair. This can
> + * fail due to ENOSPC, so the caller must be able to cancel the transaction
> + * without shutting down the fs.
> + */
> +int
> +xfs_trans_reserve_more(
> + struct xfs_trans *tp,
> + unsigned int blocks,
> + unsigned int rtextents)
This basically seems to duplicate xfs_trans_reserve except that it skips
the log reservation. What about just allowing to pass a NULL resp
agument to xfs_trans_reserve to skip the log reservation and reuse the
code?
Otherwise this looks good to me.
