[PATCH 2/2] f2fs: avoid new_inode's flags overwrite the old_node

[PATCH 2/2] f2fs: avoid new_inode's flags overwrite the old_node

[Kinglong Mee]

Note, rename with old_inode (enc_name) and new_inode (no_enc_name),
the result will be old_indoe(enc_name) and new_inode(enc_name).

Needs swap the enc_name flags?
If needed, this patch needs update of clear the flags.
otherwise, this patch only fix the logic with any result changing.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/f2fs/namei.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 3231a0a..db2ab7f 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -846,6 +846,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct page *old_page, *new_page;
 	struct f2fs_dir_entry *old_dir_entry = NULL, *new_dir_entry = NULL;
 	struct f2fs_dir_entry *old_entry, *new_entry;
+	bool old_inode_enc = false, new_inode_enc = false;
 	int old_nlink = 0, new_nlink = 0;
 	int err = -ENOENT;
 
@@ -917,17 +918,16 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	f2fs_lock_op(sbi);
 
+	old_inode_enc = file_enc_name(old_inode);
+	new_inode_enc = file_enc_name(new_inode);
+
 	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
 	if (err)
 		goto out_unlock;
-	if (file_enc_name(new_inode))
-		file_set_enc_name(old_inode);
 
 	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
 	if (err)
 		goto out_undo;
-	if (file_enc_name(old_inode))
-		file_set_enc_name(new_inode);
 
 	/* update ".." directory entry info of old dentry */
 	if (old_dir_entry)
@@ -942,6 +942,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	down_write(&F2FS_I(old_inode)->i_sem);
 	file_lost_pino(old_inode);
+	if (new_inode_enc)
+		file_set_enc_name(old_inode);
 	up_write(&F2FS_I(old_inode)->i_sem);
 
 	old_dir->i_ctime = current_time(old_dir);
@@ -957,6 +959,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	down_write(&F2FS_I(new_inode)->i_sem);
 	file_lost_pino(new_inode);
+	if (old_inode_enc)
+		file_set_enc_name(new_inode);
 	up_write(&F2FS_I(new_inode)->i_sem);
 
 	new_dir->i_ctime = current_time(new_dir);
-- 
2.9.3


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 2/2] f2fs: avoid new_inode's flags overwrite the old_node

[Kinglong Mee]

On 3/4/2017 21:44, Kinglong Mee wrote:
> Note, rename with old_inode (enc_name) and new_inode (no_enc_name),
> the result will be old_indoe(enc_name) and new_inode(enc_name).
> 
> Needs swap the enc_name flags?
> If needed, this patch needs update of clear the flags.
> otherwise, this patch only fix the logic with any result changing.

In fscrypt_has_permitted_context(), 

187         /* no restrictions if the parent directory is not encrypted */
188         if (!parent->i_sb->s_cop->is_encrypted(parent))
189                 return 1;
190         /* if the child directory is not encrypted, this is always a problem */
191         if (!parent->i_sb->s_cop->is_encrypted(child))
192                 return 0;

Move an encrypted file to an non-encrypted directory is allowed, but,
move an non-encrypted file to an encrypted directory is not allowed.

So that, the cross rename between an encrypted file and an non-encrypted file
is not allowed, is it right?

If that's right, the flags setting should be removed here (cross rename), it's needless.
And, only update the flags for normal rename in f2fs_rename().

Also, after commit e7d5545285ed
("f2fs crypto: add filename encryption for roll-forward recovery") the cross rename of
encrypted file don't change they filename in raw inode.

Please ignore this patch, I will resolve the above problem and resend new patch.

thanks,
Kinglong Mee

> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/f2fs/namei.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 3231a0a..db2ab7f 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -846,6 +846,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	struct page *old_page, *new_page;
>  	struct f2fs_dir_entry *old_dir_entry = NULL, *new_dir_entry = NULL;
>  	struct f2fs_dir_entry *old_entry, *new_entry;
> +	bool old_inode_enc = false, new_inode_enc = false;
>  	int old_nlink = 0, new_nlink = 0;
>  	int err = -ENOENT;
>  
> @@ -917,17 +918,16 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	f2fs_lock_op(sbi);
>  
> +	old_inode_enc = file_enc_name(old_inode);
> +	new_inode_enc = file_enc_name(new_inode);
> +
>  	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
>  	if (err)
>  		goto out_unlock;
> -	if (file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
>  
>  	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
>  	if (err)
>  		goto out_undo;
> -	if (file_enc_name(old_inode))
> -		file_set_enc_name(new_inode);
>  
>  	/* update ".." directory entry info of old dentry */
>  	if (old_dir_entry)
> @@ -942,6 +942,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	down_write(&F2FS_I(old_inode)->i_sem);
>  	file_lost_pino(old_inode);
> +	if (new_inode_enc)
> +		file_set_enc_name(old_inode);
>  	up_write(&F2FS_I(old_inode)->i_sem);
>  
>  	old_dir->i_ctime = current_time(old_dir);
> @@ -957,6 +959,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	down_write(&F2FS_I(new_inode)->i_sem);
>  	file_lost_pino(new_inode);
> +	if (old_inode_enc)
> +		file_set_enc_name(new_inode);
>  	up_write(&F2FS_I(new_inode)->i_sem);
>  
>  	new_dir->i_ctime = current_time(new_dir);
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH] f2fs: sync the enc name flags with disk level filename

[Kinglong Mee]

A cross rename between two encrypted files, the disk level filenames 
aren't updated for the file_enc_name(to) checking.

Also, cross rename between encrypted file and non-encrypted file under
a non-encrypted file, the enc name flags should update at same time
as the disk level filename.

This patch,
1. make sure update the disk level filename in update_dent_inode,
2. a new function exchange_dent_inode for the disk-level filename update,
3. only set the enc name flags in init_inode_metadata.

Fixes: e7d5545285ed ("f2fs crypto: add filename encryption for roll-forward recovery")
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/f2fs/dir.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/inline.c |  2 --
 fs/f2fs/namei.c  | 19 ++----------
 4 files changed, 88 insertions(+), 25 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 295a223..b1bb0b1 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -337,17 +337,93 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
 int update_dent_inode(struct inode *inode, struct inode *to,
 					const struct qstr *name)
 {
-	struct page *page;
+	struct page *topage = NULL, *page;
+	struct f2fs_inode *tori;
+	struct qstr newname = *name;
+	int err = 0;
 
-	if (file_enc_name(to))
+	if (file_enc_name(to) && (inode == to))
 		return 0;
 
 	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
 	if (IS_ERR(page))
 		return PTR_ERR(page);
 
-	init_dent_inode(name, page);
+	file_clear_enc_name(inode);
+	if (file_enc_name(to)) {
+		topage = get_node_page(F2FS_I_SB(to), to->i_ino);
+		if (IS_ERR(topage)) {
+			f2fs_put_page(page, 1);
+			return PTR_ERR(topage);
+		}
+
+		tori = F2FS_INODE(topage);
+		newname.name = tori->i_name;
+		newname.len = tori->i_namelen;
+
+		file_set_enc_name(inode);
+	}
+
+	init_dent_inode(&newname, page);
+
 	f2fs_put_page(page, 1);
+	if (file_enc_name(to))
+		f2fs_put_page(topage, 1);
+
+	return err;
+}
+
+int exchange_dent_inode(struct inode *src, struct inode *dst,
+			const struct qstr *srcname, const struct qstr *dstname)
+{
+	struct page *srcpage = NULL, *dstpage = NULL;
+	char tmp_srcname[F2FS_NAME_LEN], tmp_dstname[F2FS_NAME_LEN];
+	struct qstr new_srcname = *srcname;
+	struct qstr new_dstname = *dstname;
+
+	if (src == dst)
+		return 0;
+
+	srcpage = get_node_page(F2FS_I_SB(src), src->i_ino);
+	if (IS_ERR(srcpage))
+		return PTR_ERR(srcpage);
+
+	dstpage = get_node_page(F2FS_I_SB(dst), dst->i_ino);
+	if (IS_ERR(dstpage)) {
+		f2fs_put_page(srcpage, 1);
+		return PTR_ERR(dstpage);
+	}
+
+	f2fs_wait_on_page_writeback(srcpage, NODE, true);
+	f2fs_wait_on_page_writeback(dstpage, NODE, true);
+
+	file_clear_enc_name(dst);
+	if (file_enc_name(src)) {
+		struct f2fs_inode *srcri = F2FS_INODE(srcpage);
+
+		memcpy(tmp_srcname, srcri->i_name, srcri->i_namelen);
+		new_srcname.name = tmp_srcname;
+		new_srcname.len = srcri->i_namelen;
+
+		file_set_enc_name(dst);
+	}
+
+	file_clear_enc_name(src);
+	if (file_enc_name(dst)) {
+		struct f2fs_inode *dstri = F2FS_INODE(dstpage);
+
+		memcpy(tmp_dstname, dstri->i_name, dstri->i_namelen);
+		new_dstname.name = tmp_dstname;
+		new_dstname.len = dstri->i_namelen;
+
+		file_set_enc_name(src);
+	}
+
+	init_dent_inode(&new_dstname, srcpage);
+	init_dent_inode(&new_srcname, dstpage);
+
+	f2fs_put_page(srcpage, 1);
+	f2fs_put_page(dstpage, 1);
 
 	return 0;
 }
@@ -435,9 +511,14 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
 		set_cold_node(inode, page);
 	}
 
-	if (new_name)
+	if (new_name) {
 		init_dent_inode(new_name, page);
 
+		file_clear_enc_name(inode);
+		if (f2fs_encrypted_inode(dir))
+			file_set_enc_name(inode);
+	}
+
 	/*
 	 * This file should be checkpointed during fsync.
 	 * We lost i_pino from now on.
@@ -596,8 +677,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
 			err = PTR_ERR(page);
 			goto fail;
 		}
-		if (f2fs_encrypted_inode(dir))
-			file_set_enc_name(inode);
 	}
 
 	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7e29249..52ea953 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -440,6 +440,7 @@ struct f2fs_map_blocks {
 #define file_clear_encrypt(inode) clear_file(inode, FADVISE_ENCRYPT_BIT)
 #define file_enc_name(inode)	is_file(inode, FADVISE_ENC_NAME_BIT)
 #define file_set_enc_name(inode) set_file(inode, FADVISE_ENC_NAME_BIT)
+#define file_clear_enc_name(inode) clear_file(inode, FADVISE_ENC_NAME_BIT)
 #define file_keep_isize(inode)	is_file(inode, FADVISE_KEEP_SIZE_BIT)
 #define file_set_keep_isize(inode) set_file(inode, FADVISE_KEEP_SIZE_BIT)
 
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index e32a9e5..41fe27d 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
 			err = PTR_ERR(page);
 			goto fail;
 		}
-		if (f2fs_encrypted_inode(dir))
-			file_set_enc_name(inode);
 	}
 
 	f2fs_wait_on_page_writeback(ipage, NODE, true);
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 3231a0a..57050ff 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -779,8 +779,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	down_write(&F2FS_I(old_inode)->i_sem);
 	file_lost_pino(old_inode);
-	if (new_inode && file_enc_name(new_inode))
-		file_set_enc_name(old_inode);
 	up_write(&F2FS_I(old_inode)->i_sem);
 
 	old_inode->i_ctime = current_time(old_inode);
@@ -917,17 +915,10 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	f2fs_lock_op(sbi);
 
-	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
+	err = exchange_dent_inode(old_inode, new_inode,
+				  &old_dentry->d_name, &new_dentry->d_name);
 	if (err)
 		goto out_unlock;
-	if (file_enc_name(new_inode))
-		file_set_enc_name(old_inode);
-
-	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
-	if (err)
-		goto out_undo;
-	if (file_enc_name(old_inode))
-		file_set_enc_name(new_inode);
 
 	/* update ".." directory entry info of old dentry */
 	if (old_dir_entry)
@@ -972,12 +963,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
 		f2fs_sync_fs(sbi->sb, 1);
 	return 0;
-out_undo:
-	/*
-	 * Still we may fail to recover name info of f2fs_inode here
-	 * Drop it, once its name is set as encrypted
-	 */
-	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
 out_unlock:
 	f2fs_unlock_op(sbi);
 out_new_dir:
-- 
2.9.3


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[PATCH v2] f2fs: sync the enc name flags with disk level filename

[Kinglong Mee]

A cross rename between two encrypted files, the disk level filenames
aren't updated for the file_enc_name(to) checking.

Also, cross rename between encrypted file and non-encrypted file under
a non-encrypted file, the enc name flags should update at same time
as the disk level filename.

This patch,
1. make sure update the disk level filename in update_dent_inode,
2. a new function exchange_dent_inode for the disk-level filename update,
3. only set the enc name flags in init_inode_metadata.

v2, add the missing function define of exchange_dent_inode in f2fs.h

Fixes: e7d5545285ed ("f2fs crypto: add filename encryption for roll-forward recovery")
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/f2fs/dir.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 fs/f2fs/f2fs.h   |  3 ++
 fs/f2fs/inline.c |  2 --
 fs/f2fs/namei.c  | 19 ++----------
 4 files changed, 90 insertions(+), 25 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 295a223..b1bb0b1 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -337,17 +337,93 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
 int update_dent_inode(struct inode *inode, struct inode *to,
 					const struct qstr *name)
 {
-	struct page *page;
+	struct page *topage = NULL, *page;
+	struct f2fs_inode *tori;
+	struct qstr newname = *name;
+	int err = 0;
 
-	if (file_enc_name(to))
+	if (file_enc_name(to) && (inode == to))
 		return 0;
 
 	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
 	if (IS_ERR(page))
 		return PTR_ERR(page);
 
-	init_dent_inode(name, page);
+	file_clear_enc_name(inode);
+	if (file_enc_name(to)) {
+		topage = get_node_page(F2FS_I_SB(to), to->i_ino);
+		if (IS_ERR(topage)) {
+			f2fs_put_page(page, 1);
+			return PTR_ERR(topage);
+		}
+
+		tori = F2FS_INODE(topage);
+		newname.name = tori->i_name;
+		newname.len = tori->i_namelen;
+
+		file_set_enc_name(inode);
+	}
+
+	init_dent_inode(&newname, page);
+
 	f2fs_put_page(page, 1);
+	if (file_enc_name(to))
+		f2fs_put_page(topage, 1);
+
+	return err;
+}
+
+int exchange_dent_inode(struct inode *src, struct inode *dst,
+			const struct qstr *srcname, const struct qstr *dstname)
+{
+	struct page *srcpage = NULL, *dstpage = NULL;
+	char tmp_srcname[F2FS_NAME_LEN], tmp_dstname[F2FS_NAME_LEN];
+	struct qstr new_srcname = *srcname;
+	struct qstr new_dstname = *dstname;
+
+	if (src == dst)
+		return 0;
+
+	srcpage = get_node_page(F2FS_I_SB(src), src->i_ino);
+	if (IS_ERR(srcpage))
+		return PTR_ERR(srcpage);
+
+	dstpage = get_node_page(F2FS_I_SB(dst), dst->i_ino);
+	if (IS_ERR(dstpage)) {
+		f2fs_put_page(srcpage, 1);
+		return PTR_ERR(dstpage);
+	}
+
+	f2fs_wait_on_page_writeback(srcpage, NODE, true);
+	f2fs_wait_on_page_writeback(dstpage, NODE, true);
+
+	file_clear_enc_name(dst);
+	if (file_enc_name(src)) {
+		struct f2fs_inode *srcri = F2FS_INODE(srcpage);
+
+		memcpy(tmp_srcname, srcri->i_name, srcri->i_namelen);
+		new_srcname.name = tmp_srcname;
+		new_srcname.len = srcri->i_namelen;
+
+		file_set_enc_name(dst);
+	}
+
+	file_clear_enc_name(src);
+	if (file_enc_name(dst)) {
+		struct f2fs_inode *dstri = F2FS_INODE(dstpage);
+
+		memcpy(tmp_dstname, dstri->i_name, dstri->i_namelen);
+		new_dstname.name = tmp_dstname;
+		new_dstname.len = dstri->i_namelen;
+
+		file_set_enc_name(src);
+	}
+
+	init_dent_inode(&new_dstname, srcpage);
+	init_dent_inode(&new_srcname, dstpage);
+
+	f2fs_put_page(srcpage, 1);
+	f2fs_put_page(dstpage, 1);
 
 	return 0;
 }
