f2fs: fix ref of discard command

PATCH In review Jaegeuk Kim 3 participants 8 messages Jun 12, 2017 – Jun 24, 2017

Subsystems (2)

VFS F2FS file system

lore

Share & tools

lore

Jaegeuk Kim

author maintainer

2017-06-12 03:04 UTC #0 lore raw

PATCH 1/1 f2fs: fix ref of discard command

This patch resolves kernel panic for xfstests/081, caused by recent f2fs_bug_on

f2fs: add f2fs_bug_on in __remove_discard_cmd

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

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

fs/f2fs/segment.c +2 -0

index 86a0c1095939..a6d77388a806 100644
@@ -1025,6 +1025,8 @@ static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond)
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (!wait_cond || (dc->state == D_DONE && !dc->ref)) {
 			wait_for_completion_io(&dc->wait);
+			if (dc->state == D_DONE && dc->ref)
+				dc->ref--;
 			__remove_discard_cmd(sbi, dc);
 		} else {
 			dc->ref++;

Signature · 1 line

2.13.0.rc1.294.g07d810a77f-goog

Chao Yu

2017-06-12 11:13 UTC #1 lore raw

Hi Jaegeuk,

Quoted 14 lines · Jaegeuk Kim

This patch resolves kernel panic for xfstests/081, caused by recent f2fs_bug_on
f2fs: add f2fs_bug_on in __remove_discard_cmd
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)
fs/f2fs/segment.c +0 -0
index 86a0c1095939..a6d77388a806 100644

fs/f2fs/segment.c static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond) · line 1029

Quoted 5 lines

fs/f2fs/segment.c +2 -0

@@ -1025,6 +1025,8 @@ static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond)
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (!wait_cond || (dc->state == D_DONE && !dc->ref)) {
 			wait_for_completion_io(&dc->wait);
+			if (dc->state == D_DONE && dc->ref)
+				dc->ref--;

Should set dc->ref to 0 to avoid panic once we add other referrers?

Thanks,

Quoted 4 lines

fs/f2fs/segment.c +0 -0

 			__remove_discard_cmd(sbi, dc);
 		} else {
 			dc->ref++;

Jaegeuk Kim

author maintainer

2017-06-14 14:26 UTC #2 lore raw

Quoted 12 lines · Chao Yu

Hi Jaegeuk,
Quoted 9 lines · Jaegeuk Kim
This patch resolves kernel panic for xfstests/081, caused by recent f2fs_bug_on
f2fs: add f2fs_bug_on in __remove_discard_cmd
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 12 lines

fs/f2fs/segment.c static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond) · line 1029
Quoted 10 lines
fs/f2fs/segment.c +2 -0
index 86a0c1095939..a6d77388a806 100644
@@ -1025,6 +1025,8 @@ static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond)
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (!wait_cond || (dc->state == D_DONE && !dc->ref)) {
 			wait_for_completion_io(&dc->wait);
+			if (dc->state == D_DONE && dc->ref)
+				dc->ref--;
Should set dc->ref to 0 to avoid panic once we add other referrers?

Sorry, could you please explain this in more detail?

Thanks,

Quoted 7 lines

Thanks,

Quoted 4 lines

fs/f2fs/segment.c +0 -0

 			__remove_discard_cmd(sbi, dc);
 		} else {
 			dc->ref++;

Chao Yu

maintainer

2017-06-14 14:59 UTC #3 lore raw

Quoted 26 lines · Jaegeuk Kim

Quoted 12 lines · Chao Yu
Hi Jaegeuk,
Quoted 9 lines · Jaegeuk Kim
This patch resolves kernel panic for xfstests/081, caused by recent f2fs_bug_on
f2fs: add f2fs_bug_on in __remove_discard_cmd
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)
Quoted 12 lines
fs/f2fs/segment.c static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond) · line 1029
Quoted 10 lines
fs/f2fs/segment.c +2 -0
index 86a0c1095939..a6d77388a806 100644
@@ -1025,6 +1025,8 @@ static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond)
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (!wait_cond || (dc->state == D_DONE && !dc->ref)) {
 			wait_for_completion_io(&dc->wait);
+			if (dc->state == D_DONE && dc->ref)
+				dc->ref--;
Should set dc->ref to 0 to avoid panic once we add other referrers?

Quoted 1 line

Sorry, could you please explain this in more detail?

Oh, I just assume later we may add another referrer for some reason which will make dc->ref = 2, so dc->ref-- is not enough to avoid the bug_on in __remove_discard_cmd. I think reseting dc->ref is more safe here, how do you think?

Thanks,

Quoted 19 lines

Thanks,
Quoted 7 lines
Thanks,
Quoted 4 lines
fs/f2fs/segment.c +0 -0
 			__remove_discard_cmd(sbi, dc);
 		} else {
 			dc->ref++;
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

author maintainer

2017-06-14 15:20 UTC #4 lore raw

Quoted 27 lines · Chao Yu

Quoted 26 lines · Jaegeuk Kim
Quoted 24 lines · Chao Yu
Hi Jaegeuk,
Quoted 14 lines · Jaegeuk Kim
This patch resolves kernel panic for xfstests/081, caused by recent f2fs_bug_on
f2fs: add f2fs_bug_on in __remove_discard_cmd
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)
fs/f2fs/segment.c +0 -0
index 86a0c1095939..a6d77388a806 100644
fs/f2fs/segment.c static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond) · line 1029
Quoted 5 lines
fs/f2fs/segment.c +2 -0
@@ -1025,6 +1025,8 @@ static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond)
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (!wait_cond || (dc->state == D_DONE && !dc->ref)) {
 			wait_for_completion_io(&dc->wait);
+			if (dc->state == D_DONE && dc->ref)
+				dc->ref--;
Should set dc->ref to 0 to avoid panic once we add other referrers?

Quoted 6 lines

Quoted 1 line
Sorry, could you please explain this in more detail?
Oh, I just assume later we may add another referrer for some reason which will make dc->ref = 2, so dc->ref-- is not enough to avoid the bug_on in __remove_discard_cmd. I think reseting dc->ref is more safe here, how do you think?

Well, for now, it makes more sense to do like this when considering ref flow, IIUC. What will make dc->ref = 2 later? Even in that case, why not making zero by adding dc->ref-- appropriately?

Thanks,

Quoted 22 lines

Thanks,
Quoted 19 lines
Thanks,
Quoted 7 lines
Thanks,
Quoted 4 lines
fs/f2fs/segment.c +0 -0
 			__remove_discard_cmd(sbi, dc);
 		} else {
 			dc->ref++;
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

maintainer

2017-06-14 15:40 UTC #5 lore raw

Quoted 30 lines · Jaegeuk Kim

Quoted 29 lines · Chao Yu
Quoted 27 lines · Jaegeuk Kim
Quoted 12 lines · Chao Yu
Hi Jaegeuk,
Quoted 9 lines · Jaegeuk Kim
This patch resolves kernel panic for xfstests/081, caused by recent f2fs_bug_on
f2fs: add f2fs_bug_on in __remove_discard_cmd
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)
Quoted 12 lines
fs/f2fs/segment.c static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond) · line 1029
Quoted 10 lines
fs/f2fs/segment.c +2 -0
index 86a0c1095939..a6d77388a806 100644
@@ -1025,6 +1025,8 @@ static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond)
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (!wait_cond || (dc->state == D_DONE && !dc->ref)) {
 			wait_for_completion_io(&dc->wait);
+			if (dc->state == D_DONE && dc->ref)
+				dc->ref--;
Should set dc->ref to 0 to avoid panic once we add other referrers?
Sorry, could you please explain this in more detail?

Quoted 7 lines

Quoted 4 lines
Oh, I just assume later we may add another referrer for some reason which will make dc->ref = 2, so dc->ref-- is not enough to avoid the bug_on in __remove_discard_cmd. I think reseting dc->ref is more safe here, how do you think?
Well, for now, it makes more sense to do like this when considering ref flow, IIUC. What will make dc->ref = 2 later? Even in that case, why not making zero

It's just assumption, till now, I do not have it in my mind ;)

Quoted 1 line

by adding dc->ref-- appropriately?

You mean as below?

dc->ref--;
if (dc->ref > 0)

	dc->ref--;

Thanks,

Quoted 25 lines

Thanks,
Quoted 22 lines
Thanks,
Quoted 19 lines
Thanks,
Quoted 7 lines
Thanks,
Quoted 4 lines
fs/f2fs/segment.c +0 -0
 			__remove_discard_cmd(sbi, dc);
 		} else {
 			dc->ref++;
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

2017-06-23 08:16 UTC #6 lore raw

In reply to Jaegeuk Kim

Hi Jaegeuk,

Quoted 14 lines · Jaegeuk Kim

This patch resolves kernel panic for xfstests/081, caused by recent f2fs_bug_on
f2fs: add f2fs_bug_on in __remove_discard_cmd
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)
fs/f2fs/segment.c +0 -0
index 86a0c1095939..a6d77388a806 100644

