xfs
[Top] [All Lists]

Re: [PATCH 3/8] xfs: bulk AIL insertion during transaction commit

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/8] xfs: bulk AIL insertion during transaction commit
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 2 Dec 2010 12:32:13 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20101130224057.GA16143@xxxxxxxxxxxxx>
References: <1290993152-20999-1-git-send-email-david@xxxxxxxxxxxxx> <1290993152-20999-4-git-send-email-david@xxxxxxxxxxxxx> <20101130224057.GA16143@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Tue, Nov 30, 2010 at 05:40:57PM -0500, Christoph Hellwig wrote:
> Generally looks good, a few minor comments below:
> 
> > +void
> > +xfs_trans_committed_bulk(
> > +   struct xfs_ail          *ailp,
> > +   struct xfs_log_vec      *log_vector,
> > +   xfs_lsn_t               commit_lsn,
> > +   int                     aborted)
> > +{
> > +#define LGIA_SIZE  32
> > +   struct xfs_log_item     *lgia[LGIA_SIZE];
> 
> The lgia name seems a bit to obscure to me.

lgia == "log item array"

> What about just log_items
> as variable name, and LOG_ITEM_BATCH_SIZE or similar for the size?

Ok, that's easy to do.

> > +                   spin_lock(&ailp->xa_lock);
> > +                   xfs_trans_ail_update_bulk(ailp, lgia, LGIA_SIZE,
> > +                                                   commit_lsn);
> > +                   for (i = 0; i < LGIA_SIZE; i++)
> > +                           IOP_UNPIN(lgia[i], 0);
> 
> > +           spin_lock(&ailp->xa_lock);
> > +           xfs_trans_ail_update_bulk(ailp, lgia, i, commit_lsn);
> > +           for (j = 0; j < i; j++)
> > +                   IOP_UNPIN(lgia[j], 0);
> 
> It might be worth to factor these two little sniplets into a little
> helper.

I thought about it, but didn't because there was more code in
writing a helper. I'll do it anyway...

> 
> > + * Bulk update version of xfs_trans_ail_update.
> 
> That won't be a very useful comment anymore once xfs_trans_ail_update is
> implemented as a wrapper around this function..

Yup, but when the wrapper is introduced the comment is updated
appropriately.

> 
> > +   struct xfs_log_item     **lgia,
> 
> Same naming comment here.

Will fix.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH 3/8] xfs: bulk AIL insertion during transaction commit, Dave Chinner <=