Re: [f2fs-dev] [PATCH 1/2] f2fs: reorganize __f2fs_add_link
Hi Chao,
On Fri, Jul 24, 2015 at 06:31:16PM +0800, Chao Yu wrote:
Hi Jaegeuk,
-Original Message-
From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
Sent: Friday, July 24, 2015 2:15 AM
To: Chao Yu
Cc: linux-f2fs-devel@lists.sourceforge.net; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 1/2] f2fs: reorganize __f2fs_add_link
Hi Chao,
On Wed, Jul 22, 2015 at 06:21:52PM +0800, Chao Yu wrote:
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 chao2...@samsung.com
---
fs/f2fs/dir.c| 129
---
fs/f2fs/f2fs.h | 4 +-
fs/f2fs/inline.c | 2 +-
3 files changed, 78 insertions(+), 57 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index a34ebd8..0ad9a1c 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -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);
+
+ 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?
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,
Thanks,
Thanks,
--
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [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?
On Fri, Jul 24, 2015 at 08:52:00AM -0700, Jaegeuk Kim wrote:
Hi Chao,
On Fri, Jul 24, 2015 at 06:31:16PM +0800, Chao Yu wrote:
Hi Jaegeuk,
-Original Message-
From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
Sent: Friday, July 24, 2015 2:15 AM
To: Chao Yu
Cc: linux-f2fs-devel@lists.sourceforge.net; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 1/2] f2fs: reorganize __f2fs_add_link
Hi Chao,
On Wed, Jul 22, 2015 at 06:21:52PM +0800, Chao Yu wrote:
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 chao2...@samsung.com
---
fs/f2fs/dir.c| 129
---
fs/f2fs/f2fs.h | 4 +-
fs/f2fs/inline.c | 2 +-
3 files changed, 78 insertions(+), 57 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index a34ebd8..0ad9a1c 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -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);
+
+ 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?
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
[f2fs-dev] [PATCH 1/2] 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 chao2...@samsung.com
---
fs/f2fs/dir.c| 129 ---
fs/f2fs/f2fs.h | 4 +-
fs/f2fs/inline.c | 2 +-
3 files changed, 78 insertions(+), 57 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index a34ebd8..0ad9a1c 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -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);
+
+ update_inode(inode, page);
+ f2fs_put_page(page, 1);
+ return 0;
+
+put_error:
+ f2fs_put_page(page, 1);
+error:
+ /* once the failed inode becomes a bad inode, i_mode is S_IFREG */
+ truncate_inode_pages(inode-i_data, 0);
+ truncate_blocks(inode, 0, false);
+ remove_dirty_dir_inode(inode);
+ remove_inode_page(inode);
+ return err;
+}
+
+struct page *update_inode_metadata(struct inode *dir, struct inode *inode,
+ const struct qstr *name)
+{
+ struct page *page;
+
+ page = get_node_page(F2FS_I_SB(inode), inode-i_ino);
+ if (IS_ERR(page))
+ return page;
+ if (!is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE))
set_cold_node(inode, page);
- }
if (name)
init_dent_inode(name, page);
@@ -433,20 +458,10 @@ struct page *init_inode_metadata(struct inode *inode,
struct inode *dir,
* we should remove this inode from orphan list.
*/
if (inode-i_nlink == 0)
- remove_orphan_inode(F2FS_I_SB(dir), inode-i_ino);
+ remove_orphan_inode(F2FS_I_SB(inode), inode-i_ino);
inc_nlink(inode);
}
return page;
-
-put_error:
- f2fs_put_page(page, 1);
-error:
- /* once the failed inode becomes a bad inode, i_mode is S_IFREG */
- truncate_inode_pages(inode-i_data, 0);
- truncate_blocks(inode, 0, false);
- remove_dirty_dir_inode(inode);
- remove_inode_page(inode);
- return ERR_PTR(err);
}
void update_parent_metadata(struct inode *dir, struct inode *inode,
@@ -537,10 +552,18 @@ int __f2fs_add_link(struct inode *dir, const struct qstr
*name,
new_name.name = fname_name(fname);
new_name.len = fname_len(fname);
+ if (inode) {
+ down_write(F2FS_I(inode)-i_sem);
+ err =
