On 04 Mar 2022 at 12:44, Dave Chinner wrote:
> On Tue, Mar 01, 2022 at 04:09:33PM +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@xxxxxxxxxx>
>> Signed-off-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
>> Suggested-by: Dave Chinner <dchinner@xxxxxxxxxx>
>> ---
>> fs/xfs/libxfs/xfs_format.h | 33 ++++++++++++--
>> fs/xfs/libxfs/xfs_inode_buf.c | 49 ++++++++++++++++++--
>> fs/xfs/libxfs/xfs_inode_fork.h | 6 +++
>> fs/xfs/libxfs/xfs_log_format.h | 33 ++++++++++++--
>> fs/xfs/xfs_inode_item.c | 23 ++++++++--
>> fs/xfs/xfs_inode_item_recover.c | 79 ++++++++++++++++++++++++++++-----
>> 6 files changed, 196 insertions(+), 27 deletions(-)
>
> .....
>
>> +static xfs_failaddr_t
>> +xfs_dinode_verify_nrext64(
>> + struct xfs_mount *mp,
>> + struct xfs_dinode *dip)
>> +{
>> + if (xfs_dinode_has_nrext64(dip)) {
>> + if (!xfs_has_nrext64(mp))
>> + return __this_address;
>> + if (dip->di_nrext64_pad != 0)
>> + return __this_address;
>> + } else if (dip->di_version >= 3) {
>> + if (dip->di_v3_pad != 0)
>> + return __this_address;
>> + }
>> +
>> + return NULL;
>> +}
>
> Shouldn't this also check that di_v2_pad is zero if it's a v2 inode?
>
xfs_dinode_verify_nrext64() is meant for checking only those parts of an inode
that are influenced by "large extent counters" feature. Hence, I don't think
we should check di_v2_pad field in this function.
> Also, this isn't verifying the actual extent count range. Maybe
> that's done somewhere else now, and if so, shouldn't we move all the
> extent count verification checks into a single function called,
> say, xfs_dinode_verify_extent_counts()?
>
Validation of extent count had been performed by xfs_dinode_verify_fork(). I
think it still continues to be the right place for validating extent counts
since they are per-fork attributes.
>> @@ -348,21 +366,60 @@ xlog_recover_inode_commit_pass2(
>> goto out_release;
>> }
>> }
>> - if (unlikely(ldip->di_nextents + ldip->di_anextents > ldip->di_nblocks)){
>> - XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(5)",
>> +
>> + if (xfs_log_dinode_has_nrext64(ldip)) {
>> + if (!xfs_has_nrext64(mp) || (ldip->di_nrext64_pad != 0)) {
>> + XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(5)",
>
> Can we have a meaningful error like "Bad log dinode large extent
> count format" rather than something we have to go look up the source
> code to understand when someone reports a problem?
>
Ok. I will change the error message to apply the above review.
>> + XFS_ERRLEVEL_LOW, mp, ldip,
>> + sizeof(*ldip));
>> + xfs_alert(mp,
>> + "%s: Bad inode log record, rec ptr "PTR_FMT", "
>> + "dino ptr "PTR_FMT", dino bp "PTR_FMT", "
>> + "ino %Ld, xfs_has_nrext64(mp) = %d, "
>> + "ldip->di_nrext64_pad = %u",
>
> What's the point of printing pointers here? Just print the inode
> number and the bad values - we log the pointers in the
> the log recovery tracepoints so there's no need to print them in
> user facing errors because we can't do anything with them without a
> debugger attached.
>
> Hence we really only need to dump the inode number and the bad extent
> format information - we already have the error context/location from
> the corruption error report above. Hence all we need here is:
>
> xfs_alert(mp,
> "Bad inode 0x%llx, nrext64 %d, padding 0x%x"
> in_f->ilf_ino, xfs_has_nrext64(mp).
> ldip->di_nrext64_pad);
>
> The other new alerts can be cleaned up like this, too.
>
Ok. I will clean it up.
>> + __func__, item, dip, bp, in_f->ilf_ino,
>> + xfs_has_nrext64(mp), ldip->di_nrext64_pad);
>> + error = -EFSCORRUPTED;
>> + goto out_release;
>> + }
>> + } else {
>> + if (ldip->di_version == 3 && ldip->di_big_nextents != 0) {
>> + XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(6)",
>> + XFS_ERRLEVEL_LOW, mp, ldip,
>> + sizeof(*ldip));
>> + xfs_alert(mp,
>> + "%s: Bad inode log record, rec ptr "PTR_FMT", "
>> + "dino ptr "PTR_FMT", dino bp "PTR_FMT", "
>> + "ino %Ld, ldip->di_big_dextcnt = %llu",
>> + __func__, item, dip, bp, in_f->ilf_ino,
>> + ldip->di_big_nextents);
>> + error = -EFSCORRUPTED;
>> + goto out_release;
>> + }
>> + }
>> +
>> + if (xfs_log_dinode_has_nrext64(ldip)) {
>> + nextents = ldip->di_big_nextents;
>> + anextents = ldip->di_big_anextents;
>> + } else {
>> + nextents = ldip->di_nextents;
>> + anextents = ldip->di_anextents;
>> + }
>
> Also, this can be put in the above if statements, it does not need
> a separate identical if clause.
I agree. I will move these assignments to the previous if/else statement.
>> +
>> + if (unlikely(nextents + anextents > ldip->di_nblocks)) {
>> + XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
>> XFS_ERRLEVEL_LOW, mp, ldip,
>> sizeof(*ldip));
>> xfs_alert(mp,
>> "%s: Bad inode log record, rec ptr "PTR_FMT", dino ptr "PTR_FMT", "
>> - "dino bp "PTR_FMT", ino %Ld, total extents = %d, nblocks = %Ld",
>> + "dino bp "PTR_FMT", ino %Ld, total extents = %llu, nblocks = %Ld",
>> __func__, item, dip, bp, in_f->ilf_ino,
>> - ldip->di_nextents + ldip->di_anextents,
>> - ldip->di_nblocks);
>> + nextents + anextents, ldip->di_nblocks);
>> error = -EFSCORRUPTED;
>> goto out_release;
>> }
>
> ALso, I think that xlog_recover_inode_commit_pass2() is already too
> big without adding this new verification to it. Can we factor this
> into a separate function (say xlog_dinode_verify_extent_counts())
>
Sure. I will move extent count validation into a new function.
>> if (unlikely(ldip->di_forkoff > mp->m_sb.sb_inodesize)) {
>> - XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(6)",
>> + XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(8)",
>> XFS_ERRLEVEL_LOW, mp, ldip,
>> sizeof(*ldip));
>> xfs_alert(mp,
>> @@ -374,7 +431,7 @@ xlog_recover_inode_commit_pass2(
>> }
>> isize = xfs_log_dinode_size(mp);
>> if (unlikely(item->ri_buf[1].i_len > isize)) {
>> - XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
>> + XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(9)",
>> XFS_ERRLEVEL_LOW, mp, ldip,
>> sizeof(*ldip));
>> xfs_alert(mp,
>
> And this is exactly why I don't like these numbered warnings. Make
> the warning descriptive rather than numbered -
> changing/adding/removing a warning shouldn't force us to change a
> bunch of unrelated warninngs...
I will write a new patch to replace these numbered warnings with descriptive
ones.
--
chandan
