On 2018/10/12 下午9:52, Josef Bacik wrote:
> On Fri, Oct 12, 2018 at 02:18:19PM +0800, Qu Wenruo wrote:
>> We have a complex loop design for find_free_extent(), that has different
>> behavior for each loop, some even includes new chunk allocation.
>>
>> Instead of putting such a long code into find_free_extent() and makes it
>> harder to read, just extract them into find_free_extent_update_loop().
>>
>> With all the cleanups, the main find_free_extent() should be pretty
>> barebone:
>>
>> find_free_extent()
>> |- Iterate through all block groups
>> | |- Get a valid block group
>> | |- Try to do clustered allocation in that block group
>> | |- Try to do unclustered allocation in that block group
>> | |- Check if the result is valid
>> | | |- If valid, then exit
>> | |- Jump to next block group
>> |
>> |- Push harder to find free extents
>> |- If not found, re-iterate all block groups
>>
>> Signed-off-by: Qu Wenruo <w...@suse.com>
>> Reviewed-by: Su Yue <suy.f...@cn.fujitsu.com>
>> ---
>> fs/btrfs/extent-tree.c | 217 ++++++++++++++++++++++-------------------
>> 1 file changed, 117 insertions(+), 100 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index e6bfa91af41c..938569d2c583 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -7236,7 +7236,9 @@ struct find_free_extent_ctl {
>> /* RAID index, converted from flags */
>> int index;
>>
>> - /* Current loop number */
>> + /*
>> + * Current loop number, check find_free_extent_update_loop() for details
>> + */
>> int loop;
>>
>> /*
>> @@ -7433,6 +7435,117 @@ static int find_free_extent_unclustered(struct
>> btrfs_block_group_cache *bg,
>> return 0;
>> }
>>
>> +/*
>> + * Return >0 means caller needs to re-search for free extent
>> + * Return 0 means we have the needed free extent.
>> + * Return <0 means we failed to locate any free extent.
>> + */
>> +static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
>> + struct btrfs_free_cluster *last_ptr,
>> + struct btrfs_key *ins,
>> + struct find_free_extent_ctl *ffe_ctl,
>> + int full_search, bool use_cluster)
>> +{
>> + struct btrfs_root *root = fs_info->extent_root;
>> + int ret;
>> +
>> + if ((ffe_ctl->loop == LOOP_CACHING_NOWAIT) &&
>> + ffe_ctl->have_caching_bg && !ffe_ctl->orig_have_caching_bg)
>> + ffe_ctl->orig_have_caching_bg = true;
>> +
>> + if (!ins->objectid && ffe_ctl->loop >= LOOP_CACHING_WAIT &&
>> + ffe_ctl->have_caching_bg)
>> + return 1;
>> +
>> + if (!ins->objectid && ++(ffe_ctl->index) < BTRFS_NR_RAID_TYPES)
>> + return 1;
>> +
>> + /*
>> + * LOOP_CACHING_NOWAIT, search partially cached block groups, kicking
>> + * caching kthreads as we move along
>> + * LOOP_CACHING_WAIT, search everything, and wait if our bg is caching
>> + * LOOP_ALLOC_CHUNK, force a chunk allocation and try again
>> + * LOOP_NO_EMPTY_SIZE, set empty_size and empty_cluster to 0 and try
>> + * again
>> + */
>> + if (!ins->objectid && ffe_ctl->loop < LOOP_NO_EMPTY_SIZE) {
>> + ffe_ctl->index = 0;
>> + if (ffe_ctl->loop == LOOP_CACHING_NOWAIT) {
>> + /*
>> + * We want to skip the LOOP_CACHING_WAIT step if we
>> + * don't have any uncached bgs and we've already done a
>> + * full search through.
>> + */
>> + if (ffe_ctl->orig_have_caching_bg || !full_search)
>> + ffe_ctl->loop = LOOP_CACHING_WAIT;
>> + else
>> + ffe_ctl->loop = LOOP_ALLOC_CHUNK;
>> + } else {
>> + ffe_ctl->loop++;
>> + }
>> +
>> + if (ffe_ctl->loop == LOOP_ALLOC_CHUNK) {
>> + struct btrfs_trans_handle *trans;
>> + int exist = 0;
>> +
>> + trans = current->journal_info;
>> + if (trans)
>> + exist = 1;
>> + else
>> + trans = btrfs_join_transaction(root);
>> +
>> + if (IS_ERR(trans)) {
>> + ret = PTR_ERR(trans);
>> + return ret;
>> + }
>> +
>> + ret = do_chunk_alloc(trans, ffe_ctl->flags,
>> + CHUNK_ALLOC_FORCE);
>> +
>> + /*
>> + * If we can't allocate a new chunk we've already looped
>> + * through at least once, move on to the NO_EMPTY_SIZE
>> + * case.
>> + */
>> + if (ret == -ENOSPC)
>> + ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
>> +
>> + /* Do not bail out on ENOSPC since we can do more. */
>> + if (ret < 0 && ret != -ENOSPC)
>> + btrfs_abort_transaction(trans, ret);
>> + else
>> + ret = 0;
>> + if (!exist)
>> + btrfs_end_transaction(trans);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + if (ffe_ctl->loop == LOOP_NO_EMPTY_SIZE) {
>> + /*
>> + * Don't loop again if we already have no empty_size and
>> + * no empty_cluster.
>> + */
>> + if (ffe_ctl->empty_size == 0 &&
>> + ffe_ctl->empty_cluster == 0)
>> + return -ENOSPC;
>> + ffe_ctl->empty_size = 0;
>> + ffe_ctl->empty_cluster = 0;
>> + }
>> + return 1;
>> + } else if (!ins->objectid) {
>> + ret = -ENOSPC;
>> + } else if (ins->objectid) {
>> + if (!use_cluster && last_ptr) {
>> + spin_lock(&last_ptr->lock);
>> + last_ptr->window_start = ins->objectid;
>> + spin_unlock(&last_ptr->lock);
>> + }
>> + ret = 0;
>> + }
>> + return ret;
>> +}
>
> Rework this so the
>
> if (ins->objectid)
> blah
>
> is the first thing, that way you don't have to do the
>
> if (!ins->objectid && <other things>)
>
> for all the other if statements here. The fast path should be the first
> thing,
> then we can deal with all of the other crap last. Thanks,Indeed, the ENOSPC should be in the last else branch. Thanks, Qu > > Josef >
signature.asc
Description: OpenPGP digital signature
