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(+)
diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
index 386303d..8261021 100644
--- a/fs/jffs2/readinode.c
+++ b/fs/jffs2/readinode.c
@@ -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");
--
1.8.0.2
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(-)
diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index 0331072..fb30161 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -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);
--
1.8.0.2
@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(+)
diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index fb30161..610a22c 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -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)
--
1.8.0.2
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.
>
> ...
>
> --- a/fs/jffs2/nodemgmt.c
> +++ b/fs/jffs2/nodemgmt.c
> @@ -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);
Looks OK. We can do this:
--- a/fs/jffs2/nodemgmt.c~jffs2-avoid-soft-lockup-in-jffs2_reserve_space_gc-fix
+++ a/fs/jffs2/nodemgmt.c
@@ -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?
Hi Andrew,
On Tue, Feb 11, 2014 at 3:54 PM, Andrew Morton
<[email protected]> wrote:
> 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
>> --- a/fs/jffs2/nodemgmt.c
>> +++ b/fs/jffs2/nodemgmt.c
>> @@ -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);
>
> Looks OK. We can do this:
>
Yeah, thanks for the cleanup.
> --- a/fs/jffs2/nodemgmt.c~jffs2-avoid-soft-lockup-in-jffs2_reserve_space_gc-fix
> +++ a/fs/jffs2/nodemgmt.c
> @@ -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);
> _