xfs
[Top] [All Lists]

Re: [PATCH 18/47] xfs: refactor redo intent item processing

To: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
Subject: Re: [PATCH 18/47] xfs: refactor redo intent item processing
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 1 Aug 2016 01:10:23 -0700
Cc: david@xxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, vishal.l.verma@xxxxxxxxx, bfoster@xxxxxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <146907708421.25461.405239727630066080.stgit@xxxxxxxxxxxxxxxx>
References: <146907695530.25461.3225785294902719773.stgit@xxxxxxxxxxxxxxxx> <146907708421.25461.405239727630066080.stgit@xxxxxxxxxxxxxxxx>
User-agent: Mutt/1.6.1 (2016-04-27)
> +int
> +xfs_efi_recover(
> +     struct xfs_mount        *mp,
> +     struct xfs_efi_log_item *efip)
> +{
> +     struct xfs_efd_log_item *efdp;
> +     struct xfs_trans        *tp;
> +     int                     i;
> +     int                     error = 0;
> +     xfs_extent_t            *extp;
> +     xfs_fsblock_t           startblock_fsb;
> +
> +     ASSERT(!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags));
> +
> +     /*
> +      * First check the validity of the extents described by the
> +      * EFI.  If any are bad, then assume that all are bad and
> +      * 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)) {
> +                     /*
> +                      * This will pull the EFI from the AIL and
> +                      * free the memory associated with it.
> +                      */
> +                     set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
> +                     xfs_efi_release(efip);
> +                     return -EIO;
> +             }
> +     }

I know it's just a code move, but there are lots of superflous braces
here, please remove them.

> +     error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> +     if (error)
> +             return error;
> +     efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
> +
> +     for (i = 0; i < efip->efi_format.efi_nextents; i++) {
> +             extp = &(efip->efi_format.efi_extents[i]);

and here..

> +             ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0);

This new check seems useful, but nothing in the changelog mentions
why it has been added.

Otherwise this looks fine to me.

<Prev in Thread] Current Thread [Next in Thread>