On 09/04/2013 10:27 PM, Dave Chinner wrote:
> On Tue, Sep 03, 2013 at 02:25:06PM -0400, Brian Foster wrote:
>> Replace xfs_dialloc_ag() with an implementation that looks for a
>> record in the finobt. The finobt only tracks records with at least
>> one free inode. This eliminates the need for the intra-ag scan in
>> the original algorithm. Once the inode is allocated, update the
>> finobt appropriately (possibly removing the record) as well as the
>> inobt.
>>
>> Move the original xfs_dialloc_ag() algorithm to
>> xfs_dialloc_ag_slow() and fall back as such if finobt support is
>> not enabled.
>>
>> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
>> ---
>> fs/xfs/xfs_ialloc.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 135 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
>> index e64a728..516f4af 100644
>> --- a/fs/xfs/xfs_ialloc.c
>> +++ b/fs/xfs/xfs_ialloc.c
>> @@ -708,7 +708,7 @@ xfs_ialloc_get_rec(
>> * available.
>> */
...
>
> Why do we need to initialise both cursors at once? We only do the
> operations one at a time, and you should actually use 2 cursors
> for the finobt lookup.....
>
No good reason. I probably just did that to simplify error handling.
>> +
>> + /*
>> + * Search the finobt.
>> + */
>> + error = xfs_inobt_lookup(fcur, pagino, XFS_LOOKUP_LE, &i);
>> + if (error)
>> + goto error;
>> + if (i == 0) {
>> + error = xfs_inobt_lookup(fcur, pagino, XFS_LOOKUP_GE, &i);
>> + if (error)
>> + goto error;
>> + XFS_WANT_CORRUPTED_GOTO(i == 1, error);
>> + }
>
> .... because this biases allocation to lower inode numbers than the
> target. i.e we only ever search for higher numbers if here are none
> lower. That's quite different to the current algorithm which first
> searches for the *closest* free inode.
>
> That is, we should be using two cursors for the free inode search,
> one for LE, the other for GT. If they both return records then, like
> the "slow" algorithm, calculate the diff between them and the target
> inode, and select the closer one (smallest diff). Destroy the cursor
> you don't need.
>
Ah, Ok. I hadn't taken a close enough look at the existing algorithm
yet, to be honest. I'll do so and incorporate the closest free inode
heuristic.
...
>> + xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -1);
>> + xfs_perag_put(pag);
>> +
>> + error = xfs_check_agi_freecount(fcur, agi);
>> + if (error)
>> + goto error;
>> + error = xfs_check_agi_freecount(icur, agi);
>> + if (error)
>> + goto error;
>
> Failures here will result in 2 calls to xfs_perag_put(pag);
>
Yeah, thanks.
Brian
>> +
>> + xfs_btree_del_cursor(icur, XFS_BTREE_NOERROR);
>> + xfs_btree_del_cursor(fcur, XFS_BTREE_ERROR);
>> + *inop = ino;
>> + return 0;
>> +error:
>> + xfs_perag_put(pag);
>> + xfs_btree_del_cursor(icur, XFS_BTREE_ERROR);
>> + xfs_btree_del_cursor(fcur, XFS_BTREE_ERROR);
>> + return error;
>> +}
>
> Otherwise it looks good.
>
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
