Re: [PATCH 09/12] xfs: Introduce delayed logging core code

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 09/12] xfs: Introduce delayed logging core code
Date: Mon, 17 May 2010 15:51:01 +1000	[thread overview]
Message-ID: <20100517055101.GK8120@dastard> (raw)
In-Reply-To: <20100510121628.GD7165@dastard>

On Mon, May 10, 2010 at 10:16:28PM +1000, Dave Chinner wrote:
> On Mon, May 10, 2010 at 07:44:35AM -0400, Christoph Hellwig wrote:
> > > +		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];
> > > +		if (!ret_lv)
> > > +			ret_lv = new_lv;
> > > +		else
> > > +			lv->lv_next = new_lv;
> > > +		lv = new_lv;
> > 
> > I'd suggest already setting up lv->lv_niovecs and lv->lv_item here
> > instead of in xfs_trans_fill_log_vecs.  That way xfs_trans_fill_log_vecs
> > can be simplified to:
> > 
> > STATIC void
> > xfs_trans_fill_log_vecs(
> > 	struct xfs_trans	*tp,
> > 	struct xfs_log_vec	*log_vector)
> > {
> > 	struct xfs_log_vec	*lv;
> > 
> > 	for (lv = log_vector; lv = lv->lv_next; lv)
> > 		IOP_FORMAT(lidp->lid_item, lv->lv_iovecp);
> > }
> > 
> > Or just inlined into the caller or even xfs_log_commit_cil given how simple
> > it is now.  Moving it to xfs_log_commit_cil would also help avoiding the
> > locking imbalance where xfs_log_commit_cil is called with xc_ctx_lock
> > held but returns without it after the last patch in the series.  That
> > again might allow merging the IOP_FORMAT loop into xlog_cil_format_items.

OK, having looked into this in more detail, I agree that this makes
the code much simpler and formatting in xlog_cil_format_items()
makes sense for cleaning up the unbalanced locking.
xfs_trans_fill_log_vecs() basically goes away completely.

> > Btw, I wonder if xfs_log_commit_cil should simply move to xfs_trans.c?
> > That would avoid having to export xfs_trans_unreserve_and_mod_sb,
> > xfs_trans_free_items and xfs_trans_free from there, and only require
> > exporting xlog_cil_format_items (if we didn't move that one as well,
> > then xlog_cil_insert), while keeping things a lot more symmetric with
> > the traditional commit path.
> 
> I did find it a bit hard trying to draw the line between the trans
> subsystem and the logging subsystem because of the interactions and
> the way they changed as I developed the code and fixed bugs. I'll
> have a closer look at what you are suggesting (it seems reasonable
> just from a quick scan) and see how much it cleans up.

Unfortunately, it's still not a totally clean separation - either
way something has to be exported. I'd prefer not to export the CIL
context or the context lock outside xfs_log_cil.c, so that kind of
rules out moving xfs_log_commit_cil(). I moved a couple of things
from there back to xfs_trans_commit_cil(), but we still need
xfs_trans_item_committed() and xfs_trans_unreserve_and_mod_sb()
exported. That seems to be the cleanest I can come up with right
now...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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