Re: [2.6.38-rc6] create->rebalance->mount crash...

[Miao Xie <miaox@cn.fujitsu.com>]

Hi, Chris and Liu

On thu, 24 Feb 2011 09:35:32 -0500, Chris Mason wrote:
[SNIP]
>> [PATCH] btrfs: fix OOPS of empty filesystem after balance
>>
>> btrfs will exclude unused block groups via a thread.
>> When a empty filesystem is balanced, the block group with tag "DATA" may be dropped,
>> and after umount, this will lead to OOPS when we mount it again.
>
> Thanks for tracking this down!  Comment below:
[SNIP]
>> -static void init_global_block_rsv(struct btrfs_fs_info *fs_info)
>> +static int init_global_block_rsv(struct btrfs_fs_info *fs_info)
>>   {
>>       struct btrfs_space_info *space_info;
>>
>> +    space_info = __find_space_info(fs_info, BTRFS_BLOCK_GROUP_DATA);
>> +    if (!space_info)
>> +        return -EAGAIN;
>> +
>>       space_info = __find_space_info(fs_info, BTRFS_BLOCK_GROUP_SYSTEM);
>>       fs_info->chunk_block_rsv.space_info = space_info;
>>       fs_info->chunk_block_rsv.priority = 10;
>> @@ -3884,6 +3888,8 @@ static void init_global_block_rsv(struct btrfs_fs_info *fs_info)
>>       btrfs_add_durable_block_rsv(fs_info,&fs_info->delalloc_block_rsv);
>>
>>       update_global_block_rsv(fs_info);
>> +
>> +    return 0;
>>   }
>>
>>   static void release_global_block_rsv(struct btrfs_fs_info *fs_info)
>> @@ -8514,7 +8520,13 @@ int btrfs_read_block_groups(struct btrfs_root *root)
>>               set_block_group_ro(cache);
>>       }
>>
>> -    init_global_block_rsv(info);
>> +again:
>> +    ret = init_global_block_rsv(info);
>> +    if (ret == -EAGAIN) {
>> +        update_space_info(info, BTRFS_BLOCK_GROUP_DATA, 0, 0,
>> +&space_info);
>> +        goto again;
>> +    }
>>       ret = 0;
>
> Are we looping here because we expect the init_global_block_rsv to fail
> more than once?  If so we need a cond_resched or something in there.
>
> But if the EAGAIN is only returned once we should avoid the loop and
> open code the call again.

I don't think we should create a space information object in init_global_block_rsv(),
which just does initialize the global block reservation object.

I think it is better to split btrfs_read_block_group() to three steps.
Step 1: create and initialize the space information object.
Step 2: read the block groups and update the space information.
Step 3: initialize the global block reservation object.
In this way, the logic of the source is clear, and avoid sometrivial mistake.

BTW: I found the btrfs filesystem just has three types of data(file data, meta data,
system meta data), why not add a space information array with three elements into fs_info?
In this way, we can simplify the source code of the space information, and needn't use
RCU lock to protect the space information object list. (I didn't find a lock to protect
the space information object list in the write-side. Is it right?)

Regards
Miao

>
> -chris
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>