Re: [PATCH 2/3] xfs: cleanup the transaction commit path a bit

[Dave Chinner <david@fromorbit.com>]

On Mon, Nov 28, 2011 at 03:25:24AM -0500, Christoph Hellwig wrote:
> Now that the nodelaylog mode is gone we can simplify the transaction commit
> path a bit by removing the xfs_trans_commit_cil routine.  Restoring the
> process flags is merged into xfs_trans_commit which already does it for
> the error path, and allocating the log vectors is merged into
> xlog_cil_format_items, which already fills them with data, thus avoiding
> one loop over all log items.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> ---
>  fs/xfs/xfs_log.h     |    3 -
>  fs/xfs/xfs_log_cil.c |   82 +++++++++++++++++++++++++++++++++++++--------------
>  fs/xfs/xfs_trans.c   |   81 ++------------------------------------------------
>  3 files changed, 66 insertions(+), 100 deletions(-)
> 
> Index: xfs/fs/xfs/xfs_log_cil.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_log_cil.c	2011-11-28 09:17:38.320029780 +0100
> +++ xfs/fs/xfs/xfs_log_cil.c	2011-11-28 09:21:33.195424016 +0100
> @@ -161,38 +161,75 @@ xlog_cil_init_post_recovery(
>   * to the copied region inside the buffer we just allocated. This allows us to
>   * format the regions into the iclog as though they are being formatted
>   * directly out of the objects themselves.
> + *
> + * Note that this format differs from the old log vector format in that there
> + * is no transaction header in these log vectors.

Do we even need this comment given the old itransaction commit code
that did this is now gone?

>   */
> -static void
> -xlog_cil_format_items(
> -	struct log		*log,
> -	struct xfs_log_vec	*log_vector)
> +static struct xfs_log_vec *
> +xlog_cil_prepare_log_vecs(
> +	struct xfs_trans	*tp)
>  {
> -	struct xfs_log_vec *lv;
> +	struct xfs_log_item_desc *lidp;
> +	struct xfs_log_vec	*lv = NULL;
> +	struct xfs_log_vec	*ret_lv = NULL;
>  
> -	ASSERT(log_vector);
> -	for (lv = log_vector; lv; lv = lv->lv_next) {
> +
> +	/* Bail out if we didn't find a log item.  */
> +	if (list_empty(&tp->t_items)) {
> +		ASSERT(0);
> +		return NULL;
> +	}
> +
> +	list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> +		struct xfs_log_vec *new_lv;
>  		void	*ptr;
>  		int	index;
>  		int	len = 0;
>  
> +		/* Skip items which aren't dirty in this transaction. */
> +		if (!(lidp->lid_flags & XFS_LID_DIRTY))
> +			continue;
> +
> +		/* Skip items that do not have any vectors for writing */
> +		lidp->lid_size = IOP_SIZE(lidp->lid_item);
> +		if (!lidp->lid_size)
> +			continue;
> +
> +		new_lv = kmem_zalloc(sizeof(*new_lv) +
> +				lidp->lid_size * sizeof(struct xfs_log_iovec),
> +				KM_SLEEP);
> +
> +		/* The allocated iovec region lies beyond the log vector. */
> +		new_lv->lv_iovecp = (struct xfs_log_iovec *)&new_lv[1];
> +		new_lv->lv_niovecs = lidp->lid_size;
> +		new_lv->lv_item = lidp->lid_item;
> +
>  		/* build the vector array and calculate it's length */
> -		IOP_FORMAT(lv->lv_item, lv->lv_iovecp);
> -		for (index = 0; index < lv->lv_niovecs; index++)
> -			len += lv->lv_iovecp[index].i_len;
> -
> -		lv->lv_buf_len = len;
> -		lv->lv_buf = kmem_alloc(lv->lv_buf_len, KM_SLEEP|KM_NOFS);
> -		ptr = lv->lv_buf;
> +		IOP_FORMAT(new_lv->lv_item, new_lv->lv_iovecp);
> +		for (index = 0; index < new_lv->lv_niovecs; index++)
> +			len += new_lv->lv_iovecp[index].i_len;

Would it make sense to have IOP_FORMAT return the length of the
vectors (calculated as itis built) rather than having to add them up
after the fact? That would avoid an extra pass across the vector
array. Regardless, it can be done as a future patch...

Otherwise, looks OK to me.

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

-- 
Dave Chinner
david@fromorbit.com

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