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
|