Re: [PATCH 2/3] [PATCH 2/3] xfs: do not use xfs_mod_incore_sb for per-cpu counters

[Dave Chinner <david@fromorbit.com>]

On Wed, Sep 29, 2010 at 03:22:23AM -0400, Christoph Hellwig wrote:
> Export xfs_icsb_modify_counters and always use it for modifying the per-cpu
> counters.  Remove support for per-cpu counters from xfs_mod_incore_sb to
> simplify it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Couple of things, though.

>  			nblks += cur->bc_private.b.allocated;
>  		ASSERT(nblks <= da_old);
>  		if (nblks < da_old)
> -			xfs_mod_incore_sb(ip->i_mount, XFS_SBS_FDBLOCKS,
> +			xfs_icsb_modify_counters(ip->i_mount, XFS_SBS_FDBLOCKS,
>  				(int64_t)(da_old - nblks), rsvd);
>  	}
>  	/*
> @@ -1078,8 +1078,8 @@ xfs_bmap_add_extent_delay_real(
>  		temp2 = xfs_bmap_worst_indlen(ip, temp2);
>  		diff = (int)(temp + temp2 - startblockval(PREV.br_startblock) -
>  			(cur ? cur->bc_private.b.allocated : 0));
> -		if (diff > 0 &&
> -		    xfs_mod_incore_sb(ip->i_mount, XFS_SBS_FDBLOCKS, -((int64_t)diff), rsvd)) {
> +		if (diff > 0 && xfs_icsb_modify_counters(ip->i_mount,
> +				XFS_SBS_FDBLOCKS, -((int64_t)diff), rsvd)) {

Not sure I like the indenting of the second line. I'd prefer the
parameters to have a little more indent or use three lines...

>  int
>  xfs_mod_incore_sb(
> -	xfs_mount_t	*mp,
> -	xfs_sb_field_t	field,
> -	int64_t		delta,
> -	int		rsvd)
> +	struct xfs_mount	*mp,
> +	xfs_sb_field_t		field,
> +	int64_t			delta,
> +	int			rsvd)
>  {
> -	int	status;
> +	int			status;
>  
> -	/* check for per-cpu counters */
> -	switch (field) {
> -#ifdef HAVE_PERCPU_SB
> -	case XFS_SBS_ICOUNT:
> -	case XFS_SBS_IFREE:
> -	case XFS_SBS_FDBLOCKS:
> -		status = xfs_icsb_modify_counters(mp, field, delta, rsvd);
> -		break;
> -#endif
> -	default:
> -		spin_lock(&mp->m_sb_lock);
> -		status = xfs_mod_incore_sb_unlocked(mp, field, delta, rsvd);
> -		spin_unlock(&mp->m_sb_lock);
> -		break;
> -	}
> +	ASSERT(field < XFS_SBS_ICOUNT || field > XFS_SBS_FDBLOCKS);

That assert will cause issues with:

> @@ -97,6 +99,8 @@ extern void	xfs_icsb_sync_counters_locke
>  #define xfs_icsb_reinit_counters(mp)		do { } while (0)
>  #define xfs_icsb_sync_counters(mp, flags)	do { } while (0)
>  #define xfs_icsb_sync_counters_locked(mp, flags) do { } while (0)
> +#define xfs_icsb_modify_counters(mp, field, delta, rsvd) \
> +	xfs_mod_incore_sb(mp, field, delta, rsvd)
>  #endif

UP builds. Perhaps a CONFIG_SMP only assert? Given that the per-cpu
counter rework I'm doing doesn't have a different code path for
UP vs SMP, it'd only be a temporary concern....

>  undo_blocks:
>  	if (blocks > 0) {
> -		(void) xfs_mod_incore_sb(tp->t_mountp, XFS_SBS_FDBLOCKS,
> +		(void) xfs_icsb_modify_counters(tp->t_mountp, XFS_SBS_FDBLOCKS,
>  					 (int64_t)blocks, rsvd);

You can kill the (void) cast there.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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