@@ -435,9 +511,14 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
 		set_cold_node(inode, page);
 	}
 
-	if (new_name)
+	if (new_name) {
 		init_dent_inode(new_name, page);
 
+		file_clear_enc_name(inode);
+		if (f2fs_encrypted_inode(dir))
+			file_set_enc_name(inode);
+	}
+
 	/*
 	 * This file should be checkpointed during fsync.
 	 * We lost i_pino from now on.
@@ -596,8 +677,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
 			err = PTR_ERR(page);
 			goto fail;
 		}
-		if (f2fs_encrypted_inode(dir))
-			file_set_enc_name(inode);
 	}
 
 	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7e29249..ce24fe9 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -440,6 +440,7 @@ struct f2fs_map_blocks {
 #define file_clear_encrypt(inode) clear_file(inode, FADVISE_ENCRYPT_BIT)
 #define file_enc_name(inode)	is_file(inode, FADVISE_ENC_NAME_BIT)
 #define file_set_enc_name(inode) set_file(inode, FADVISE_ENC_NAME_BIT)
+#define file_clear_enc_name(inode) clear_file(inode, FADVISE_ENC_NAME_BIT)
 #define file_keep_isize(inode)	is_file(inode, FADVISE_KEEP_SIZE_BIT)
 #define file_set_keep_isize(inode) set_file(inode, FADVISE_KEEP_SIZE_BIT)
 
@@ -2101,6 +2102,8 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
 			struct page *page, struct inode *inode);
 int update_dent_inode(struct inode *inode, struct inode *to,
 			const struct qstr *name);
+int exchange_dent_inode(struct inode *src, struct inode *dst,
+			const struct qstr *srcname, const struct qstr *dstname);
 void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
 			const struct qstr *name, f2fs_hash_t name_hash,
 			unsigned int bit_pos);
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index e32a9e5..41fe27d 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
 			err = PTR_ERR(page);
 			goto fail;
 		}
-		if (f2fs_encrypted_inode(dir))
-			file_set_enc_name(inode);
 	}
 
 	f2fs_wait_on_page_writeback(ipage, NODE, true);
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 3231a0a..57050ff 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -779,8 +779,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	down_write(&F2FS_I(old_inode)->i_sem);
 	file_lost_pino(old_inode);
-	if (new_inode && file_enc_name(new_inode))
-		file_set_enc_name(old_inode);
 	up_write(&F2FS_I(old_inode)->i_sem);
 
 	old_inode->i_ctime = current_time(old_inode);
@@ -917,17 +915,10 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	f2fs_lock_op(sbi);
 
-	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
+	err = exchange_dent_inode(old_inode, new_inode,
+				  &old_dentry->d_name, &new_dentry->d_name);
 	if (err)
 		goto out_unlock;
-	if (file_enc_name(new_inode))
-		file_set_enc_name(old_inode);
-
-	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
-	if (err)
-		goto out_undo;
-	if (file_enc_name(old_inode))
-		file_set_enc_name(new_inode);
 
 	/* update ".." directory entry info of old dentry */
 	if (old_dir_entry)
@@ -972,12 +963,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
 		f2fs_sync_fs(sbi->sb, 1);
 	return 0;
-out_undo:
-	/*
-	 * Still we may fail to recover name info of f2fs_inode here
-	 * Drop it, once its name is set as encrypted
-	 */
-	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
 out_unlock:
 	f2fs_unlock_op(sbi);
 out_new_dir:
-- 
2.9.3


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 2/2] f2fs: avoid new_inode's flags overwrite the old_node

[Chao Yu]

On 2017/3/4 21:44, Kinglong Mee wrote:
> Note, rename with old_inode (enc_name) and new_inode (no_enc_name),
> the result will be old_indoe(enc_name) and new_inode(enc_name).

IIRC, we don't allow rename files in different dirs that:
1. One dir is encrypted, another is not encrypted;
2. or src/dst dir has different encryption policy.

So that will not happen, right?

Thanks,

> 
> Needs swap the enc_name flags?
> If needed, this patch needs update of clear the flags.
> otherwise, this patch only fix the logic with any result changing.
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/f2fs/namei.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 3231a0a..db2ab7f 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -846,6 +846,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	struct page *old_page, *new_page;
>  	struct f2fs_dir_entry *old_dir_entry = NULL, *new_dir_entry = NULL;
>  	struct f2fs_dir_entry *old_entry, *new_entry;
> +	bool old_inode_enc = false, new_inode_enc = false;
>  	int old_nlink = 0, new_nlink = 0;
>  	int err = -ENOENT;
>  
> @@ -917,17 +918,16 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	f2fs_lock_op(sbi);
>  
> +	old_inode_enc = file_enc_name(old_inode);
> +	new_inode_enc = file_enc_name(new_inode);
> +
>  	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
>  	if (err)
>  		goto out_unlock;
> -	if (file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
>  
>  	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
>  	if (err)
>  		goto out_undo;
> -	if (file_enc_name(old_inode))
> -		file_set_enc_name(new_inode);
>  
>  	/* update ".." directory entry info of old dentry */
>  	if (old_dir_entry)
> @@ -942,6 +942,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	down_write(&F2FS_I(old_inode)->i_sem);
>  	file_lost_pino(old_inode);
> +	if (new_inode_enc)
> +		file_set_enc_name(old_inode);
>  	up_write(&F2FS_I(old_inode)->i_sem);
>  
>  	old_dir->i_ctime = current_time(old_dir);
> @@ -957,6 +959,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	down_write(&F2FS_I(new_inode)->i_sem);
>  	file_lost_pino(new_inode);
> +	if (old_inode_enc)
> +		file_set_enc_name(new_inode);
>  	up_write(&F2FS_I(new_inode)->i_sem);
>  
>  	new_dir->i_ctime = current_time(new_dir);
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 2/2] f2fs: avoid new_inode's flags overwrite the old_node

[Kinglong Mee]

On 3/6/2017 19:06, Chao Yu wrote:
> On 2017/3/4 21:44, Kinglong Mee wrote:
>> Note, rename with old_inode (enc_name) and new_inode (no_enc_name),
>> the result will be old_indoe(enc_name) and new_inode(enc_name).
> 
> IIRC, we don't allow rename files in different dirs that:
> 1. One dir is encrypted, another is not encrypted;
> 2. or src/dst dir has different encryption policy.
> 
> So that will not happen, right?

Yes, you are right.
This patch is bad one, please ignore this and check my latest fix, 
[PATCH v2] f2fs: sync the enc name flags with disk level filename

thanks,
Kinglong Mee
> 
> Thanks,
> 
>>
>> Needs swap the enc_name flags?
>> If needed, this patch needs update of clear the flags.
>> otherwise, this patch only fix the logic with any result changing.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/f2fs/namei.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index 3231a0a..db2ab7f 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -846,6 +846,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  	struct page *old_page, *new_page;
>>  	struct f2fs_dir_entry *old_dir_entry = NULL, *new_dir_entry = NULL;
>>  	struct f2fs_dir_entry *old_entry, *new_entry;
>> +	bool old_inode_enc = false, new_inode_enc = false;
>>  	int old_nlink = 0, new_nlink = 0;
>>  	int err = -ENOENT;
>>  
>> @@ -917,17 +918,16 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	f2fs_lock_op(sbi);
>>  
>> +	old_inode_enc = file_enc_name(old_inode);
>> +	new_inode_enc = file_enc_name(new_inode);
>> +
>>  	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
>>  	if (err)
>>  		goto out_unlock;
>> -	if (file_enc_name(new_inode))
>> -		file_set_enc_name(old_inode);
>>  
>>  	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
>>  	if (err)
>>  		goto out_undo;
>> -	if (file_enc_name(old_inode))
>> -		file_set_enc_name(new_inode);
>>  
>>  	/* update ".." directory entry info of old dentry */
>>  	if (old_dir_entry)
>> @@ -942,6 +942,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	down_write(&F2FS_I(old_inode)->i_sem);
>>  	file_lost_pino(old_inode);
>> +	if (new_inode_enc)
>> +		file_set_enc_name(old_inode);
>>  	up_write(&F2FS_I(old_inode)->i_sem);
>>  
>>  	old_dir->i_ctime = current_time(old_dir);
>> @@ -957,6 +959,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	down_write(&F2FS_I(new_inode)->i_sem);
>>  	file_lost_pino(new_inode);
>> +	if (old_inode_enc)
>> +		file_set_enc_name(new_inode);
>>  	up_write(&F2FS_I(new_inode)->i_sem);
>>  
>>  	new_dir->i_ctime = current_time(new_dir);
>>
> 
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH v2] f2fs: sync the enc name flags with disk level filename

[Jaegeuk Kim]

Hi Kinglong,

Can we make a testcase in xfstests to check this clearly?
1. creat A under encrypted dir
2. rename A to B
3. fsync B
4. power-cut

Is this your concern?

Hmm, on-disk filename is used when doing roll-forward recovery, and it seems
there is a hole in try_to_fix_pino() to recover its pino without filename.

Like this?

>From 7efde035d9f930fc63c30b25327e870b021750f3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Tue, 7 Mar 2017 11:22:45 -0800
Subject: [PATCH] f2fS: don't allow to get pino when filename is encrypted

After renaming an encrypted file, we have no way to get its
encrypted filename from its dentry.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e0b2378f8519..852195b3bcce 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -110,6 +110,9 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
 {
 	struct dentry *dentry;
 
+	if (file_enc_name(inode))
+		return 0;
+
 	inode = igrab(inode);
 	dentry = d_find_any_alias(inode);
 	iput(inode);
-- 
2.11.0


On 03/06, Kinglong Mee wrote:
> A cross rename between two encrypted files, the disk level filenames
> aren't updated for the file_enc_name(to) checking.
> 
> Also, cross rename between encrypted file and non-encrypted file under
> a non-encrypted file, the enc name flags should update at same time
> as the disk level filename.
> 
> This patch,
> 1. make sure update the disk level filename in update_dent_inode,
> 2. a new function exchange_dent_inode for the disk-level filename update,
> 3. only set the enc name flags in init_inode_metadata.
> 
> v2, add the missing function define of exchange_dent_inode in f2fs.h
> 
> Fixes: e7d5545285ed ("f2fs crypto: add filename encryption for roll-forward recovery")
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/f2fs/dir.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  fs/f2fs/f2fs.h   |  3 ++
>  fs/f2fs/inline.c |  2 --
>  fs/f2fs/namei.c  | 19 ++----------
>  4 files changed, 90 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 295a223..b1bb0b1 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -337,17 +337,93 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>  int update_dent_inode(struct inode *inode, struct inode *to,
>  					const struct qstr *name)
>  {
> -	struct page *page;
> +	struct page *topage = NULL, *page;
> +	struct f2fs_inode *tori;
> +	struct qstr newname = *name;
> +	int err = 0;
>  
> -	if (file_enc_name(to))
> +	if (file_enc_name(to) && (inode == to))
>  		return 0;
>  
>  	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
>  	if (IS_ERR(page))
>  		return PTR_ERR(page);
>  
> -	init_dent_inode(name, page);
> +	file_clear_enc_name(inode);
> +	if (file_enc_name(to)) {
> +		topage = get_node_page(F2FS_I_SB(to), to->i_ino);
> +		if (IS_ERR(topage)) {
> +			f2fs_put_page(page, 1);
> +			return PTR_ERR(topage);
> +		}
> +
> +		tori = F2FS_INODE(topage);
> +		newname.name = tori->i_name;
> +		newname.len = tori->i_namelen;
> +
> +		file_set_enc_name(inode);
> +	}
> +
> +	init_dent_inode(&newname, page);
> +
>  	f2fs_put_page(page, 1);
> +	if (file_enc_name(to))
> +		f2fs_put_page(topage, 1);
> +
> +	return err;
> +}
> +
> +int exchange_dent_inode(struct inode *src, struct inode *dst,
> +			const struct qstr *srcname, const struct qstr *dstname)
> +{
> +	struct page *srcpage = NULL, *dstpage = NULL;
> +	char tmp_srcname[F2FS_NAME_LEN], tmp_dstname[F2FS_NAME_LEN];
> +	struct qstr new_srcname = *srcname;
> +	struct qstr new_dstname = *dstname;
> +
> +	if (src == dst)
> +		return 0;
> +
> +	srcpage = get_node_page(F2FS_I_SB(src), src->i_ino);
> +	if (IS_ERR(srcpage))
> +		return PTR_ERR(srcpage);
> +
> +	dstpage = get_node_page(F2FS_I_SB(dst), dst->i_ino);
> +	if (IS_ERR(dstpage)) {
> +		f2fs_put_page(srcpage, 1);
> +		return PTR_ERR(dstpage);
> +	}
> +
> +	f2fs_wait_on_page_writeback(srcpage, NODE, true);
> +	f2fs_wait_on_page_writeback(dstpage, NODE, true);
> +
> +	file_clear_enc_name(dst);
> +	if (file_enc_name(src)) {
> +		struct f2fs_inode *srcri = F2FS_INODE(srcpage);
> +
> +		memcpy(tmp_srcname, srcri->i_name, srcri->i_namelen);
> +		new_srcname.name = tmp_srcname;
> +		new_srcname.len = srcri->i_namelen;
> +
> +		file_set_enc_name(dst);
> +	}
> +
> +	file_clear_enc_name(src);
> +	if (file_enc_name(dst)) {
> +		struct f2fs_inode *dstri = F2FS_INODE(dstpage);
> +
> +		memcpy(tmp_dstname, dstri->i_name, dstri->i_namelen);
> +		new_dstname.name = tmp_dstname;
> +		new_dstname.len = dstri->i_namelen;
> +
> +		file_set_enc_name(src);
> +	}
> +
> +	init_dent_inode(&new_dstname, srcpage);
> +	init_dent_inode(&new_srcname, dstpage);
> +
> +	f2fs_put_page(srcpage, 1);
> +	f2fs_put_page(dstpage, 1);
>  
>  	return 0;
>  }
> @@ -435,9 +511,14 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>  		set_cold_node(inode, page);
>  	}
>  
> -	if (new_name)
> +	if (new_name) {
>  		init_dent_inode(new_name, page);
>  
> +		file_clear_enc_name(inode);
> +		if (f2fs_encrypted_inode(dir))
> +			file_set_enc_name(inode);
> +	}
> +
>  	/*
>  	 * This file should be checkpointed during fsync.
>  	 * We lost i_pino from now on.
> @@ -596,8 +677,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>  			err = PTR_ERR(page);
>  			goto fail;
>  		}
> -		if (f2fs_encrypted_inode(dir))
> -			file_set_enc_name(inode);
>  	}
>  
>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 7e29249..ce24fe9 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -440,6 +440,7 @@ struct f2fs_map_blocks {
>  #define file_clear_encrypt(inode) clear_file(inode, FADVISE_ENCRYPT_BIT)
>  #define file_enc_name(inode)	is_file(inode, FADVISE_ENC_NAME_BIT)
>  #define file_set_enc_name(inode) set_file(inode, FADVISE_ENC_NAME_BIT)
> +#define file_clear_enc_name(inode) clear_file(inode, FADVISE_ENC_NAME_BIT)
>  #define file_keep_isize(inode)	is_file(inode, FADVISE_KEEP_SIZE_BIT)
>  #define file_set_keep_isize(inode) set_file(inode, FADVISE_KEEP_SIZE_BIT)
>  
> @@ -2101,6 +2102,8 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>  			struct page *page, struct inode *inode);
>  int update_dent_inode(struct inode *inode, struct inode *to,
>  			const struct qstr *name);
> +int exchange_dent_inode(struct inode *src, struct inode *dst,
> +			const struct qstr *srcname, const struct qstr *dstname);
>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>  			const struct qstr *name, f2fs_hash_t name_hash,
>  			unsigned int bit_pos);
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index e32a9e5..41fe27d 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>  			err = PTR_ERR(page);
>  			goto fail;
>  		}
> -		if (f2fs_encrypted_inode(dir))
> -			file_set_enc_name(inode);
>  	}
>  
>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 3231a0a..57050ff 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -779,8 +779,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	down_write(&F2FS_I(old_inode)->i_sem);
>  	file_lost_pino(old_inode);
> -	if (new_inode && file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
>  	up_write(&F2FS_I(old_inode)->i_sem);
>  
>  	old_inode->i_ctime = current_time(old_inode);
> @@ -917,17 +915,10 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	f2fs_lock_op(sbi);
>  
> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
> +	err = exchange_dent_inode(old_inode, new_inode,
> +				  &old_dentry->d_name, &new_dentry->d_name);
>  	if (err)
>  		goto out_unlock;
> -	if (file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
> -
> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
> -	if (err)
> -		goto out_undo;
> -	if (file_enc_name(old_inode))
> -		file_set_enc_name(new_inode);
>  
>  	/* update ".." directory entry info of old dentry */
>  	if (old_dir_entry)
> @@ -972,12 +963,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>  		f2fs_sync_fs(sbi->sb, 1);
>  	return 0;
> -out_undo:
> -	/*
> -	 * Still we may fail to recover name info of f2fs_inode here
> -	 * Drop it, once its name is set as encrypted
> -	 */
> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
>  out_unlock:
>  	f2fs_unlock_op(sbi);
>  out_new_dir:
> -- 
> 2.9.3

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

