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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;





[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux