[PATCH 3/3] xfs: Increase the default size of the reserved blocks pool

[Alex Elder]

On Thu, 2010-03-04 at 12:46 +1100, Dave Chinner wrote:
> The current default size of the reserved blocks pool is easy to deplete
> with certain workloads, in particular workloads that do lots of concurrent
> delayed allocation extent conversions.  If enough transactions are running
> in parallel and the entire pool is consumed then subsequent calls to
> xfs_trans_reserve() will fail with ENOSPC.  Also add a rate limited
> warning so we know if this starts happening again.
> 
> This is an updated version of an old patch from Lachlan McIlroy.

Looks good.  The comment and code rearrangements are an
improvement.

I have also reviewed the other two patches in the series
(including the updated patch 2) and they too look good.

> Signed-off-by: Dave Chinner <david at fromorbit.com>
         So is it got to be fromorbit or redhat?
         (You used both in this series.)

Reviewed-by: Alex Elder <aelder at sgi.com>

> ---
>  fs/xfs/xfs_mount.c |   49 +++++++++++++++++++++++++++++--------------------
>  1 files changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index c207fef..e79b56b 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1097,13 +1097,15 @@ xfs_default_resblks(xfs_mount_t *mp)
>  	__uint64_t resblks;
>  
>  	/*
> -	 * We default to 5% or 1024 fsbs of space reserved, whichever is smaller.
> -	 * This may drive us straight to ENOSPC on mount, but that implies
> -	 * we were already there on the last unmount. Warn if this occurs.
> +	 * We default to 5% or 8192 fsbs of space reserved, whichever is
> +	 * smaller.  This is intended to cover concurrent allocation
> +	 * transactions when we initially hit enospc. These each require a 4
> +	 * block reservation. Hence by default we cover roughly 2000 concurrent
> +	 * allocation reservations.
>  	 */
>  	resblks = mp->m_sb.sb_dblocks;
>  	do_div(resblks, 20);
> -	resblks = min_t(__uint64_t, resblks, 1024);
> +	resblks = min_t(__uint64_t, resblks, 8192);
>  	return resblks;
>  }
>  
> @@ -1417,6 +1419,9 @@ xfs_mountfs(
>  	 * when at ENOSPC. This is needed for operations like create with
>  	 * attr, unwritten extent conversion at ENOSPC, etc. Data allocations
>  	 * are not allowed to use this reserved space.
> +	 *
> +	 * This may drive us straight to ENOSPC on mount, but that implies
> +	 * we were already there on the last unmount. Warn if this occurs.
>  	 */
>  	if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
>  		resblks = xfs_default_resblks(mp);
> @@ -1725,26 +1730,30 @@ xfs_mod_incore_sb_unlocked(
>  				lcounter += rem;
>  			}
>  		} else {				/* Taking blocks away */
> -
>  			lcounter += delta;
> +			if (lcounter >= 0) {
> +				mp->m_sb.sb_fdblocks = lcounter +
> +							XFS_ALLOC_SET_ASIDE(mp);
> +				return 0;
> +			}
>  
> -		/*
> -		 * If were out of blocks, use any available reserved blocks if
> -		 * were allowed to.
> -		 */

I was just looking at this code yesterday, and got confused
by the indentation of this comment.  You beat me to fixing it.
I also think your rearranging of the logic below is an improvement.

> +			/*
> +			 * We are out of blocks, use any available reserved
> +			 * blocks if were allowed to.
> +			 */
> +			if (!rsvd)
> +				return XFS_ERROR(ENOSPC);
>  
> -			if (lcounter < 0) {
> -				if (rsvd) {
> -					lcounter = (long long)mp->m_resblks_avail + delta;
> -					if (lcounter < 0) {
> -						return XFS_ERROR(ENOSPC);
> -					}
> -					mp->m_resblks_avail = lcounter;
> -					return 0;
> -				} else {	/* not reserved */
> -					return XFS_ERROR(ENOSPC);
> -				}
> +			lcounter = (long long)mp->m_resblks_avail + delta;
> +			if (lcounter >= 0) {
> +				mp->m_resblks_avail = lcounter;
> +				return 0;
>  			}
> +			printk_once(KERN_WARNING
> +				"Filesystem \"%s\": reserve blocks depleted! "
> +				"Consider increasing reserve pool size.",
> +				mp->m_fsname);
> +			return XFS_ERROR(ENOSPC);
>  		}
>  
>  		mp->m_sb.sb_fdblocks = lcounter + XFS_ALLOC_SET_ASIDE(mp);