Re: [PATCH v2] f2fs: sync the enc name flags with disk level filename

[Kinglong Mee]

On 3/8/2017 03:30, Jaegeuk Kim wrote:
> Hi Kinglong,
> 
> Can we make a testcase in xfstests to check this clearly?
> 1. creat A under encrypted dir
> 2. rename A to B
> 3. fsync B
> 4. power-cut
> 
> Is this your concern?

Yes, it is.
If B isn't exist, rename A to B means create a new A (unlink the old).
B is exist, the inode (include disk node) will be replace by A, but,
for the file_enc_name, the disk i_name/i_namelen isn't updated.

For the rename, A and B lost there parent, so the 3 step will cause checkpoint, 
after that, everything is updated to disk (with the unchanged filename), 
so that the 4 step of power-cut can't cause a roll_forward.

If fsync A for flags the inode and then rename as,
1. creat A and B under encrypted dir
2. fsync A          (after rename, B will be unlink)
3. rename A to B
4. godown 
5. umount/mount

A roll_forward (recover_dentry) happens, the result is same as after setp1,
that's not my needs.

For the IS_CHECKPOINTED flag, after a new created file sync to the disk,
it's cleared, and never appears again, after that a roll_forward of 
recover_dentry will never happen (it means the disk i_name will not be
used forever). Am I right?

If that's right, next update_dent_inode after file created is useless,
we can remove them include the FADVISE_ENC_NAME_BIT.

> 
> Hmm, on-disk filename is used when doing roll-forward recovery, and it seems
> there is a hole in try_to_fix_pino() to recover its pino without filename.
> 
> Like this?
> 
>>From 7efde035d9f930fc63c30b25327e870b021750f3 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> Date: Tue, 7 Mar 2017 11:22:45 -0800
> Subject: [PATCH] f2fS: don't allow to get pino when filename is encrypted
> 
> After renaming an encrypted file, we have no way to get its
> encrypted filename from its dentry.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/file.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index e0b2378f8519..852195b3bcce 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -110,6 +110,9 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>  {
>  	struct dentry *dentry;
>  
> +	if (file_enc_name(inode))
> +		return 0;
> +
>  	inode = igrab(inode);
>  	dentry = d_find_any_alias(inode);
>  	iput(inode);
> 

The renamed inode will lost their parent ino, and try to fix it when fsync.
With the file_enc_name checking, get_parent_ino return 0 without parent ino
for encrypted inode, after that the FADVISE_COLD_BIT will exist forever, 
fsync the encrypted inode causing do_checkpoint every time.

static void try_to_fix_pino(struct inode *inode)
{
        struct f2fs_inode_info *fi = F2FS_I(inode);
        nid_t pino;

        down_write(&fi->i_sem);
        if (file_wrong_pino(inode) && inode->i_nlink == 1 &&
                        get_parent_ino(inode, &pino)) {
                f2fs_i_pino_write(inode, pino);
                file_got_pino(inode);
        }
        up_write(&fi->i_sem);
}

thanks,
Kinglong Mee

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

Re: [PATCH v2] f2fs: sync the enc name flags with disk level filename

[Chao Yu]

On 2017/3/6 16:11, Kinglong Mee wrote:
> A cross rename between two encrypted files, the disk level filenames
> aren't updated for the file_enc_name(to) checking.
> 
> Also, cross rename between encrypted file and non-encrypted file under
> a non-encrypted file, the enc name flags should update at same time
> as the disk level filename.
> 
> This patch,
> 1. make sure update the disk level filename in update_dent_inode,

Doesn't need to do this since tagging lost_pino is enough, later, fsync will
trigger checkpoint, and then nodes and dents will be persistent with checkpoint,
so after abnormal power off, roll-forward doesn't need to recover dent from
inode::filename.

Thanks,

> 2. a new function exchange_dent_inode for the disk-level filename update,
> 3. only set the enc name flags in init_inode_metadata.
> 
> v2, add the missing function define of exchange_dent_inode in f2fs.h
> 
> Fixes: e7d5545285ed ("f2fs crypto: add filename encryption for roll-forward recovery")
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/f2fs/dir.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  fs/f2fs/f2fs.h   |  3 ++
>  fs/f2fs/inline.c |  2 --
>  fs/f2fs/namei.c  | 19 ++----------
>  4 files changed, 90 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 295a223..b1bb0b1 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -337,17 +337,93 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>  int update_dent_inode(struct inode *inode, struct inode *to,
>  					const struct qstr *name)
>  {
> -	struct page *page;
> +	struct page *topage = NULL, *page;
> +	struct f2fs_inode *tori;
> +	struct qstr newname = *name;
> +	int err = 0;
>  
> -	if (file_enc_name(to))
> +	if (file_enc_name(to) && (inode == to))
>  		return 0;
>  
>  	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
>  	if (IS_ERR(page))
>  		return PTR_ERR(page);
>  
> -	init_dent_inode(name, page);
> +	file_clear_enc_name(inode);
> +	if (file_enc_name(to)) {
> +		topage = get_node_page(F2FS_I_SB(to), to->i_ino);
> +		if (IS_ERR(topage)) {
> +			f2fs_put_page(page, 1);
> +			return PTR_ERR(topage);
> +		}
> +
> +		tori = F2FS_INODE(topage);
> +		newname.name = tori->i_name;
> +		newname.len = tori->i_namelen;
> +
> +		file_set_enc_name(inode);
> +	}
> +
> +	init_dent_inode(&newname, page);
> +
>  	f2fs_put_page(page, 1);
> +	if (file_enc_name(to))
> +		f2fs_put_page(topage, 1);
> +
> +	return err;
> +}
> +
> +int exchange_dent_inode(struct inode *src, struct inode *dst,
> +			const struct qstr *srcname, const struct qstr *dstname)
> +{
> +	struct page *srcpage = NULL, *dstpage = NULL;
> +	char tmp_srcname[F2FS_NAME_LEN], tmp_dstname[F2FS_NAME_LEN];
> +	struct qstr new_srcname = *srcname;
> +	struct qstr new_dstname = *dstname;
> +
> +	if (src == dst)
> +		return 0;
> +
> +	srcpage = get_node_page(F2FS_I_SB(src), src->i_ino);
> +	if (IS_ERR(srcpage))
> +		return PTR_ERR(srcpage);
> +
> +	dstpage = get_node_page(F2FS_I_SB(dst), dst->i_ino);
> +	if (IS_ERR(dstpage)) {
> +		f2fs_put_page(srcpage, 1);
> +		return PTR_ERR(dstpage);
> +	}
> +
> +	f2fs_wait_on_page_writeback(srcpage, NODE, true);
> +	f2fs_wait_on_page_writeback(dstpage, NODE, true);
> +
> +	file_clear_enc_name(dst);
> +	if (file_enc_name(src)) {
> +		struct f2fs_inode *srcri = F2FS_INODE(srcpage);
> +
> +		memcpy(tmp_srcname, srcri->i_name, srcri->i_namelen);
> +		new_srcname.name = tmp_srcname;
> +		new_srcname.len = srcri->i_namelen;
> +
> +		file_set_enc_name(dst);
> +	}
> +
> +	file_clear_enc_name(src);
> +	if (file_enc_name(dst)) {
> +		struct f2fs_inode *dstri = F2FS_INODE(dstpage);
> +
> +		memcpy(tmp_dstname, dstri->i_name, dstri->i_namelen);
> +		new_dstname.name = tmp_dstname;
> +		new_dstname.len = dstri->i_namelen;
> +
> +		file_set_enc_name(src);
> +	}
> +
> +	init_dent_inode(&new_dstname, srcpage);
> +	init_dent_inode(&new_srcname, dstpage);
> +
> +	f2fs_put_page(srcpage, 1);
> +	f2fs_put_page(dstpage, 1);
>  
>  	return 0;
>  }
> @@ -435,9 +511,14 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>  		set_cold_node(inode, page);
>  	}
>  
> -	if (new_name)
> +	if (new_name) {
>  		init_dent_inode(new_name, page);
>  
> +		file_clear_enc_name(inode);
> +		if (f2fs_encrypted_inode(dir))
> +			file_set_enc_name(inode);
> +	}
> +
>  	/*
>  	 * This file should be checkpointed during fsync.
>  	 * We lost i_pino from now on.
> @@ -596,8 +677,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>  			err = PTR_ERR(page);
>  			goto fail;
>  		}
> -		if (f2fs_encrypted_inode(dir))
> -			file_set_enc_name(inode);
>  	}
>  
>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 7e29249..ce24fe9 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -440,6 +440,7 @@ struct f2fs_map_blocks {
>  #define file_clear_encrypt(inode) clear_file(inode, FADVISE_ENCRYPT_BIT)
>  #define file_enc_name(inode)	is_file(inode, FADVISE_ENC_NAME_BIT)
>  #define file_set_enc_name(inode) set_file(inode, FADVISE_ENC_NAME_BIT)
> +#define file_clear_enc_name(inode) clear_file(inode, FADVISE_ENC_NAME_BIT)
>  #define file_keep_isize(inode)	is_file(inode, FADVISE_KEEP_SIZE_BIT)
>  #define file_set_keep_isize(inode) set_file(inode, FADVISE_KEEP_SIZE_BIT)
>  
> @@ -2101,6 +2102,8 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>  			struct page *page, struct inode *inode);
>  int update_dent_inode(struct inode *inode, struct inode *to,
>  			const struct qstr *name);
> +int exchange_dent_inode(struct inode *src, struct inode *dst,
> +			const struct qstr *srcname, const struct qstr *dstname);
>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>  			const struct qstr *name, f2fs_hash_t name_hash,
>  			unsigned int bit_pos);
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index e32a9e5..41fe27d 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>  			err = PTR_ERR(page);
>  			goto fail;
>  		}
> -		if (f2fs_encrypted_inode(dir))
> -			file_set_enc_name(inode);
>  	}
>  
>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 3231a0a..57050ff 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -779,8 +779,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	down_write(&F2FS_I(old_inode)->i_sem);
>  	file_lost_pino(old_inode);
> -	if (new_inode && file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
>  	up_write(&F2FS_I(old_inode)->i_sem);
>  
>  	old_inode->i_ctime = current_time(old_inode);
> @@ -917,17 +915,10 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	f2fs_lock_op(sbi);
>  
> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
> +	err = exchange_dent_inode(old_inode, new_inode,
> +				  &old_dentry->d_name, &new_dentry->d_name);
>  	if (err)
>  		goto out_unlock;
> -	if (file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
> -
> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
> -	if (err)
> -		goto out_undo;
> -	if (file_enc_name(old_inode))
> -		file_set_enc_name(new_inode);
>  
>  	/* update ".." directory entry info of old dentry */
>  	if (old_dir_entry)
> @@ -972,12 +963,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>  		f2fs_sync_fs(sbi->sb, 1);
>  	return 0;
> -out_undo:
> -	/*
> -	 * Still we may fail to recover name info of f2fs_inode here
> -	 * Drop it, once its name is set as encrypted
> -	 */
> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
>  out_unlock:
>  	f2fs_unlock_op(sbi);
>  out_new_dir:
> 


------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

Re: [PATCH v2] f2fs: sync the enc name flags with disk level filename

[Kinglong Mee]

On 3/8/2017 20:14, Chao Yu wrote:
> On 2017/3/6 16:11, Kinglong Mee wrote:
>> A cross rename between two encrypted files, the disk level filenames
>> aren't updated for the file_enc_name(to) checking.
>>
>> Also, cross rename between encrypted file and non-encrypted file under
>> a non-encrypted file, the enc name flags should update at same time
>> as the disk level filename.
>>
>> This patch,
>> 1. make sure update the disk level filename in update_dent_inode,
> 
> Doesn't need to do this since tagging lost_pino is enough, later, fsync will
> trigger checkpoint, and then nodes and dents will be persistent with checkpoint,
> so after abnormal power off, roll-forward doesn't need to recover dent from
> inode::filename.

Yes, that's right.
So the update_dent_inode update the disk level is useless, 
the disk level filename will never be used forever? right?

thanks,
Kinglong Mee

> 
> Thanks,
> 
>> 2. a new function exchange_dent_inode for the disk-level filename update,
>> 3. only set the enc name flags in init_inode_metadata.
>>
>> v2, add the missing function define of exchange_dent_inode in f2fs.h
>>
>> Fixes: e7d5545285ed ("f2fs crypto: add filename encryption for roll-forward recovery")
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/f2fs/dir.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>  fs/f2fs/f2fs.h   |  3 ++
>>  fs/f2fs/inline.c |  2 --
>>  fs/f2fs/namei.c  | 19 ++----------
>>  4 files changed, 90 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index 295a223..b1bb0b1 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -337,17 +337,93 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>>  int update_dent_inode(struct inode *inode, struct inode *to,
>>  					const struct qstr *name)
>>  {
>> -	struct page *page;
>> +	struct page *topage = NULL, *page;
>> +	struct f2fs_inode *tori;
>> +	struct qstr newname = *name;
>> +	int err = 0;
>>  
>> -	if (file_enc_name(to))
>> +	if (file_enc_name(to) && (inode == to))
>>  		return 0;
>>  
>>  	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
>>  	if (IS_ERR(page))
>>  		return PTR_ERR(page);
>>  
>> -	init_dent_inode(name, page);
>> +	file_clear_enc_name(inode);
>> +	if (file_enc_name(to)) {
>> +		topage = get_node_page(F2FS_I_SB(to), to->i_ino);
>> +		if (IS_ERR(topage)) {
>> +			f2fs_put_page(page, 1);
>> +			return PTR_ERR(topage);
>> +		}
>> +
>> +		tori = F2FS_INODE(topage);
>> +		newname.name = tori->i_name;
>> +		newname.len = tori->i_namelen;
>> +
>> +		file_set_enc_name(inode);
>> +	}
>> +
>> +	init_dent_inode(&newname, page);
>> +
>>  	f2fs_put_page(page, 1);
>> +	if (file_enc_name(to))
>> +		f2fs_put_page(topage, 1);
>> +
>> +	return err;
>> +}
>> +
>> +int exchange_dent_inode(struct inode *src, struct inode *dst,
>> +			const struct qstr *srcname, const struct qstr *dstname)
>> +{
>> +	struct page *srcpage = NULL, *dstpage = NULL;
>> +	char tmp_srcname[F2FS_NAME_LEN], tmp_dstname[F2FS_NAME_LEN];
>> +	struct qstr new_srcname = *srcname;
>> +	struct qstr new_dstname = *dstname;
>> +
>> +	if (src == dst)
>> +		return 0;
>> +
>> +	srcpage = get_node_page(F2FS_I_SB(src), src->i_ino);
>> +	if (IS_ERR(srcpage))
>> +		return PTR_ERR(srcpage);
>> +
>> +	dstpage = get_node_page(F2FS_I_SB(dst), dst->i_ino);
>> +	if (IS_ERR(dstpage)) {
>> +		f2fs_put_page(srcpage, 1);
>> +		return PTR_ERR(dstpage);
>> +	}
>> +
>> +	f2fs_wait_on_page_writeback(srcpage, NODE, true);
>> +	f2fs_wait_on_page_writeback(dstpage, NODE, true);
>> +
>> +	file_clear_enc_name(dst);
>> +	if (file_enc_name(src)) {
>> +		struct f2fs_inode *srcri = F2FS_INODE(srcpage);
>> +
>> +		memcpy(tmp_srcname, srcri->i_name, srcri->i_namelen);
>> +		new_srcname.name = tmp_srcname;
>> +		new_srcname.len = srcri->i_namelen;
>> +
>> +		file_set_enc_name(dst);
>> +	}
>> +
>> +	file_clear_enc_name(src);
>> +	if (file_enc_name(dst)) {
>> +		struct f2fs_inode *dstri = F2FS_INODE(dstpage);
>> +
>> +		memcpy(tmp_dstname, dstri->i_name, dstri->i_namelen);
>> +		new_dstname.name = tmp_dstname;
>> +		new_dstname.len = dstri->i_namelen;
>> +
>> +		file_set_enc_name(src);
>> +	}
>> +
>> +	init_dent_inode(&new_dstname, srcpage);
>> +	init_dent_inode(&new_srcname, dstpage);
>> +
>> +	f2fs_put_page(srcpage, 1);
>> +	f2fs_put_page(dstpage, 1);
>>  
>>  	return 0;
>>  }
>> @@ -435,9 +511,14 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>>  		set_cold_node(inode, page);
>>  	}
>>  
>> -	if (new_name)
>> +	if (new_name) {
>>  		init_dent_inode(new_name, page);
>>  
>> +		file_clear_enc_name(inode);
>> +		if (f2fs_encrypted_inode(dir))
>> +			file_set_enc_name(inode);
>> +	}
>> +
>>  	/*
>>  	 * This file should be checkpointed during fsync.
>>  	 * We lost i_pino from now on.
>> @@ -596,8 +677,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>>  			err = PTR_ERR(page);
>>  			goto fail;
>>  		}
>> -		if (f2fs_encrypted_inode(dir))
>> -			file_set_enc_name(inode);
>>  	}
>>  
>>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 7e29249..ce24fe9 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -440,6 +440,7 @@ struct f2fs_map_blocks {
>>  #define file_clear_encrypt(inode) clear_file(inode, FADVISE_ENCRYPT_BIT)
>>  #define file_enc_name(inode)	is_file(inode, FADVISE_ENC_NAME_BIT)
>>  #define file_set_enc_name(inode) set_file(inode, FADVISE_ENC_NAME_BIT)
>> +#define file_clear_enc_name(inode) clear_file(inode, FADVISE_ENC_NAME_BIT)
>>  #define file_keep_isize(inode)	is_file(inode, FADVISE_KEEP_SIZE_BIT)
>>  #define file_set_keep_isize(inode) set_file(inode, FADVISE_KEEP_SIZE_BIT)
>>  
>> @@ -2101,6 +2102,8 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>>  			struct page *page, struct inode *inode);
>>  int update_dent_inode(struct inode *inode, struct inode *to,
>>  			const struct qstr *name);
>> +int exchange_dent_inode(struct inode *src, struct inode *dst,
>> +			const struct qstr *srcname, const struct qstr *dstname);
>>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>>  			const struct qstr *name, f2fs_hash_t name_hash,
>>  			unsigned int bit_pos);
>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>> index e32a9e5..41fe27d 100644
>> --- a/fs/f2fs/inline.c
>> +++ b/fs/f2fs/inline.c
>> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>>  			err = PTR_ERR(page);
>>  			goto fail;
>>  		}
>> -		if (f2fs_encrypted_inode(dir))
>> -			file_set_enc_name(inode);
>>  	}
>>  
>>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index 3231a0a..57050ff 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -779,8 +779,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	down_write(&F2FS_I(old_inode)->i_sem);
>>  	file_lost_pino(old_inode);
>> -	if (new_inode && file_enc_name(new_inode))
>> -		file_set_enc_name(old_inode);
>>  	up_write(&F2FS_I(old_inode)->i_sem);
>>  
>>  	old_inode->i_ctime = current_time(old_inode);
>> @@ -917,17 +915,10 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	f2fs_lock_op(sbi);
>>  
>> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
>> +	err = exchange_dent_inode(old_inode, new_inode,
>> +				  &old_dentry->d_name, &new_dentry->d_name);
>>  	if (err)
>>  		goto out_unlock;
>> -	if (file_enc_name(new_inode))
>> -		file_set_enc_name(old_inode);
>> -
>> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
>> -	if (err)
>> -		goto out_undo;
>> -	if (file_enc_name(old_inode))
>> -		file_set_enc_name(new_inode);
>>  
>>  	/* update ".." directory entry info of old dentry */
>>  	if (old_dir_entry)
>> @@ -972,12 +963,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>>  		f2fs_sync_fs(sbi->sb, 1);
>>  	return 0;
>> -out_undo:
>> -	/*
>> -	 * Still we may fail to recover name info of f2fs_inode here
>> -	 * Drop it, once its name is set as encrypted
>> -	 */
>> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
>>  out_unlock:
>>  	f2fs_unlock_op(sbi);
>>  out_new_dir:
>>
> 
> 

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

Re: [PATCH v2] f2fs: sync the enc name flags with disk level filename

[Chao Yu]

On 2017/3/8 21:16, Kinglong Mee wrote:
> On 3/8/2017 20:14, Chao Yu wrote:
>> On 2017/3/6 16:11, Kinglong Mee wrote:
>>> A cross rename between two encrypted files, the disk level filenames
>>> aren't updated for the file_enc_name(to) checking.
>>>
>>> Also, cross rename between encrypted file and non-encrypted file under
>>> a non-encrypted file, the enc name flags should update at same time
>>> as the disk level filename.
>>>
>>> This patch,
>>> 1. make sure update the disk level filename in update_dent_inode,
>>
>> Doesn't need to do this since tagging lost_pino is enough, later, fsync will
>> trigger checkpoint, and then nodes and dents will be persistent with checkpoint,
>> so after abnormal power off, roll-forward doesn't need to recover dent from
>> inode::filename.
> 
> Yes, that's right.
> So the update_dent_inode update the disk level is useless, 
> the disk level filename will never be used forever? right?

Yup. :)

Thanks,

> 
> thanks,
> Kinglong Mee
> 
>>
>> Thanks,
>>
>>> 2. a new function exchange_dent_inode for the disk-level filename update,
>>> 3. only set the enc name flags in init_inode_metadata.
>>>
>>> v2, add the missing function define of exchange_dent_inode in f2fs.h
>>>
>>> Fixes: e7d5545285ed ("f2fs crypto: add filename encryption for roll-forward recovery")
>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>> ---
>>>  fs/f2fs/dir.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>  fs/f2fs/f2fs.h   |  3 ++
>>>  fs/f2fs/inline.c |  2 --
>>>  fs/f2fs/namei.c  | 19 ++----------
>>>  4 files changed, 90 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>>> index 295a223..b1bb0b1 100644
>>> --- a/fs/f2fs/dir.c
>>> +++ b/fs/f2fs/dir.c
>>> @@ -337,17 +337,93 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>>>  int update_dent_inode(struct inode *inode, struct inode *to,
>>>  					const struct qstr *name)
>>>  {
>>> -	struct page *page;
>>> +	struct page *topage = NULL, *page;
>>> +	struct f2fs_inode *tori;
>>> +	struct qstr newname = *name;
>>> +	int err = 0;
>>>  
>>> -	if (file_enc_name(to))
>>> +	if (file_enc_name(to) && (inode == to))
>>>  		return 0;
>>>  
>>>  	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
>>>  	if (IS_ERR(page))
>>>  		return PTR_ERR(page);
>>>  
>>> -	init_dent_inode(name, page);
>>> +	file_clear_enc_name(inode);
>>> +	if (file_enc_name(to)) {
>>> +		topage = get_node_page(F2FS_I_SB(to), to->i_ino);
>>> +		if (IS_ERR(topage)) {
>>> +			f2fs_put_page(page, 1);
>>> +			return PTR_ERR(topage);
>>> +		}
>>> +
>>> +		tori = F2FS_INODE(topage);
>>> +		newname.name = tori->i_name;
>>> +		newname.len = tori->i_namelen;
>>> +
>>> +		file_set_enc_name(inode);
>>> +	}
>>> +
>>> +	init_dent_inode(&newname, page);
>>> +
>>>  	f2fs_put_page(page, 1);
>>> +	if (file_enc_name(to))
>>> +		f2fs_put_page(topage, 1);
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +int exchange_dent_inode(struct inode *src, struct inode *dst,
>>> +			const struct qstr *srcname, const struct qstr *dstname)
>>> +{
>>> +	struct page *srcpage = NULL, *dstpage = NULL;
>>> +	char tmp_srcname[F2FS_NAME_LEN], tmp_dstname[F2FS_NAME_LEN];
>>> +	struct qstr new_srcname = *srcname;
>>> +	struct qstr new_dstname = *dstname;
>>> +
>>> +	if (src == dst)
>>> +		return 0;
>>> +
>>> +	srcpage = get_node_page(F2FS_I_SB(src), src->i_ino);
>>> +	if (IS_ERR(srcpage))
>>> +		return PTR_ERR(srcpage);
>>> +
>>> +	dstpage = get_node_page(F2FS_I_SB(dst), dst->i_ino);
>>> +	if (IS_ERR(dstpage)) {
>>> +		f2fs_put_page(srcpage, 1);
>>> +		return PTR_ERR(dstpage);
>>> +	}
>>> +
>>> +	f2fs_wait_on_page_writeback(srcpage, NODE, true);
>>> +	f2fs_wait_on_page_writeback(dstpage, NODE, true);
>>> +
>>> +	file_clear_enc_name(dst);
>>> +	if (file_enc_name(src)) {
>>> +		struct f2fs_inode *srcri = F2FS_INODE(srcpage);
>>> +
>>> +		memcpy(tmp_srcname, srcri->i_name, srcri->i_namelen);
>>> +		new_srcname.name = tmp_srcname;
>>> +		new_srcname.len = srcri->i_namelen;
>>> +
>>> +		file_set_enc_name(dst);
>>> +	}
>>> +
>>> +	file_clear_enc_name(src);
>>> +	if (file_enc_name(dst)) {
>>> +		struct f2fs_inode *dstri = F2FS_INODE(dstpage);
>>> +
>>> +		memcpy(tmp_dstname, dstri->i_name, dstri->i_namelen);
>>> +		new_dstname.name = tmp_dstname;
>>> +		new_dstname.len = dstri->i_namelen;
>>> +
>>> +		file_set_enc_name(src);
>>> +	}
>>> +
>>> +	init_dent_inode(&new_dstname, srcpage);
>>> +	init_dent_inode(&new_srcname, dstpage);
>>> +
>>> +	f2fs_put_page(srcpage, 1);
>>> +	f2fs_put_page(dstpage, 1);
>>>  
>>>  	return 0;
>>>  }
>>> @@ -435,9 +511,14 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>>>  		set_cold_node(inode, page);
>>>  	}
>>>  
>>> -	if (new_name)
>>> +	if (new_name) {
>>>  		init_dent_inode(new_name, page);
>>>  
>>> +		file_clear_enc_name(inode);
>>> +		if (f2fs_encrypted_inode(dir))
>>> +			file_set_enc_name(inode);
>>> +	}
>>> +
>>>  	/*
>>>  	 * This file should be checkpointed during fsync.
>>>  	 * We lost i_pino from now on.
>>> @@ -596,8 +677,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>>>  			err = PTR_ERR(page);
>>>  			goto fail;
>>>  		}
>>> -		if (f2fs_encrypted_inode(dir))
>>> -			file_set_enc_name(inode);
>>>  	}
>>>  
>>>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 7e29249..ce24fe9 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -440,6 +440,7 @@ struct f2fs_map_blocks {
>>>  #define file_clear_encrypt(inode) clear_file(inode, FADVISE_ENCRYPT_BIT)
>>>  #define file_enc_name(inode)	is_file(inode, FADVISE_ENC_NAME_BIT)
>>>  #define file_set_enc_name(inode) set_file(inode, FADVISE_ENC_NAME_BIT)
>>> +#define file_clear_enc_name(inode) clear_file(inode, FADVISE_ENC_NAME_BIT)
>>>  #define file_keep_isize(inode)	is_file(inode, FADVISE_KEEP_SIZE_BIT)
>>>  #define file_set_keep_isize(inode) set_file(inode, FADVISE_KEEP_SIZE_BIT)
>>>  
>>> @@ -2101,6 +2102,8 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>>>  			struct page *page, struct inode *inode);
>>>  int update_dent_inode(struct inode *inode, struct inode *to,
>>>  			const struct qstr *name);
>>> +int exchange_dent_inode(struct inode *src, struct inode *dst,
>>> +			const struct qstr *srcname, const struct qstr *dstname);
>>>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>>>  			const struct qstr *name, f2fs_hash_t name_hash,
>>>  			unsigned int bit_pos);
>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>>> index e32a9e5..41fe27d 100644
>>> --- a/fs/f2fs/inline.c
>>> +++ b/fs/f2fs/inline.c
>>> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>>>  			err = PTR_ERR(page);
>>>  			goto fail;
>>>  		}
>>> -		if (f2fs_encrypted_inode(dir))
>>> -			file_set_enc_name(inode);
>>>  	}
>>>  
>>>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
>>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>>> index 3231a0a..57050ff 100644
>>> --- a/fs/f2fs/namei.c
>>> +++ b/fs/f2fs/namei.c
>>> @@ -779,8 +779,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  
>>>  	down_write(&F2FS_I(old_inode)->i_sem);
>>>  	file_lost_pino(old_inode);
>>> -	if (new_inode && file_enc_name(new_inode))
>>> -		file_set_enc_name(old_inode);
>>>  	up_write(&F2FS_I(old_inode)->i_sem);
>>>  
>>>  	old_inode->i_ctime = current_time(old_inode);
>>> @@ -917,17 +915,10 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  
>>>  	f2fs_lock_op(sbi);
>>>  
>>> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
>>> +	err = exchange_dent_inode(old_inode, new_inode,
>>> +				  &old_dentry->d_name, &new_dentry->d_name);
>>>  	if (err)
>>>  		goto out_unlock;
>>> -	if (file_enc_name(new_inode))
>>> -		file_set_enc_name(old_inode);
>>> -
>>> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
>>> -	if (err)
>>> -		goto out_undo;
>>> -	if (file_enc_name(old_inode))
>>> -		file_set_enc_name(new_inode);
>>>  
>>>  	/* update ".." directory entry info of old dentry */
>>>  	if (old_dir_entry)
>>> @@ -972,12 +963,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>>>  		f2fs_sync_fs(sbi->sb, 1);
>>>  	return 0;
>>> -out_undo:
>>> -	/*
>>> -	 * Still we may fail to recover name info of f2fs_inode here
>>> -	 * Drop it, once its name is set as encrypted
>>> -	 */
>>> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
>>>  out_unlock:
>>>  	f2fs_unlock_op(sbi);
>>>  out_new_dir:
>>>
>>
>>
> 
> .
> 


------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

Re: [PATCH v2] f2fs: sync the enc name flags with disk level filename

[Kinglong Mee]

On 3/8/2017 18:56, Kinglong Mee wrote:
> On 3/8/2017 03:30, Jaegeuk Kim wrote:
>> Hi Kinglong,
>>
>> Can we make a testcase in xfstests to check this clearly?
>> 1. creat A under encrypted dir
>> 2. rename A to B
>> 3. fsync B
>> 4. power-cut
>>
>> Is this your concern?
> 
> Yes, it is.
> If B isn't exist, rename A to B means create a new A (unlink the old).
> B is exist, the inode (include disk node) will be replace by A, but,
> for the file_enc_name, the disk i_name/i_namelen isn't updated.
> 
> For the rename, A and B lost there parent, so the 3 step will cause checkpoint, 
> after that, everything is updated to disk (with the unchanged filename), 
> so that the 4 step of power-cut can't cause a roll_forward.
> 
> If fsync A for flags the inode and then rename as,
> 1. creat A and B under encrypted dir
> 2. fsync A          (after rename, B will be unlink)
> 3. rename A to B
> 4. godown 
> 5. umount/mount
> 
> A roll_forward (recover_dentry) happens, the result is same as after setp1,
> that's not my needs.
> 
> For the IS_CHECKPOINTED flag, after a new created file sync to the disk,
> it's cleared, and never appears again, after that a roll_forward of 
> recover_dentry will never happen (it means the disk i_name will not be
> used forever). Am I right?
> 
> If that's right, next update_dent_inode after file created is useless,
> we can remove them include the FADVISE_ENC_NAME_BIT.

Hi Jaegeuk, 

What's your opinion?
If you agree that they are useless of the disk-level filename updating, 
I will cleanup them.

thanks,
Kinglong Mee

> 
>>
>> Hmm, on-disk filename is used when doing roll-forward recovery, and it seems
>> there is a hole in try_to_fix_pino() to recover its pino without filename.
>>
>> Like this?
>>
>> >From 7efde035d9f930fc63c30b25327e870b021750f3 Mon Sep 17 00:00:00 2001
>> From: Jaegeuk Kim <jaegeuk@kernel.org>
>> Date: Tue, 7 Mar 2017 11:22:45 -0800
>> Subject: [PATCH] f2fS: don't allow to get pino when filename is encrypted
>>
>> After renaming an encrypted file, we have no way to get its
>> encrypted filename from its dentry.
>>
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>> ---
>>  fs/f2fs/file.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index e0b2378f8519..852195b3bcce 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -110,6 +110,9 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>>  {
>>  	struct dentry *dentry;
>>  
>> +	if (file_enc_name(inode))
>> +		return 0;
>> +
>>  	inode = igrab(inode);
>>  	dentry = d_find_any_alias(inode);
>>  	iput(inode);
>>

Yes, this patch is needed, but it causes the following problem of 
the forever lost parent inode flags for encrypted files after rename.

thanks,
Kinglong Mee

> 
> The renamed inode will lost their parent ino, and try to fix it when fsync.
> With the file_enc_name checking, get_parent_ino return 0 without parent ino
> for encrypted inode, after that the FADVISE_COLD_BIT will exist forever, 
> fsync the encrypted inode causing do_checkpoint every time.
> 
> static void try_to_fix_pino(struct inode *inode)
> {
>         struct f2fs_inode_info *fi = F2FS_I(inode);
>         nid_t pino;
> 
>         down_write(&fi->i_sem);
>         if (file_wrong_pino(inode) && inode->i_nlink == 1 &&
>                         get_parent_ino(inode, &pino)) {
>                 f2fs_i_pino_write(inode, pino);
>                 file_got_pino(inode);
>         }
>         up_write(&fi->i_sem);
> }
> 
> thanks,
> Kinglong Mee
> 

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

Re: [PATCH v2] f2fs: sync the enc name flags with disk level filename

[Jaegeuk Kim]

On 03/10, Kinglong Mee wrote:
> On 3/8/2017 18:56, Kinglong Mee wrote:
> > On 3/8/2017 03:30, Jaegeuk Kim wrote:
> >> Hi Kinglong,
> >>
> >> Can we make a testcase in xfstests to check this clearly?
> >> 1. creat A under encrypted dir
> >> 2. rename A to B
> >> 3. fsync B
> >> 4. power-cut
> >>
> >> Is this your concern?
> > 
> > Yes, it is.
> > If B isn't exist, rename A to B means create a new A (unlink the old).
> > B is exist, the inode (include disk node) will be replace by A, but,
> > for the file_enc_name, the disk i_name/i_namelen isn't updated.
> > 
> > For the rename, A and B lost there parent, so the 3 step will cause checkpoint, 
> > after that, everything is updated to disk (with the unchanged filename), 
> > so that the 4 step of power-cut can't cause a roll_forward.
> > 
> > If fsync A for flags the inode and then rename as,
> > 1. creat A and B under encrypted dir
> > 2. fsync A          (after rename, B will be unlink)
> > 3. rename A to B
> > 4. godown 
> > 5. umount/mount
> > 
> > A roll_forward (recover_dentry) happens, the result is same as after setp1,
> > that's not my needs.
> > 
> > For the IS_CHECKPOINTED flag, after a new created file sync to the disk,
> > it's cleared, and never appears again, after that a roll_forward of 
> > recover_dentry will never happen (it means the disk i_name will not be
> > used forever). Am I right?
> > 
> > If that's right, next update_dent_inode after file created is useless,
> > we can remove them include the FADVISE_ENC_NAME_BIT.
> 
> Hi Jaegeuk, 
> 
> What's your opinion?
> If you agree that they are useless of the disk-level filename updating, 
> I will cleanup them.

Agreed to that. Indeed, once checkpoint is done, we don't need to update
there-in filename at all. You may need to check whether inode is checkpointed
or not to detect that. BTW, it doesn't need to consider encrypted names only
though.

Thanks,

> 
> thanks,
> Kinglong Mee
> 
> > 
> >>
> >> Hmm, on-disk filename is used when doing roll-forward recovery, and it seems
> >> there is a hole in try_to_fix_pino() to recover its pino without filename.
> >>
> >> Like this?
> >>
> >> >From 7efde035d9f930fc63c30b25327e870b021750f3 Mon Sep 17 00:00:00 2001
> >> From: Jaegeuk Kim <jaegeuk@kernel.org>
> >> Date: Tue, 7 Mar 2017 11:22:45 -0800
> >> Subject: [PATCH] f2fS: don't allow to get pino when filename is encrypted
> >>
> >> After renaming an encrypted file, we have no way to get its
> >> encrypted filename from its dentry.
> >>
> >> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >> ---
> >>  fs/f2fs/file.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >> index e0b2378f8519..852195b3bcce 100644
> >> --- a/fs/f2fs/file.c
> >> +++ b/fs/f2fs/file.c
> >> @@ -110,6 +110,9 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
> >>  {
> >>  	struct dentry *dentry;
> >>  
> >> +	if (file_enc_name(inode))
> >> +		return 0;
> >> +
> >>  	inode = igrab(inode);
> >>  	dentry = d_find_any_alias(inode);
> >>  	iput(inode);
> >>
> 
> Yes, this patch is needed, but it causes the following problem of 
> the forever lost parent inode flags for encrypted files after rename.
> 
> thanks,
> Kinglong Mee
> 
> > 
> > The renamed inode will lost their parent ino, and try to fix it when fsync.
> > With the file_enc_name checking, get_parent_ino return 0 without parent ino
> > for encrypted inode, after that the FADVISE_COLD_BIT will exist forever, 
> > fsync the encrypted inode causing do_checkpoint every time.
> > 
> > static void try_to_fix_pino(struct inode *inode)
> > {
> >         struct f2fs_inode_info *fi = F2FS_I(inode);
> >         nid_t pino;
> > 
> >         down_write(&fi->i_sem);
> >         if (file_wrong_pino(inode) && inode->i_nlink == 1 &&
> >                         get_parent_ino(inode, &pino)) {
> >                 f2fs_i_pino_write(inode, pino);
> >                 file_got_pino(inode);
> >         }
> >         up_write(&fi->i_sem);
> > }
> > 
> > thanks,
> > Kinglong Mee
> > 

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

[PATCH v3] f2fs: cleanup the disk level filename updating

[Kinglong Mee]

As discuss with Jaegeuk and Chao,
"Once checkpoint is done, f2fs doesn't need to update there-in filename at all."

The disk-level filename is used only one case,
1. create a file A under a dir
2. sync A
3. godown
4. umount
5. mount (roll_forward)

Only the rename/cross_rename changes the filename, if it happens,
a. between step 1 and 2, the sync A will caused checkpoint, so that,
   the roll_forward at step 5 never happens.
b. after step 2, the roll_forward happens, file A will roll forward
   to the result as after step 1.

So that, any updating the disk filename is useless, just cleanup it.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/f2fs/dir.c    | 25 ++++---------------------
 fs/f2fs/f2fs.h   |  2 --
 fs/f2fs/file.c   |  8 --------
 fs/f2fs/inline.c |  2 --
 fs/f2fs/namei.c  | 29 -----------------------------
 5 files changed, 4 insertions(+), 62 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 8d5c62b..058c4f3 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -337,24 +337,6 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
 	set_page_dirty(ipage);
 }
 
-int update_dent_inode(struct inode *inode, struct inode *to,
-					const struct qstr *name)
-{
-	struct page *page;
-
-	if (file_enc_name(to))
-		return 0;
-
-	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
-	if (IS_ERR(page))
-		return PTR_ERR(page);
-
-	init_dent_inode(name, page);
-	f2fs_put_page(page, 1);
-
-	return 0;
-}
-
 void do_make_empty_dir(struct inode *inode, struct inode *parent,
 					struct f2fs_dentry_ptr *d)
 {
@@ -438,8 +420,11 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
 		set_cold_node(inode, page);
 	}
 
-	if (new_name)
+	if (new_name) {
 		init_dent_inode(new_name, page);
+		if (f2fs_encrypted_inode(dir))
+			file_set_enc_name(inode);
+	}
 
 	/*
 	 * This file should be checkpointed during fsync.
@@ -599,8 +584,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
 			err = PTR_ERR(page);
 			goto fail;
 		}
-		if (f2fs_encrypted_inode(dir))
-			file_set_enc_name(inode);
 	}
 
 	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7edb3be..5dbc0c0 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2093,8 +2093,6 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr,
 			struct page **page);
 void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
 			struct page *page, struct inode *inode);
-int update_dent_inode(struct inode *inode, struct inode *to,
-			const struct qstr *name);
 void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
 			const struct qstr *name, f2fs_hash_t name_hash,
 			unsigned int bit_pos);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 4ec764e..8c7923f 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -110,20 +110,12 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
 {
 	struct dentry *dentry;
 
-	if (file_enc_name(inode))
-		return 0;
-
 	inode = igrab(inode);
 	dentry = d_find_any_alias(inode);
 	iput(inode);
 	if (!dentry)
 		return 0;
 
-	if (update_dent_inode(inode, inode, &dentry->d_name)) {
-		dput(dentry);
-		return 0;
-	}
-
 	*pino = parent_ino(dentry);
 	dput(dentry);
 	return 1;
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index e32a9e5..41fe27d 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
 			err = PTR_ERR(page);
 			goto fail;
 		}
-		if (f2fs_encrypted_inode(dir))
-			file_set_enc_name(inode);
 	}
 
 	f2fs_wait_on_page_writeback(ipage, NODE, true);
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 25c073f..8906c9f 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -720,13 +720,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		if (err)
 			goto put_out_dir;
 
-		err = update_dent_inode(old_inode, new_inode,
-						&new_dentry->d_name);
-		if (err) {
-			release_orphan_inode(sbi);
-			goto put_out_dir;
-		}
-
 		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
 
 		new_inode->i_ctime = current_time(new_inode);
@@ -779,8 +772,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	down_write(&F2FS_I(old_inode)->i_sem);
 	file_lost_pino(old_inode);
-	if (new_inode && file_enc_name(new_inode))
-		file_set_enc_name(old_inode);
 	up_write(&F2FS_I(old_inode)->i_sem);
 
 	old_inode->i_ctime = current_time(old_inode);
@@ -917,18 +908,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	f2fs_lock_op(sbi);
 
-	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
-	if (err)
-		goto out_unlock;
-	if (file_enc_name(new_inode))
-		file_set_enc_name(old_inode);
-
-	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
-	if (err)
-		goto out_undo;
-	if (file_enc_name(old_inode))
-		file_set_enc_name(new_inode);
-
 	/* update ".." directory entry info of old dentry */
 	if (old_dir_entry)
 		f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
@@ -972,14 +951,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
 		f2fs_sync_fs(sbi->sb, 1);
 	return 0;
-out_undo:
-	/*
-	 * Still we may fail to recover name info of f2fs_inode here
-	 * Drop it, once its name is set as encrypted
-	 */
-	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
-out_unlock:
-	f2fs_unlock_op(sbi);
 out_new_dir:
 	if (new_dir_entry) {
 		f2fs_dentry_kunmap(new_inode, new_dir_page);
-- 
2.9.3


------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

Re: [PATCH v3] f2fs: cleanup the disk level filename updating

[Jaegeuk Kim]

On 03/10, Kinglong Mee wrote:
> As discuss with Jaegeuk and Chao,
> "Once checkpoint is done, f2fs doesn't need to update there-in filename at all."
> 
> The disk-level filename is used only one case,
> 1. create a file A under a dir
> 2. sync A
> 3. godown
> 4. umount
> 5. mount (roll_forward)
> 
> Only the rename/cross_rename changes the filename, if it happens,
> a. between step 1 and 2, the sync A will caused checkpoint, so that,
>    the roll_forward at step 5 never happens.
> b. after step 2, the roll_forward happens, file A will roll forward
>    to the result as after step 1.

I've checked the roll-forward recovery again and found, if pino is recovered,
it tries to recover dentry with the written filename.

So,
1. create a
2. rename a b
3. fsync b, will trigger checkpoint and recover pino
4. fsync b
5. godown

Then, roll-forward recovery will do recover_dentry with "a". So, correct pino
should have valid filename.

Thoughts?

Thanks,

> 
> So that, any updating the disk filename is useless, just cleanup it.
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/f2fs/dir.c    | 25 ++++---------------------
>  fs/f2fs/f2fs.h   |  2 --
>  fs/f2fs/file.c   |  8 --------
>  fs/f2fs/inline.c |  2 --
>  fs/f2fs/namei.c  | 29 -----------------------------
>  5 files changed, 4 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 8d5c62b..058c4f3 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -337,24 +337,6 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>  	set_page_dirty(ipage);
>  }
>  
> -int update_dent_inode(struct inode *inode, struct inode *to,
> -					const struct qstr *name)
> -{
> -	struct page *page;
> -
> -	if (file_enc_name(to))
> -		return 0;
> -
> -	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
> -	if (IS_ERR(page))
> -		return PTR_ERR(page);
> -
> -	init_dent_inode(name, page);
> -	f2fs_put_page(page, 1);
> -
> -	return 0;
> -}
> -
>  void do_make_empty_dir(struct inode *inode, struct inode *parent,
>  					struct f2fs_dentry_ptr *d)
>  {
> @@ -438,8 +420,11 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>  		set_cold_node(inode, page);
>  	}
>  
> -	if (new_name)
> +	if (new_name) {
>  		init_dent_inode(new_name, page);
> +		if (f2fs_encrypted_inode(dir))
> +			file_set_enc_name(inode);
> +	}
>  
>  	/*
>  	 * This file should be checkpointed during fsync.
> @@ -599,8 +584,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>  			err = PTR_ERR(page);
>  			goto fail;
>  		}
> -		if (f2fs_encrypted_inode(dir))
> -			file_set_enc_name(inode);
>  	}
>  
>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 7edb3be..5dbc0c0 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2093,8 +2093,6 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr,
>  			struct page **page);
>  void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>  			struct page *page, struct inode *inode);
> -int update_dent_inode(struct inode *inode, struct inode *to,
> -			const struct qstr *name);
>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>  			const struct qstr *name, f2fs_hash_t name_hash,
>  			unsigned int bit_pos);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 4ec764e..8c7923f 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -110,20 +110,12 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>  {
>  	struct dentry *dentry;
>  
> -	if (file_enc_name(inode))
> -		return 0;
> -
>  	inode = igrab(inode);
>  	dentry = d_find_any_alias(inode);
>  	iput(inode);
>  	if (!dentry)
>  		return 0;
>  
> -	if (update_dent_inode(inode, inode, &dentry->d_name)) {
> -		dput(dentry);
> -		return 0;
> -	}
> -
>  	*pino = parent_ino(dentry);
>  	dput(dentry);
>  	return 1;
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index e32a9e5..41fe27d 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>  			err = PTR_ERR(page);
>  			goto fail;
>  		}
> -		if (f2fs_encrypted_inode(dir))
> -			file_set_enc_name(inode);
>  	}
>  
>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 25c073f..8906c9f 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -720,13 +720,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		if (err)
>  			goto put_out_dir;
>  
> -		err = update_dent_inode(old_inode, new_inode,
> -						&new_dentry->d_name);
> -		if (err) {
> -			release_orphan_inode(sbi);
> -			goto put_out_dir;
> -		}
> -
>  		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
>  
>  		new_inode->i_ctime = current_time(new_inode);
> @@ -779,8 +772,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	down_write(&F2FS_I(old_inode)->i_sem);
>  	file_lost_pino(old_inode);
> -	if (new_inode && file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
>  	up_write(&F2FS_I(old_inode)->i_sem);
>  
>  	old_inode->i_ctime = current_time(old_inode);
> @@ -917,18 +908,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	f2fs_lock_op(sbi);
>  
> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
> -	if (err)
> -		goto out_unlock;
> -	if (file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
> -
> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
> -	if (err)
> -		goto out_undo;
> -	if (file_enc_name(old_inode))
> -		file_set_enc_name(new_inode);
> -
>  	/* update ".." directory entry info of old dentry */
>  	if (old_dir_entry)
>  		f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
> @@ -972,14 +951,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>  		f2fs_sync_fs(sbi->sb, 1);
>  	return 0;
> -out_undo:
> -	/*
> -	 * Still we may fail to recover name info of f2fs_inode here
> -	 * Drop it, once its name is set as encrypted
> -	 */
> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
> -out_unlock:
> -	f2fs_unlock_op(sbi);
>  out_new_dir:
>  	if (new_dir_entry) {
>  		f2fs_dentry_kunmap(new_inode, new_dir_page);
> -- 
> 2.9.3

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v3] f2fs: cleanup the disk level filename updating

[Kinglong Mee]

On 3/14/2017 02:23, Jaegeuk Kim wrote:
> On 03/10, Kinglong Mee wrote:
>> As discuss with Jaegeuk and Chao,
>> "Once checkpoint is done, f2fs doesn't need to update there-in filename at all."
>>
>> The disk-level filename is used only one case,
>> 1. create a file A under a dir
>> 2. sync A
>> 3. godown
>> 4. umount
>> 5. mount (roll_forward)
>>
>> Only the rename/cross_rename changes the filename, if it happens,
>> a. between step 1 and 2, the sync A will caused checkpoint, so that,
>>    the roll_forward at step 5 never happens.
>> b. after step 2, the roll_forward happens, file A will roll forward
>>    to the result as after step 1.
> 
> I've checked the roll-forward recovery again and found, if pino is recovered,
> it tries to recover dentry with the written filename.
> 
> So,
> 1. create a
> 2. rename a b
> 3. fsync b, will trigger checkpoint and recover pino
> 4. fsync b
> 5. godown
> 
> Then, roll-forward recovery will do recover_dentry with "a". So, correct pino
> should have valid filename.

Is it the right operation? but the f2fs code doesn't do like that, see below.

> 
> Thoughts?

If b isn't exist when renaming, f2fs creates a new directory entry
(f2fs_add_link with name "b"), but no new inode or nid created.

The recover_dentry depends on FSYNC_BIT_SHIFT and DENT_BIT_SHIFT, as your procedures,
a roll-forward recovery will do, but no recover_dentry happens.

[ 3607.068713] do_read_inode: ino 3, name (0:0)
[ 3607.090464] do_read_inode: ino 56398, name (0:1) b
[ 3607.110743] F2FS-fs (sdb1): recover_inode: ino = dc4e, name = b
[ 3607.111802] F2FS-fs (sdb1): recover_data: ino = dc4e (i_size: recover) recovered = 0, err = 0
[ 3607.117374] F2FS-fs (sdb1): checkpoint: version = 31d5d90f
[ 3607.118384] F2FS-fs (sdb1): Mounted with checkpoint version = 31d5d90f
[ 3607.288552] NFSD: starting 90-second grace period (net ffffffffbeefd140)

After the first fsync at step 3, the IS_CHECKPOINTED is set, after that,
IS_CHECKPOINTED will exist in the nat entry forever, so DENT_BIT_SHIFT never be set.

1397 int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
1398                         struct writeback_control *wbc, bool atomic)

