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

[Chandan Babu R <chandan.babu@oracle.com>]

On 25 Mar 2022 at 03:23, Dave Chinner wrote:
> 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.
>

I agree. I will make the suggested change.

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

Yes, I think it will make it easier to determine the inode fields of the two
extent counts.

-- 
chandan