xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 6/9] xfs: update and factor xfs_trans_committed()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Sat, 6 Mar 2010 23:01:09 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100306112458.GA20821@xxxxxxxxxxxxx>
References: <1267840284-4652-1-git-send-email-david@xxxxxxxxxxxxx> <1267840284-4652-7-git-send-email-david@xxxxxxxxxxxxx> <20100306112458.GA20821@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Sat, Mar 06, 2010 at 06:24:58AM -0500, Christoph Hellwig wrote:
> On Sat, Mar 06, 2010 at 12:51:21PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> 
> Looks good,
> 
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> 
> 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?

Will do.

> > +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.

Fair call.

> 
> > +
> > +   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.

Ok, will do.

> 
> > +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);

Once again, that's code I didn't touch but the diff has moved about.
I'll clean up the entire function given what the diff is doing...

> > +   /* 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);
>       }

Yes, makes sense.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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