Re: [PATCH 1/2] xfs: always init fdblocks in mount

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Mar 16, 2020 at 09:07:07PM +0800, Zheng Bin wrote:
> Use fuzz(hydra) to test XFS and automatically generate
> tmp.img(XFS v5 format, but some metadata is wrong)
> 
> xfs_repair information(just one AG):
> agf_freeblks 0, counted 3224 in ag 0
> agf_longest 0, counted 3224 in ag 0
> sb_fdblocks 3228, counted 3224
> 
> Test as follows:
> mount tmp.img tmpdir
> cp file1M tmpdir
> sync
> 
> In 4.19-stable, sync will stuck, while in linux-next, sync not stuck.
> The reason is same to commit d0c7feaf8767
> ("xfs: add agf freeblocks verify in xfs_agf_verify"), cause agf_longest
> is 0, we can not block this in xfs_agf_verify.

Uh.... what are you saying here?  That the allocator misbehaves and
loops forever if sb_fdblocks > sum(agf_freeblks) after mount?

Also, uh, what do you mean by "sync not stuck"?  Writeback will fail on
allocation error, right...?  So I think the problem with incorrect AGF
contents (on upstream) is that writeback will fail due to ENOSPC, which
should never happen under normal circumstance?

> 
> Make sure fdblocks is always inited in mount(also init ifree, icount).
> 
> xfs_mountfs
>   xfs_check_summary_counts
>     xfs_initialize_perag_data
> 
> Signed-off-by: Zheng Bin <zhengbin13@huawei.com>
> ---
>  fs/xfs/xfs_mount.c | 33 ---------------------------------
>  1 file changed, 33 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index c5513e5..dc41801 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -594,39 +594,6 @@ xfs_check_summary_counts(
>  		return -EFSCORRUPTED;
>  	}
> 
> -	/*
> -	 * Now the log is mounted, we know if it was an unclean shutdown or
> -	 * not. If it was, with the first phase of recovery has completed, we
> -	 * have consistent AG blocks on disk. We have not recovered EFIs yet,
> -	 * but they are recovered transactionally in the second recovery phase
> -	 * later.
> -	 *
> -	 * If the log was clean when we mounted, we can check the summary
> -	 * counters.  If any of them are obviously incorrect, we can recompute
> -	 * them from the AGF headers in the next step.
> -	 */
> -	if (XFS_LAST_UNMOUNT_WAS_CLEAN(mp) &&
> -	    (mp->m_sb.sb_fdblocks > mp->m_sb.sb_dblocks ||
> -	     !xfs_verify_icount(mp, mp->m_sb.sb_icount) ||
> -	     mp->m_sb.sb_ifree > mp->m_sb.sb_icount))
> -		xfs_fs_mark_sick(mp, XFS_SICK_FS_COUNTERS);
> -
> -	/*
> -	 * We can safely re-initialise incore superblock counters from the
> -	 * per-ag data. These may not be correct if the filesystem was not
> -	 * cleanly unmounted, so we waited for recovery to finish before doing
> -	 * this.
> -	 *
> -	 * If the filesystem was cleanly unmounted or the previous check did
> -	 * not flag anything weird, then we can trust the values in the
> -	 * superblock to be correct and we don't need to do anything here.
> -	 * Otherwise, recalculate the summary counters.
> -	 */
> -	if ((!xfs_sb_version_haslazysbcount(&mp->m_sb) ||
> -	     XFS_LAST_UNMOUNT_WAS_CLEAN(mp)) &&
> -	    !xfs_fs_has_sickness(mp, XFS_SICK_FS_COUNTERS))
> -		return 0;
> -
>  	return xfs_initialize_perag_data(mp, mp->m_sb.sb_agcount);

The downside of this is that now we /always/ have to make two trips
around all of the AGs at mount time.  If you're proposing to require a
fresh fdblocks recomputation at mount, could you please refactor all of
the mount-time AG walks into a single loop?  And perhaps use xfs_pwork
so that we don't have to do it serially?

--D

>  }
> 
> --
> 2.7.4
>