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@xxxxxxxxxxxxx
|