Re: [PATCH RESEND v4 2/8] btrfs: Make btrfs_parse_options() parse mount option in a atomic way

[Miao Xie <miaoxie@huawei.com>]

On Fri, 30 Jan 2015 10:51:52 +0800, Qu Wenruo wrote:
> 
> -------- Original Message --------
> Subject: Re: [PATCH RESEND v4 2/8] btrfs: Make btrfs_parse_options() parse mount
> option in a atomic way
> From: Miao Xie <miaoxie@huawei.com>
> To: Qu Wenruo <quwenruo@cn.fujitsu.com>, <linux-btrfs@vger.kernel.org>
> Date: 2015年01月30日 10:06
>> On Fri, 30 Jan 2015 09:33:17 +0800, Qu Wenruo wrote:
>>> -------- Original Message --------
>>> Subject: Re: [PATCH RESEND v4 2/8] btrfs: Make btrfs_parse_options() parse mount
>>> option in a atomic way
>>> From: Miao Xie <miaoxie@huawei.com>
>>> To: Qu Wenruo <quwenruo@cn.fujitsu.com>, <linux-btrfs@vger.kernel.org>
>>> Date: 2015年01月30日 09:29
>>>> On Fri, 30 Jan 2015 09:20:46 +0800, Qu Wenruo wrote:
>>>>>> Here need ACCESS_ONCE to wrap info->mount_opt, or the complier might use
>>>>>> info->mount_opt instead of new_opt.
>>>>> Thanks for pointing out this one.
>>>>>> But I worried that this is not key reason of the wrong space cache. Could
>>>>>> you explain the race condition which caused the wrong space cache?
>>>>>>
>>>>>> Thanks
>>>>>> Miao
>>>>> CPU0:
>>>>> remount()
>>>>> |- sync_fs() <- after sync_fs() we can start new trans
>>>>> |- btrfs_parse_options()     CPU1:
>>>>>                       |- start_transaction()
>>>>>                       |- Do some bg allocation, not recorded in space_cache.
>>>> I think it is a bug if a free space is not recorded in space cache. Could you
>>>> explain why it is not recorded?
>>>>
>>>> Thanks
>>>> Miao
>>> IIRC, in that window, the fs_info->mount_opt's SPACE_CACHE bit is cleared.
>>> So space cache is not recorded.
>> SPACE_CACHE is used to control cache write out, not in-memory cache. All the
>> free space should be recorded in in-memory cache.And when we write out
>> the in-memory space cache, we need protect the space cache from changing.
>>
>> Thanks
>> Miao
> You're right, the wrong space cache problem is not caused by the non-atomic
> mount option problem.
> But the atomic mount option change with per-transaction mount option will at
> least make it disappear
> when using nospace_cache mount option.

But we need fix a problem, not hide a problem.

Thanks
Miao

> 
> Thanks,
> Qu
>>
>>> Thanks,
>>> Qu
>>>>>        |- set SPACE_CACHE bit due to cache_gen
>>>>>
>>>>>                       |- commit_transaction()
>>>>>                           |- write space cache and update cache_gen.
>>>>>                               but since some of it is not recorded in space
>>>>> cache,
>>>>>                               the space cache missing some records.
>>>>>        |- clear SPACE_CACHE bit dut to nospace_cache
>>>>>
>>>>> So the space cache is wrong.
>>>>>
>>>>> Thanks,
>>>>> Qu
>>>>>>> +    }
>>>>>>>         kfree(orig);
>>>>>>>         return ret;
>>>>>>>     }
>>>>>>>
>>>>> .
>>>>>
>>>
> 
> .
>