1460                         if (!atomic || page == last_page) {
1461                                 set_fsync_mark(page, 1);
1462                                 if (IS_INODE(page)) {
1463                                         if (is_inode_flag_set(inode,
1464                                                                 FI_DIRTY_INODE))
1465                                                 update_inode(inode, page);
1466                                         set_dentry_mark(page,
1467                                                 need_dentry_mark(sbi, ino));
1468                                 }
1469                                 /*  may be written by other thread */
1470                                 if (!PageDirty(page))
1471                                         set_page_dirty(page);
1472                         }

 195 int need_dentry_mark(struct f2fs_sb_info *sbi, nid_t nid)
 196 {
 197         struct f2fs_nm_info *nm_i = NM_I(sbi);
 198         struct nat_entry *e;
 199         bool need = false;
 200
 201         down_read(&nm_i->nat_tree_lock);
 202         e = __lookup_nat_cache(nm_i, nid);
 203         if (e) {
 204                 if (!get_nat_flag(e, IS_CHECKPOINTED) &&
 205                                 !get_nat_flag(e, HAS_FSYNCED_INODE))
 206                         need = true;
 207         }
 208         up_read(&nm_i->nat_tree_lock);
 209         return need;
 210 }

thanks,
Kinglong Mee

> 
> Thanks,
> 
>>
>> So that, any updating the disk filename is useless, just cleanup it.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/f2fs/dir.c    | 25 ++++---------------------
>>  fs/f2fs/f2fs.h   |  2 --
>>  fs/f2fs/file.c   |  8 --------
>>  fs/f2fs/inline.c |  2 --
>>  fs/f2fs/namei.c  | 29 -----------------------------
>>  5 files changed, 4 insertions(+), 62 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index 8d5c62b..058c4f3 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -337,24 +337,6 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>>  	set_page_dirty(ipage);
>>  }
>>  
>> -int update_dent_inode(struct inode *inode, struct inode *to,
>> -					const struct qstr *name)
>> -{
>> -	struct page *page;
>> -
>> -	if (file_enc_name(to))
>> -		return 0;
>> -
>> -	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
>> -	if (IS_ERR(page))
>> -		return PTR_ERR(page);
>> -
>> -	init_dent_inode(name, page);
>> -	f2fs_put_page(page, 1);
>> -
>> -	return 0;
>> -}
>> -
>>  void do_make_empty_dir(struct inode *inode, struct inode *parent,
>>  					struct f2fs_dentry_ptr *d)
>>  {
>> @@ -438,8 +420,11 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>>  		set_cold_node(inode, page);
>>  	}
>>  
>> -	if (new_name)
>> +	if (new_name) {
>>  		init_dent_inode(new_name, page);
>> +		if (f2fs_encrypted_inode(dir))
>> +			file_set_enc_name(inode);
>> +	}
>>  
>>  	/*
>>  	 * This file should be checkpointed during fsync.
>> @@ -599,8 +584,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>>  			err = PTR_ERR(page);
>>  			goto fail;
>>  		}
>> -		if (f2fs_encrypted_inode(dir))
>> -			file_set_enc_name(inode);
>>  	}
>>  
>>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 7edb3be..5dbc0c0 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -2093,8 +2093,6 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr,
>>  			struct page **page);
>>  void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>>  			struct page *page, struct inode *inode);
>> -int update_dent_inode(struct inode *inode, struct inode *to,
>> -			const struct qstr *name);
>>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>>  			const struct qstr *name, f2fs_hash_t name_hash,
>>  			unsigned int bit_pos);
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 4ec764e..8c7923f 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -110,20 +110,12 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>>  {
>>  	struct dentry *dentry;
>>  
>> -	if (file_enc_name(inode))
>> -		return 0;
>> -
>>  	inode = igrab(inode);
>>  	dentry = d_find_any_alias(inode);
>>  	iput(inode);
>>  	if (!dentry)
>>  		return 0;
>>  
>> -	if (update_dent_inode(inode, inode, &dentry->d_name)) {
>> -		dput(dentry);
>> -		return 0;
>> -	}
>> -
>>  	*pino = parent_ino(dentry);
>>  	dput(dentry);
>>  	return 1;
>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>> index e32a9e5..41fe27d 100644
>> --- a/fs/f2fs/inline.c
>> +++ b/fs/f2fs/inline.c
>> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>>  			err = PTR_ERR(page);
>>  			goto fail;
>>  		}
>> -		if (f2fs_encrypted_inode(dir))
>> -			file_set_enc_name(inode);
>>  	}
>>  
>>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index 25c073f..8906c9f 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -720,13 +720,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  		if (err)
>>  			goto put_out_dir;
>>  
>> -		err = update_dent_inode(old_inode, new_inode,
>> -						&new_dentry->d_name);
>> -		if (err) {
>> -			release_orphan_inode(sbi);
>> -			goto put_out_dir;
>> -		}
>> -
>>  		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
>>  
>>  		new_inode->i_ctime = current_time(new_inode);
>> @@ -779,8 +772,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	down_write(&F2FS_I(old_inode)->i_sem);
>>  	file_lost_pino(old_inode);
>> -	if (new_inode && file_enc_name(new_inode))
>> -		file_set_enc_name(old_inode);
>>  	up_write(&F2FS_I(old_inode)->i_sem);
>>  
>>  	old_inode->i_ctime = current_time(old_inode);
>> @@ -917,18 +908,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	f2fs_lock_op(sbi);
>>  
>> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
>> -	if (err)
>> -		goto out_unlock;
>> -	if (file_enc_name(new_inode))
>> -		file_set_enc_name(old_inode);
>> -
>> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
>> -	if (err)
>> -		goto out_undo;
>> -	if (file_enc_name(old_inode))
>> -		file_set_enc_name(new_inode);
>> -
>>  	/* update ".." directory entry info of old dentry */
>>  	if (old_dir_entry)
>>  		f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
>> @@ -972,14 +951,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>>  		f2fs_sync_fs(sbi->sb, 1);
>>  	return 0;
>> -out_undo:
>> -	/*
>> -	 * Still we may fail to recover name info of f2fs_inode here
>> -	 * Drop it, once its name is set as encrypted
>> -	 */
>> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
>> -out_unlock:
>> -	f2fs_unlock_op(sbi);
>>  out_new_dir:
>>  	if (new_dir_entry) {
>>  		f2fs_dentry_kunmap(new_inode, new_dir_page);
>> -- 
>> 2.9.3
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v3] f2fs: cleanup the disk level filename updating

