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