jffs2: fix unbalanced locking

PATCH In review Li Zefan 3 participants 6 messages Feb 8, 2014 – Feb 12, 2014

Subsystems (2)

Memory technology devices (MTD) Journalling flash file system V2 (JFFS2)

lore

Share & tools

lore

Li Zefan

author

2014-02-08 02:14 UTC #0 lore raw

PATCH 1/3 jffs2: fix unbalanced locking

This was found by our internal debugging feature on runtime, but this bug won't lead to deadlock, as the structure that this lock is embedded in is freed on error.

Cc <[email protected]>

Signed-off-by Li Zefan <[email protected]>

---
 fs/jffs2/readinode.c | 3 +++
 1 file changed, 3 insertions(+)

fs/jffs2/readinode.c +3 -0

index 386303d..8261021 100644
@@ -1143,6 +1143,7 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
 		JFFS2_ERROR("cannot read nodes for ino %u, returned error is %d\n", f->inocache->ino, ret);
 		if (f->inocache->state == INO_STATE_READING)
 			jffs2_set_inocache_state(c, f->inocache, INO_STATE_CHECKEDABSENT);
+		mutex_unlock(&f->sem);
 		return ret;
 	}

@@ -1159,6 +1160,7 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
 			jffs2_free_tmp_dnode_info(rii.mdata_tn);
 			rii.mdata_tn = NULL;
 		}
+		mutex_unlock(&f->sem);
 		return ret;
 	}

@@ -1183,6 +1185,7 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
 			if (!rii.fds) {
 				if (f->inocache->state == INO_STATE_READING)
 					jffs2_set_inocache_state(c, f->inocache, INO_STATE_CHECKEDABSENT);
+				mutex_unlock(&f->sem);
 				return -EIO;
 			}
 			JFFS2_NOTICE("but it has children so we fake some modes for it\n");

Signature · 1 line

1.8.0.2

Li Zefan

author

2014-02-08 02:15 UTC #1 lore raw

PATCH 2/3 jffs2: avoid soft-lockup in jffs2_reserve_space_gc()

We triggered soft-lockup under stress test on 2.6.34 kernel.

BUG: soft lockup - CPU#1 stuck for 60009ms! [lockf2.test:14488]

...
[<bf09a4d4>] (jffs2_do_reserve_space+0x420/0x440 [jffs2]) [<bf09a528>] (jffs2_reserve_space_gc+0x34/0x78 [jffs2]) [<bf0a1350>] (jffs2_garbage_collect_dnode.isra.3+0x264/0x478 [jffs2]) [<bf0a2078>] (jffs2_garbage_collect_pass+0x9c0/0xe4c [jffs2]) [<bf09a670>] (jffs2_reserve_space+0x104/0x2a8 [jffs2]) [<bf09dc48>] (jffs2_write_inode_range+0x5c/0x4d4 [jffs2]) [<bf097d8c>] (jffs2_write_end+0x198/0x2c0 [jffs2]) [<c00e00a4>] (generic_file_buffered_write+0x158/0x200) [<c00e14f4>] (__generic_file_aio_write+0x3a4/0x414) [<c00e15c0>] (generic_file_aio_write+0x5c/0xbc) [<c012334c>] (do_sync_write+0x98/0xd4)
[<c0123a84>] (vfs_write+0xa8/0x150)
[<c0123d74>] (sys_write+0x3c/0xc0)]

Fix this by adding a cond_resched() in the while loop.

Cc <[email protected]>

Signed-off-by Li Zefan <[email protected]>

---
 fs/jffs2/nodemgmt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

fs/jffs2/nodemgmt.c +8 -3

index 0331072..fb30161 100644
@@ -216,15 +216,20 @@ int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize,
 
 	jffs2_dbg(1, "%s(): Requested 0x%x bytes\n", __func__, minsize);
 
