Re: [PATCH 07/10] xfs: hoist recovered extent-free intent checks out of xfs_efi_item_recover

Brian Foster
2020-12-04T14:01:52Z

On Thu, Dec 03, 2020 at 05:12:18PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> When we recover a extent-free intent from the log, we need to validate
> its contents before we try to replay them. Hoist the checking code into
> a separate function in preparation to refactor this code to use
> validation helpers.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> fs/xfs/xfs_extfree_item.c | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 6c11bfc3d452..5e0f0b0a6c83 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -578,6 +578,25 @@ const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
> .cancel_item = xfs_extent_free_cancel_item,
> };
>
> +/* Is this recovered EFI ok? */
> +static inline bool
> +xfs_efi_validate_ext(
> + struct xfs_mount *mp,
> + struct xfs_extent *extp)
> +{
> + xfs_fsblock_t startblock_fsb;
> +
> + startblock_fsb = XFS_BB_TO_FSB(mp,
> + XFS_FSB_TO_DADDR(mp, extp->ext_start));
> + if (startblock_fsb == 0 ||
> + extp->ext_len == 0 ||
> + startblock_fsb >= mp->m_sb.sb_dblocks ||
> + extp->ext_len >= mp->m_sb.sb_agblocks)
> + return false;
> +
> + return true;
> +}
> +
> /*
> * Process an extent free intent item that was recovered from
> * the log. We need to free the extents that it describes.
> @@ -592,7 +611,6 @@ xfs_efi_item_recover(
> struct xfs_efd_log_item *efdp;
> struct xfs_trans *tp;
> struct xfs_extent *extp;
> - xfs_fsblock_t startblock_fsb;
> int i;
> int error = 0;
>
> @@ -602,14 +620,11 @@ xfs_efi_item_recover(
> * just toss the EFI.
> */
> for (i = 0; i < efip->efi_format.efi_nextents; i++) {
> - extp = &efip->efi_format.efi_extents[i];
> - startblock_fsb = XFS_BB_TO_FSB(mp,
> - XFS_FSB_TO_DADDR(mp, extp->ext_start));
> - if (startblock_fsb == 0 ||
> - extp->ext_len == 0 ||
> - startblock_fsb >= mp->m_sb.sb_dblocks ||
> - extp->ext_len >= mp->m_sb.sb_agblocks)
> + if (!xfs_efi_validate_ext(mp,
> + &efip->efi_format.efi_extents[i])) {
> + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> + }
> }
>
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
>