Re: [PATCH 6/9] xfs: update and factor xfs_trans_committed()

[Christoph Hellwig <hch@infradead.org>]

On Sat, Mar 06, 2010 at 12:51:21PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The function header to xfs-trans_committed has long had this
> comment:
> 
>  * THIS SHOULD BE REWRITTEN TO USE xfs_trans_next_item()
> 
> To prepare for different methods of committing items, convert the
> code to use xfs_trans_next_item() and factor the code into smaller,
> more digestible chunks.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

A few nits left over from the old code that might be worth fixing
while you're at it:

> -STATIC void	xfs_trans_chunk_committed(xfs_log_item_chunk_t *, xfs_lsn_t, int);

Once you start reoerding the functions, can we also move
xfs_trans_committed above it's user?

> +static void
> +xfs_trans_item_committed(
> +	xfs_log_item_t	*lip,
> +	xfs_lsn_t	commit_lsn,
> +	int		aborted)
>  {
> +	xfs_lsn_t	item_lsn;
> +	struct xfs_ail	*ailp;

Might be worth to switch to the struct types consistently and give
the variables another tab of indentation.

> +
> +	if (aborted)
> +		lip->li_flags |= XFS_LI_ABORTED;
>  
>  	/*
> +	 * Send in the ABORTED flag to the COMMITTED routine so that it knows
> +	 * whether the transaction was aborted or not.
>  	 */
> +	item_lsn = IOP_COMMITTED(lip, commit_lsn);

If we want to keep the comment it should be moved above the

	if (aborted)

abive.  But I'd just drop it.

> +xfs_trans_committed(
> +	xfs_trans_t	*tp,
> +	int		abortflag)
>  {
>  	xfs_log_item_desc_t	*lidp;
> +	xfs_log_item_chunk_t	*licp;
> +	xfs_log_item_chunk_t	*next_licp;
>  
> +	/*
> +	 * Call the transaction's completion callback if there
> +	 * is one.
> +	 */
> +	if (tp->t_callback != NULL) {
> +		tp->t_callback(tp, tp->t_callarg);
> +	}

	if (tp->t_callback)
		tp->t_callback(tp, tp->t_callarg);

> +	/* free the item chunks, ignoring the embedded chunk */
> +	licp = tp->t_items.lic_next;
> +	while (licp != NULL) {
> +		next_licp = licp->lic_next;
> +		ASSERT(xfs_lic_are_all_free(licp));
> +		kmem_free(licp);
> +		licp = next_licp;

	for (licp = tp->t_items.lic_next; licp != NULL; licp = next_licp) {
		next_licp = licp->lic_next;
		ASSERT(xfs_lic_are_all_free(licp));
		kmem_free(licp);
	}


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