f2fs: reorganize __f2fs_add_link

Firstly, spliting init_inode_metadata into two parts as below since they are relatively independent.
1) init_inode_metadata: init datas belong to newly created inode;
2) update_inode_metadata: update inode info for linking inode to new dentry.

Secondly, move init_inode_metadata to the front of __f2fs_add_link, So the flow of __f2fs_add_link will be reorganized as:
1) init inode meta data for new creatation;
2) lookup dentry page and insert new directory entry in the page;
3) update meta data of inode and parent.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/dir.c    | 129 ++++++++++++++++++++++++++++++++-----------------------
 fs/f2fs/f2fs.h   |   4 +-
 fs/f2fs/inline.c |   2 +-
 3 files changed, 78 insertions(+), 57 deletions(-)

fs/f2fs/dir.c +21 -17

index a34ebd8..0ad9a1c 100644
@@ -381,43 +381,68 @@ static int make_empty_dir(struct inode *inode,
 	return 0;
 }
 
-struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
+int init_inode_metadata(struct inode *inode, struct inode *dir,
 			const struct qstr *name, struct page *dpage)
 {
 	struct page *page;
 	int err;
 
-	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
-		page = new_inode_page(inode);
-		if (IS_ERR(page))
-			return page;
+	if (!is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE))
+		return 0;
 
-		if (S_ISDIR(inode->i_mode)) {
-			err = make_empty_dir(inode, dir, page);
-			if (err)
-				goto error;
-		}
+	page = new_inode_page(inode);
+	if (IS_ERR(page))
+		return PTR_ERR(page);
 
-		err = f2fs_init_acl(inode, dir, page, dpage);
+	if (S_ISDIR(inode->i_mode)) {
+		err = make_empty_dir(inode, dir, page);
 		if (err)
-			goto put_error;
+			goto error;
+	}
 
-		err = f2fs_init_security(inode, dir, name, page);
+	err = f2fs_init_acl(inode, dir, page, dpage);
+	if (err)
+		goto put_error;
+
+	err = f2fs_init_security(inode, dir, name, page);
+	if (err)
+		goto put_error;
+
+	if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
+		err = f2fs_inherit_context(dir, inode, page);
 		if (err)
 			goto put_error;
+	}
 
-		if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
-			err = f2fs_inherit_context(dir, inode, page);
-			if (err)
-				goto put_error;

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, July 24, 2015 2:15 AM
To: Chao Yu
Cc: [email protected]; [email protected]
Subject: Re: [PATCH 1/2] f2fs: reorganize __f2fs_add_link

Hi Chao,

Quoted 84 lines · Chao Yu

Firstly, spliting init_inode_metadata into two parts as below since they are relatively independent.
1) init_inode_metadata: init datas belong to newly created inode;
2) update_inode_metadata: update inode info for linking inode to new dentry.

Secondly, move init_inode_metadata to the front of __f2fs_add_link, So the flow of __f2fs_add_link will be reorganized as:
1) init inode meta data for new creatation;
2) lookup dentry page and insert new directory entry in the page;
3) update meta data of inode and parent.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/dir.c    | 129 ++++++++++++++++++++++++++++++++-----------------------
 fs/f2fs/f2fs.h   |   4 +-
 fs/f2fs/inline.c |   2 +-
 3 files changed, 78 insertions(+), 57 deletions(-)

fs/f2fs/dir.c +23 -22

index a34ebd8..0ad9a1c 100644
@@ -381,43 +381,68 @@ static int make_empty_dir(struct inode *inode,
 	return 0;
 }
 
-struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
+int init_inode_metadata(struct inode *inode, struct inode *dir,
 			const struct qstr *name, struct page *dpage)
 {
 	struct page *page;
 	int err;
 
-	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
-		page = new_inode_page(inode);
-		if (IS_ERR(page))
-			return page;
+	if (!is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE))
+		return 0;
 
-		if (S_ISDIR(inode->i_mode)) {
-			err = make_empty_dir(inode, dir, page);
-			if (err)
-				goto error;
-		}
+	page = new_inode_page(inode);
+	if (IS_ERR(page))
+		return PTR_ERR(page);
 
-		err = f2fs_init_acl(inode, dir, page, dpage);
+	if (S_ISDIR(inode->i_mode)) {
+		err = make_empty_dir(inode, dir, page);
 		if (err)
-			goto put_error;
+			goto error;
+	}
 
-		err = f2fs_init_security(inode, dir, name, page);
+	err = f2fs_init_acl(inode, dir, page, dpage);
+	if (err)
+		goto put_error;
+
+	err = f2fs_init_security(inode, dir, name, page);
+	if (err)
+		goto put_error;
+
+	if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
+		err = f2fs_inherit_context(dir, inode, page);
 		if (err)
 			goto put_error;
