On Thu, Jan 04, 2024 at 09:29:16PM -0800, Christoph Hellwig wrote:
> Looks good,
>
> but a few nitpick below:
>
> > +int
> > +xchk_trans_alloc_empty(
> > + struct xfs_scrub *sc)
> > +{
> > + return xfs_trans_alloc_empty(sc->mp, &sc->tp);
> > +}
>
> Can this and the conversion of an existing not quota related caller
> of xfs_trans_alloc_empty be split into a separate patch that also
> documents why this pretty trivial helper is useful?
Done.
> > +#ifdef CONFIG_XFS_QUOTA
> > +void xchk_qcheck_set_corrupt(struct xfs_scrub *sc, unsigned int dqtype,
> > + xfs_dqid_t id);
> > +#endif /* CONFIG_XFS_QUOTA */
>
> No need for the ifdef here.
Fixed.
> > + /* Figure out the data / rt device block counts. */
> > + xfs_ilock(ip, XFS_IOLOCK_SHARED);
> > + if (isreg)
> > + xfs_ilock(ip, XFS_MMAPLOCK_SHARED);
> > + if (XFS_IS_REALTIME_INODE(ip)) {
> > + ilock_flags = xfs_ilock_data_map_shared(ip);
> > + error = xfs_iread_extents(tp, ip, XFS_DATA_FORK);
> > + if (error)
> > + goto out_incomplete;
> > + } else {
> > + ilock_flags = XFS_ILOCK_SHARED;
> > + xfs_ilock(ip, XFS_ILOCK_SHARED);
> > + }
>
> The need to call xfs_iread_extents only for RT inodes here look good,
> but I guess it is explained by the logic in xfs_inode_count_blocks.
> Maybe add a comment?
/*
* Read in the data fork for rt files so that _count_blocks
* can count the number of blocks allocated from the rt volume.
* Inodes do not track that separately.
*/
> > +/*
> > + * Load an array element, but zero the buffer if there's no data because we
> > + * haven't stored to that array element yet.
> > + */
> > +static inline int
> > +xfarray_load_sparse(
> > + struct xfarray *array,
> > + uint64_t idx,
> > + void *rec)
> > +{
> > + int error = xfarray_load(array, idx, rec);
> > +
> > + if (error == -ENODATA) {
> > + memset(rec, 0, array->obj_size);
> > + return 0;
> > + }
> > + return error;
> > +}
>
> Please split this into a separate prep patch.
Done.
> > +/* Compute the number of data and realtime blocks used by a file. */
> > +void
> > +xfs_inode_count_blocks(
> > + struct xfs_trans *tp,
> > + struct xfs_inode *ip,
> > + xfs_filblks_t *dblocks,
> > + xfs_filblks_t *rblocks)
> > +{
> > + struct xfs_ifork *ifp = xfs_ifork_ptr(ip, XFS_DATA_FORK);
> > +
> > + if (!XFS_IS_REALTIME_INODE(ip)) {
> > + *dblocks = ip->i_nblocks;
> > + *rblocks = 0;
> > + return;
> > + }
> > +
> > + *rblocks = 0;
> > + xfs_bmap_count_leaves(ifp, rblocks);
> > + *dblocks = ip->i_nblocks - *rblocks;
> > +}
>
> Same for this one. The flow here also reads a little odd to me,
> what speaks against:
>
> *rblocks = 0;
> if (XFS_IS_REALTIME_INODE(ip))
> xfs_bmap_count_leaves(&ip->i_df, rblocks);
> *dblocks = ip->i_nblocks - *rblocks;
Yeah, that is more tidy. Thanks for the suggestion, I'll incorporate
that.
--D
