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

[Brian Foster <bfoster@redhat.com>]

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