xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [RFC PATCH 09/11] xfs: use and update the finobt on inode allocation
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 05 Sep 2013 12:18:47 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130905022719.GW23571@dastard>
References: <1378232708-57156-1-git-send-email-bfoster@xxxxxxxxxx> <1378232708-57156-10-git-send-email-bfoster@xxxxxxxxxx> <20130905022719.GW23571@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130805 Thunderbird/17.0.8
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.
> 

<Prev in Thread] Current Thread [Next in Thread>