xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 6/9] xfs: update and factor xfs_trans_committed()
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sat, 6 Mar 2010 06:24:58 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1267840284-4652-7-git-send-email-david@xxxxxxxxxxxxx>
References: <1267840284-4652-1-git-send-email-david@xxxxxxxxxxxxx> <1267840284-4652-7-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
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);
        }


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