Re: [PATCH 2/5] xfs: implement live quotacheck inode scan

Christoph Hellwig
2024-01-05T05:29:17Z

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?

> +#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.

> + /* 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?

> +/*
> + * 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.

> +/* 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;