On Wed, Nov 29, 2023 at 09:07:56PM -0800, Christoph Hellwig wrote:
> 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.
Oh! Apparently xfs_bmbt_disk_get_startoff does take a const struct
pointer now.
> > + 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.
Ooof, that state handling needs tightening up. There are three states,
really -- "irrelevant to this repair", "set the iflag", and "no idea, do
a scan". That first state is open-coded in _discover_shared, which is
wasteful because that is decidable in xrep_bmap.
>
> > +/*
> > + * 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?
Hmm. Maybe not a NULL resp, but an empty one looks like it would work
fine with less code duplication.
> Otherwise this looks good to me.
Cool! Thanks for reviewing!
--D