-	spin_lock(&c->erase_completion_lock);
-	while(ret == -EAGAIN) {
+	while (true) {
+		spin_lock(&c->erase_completion_lock);
 		ret = jffs2_do_reserve_space(c, minsize, len, sumsize);
 		if (ret) {
 			jffs2_dbg(1, "%s(): looping, ret is %d\n",
 				  __func__, ret);
 		}
+		spin_unlock(&c->erase_completion_lock);
+
+		if (ret == -EAGAIN)
+			cond_resched();
+		else
+			break;
 	}
-	spin_unlock(&c->erase_completion_lock);
 	if (!ret)
 		ret = jffs2_prealloc_raw_node_refs(c, c->nextblock, 1);

Signature · 1 line

1.8.0.2

Li Zefan

author

2014-02-08 02:16 UTC #5 lore raw

PATCH 3/3 jffs2: remove wait queue after schedule()

In reply to Li Zefan

@wait is a local variable, so if we don't remove it from the wait queue list, later wake_up() may end up accessing invalid memory.

This was spotted by eyes.

Cc <[email protected]>

Signed-off-by Li Zefan <[email protected]>

---
 fs/jffs2/nodemgmt.c | 1 +
 1 file changed, 1 insertion(+)

fs/jffs2/nodemgmt.c +1 -0

index fb30161..610a22c 100644
@@ -179,6 +179,7 @@ int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize,
 					spin_unlock(&c->erase_completion_lock);
 
 					schedule();
+					remove_wait_queue(&c->erase_wait, &wait);
 				} else
 					spin_unlock(&c->erase_completion_lock);
 			} else if (ret)

Signature · 1 line

1.8.0.2

Andrew Morton

2014-02-11 23:54 UTC #2 lore raw

Re: [PATCH 2/3] jffs2: avoid soft-lockup in jffs2_reserve_space_gc()

In reply to Li Zefan

Quoted 37 lines · On Sat, 8 Feb 2014 10:15:39 +0800 Li Zefan <[email protected]> wrote:

We triggered soft-lockup under stress test on 2.6.34 kernel.
BUG: soft lockup - CPU#1 stuck for 60009ms! [lockf2.test:14488]
...
[<bf09a4d4>] (jffs2_do_reserve_space+0x420/0x440 [jffs2]) [<bf09a528>] (jffs2_reserve_space_gc+0x34/0x78 [jffs2]) [<bf0a1350>] (jffs2_garbage_collect_dnode.isra.3+0x264/0x478 [jffs2]) [<bf0a2078>] (jffs2_garbage_collect_pass+0x9c0/0xe4c [jffs2]) [<bf09a670>] (jffs2_reserve_space+0x104/0x2a8 [jffs2]) [<bf09dc48>] (jffs2_write_inode_range+0x5c/0x4d4 [jffs2]) [<bf097d8c>] (jffs2_write_end+0x198/0x2c0 [jffs2]) [<c00e00a4>] (generic_file_buffered_write+0x158/0x200) [<c00e14f4>] (__generic_file_aio_write+0x3a4/0x414) [<c00e15c0>] (generic_file_aio_write+0x5c/0xbc) [<c012334c>] (do_sync_write+0x98/0xd4)
[<c0123a84>] (vfs_write+0xa8/0x150)
[<c0123d74>] (sys_write+0x3c/0xc0)]
Fix this by adding a cond_resched() in the while loop.
...
fs/jffs2/nodemgmt.c +2 -2
@@ -216,15 +216,20 @@ int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize,
 
 	jffs2_dbg(1, "%s(): Requested 0x%x bytes\n", __func__, minsize);
 
