xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 09/12] xfs: Introduce delayed logging core code
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 17 May 2010 15:51:01 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100510121628.GD7165@dastard>
References: <1273210860-23414-1-git-send-email-david@xxxxxxxxxxxxx> <1273210860-23414-10-git-send-email-david@xxxxxxxxxxxxx> <20100510114435.GA27624@xxxxxxxxxxxxx> <20100510121628.GD7165@dastard>
User-agent: Mutt/1.5.20 (2009-06-14)
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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>