xfs
[Top] [All Lists]

Re: [PATCH 3/6] xfs: inode unlinked list needs to recalculate the inode

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/6] xfs: inode unlinked list needs to recalculate the inode CRC
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Mon, 03 Jun 2013 14:18:43 -0400
Cc: xfs@xxxxxxxxxxx, bpm@xxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1370237332-24757-4-git-send-email-david@xxxxxxxxxxxxx>
References: <1370237332-24757-1-git-send-email-david@xxxxxxxxxxxxx> <1370237332-24757-4-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6
On 06/03/2013 01:28 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The inode unlinked list manipulations operate directly on the inode
> buffer, and so bypass the inode CRC calculation mechanisms. Hence an
> inode on the unlinked list has an invalid CRC. Fix this by
> recalculating the CRC whenever we modify an unlinked list pointer in
> an inode, ncluding during log recovery. This is trivial to do and
> results in  unlinked list operations always leaving a consistent
> inode in the buffer.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_inode.c       |   16 ++++++++++++++++
>  fs/xfs/xfs_log_recover.c |    9 +++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index efbe1ac..c50e785 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1638,6 +1638,10 @@ xfs_iunlink(
>               dip->di_next_unlinked = agi->agi_unlinked[bucket_index];
>               offset = ip->i_imap.im_boffset +
>                       offsetof(xfs_dinode_t, di_next_unlinked);
> +
> +             /* need to recalc the inode CRC if appropriate */
> +             xfs_dinode_calc_crc(mp, dip);
> +
>               xfs_trans_inode_buf(tp, ibp);
>               xfs_trans_log_buf(tp, ibp, offset,
>                                 (offset + sizeof(xfs_agino_t) - 1));
> @@ -1723,6 +1727,10 @@ xfs_iunlink_remove(
>                       dip->di_next_unlinked = cpu_to_be32(NULLAGINO);
>                       offset = ip->i_imap.im_boffset +
>                               offsetof(xfs_dinode_t, di_next_unlinked);
> +
> +                     /* need to recalc the inode CRC if appropriate */
> +                     xfs_dinode_calc_crc(mp, dip);
> +
>                       xfs_trans_inode_buf(tp, ibp);
>                       xfs_trans_log_buf(tp, ibp, offset,
>                                         (offset + sizeof(xfs_agino_t) - 1));
> @@ -1796,6 +1804,10 @@ xfs_iunlink_remove(
>                       dip->di_next_unlinked = cpu_to_be32(NULLAGINO);
>                       offset = ip->i_imap.im_boffset +
>                               offsetof(xfs_dinode_t, di_next_unlinked);
> +
> +                     /* need to recalc the inode CRC if appropriate */
> +                     xfs_dinode_calc_crc(mp, dip);
> +
>                       xfs_trans_inode_buf(tp, ibp);
>                       xfs_trans_log_buf(tp, ibp, offset,
>                                         (offset + sizeof(xfs_agino_t) - 1));
> @@ -1809,6 +1821,10 @@ xfs_iunlink_remove(
>               last_dip->di_next_unlinked = cpu_to_be32(next_agino);
>               ASSERT(next_agino != 0);
>               offset = last_offset + offsetof(xfs_dinode_t, di_next_unlinked);
> +
> +             /* need to recalc the inode CRC if appropriate */
> +             xfs_dinode_calc_crc(mp, dip);
> +

Ugh, sorry I didn't notice this last time around, but this one looks
like it should recalculate the crc on last_dip instead of dip.

Brian

>               xfs_trans_inode_buf(tp, last_ibp);
>               xfs_trans_log_buf(tp, last_ibp, offset,
>                                 (offset + sizeof(xfs_agino_t) - 1));
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 83088d9..45a85ff 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1912,6 +1912,15 @@ xlog_recover_do_inode_buffer(
>               buffer_nextp = (xfs_agino_t *)xfs_buf_offset(bp,
>                                             next_unlinked_offset);
>               *buffer_nextp = *logged_nextp;
> +
> +             /*
> +              * If necessary, recalculate the CRC in the on-disk inode. We
> +              * have to leave the inode in a consistent state for whoever
> +              * reads it next....
> +              */
> +             xfs_dinode_calc_crc(mp, (struct xfs_dinode *)
> +                             xfs_buf_offset(bp, i * mp->m_sb.sb_inodesize));
> +
>       }
>  
>       return 0;
> 

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