fs/f2fs/segment.c static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond) · line 1029

Quoted 5 lines

fs/f2fs/segment.c +2 -0

@@ -1025,6 +1025,8 @@ static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond)
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (!wait_cond || (dc->state == D_DONE && !dc->ref)) {
 			wait_for_completion_io(&dc->wait);
+			if (dc->state == D_DONE && dc->ref)
+				dc->ref--;

How about using ("f2fs: stop discard thread in prior during umount") instead of this one? As dereference of dc here will lead use-after-free of real referrer.

Thanks,

Quoted 4 lines

fs/f2fs/segment.c +0 -0

 			__remove_discard_cmd(sbi, dc);
 		} else {
 			dc->ref++;

Jaegeuk Kim

author maintainer

2017-06-24 16:26 UTC #7 lore raw

Quoted 12 lines · Chao Yu

Hi Jaegeuk,
Quoted 9 lines · Jaegeuk Kim
This patch resolves kernel panic for xfstests/081, caused by recent f2fs_bug_on
f2fs: add f2fs_bug_on in __remove_discard_cmd
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 13 lines

fs/f2fs/segment.c static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond) · line 1029
Quoted 10 lines
fs/f2fs/segment.c +2 -0
index 86a0c1095939..a6d77388a806 100644
@@ -1025,6 +1025,8 @@ static void __wait_discard_cmd(struct f2fs_sb_info *sbi, bool wait_cond)
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (!wait_cond || (dc->state == D_DONE && !dc->ref)) {
 			wait_for_completion_io(&dc->wait);
+			if (dc->state == D_DONE && dc->ref)
+				dc->ref--;
How about using ("f2fs: stop discard thread in prior during umount") instead of this one? As dereference of dc here will lead use-after-free of real referrer.

Yup, I'll also verify that.
Thank you. :)

Quoted 7 lines

Thanks,

Quoted 4 lines

fs/f2fs/segment.c +0 -0

 			__remove_discard_cmd(sbi, dc);
 		} else {
 			dc->ref++;