linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment
@ 2013-05-20  3:32 Jaegeuk Kim
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 02/15] f2fs: remove unnecessary flag set Jaegeuk Kim
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

We don't need to assign a value redundantly.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/recovery.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 60c8a50..2941987 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -126,7 +126,6 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
 
 		entry = get_fsync_inode(head, ino_of_node(page));
 		if (entry) {
-			entry->blkaddr = blkaddr;
 			if (IS_INODE(page) && is_dent_dnode(page))
 				set_inode_flag(F2FS_I(entry->inode),
 							FI_INC_LINK);
@@ -150,10 +149,10 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
 				kmem_cache_free(fsync_entry_slab, entry);
 				goto unlock_out;
 			}
-
 			list_add_tail(&entry->list, head);
-			entry->blkaddr = blkaddr;
 		}
+		entry->blkaddr = blkaddr;
+
 		if (IS_INODE(page)) {
 			err = recover_inode(entry->inode, page);
 			if (err == -ENOENT) {
-- 
1.8.1.3.566.gaa39828


------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [f2fs-dev] [PATCH 02/15] f2fs: remove unnecessary flag set
  2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
@ 2013-05-20  3:32 ` Jaegeuk Kim
  2013-05-20  7:31   ` Jaegeuk Kim
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 03/15] f2fs: fix por_doing variable coverage Jaegeuk Kim
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

If an inode is recovered with its dentry, it will not invoke __f2fs_add_link,
since the recovery routine checks its dentry before calling __f2fs_add_link.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/recovery.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 2941987..993b601 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -125,11 +125,7 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
 			goto next;
 
 		entry = get_fsync_inode(head, ino_of_node(page));
-		if (entry) {
-			if (IS_INODE(page) && is_dent_dnode(page))
-				set_inode_flag(F2FS_I(entry->inode),
-							FI_INC_LINK);
-		} else {
+		if (!entry) {
 			if (IS_INODE(page) && is_dent_dnode(page)) {
 				err = recover_inode_page(sbi, page);
 				if (err)
-- 
1.8.1.3.566.gaa39828


------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [f2fs-dev] [PATCH 03/15] f2fs: fix por_doing variable coverage
  2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 02/15] f2fs: remove unnecessary flag set Jaegeuk Kim
@ 2013-05-20  3:32 ` Jaegeuk Kim
  2013-05-20  3:32 ` [PATCH 04/15] f2fs: fix BUG_ON during f2fs_evict_inode(dir) Jaegeuk Kim
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

The reason of using sbi->por_doing is to alleviate data writes during the
recovery.
The find_fsync_dnodes() produces some dirty dentry pages, so we should
cover it too with sbi->por_doing.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/recovery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 993b601..f77aedd 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -377,6 +377,7 @@ int recover_fsync_data(struct f2fs_sb_info *sbi)
 	INIT_LIST_HEAD(&inode_list);
 
 	/* step #1: find fsynced inode numbers */
+	sbi->por_doing = 1;
 	err = find_fsync_dnodes(sbi, &inode_list);
 	if (err)
 		goto out;
@@ -385,13 +386,12 @@ int recover_fsync_data(struct f2fs_sb_info *sbi)
 		goto out;
 
 	/* step #2: recover data */
-	sbi->por_doing = 1;
 	err = recover_data(sbi, &inode_list, CURSEG_WARM_NODE);
-	sbi->por_doing = 0;
 	BUG_ON(!list_empty(&inode_list));
 out:
 	destroy_fsync_dnodes(sbi, &inode_list);
 	kmem_cache_destroy(fsync_entry_slab);
+	sbi->por_doing = 0;
 	write_checkpoint(sbi, false);
 	return err;
 }
-- 
1.8.1.3.566.gaa39828


------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 04/15] f2fs: fix BUG_ON during f2fs_evict_inode(dir)
  2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 02/15] f2fs: remove unnecessary flag set Jaegeuk Kim
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 03/15] f2fs: fix por_doing variable coverage Jaegeuk Kim
@ 2013-05-20  3:32 ` Jaegeuk Kim
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 05/15] f2fs: remove unnecessary por_doing check Jaegeuk Kim
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: Jaegeuk Kim, linux-fsdevel, linux-kernel, linux-f2fs-devel

During the dentry recovery routine, recover_inode() triggers __f2fs_add_link
with its directory inode.

In the following scenario, a bug is captured.
 1. dir = f2fs_iget(pino)
 2. __f2fs_add_link(dir, name)
 3. iput(dir)
  -> f2fs_evict_inode() faces with BUG_ON(atomic_read(fi->dirty_dents))

Kernel BUG at ffffffffa01c0676 [verbose debug info unavailable]
[<ffffffffa01c0676>] f2fs_evict_inode+0x276/0x300 [f2fs]
Call Trace:
 [<ffffffff8118ea00>] evict+0xb0/0x1b0
 [<ffffffff8118f1c5>] iput+0x105/0x190
 [<ffffffffa01d2dac>] recover_fsync_data+0x3bc/0x1070 [f2fs]
 [<ffffffff81692e8a>] ? io_schedule+0xaa/0xd0
 [<ffffffff81690acb>] ? __wait_on_bit_lock+0x7b/0xc0
 [<ffffffff8111a0e7>] ? __lock_page+0x67/0x70
 [<ffffffff81165e21>] ? kmem_cache_alloc+0x31/0x140
 [<ffffffff8118a502>] ? __d_instantiate+0x92/0xf0
 [<ffffffff812a949b>] ? security_d_instantiate+0x1b/0x30
 [<ffffffff8118a5b4>] ? d_instantiate+0x54/0x70

This means that we should flush all the dentry pages between iget and iput().
But, during the recovery routine, it is unallowed due to consistency, so we
have to wait the whole recovery process.
And then, write_checkpoint flushes all the dirty dentry blocks, and nicely we
can put the stale dir inodes from the dirty_dir_inode_list.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/checkpoint.c | 23 +++++++++++++++++++++++
 fs/f2fs/f2fs.h       |  2 ++
 fs/f2fs/recovery.c   | 14 +++++++++-----
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index b1de01d..3d11449 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -514,6 +514,29 @@ void remove_dirty_dir_inode(struct inode *inode)
 	}
 out:
 	spin_unlock(&sbi->dir_inode_lock);
+
+	/* Only from the recovery routine */
+	if (is_inode_flag_set(F2FS_I(inode), FI_DELAY_IPUT))
+		iput(inode);
+}
+
+struct inode *check_dirty_dir_inode(struct f2fs_sb_info *sbi, nid_t ino)
+{
+	struct list_head *head = &sbi->dir_inode_list;
+	struct list_head *this;
+	struct inode *inode = NULL;
+
+	spin_lock(&sbi->dir_inode_lock);
+	list_for_each(this, head) {
+		struct dir_inode_entry *entry;
+		entry = list_entry(this, struct dir_inode_entry, list);
+		if (entry->inode->i_ino == ino) {
+			inode = entry->inode;
+			break;
+		}
+	}
+	spin_unlock(&sbi->dir_inode_lock);
+	return inode;
 }
 
 void sync_dirty_dir_inodes(struct f2fs_sb_info *sbi)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 20aab02..ef6cac8 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -846,6 +846,7 @@ enum {
 	FI_INC_LINK,		/* need to increment i_nlink */
 	FI_ACL_MODE,		/* indicate acl mode */
 	FI_NO_ALLOC,		/* should not allocate any blocks */
+	FI_DELAY_IPUT,		/* used for the recovery */
 };
 
 static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
@@ -1012,6 +1013,7 @@ int recover_orphan_inodes(struct f2fs_sb_info *);
 int get_valid_checkpoint(struct f2fs_sb_info *);
 void set_dirty_dir_page(struct inode *, struct page *);
 void remove_dirty_dir_inode(struct inode *);
+struct inode *check_dirty_dir_inode(struct f2fs_sb_info *, nid_t);
 void sync_dirty_dir_inodes(struct f2fs_sb_info *);
 void write_checkpoint(struct f2fs_sb_info *, bool);
 void init_orphan_info(struct f2fs_sb_info *);
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index f77aedd..c573944 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -42,6 +42,7 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 {
 	struct f2fs_node *raw_node = (struct f2fs_node *)kmap(ipage);
 	struct f2fs_inode *raw_inode = &(raw_node->i);
+	nid_t pino = le32_to_cpu(raw_inode->i_pino);
 	struct qstr name;
 	struct f2fs_dir_entry *de;
 	struct page *page;
@@ -51,10 +52,14 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 	if (!is_dent_dnode(ipage))
 		goto out;
 
-	dir = f2fs_iget(inode->i_sb, le32_to_cpu(raw_inode->i_pino));
-	if (IS_ERR(dir)) {
-		err = PTR_ERR(dir);
-		goto out;
+	dir = check_dirty_dir_inode(F2FS_SB(inode->i_sb), pino);
+	if (!dir) {
+		dir = f2fs_iget(inode->i_sb, pino);
+		if (IS_ERR(dir)) {
+			err = PTR_ERR(dir);
+			goto out;
+		}
+		set_inode_flag(F2FS_I(dir), FI_DELAY_IPUT);
 	}
 
 	name.len = le32_to_cpu(raw_inode->i_namelen);
@@ -67,7 +72,6 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 	} else {
 		err = __f2fs_add_link(dir, &name, inode);
 	}
-	iput(dir);
 out:
 	kunmap(ipage);
 	return err;
-- 
1.8.1.3.566.gaa39828


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [f2fs-dev] [PATCH 05/15] f2fs: remove unnecessary por_doing check
  2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
                   ` (2 preceding siblings ...)
  2013-05-20  3:32 ` [PATCH 04/15] f2fs: fix BUG_ON during f2fs_evict_inode(dir) Jaegeuk Kim
@ 2013-05-20  3:32 ` Jaegeuk Kim
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 06/15] f2fs: skip get_node_page if locked node page is passed Jaegeuk Kim
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

