[PATCH 1/2] f2fs: introduce f2fs_do_tmpfile for code consistency

[PATCH 1/2] f2fs: introduce f2fs_do_tmpfile for code consistency

[Jaegeuk Kim]

This patch adds f2fs_do_tmpfile to eliminate the redundant init_inode_metadata
flow.
Throught this, we can provide the consistent lock usage, e.g., fi->i_sem,  and
this will enable better debugging stuffs.

Cc: Chao Yu <chao2.yu@samsung.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c   | 27 ++++++++++++++++++++++++---
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/namei.c | 30 ++++++++----------------------
 3 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 09760d5..fe6aaca 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -337,8 +337,7 @@ static struct page *init_inode_metadata(struct inode *inode,
 	struct page *page;
 	int err;
 
-	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE) &&
-			inode->i_nlink) {
+	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
 		page = new_inode_page(inode, name);
 		if (IS_ERR(page))
 			return page;
@@ -364,7 +363,8 @@ static struct page *init_inode_metadata(struct inode *inode,
 		set_cold_node(inode, page);
 	}
 
-	init_dent_inode(name, page);
+	if (name)
+		init_dent_inode(name, page);
 
 	/*
 	 * This file should be checkpointed during fsync.
@@ -537,6 +537,27 @@ fail:
 	return err;
 }
 
+int f2fs_do_tmpfile(struct inode *inode, struct inode *dir)
+{
+	struct page *page;
+	int err = 0;
+
+	down_write(&F2FS_I(inode)->i_sem);
+	page = init_inode_metadata(inode, dir, NULL);
+	if (IS_ERR(page)) {
+		err = PTR_ERR(page);
+		goto fail;
+	}
+	/* we don't need to mark_inode_dirty now */
+	update_inode(inode, page);
+	f2fs_put_page(page, 1);
+
+	update_parent_metadata(dir, inode, F2FS_I(dir)->i_current_depth);
+fail:
+	up_write(&F2FS_I(inode)->i_sem);
+	return err;
+}
+
 /*
  * It only removes the dentry from the dentry page,corresponding name
  * entry in name page does not need to be touched during deletion.
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 58df97e..ca421a8 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1136,6 +1136,7 @@ void f2fs_set_link(struct inode *, struct f2fs_dir_entry *,
 int update_dent_inode(struct inode *, const struct qstr *);
 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_do_tmpfile(struct inode *, struct inode *);
 int f2fs_make_empty(struct inode *, struct inode *);
 bool f2fs_empty_dir(struct inode *);
 
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 0d55517..9de4ab3 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -494,7 +494,6 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 	struct super_block *sb = dir->i_sb;
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	struct inode *inode;
-	struct page *page;
 	int err;
 
 	inode = f2fs_new_inode(dir, mode);
@@ -509,38 +508,25 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 	err = acquire_orphan_inode(sbi);
 	if (err)
 		goto out;
+
+	err = f2fs_do_tmpfile(inode, dir);
+	if (err)
+		goto release_out;
+
 	/*
 	 * add this non-linked tmpfile to orphan list, in this way we could
 	 * remove all unused data of tmpfile after abnormal power-off.
 	 */
 	add_orphan_inode(sbi, inode->i_ino);
-
-	page = new_inode_page(inode, NULL);
-	if (IS_ERR(page)) {
-		err = PTR_ERR(page);
-		goto remove_out;
-	}
-
-	err = f2fs_init_acl(inode, dir, page);
-	if (err)
-		goto unlock_out;
-
-	err = f2fs_init_security(inode, dir, NULL, page);
-	if (err)
-		goto unlock_out;
-
-	f2fs_put_page(page, 1);
 	f2fs_unlock_op(sbi);
 
 	alloc_nid_done(sbi, inode->i_ino);
-	mark_inode_dirty(inode);
 	d_tmpfile(dentry, inode);
 	unlock_new_inode(inode);
 	return 0;
-unlock_out:
-	f2fs_put_page(page, 1);
-remove_out:
-	remove_orphan_inode(sbi, inode->i_ino);
+
+release_out:
+	release_orphan_inode(sbi);
 out:
 	f2fs_unlock_op(sbi);
 	clear_nlink(inode);
-- 
1.8.5.2 (Apple Git-48)

[PATCH 2/2] f2fs: clean up an unused parameter and assignment

[Jaegeuk Kim]

This patch cleans up simple unnecessary codes.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c   |  2 +-
 fs/f2fs/f2fs.h  |  2 +-
 fs/f2fs/namei.c | 26 +++++++++-----------------
 fs/f2fs/node.c  |  2 +-
 4 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index fe6aaca..1983c99 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -338,7 +338,7 @@ static struct page *init_inode_metadata(struct inode *inode,
 	int err;
 
 	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
-		page = new_inode_page(inode, name);
+		page = new_inode_page(inode);
 		if (IS_ERR(page))
 			return page;
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index ca421a8..3f0291b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1174,7 +1174,7 @@ int truncate_inode_blocks(struct inode *, pgoff_t);
 int truncate_xattr_node(struct inode *, struct page *);
 int wait_on_node_pages_writeback(struct f2fs_sb_info *, nid_t);
 void remove_inode_page(struct inode *);
-struct page *new_inode_page(struct inode *, const struct qstr *);
+struct page *new_inode_page(struct inode *);
 struct page *new_node_page(struct dnode_of_data *, unsigned int, struct page *);
 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/namei.c b/fs/f2fs/namei.c
index 9de4ab3..d1f7c20 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -23,14 +23,13 @@
 
 static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
 {
-	struct super_block *sb = dir->i_sb;
-	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb);
 	nid_t ino;
 	struct inode *inode;
 	bool nid_free = false;
 	int err;
 
-	inode = new_inode(sb);
+	inode = new_inode(dir->i_sb);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
@@ -103,8 +102,7 @@ static inline void set_cold_files(struct f2fs_sb_info *sbi, struct inode *inode,
 static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 						bool excl)
 {
-	struct super_block *sb = dir->i_sb;
-	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb);
 	struct inode *inode;
 	nid_t ino = 0;
 	int err;
@@ -147,8 +145,7 @@ static int f2fs_link(struct dentry *old_dentry, struct inode *dir,
 		struct dentry *dentry)
 {
 	struct inode *inode = old_dentry->d_inode;
-	struct super_block *sb = dir->i_sb;
-	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb);
 	int err;
 
 	f2fs_balance_fs(sbi);
@@ -208,8 +205,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 
 static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
 {
-	struct super_block *sb = dir->i_sb;
-	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb);
 	struct inode *inode = dentry->d_inode;
 	struct f2fs_dir_entry *de;
 	struct page *page;
@@ -243,8 +239,7 @@ fail:
 static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
 					const char *symname)
 {
-	struct super_block *sb = dir->i_sb;
-	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb);
 	struct inode *inode;
 	size_t symlen = strlen(symname) + 1;
 	int err;
@@ -331,8 +326,7 @@ static int f2fs_rmdir(struct inode *dir, struct dentry *dentry)
 static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
 				umode_t mode, dev_t rdev)
 {
-	struct super_block *sb = dir->i_sb;
-	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb);
 	struct inode *inode;
 	int err = 0;
 
@@ -370,8 +364,7 @@ out:
 static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			struct inode *new_dir, struct dentry *new_dentry)
 {
-	struct super_block *sb = old_dir->i_sb;
-	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(old_dir->i_sb);
 	struct inode *old_inode = old_dentry->d_inode;
 	struct inode *new_inode = new_dentry->d_inode;
 	struct page *old_dir_page;
@@ -491,8 +484,7 @@ out:
 
 static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 {
-	struct super_block *sb = dir->i_sb;
-	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb);
 	struct inode *inode;
 	int err;
 
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 9dfb9a0..771a8ac 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -843,7 +843,7 @@ void remove_inode_page(struct inode *inode)
 	truncate_node(&dn);
 }
 
-struct page *new_inode_page(struct inode *inode, const struct qstr *name)
+struct page *new_inode_page(struct inode *inode)
 {
 	struct dnode_of_data dn;
 
-- 
1.8.5.2 (Apple Git-48)

RE: [PATCH 1/2] f2fs: introduce f2fs_do_tmpfile for code consistency

[Chao Yu]

Hi,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Monday, June 23, 2014 9:13 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim; Chao Yu
> Subject: [PATCH 1/2] f2fs: introduce f2fs_do_tmpfile for code consistency
> 
> This patch adds f2fs_do_tmpfile to eliminate the redundant init_inode_metadata
> flow.
> Throught this, we can provide the consistent lock usage, e.g., fi->i_sem,  and
> this will enable better debugging stuffs.
> 
> Cc: Chao Yu <chao2.yu@samsung.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/dir.c   | 27 ++++++++++++++++++++++++---
>  fs/f2fs/f2fs.h  |  1 +
>  fs/f2fs/namei.c | 30 ++++++++----------------------
>  3 files changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 09760d5..fe6aaca 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -337,8 +337,7 @@ static struct page *init_inode_metadata(struct inode *inode,
>  	struct page *page;
>  	int err;
> 
> -	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE) &&
> -			inode->i_nlink) {
> +	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
>  		page = new_inode_page(inode, name);
>  		if (IS_ERR(page))
>  			return page;
> @@ -364,7 +363,8 @@ static struct page *init_inode_metadata(struct inode *inode,
>  		set_cold_node(inode, page);
>  	}
> 
> -	init_dent_inode(name, page);
> +	if (name)
> +		init_dent_inode(name, page);
> 
>  	/*
>  	 * This file should be checkpointed during fsync.
> @@ -537,6 +537,27 @@ fail:
>  	return err;
>  }
> 
> +int f2fs_do_tmpfile(struct inode *inode, struct inode *dir)
> +{
> +	struct page *page;
> +	int err = 0;
> +
> +	down_write(&F2FS_I(inode)->i_sem);

Here I did not find nlink/pino/xattr_ver which we should protect by ->i_sem,
Am I missing something?

> +	page = init_inode_metadata(inode, dir, NULL);
> +	if (IS_ERR(page)) {
> +		err = PTR_ERR(page);
> +		goto fail;
> +	}
> +	/* we don't need to mark_inode_dirty now */
> +	update_inode(inode, page);
> +	f2fs_put_page(page, 1);
> +
> +	update_parent_metadata(dir, inode, F2FS_I(dir)->i_current_depth);

Things we did in update_parent_metadata() is: 1.clear FI_NEW_INODE of inode flag,
2. update mtime&ctime of dir and mark it dirty.
IMO, we do not need the update op because we do not access/modify current dir
as we did normally through f2fs_add_link path.
So how about using clear_inode_flag instead of update_parent_metadata and leave
current dir as clean?

Thanks,
Yu

> +fail:
> +	up_write(&F2FS_I(inode)->i_sem);
> +	return err;
> +}
> +
>  /*
>   * It only removes the dentry from the dentry page,corresponding name
>   * entry in name page does not need to be touched during deletion.
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 58df97e..ca421a8 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1136,6 +1136,7 @@ void f2fs_set_link(struct inode *, struct f2fs_dir_entry *,
>  int update_dent_inode(struct inode *, const struct qstr *);
>  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_do_tmpfile(struct inode *, struct inode *);
>  int f2fs_make_empty(struct inode *, struct inode *);
>  bool f2fs_empty_dir(struct inode *);
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 0d55517..9de4ab3 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -494,7 +494,6 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t
> mode)
>  	struct super_block *sb = dir->i_sb;
>  	struct f2fs_sb_info *sbi = F2FS_SB(sb);
>  	struct inode *inode;
> -	struct page *page;
>  	int err;
> 
>  	inode = f2fs_new_inode(dir, mode);
> @@ -509,38 +508,25 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t
> mode)
>  	err = acquire_orphan_inode(sbi);
>  	if (err)
>  		goto out;
> +
> +	err = f2fs_do_tmpfile(inode, dir);
> +	if (err)
> +		goto release_out;
> +
>  	/*
>  	 * add this non-linked tmpfile to orphan list, in this way we could
>  	 * remove all unused data of tmpfile after abnormal power-off.
>  	 */
>  	add_orphan_inode(sbi, inode->i_ino);
> -
> -	page = new_inode_page(inode, NULL);
> -	if (IS_ERR(page)) {
> -		err = PTR_ERR(page);
> -		goto remove_out;
> -	}
> -
> -	err = f2fs_init_acl(inode, dir, page);
> -	if (err)
> -		goto unlock_out;
> -
> -	err = f2fs_init_security(inode, dir, NULL, page);
> -	if (err)
> -		goto unlock_out;
> -
> -	f2fs_put_page(page, 1);
>  	f2fs_unlock_op(sbi);
> 
>  	alloc_nid_done(sbi, inode->i_ino);
> -	mark_inode_dirty(inode);
>  	d_tmpfile(dentry, inode);
>  	unlock_new_inode(inode);
>  	return 0;
> -unlock_out:
> -	f2fs_put_page(page, 1);
> -remove_out:
> -	remove_orphan_inode(sbi, inode->i_ino);
> +
> +release_out:
> +	release_orphan_inode(sbi);
>  out:
>  	f2fs_unlock_op(sbi);
>  	clear_nlink(inode);
> --
> 1.8.5.2 (Apple Git-48)

Re: [PATCH 1/2 v2] f2fs: introduce f2fs_do_tmpfile for code consistency

[Jaegeuk Kim]

Change log from v1:
 o avoid mark_inode_dirty(dir), suggested by Chao Yu

>From f28528b0d835d7f2168e9a729016f68350be0308 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Fri, 20 Jun 2014 21:37:02 -0700
Subject: [PATCH v2] f2fs: introduce f2fs_do_tmpfile for code consistency

This patch adds f2fs_do_tmpfile to eliminate the redundant init_inode_metadata
flow.
Throught this, we can provide the consistent lock usage, e.g., fi->i_sem,  and
this will enable better debugging stuffs.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c   | 27 ++++++++++++++++++++++++---
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/namei.c | 30 ++++++++----------------------
 3 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 09760d5..0ac9e2e92 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -337,8 +337,7 @@ static struct page *init_inode_metadata(struct inode *inode,
 	struct page *page;
 	int err;
 
-	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE) &&
-			inode->i_nlink) {
+	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
 		page = new_inode_page(inode, name);
 		if (IS_ERR(page))
 			return page;
@@ -364,7 +363,8 @@ static struct page *init_inode_metadata(struct inode *inode,
 		set_cold_node(inode, page);
 	}
 
-	init_dent_inode(name, page);
+	if (name)
+		init_dent_inode(name, page);
 
 	/*
 	 * This file should be checkpointed during fsync.
@@ -537,6 +537,27 @@ fail:
 	return err;
 }
 
+int f2fs_do_tmpfile(struct inode *inode, struct inode *dir)
+{
+	struct page *page;
+	int err = 0;
+
+	down_write(&F2FS_I(inode)->i_sem);
+	page = init_inode_metadata(inode, dir, NULL);
+	if (IS_ERR(page)) {
+		err = PTR_ERR(page);
+		goto fail;
+	}
+	/* we don't need to mark_inode_dirty now */
+	update_inode(inode, page);
+	f2fs_put_page(page, 1);
+
+	clear_inode_flag(F2FS_I(inode), FI_NEW_INODE);
+fail:
+	up_write(&F2FS_I(inode)->i_sem);
+	return err;
+}
+
 /*
  * It only removes the dentry from the dentry page,corresponding name
  * entry in name page does not need to be touched during deletion.
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 58df97e..ca421a8 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1136,6 +1136,7 @@ void f2fs_set_link(struct inode *, struct f2fs_dir_entry *,
 int update_dent_inode(struct inode *, const struct qstr *);
 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_do_tmpfile(struct inode *, struct inode *);
 int f2fs_make_empty(struct inode *, struct inode *);
 bool f2fs_empty_dir(struct inode *);
 
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 0d55517..9de4ab3 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -494,7 +494,6 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 	struct super_block *sb = dir->i_sb;
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	struct inode *inode;
-	struct page *page;
 	int err;
 
 	inode = f2fs_new_inode(dir, mode);
@@ -509,38 +508,25 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 	err = acquire_orphan_inode(sbi);
 	if (err)
 		goto out;
+
+	err = f2fs_do_tmpfile(inode, dir);
+	if (err)
+		goto release_out;
+
 	/*
 	 * add this non-linked tmpfile to orphan list, in this way we could
 	 * remove all unused data of tmpfile after abnormal power-off.
 	 */
 	add_orphan_inode(sbi, inode->i_ino);
-
-	page = new_inode_page(inode, NULL);
-	if (IS_ERR(page)) {
-		err = PTR_ERR(page);
-		goto remove_out;
-	}
-
-	err = f2fs_init_acl(inode, dir, page);
-	if (err)
-		goto unlock_out;
-
-	err = f2fs_init_security(inode, dir, NULL, page);
-	if (err)
-		goto unlock_out;
-
-	f2fs_put_page(page, 1);
 	f2fs_unlock_op(sbi);
 
 	alloc_nid_done(sbi, inode->i_ino);
-	mark_inode_dirty(inode);
 	d_tmpfile(dentry, inode);
 	unlock_new_inode(inode);
 	return 0;
-unlock_out:
-	f2fs_put_page(page, 1);
-remove_out:
-	remove_orphan_inode(sbi, inode->i_ino);
+
+release_out:
+	release_orphan_inode(sbi);
 out:
 	f2fs_unlock_op(sbi);
 	clear_nlink(inode);
-- 
1.8.5.2 (Apple Git-48)


-- 
Jaegeuk Kim