-	spin_lock(&c->erase_completion_lock);
-	while(ret == -EAGAIN) {
+	while (true) {
+		spin_lock(&c->erase_completion_lock);
 		ret = jffs2_do_reserve_space(c, minsize, len, sumsize);
 		if (ret) {
 			jffs2_dbg(1, "%s(): looping, ret is %d\n",
 				  __func__, ret);
 		}

fs/jffs2/nodemgmt.c int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, · line 234

Quoted 10 lines

fs/jffs2/nodemgmt.c +6 -1

+		spin_unlock(&c->erase_completion_lock);
+
+		if (ret == -EAGAIN)
+			cond_resched();
+		else
+			break;
 	}
-	spin_unlock(&c->erase_completion_lock);
 	if (!ret)
 		ret = jffs2_prealloc_raw_node_refs(c, c->nextblock, 1);

Looks OK. We can do this:

fs/jffs2/nodemgmt.c renamed from fs/jffs2/nodemgmt.c~jffs2-avoid-soft-lockup-in-jffs2_reserve_space_gc-fix +1 -1

@@ -211,7 +211,7 @@ out:
 int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize,
 			   uint32_t *len, uint32_t sumsize)
 {
-	int ret = -EAGAIN;
+	int ret;
 	minsize = PAD(minsize);
 
 	jffs2_dbg(1, "%s(): Requested 0x%x bytes\n", __func__, minsize);

I now have four jffs2 bugfixes but cannot unload them on anyone. Waddup?

Brian Norris

2014-02-12 01:00 UTC #3 lore raw

Re: [PATCH 2/3] jffs2: avoid soft-lockup in jffs2_reserve_space_gc()

Hi Andrew,

Quoted 3 lines · Andrew Morton

On Sat, 8 Feb 2014 10:15:39 +0800 Li Zefan <[email protected]> wrote:
I now have four jffs2 bugfixes but cannot unload them on anyone. Waddup?

Well, at best we have 3 "maintainers" involved in MTD (David, Artem, and me), but David is often quite unresponsive unless you yell, and Artem has more or less left non-UBI/UBIFS stuff to me. I personally have little knowledge of JFFS2, and I have seen a fair number of dubious JFFS2 patches from people with automated tools and no testing. So I'm understandably cautious to merge them. But if you have properly tested (or at least reviewed) patches sitting around, I can take a look at them and merge them. Are you referring to the top 4 here?

http://www.spinics.net/linux/lists/mm-commits/

Brian

Li Zefan

author

2014-02-12 01:42 UTC #4 lore raw

Re: [PATCH 2/3] jffs2: avoid soft-lockup in jffs2_reserve_space_gc()

In reply to Andrew Morton

Quoted 19 lines

fs/jffs2/nodemgmt.c +6 -2

@@ -216,15 +216,20 @@ int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize,
 
 	jffs2_dbg(1, "%s(): Requested 0x%x bytes\n", __func__, minsize);
 
-	spin_lock(&c->erase_completion_lock);
-	while(ret == -EAGAIN) {
+	while (true) {
+		spin_lock(&c->erase_completion_lock);
 		ret = jffs2_do_reserve_space(c, minsize, len, sumsize);
 		if (ret) {
 			jffs2_dbg(1, "%s(): looping, ret is %d\n",
 				  __func__, ret);
 		}
+		spin_unlock(&c->erase_completion_lock);
+
+		if (ret == -EAGAIN)
+			cond_resched();

Quoted 9 lines

fs/jffs2/nodemgmt.c int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, · line 234
Quoted 6 lines
fs/jffs2/nodemgmt.c +2 -1
+		else
+			break;
 	}
-	spin_unlock(&c->erase_completion_lock);
 	if (!ret)
 		ret = jffs2_prealloc_raw_node_refs(c, c->nextblock, 1);
Looks OK. We can do this:

Yeah, thanks for the cleanup.

Quoted 12 lines

fs/jffs2/nodemgmt.c renamed from fs/jffs2/nodemgmt.c~jffs2-avoid-soft-lockup-in-jffs2_reserve_space_gc-fix +1 -1

@@ -211,7 +211,7 @@ out:
 int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize,
 			   uint32_t *len, uint32_t sumsize)
 {
-	int ret = -EAGAIN;
+	int ret;
 	minsize = PAD(minsize);
 
 	jffs2_dbg(1, "%s(): Requested 0x%x bytes\n", __func__, minsize);