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: Alex Elder <aelder@xxxxxxx>
Date: Thu, 27 Jan 2011 17:21:43 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110121092550.694829728@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110121092227.115815324@xxxxxxxxxxxxxxxxxxxxxx> <20110121092550.694829728@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: aelder@xxxxxxx
On Fri, 2011-01-21 at 04:22 -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>

Two comments below.  First one takes a little thought,
the second is nothing.  I'm OK with the change as-is.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> Index: xfs/fs/xfs/xfs_alloc.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_alloc.c       2011-01-03 13:11:28.893004699 +0100
 . . .
> +
> +     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)));
> +

You might as well make use of the return value here.
It looks like the callers of xfs_alloc_ag_vextent()
are prepared to handle EFSCORRUPTED already.  I
realize that changes the behavior of the code and
doesn't just refactor it, and I haven't really
followed up exhaustively on the implications.

> +             /*
> +              * 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,
> +                              -((long)(args->len)));

How about:   -(long) args->len

>       }
> +
> +     XFS_STATS_INC(xs_allocx);
> +     XFS_STATS_ADD(xs_allocb, args->len);
>       return 0;

. . .

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