xfs
[Top] [All Lists]

Re: [PATCH 2/9] xfs: fix incorrect remote symlink block count

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/9] xfs: fix incorrect remote symlink block count
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 29 May 2013 12:39:33 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1369636707-15150-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1369636707-15150-1-git-send-email-david@xxxxxxxxxxxxx> <1369636707-15150-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6
On 05/27/2013 02:38 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> When CRCs are enabled, the number of blocks needed to hold a remote
> symlink on a 1k block size filesystem may be 2 instead of 1. The
> transaction reservation for the allocated bloks was not taking this
> into account and only allocating one block. hence when trying to
> read or invalidate such symlinks, we are mapping a hole where there
> should be a block and things go bad at that point.
> 
> Fix the reservation to use the correct block count, clean up the
> block count calculation similar to the remote attribute calculation,
> and add a debug guard to detect when we don't write the entire
> symlink to disk.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

Unrelated question... I noticed we update di_size in xfs_symlink(). I
presume this is safe because, even in the non-local case, we actually
log the data (the path) in the buffers as well, right?

Brian

>  fs/xfs/xfs_symlink.c |   20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 5f234389..195a403 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -56,16 +56,9 @@ xfs_symlink_blocks(
>       struct xfs_mount *mp,
>       int             pathlen)
>  {
> -     int             fsblocks = 0;
> -     int             len = pathlen;
> +     int buflen = XFS_SYMLINK_BUF_SPACE(mp, mp->m_sb.sb_blocksize);
>  
> -     do {
> -             fsblocks++;
> -             len -= XFS_SYMLINK_BUF_SPACE(mp, mp->m_sb.sb_blocksize);
> -     } while (len > 0);
> -
> -     ASSERT(fsblocks <= XFS_SYMLINK_MAPS);
> -     return fsblocks;
> +     return (pathlen + buflen - 1) / buflen;
>  }
>  
>  static int
> @@ -405,7 +398,7 @@ xfs_symlink(
>       if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
>               fs_blocks = 0;
>       else
> -             fs_blocks = XFS_B_TO_FSB(mp, pathlen);
> +             fs_blocks = xfs_symlink_blocks(mp, pathlen);
>       resblks = XFS_SYMLINK_SPACE_RES(mp, link_name->len, fs_blocks);
>       error = xfs_trans_reserve(tp, resblks, XFS_SYMLINK_LOG_RES(mp), 0,
>                       XFS_TRANS_PERM_LOG_RES, XFS_SYMLINK_LOG_COUNT);
> @@ -512,7 +505,7 @@ xfs_symlink(
>               cur_chunk = target_path;
>               offset = 0;
>               for (n = 0; n < nmaps; n++) {
> -                     char *buf;
> +                     char    *buf;
>  
>                       d = XFS_FSB_TO_DADDR(mp, mval[n].br_startblock);
>                       byte_cnt = XFS_FSB_TO_B(mp, mval[n].br_blockcount);
> @@ -525,9 +518,7 @@ xfs_symlink(
>                       bp->b_ops = &xfs_symlink_buf_ops;
>  
>                       byte_cnt = XFS_SYMLINK_BUF_SPACE(mp, byte_cnt);
> -                     if (pathlen < byte_cnt) {
> -                             byte_cnt = pathlen;
> -                     }
> +                     byte_cnt = min(byte_cnt, pathlen);
>  
>                       buf = bp->b_addr;
>                       buf += xfs_symlink_hdr_set(mp, ip->i_ino, offset,
> @@ -542,6 +533,7 @@ xfs_symlink(
>                       xfs_trans_log_buf(tp, bp, 0, (buf + byte_cnt - 1) -
>                                                       (char *)bp->b_addr);
>               }
> +             ASSERT(pathlen == 0);
>       }
>  
>       /*
> 

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