Re: [f2fs-dev] [PATCH] f2fs: quota: fix potential deadlock

[Chao Yu <chao@kernel.org>]

On 2022/1/5 23:01, Greg KH wrote:
> On Tue, Jan 04, 2022 at 11:48:25PM +0800, Chao Yu wrote:
>> On 2022/1/4 23:17, Greg KH wrote:
>>> On Tue, Jan 04, 2022 at 11:05:36PM +0800, Chao Yu wrote:
>>>> On 2022/1/4 21:18, Greg KH wrote:
>>>>> On Tue, Jan 04, 2022 at 09:05:13PM +0800, Chao Yu wrote:
>>>>>> commit a5c0042200b28fff3bde6fa128ddeaef97990f8d upstream.
>>>>>>
>>>>>> As Yi Zhuang reported in bugzilla:
>>>>>>
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=214299
>>>>>>
>>>>>> There is potential deadlock during quota data flush as below:
>>>>>>
>>>>>> Thread A:			Thread B:
>>>>>> f2fs_dquot_acquire
>>>>>> down_read(&sbi->quota_sem)
>>>>>> 				f2fs_write_checkpoint
>>>>>> 				block_operations
>>>>>> 				f2fs_look_all
>>>>>> 				down_write(&sbi->cp_rwsem)
>>>>>> f2fs_quota_write
>>>>>> f2fs_write_begin
>>>>>> __do_map_lock
>>>>>> f2fs_lock_op
>>>>>> down_read(&sbi->cp_rwsem)
>>>>>> 				__need_flush_qutoa
>>>>>> 				down_write(&sbi->quota_sem)
>>>>>>
>>>>>> This patch changes block_operations() to use trylock, if it fails,
>>>>>> it means there is potential quota data updater, in this condition,
>>>>>> let's flush quota data first and then trylock again to check dirty
>>>>>> status of quota data.
>>>>>>
>>>>>> The side effect is: in heavy race condition (e.g. multi quota data
>>>>>> upaters vs quota data flusher), it may decrease the probability of
>>>>>> synchronizing quota data successfully in checkpoint() due to limited
>>>>>> retry time of quota flush.
>>>>>>
>>>>>> Fixes: db6ec53b7e03 ("f2fs: add a rw_sem to cover quota flag changes")
>>>>>> Cc: stable@vger.kernel.org # v5.3+
>>>>>> Reported-by: Yi Zhuang <zhuangyi1@huawei.com>
>>>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>>> ---
>>>>>>     fs/f2fs/checkpoint.c | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>>> index 83e9bc0f91ff..7b0282724231 100644
>>>>>> --- a/fs/f2fs/checkpoint.c
>>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>>> @@ -1162,7 +1162,8 @@ static bool __need_flush_quota(struct f2fs_sb_info *sbi)
>>>>>>     	if (!is_journalled_quota(sbi))
>>>>>>     		return false;
>>>>>> -	down_write(&sbi->quota_sem);
>>>>>> +	if (!down_write_trylock(&sbi->quota_sem))
>>>>>> +		return true;
>>>>>>     	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) {
>>>>>>     		ret = false;
>>>>>>     	} else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) {
>>>>>> -- 
>>>>>> 2.32.0
>>>>>>
>>>>>
>>>>> What stable tree(s) is this for?
>>>>
>>>> Oh, please help to try applying to 5.4, 5.10, and 5.15 stable trees, thanks!
>>>
>>> This is already in the 5.15.6 kernel release, do you need it applied
>>> there again?  :)
>>
>> Oops, no, so 5.4 and 5.10 is enough. ;)
>> We can skip 5.15 since this patch was merged in 5.15-rc1 at the first time.
> 
> It was merged in 5.16-rc1, and then backported to 5.15.6.  You might
> want to check your git scripts.

Oh, yes, you're right, it looks we merged patches with same title in both 5.15-rc1
and 5.16-rc1, that won't happen usually... so I positioning the wrong merge time
only based on patch title...

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=a5c0042200b28fff3bde6fa128ddeaef97990f8d

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=9de71ede81e6d1a111fdd868b2d78d459fa77f80

> 
> Anyway, now queued up, thanks.

Thank you!

Thanks,

> 
> greg k-h


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel