Re: [PATCH V8 14/19] xfs: Introduce per-inode 64-bit extent counters

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 21, 2022 at 10:47:45AM +0530, Chandan Babu R wrote:
> This commit introduces new fields in the on-disk inode format to support
> 64-bit data fork extent counters and 32-bit attribute fork extent
> counters. The new fields will be used only when an inode has
> XFS_DIFLAG2_NREXT64 flag set. Otherwise we continue to use the regular 32-bit
> data fork extent counters and 16-bit attribute fork extent counters.
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>

Looks good, minor nits below.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

> +STATIC int
> +xlog_dinode_verify_extent_counts(
> +	struct xfs_mount	*mp,
> +	struct xfs_log_dinode	*ldip)
> +{
> +	xfs_extnum_t		nextents;
> +	xfs_aextnum_t		anextents;
> +
> +	if (xfs_log_dinode_has_large_extent_counts(ldip)) {
> +		if (!xfs_has_large_extent_counts(mp) ||
> +		    (ldip->di_nrext64_pad != 0)) {
> +			XFS_CORRUPTION_ERROR(
> +				"Bad log dinode large extent count format",
> +				XFS_ERRLEVEL_LOW, mp, ldip, sizeof(*ldip));
> +			xfs_alert(mp,
> +				"Bad inode 0x%llx, nrext64 %d, padding 0x%x",

"large extent counts" rather than "nrext64"?

> +				ldip->di_ino, xfs_has_large_extent_counts(mp),
> +				ldip->di_nrext64_pad);
> +			return -EFSCORRUPTED;
> +		}
> +
> +		nextents = ldip->di_big_nextents;
> +		anextents = ldip->di_big_anextents;
> +	} else {
> +		if (ldip->di_version == 3 && ldip->di_v3_pad != 0) {
> +			XFS_CORRUPTION_ERROR(
> +				"Bad log dinode di_v3_pad",
> +				XFS_ERRLEVEL_LOW, mp, ldip, sizeof(*ldip));
> +			xfs_alert(mp,
> +				"Bad inode 0x%llx, di_v3_pad %llu",
> +				ldip->di_ino, ldip->di_v3_pad);

the padding should be dumped as a hexadecimal value - it's much
easier to see the bit corruption patterns in hex dumps than decimal
output.

> +			return -EFSCORRUPTED;
> +		}
> +
> +		nextents = ldip->di_nextents;
> +		anextents = ldip->di_anextents;
> +	}
> +
> +	if (unlikely(nextents + anextents > ldip->di_nblocks)) {
> +		XFS_CORRUPTION_ERROR("Bad log dinode extent counts",
> +				XFS_ERRLEVEL_LOW, mp, ldip, sizeof(*ldip));
> +		xfs_alert(mp,
> +			"Bad inode 0x%llx, nextents 0x%llx, anextents 0x%x, nblocks 0x%llx",
> +			ldip->di_ino, nextents, anextents, ldip->di_nblocks);
> +		return -EFSCORRUPTED;

Maybe should indicate large extent count status here, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com