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?
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()?
> @@ -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?
> + 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.
> + __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.
> +
> + 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())
> 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...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
