On Tue, May 21, 2013 at 06:02:09PM +1000, 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
blocks
> into account and only allocating one block. hence when trying to
H
fixed.
> 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>
> ---
> 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;
Nice to get rid of that loop.
> }
>
> 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);
The min is necessary due since we can have up to three extents in here,
according to the comment above the define for XFS_SYMLINK_MAPS. So byte_cnt
can be less than pathlen.
Looks good.
Reviewed-by: Ben Myers <bpm@xxxxxxx>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