This por_doing check is totally not related to the recovery process.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/namei.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 47abc97..729b285 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -149,8 +149,7 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 
 	alloc_nid_done(sbi, ino);
 
-	if (!sbi->por_doing)
-		d_instantiate(dentry, inode);
+	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
 	return 0;
 out:
-- 
1.8.1.3.566.gaa39828


------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [f2fs-dev] [PATCH 06/15] f2fs: skip get_node_page if locked node page is passed
  2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
                   ` (3 preceding siblings ...)
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 05/15] f2fs: remove unnecessary por_doing check Jaegeuk Kim
@ 2013-05-20  3:32 ` Jaegeuk Kim
  2013-05-20  3:32 ` [PATCH 07/15] f2fs: change get_new_data_page to pass a locked node page Jaegeuk Kim
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

If get_dnode_of_data gets a locked node page, let's skip redundant
get_node_page calls.
This is for the futher enhancement.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/node.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 9641534..f63f0a4 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -408,10 +408,13 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int mode)
 	level = get_node_path(index, offset, noffset);
 
 	nids[0] = dn->inode->i_ino;
-	npage[0] = get_node_page(sbi, nids[0]);
-	if (IS_ERR(npage[0]))
-		return PTR_ERR(npage[0]);
+	npage[0] = dn->inode_page;
 
+	if (!npage[0]) {
+		npage[0] = get_node_page(sbi, nids[0]);
+		if (IS_ERR(npage[0]))
+			return PTR_ERR(npage[0]);
+	}
 	parent = npage[0];
 	if (level != 0)
 		nids[1] = get_nid(parent, offset[0], true);
-- 
1.8.1.3.566.gaa39828


------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 07/15] f2fs: change get_new_data_page to pass a locked node page
  2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
                   ` (4 preceding siblings ...)
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 06/15] f2fs: skip get_node_page if locked node page is passed Jaegeuk Kim
@ 2013-05-20  3:32 ` Jaegeuk Kim
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 08/15] f2fs: update inode page after creation Jaegeuk Kim
  2013-05-20  3:32 ` [PATCH 09/15] f2fs: add debug msgs in the recovery routine Jaegeuk Kim
  7 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: Jaegeuk Kim, linux-fsdevel, linux-kernel, linux-f2fs-devel

This patch is for passing a locked node page to get_dnode_of_data.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/data.c | 12 +++++++-----
 fs/f2fs/dir.c  |  4 ++--
 fs/f2fs/f2fs.h |  2 +-
 fs/f2fs/file.c |  2 +-
 4 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 05fb5c6..af74549 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -280,8 +280,8 @@ repeat:
  * Also, caller should grab and release a mutex by calling mutex_lock_op() and
  * mutex_unlock_op().
  */
-struct page *get_new_data_page(struct inode *inode, pgoff_t index,
-						bool new_i_size)
+struct page *get_new_data_page(struct inode *inode,
+		struct page *npage, pgoff_t index, bool new_i_size)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 	struct address_space *mapping = inode->i_mapping;
@@ -289,18 +289,20 @@ struct page *get_new_data_page(struct inode *inode, pgoff_t index,
 	struct dnode_of_data dn;
 	int err;
 
-	set_new_dnode(&dn, inode, NULL, NULL, 0);
+	set_new_dnode(&dn, inode, npage, npage, 0);
 	err = get_dnode_of_data(&dn, index, ALLOC_NODE);
 	if (err)
 		return ERR_PTR(err);
 
 	if (dn.data_blkaddr == NULL_ADDR) {
 		if (reserve_new_block(&dn)) {
-			f2fs_put_dnode(&dn);
+			if (!npage)
+				f2fs_put_dnode(&dn);
 			return ERR_PTR(-ENOSPC);
 		}
 	}
-	f2fs_put_dnode(&dn);
+	if (!npage)
+		f2fs_put_dnode(&dn);
 repeat:
 	page = grab_cache_page(mapping, index);
 	if (!page)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 1ac6b93..7db6e58 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -287,7 +287,7 @@ static int make_empty_dir(struct inode *inode, struct inode *parent)
 	struct f2fs_dir_entry *de;
 	void *kaddr;
 
-	dentry_page = get_new_data_page(inode, 0, true);
+	dentry_page = get_new_data_page(inode, NULL, 0, true);
 	if (IS_ERR(dentry_page))
 		return PTR_ERR(dentry_page);
 
@@ -448,7 +448,7 @@ start:
 	bidx = dir_block_index(level, (le32_to_cpu(dentry_hash) % nbucket));
 
 	for (block = bidx; block <= (bidx + nblock - 1); block++) {
-		dentry_page = get_new_data_page(dir, block, true);
+		dentry_page = get_new_data_page(dir, NULL, block, true);
 		if (IS_ERR(dentry_page))
 			return PTR_ERR(dentry_page);
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index ef6cac8..cbae2b6 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1027,7 +1027,7 @@ int reserve_new_block(struct dnode_of_data *);
 void update_extent_cache(block_t, struct dnode_of_data *);
 struct page *find_data_page(struct inode *, pgoff_t, bool);
 struct page *get_lock_data_page(struct inode *, pgoff_t);
-struct page *get_new_data_page(struct inode *, pgoff_t, bool);
+struct page *get_new_data_page(struct inode *, struct page *, pgoff_t, bool);
 int f2fs_readpage(struct f2fs_sb_info *, struct page *, block_t, int);
 int do_write_data_page(struct page *);
 
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 1cae864..b8e34db 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -387,7 +387,7 @@ static void fill_zero(struct inode *inode, pgoff_t index,
 	f2fs_balance_fs(sbi);
 
 	ilock = mutex_lock_op(sbi);
-	page = get_new_data_page(inode, index, false);
+	page = get_new_data_page(inode, NULL, index, false);
 	mutex_unlock_op(sbi, ilock);
 
 	if (!IS_ERR(page)) {
-- 
1.8.1.3.566.gaa39828

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [f2fs-dev] [PATCH 08/15] f2fs: update inode page after creation
  2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
                   ` (5 preceding siblings ...)
  2013-05-20  3:32 ` [PATCH 07/15] f2fs: change get_new_data_page to pass a locked node page Jaegeuk Kim
@ 2013-05-20  3:32 ` Jaegeuk Kim
  2013-05-20  3:32 ` [PATCH 09/15] f2fs: add debug msgs in the recovery routine Jaegeuk Kim
  7 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

I found a bug when testing power-off-recovery as follows.

[Bug Scenario]
1. create a file
2. fsync the file
3. reboot w/o any sync
4. try to recover the file
 - found its fsync mark
 - found its dentry mark
   : try to recover its dentry
    - get its file name
    - get its parent inode number
     : here we got zero value

The reason why we get the wrong parent inode number is that we didn't
synchronize the inode page with its newly created inode information perfectly.

Especially, previous f2fs stores fi->i_pino and writes it to the cached
node page in a wrong order, which incurs the zero-valued i_pino during the
recovery.

So, this patch modifies the creation flow to fix the synchronization order of
inode page with its inode.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/data.c |  1 +
 fs/f2fs/dir.c  | 85 +++++++++++++++++++++++++++++++---------------------------
 fs/f2fs/f2fs.h |  3 +--
 fs/f2fs/node.c | 12 +++------
 4 files changed, 51 insertions(+), 50 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index af74549..c320f7f 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -279,6 +279,7 @@ repeat:
  *
  * Also, caller should grab and release a mutex by calling mutex_lock_op() and
  * mutex_unlock_op().
+ * Note that, npage is set only by make_empty_dir.
  */
 struct page *get_new_data_page(struct inode *inode,
 		struct page *npage, pgoff_t index, bool new_i_size)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 7db6e58..fc1dacf 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -264,15 +264,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
 	f2fs_put_page(page, 1);
 }
 
-void init_dent_inode(const struct qstr *name, struct page *ipage)
+static void init_dent_inode(const struct qstr *name, struct page *ipage)
 {
 	struct f2fs_node *rn;
 
-	if (IS_ERR(ipage))
-		return;
-
-	wait_on_page_writeback(ipage);
-
 	/* copy name info. to this inode page */
 	rn = (struct f2fs_node *)page_address(ipage);
 	rn->i.i_namelen = cpu_to_le32(name->len);
@@ -280,14 +275,15 @@ void init_dent_inode(const struct qstr *name, struct page *ipage)
 	set_page_dirty(ipage);
 }
 
-static int make_empty_dir(struct inode *inode, struct inode *parent)
+static int make_empty_dir(struct inode *inode,
+		struct inode *parent, struct page *page)
 {
 	struct page *dentry_page;
 	struct f2fs_dentry_block *dentry_blk;
 	struct f2fs_dir_entry *de;
 	void *kaddr;
 
-	dentry_page = get_new_data_page(inode, NULL, 0, true);
+	dentry_page = get_new_data_page(inode, page, 0, true);
 	if (IS_ERR(dentry_page))
 		return PTR_ERR(dentry_page);
 
@@ -317,42 +313,47 @@ static int make_empty_dir(struct inode *inode, struct inode *parent)
 	return 0;
 }
 
-static int init_inode_metadata(struct inode *inode,
+static struct page *init_inode_metadata(struct inode *inode,
 		struct inode *dir, const struct qstr *name)
 {
+	struct page *page;
+	int err;
+
 	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
-		int err;
-		err = new_inode_page(inode, name);
-		if (err)
-			return err;
+		page = new_inode_page(inode, name);
+		if (IS_ERR(page))
+			return page;
 
 		if (S_ISDIR(inode->i_mode)) {
-			err = make_empty_dir(inode, dir);
-			if (err) {
-				remove_inode_page(inode);
-				return err;
-			}
+			err = make_empty_dir(inode, dir, page);
+			if (err)
+				goto error;
 		}
 
 		err = f2fs_init_acl(inode, dir);
-		if (err) {
-			remove_inode_page(inode);
-			return err;
-		}
+		if (err)
+			goto error;
+
+		wait_on_page_writeback(page);
 	} else {
-		struct page *ipage;
-		ipage = get_node_page(F2FS_SB(dir->i_sb), inode->i_ino);
-		if (IS_ERR(ipage))
-			return PTR_ERR(ipage);
-		set_cold_node(inode, ipage);
-		init_dent_inode(name, ipage);
-		f2fs_put_page(ipage, 1);
+		page = get_node_page(F2FS_SB(dir->i_sb), inode->i_ino);
+		if (IS_ERR(page))
+			return page;
+
+		wait_on_page_writeback(page);
+		set_cold_node(inode, page);
 	}
-	if (is_inode_flag_set(F2FS_I(inode), FI_INC_LINK)) {
+
+	init_dent_inode(name, page);
+
+	if (is_inode_flag_set(F2FS_I(inode), FI_INC_LINK))
 		inc_nlink(inode);
-		update_inode_page(inode);
-	}
-	return 0;
+	return page;
+
+error:
+	f2fs_put_page(page, 1);
+	remove_inode_page(inode);
+	return ERR_PTR(err);
 }
 
 static void update_parent_metadata(struct inode *dir, struct inode *inode,
@@ -423,6 +424,7 @@ int __f2fs_add_link(struct inode *dir, const struct qstr *name, struct inode *in
 	struct page *dentry_page = NULL;
 	struct f2fs_dentry_block *dentry_blk = NULL;
 	int slots = GET_DENTRY_SLOTS(namelen);
+	struct page *page;
 	int err = 0;
 	int i;
 
@@ -465,12 +467,13 @@ start:
 	++level;
 	goto start;
 add_dentry:
-	err = init_inode_metadata(inode, dir, name);
-	if (err)
-		goto fail;
-
 	wait_on_page_writeback(dentry_page);
 
+	page = init_inode_metadata(inode, dir, name);
+	if (IS_ERR(page)) {
+		err = PTR_ERR(page);
+		goto fail;
+	}
 	de = &dentry_blk->dentry[bit_pos];
 	de->hash_code = dentry_hash;
 	de->name_len = cpu_to_le16(namelen);
@@ -481,10 +484,12 @@ add_dentry:
 		test_and_set_bit_le(bit_pos + i, &dentry_blk->dentry_bitmap);
 	set_page_dirty(dentry_page);
 
-	update_parent_metadata(dir, inode, current_depth);
-
-	/* update parent inode number before releasing dentry page */
+	/* we don't need to mark_inode_dirty now */
 	F2FS_I(inode)->i_pino = dir->i_ino;
+	update_inode(inode, page);
+	f2fs_put_page(page, 1);
+
+	update_parent_metadata(dir, inode, current_depth);
 fail:
 	kunmap(dentry_page);
 	f2fs_put_page(dentry_page, 1);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index cbae2b6..9360a03 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -914,7 +914,6 @@ struct f2fs_dir_entry *f2fs_parent_dir(struct inode *, struct page **);
 ino_t f2fs_inode_by_name(struct inode *, struct qstr *);
 void f2fs_set_link(struct inode *, struct f2fs_dir_entry *,
 				struct page *, struct inode *);
-void init_dent_inode(const struct qstr *, struct page *);
 int __f2fs_add_link(struct inode *, const struct qstr *, struct inode *);
 void f2fs_delete_entry(struct f2fs_dir_entry *, struct page *, struct inode *);
 int f2fs_make_empty(struct inode *, struct inode *);
@@ -949,7 +948,7 @@ void get_node_info(struct f2fs_sb_info *, nid_t, struct node_info *);
 int get_dnode_of_data(struct dnode_of_data *, pgoff_t, int);
 int truncate_inode_blocks(struct inode *, pgoff_t);
 int remove_inode_page(struct inode *);
-int new_inode_page(struct inode *, const struct qstr *);
+struct page *new_inode_page(struct inode *, const struct qstr *);
 struct page *new_node_page(struct dnode_of_data *, unsigned int);
 void ra_node_page(struct f2fs_sb_info *, nid_t);
 struct page *get_node_page(struct f2fs_sb_info *, pgoff_t);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index f63f0a4..b41482d 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -806,19 +806,15 @@ int remove_inode_page(struct inode *inode)
 	return 0;
 }
 
-int new_inode_page(struct inode *inode, const struct qstr *name)
+struct page *new_inode_page(struct inode *inode, const struct qstr *name)
 {
-	struct page *page;
 	struct dnode_of_data dn;
 
 	/* allocate inode page for new inode */
 	set_new_dnode(&dn, inode, NULL, NULL, inode->i_ino);
-	page = new_node_page(&dn, 0);
-	init_dent_inode(name, page);
-	if (IS_ERR(page))
-		return PTR_ERR(page);
-	f2fs_put_page(page, 1);
-	return 0;
+
+	/* caller should f2fs_put_page(page, 1); */
+	return new_node_page(&dn, 0);
 }
 
 struct page *new_node_page(struct dnode_of_data *dn, unsigned int ofs)
-- 
1.8.1.3.566.gaa39828


------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 09/15] f2fs: add debug msgs in the recovery routine
  2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
                   ` (6 preceding siblings ...)
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 08/15] f2fs: update inode page after creation Jaegeuk Kim
@ 2013-05-20  3:32 ` Jaegeuk Kim
  7 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  3:32 UTC (permalink / raw)
  Cc: Jaegeuk Kim, linux-fsdevel, linux-kernel, linux-f2fs-devel

This patch adds some trivial debugging messages in the recovery process.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/node.c     |  1 -
 fs/f2fs/recovery.c | 44 +++++++++++++++++++++++++-------------------
 2 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index b41482d..5a59780 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1495,7 +1495,6 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
 		WARN_ON(1);
 	set_node_addr(sbi, &new_ni, NEW_ADDR);
 	inc_valid_inode_count(sbi);
-
 	f2fs_put_page(ipage, 1);
 	return 0;
 }
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index c573944..774efdb 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -49,9 +49,6 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 	struct inode *dir;
 	int err = 0;
 
-	if (!is_dent_dnode(ipage))
-		goto out;
-
 	dir = check_dirty_dir_inode(F2FS_SB(inode->i_sb), pino);
 	if (!dir) {
 		dir = f2fs_iget(inode->i_sb, pino);
@@ -73,6 +70,9 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 		err = __f2fs_add_link(dir, &name, inode);
 	}
 out:
+	f2fs_msg(inode->i_sb, KERN_NOTICE, "recover_inode and its dentry: "
+			"ino = %x, name = %s, dir = %lx, err = %d",
+			ino_of_node(ipage), raw_inode->i_name, dir->i_ino, err);
 	kunmap(ipage);
 	return err;
 }
@@ -83,6 +83,9 @@ static int recover_inode(struct inode *inode, struct page *node_page)
 	struct f2fs_node *raw_node = (struct f2fs_node *)kaddr;
 	struct f2fs_inode *raw_inode = &(raw_node->i);
 
+	if (!IS_INODE(node_page))
+		return 0;
+
 	inode->i_mode = le16_to_cpu(raw_inode->i_mode);
 	i_size_write(inode, le64_to_cpu(raw_inode->i_size));
 	inode->i_atime.tv_sec = le64_to_cpu(raw_inode->i_mtime);
@@ -92,7 +95,12 @@ static int recover_inode(struct inode *inode, struct page *node_page)
 	inode->i_ctime.tv_nsec = le32_to_cpu(raw_inode->i_ctime_nsec);
 	inode->i_mtime.tv_nsec = le32_to_cpu(raw_inode->i_mtime_nsec);
 
-	return recover_dentry(node_page, inode);
+	if (is_dent_dnode(node_page))
+		return recover_dentry(node_page, inode);
+
+	f2fs_msg(inode->i_sb, KERN_NOTICE, "recover_inode: ino = %x, name = %s",
+			ino_of_node(node_page), raw_inode->i_name);
+	return 0;
 }
 
 static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
@@ -123,7 +131,7 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
 		lock_page(page);
 
 		if (cp_ver != cpver_of_node(page))
-			goto unlock_out;
+			break;
 
 		if (!is_fsync_dnode(page))
 			goto next;
@@ -133,40 +141,33 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
 			if (IS_INODE(page) && is_dent_dnode(page)) {
 				err = recover_inode_page(sbi, page);
 				if (err)
-					goto unlock_out;
+					break;
 			}
 
 			/* add this fsync inode to the list */
 			entry = kmem_cache_alloc(fsync_entry_slab, GFP_NOFS);
 			if (!entry) {
 				err = -ENOMEM;
-				goto unlock_out;
+				break;
 			}
 
 			entry->inode = f2fs_iget(sbi->sb, ino_of_node(page));
 			if (IS_ERR(entry->inode)) {
 				err = PTR_ERR(entry->inode);
 				kmem_cache_free(fsync_entry_slab, entry);
-				goto unlock_out;
+				break;
 			}
 			list_add_tail(&entry->list, head);
 		}
 		entry->blkaddr = blkaddr;
 
-		if (IS_INODE(page)) {
-			err = recover_inode(entry->inode, page);
-			if (err == -ENOENT) {
-				goto next;
-			} else if (err) {
-				err = -EINVAL;
-				goto unlock_out;
-			}
-		}
+		err = recover_inode(entry->inode, page);
+		if (err && err != -ENOENT)
+			break;
 next:
 		/* check next segment */
 		blkaddr = next_blkaddr_of_node(page);
 	}
-unlock_out:
 	unlock_page(page);
 out:
 	__free_pages(page, 0);
@@ -244,7 +245,7 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	struct dnode_of_data dn;
 	struct f2fs_summary sum;
 	struct node_info ni;
-	int err = 0;
+	int err = 0, recovered = 0;
 	int ilock;
 
 	start = start_bidx_of_node(ofs_of_node(page));
@@ -289,6 +290,7 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 			/* write dummy data page */
 			recover_data_page(sbi, NULL, &sum, src, dest);
 			update_extent_cache(dest, &dn);
+			recovered++;
 		}
 		dn.ofs_in_node++;
 	}
@@ -306,6 +308,10 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	recover_node_page(sbi, dn.node_page, &sum, &ni, blkaddr);
 	f2fs_put_dnode(&dn);
 	mutex_unlock_op(sbi, ilock);
+
+	f2fs_msg(sbi->sb, KERN_NOTICE, "recover_data: ino = %lx, "
+			"recovered_data = %d blocks",
+			inode->i_ino, recovered);
 	return 0;
 }
 
-- 
1.8.1.3.566.gaa39828

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 02/15] f2fs: remove unnecessary flag set
  2013-05-20  3:32 ` [f2fs-dev] [PATCH 02/15] f2fs: remove unnecessary flag set Jaegeuk Kim
@ 2013-05-20  7:31   ` Jaegeuk Kim
  0 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2013-05-20  7:31 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-kernel, linux-f2fs-devel

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

Sorry for the noise.
This patch should be ignored.
Thanks,

2013-05-20 (월), 12:32 +0900, Jaegeuk Kim:
> If an inode is recovered with its dentry, it will not invoke __f2fs_add_link,
> since the recovery routine checks its dentry before calling __f2fs_add_link.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> ---
>  fs/f2fs/recovery.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 2941987..993b601 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -125,11 +125,7 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
>  			goto next;
>  
>  		entry = get_fsync_inode(head, ino_of_node(page));
> -		if (entry) {
> -			if (IS_INODE(page) && is_dent_dnode(page))
> -				set_inode_flag(F2FS_I(entry->inode),
> -							FI_INC_LINK);
> -		} else {
> +		if (!entry) {
>  			if (IS_INODE(page) && is_dent_dnode(page)) {
>  				err = recover_inode_page(sbi, page);
>  				if (err)

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2013-05-20  7:31 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-20  3:32 [f2fs-dev] [PATCH 01/15] f2fs: remove redundant assignment Jaegeuk Kim
2013-05-20  3:32 ` [f2fs-dev] [PATCH 02/15] f2fs: remove unnecessary flag set Jaegeuk Kim
2013-05-20  7:31   ` Jaegeuk Kim
2013-05-20  3:32 ` [f2fs-dev] [PATCH 03/15] f2fs: fix por_doing variable coverage Jaegeuk Kim
2013-05-20  3:32 ` [PATCH 04/15] f2fs: fix BUG_ON during f2fs_evict_inode(dir) Jaegeuk Kim
2013-05-20  3:32 ` [f2fs-dev] [PATCH 05/15] f2fs: remove unnecessary por_doing check Jaegeuk Kim
2013-05-20  3:32 ` [f2fs-dev] [PATCH 06/15] f2fs: skip get_node_page if locked node page is passed Jaegeuk Kim
2013-05-20  3:32 ` [PATCH 07/15] f2fs: change get_new_data_page to pass a locked node page Jaegeuk Kim
2013-05-20  3:32 ` [f2fs-dev] [PATCH 08/15] f2fs: update inode page after creation Jaegeuk Kim
2013-05-20  3:32 ` [PATCH 09/15] f2fs: add debug msgs in the recovery routine Jaegeuk Kim

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