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

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

On Mon, Aug 01, 2016 at 01:10:23AM -0700, Christoph Hellwig wrote:
> > +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.

Ok.  It took me a while to figure out that you were referring to the
parentheses on the if tests and not the curly braces around the code
blocks.

> > +	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.

I'm adding the following to the commit message:

"Furthermore, ensure that log recovery only replays the redo items
that were in the AIL prior to recovery by checking the item LSN
against the largest LSN seen during log scanning.  As written this
should never happen, but we can be defensive anyway."

--D