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

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

-------- 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日 11:21
> 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
Yes, But I don't mean to hide it.
If it's a bug in space cache, it will still be reproducible on with 
space_cache mount option.

The patch itself is focused on the space cache created with 
nospace_cache mount option,
at least it's doing it job.

The wrong space cahce problem is another story then.
I'll remove the wrong space cache description in the commit message in 
next version.

Thanks,
Qu
>
>> 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;
>>>>>>>>      }
>>>>>>>>
>>>>>> .
>>>>>>
>> .
>>