Date: Tue, 25 Jan 2011 15:30:03 +1100
From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 2/6] xfs: factor agf counter updates into a helper
Message-ID: <20110125043003.GJ11040@dastard>
References: <20110121092227.115815324@bombadil.infradead.org>
 <20110121092550.694829728@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20110121092550.694829728@bombadil.infradead.org>
User-Agent: Mutt/1.5.20 (2009-06-14)

On Fri, Jan 21, 2011 at 04:22:29AM -0500, Christoph Hellwig wrote:
> Updating the AGF/trans counters is duplicated between allocating and
> freeing extents.  Factor the code into a common helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good, but one minor thing:


>  		ASSERT(0);
>  		/* NOTREACHED */
>  	}
> -	if (error)
> +
> +	if (error || args->agbno == NULLAGBLOCK)
>  		return error;
> -	/*
> -	 * If the allocation worked, need to change the agf structure
> -	 * (and log it), and the superblock.
> -	 */
> -	if (args->agbno != NULLAGBLOCK) {
> -		xfs_agf_t	*agf;	/* allocation group freelist header */
> -		long		slen = (long)args->len;
> -
> -		ASSERT(args->len >= args->minlen && args->len <= args->maxlen);
> -		ASSERT(!(args->wasfromfl) || !args->isfl);
> -		ASSERT(args->agbno % args->alignment == 0);
> -		if (!(args->wasfromfl)) {
> -
> -			agf = XFS_BUF_TO_AGF(args->agbp);
> -			be32_add_cpu(&agf->agf_freeblks, -(args->len));
> -			xfs_trans_agblocks_delta(args->tp,
> -						 -((long)(args->len)));
> -			args->pag->pagf_freeblks -= args->len;
> -			ASSERT(be32_to_cpu(agf->agf_freeblks) <=
> -				be32_to_cpu(agf->agf_length));
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The assert is lost in this path.

> -			xfs_alloc_log_agf(args->tp, args->agbp,
> -						XFS_AGF_FREEBLKS);
> -			/*
> -			 * Search the busylist for these blocks and mark the
> -			 * transaction as synchronous if blocks are found. This
> -			 * avoids the need to block due to a synchronous log
> -			 * force to ensure correct ordering as the synchronous
> -			 * transaction will guarantee that for us.
> -			 */
> -			if (xfs_alloc_busy_search(args->mp, args->agno,
> -						args->agbno, args->len))
> -				xfs_trans_set_sync(args->tp);
> -		}
> -		if (!args->isfl)
> -			xfs_trans_mod_sb(args->tp,
> -				args->wasdel ? XFS_TRANS_SB_RES_FDBLOCKS :
> -					XFS_TRANS_SB_FDBLOCKS, -slen);
> -		XFS_STATS_INC(xs_allocx);
> -		XFS_STATS_ADD(xs_allocb, args->len);
> +
> +	ASSERT(args->len >= args->minlen);
> +	ASSERT(args->len <= args->maxlen);
> +	ASSERT(!args->wasfromfl || !args->isfl);
> +	ASSERT(args->agbno % args->alignment == 0);
> +
> +	if (!args->wasfromfl) {
> +		xfs_alloc_update_counters(args->tp, args->pag, args->agbp,
> +					  -((long)(args->len)));

Perhaps catching the error here and adding an ASSERT(error == 0)
would be a good idea.

Otherwise,

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com