Re: [PATCH 1/7] factor out xfs_read_agi helper

[Dave Chinner <david@fromorbit.com>]

On Mon, Oct 27, 2008 at 09:39:01AM -0400, Christoph Hellwig wrote:
> Add a helper to read the AGI header and perform basic verification.
> Based on hunks from a larger patch from Dave Chinner.
> 
> (First sent on Juli 23rd)

Couple of small things.

> @@ -1882,45 +1862,23 @@ xfs_iunlink_remove(
>  	short		bucket_index;
>  	int		offset, last_offset = 0;
>  	int		error;
> -	int		agi_ok;
>  
> -	/*
> -	 * First pull the on-disk inode from the AGI unlinked list.
> -	 */
>  	mp = tp->t_mountp;
> -
>  	agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
> -	agdaddr = XFS_AG_DADDR(mp, agno, XFS_AGI_DADDR(mp));
>  
>  	/*
>  	 * Get the agi buffer first.  It ensures lock ordering
>  	 * on the list.
>  	 */
> -	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp, agdaddr,
> -				   XFS_FSS_TO_BB(mp, 1), 0, &agibp);
> +	error = xfs_read_agi(mp, tp, agno, &agibp);
>  	if (error) {
>  		cmn_err(CE_WARN,
> -			"xfs_iunlink_remove: xfs_trans_read_buf()  returned an error %d on %s.  Returning error.",
> +			"xfs_iunlink_remove: xfs_read_agi() returned an error %d on %s. Returning error.",
>  			error, mp->m_fsname);
>  		return error;
>  	}

Do we need this warning here? xfs_read_agi() will have already
issued an error, right? Also, xfs_fs_cmn_err() is probably better
to use here rather than manually encoding the fsname into the error
message....

> @@ -3295,22 +3297,18 @@ xlog_recover_process_iunlinks(
>  
>  				/*
>  				 * Reacquire the agibuffer and continue around
> -				 * the loop.
> +				 * the loop. This should never fail as we know
> +				 * the buffer was good earlier on.
>  				 */
> -				agibp = xfs_buf_read(mp->m_ddev_targp,
> -						XFS_AG_DADDR(mp, agno,
> -							XFS_AGI_DADDR(mp)),
> -						XFS_FSS_TO_BB(mp, 1), 0);
> -				if (XFS_BUF_ISERROR(agibp)) {
> -					xfs_ioerror_alert(
> -				"xlog_recover_process_iunlinks(#2)",
> -						log->l_mp, agibp,
> -						XFS_AG_DADDR(mp, agno,
> -							XFS_AGI_DADDR(mp)));
> +				error = xfs_read_agi(mp, NULL, agno, &agibp);
> +				ASSERT(error == 0);
> +				if (error) {
> +					xfs_fs_cmn_err(CE_ALERT, mp,
> +					"xlog_recover_process_iunlinks(#2)"
> +					"agi read failed agno %d error %d",
> +							agno, error);

Move the assert into the if (error) branch after the message is
logged so that it is clear the reason for the assert failure.

Otherwise seems fine.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com