[Jaegeuk Kim]

On 03/14, Kinglong Mee wrote:
> On 3/14/2017 02:23, Jaegeuk Kim wrote:
> > On 03/10, Kinglong Mee wrote:
> >> As discuss with Jaegeuk and Chao,
> >> "Once checkpoint is done, f2fs doesn't need to update there-in filename at all."
> >>
> >> The disk-level filename is used only one case,
> >> 1. create a file A under a dir
> >> 2. sync A
> >> 3. godown
> >> 4. umount
> >> 5. mount (roll_forward)
> >>
> >> Only the rename/cross_rename changes the filename, if it happens,
> >> a. between step 1 and 2, the sync A will caused checkpoint, so that,
> >>    the roll_forward at step 5 never happens.
> >> b. after step 2, the roll_forward happens, file A will roll forward
> >>    to the result as after step 1.
> > 
> > I've checked the roll-forward recovery again and found, if pino is recovered,
> > it tries to recover dentry with the written filename.
> > 
> > So,
> > 1. create a
> > 2. rename a b
> > 3. fsync b, will trigger checkpoint and recover pino
> > 4. fsync b
> > 5. godown
> > 
> > Then, roll-forward recovery will do recover_dentry with "a". So, correct pino
> > should have valid filename.
> 
> Is it the right operation? but the f2fs code doesn't do like that, see below.
> 
> > 
> > Thoughts?
> 
> If b isn't exist when renaming, f2fs creates a new directory entry
> (f2fs_add_link with name "b"), but no new inode or nid created.
> 
> The recover_dentry depends on FSYNC_BIT_SHIFT and DENT_BIT_SHIFT, as your procedures,
> a roll-forward recovery will do, but no recover_dentry happens.

Yeah, right. We already did checkpoint. ;)
So, it means correct pino allows us to do roll-forward recovery in fsync.
And, we lose it by link/rename and only recover it after checkpoint in fsync.

Thanks,

> 
> [ 3607.068713] do_read_inode: ino 3, name (0:0)
> [ 3607.090464] do_read_inode: ino 56398, name (0:1) b
> [ 3607.110743] F2FS-fs (sdb1): recover_inode: ino = dc4e, name = b
> [ 3607.111802] F2FS-fs (sdb1): recover_data: ino = dc4e (i_size: recover) recovered = 0, err = 0
> [ 3607.117374] F2FS-fs (sdb1): checkpoint: version = 31d5d90f
> [ 3607.118384] F2FS-fs (sdb1): Mounted with checkpoint version = 31d5d90f
> [ 3607.288552] NFSD: starting 90-second grace period (net ffffffffbeefd140)
> 
> After the first fsync at step 3, the IS_CHECKPOINTED is set, after that,
> IS_CHECKPOINTED will exist in the nat entry forever, so DENT_BIT_SHIFT never be set.
> 
> 1397 int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> 1398                         struct writeback_control *wbc, bool atomic)
> 
> 1460                         if (!atomic || page == last_page) {
> 1461                                 set_fsync_mark(page, 1);
> 1462                                 if (IS_INODE(page)) {
> 1463                                         if (is_inode_flag_set(inode,
> 1464                                                                 FI_DIRTY_INODE))
> 1465                                                 update_inode(inode, page);
> 1466                                         set_dentry_mark(page,
> 1467                                                 need_dentry_mark(sbi, ino));
> 1468                                 }
> 1469                                 /*  may be written by other thread */
> 1470                                 if (!PageDirty(page))
> 1471                                         set_page_dirty(page);
> 1472                         }
> 
>  195 int need_dentry_mark(struct f2fs_sb_info *sbi, nid_t nid)
>  196 {
>  197         struct f2fs_nm_info *nm_i = NM_I(sbi);
>  198         struct nat_entry *e;
>  199         bool need = false;
>  200
>  201         down_read(&nm_i->nat_tree_lock);
>  202         e = __lookup_nat_cache(nm_i, nid);
>  203         if (e) {
>  204                 if (!get_nat_flag(e, IS_CHECKPOINTED) &&
>  205                                 !get_nat_flag(e, HAS_FSYNCED_INODE))
>  206                         need = true;
>  207         }
>  208         up_read(&nm_i->nat_tree_lock);
>  209         return need;
>  210 }
> 
> thanks,
> Kinglong Mee
> 
> > 
> > Thanks,
> > 
> >>
> >> So that, any updating the disk filename is useless, just cleanup it.
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >> ---
> >>  fs/f2fs/dir.c    | 25 ++++---------------------
> >>  fs/f2fs/f2fs.h   |  2 --
> >>  fs/f2fs/file.c   |  8 --------
> >>  fs/f2fs/inline.c |  2 --
> >>  fs/f2fs/namei.c  | 29 -----------------------------
> >>  5 files changed, 4 insertions(+), 62 deletions(-)
> >>
> >> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> >> index 8d5c62b..058c4f3 100644
> >> --- a/fs/f2fs/dir.c
> >> +++ b/fs/f2fs/dir.c
> >> @@ -337,24 +337,6 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
> >>  	set_page_dirty(ipage);
> >>  }
> >>  
> >> -int update_dent_inode(struct inode *inode, struct inode *to,
> >> -					const struct qstr *name)
> >> -{
> >> -	struct page *page;
> >> -
> >> -	if (file_enc_name(to))
> >> -		return 0;
> >> -
> >> -	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
> >> -	if (IS_ERR(page))
> >> -		return PTR_ERR(page);
> >> -
> >> -	init_dent_inode(name, page);
> >> -	f2fs_put_page(page, 1);
> >> -
> >> -	return 0;
> >> -}
> >> -
> >>  void do_make_empty_dir(struct inode *inode, struct inode *parent,
> >>  					struct f2fs_dentry_ptr *d)
> >>  {
> >> @@ -438,8 +420,11 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
> >>  		set_cold_node(inode, page);
> >>  	}
> >>  
> >> -	if (new_name)
> >> +	if (new_name) {
> >>  		init_dent_inode(new_name, page);
> >> +		if (f2fs_encrypted_inode(dir))
> >> +			file_set_enc_name(inode);
> >> +	}
> >>  
> >>  	/*
> >>  	 * This file should be checkpointed during fsync.
> >> @@ -599,8 +584,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
> >>  			err = PTR_ERR(page);
> >>  			goto fail;
> >>  		}
> >> -		if (f2fs_encrypted_inode(dir))
> >> -			file_set_enc_name(inode);
> >>  	}
> >>  
> >>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> index 7edb3be..5dbc0c0 100644
> >> --- a/fs/f2fs/f2fs.h
> >> +++ b/fs/f2fs/f2fs.h
> >> @@ -2093,8 +2093,6 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr,
> >>  			struct page **page);
> >>  void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
> >>  			struct page *page, struct inode *inode);
> >> -int update_dent_inode(struct inode *inode, struct inode *to,
> >> -			const struct qstr *name);
> >>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
> >>  			const struct qstr *name, f2fs_hash_t name_hash,
> >>  			unsigned int bit_pos);
> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >> index 4ec764e..8c7923f 100644
> >> --- a/fs/f2fs/file.c
> >> +++ b/fs/f2fs/file.c
> >> @@ -110,20 +110,12 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
> >>  {
> >>  	struct dentry *dentry;
> >>  
> >> -	if (file_enc_name(inode))
> >> -		return 0;
> >> -
> >>  	inode = igrab(inode);
> >>  	dentry = d_find_any_alias(inode);
> >>  	iput(inode);
> >>  	if (!dentry)
> >>  		return 0;
> >>  
> >> -	if (update_dent_inode(inode, inode, &dentry->d_name)) {
> >> -		dput(dentry);
> >> -		return 0;
> >> -	}
> >> -
> >>  	*pino = parent_ino(dentry);
> >>  	dput(dentry);
> >>  	return 1;
> >> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> >> index e32a9e5..41fe27d 100644
> >> --- a/fs/f2fs/inline.c
> >> +++ b/fs/f2fs/inline.c
> >> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
> >>  			err = PTR_ERR(page);
> >>  			goto fail;
> >>  		}
> >> -		if (f2fs_encrypted_inode(dir))
> >> -			file_set_enc_name(inode);
> >>  	}
> >>  
> >>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
> >> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> >> index 25c073f..8906c9f 100644
> >> --- a/fs/f2fs/namei.c
> >> +++ b/fs/f2fs/namei.c
> >> @@ -720,13 +720,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>  		if (err)
> >>  			goto put_out_dir;
> >>  
> >> -		err = update_dent_inode(old_inode, new_inode,
> >> -						&new_dentry->d_name);
> >> -		if (err) {
> >> -			release_orphan_inode(sbi);
> >> -			goto put_out_dir;
> >> -		}
> >> -
> >>  		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
> >>  
> >>  		new_inode->i_ctime = current_time(new_inode);
> >> @@ -779,8 +772,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>  
> >>  	down_write(&F2FS_I(old_inode)->i_sem);
> >>  	file_lost_pino(old_inode);
> >> -	if (new_inode && file_enc_name(new_inode))
> >> -		file_set_enc_name(old_inode);
> >>  	up_write(&F2FS_I(old_inode)->i_sem);
> >>  
> >>  	old_inode->i_ctime = current_time(old_inode);
> >> @@ -917,18 +908,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>  
> >>  	f2fs_lock_op(sbi);
> >>  
> >> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
> >> -	if (err)
> >> -		goto out_unlock;
> >> -	if (file_enc_name(new_inode))
> >> -		file_set_enc_name(old_inode);
> >> -
> >> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
> >> -	if (err)
> >> -		goto out_undo;
> >> -	if (file_enc_name(old_inode))
> >> -		file_set_enc_name(new_inode);
> >> -
> >>  	/* update ".." directory entry info of old dentry */
> >>  	if (old_dir_entry)
> >>  		f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
> >> @@ -972,14 +951,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
> >>  		f2fs_sync_fs(sbi->sb, 1);
> >>  	return 0;
> >> -out_undo:
> >> -	/*
> >> -	 * Still we may fail to recover name info of f2fs_inode here
> >> -	 * Drop it, once its name is set as encrypted
> >> -	 */
> >> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
> >> -out_unlock:
> >> -	f2fs_unlock_op(sbi);
> >>  out_new_dir:
> >>  	if (new_dir_entry) {
> >>  		f2fs_dentry_kunmap(new_inode, new_dir_page);
> >> -- 
> >> 2.9.3
> > 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v3] f2fs: cleanup the disk level filename updating

[Chao Yu]

On 2017/3/10 16:28, Kinglong Mee wrote:
> As discuss with Jaegeuk and Chao,
> "Once checkpoint is done, f2fs doesn't need to update there-in filename at all."
> 
> The disk-level filename is used only one case,
> 1. create a file A under a dir
> 2. sync A
> 3. godown
> 4. umount
> 5. mount (roll_forward)
> 
> Only the rename/cross_rename changes the filename, if it happens,
> a. between step 1 and 2, the sync A will caused checkpoint, so that,
>    the roll_forward at step 5 never happens.
> b. after step 2, the roll_forward happens, file A will roll forward
>    to the result as after step 1.
> 
> So that, any updating the disk filename is useless, just cleanup it.
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/f2fs/dir.c    | 25 ++++---------------------
>  fs/f2fs/f2fs.h   |  2 --
>  fs/f2fs/file.c   |  8 --------
>  fs/f2fs/inline.c |  2 --
>  fs/f2fs/namei.c  | 29 -----------------------------
>  5 files changed, 4 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 8d5c62b..058c4f3 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -337,24 +337,6 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>  	set_page_dirty(ipage);
>  }
>  
> -int update_dent_inode(struct inode *inode, struct inode *to,
> -					const struct qstr *name)
> -{
> -	struct page *page;
> -
> -	if (file_enc_name(to))
> -		return 0;
> -
> -	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
> -	if (IS_ERR(page))
> -		return PTR_ERR(page);
> -
> -	init_dent_inode(name, page);
> -	f2fs_put_page(page, 1);
> -
> -	return 0;
> -}
> -
>  void do_make_empty_dir(struct inode *inode, struct inode *parent,
>  					struct f2fs_dentry_ptr *d)
>  {
> @@ -438,8 +420,11 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>  		set_cold_node(inode, page);
>  	}
>  
> -	if (new_name)
> +	if (new_name) {
>  		init_dent_inode(new_name, page);
> +		if (f2fs_encrypted_inode(dir))
> +			file_set_enc_name(inode);

Please check tmpfile path, we will skip here since new_name will be null, so if
we do not set enc_name flag in add_link, we will lose the flag for tmpfile which
will be linked to dents later.

Other parts look good to me.

Thanks,

> +	}
>  
>  	/*
>  	 * This file should be checkpointed during fsync.
> @@ -599,8 +584,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>  			err = PTR_ERR(page);
>  			goto fail;
>  		}
> -		if (f2fs_encrypted_inode(dir))
> -			file_set_enc_name(inode);
>  	}
>  
>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 7edb3be..5dbc0c0 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2093,8 +2093,6 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr,
>  			struct page **page);
>  void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>  			struct page *page, struct inode *inode);
> -int update_dent_inode(struct inode *inode, struct inode *to,
> -			const struct qstr *name);
>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>  			const struct qstr *name, f2fs_hash_t name_hash,
>  			unsigned int bit_pos);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 4ec764e..8c7923f 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -110,20 +110,12 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>  {
>  	struct dentry *dentry;
>  
> -	if (file_enc_name(inode))
> -		return 0;
> -
>  	inode = igrab(inode);
>  	dentry = d_find_any_alias(inode);
>  	iput(inode);
>  	if (!dentry)
>  		return 0;
>  
> -	if (update_dent_inode(inode, inode, &dentry->d_name)) {
> -		dput(dentry);
> -		return 0;
> -	}
> -
>  	*pino = parent_ino(dentry);
>  	dput(dentry);
>  	return 1;
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index e32a9e5..41fe27d 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>  			err = PTR_ERR(page);
>  			goto fail;
>  		}
> -		if (f2fs_encrypted_inode(dir))
> -			file_set_enc_name(inode);
>  	}
>  
>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 25c073f..8906c9f 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -720,13 +720,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		if (err)
>  			goto put_out_dir;
>  
> -		err = update_dent_inode(old_inode, new_inode,
> -						&new_dentry->d_name);
> -		if (err) {
> -			release_orphan_inode(sbi);
> -			goto put_out_dir;
> -		}
> -
>  		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
>  
>  		new_inode->i_ctime = current_time(new_inode);
> @@ -779,8 +772,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	down_write(&F2FS_I(old_inode)->i_sem);
>  	file_lost_pino(old_inode);
> -	if (new_inode && file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
>  	up_write(&F2FS_I(old_inode)->i_sem);
>  
>  	old_inode->i_ctime = current_time(old_inode);
> @@ -917,18 +908,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	f2fs_lock_op(sbi);
>  
> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
> -	if (err)
> -		goto out_unlock;
> -	if (file_enc_name(new_inode))
> -		file_set_enc_name(old_inode);
> -
> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
> -	if (err)
> -		goto out_undo;
> -	if (file_enc_name(old_inode))
> -		file_set_enc_name(new_inode);
> -
>  	/* update ".." directory entry info of old dentry */
>  	if (old_dir_entry)
>  		f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
> @@ -972,14 +951,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>  		f2fs_sync_fs(sbi->sb, 1);
>  	return 0;
> -out_undo:
> -	/*
> -	 * Still we may fail to recover name info of f2fs_inode here
> -	 * Drop it, once its name is set as encrypted
> -	 */
> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
> -out_unlock:
> -	f2fs_unlock_op(sbi);
>  out_new_dir:
>  	if (new_dir_entry) {
>  		f2fs_dentry_kunmap(new_inode, new_dir_page);
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v3] f2fs: cleanup the disk level filename updating

