Re: [PATCH v3 06/11] xfs: use and update the finobt on inode allocation

Dave Chinner
2014-02-20T00:39:28Z

On Fri, Feb 14, 2014 at 03:01:45PM -0500, Brian Foster wrote:
> On 02/11/2014 02:17 AM, Dave Chinner wrote:
> > On Tue, Feb 04, 2014 at 12:49:37PM -0500, Brian Foster wrote:
> ...
> >> + /*
> >> + * Lookup and modify the equivalent record in the inobt.
> >> + */
> >> + tcur = xfs_inobt_init_cursor(mp, tp, agbp, agno, XFS_BTNUM_INO);
> >
> > In case ou hadn't guessed, I don't like the "tcur/trec" variables
> > because they make me thing "temporary" which they aren't. In this
> > case it is the inobt cursor and record....
> >
> > In fact, this whole segment could be factored into a function like
> > xfs_dialloc_ag_inobt_update(), hence removing the second cursor from
> > xfs_dialloc_ag() function altogether and that would clean a lot of
> > the logic up....
> >
> ...
> >> + /*
> >> + * Update the perag and superblock.
> >> + */
> >> + be32_add_cpu(&agi->agi_freecount, -1);
> >> + xfs_ialloc_log_agi(tp, agbp, XFS_AGI_FREECOUNT);
> >> + pag->pagi_freecount--;
> >> +
> >> + xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -1);
> >
> > This will need to be done before you update the inobt, though, so
> > you can run the xfs_check_agi_freecount() count in it and it will
> > come out correct....
> >
>
> After cleaning up some of this code and taking a closer look, I end up
> with something just short of complete removal of the inobt cursor in
> this function. Reason being... the point above with regard to checking
> the btrees against the agi freecount pre and post modification.

I can't really comment all that well without having seen the
factored code you've written....

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com