* [PATCH] jffs2: bug fix of creating node when gc or find space
@ 2015-12-11 1:36 gaoyongliang
2015-12-11 2:58 ` Brian Norris
0 siblings, 1 reply; 4+ messages in thread
From: gaoyongliang @ 2015-12-11 1:36 UTC (permalink / raw)
To: linux-mtd; +Cc: akpm, zhihui.gao, chenjie6, gaoyongliang, stable
From: gaoyongliang <gaoyongliang@huawei.com>
1. create inode by insert_inode_locked, and return inode with I_NEW.
2. has writed the dnode successfully in the end of one block, then
release the c->alloc_sem, but the dirent is not written yet.
3. the gc or jffs2_reserve_space which has got the c->alloc_sem may
find the block which the dnode we just writed in, the dnode can't
be gc because the dirent is not written and the state of inode is
still I_NEW, we can only wait on inode without release c->alloc_sem.
4. we can't get the c->alloc_sem to write the dirent, so the inode
state keep I_NEW forever.
5. this will cause deadlock, like ABBA deadlock.
lockf2.test D c02dead8 0 11666 1 0x00000001
locked:
c90f9be8 &inode->i_mutex 0 [<c00bf158>] generic_file_aio_write+0x40/0xb0
c2c54c44 &c->alloc_sem 1 [<bf056f9c>] jffs2_garbage_collect_pass+0x1c/0xf08 [jffs2]
[<c02dead8>] (__schedule+0x458/0x604) from [<c0114090>] (inode_wait+0x8/0x10)
[<c0114090>] (inode_wait+0x8/0x10) from [<c02dd050>] (__wait_on_bit+0x54/0xa0)
[<c02dd050>] (__wait_on_bit+0x54/0xa0) from [<c02dd114>] (out_of_line_wait_on_bit+0x78/0x84)
[<c02dd114>] (out_of_line_wait_on_bit+0x78/0x84) from [<c01157a0>] (iget_locked+0x90/0x1b0)
[<c01157a0>] (iget_locked+0x90/0x1b0) from [<bf059fe8>] (jffs2_iget+0xc/0x344 [jffs2])
[<bf059fe8>] (jffs2_iget+0xc/0x344 [jffs2]) from [<bf05a6fc>] (jffs2_gc_fetch_inode+0x104/0x158 [jffs2])
[<bf05a6fc>] (jffs2_gc_fetch_inode+0x104/0x158 [jffs2]) from [<bf0577f0>] (jffs2_garbage_collect_pass+0x870/0xf08 [jffs2])
[<bf0577f0>] (jffs2_garbage_collect_pass+0x870/0xf08 [jffs2]) from [<bf051248>] (jffs2_reserve_space+0x154/0x3b4 [jffs2])
[<bf051248>] (jffs2_reserve_space+0x154/0x3b4 [jffs2]) from [<bf053eac>] (jffs2_write_inode_range+0x58/0x3ac [jffs2])
[<bf053eac>] (jffs2_write_inode_range+0x58/0x3ac [jffs2]) from [<bf04ec20>] (jffs2_write_end+0x11c/0x224 [jffs2])
[<bf04ec20>] (jffs2_write_end+0x11c/0x224 [jffs2]) from [<c00bdfa4>] (generic_file_buffered_write+0x160/0x23c)
[<c00bdfa4>] (generic_file_buffered_write+0x160/0x23c) from [<c00bf0ac>] (__generic_file_aio_write+0x328/0x394)
[<c00bf0ac>] (__generic_file_aio_write+0x328/0x394) from [<c00bf16c>] (generic_file_aio_write+0x54/0xb0)
[<c00bf16c>] (generic_file_aio_write+0x54/0xb0) from [<c00fdc24>] (do_sync_write+0x74/0x98)
[<c00fdc24>] (do_sync_write+0x74/0x98) from [<c00fe550>] (vfs_write+0xcc/0x174)
[<c00fe550>] (vfs_write+0xcc/0x174) from [<c00fe8a8>] (SyS_write+0x38/0x64)
[<c00fe8a8>] (SyS_write+0x38/0x64) from [<c000f0c0>] (ret_fast_syscall+0x0/0x58)
Fixes: e72e6497e74811e01d72b4c1b7537b3aea3ee857
Cc: <stable@vger.kernel.org> # 3.10.y+
Signed-off-by: gaoyongliang <gaoyongliang@huawei.com>
---
fs/jffs2/dir.c | 25 +++++++++++++++++++++++++
fs/jffs2/fs.c | 1 +
fs/jffs2/gc.c | 11 +++++++++++
fs/jffs2/jffs2_fs_sb.h | 1 +
fs/jffs2/nodelist.h | 4 ++++
fs/jffs2/super.c | 1 +
fs/jffs2/write.c | 1 +
7 files changed, 44 insertions(+), 0 deletions(-)
diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
index d211b8e..808ec17 100644
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -195,7 +195,9 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry,
no chance of AB-BA deadlock involving its f->sem). */
mutex_unlock(&f->sem);
+ mutex_lock(&c->create_sem);
ret = jffs2_do_create(c, dir_f, f, ri, &dentry->d_name);
+ mutex_unlock(&c->create_sem);
if (ret)
goto fail;
@@ -208,12 +210,14 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry,
f->inocache->pino_nlink, inode->i_mapping->nrpages);
unlock_new_inode(inode);
+ f->inocache->state_new = INO_STATE_N_NEW;
d_instantiate(dentry, inode);
return 0;
fail:
iget_failed(inode);
jffs2_free_raw_inode(ri);
+ f->inocache->state_new = INO_STATE_N_NEW;
return ret;
}
@@ -304,11 +308,13 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
* Just the node will do for now, though
*/
namelen = dentry->d_name.len;
+ mutex_lock(&c->create_sem);
ret = jffs2_reserve_space(c, sizeof(*ri) + targetlen, &alloclen,
ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
if (ret) {
jffs2_free_raw_inode(ri);
+ mutex_unlock(&c->create_sem);
return ret;
}
@@ -317,6 +323,7 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
if (IS_ERR(inode)) {
jffs2_free_raw_inode(ri);
jffs2_complete_reservation(c);
+ mutex_unlock(&c->create_sem);
return PTR_ERR(inode);
}
@@ -429,11 +436,15 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
jffs2_complete_reservation(c);
unlock_new_inode(inode);
+ f->inocache->state_new = INO_STATE_N_NEW;
+ mutex_unlock(&c->create_sem);
d_instantiate(dentry, inode);
return 0;
fail:
iget_failed(inode);
+ f->inocache->state_new = INO_STATE_N_NEW;
+ mutex_unlock(&c->create_sem);
return ret;
}
@@ -463,11 +474,13 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
* Just the node will do for now, though
*/
namelen = dentry->d_name.len;
+ mutex_lock(&c->create_sem);
ret = jffs2_reserve_space(c, sizeof(*ri), &alloclen, ALLOC_NORMAL,
JFFS2_SUMMARY_INODE_SIZE);
if (ret) {
jffs2_free_raw_inode(ri);
+ mutex_unlock(&c->create_sem);
return ret;
}
@@ -476,6 +489,7 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
if (IS_ERR(inode)) {
jffs2_free_raw_inode(ri);
jffs2_complete_reservation(c);
+ mutex_unlock(&c->create_sem);
return PTR_ERR(inode);
}
@@ -574,11 +588,15 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
jffs2_complete_reservation(c);
unlock_new_inode(inode);
+ f->inocache->state_new = INO_STATE_N_NEW;
+ mutex_unlock(&c->create_sem);
d_instantiate(dentry, inode);
return 0;
fail:
iget_failed(inode);
+ f->inocache->state_new = INO_STATE_N_NEW;
+ mutex_unlock(&c->create_sem);
return ret;
}
@@ -634,11 +652,13 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
* Just the node will do for now, though
*/
namelen = dentry->d_name.len;
+ mutex_lock(&c->create_sem);
ret = jffs2_reserve_space(c, sizeof(*ri) + devlen, &alloclen,
ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
if (ret) {
jffs2_free_raw_inode(ri);
+ mutex_unlock(&c->create_sem);
return ret;
}
@@ -647,6 +667,7 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
if (IS_ERR(inode)) {
jffs2_free_raw_inode(ri);
jffs2_complete_reservation(c);
+ mutex_unlock(&c->create_sem);
return PTR_ERR(inode);
}
inode->i_op = &jffs2_file_inode_operations;
@@ -746,11 +767,15 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
jffs2_complete_reservation(c);
unlock_new_inode(inode);
+ f->inocache->state_new = INO_STATE_N_NEW;
+ mutex_unlock(&c->create_sem);
d_instantiate(dentry, inode);
return 0;
fail:
iget_failed(inode);
+ f->inocache->state_new = INO_STATE_N_NEW;
+ mutex_unlock(&c->create_sem);
return ret;
}
diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 2caf168..c98fd3c 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -482,6 +482,7 @@ struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r
mutex_unlock(&f->sem);
make_bad_inode(inode);
iput(inode);
+ f->inocache->state_new = INO_STATE_N_NEW;
return ERR_PTR(-EINVAL);
}
diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
index 5a2dec2..51f2a4b 100644
--- a/fs/jffs2/gc.c
+++ b/fs/jffs2/gc.c
@@ -333,6 +333,17 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
__func__, jeb->offset, ref_offset(raw), ref_flags(raw),
ic->ino);
+ /* create but not finished so find next block */
+ if (ic->state_new == INO_STATE_I_NEW) {
+ jffs2_dbg(1, "%s(): ino #%u in state new so find next block\n",
+ __func__, ic->ino);
+ c->gcblock = NULL;
+ list_add_tail(&jeb->list, &c->dirty_list);
+ mutex_unlock(&c->alloc_sem);
+ spin_unlock(&c->inocache_lock);
+ return 0;
+ }
+
/* Three possibilities:
1. Inode is already in-core. We must iget it and do proper
updating to its fragtree, etc.
diff --git a/fs/jffs2/jffs2_fs_sb.h b/fs/jffs2/jffs2_fs_sb.h
index 046fee8..32193b7 100644
--- a/fs/jffs2/jffs2_fs_sb.h
+++ b/fs/jffs2/jffs2_fs_sb.h
@@ -57,6 +57,7 @@ struct jffs2_sb_info {
struct completion gc_thread_start; /* GC thread start completion */
struct completion gc_thread_exit; /* GC thread exit completion port */
+ struct mutex create_sem; /* Used to protect write dnode and dirent in order */
struct mutex alloc_sem; /* Used to protect all the following
fields, and also to protect against
out-of-order writing of nodes. And GC. */
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index fa35ff7..ec377e0 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -180,6 +180,7 @@ struct jffs2_inode_cache {
here; other inodes store nlink.
Zero always means that it's
completely unlinked. */
+ uint32_t state_new; /* create flag */
};
/* Inode states for 'state' above. We need the 'GC' state to prevent
@@ -193,6 +194,9 @@ struct jffs2_inode_cache {
#define INO_STATE_READING 5 /* In read_inode() */
#define INO_STATE_CLEARING 6 /* In clear_inode() */
+#define INO_STATE_N_NEW 0 /* Finish to create */
+#define INO_STATE_I_NEW 1 /* Just create but not finish */
+
#define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
#define RAWNODE_CLASS_INODE_CACHE 0
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index d86c5e3..99c5715 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -292,6 +292,7 @@ static int jffs2_fill_super(struct super_block *sb, void *data, int silent)
/* Initialize JFFS2 superblock locks, the further initialization will
* be done later */
+ mutex_init(&c->create_sem);
mutex_init(&c->alloc_sem);
mutex_init(&c->erase_free_sem);
init_waitqueue_head(&c->erase_wait);
diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
index b634de4..dc8242d 100644
--- a/fs/jffs2/write.c
+++ b/fs/jffs2/write.c
@@ -36,6 +36,7 @@ int jffs2_do_new_inode(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
f->inocache->pino_nlink = 1; /* Will be overwritten shortly for directories */
f->inocache->nodes = (struct jffs2_raw_node_ref *)f->inocache;
f->inocache->state = INO_STATE_PRESENT;
+ f->inocache->state_new = INO_STATE_I_NEW;
jffs2_add_ino_cache(c, f->inocache);
jffs2_dbg(1, "%s(): Assigned ino# %d\n", __func__, f->inocache->ino);
--
1.7.6
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] jffs2: bug fix of creating node when gc or find space
2015-12-11 1:36 [PATCH] jffs2: bug fix of creating node when gc or find space gaoyongliang
@ 2015-12-11 2:58 ` Brian Norris
2015-12-12 6:54 ` gaoyongliang
0 siblings, 1 reply; 4+ messages in thread
From: Brian Norris @ 2015-12-11 2:58 UTC (permalink / raw)
To: gaoyongliang, David Woodhouse
Cc: linux-mtd, zhihui.gao, akpm, stable, chenjie6
+ David (JFFS2 "maintainer")
David,
Looks like there was a similar patch / bug report from Huawei about a
year ago, though this one has some modifications:
http://patchwork.ozlabs.org/patch/416301/
It's among the outstanding jffs2 patches from the last 2 years, after I
filtered out the noise:
http://patchwork.ozlabs.org/project/linux-mtd/list/?q=jffs2
They're mostly marked as delegated to you. What would you like to do
with them?
On Fri, Dec 11, 2015 at 09:36:55AM +0800, gaoyongliang@huawei.com wrote:
> From: gaoyongliang <gaoyongliang@huawei.com>
gayongliang,
It's standard practice to use your full name for patch submittions in
the 'From:' and 'Signed-off-by:' portions. See
Documentation/SubmittingPatches. While I can probably guess at the space
and capitalization of your name, it's probably best if you can do this
yourself.
>
> 1. create inode by insert_inode_locked, and return inode with I_NEW.
> 2. has writed the dnode successfully in the end of one block, then
> release the c->alloc_sem, but the dirent is not written yet.
> 3. the gc or jffs2_reserve_space which has got the c->alloc_sem may
> find the block which the dnode we just writed in, the dnode can't
> be gc because the dirent is not written and the state of inode is
> still I_NEW, we can only wait on inode without release c->alloc_sem.
> 4. we can't get the c->alloc_sem to write the dirent, so the inode
> state keep I_NEW forever.
> 5. this will cause deadlock, like ABBA deadlock.
>
> lockf2.test D c02dead8 0 11666 1 0x00000001
> locked:
> c90f9be8 &inode->i_mutex 0 [<c00bf158>] generic_file_aio_write+0x40/0xb0
> c2c54c44 &c->alloc_sem 1 [<bf056f9c>] jffs2_garbage_collect_pass+0x1c/0xf08 [jffs2]
> [<c02dead8>] (__schedule+0x458/0x604) from [<c0114090>] (inode_wait+0x8/0x10)
> [<c0114090>] (inode_wait+0x8/0x10) from [<c02dd050>] (__wait_on_bit+0x54/0xa0)
> [<c02dd050>] (__wait_on_bit+0x54/0xa0) from [<c02dd114>] (out_of_line_wait_on_bit+0x78/0x84)
> [<c02dd114>] (out_of_line_wait_on_bit+0x78/0x84) from [<c01157a0>] (iget_locked+0x90/0x1b0)
> [<c01157a0>] (iget_locked+0x90/0x1b0) from [<bf059fe8>] (jffs2_iget+0xc/0x344 [jffs2])
> [<bf059fe8>] (jffs2_iget+0xc/0x344 [jffs2]) from [<bf05a6fc>] (jffs2_gc_fetch_inode+0x104/0x158 [jffs2])
> [<bf05a6fc>] (jffs2_gc_fetch_inode+0x104/0x158 [jffs2]) from [<bf0577f0>] (jffs2_garbage_collect_pass+0x870/0xf08 [jffs2])
> [<bf0577f0>] (jffs2_garbage_collect_pass+0x870/0xf08 [jffs2]) from [<bf051248>] (jffs2_reserve_space+0x154/0x3b4 [jffs2])
> [<bf051248>] (jffs2_reserve_space+0x154/0x3b4 [jffs2]) from [<bf053eac>] (jffs2_write_inode_range+0x58/0x3ac [jffs2])
> [<bf053eac>] (jffs2_write_inode_range+0x58/0x3ac [jffs2]) from [<bf04ec20>] (jffs2_write_end+0x11c/0x224 [jffs2])
> [<bf04ec20>] (jffs2_write_end+0x11c/0x224 [jffs2]) from [<c00bdfa4>] (generic_file_buffered_write+0x160/0x23c)
> [<c00bdfa4>] (generic_file_buffered_write+0x160/0x23c) from [<c00bf0ac>] (__generic_file_aio_write+0x328/0x394)
> [<c00bf0ac>] (__generic_file_aio_write+0x328/0x394) from [<c00bf16c>] (generic_file_aio_write+0x54/0xb0)
> [<c00bf16c>] (generic_file_aio_write+0x54/0xb0) from [<c00fdc24>] (do_sync_write+0x74/0x98)
> [<c00fdc24>] (do_sync_write+0x74/0x98) from [<c00fe550>] (vfs_write+0xcc/0x174)
> [<c00fe550>] (vfs_write+0xcc/0x174) from [<c00fe8a8>] (SyS_write+0x38/0x64)
> [<c00fe8a8>] (SyS_write+0x38/0x64) from [<c000f0c0>] (ret_fast_syscall+0x0/0x58)
>
> Fixes: e72e6497e74811e01d72b4c1b7537b3aea3ee857
FWIW, this isn't the preferred style for 'Fixes:' lines. It's nice to
include the commit subject too, like:
Fixes: commit ("e72e6497e748 jffs2: Fix NFS race by using insert_inode_locked()")
> Cc: <stable@vger.kernel.org> # 3.10.y+
> Signed-off-by: gaoyongliang <gaoyongliang@huawei.com>
> ---
> fs/jffs2/dir.c | 25 +++++++++++++++++++++++++
> fs/jffs2/fs.c | 1 +
> fs/jffs2/gc.c | 11 +++++++++++
> fs/jffs2/jffs2_fs_sb.h | 1 +
> fs/jffs2/nodelist.h | 4 ++++
> fs/jffs2/super.c | 1 +
> fs/jffs2/write.c | 1 +
> 7 files changed, 44 insertions(+), 0 deletions(-)
This patch looks awfully similar to chenjie's patch, but it has a bit
different diffstat. Presumably there were reasons for the changes. Can
you include a changelog? (e.g., "changed foo becuase of bar")
No other comment from me on the patch.
Brian
>
> diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
> index d211b8e..808ec17 100644
> --- a/fs/jffs2/dir.c
> +++ b/fs/jffs2/dir.c
> @@ -195,7 +195,9 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry,
> no chance of AB-BA deadlock involving its f->sem). */
> mutex_unlock(&f->sem);
>
> + mutex_lock(&c->create_sem);
> ret = jffs2_do_create(c, dir_f, f, ri, &dentry->d_name);
> + mutex_unlock(&c->create_sem);
> if (ret)
> goto fail;
>
> @@ -208,12 +210,14 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry,
> f->inocache->pino_nlink, inode->i_mapping->nrpages);
>
> unlock_new_inode(inode);
> + f->inocache->state_new = INO_STATE_N_NEW;
> d_instantiate(dentry, inode);
> return 0;
>
> fail:
> iget_failed(inode);
> jffs2_free_raw_inode(ri);
> + f->inocache->state_new = INO_STATE_N_NEW;
> return ret;
> }
>
> @@ -304,11 +308,13 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
> * Just the node will do for now, though
> */
> namelen = dentry->d_name.len;
> + mutex_lock(&c->create_sem);
> ret = jffs2_reserve_space(c, sizeof(*ri) + targetlen, &alloclen,
> ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>
> if (ret) {
> jffs2_free_raw_inode(ri);
> + mutex_unlock(&c->create_sem);
> return ret;
> }
>
> @@ -317,6 +323,7 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
> if (IS_ERR(inode)) {
> jffs2_free_raw_inode(ri);
> jffs2_complete_reservation(c);
> + mutex_unlock(&c->create_sem);
> return PTR_ERR(inode);
> }
>
> @@ -429,11 +436,15 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
> jffs2_complete_reservation(c);
>
> unlock_new_inode(inode);
> + f->inocache->state_new = INO_STATE_N_NEW;
> + mutex_unlock(&c->create_sem);
> d_instantiate(dentry, inode);
> return 0;
>
> fail:
> iget_failed(inode);
> + f->inocache->state_new = INO_STATE_N_NEW;
> + mutex_unlock(&c->create_sem);
> return ret;
> }
>
> @@ -463,11 +474,13 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
> * Just the node will do for now, though
> */
> namelen = dentry->d_name.len;
> + mutex_lock(&c->create_sem);
> ret = jffs2_reserve_space(c, sizeof(*ri), &alloclen, ALLOC_NORMAL,
> JFFS2_SUMMARY_INODE_SIZE);
>
> if (ret) {
> jffs2_free_raw_inode(ri);
> + mutex_unlock(&c->create_sem);
> return ret;
> }
>
> @@ -476,6 +489,7 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
> if (IS_ERR(inode)) {
> jffs2_free_raw_inode(ri);
> jffs2_complete_reservation(c);
> + mutex_unlock(&c->create_sem);
> return PTR_ERR(inode);
> }
>
> @@ -574,11 +588,15 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
> jffs2_complete_reservation(c);
>
> unlock_new_inode(inode);
> + f->inocache->state_new = INO_STATE_N_NEW;
> + mutex_unlock(&c->create_sem);
> d_instantiate(dentry, inode);
> return 0;
>
> fail:
> iget_failed(inode);
> + f->inocache->state_new = INO_STATE_N_NEW;
> + mutex_unlock(&c->create_sem);
> return ret;
> }
>
> @@ -634,11 +652,13 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
> * Just the node will do for now, though
> */
> namelen = dentry->d_name.len;
> + mutex_lock(&c->create_sem);
> ret = jffs2_reserve_space(c, sizeof(*ri) + devlen, &alloclen,
> ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>
> if (ret) {
> jffs2_free_raw_inode(ri);
> + mutex_unlock(&c->create_sem);
> return ret;
> }
>
> @@ -647,6 +667,7 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
> if (IS_ERR(inode)) {
> jffs2_free_raw_inode(ri);
> jffs2_complete_reservation(c);
> + mutex_unlock(&c->create_sem);
> return PTR_ERR(inode);
> }
> inode->i_op = &jffs2_file_inode_operations;
> @@ -746,11 +767,15 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
> jffs2_complete_reservation(c);
>
> unlock_new_inode(inode);
> + f->inocache->state_new = INO_STATE_N_NEW;
> + mutex_unlock(&c->create_sem);
> d_instantiate(dentry, inode);
> return 0;
>
> fail:
> iget_failed(inode);
> + f->inocache->state_new = INO_STATE_N_NEW;
> + mutex_unlock(&c->create_sem);
> return ret;
> }
>
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index 2caf168..c98fd3c 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -482,6 +482,7 @@ struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r
> mutex_unlock(&f->sem);
> make_bad_inode(inode);
> iput(inode);
> + f->inocache->state_new = INO_STATE_N_NEW;
> return ERR_PTR(-EINVAL);
> }
>
> diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
> index 5a2dec2..51f2a4b 100644
> --- a/fs/jffs2/gc.c
> +++ b/fs/jffs2/gc.c
> @@ -333,6 +333,17 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
> __func__, jeb->offset, ref_offset(raw), ref_flags(raw),
> ic->ino);
>
> + /* create but not finished so find next block */
> + if (ic->state_new == INO_STATE_I_NEW) {
> + jffs2_dbg(1, "%s(): ino #%u in state new so find next block\n",
> + __func__, ic->ino);
> + c->gcblock = NULL;
> + list_add_tail(&jeb->list, &c->dirty_list);
> + mutex_unlock(&c->alloc_sem);
> + spin_unlock(&c->inocache_lock);
> + return 0;
> + }
> +
> /* Three possibilities:
> 1. Inode is already in-core. We must iget it and do proper
> updating to its fragtree, etc.
> diff --git a/fs/jffs2/jffs2_fs_sb.h b/fs/jffs2/jffs2_fs_sb.h
> index 046fee8..32193b7 100644
> --- a/fs/jffs2/jffs2_fs_sb.h
> +++ b/fs/jffs2/jffs2_fs_sb.h
> @@ -57,6 +57,7 @@ struct jffs2_sb_info {
> struct completion gc_thread_start; /* GC thread start completion */
> struct completion gc_thread_exit; /* GC thread exit completion port */
>
> + struct mutex create_sem; /* Used to protect write dnode and dirent in order */
> struct mutex alloc_sem; /* Used to protect all the following
> fields, and also to protect against
> out-of-order writing of nodes. And GC. */
> diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
> index fa35ff7..ec377e0 100644
> --- a/fs/jffs2/nodelist.h
> +++ b/fs/jffs2/nodelist.h
> @@ -180,6 +180,7 @@ struct jffs2_inode_cache {
> here; other inodes store nlink.
> Zero always means that it's
> completely unlinked. */
> + uint32_t state_new; /* create flag */
> };
>
> /* Inode states for 'state' above. We need the 'GC' state to prevent
> @@ -193,6 +194,9 @@ struct jffs2_inode_cache {
> #define INO_STATE_READING 5 /* In read_inode() */
> #define INO_STATE_CLEARING 6 /* In clear_inode() */
>
> +#define INO_STATE_N_NEW 0 /* Finish to create */
> +#define INO_STATE_I_NEW 1 /* Just create but not finish */
> +
> #define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
>
> #define RAWNODE_CLASS_INODE_CACHE 0
> diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
> index d86c5e3..99c5715 100644
> --- a/fs/jffs2/super.c
> +++ b/fs/jffs2/super.c
> @@ -292,6 +292,7 @@ static int jffs2_fill_super(struct super_block *sb, void *data, int silent)
>
> /* Initialize JFFS2 superblock locks, the further initialization will
> * be done later */
> + mutex_init(&c->create_sem);
> mutex_init(&c->alloc_sem);
> mutex_init(&c->erase_free_sem);
> init_waitqueue_head(&c->erase_wait);
> diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
> index b634de4..dc8242d 100644
> --- a/fs/jffs2/write.c
> +++ b/fs/jffs2/write.c
> @@ -36,6 +36,7 @@ int jffs2_do_new_inode(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
> f->inocache->pino_nlink = 1; /* Will be overwritten shortly for directories */
> f->inocache->nodes = (struct jffs2_raw_node_ref *)f->inocache;
> f->inocache->state = INO_STATE_PRESENT;
> + f->inocache->state_new = INO_STATE_I_NEW;
>
> jffs2_add_ino_cache(c, f->inocache);
> jffs2_dbg(1, "%s(): Assigned ino# %d\n", __func__, f->inocache->ino);
> --
> 1.7.6
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] jffs2: bug fix of creating node when gc or find space
2015-12-11 2:58 ` Brian Norris
@ 2015-12-12 6:54 ` gaoyongliang
0 siblings, 0 replies; 4+ messages in thread
From: gaoyongliang @ 2015-12-12 6:54 UTC (permalink / raw)
To: Brian Norris, David Woodhouse
Cc: linux-mtd, zhihui.gao, akpm, stable, chenjie6, James.Liu,
Kuaikuai.Wu, panxuesong
Dear Brian,
Thanks for your advises for this patch.
The reported bug is the same as Chen Jie in Huawei, but his fix patch has
some problem.
1. when gc find one block which created node but not finished, Chen Jie's
patch only set 'c->gcblock = NULL', but not return this jeb to the block
list. when delete jeb->list in jffs2_mark_node_obsolete(), it would make
the kernel panic because of double list_del().
+ /*create but not finished so find next block*/
+ if (ic->state_new == INO_STATE_I_NEW) {
+ jffs2_dbg(1, "%s(): ino #%u in state new so find next block\n",
+ __func__, ic->ino);
+ c->gcblock = NULL;
+ mutex_unlock(&c->alloc_sem);
+ spin_unlock(&c->inocache_lock);
+ return 0;
+ }
Unable to handle kernel paging request at virtual address 00100104
......
[<bf2bcd04>] (jffs2_mark_node_obsolete+0x298/0x344 [jffs2]) from [<bf2bb19c>] (jffs2_obsolete_node_frag+0x38/0x60 [jffs2])
[<bf2bb19c>] (jffs2_obsolete_node_frag+0x38/0x60 [jffs2]) from [<bf2bb9d8>] (jffs2_add_full_dnode_to_inode+0x2ac/0x3c0 [jffs2])
[<bf2bb9d8>] (jffs2_add_full_dnode_to_inode+0x2ac/0x3c0 [jffs2]) from [<bf2c3704>] (jffs2_garbage_collect_dnode.isra.3+0x408/0x47c [jffs2])
[<bf2c3704>] (jffs2_garbage_collect_dnode.isra.3+0x408/0x47c [jffs2]) from [<bf2c43e0>] (jffs2_garbage_collect_pass+0xb14/0xfbc [jffs2])
[<bf2c43e0>] (jffs2_garbage_collect_pass+0xb14/0xfbc [jffs2]) from [<bf2bd414>] (jffs2_reserve_space+0x154/0x3b4 [jffs2])
[<bf2bd414>] (jffs2_reserve_space+0x154/0x3b4 [jffs2]) from [<bf2c05f8>] (jffs2_write_inode_range+0x58/0x3ac [jffs2])
[<bf2c05f8>] (jffs2_write_inode_range+0x58/0x3ac [jffs2]) from [<bf2bac78>] (jffs2_write_end+0x11c/0x224 [jffs2])
[<bf2bac78>] (jffs2_write_end+0x11c/0x224 [jffs2]) from [<c00beb78>] (generic_file_buffered_write+0x160/0x23c)
[<c00beb78>] (generic_file_buffered_write+0x160/0x23c) from [<c00bfc80>] (__generic_file_aio_write+0x328/0x394)
[<c00bfc80>] (__generic_file_aio_write+0x328/0x394) from [<c00bfd40>] (generic_file_aio_write+0x54/0xb0)
[<c00bfd40>] (generic_file_aio_write+0x54/0xb0) from [<c00feca0>] (do_sync_write+0x74/0x98)
[<c00feca0>] (do_sync_write+0x74/0x98) from [<c00ff63c>] (vfs_write+0xcc/0x1a8)
[<c00ff63c>] (vfs_write+0xcc/0x1a8) from [<c00ffa30>] (SyS_write+0x38/0x64)
[<c00ffa30>] (SyS_write+0x38/0x64) from [<c00102c0>] (ret_fast_syscall+0x0/0x58)
I think this jeb should be added to the dirty list.
2. when hundreds of processes are creating node but not finished(after finishing
writing dnode, it will release the lock), a lot of jebs would be added to
dirty_list, but this jebs can't be gc until finishing to create node. it
would cause 'no free space left for GC', actually, there are some jebs in
the dirty list.
jffs2: Argh. No free space left for GC. nr_erasing_blocks is 0. nr_free_blocks is 0. (erasableempty: yes, erasingempty: yes, erasependingempty: yes)
jffs2: Error garbage collecting node at 00c5ecf8!
I add a new create_sem to protect creating dnode and dirent in order.
Gao Yongliang
On 2015/12/11 10:58, Brian Norris wrote:
> + David (JFFS2 "maintainer")
>
> David,
>
> Looks like there was a similar patch / bug report from Huawei about a
> year ago, though this one has some modifications:
>
> http://patchwork.ozlabs.org/patch/416301/
>
> It's among the outstanding jffs2 patches from the last 2 years, after I
> filtered out the noise:
>
> http://patchwork.ozlabs.org/project/linux-mtd/list/?q=jffs2
>
> They're mostly marked as delegated to you. What would you like to do
> with them?
>
> On Fri, Dec 11, 2015 at 09:36:55AM +0800, gaoyongliang@huawei.com wrote:
>> From: gaoyongliang <gaoyongliang@huawei.com>
>
> gayongliang,
>
> It's standard practice to use your full name for patch submittions in
> the 'From:' and 'Signed-off-by:' portions. See
> Documentation/SubmittingPatches. While I can probably guess at the space
> and capitalization of your name, it's probably best if you can do this
> yourself.
>
>>
>> 1. create inode by insert_inode_locked, and return inode with I_NEW.
>> 2. has writed the dnode successfully in the end of one block, then
>> release the c->alloc_sem, but the dirent is not written yet.
>> 3. the gc or jffs2_reserve_space which has got the c->alloc_sem may
>> find the block which the dnode we just writed in, the dnode can't
>> be gc because the dirent is not written and the state of inode is
>> still I_NEW, we can only wait on inode without release c->alloc_sem.
>> 4. we can't get the c->alloc_sem to write the dirent, so the inode
>> state keep I_NEW forever.
>> 5. this will cause deadlock, like ABBA deadlock.
>>
>> lockf2.test D c02dead8 0 11666 1 0x00000001
>> locked:
>> c90f9be8 &inode->i_mutex 0 [<c00bf158>] generic_file_aio_write+0x40/0xb0
>> c2c54c44 &c->alloc_sem 1 [<bf056f9c>] jffs2_garbage_collect_pass+0x1c/0xf08 [jffs2]
>> [<c02dead8>] (__schedule+0x458/0x604) from [<c0114090>] (inode_wait+0x8/0x10)
>> [<c0114090>] (inode_wait+0x8/0x10) from [<c02dd050>] (__wait_on_bit+0x54/0xa0)
>> [<c02dd050>] (__wait_on_bit+0x54/0xa0) from [<c02dd114>] (out_of_line_wait_on_bit+0x78/0x84)
>> [<c02dd114>] (out_of_line_wait_on_bit+0x78/0x84) from [<c01157a0>] (iget_locked+0x90/0x1b0)
>> [<c01157a0>] (iget_locked+0x90/0x1b0) from [<bf059fe8>] (jffs2_iget+0xc/0x344 [jffs2])
>> [<bf059fe8>] (jffs2_iget+0xc/0x344 [jffs2]) from [<bf05a6fc>] (jffs2_gc_fetch_inode+0x104/0x158 [jffs2])
>> [<bf05a6fc>] (jffs2_gc_fetch_inode+0x104/0x158 [jffs2]) from [<bf0577f0>] (jffs2_garbage_collect_pass+0x870/0xf08 [jffs2])
>> [<bf0577f0>] (jffs2_garbage_collect_pass+0x870/0xf08 [jffs2]) from [<bf051248>] (jffs2_reserve_space+0x154/0x3b4 [jffs2])
>> [<bf051248>] (jffs2_reserve_space+0x154/0x3b4 [jffs2]) from [<bf053eac>] (jffs2_write_inode_range+0x58/0x3ac [jffs2])
>> [<bf053eac>] (jffs2_write_inode_range+0x58/0x3ac [jffs2]) from [<bf04ec20>] (jffs2_write_end+0x11c/0x224 [jffs2])
>> [<bf04ec20>] (jffs2_write_end+0x11c/0x224 [jffs2]) from [<c00bdfa4>] (generic_file_buffered_write+0x160/0x23c)
>> [<c00bdfa4>] (generic_file_buffered_write+0x160/0x23c) from [<c00bf0ac>] (__generic_file_aio_write+0x328/0x394)
>> [<c00bf0ac>] (__generic_file_aio_write+0x328/0x394) from [<c00bf16c>] (generic_file_aio_write+0x54/0xb0)
>> [<c00bf16c>] (generic_file_aio_write+0x54/0xb0) from [<c00fdc24>] (do_sync_write+0x74/0x98)
>> [<c00fdc24>] (do_sync_write+0x74/0x98) from [<c00fe550>] (vfs_write+0xcc/0x174)
>> [<c00fe550>] (vfs_write+0xcc/0x174) from [<c00fe8a8>] (SyS_write+0x38/0x64)
>> [<c00fe8a8>] (SyS_write+0x38/0x64) from [<c000f0c0>] (ret_fast_syscall+0x0/0x58)
>>
>> Fixes: e72e6497e74811e01d72b4c1b7537b3aea3ee857
>
> FWIW, this isn't the preferred style for 'Fixes:' lines. It's nice to
> include the commit subject too, like:
>
> Fixes: commit ("e72e6497e748 jffs2: Fix NFS race by using insert_inode_locked()")
>
>> Cc: <stable@vger.kernel.org> # 3.10.y+
>> Signed-off-by: gaoyongliang <gaoyongliang@huawei.com>
>> ---
>> fs/jffs2/dir.c | 25 +++++++++++++++++++++++++
>> fs/jffs2/fs.c | 1 +
>> fs/jffs2/gc.c | 11 +++++++++++
>> fs/jffs2/jffs2_fs_sb.h | 1 +
>> fs/jffs2/nodelist.h | 4 ++++
>> fs/jffs2/super.c | 1 +
>> fs/jffs2/write.c | 1 +
>> 7 files changed, 44 insertions(+), 0 deletions(-)
>
> This patch looks awfully similar to chenjie's patch, but it has a bit
> different diffstat. Presumably there were reasons for the changes. Can
> you include a changelog? (e.g., "changed foo becuase of bar")
>
> No other comment from me on the patch.
>
> Brian
>
>>
>> diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
>> index d211b8e..808ec17 100644
>> --- a/fs/jffs2/dir.c
>> +++ b/fs/jffs2/dir.c
>> @@ -195,7 +195,9 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry,
>> no chance of AB-BA deadlock involving its f->sem). */
>> mutex_unlock(&f->sem);
>>
>> + mutex_lock(&c->create_sem);
>> ret = jffs2_do_create(c, dir_f, f, ri, &dentry->d_name);
>> + mutex_unlock(&c->create_sem);
>> if (ret)
>> goto fail;
>>
>> @@ -208,12 +210,14 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry,
>> f->inocache->pino_nlink, inode->i_mapping->nrpages);
>>
>> unlock_new_inode(inode);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> d_instantiate(dentry, inode);
>> return 0;
>>
>> fail:
>> iget_failed(inode);
>> jffs2_free_raw_inode(ri);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> return ret;
>> }
>>
>> @@ -304,11 +308,13 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
>> * Just the node will do for now, though
>> */
>> namelen = dentry->d_name.len;
>> + mutex_lock(&c->create_sem);
>> ret = jffs2_reserve_space(c, sizeof(*ri) + targetlen, &alloclen,
>> ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>>
>> if (ret) {
>> jffs2_free_raw_inode(ri);
>> + mutex_unlock(&c->create_sem);
>> return ret;
>> }
>>
>> @@ -317,6 +323,7 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
>> if (IS_ERR(inode)) {
>> jffs2_free_raw_inode(ri);
>> jffs2_complete_reservation(c);
>> + mutex_unlock(&c->create_sem);
>> return PTR_ERR(inode);
>> }
>>
>> @@ -429,11 +436,15 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
>> jffs2_complete_reservation(c);
>>
>> unlock_new_inode(inode);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> + mutex_unlock(&c->create_sem);
>> d_instantiate(dentry, inode);
>> return 0;
>>
>> fail:
>> iget_failed(inode);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> + mutex_unlock(&c->create_sem);
>> return ret;
>> }
>>
>> @@ -463,11 +474,13 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
>> * Just the node will do for now, though
>> */
>> namelen = dentry->d_name.len;
>> + mutex_lock(&c->create_sem);
>> ret = jffs2_reserve_space(c, sizeof(*ri), &alloclen, ALLOC_NORMAL,
>> JFFS2_SUMMARY_INODE_SIZE);
>>
>> if (ret) {
>> jffs2_free_raw_inode(ri);
>> + mutex_unlock(&c->create_sem);
>> return ret;
>> }
>>
>> @@ -476,6 +489,7 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
>> if (IS_ERR(inode)) {
>> jffs2_free_raw_inode(ri);
>> jffs2_complete_reservation(c);
>> + mutex_unlock(&c->create_sem);
>> return PTR_ERR(inode);
>> }
>>
>> @@ -574,11 +588,15 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
>> jffs2_complete_reservation(c);
>>
>> unlock_new_inode(inode);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> + mutex_unlock(&c->create_sem);
>> d_instantiate(dentry, inode);
>> return 0;
>>
>> fail:
>> iget_failed(inode);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> + mutex_unlock(&c->create_sem);
>> return ret;
>> }
>>
>> @@ -634,11 +652,13 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
>> * Just the node will do for now, though
>> */
>> namelen = dentry->d_name.len;
>> + mutex_lock(&c->create_sem);
>> ret = jffs2_reserve_space(c, sizeof(*ri) + devlen, &alloclen,
>> ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>>
>> if (ret) {
>> jffs2_free_raw_inode(ri);
>> + mutex_unlock(&c->create_sem);
>> return ret;
>> }
>>
>> @@ -647,6 +667,7 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
>> if (IS_ERR(inode)) {
>> jffs2_free_raw_inode(ri);
>> jffs2_complete_reservation(c);
>> + mutex_unlock(&c->create_sem);
>> return PTR_ERR(inode);
>> }
>> inode->i_op = &jffs2_file_inode_operations;
>> @@ -746,11 +767,15 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
>> jffs2_complete_reservation(c);
>>
>> unlock_new_inode(inode);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> + mutex_unlock(&c->create_sem);
>> d_instantiate(dentry, inode);
>> return 0;
>>
>> fail:
>> iget_failed(inode);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> + mutex_unlock(&c->create_sem);
>> return ret;
>> }
>>
>> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
>> index 2caf168..c98fd3c 100644
>> --- a/fs/jffs2/fs.c
>> +++ b/fs/jffs2/fs.c
>> @@ -482,6 +482,7 @@ struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r
>> mutex_unlock(&f->sem);
>> make_bad_inode(inode);
>> iput(inode);
>> + f->inocache->state_new = INO_STATE_N_NEW;
>> return ERR_PTR(-EINVAL);
>> }
>>
>> diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
>> index 5a2dec2..51f2a4b 100644
>> --- a/fs/jffs2/gc.c
>> +++ b/fs/jffs2/gc.c
>> @@ -333,6 +333,17 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
>> __func__, jeb->offset, ref_offset(raw), ref_flags(raw),
>> ic->ino);
>>
>> + /* create but not finished so find next block */
>> + if (ic->state_new == INO_STATE_I_NEW) {
>> + jffs2_dbg(1, "%s(): ino #%u in state new so find next block\n",
>> + __func__, ic->ino);
>> + c->gcblock = NULL;
>> + list_add_tail(&jeb->list, &c->dirty_list);
>> + mutex_unlock(&c->alloc_sem);
>> + spin_unlock(&c->inocache_lock);
>> + return 0;
>> + }
>> +
>> /* Three possibilities:
>> 1. Inode is already in-core. We must iget it and do proper
>> updating to its fragtree, etc.
>> diff --git a/fs/jffs2/jffs2_fs_sb.h b/fs/jffs2/jffs2_fs_sb.h
>> index 046fee8..32193b7 100644
>> --- a/fs/jffs2/jffs2_fs_sb.h
>> +++ b/fs/jffs2/jffs2_fs_sb.h
>> @@ -57,6 +57,7 @@ struct jffs2_sb_info {
>> struct completion gc_thread_start; /* GC thread start completion */
>> struct completion gc_thread_exit; /* GC thread exit completion port */
>>
>> + struct mutex create_sem; /* Used to protect write dnode and dirent in order */
>> struct mutex alloc_sem; /* Used to protect all the following
>> fields, and also to protect against
>> out-of-order writing of nodes. And GC. */
>> diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
>> index fa35ff7..ec377e0 100644
>> --- a/fs/jffs2/nodelist.h
>> +++ b/fs/jffs2/nodelist.h
>> @@ -180,6 +180,7 @@ struct jffs2_inode_cache {
>> here; other inodes store nlink.
>> Zero always means that it's
>> completely unlinked. */
>> + uint32_t state_new; /* create flag */
>> };
>>
>> /* Inode states for 'state' above. We need the 'GC' state to prevent
>> @@ -193,6 +194,9 @@ struct jffs2_inode_cache {
>> #define INO_STATE_READING 5 /* In read_inode() */
>> #define INO_STATE_CLEARING 6 /* In clear_inode() */
>>
>> +#define INO_STATE_N_NEW 0 /* Finish to create */
>> +#define INO_STATE_I_NEW 1 /* Just create but not finish */
>> +
>> #define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
>>
>> #define RAWNODE_CLASS_INODE_CACHE 0
>> diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
>> index d86c5e3..99c5715 100644
>> --- a/fs/jffs2/super.c
>> +++ b/fs/jffs2/super.c
>> @@ -292,6 +292,7 @@ static int jffs2_fill_super(struct super_block *sb, void *data, int silent)
>>
>> /* Initialize JFFS2 superblock locks, the further initialization will
>> * be done later */
>> + mutex_init(&c->create_sem);
>> mutex_init(&c->alloc_sem);
>> mutex_init(&c->erase_free_sem);
>> init_waitqueue_head(&c->erase_wait);
>> diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
>> index b634de4..dc8242d 100644
>> --- a/fs/jffs2/write.c
>> +++ b/fs/jffs2/write.c
>> @@ -36,6 +36,7 @@ int jffs2_do_new_inode(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
>> f->inocache->pino_nlink = 1; /* Will be overwritten shortly for directories */
>> f->inocache->nodes = (struct jffs2_raw_node_ref *)f->inocache;
>> f->inocache->state = INO_STATE_PRESENT;
>> + f->inocache->state_new = INO_STATE_I_NEW;
>>
>> jffs2_add_ino_cache(c, f->inocache);
>> jffs2_dbg(1, "%s(): Assigned ino# %d\n", __func__, f->inocache->ino);
>> --
>> 1.7.6
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
> .
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] jffs2: bug fix of creating node when gc or find space
@ 2014-12-01 14:21 chenjie6
0 siblings, 0 replies; 4+ messages in thread
From: chenjie6 @ 2014-12-01 14:21 UTC (permalink / raw)
To: linux-mtd, David.Woodhouse; +Cc: lizefan, zengweilin
From: chenjie <chenjie6@huawei.com>
Creat node by insert_inode_locked, write dnode successfully but dirent
not writed ,so the gc or jffs2_reserve_space may read the block which dnode
writed, the dnode can not been readed because it was created unfinished.
lockf2.test D c02dead8 0 11666 1 0x00000001
locked:
c90f9be8 &inode->i_mutex 0 [<c00bf158>] generic_file_aio_write+0x40/0xb0
c2c54c44 &c->alloc_sem 1 [<bf056f9c>] jffs2_garbage_collect_pass+0x1c/0xf08 [jffs2]
[<c02dead8>] (__schedule+0x458/0x604) from [<c0114090>] (inode_wait+0x8/0x10)
[<c0114090>] (inode_wait+0x8/0x10) from [<c02dd050>] (__wait_on_bit+0x54/0xa0)
[<c02dd050>] (__wait_on_bit+0x54/0xa0) from [<c02dd114>] (out_of_line_wait_on_bit+0x78/0x84)
[<c02dd114>] (out_of_line_wait_on_bit+0x78/0x84) from [<c01157a0>] (iget_locked+0x90/0x1b0)
[<c01157a0>] (iget_locked+0x90/0x1b0) from [<bf059fe8>] (jffs2_iget+0xc/0x344 [jffs2])
[<bf059fe8>] (jffs2_iget+0xc/0x344 [jffs2]) from [<bf05a6fc>] (jffs2_gc_fetch_inode+0x104/0x158 [jffs2])
[<bf05a6fc>] (jffs2_gc_fetch_inode+0x104/0x158 [jffs2]) from [<bf0577f0>] (jffs2_garbage_collect_pass+0x870/0xf08 [jffs2])
[<bf0577f0>] (jffs2_garbage_collect_pass+0x870/0xf08 [jffs2]) from [<bf051248>] (jffs2_reserve_space+0x154/0x3b4 [jffs2])
[<bf051248>] (jffs2_reserve_space+0x154/0x3b4 [jffs2]) from [<bf053eac>] (jffs2_write_inode_range+0x58/0x3ac [jffs2])
[<bf053eac>] (jffs2_write_inode_range+0x58/0x3ac [jffs2]) from [<bf04ec20>] (jffs2_write_end+0x11c/0x224 [jffs2])
[<bf04ec20>] (jffs2_write_end+0x11c/0x224 [jffs2]) from [<c00bdfa4>] (generic_file_buffered_write+0x160/0x23c)
[<c00bdfa4>] (generic_file_buffered_write+0x160/0x23c) from [<c00bf0ac>] (__generic_file_aio_write+0x328/0x394)
[<c00bf0ac>] (__generic_file_aio_write+0x328/0x394) from [<c00bf16c>] (generic_file_aio_write+0x54/0xb0)
[<c00bf16c>] (generic_file_aio_write+0x54/0xb0) from [<c00fdc24>] (do_sync_write+0x74/0x98)
[<c00fdc24>] (do_sync_write+0x74/0x98) from [<c00fe550>] (vfs_write+0xcc/0x174)
[<c00fe550>] (vfs_write+0xcc/0x174) from [<c00fe8a8>] (SyS_write+0x38/0x64)
[<c00fe8a8>] (SyS_write+0x38/0x64) from [<c000f0c0>] (ret_fast_syscall+0x0/0x58)
Fixes:e72e6497e74811e01d72b4c1b7537b3aea3ee857
Cc: <stable@vger.kernel.org>
Signed-off-by: Chen Jie <chenjie6@huawei.com>
---
fs/jffs2/dir.c | 4 ++++
fs/jffs2/gc.c | 10 ++++++++++
fs/jffs2/nodelist.c | 9 +++++++++
fs/jffs2/nodelist.h | 6 ++++++
fs/jffs2/write.c | 1 +
5 files changed, 30 insertions(+)
diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
index 9385560..176bc94 100644
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -208,6 +208,7 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry,
f->inocache->pino_nlink, inode->i_mapping->nrpages);
unlock_new_inode(inode);
+ jffs2_set_inocache_state_new(c, f->inocache, INO_STATE_N_NEW);
d_instantiate(dentry, inode);
return 0;
@@ -428,6 +429,7 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
jffs2_complete_reservation(c);
unlock_new_inode(inode);
+ jffs2_set_inocache_state_new(c, f->inocache, INO_STATE_N_NEW);
d_instantiate(dentry, inode);
return 0;
@@ -573,6 +575,7 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
jffs2_complete_reservation(c);
unlock_new_inode(inode);
+ jffs2_set_inocache_state_new(c, f->inocache, INO_STATE_N_NEW);
d_instantiate(dentry, inode);
return 0;
@@ -748,6 +751,7 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
jffs2_complete_reservation(c);
unlock_new_inode(inode);
+ jffs2_set_inocache_state_new(c, f->inocache, INO_STATE_N_NEW);
d_instantiate(dentry, inode);
return 0;
diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
index 5a2dec2..e7e8c09 100644
--- a/fs/jffs2/gc.c
+++ b/fs/jffs2/gc.c
@@ -333,6 +333,16 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
__func__, jeb->offset, ref_offset(raw), ref_flags(raw),
ic->ino);
+ /*create but not finished so find next block*/
+ if (ic->state_new == INO_STATE_I_NEW) {
+ jffs2_dbg(1, "%s(): ino #%u in state new so find next block\n",
+ __func__, ic->ino);
+ c->gcblock = NULL;
+ mutex_unlock(&c->alloc_sem);
+ spin_unlock(&c->inocache_lock);
+ return 0;
+ }
+
/* Three possibilities:
1. Inode is already in-core. We must iget it and do proper
updating to its fragtree, etc.
diff --git a/fs/jffs2/nodelist.c b/fs/jffs2/nodelist.c
index 9a5449b..9f469c4 100644
--- a/fs/jffs2/nodelist.c
+++ b/fs/jffs2/nodelist.c
@@ -413,6 +413,15 @@ void jffs2_set_inocache_state(struct jffs2_sb_info *c, struct jffs2_inode_cache
spin_unlock(&c->inocache_lock);
}
+void jffs2_set_inocache_state_new(struct jffs2_sb_info *c,
+ struct jffs2_inode_cache *ic, int state)
+{
+ spin_lock(&c->inocache_lock);
+ ic->state_new = state;
+ wake_up(&c->inocache_wq);
+ spin_unlock(&c->inocache_lock);
+}
+
/* During mount, this needs no locking. During normal operation, its
callers want to do other stuff while still holding the inocache_lock.
Rather than introducing special case get_ino_cache functions or
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index fa35ff7..ac2d8c8 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -180,6 +180,7 @@ struct jffs2_inode_cache {
here; other inodes store nlink.
Zero always means that it's
completely unlinked. */
+ uint32_t state_new; /*create flag*/
};
/* Inode states for 'state' above. We need the 'GC' state to prevent
@@ -193,6 +194,9 @@ struct jffs2_inode_cache {
#define INO_STATE_READING 5 /* In read_inode() */
#define INO_STATE_CLEARING 6 /* In clear_inode() */
+#define INO_STATE_I_NEW 0 /* Just create but not finish*/
+#define INO_STATE_N_NEW 1 /* finished*/
+
#define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
#define RAWNODE_CLASS_INODE_CACHE 0
@@ -359,6 +363,8 @@ static inline struct jffs2_node_frag *frag_last(struct rb_root *root)
/* nodelist.c */
void jffs2_add_fd_to_list(struct jffs2_sb_info *c, struct jffs2_full_dirent *new, struct jffs2_full_dirent **list);
void jffs2_set_inocache_state(struct jffs2_sb_info *c, struct jffs2_inode_cache *ic, int state);
+void jffs2_set_inocache_state_new(struct jffs2_sb_info *c,
+ struct jffs2_inode_cache *ic, int state);
struct jffs2_inode_cache *jffs2_get_ino_cache(struct jffs2_sb_info *c, uint32_t ino);
void jffs2_add_ino_cache (struct jffs2_sb_info *c, struct jffs2_inode_cache *new);
void jffs2_del_ino_cache(struct jffs2_sb_info *c, struct jffs2_inode_cache *old);
diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
index b634de4..dc8242d 100644
--- a/fs/jffs2/write.c
+++ b/fs/jffs2/write.c
@@ -36,6 +36,7 @@ int jffs2_do_new_inode(struct jffs2_sb_info *c, struct jffs2_inode_info *f,
f->inocache->pino_nlink = 1; /* Will be overwritten shortly for directories */
f->inocache->nodes = (struct jffs2_raw_node_ref *)f->inocache;
f->inocache->state = INO_STATE_PRESENT;
+ f->inocache->state_new = INO_STATE_I_NEW;
jffs2_add_ino_cache(c, f->inocache);
jffs2_dbg(1, "%s(): Assigned ino# %d\n", __func__, f->inocache->ino);
--
1.8.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-12-12 6:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-11 1:36 [PATCH] jffs2: bug fix of creating node when gc or find space gaoyongliang
2015-12-11 2:58 ` Brian Norris
2015-12-12 6:54 ` gaoyongliang
-- strict thread matches above, loose matches on Subject: below --
2014-12-01 14:21 chenjie6
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).