[Kinglong Mee]

On 3/17/2017 15:04, Chao Yu wrote:
> On 2017/3/10 16:28, Kinglong Mee wrote:
>> As discuss with Jaegeuk and Chao,
>> "Once checkpoint is done, f2fs doesn't need to update there-in filename at all."
>>
>> The disk-level filename is used only one case,
>> 1. create a file A under a dir
>> 2. sync A
>> 3. godown
>> 4. umount
>> 5. mount (roll_forward)
>>
>> Only the rename/cross_rename changes the filename, if it happens,
>> a. between step 1 and 2, the sync A will caused checkpoint, so that,
>>    the roll_forward at step 5 never happens.
>> b. after step 2, the roll_forward happens, file A will roll forward
>>    to the result as after step 1.
>>
>> So that, any updating the disk filename is useless, just cleanup it.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/f2fs/dir.c    | 25 ++++---------------------
>>  fs/f2fs/f2fs.h   |  2 --
>>  fs/f2fs/file.c   |  8 --------
>>  fs/f2fs/inline.c |  2 --
>>  fs/f2fs/namei.c  | 29 -----------------------------
>>  5 files changed, 4 insertions(+), 62 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index 8d5c62b..058c4f3 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -337,24 +337,6 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>>  	set_page_dirty(ipage);
>>  }
>>  
>> -int update_dent_inode(struct inode *inode, struct inode *to,
>> -					const struct qstr *name)
>> -{
>> -	struct page *page;
>> -
>> -	if (file_enc_name(to))
>> -		return 0;
>> -
>> -	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
>> -	if (IS_ERR(page))
>> -		return PTR_ERR(page);
>> -
>> -	init_dent_inode(name, page);
>> -	f2fs_put_page(page, 1);
>> -
>> -	return 0;
>> -}
>> -
>>  void do_make_empty_dir(struct inode *inode, struct inode *parent,
>>  					struct f2fs_dentry_ptr *d)
>>  {
>> @@ -438,8 +420,11 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>>  		set_cold_node(inode, page);
>>  	}
>>  
>> -	if (new_name)
>> +	if (new_name) {
>>  		init_dent_inode(new_name, page);
>> +		if (f2fs_encrypted_inode(dir))
>> +			file_set_enc_name(inode);
> 
> Please check tmpfile path, we will skip here since new_name will be null, so if
> we do not set enc_name flag in add_link, we will lose the flag for tmpfile which
> will be linked to dents later.

f2fs calls init_inode_metadata() without filename initialize in f2fs_do_tmpfile(),
the filename is NULL and without the enc_name flag.

For whiteout, f2fs links it to dents by f2fs_add_link(), and init_inode_metadata()
will be called again with the dentry.name, so it's okay.

Any requirements for the normal tmpfile?

thanks,
Kinglong Mee

> 
> Other parts look good to me.
> 
> Thanks,
> 
>> +	}
>>  
>>  	/*
>>  	 * This file should be checkpointed during fsync.
>> @@ -599,8 +584,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>>  			err = PTR_ERR(page);
>>  			goto fail;
>>  		}
>> -		if (f2fs_encrypted_inode(dir))
>> -			file_set_enc_name(inode);
>>  	}
>>  
>>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 7edb3be..5dbc0c0 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -2093,8 +2093,6 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr,
>>  			struct page **page);
>>  void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>>  			struct page *page, struct inode *inode);
>> -int update_dent_inode(struct inode *inode, struct inode *to,
>> -			const struct qstr *name);
>>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>>  			const struct qstr *name, f2fs_hash_t name_hash,
>>  			unsigned int bit_pos);
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 4ec764e..8c7923f 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -110,20 +110,12 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>>  {
>>  	struct dentry *dentry;
>>  
>> -	if (file_enc_name(inode))
>> -		return 0;
>> -
>>  	inode = igrab(inode);
>>  	dentry = d_find_any_alias(inode);
>>  	iput(inode);
>>  	if (!dentry)
>>  		return 0;
>>  
>> -	if (update_dent_inode(inode, inode, &dentry->d_name)) {
>> -		dput(dentry);
>> -		return 0;
>> -	}
>> -
>>  	*pino = parent_ino(dentry);
>>  	dput(dentry);
>>  	return 1;
>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>> index e32a9e5..41fe27d 100644
>> --- a/fs/f2fs/inline.c
>> +++ b/fs/f2fs/inline.c
>> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>>  			err = PTR_ERR(page);
>>  			goto fail;
>>  		}
>> -		if (f2fs_encrypted_inode(dir))
>> -			file_set_enc_name(inode);
>>  	}
>>  
>>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index 25c073f..8906c9f 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -720,13 +720,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  		if (err)
>>  			goto put_out_dir;
>>  
>> -		err = update_dent_inode(old_inode, new_inode,
>> -						&new_dentry->d_name);
>> -		if (err) {
>> -			release_orphan_inode(sbi);
>> -			goto put_out_dir;
>> -		}
>> -
>>  		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
>>  
>>  		new_inode->i_ctime = current_time(new_inode);
>> @@ -779,8 +772,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	down_write(&F2FS_I(old_inode)->i_sem);
>>  	file_lost_pino(old_inode);
>> -	if (new_inode && file_enc_name(new_inode))
>> -		file_set_enc_name(old_inode);
>>  	up_write(&F2FS_I(old_inode)->i_sem);
>>  
>>  	old_inode->i_ctime = current_time(old_inode);
>> @@ -917,18 +908,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  
>>  	f2fs_lock_op(sbi);
>>  
>> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
>> -	if (err)
>> -		goto out_unlock;
>> -	if (file_enc_name(new_inode))
>> -		file_set_enc_name(old_inode);
>> -
>> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
>> -	if (err)
>> -		goto out_undo;
>> -	if (file_enc_name(old_inode))
>> -		file_set_enc_name(new_inode);
>> -
>>  	/* update ".." directory entry info of old dentry */
>>  	if (old_dir_entry)
>>  		f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
>> @@ -972,14 +951,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>>  		f2fs_sync_fs(sbi->sb, 1);
>>  	return 0;
>> -out_undo:
>> -	/*
>> -	 * Still we may fail to recover name info of f2fs_inode here
>> -	 * Drop it, once its name is set as encrypted
>> -	 */
>> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
>> -out_unlock:
>> -	f2fs_unlock_op(sbi);
>>  out_new_dir:
>>  	if (new_dir_entry) {
>>  		f2fs_dentry_kunmap(new_inode, new_dir_page);
>>
> 
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v3] f2fs: cleanup the disk level filename updating

[Chao Yu]

On 2017/3/17 15:30, Kinglong Mee wrote:
> On 3/17/2017 15:04, Chao Yu wrote:
>> On 2017/3/10 16:28, Kinglong Mee wrote:
>>> As discuss with Jaegeuk and Chao,
>>> "Once checkpoint is done, f2fs doesn't need to update there-in filename at all."
>>>
>>> The disk-level filename is used only one case,
>>> 1. create a file A under a dir
>>> 2. sync A
>>> 3. godown
>>> 4. umount
>>> 5. mount (roll_forward)
>>>
>>> Only the rename/cross_rename changes the filename, if it happens,
>>> a. between step 1 and 2, the sync A will caused checkpoint, so that,
>>>    the roll_forward at step 5 never happens.
>>> b. after step 2, the roll_forward happens, file A will roll forward
>>>    to the result as after step 1.
>>>
>>> So that, any updating the disk filename is useless, just cleanup it.
>>>
>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>> ---
>>>  fs/f2fs/dir.c    | 25 ++++---------------------
>>>  fs/f2fs/f2fs.h   |  2 --
>>>  fs/f2fs/file.c   |  8 --------
>>>  fs/f2fs/inline.c |  2 --
>>>  fs/f2fs/namei.c  | 29 -----------------------------
>>>  5 files changed, 4 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>>> index 8d5c62b..058c4f3 100644
>>> --- a/fs/f2fs/dir.c
>>> +++ b/fs/f2fs/dir.c
>>> @@ -337,24 +337,6 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
>>>  	set_page_dirty(ipage);
>>>  }
>>>  
>>> -int update_dent_inode(struct inode *inode, struct inode *to,
>>> -					const struct qstr *name)
>>> -{
>>> -	struct page *page;
>>> -
>>> -	if (file_enc_name(to))
>>> -		return 0;
>>> -
>>> -	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
>>> -	if (IS_ERR(page))
>>> -		return PTR_ERR(page);
>>> -
>>> -	init_dent_inode(name, page);
>>> -	f2fs_put_page(page, 1);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>  void do_make_empty_dir(struct inode *inode, struct inode *parent,
>>>  					struct f2fs_dentry_ptr *d)
>>>  {
>>> @@ -438,8 +420,11 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
>>>  		set_cold_node(inode, page);
>>>  	}
>>>  
>>> -	if (new_name)
>>> +	if (new_name) {
>>>  		init_dent_inode(new_name, page);
>>> +		if (f2fs_encrypted_inode(dir))
>>> +			file_set_enc_name(inode);
>>
>> Please check tmpfile path, we will skip here since new_name will be null, so if
>> we do not set enc_name flag in add_link, we will lose the flag for tmpfile which
>> will be linked to dents later.
> 
> f2fs calls init_inode_metadata() without filename initialize in f2fs_do_tmpfile(),
> the filename is NULL and without the enc_name flag.

Oh, init_inode_metadata will be called again in f2fs_add_link when link tmpfile
to target directory, so there will be no problem.

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> 
> For whiteout, f2fs links it to dents by f2fs_add_link(), and init_inode_metadata()
> will be called again with the dentry.name, so it's okay.
> 
> Any requirements for the normal tmpfile?
> 
> thanks,
> Kinglong Mee
> 
>>
>> Other parts look good to me.
>>
>> Thanks,
>>
>>> +	}
>>>  
>>>  	/*
>>>  	 * This file should be checkpointed during fsync.
>>> @@ -599,8 +584,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
>>>  			err = PTR_ERR(page);
>>>  			goto fail;
>>>  		}
>>> -		if (f2fs_encrypted_inode(dir))
>>> -			file_set_enc_name(inode);
>>>  	}
>>>  
>>>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 7edb3be..5dbc0c0 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -2093,8 +2093,6 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr,
>>>  			struct page **page);
>>>  void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>>>  			struct page *page, struct inode *inode);
>>> -int update_dent_inode(struct inode *inode, struct inode *to,
>>> -			const struct qstr *name);
>>>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>>>  			const struct qstr *name, f2fs_hash_t name_hash,
>>>  			unsigned int bit_pos);
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 4ec764e..8c7923f 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -110,20 +110,12 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>>>  {
>>>  	struct dentry *dentry;
>>>  
>>> -	if (file_enc_name(inode))
>>> -		return 0;
>>> -
>>>  	inode = igrab(inode);
>>>  	dentry = d_find_any_alias(inode);
>>>  	iput(inode);
>>>  	if (!dentry)
>>>  		return 0;
>>>  
>>> -	if (update_dent_inode(inode, inode, &dentry->d_name)) {
>>> -		dput(dentry);
>>> -		return 0;
>>> -	}
>>> -
>>>  	*pino = parent_ino(dentry);
>>>  	dput(dentry);
>>>  	return 1;
>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>>> index e32a9e5..41fe27d 100644
>>> --- a/fs/f2fs/inline.c
>>> +++ b/fs/f2fs/inline.c
>>> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>>>  			err = PTR_ERR(page);
>>>  			goto fail;
>>>  		}
>>> -		if (f2fs_encrypted_inode(dir))
>>> -			file_set_enc_name(inode);
>>>  	}
>>>  
>>>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
>>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>>> index 25c073f..8906c9f 100644
>>> --- a/fs/f2fs/namei.c
>>> +++ b/fs/f2fs/namei.c
>>> @@ -720,13 +720,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  		if (err)
>>>  			goto put_out_dir;
>>>  
>>> -		err = update_dent_inode(old_inode, new_inode,
>>> -						&new_dentry->d_name);
>>> -		if (err) {
>>> -			release_orphan_inode(sbi);
>>> -			goto put_out_dir;
>>> -		}
>>> -
>>>  		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
>>>  
>>>  		new_inode->i_ctime = current_time(new_inode);
>>> @@ -779,8 +772,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  
>>>  	down_write(&F2FS_I(old_inode)->i_sem);
>>>  	file_lost_pino(old_inode);
>>> -	if (new_inode && file_enc_name(new_inode))
>>> -		file_set_enc_name(old_inode);
>>>  	up_write(&F2FS_I(old_inode)->i_sem);
>>>  
>>>  	old_inode->i_ctime = current_time(old_inode);
>>> @@ -917,18 +908,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  
>>>  	f2fs_lock_op(sbi);
>>>  
>>> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
>>> -	if (err)
>>> -		goto out_unlock;
>>> -	if (file_enc_name(new_inode))
>>> -		file_set_enc_name(old_inode);
>>> -
>>> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
>>> -	if (err)
>>> -		goto out_undo;
>>> -	if (file_enc_name(old_inode))
>>> -		file_set_enc_name(new_inode);
>>> -
>>>  	/* update ".." directory entry info of old dentry */
>>>  	if (old_dir_entry)
>>>  		f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
>>> @@ -972,14 +951,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
>>>  		f2fs_sync_fs(sbi->sb, 1);
>>>  	return 0;
>>> -out_undo:
>>> -	/*
>>> -	 * Still we may fail to recover name info of f2fs_inode here
>>> -	 * Drop it, once its name is set as encrypted
>>> -	 */
>>> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
>>> -out_unlock:
>>> -	f2fs_unlock_op(sbi);
>>>  out_new_dir:
>>>  	if (new_dir_entry) {
>>>  		f2fs_dentry_kunmap(new_inode, new_dir_page);
>>>
>>
>>
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot