xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/7] factor out xfs_read_agi helper
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 28 Oct 2008 16:08:52 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20081027133901.GB1109@xxxxxxxxxxxxx>
Mail-followup-to: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
References: <20081027133901.GB1109@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
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

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