xfs
[Top] [All Lists]

Re: [PATCH 2/6] xfs: factor agf counter updates into a helper

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/6] xfs: factor agf counter updates into a helper
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 25 Jan 2011 15:30:03 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110121092550.694829728@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110121092227.115815324@xxxxxxxxxxxxxxxxxxxxxx> <20110121092550.694829728@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Fri, Jan 21, 2011 at 04:22:29AM -0500, Christoph Hellwig wrote:
> Updating the AGF/trans counters is duplicated between allocating and
> freeing extents.  Factor the code into a common helper.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Looks good, but one minor thing:


>               ASSERT(0);
>               /* NOTREACHED */
>       }
> -     if (error)
> +
> +     if (error || args->agbno == NULLAGBLOCK)
>               return error;
> -     /*
> -      * If the allocation worked, need to change the agf structure
> -      * (and log it), and the superblock.
> -      */
> -     if (args->agbno != NULLAGBLOCK) {
> -             xfs_agf_t       *agf;   /* allocation group freelist header */
> -             long            slen = (long)args->len;
> -
> -             ASSERT(args->len >= args->minlen && args->len <= args->maxlen);
> -             ASSERT(!(args->wasfromfl) || !args->isfl);
> -             ASSERT(args->agbno % args->alignment == 0);
> -             if (!(args->wasfromfl)) {
> -
> -                     agf = XFS_BUF_TO_AGF(args->agbp);
> -                     be32_add_cpu(&agf->agf_freeblks, -(args->len));
> -                     xfs_trans_agblocks_delta(args->tp,
> -                                              -((long)(args->len)));
> -                     args->pag->pagf_freeblks -= args->len;
> -                     ASSERT(be32_to_cpu(agf->agf_freeblks) <=
> -                             be32_to_cpu(agf->agf_length));
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The assert is lost in this path.

> -                     xfs_alloc_log_agf(args->tp, args->agbp,
> -                                             XFS_AGF_FREEBLKS);
> -                     /*
> -                      * Search the busylist for these blocks and mark the
> -                      * transaction as synchronous if blocks are found. This
> -                      * avoids the need to block due to a synchronous log
> -                      * force to ensure correct ordering as the synchronous
> -                      * transaction will guarantee that for us.
> -                      */
> -                     if (xfs_alloc_busy_search(args->mp, args->agno,
> -                                             args->agbno, args->len))
> -                             xfs_trans_set_sync(args->tp);
> -             }
> -             if (!args->isfl)
> -                     xfs_trans_mod_sb(args->tp,
> -                             args->wasdel ? XFS_TRANS_SB_RES_FDBLOCKS :
> -                                     XFS_TRANS_SB_FDBLOCKS, -slen);
> -             XFS_STATS_INC(xs_allocx);
> -             XFS_STATS_ADD(xs_allocb, args->len);
> +
> +     ASSERT(args->len >= args->minlen);
> +     ASSERT(args->len <= args->maxlen);
> +     ASSERT(!args->wasfromfl || !args->isfl);
> +     ASSERT(args->agbno % args->alignment == 0);
> +
> +     if (!args->wasfromfl) {
> +             xfs_alloc_update_counters(args->tp, args->pag, args->agbp,
> +                                       -((long)(args->len)));

Perhaps catching the error here and adding an ASSERT(error == 0)
would be a good idea.

Otherwise,

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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