+	}
 
-		if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
-			err = f2fs_inherit_context(dir, inode, page);
-			if (err)
-				goto put_error;
-		}
-	} else {
-		page = get_node_page(F2FS_I_SB(dir), inode->i_ino);
-		if (IS_ERR(page))
-			return page;
+	if (f2fs_encrypted_inode(dir))
+		file_set_enc_name(inode);

fs/f2fs/dir.c static int make_empty_dir(struct inode *inode, · line 421

Quoted 3 lines

fs/f2fs/dir.c +3 -0

+
+	update_inode(inode, page);
+	f2fs_put_page(page, 1);

We should not put the inode page here.
Since checkpoint can be called and flush all the node pages with valid NAT entres. Once power-off happens after then, we can see unreachable NAT entries.

Thanks,

Hi Jaegeuk,

Quoted 94 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, July 24, 2015 2:15 AM
To: Chao Yu
Cc: [email protected]; [email protected]
Subject: Re: [PATCH 1/2] f2fs: reorganize __f2fs_add_link

Hi Chao,

Quoted 84 lines · Chao Yu

Firstly, spliting init_inode_metadata into two parts as below since they are relatively independent.
1) init_inode_metadata: init datas belong to newly created inode;
2) update_inode_metadata: update inode info for linking inode to new dentry.

Secondly, move init_inode_metadata to the front of __f2fs_add_link, So the flow of __f2fs_add_link will be reorganized as:
1) init inode meta data for new creatation;
2) lookup dentry page and insert new directory entry in the page;
3) update meta data of inode and parent.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/dir.c    | 129 ++++++++++++++++++++++++++++++++-----------------------
 fs/f2fs/f2fs.h   |   4 +-
 fs/f2fs/inline.c |   2 +-
 3 files changed, 78 insertions(+), 57 deletions(-)

fs/f2fs/dir.c +23 -22

index a34ebd8..0ad9a1c 100644
@@ -381,43 +381,68 @@ static int make_empty_dir(struct inode *inode,
 	return 0;
 }
 
-struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
+int init_inode_metadata(struct inode *inode, struct inode *dir,
 			const struct qstr *name, struct page *dpage)
 {
 	struct page *page;
 	int err;
 
-	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
-		page = new_inode_page(inode);
-		if (IS_ERR(page))
-			return page;
+	if (!is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE))
+		return 0;
 
-		if (S_ISDIR(inode->i_mode)) {
-			err = make_empty_dir(inode, dir, page);
-			if (err)
-				goto error;
-		}
+	page = new_inode_page(inode);
+	if (IS_ERR(page))
+		return PTR_ERR(page);
 
-		err = f2fs_init_acl(inode, dir, page, dpage);
+	if (S_ISDIR(inode->i_mode)) {
+		err = make_empty_dir(inode, dir, page);
 		if (err)
-			goto put_error;
+			goto error;
+	}
 
-		err = f2fs_init_security(inode, dir, name, page);
+	err = f2fs_init_acl(inode, dir, page, dpage);
+	if (err)
+		goto put_error;
+
+	err = f2fs_init_security(inode, dir, name, page);
+	if (err)
+		goto put_error;
+
+	if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
+		err = f2fs_inherit_context(dir, inode, page);
 		if (err)
 			goto put_error;
+	}
 
-		if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
-			err = f2fs_inherit_context(dir, inode, page);
-			if (err)
-				goto put_error;
-		}
-	} else {
-		page = get_node_page(F2FS_I_SB(dir), inode->i_ino);
-		if (IS_ERR(page))
-			return page;
+	if (f2fs_encrypted_inode(dir))
+		file_set_enc_name(inode);

Quoted 8 lines

fs/f2fs/dir.c static int make_empty_dir(struct inode *inode, · line 421

Quoted 3 lines

fs/f2fs/dir.c +3 -0

+
+	update_inode(inode, page);
+	f2fs_put_page(page, 1);

We should not put the inode page here.
Since checkpoint can be called and flush all the node pages with valid NAT entres. Once power-off happens after then, we can see unreachable NAT entries.

IMO, all callers of init_inode_metadata such as __f2fs_add_link/f2fs_do_tmpfile, they are all protected well by cp_rwsem, so checkpoint & SPO will not easily bother us.

1) Lately in __f2fs_add_link, ino of ipage will be inserted into directory entry of parent's inode, this makes our nat entry of ipage reachable.
2) And also in __f2fs_tmpfile, ino will be added in orphan list, so all data resource of the inode will be evicted after CP->SPO->RFR procedure.

Is there anything I missed?

Thanks,

Quoted 2 lines

Thanks,

Hi Chao,

Quoted 109 lines · Chao Yu

Hi Jaegeuk,

Quoted 94 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, July 24, 2015 2:15 AM
To: Chao Yu
Cc: [email protected]; [email protected]
Subject: Re: [PATCH 1/2] f2fs: reorganize __f2fs_add_link

Hi Chao,

Quoted 84 lines · Chao Yu

Firstly, spliting init_inode_metadata into two parts as below since they are relatively independent.
1) init_inode_metadata: init datas belong to newly created inode;
2) update_inode_metadata: update inode info for linking inode to new dentry.

Secondly, move init_inode_metadata to the front of __f2fs_add_link, So the flow of __f2fs_add_link will be reorganized as:
1) init inode meta data for new creatation;
2) lookup dentry page and insert new directory entry in the page;
3) update meta data of inode and parent.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/dir.c    | 129 ++++++++++++++++++++++++++++++++-----------------------
 fs/f2fs/f2fs.h   |   4 +-
 fs/f2fs/inline.c |   2 +-
 3 files changed, 78 insertions(+), 57 deletions(-)

fs/f2fs/dir.c +23 -22

index a34ebd8..0ad9a1c 100644
@@ -381,43 +381,68 @@ static int make_empty_dir(struct inode *inode,
 	return 0;
 }
 
-struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
+int init_inode_metadata(struct inode *inode, struct inode *dir,
 			const struct qstr *name, struct page *dpage)
 {
 	struct page *page;
 	int err;
 
-	if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) {
-		page = new_inode_page(inode);
-		if (IS_ERR(page))
-			return page;
+	if (!is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE))
+		return 0;
 
-		if (S_ISDIR(inode->i_mode)) {
-			err = make_empty_dir(inode, dir, page);
-			if (err)
-				goto error;
-		}
+	page = new_inode_page(inode);
+	if (IS_ERR(page))
+		return PTR_ERR(page);
 
-		err = f2fs_init_acl(inode, dir, page, dpage);
+	if (S_ISDIR(inode->i_mode)) {
+		err = make_empty_dir(inode, dir, page);
 		if (err)
-			goto put_error;
+			goto error;
+	}
 
-		err = f2fs_init_security(inode, dir, name, page);
+	err = f2fs_init_acl(inode, dir, page, dpage);
+	if (err)
+		goto put_error;
+
+	err = f2fs_init_security(inode, dir, name, page);
+	if (err)
+		goto put_error;
+
+	if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
+		err = f2fs_inherit_context(dir, inode, page);
 		if (err)
 			goto put_error;
+	}
 
-		if (f2fs_encrypted_inode(dir) && f2fs_may_encrypt(inode)) {
-			err = f2fs_inherit_context(dir, inode, page);
-			if (err)
-				goto put_error;
-		}
-	} else {
-		page = get_node_page(F2FS_I_SB(dir), inode->i_ino);
-		if (IS_ERR(page))
-			return page;
+	if (f2fs_encrypted_inode(dir))
+		file_set_enc_name(inode);

Quoted 8 lines

fs/f2fs/dir.c static int make_empty_dir(struct inode *inode, · line 421

Quoted 3 lines

fs/f2fs/dir.c +3 -0

+
+	update_inode(inode, page);
+	f2fs_put_page(page, 1);

We should not put the inode page here.
Since checkpoint can be called and flush all the node pages with valid NAT entres. Once power-off happens after then, we can see unreachable NAT entries.

IMO, all callers of init_inode_metadata such as __f2fs_add_link/f2fs_do_tmpfile, they are all protected well by cp_rwsem, so checkpoint & SPO will not easily bother us.

Quoted 6 lines

1) Lately in __f2fs_add_link, ino of ipage will be inserted into directory entry of parent's inode, this makes our nat entry of ipage reachable.
2) And also in __f2fs_tmpfile, ino will be added in orphan list, so all data resource of the inode will be evicted after CP->SPO->RFR procedure.

Is there anything I missed?

Let me think about this for a while.
I think two patches are clean-ups with a little bit big changes. Currently, we've touched many parts including extent_cache, so I need to focus on stabilizing them first.
After then, I'd like to dig two clean-up patches. Ok?

Thanks,

Quoted 5 lines

Thanks,

Quoted 2 lines

Thanks,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Saturday, July 25, 2015 12:11 AM
To: Chao Yu
Cc: [email protected]; [email protected]
Subject: Re: [PATCH 1/2] f2fs: reorganize __f2fs_add_link

Hi Chao,

BTW, isn't there any problem on update_inode/mark_inode_dirty stuffs?

And, is there a hole to write uncompleted node pages unnecessarily?

Quoted 5 lines

Let me think about this for a while.
I think two patches are clean-ups with a little bit big changes. Currently, we've touched many parts including extent_cache, so I need to focus on stabilizing them first.
After then, I'd like to dig two clean-up patches. Ok?