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

[Brian Foster <bfoster@redhat.com>]

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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs