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.
Reported-by: Yi Zhuang <[email protected]>
Signed-off-by: Chao Yu <[email protected]>
---
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 7d8803a4cbc2..6f6a7d812d60 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1159,7 +1159,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
Somewhat late to the party (i.e. 3 months late), happy mailbox cleanup holidays!
On Thu, Sep 2, 2021 at 8:04 PM Chao Yu <[email protected]> wrote:
>
> As Yi Zhuang reported in bugzilla:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=214299
>
Bug report is for kernel 5.3. When I reported very similar deadlock in
google-msm 4.9 tree (
https://lore.kernel.org/linux-f2fs-devel/[email protected]/t/
), you pointed out that the code was missing commits which removed the
cp_rwsem grabbing (which are also missing from kernel 5.3):
commit 435cbab95e3966cd8310addd9e9b758dce0e8b84
Author: Jaegeuk Kim <[email protected]>
Date: Thu Apr 9 10:25:21 2020 -0700
f2fs: fix quota_sync failure due to f2fs_lock_op
commit ca7f76e680745d3b8a386638045f85dac1c4b2f4
Author: Chao Yu <[email protected]>
Date: Fri May 29 17:29:47 2020 +0800
f2fs: fix wrong discard space
commit 79963d967b492876fa17c8c2c2c17b7438683d9b
Author: Chao Yu <[email protected]>
Date: Thu Jun 18 14:36:23 2020 +0800
f2fs: shrink node_write lock coverage
Is this patch needed with these commits applied?
>
> 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.
>
> Reported-by: Yi Zhuang <[email protected]>
> Signed-off-by: Chao Yu <[email protected]>
As this patch is applied in the mainline kernel, can we CC -stable to
get this patch into the various Android kernels? Specifically,
https://android.googlesource.com/kernel/msm/+/refs/tags/android-12.0.0_r0.21/fs/f2fs/checkpoint.c#1147
needs this patch (alongside many other google-msm kernel branches).
Thanks,
--Shachar