Re: [PATCH] xfs: prevent 32bit overflow in space reservation

[Alex Elder <aelder@sgi.com>]

On Thu, 2010-09-02 at 15:17 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> If we attempt to preallocate more than 2^32 blocks of space in a
> single syscall, the transaction block reservation will overflow
> leading to a hangs in the superblock block accounting code. This
> is trivially reproduced with xfs_io. Fix the problem by capping the
> allocation reservation to the maximum number of blocks a single
> xfs_bmapi() call can allocate (2^21 blocks).

This looks OK, but I have two comments, below.

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_vnodeops.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 66d585c..91dd9c8 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -2299,15 +2299,21 @@ xfs_alloc_file_space(
>  			e = allocatesize_fsb;
>  		}
>  
> +		/*
> +		 * we can't allocate more than @nimaps extents at a time,
> +		 * so prevent a 32bit overflow on the transaction reserve
> +		 * by trying to reserve > 16TB worth of blocks for the
> +		 * preallocation.
> +

This comment could use rewording.  How about something like:

	A 32-bit block count limits the amount of space that can
	be reserved in a transaction, so we need to limit the
	number of blocks reserved to avoid overflow.  We can't
	allocate more than @nimaps extents (whose size won't
	exceed 32 bits) at a time anyway, so use that to enforce
	the limit.

> 		 */
> +		resblks = min_t(xfs_fileoff_t, (e - s), (MAXEXTLEN * nimaps));

I guess it's clear that MAXEXTLEN fits in 32 bits because of
sizeof (xfs_extlen_t).  And inspection shows that nimaps is
just 1, so this does the 32-bit limiting.  But that just
seems indirect.  (Actually, now that I've written this I
updated the above comment and it's better...)

						-Alex


>  		if (unlikely(rt)) {
> -			resrtextents = qblocks = (uint)(e - s);
> +			resrtextents = qblocks = resblks;
>  			resrtextents /= mp->m_sb.sb_rextsize;
>  			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
>  			quota_flag = XFS_QMOPT_RES_RTBLKS;
>  		} else {
>  			resrtextents = 0;
> -			resblks = qblocks = \
> -				XFS_DIOSTRAT_SPACE_RES(mp, (uint)(e - s));
> +			resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, resblks);
>  			quota_flag = XFS_QMOPT_RES_REGBLKS;
>  		}
>  



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs