f2fs: quota: fix potential deadlock

PATCH In review Chao Yu 2 participants 7 messages Jan 4, 2022 – Jan 5, 2022

Subsystems (2)

Stable branch F2FS file system

lore

Share & tools

lore

Chao Yu

author maintainer

2022-01-04 13:05 UTC #0 lore raw

PATCH 1/1 f2fs: quota: fix potential deadlock

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 [email protected] # v5.3+

Reported-by Yi Zhuang <[email protected]>

Signed-off-by Chao Yu <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/checkpoint.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/f2fs/checkpoint.c +2 -1

index 83e9bc0f91ff..7b0282724231 100644
@@ -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)) {

Signature · 1 line

2.32.0

Greg KH

maintainer

2022-01-04 13:18 UTC #1 lore raw

Quoted 59 lines · Chao Yu

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 [email protected] # v5.3+
Reported-by Yi Zhuang <[email protected]>
Signed-off-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/checkpoint.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/f2fs/checkpoint.c +2 -1
index 83e9bc0f91ff..7b0282724231 100644
@@ -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)) {
Signature · 1 line
2.32.0

What stable tree(s) is this for?

thanks,

greg k-h

Chao Yu

author maintainer

2022-01-04 15:05 UTC #2 lore raw

Quoted 51 lines · Greg KH

Quoted 50 lines · Chao Yu
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 [email protected] # v5.3+
Reported-by Yi Zhuang <[email protected]>
Signed-off-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
  fs/f2fs/checkpoint.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
fs/f2fs/checkpoint.c +0 -0
index 83e9bc0f91ff..7b0282724231 100644
@@ -1162,7 +1162,8 @@ static bool __need_flush_quota(struct f2fs_sb_info *sbi)
  	if (!is_journalled_quota(sbi))
  		return false;
 

Quoted 11 lines

Quoted 9 lines

fs/f2fs/checkpoint.c +2 -1

-	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)) {

Signature · 1 line

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!

Let me know if I should send patches for different trees separately.

Thanks,

Quoted 4 lines

thanks,
greg k-h

Greg KH

maintainer

2022-01-04 15:17 UTC #3 lore raw

Quoted 63 lines · Chao Yu

Quoted 53 lines · Greg KH
Quoted 52 lines · Chao Yu
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 [email protected] # v5.3+
Reported-by Yi Zhuang <[email protected]>
Signed-off-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
  fs/f2fs/checkpoint.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
fs/f2fs/checkpoint.c +2 -1
index 83e9bc0f91ff..7b0282724231 100644
@@ -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;
Quoted 8 lines
Quoted 6 lines
fs/f2fs/checkpoint.c +0 -0
  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) {
  		ret = false;
  	} else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) {
Signature · 1 line
2.32.0
What stable tree(s) is this for?

Quoted 1 line

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? :)

Quoted 1 line

Let me know if I should send patches for different trees separately.

If the same commit here works for all of the above, it's fine. But for some reason I don't think it will work in 5.15.y...

thanks,

greg k-h

Chao Yu

author maintainer

2022-01-04 15:48 UTC #4 lore raw

Quoted 64 lines · Greg KH

Quoted 63 lines · Chao Yu
Quoted 61 lines · Greg KH
Quoted 58 lines · Chao Yu
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 [email protected] # v5.3+
Reported-by Yi Zhuang <[email protected]>
Signed-off-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
   fs/f2fs/checkpoint.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)
fs/f2fs/checkpoint.c +2 -1
index 83e9bc0f91ff..7b0282724231 100644
@@ -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)) {
Signature · 1 line
2.32.0
What stable tree(s) is this for?

Quoted 4 lines

Quoted 1 line
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.

Quoted 5 lines

Quoted 2 lines
Let me know if I should send patches for different trees separately.
If the same commit here works for all of the above, it's fine. But for some reason I don't think it will work in 5.15.y...

Copied.

Thank you for the help!

Thanks,

Quoted 4 lines

thanks,
greg k-h

Greg KH

maintainer

2022-01-05 15:01 UTC #5 lore raw

Quoted 65 lines · Chao Yu

Quoted 64 lines · Greg KH
Quoted 63 lines · Chao Yu
Quoted 61 lines · Greg KH
Quoted 58 lines · Chao Yu
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 [email protected] # v5.3+
Reported-by Yi Zhuang <[email protected]>
Signed-off-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
   fs/f2fs/checkpoint.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)
fs/f2fs/checkpoint.c +2 -1
index 83e9bc0f91ff..7b0282724231 100644
@@ -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)) {
Signature · 1 line
2.32.0
What stable tree(s) is this for?

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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.

Anyway, now queued up, thanks.

greg k-h

Chao Yu

author maintainer

2022-01-05 15:25 UTC #6 lore raw

Quoted 68 lines · Greg KH

Quoted 67 lines · Chao Yu
Quoted 66 lines · Greg KH
Quoted 64 lines · Chao Yu
Quoted 53 lines · Greg KH
Quoted 52 lines · Chao Yu
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 [email protected] # v5.3+
Reported-by Yi Zhuang <[email protected]>
Signed-off-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
    fs/f2fs/checkpoint.c | 3 ++-
    1 file changed, 2 insertions(+), 1 deletion(-)
fs/f2fs/checkpoint.c +2 -1
index 83e9bc0f91ff..7b0282724231 100644
@@ -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;
Quoted 8 lines
Quoted 6 lines
fs/f2fs/checkpoint.c +0 -0
    	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) {
    		ret = false;
    	} else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) {
Signature · 1 line
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!

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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

Quoted 2 lines

Anyway, now queued up, thanks.

Thank you!

Thanks,

Quoted 2 lines

greg k-h