xfs
[Top] [All Lists]

Re: [PATCH] xfs: growfs: don't read garbage for new secondary superblock

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH] xfs: growfs: don't read garbage for new secondary superblocks
From: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
Date: Mon, 8 Oct 2012 14:11:44 -0300
In-reply-to: <1349697436-14203-1-git-send-email-david@xxxxxxxxxxxxx>
Mail-followup-to: xfs@xxxxxxxxxxx
References: <1349697436-14203-1-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Oct 08, 2012 at 10:57:16PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> When updating new secondary superblocks in a growfs operation, the
> sueprblock buffer is read from the newly grown region of the
> underlying device. This is not guaranteed to be zero, so violates
> the underlying assumption that the unused parts of superblocks are
> zero filled. Get a new buffer for these secondary superblocks to
> ensure that the unused regions are zero filled correctly.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_fsops.c |   21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index c25b094..4beaede 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -399,9 +399,26 @@ xfs_growfs_data_private(
>  
>       /* update secondary superblocks. */
>       for (agno = 1; agno < nagcount; agno++) {
> -             error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
> +             error = 0;
> +             /*
> +              * new secondary superblocks need to be zeroed, not read from
> +              * disk as the contents of the new area we are growing into is
> +              * completely unknown.
> +              */
> +             if (agno < oagcount) {
> +                     error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
>                                 XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
>                                 XFS_FSS_TO_BB(mp, 1), 0, &bp);
> +             } else {
> +                     bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
> +                               XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
> +                               XFS_FSS_TO_BB(mp, 1), 0);
> +                     if (bp)
> +                             xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
> +                     else
> +                             error = ENOMEM;
> +             }
> +
>               if (error) {
>                       xfs_warn(mp,
>               "error %d reading secondary superblock for ag %d",
> @@ -423,7 +440,7 @@ xfs_growfs_data_private(
>                       break; /* no point in continuing */
>               }
>       }
> -     return 0;
> +     return error;
>  
>   error0:
>       xfs_trans_cancel(tp, XFS_TRANS_ABORT);
> -- 
> 1.7.10
> 

Looks good.

Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>

-- 
--Carlos

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