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. The only way I see to push the inobt cursor into a separate function is to also push the agi/sb update to the middle of that function, which just seems ugly to me. The helper function is now non-intuitively sensitive to the context in which it is called (and conversely buries context that makes xfs_dialloc_ag() harder to follow). E.g., misplaced functionality for an "update the inobt" helper. That said, xfs_dialloc_ag() is much smaller from all of the other refactoring, trec is gone and tcur is renamed to icur. So I think clarity is still improved. Let me know if there's any major objections to that pattern or alternate suggestions. Otherwise, I'll run with this for the next version... Brian > Cheers, > > Dave. > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
