f2fs: avoid new_inode's flags overwrite the old_node

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 <[email protected]>

---
 fs/f2fs/namei.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

fs/f2fs/namei.c +6 -4

index 3231a0a..db2ab7f 100644
@@ -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);

From 7efde035d9f930fc63c30b25327e870b021750f3 Mon Sep 17 00:00:00 2001

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 <[email protected]>

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

fs/f2fs/dir.c +85 -6

index 295a223..b1bb0b1 100644
@@ -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);

fs/f2fs/f2fs.h +3 -0

index 7e29249..ce24fe9 100644
@@ -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);

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +2 -17

index 3231a0a..57050ff 100644
@@ -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:

Signature · 1 line

2.9.3

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?

Quoted 1 line

From 7efde035d9f930fc63c30b25327e870b021750f3 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
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 <[email protected]>

---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)

fs/f2fs/file.c +0 -0

index e0b2378f8519..852195b3bcce 100644

Quoted 9 lines · 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?

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.

Quoted 28 lines

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?

Quoted 1 line

From 7efde035d9f930fc63c30b25327e870b021750f3 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
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 <[email protected]>

---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)

fs/f2fs/file.c +0 -0

index e0b2378f8519..852195b3bcce 100644
@@ -110,6 +110,9 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
 {
 	struct dentry *dentry;
 

fs/f2fs/file.c static int get_parent_ino(struct inode *inode, nid_t *pino) · line 118

Quoted 7 lines

fs/f2fs/file.c +3 -0

+	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

Quoted 35 lines · Kinglong Mee

Quoted 9 lines · 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?

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?

Quoted 2 lines

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

Quoted 28 lines

Quoted 28 lines

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?

Quoted 1 line

From 7efde035d9f930fc63c30b25327e870b021750f3 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
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 <[email protected]>

---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)

fs/f2fs/file.c +0 -0

index e0b2378f8519..852195b3bcce 100644
@@ -110,6 +110,9 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
 {
 	struct dentry *dentry;
 

Quoted 7 lines

fs/f2fs/file.c static int get_parent_ino(struct inode *inode, nid_t *pino) · line 118

Quoted 7 lines

fs/f2fs/file.c +3 -0

+	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

Quoted 23 lines

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

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 <[email protected]>

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

fs/f2fs/dir.c +4 -21

index 8d5c62b..058c4f3 100644
@@ -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);

fs/f2fs/f2fs.h +0 -2

index 7edb3be..5dbc0c0 100644
@@ -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);

fs/f2fs/file.c +0 -8

index 4ec764e..8c7923f 100644
@@ -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;

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +0 -29

index 25c073f..8906c9f 100644
@@ -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);

Signature · 1 line

2.9.3

Quoted 10 lines · 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)

Quoted 5 lines

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,

Quoted 169 lines

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

Signed-off-by Kinglong Mee <[email protected]>

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

fs/f2fs/dir.c +4 -21

index 8d5c62b..058c4f3 100644
@@ -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);

fs/f2fs/f2fs.h +0 -2

index 7edb3be..5dbc0c0 100644
@@ -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);

fs/f2fs/file.c +0 -8

index 4ec764e..8c7923f 100644
@@ -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;

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +0 -26

index 25c073f..8906c9f 100644
@@ -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
-	 */

Quoted 8 lines

fs/f2fs/namei.c +0 -3

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

Signature · 1 line

2.9.3

Quoted 27 lines · Jaegeuk Kim

Quoted 10 lines · 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)

Quoted 5 lines

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

Quoted 2 lines

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.

Quoted 1 line

Quoted 1 line

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

Quoted 181 lines

Thanks,

Quoted 177 lines

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

Signed-off-by Kinglong Mee <[email protected]>

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

fs/f2fs/dir.c +4 -21

index 8d5c62b..058c4f3 100644
@@ -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);

fs/f2fs/f2fs.h +0 -2

index 7edb3be..5dbc0c0 100644
@@ -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);

fs/f2fs/file.c +0 -8

index 4ec764e..8c7923f 100644
@@ -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;

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +0 -29

index 25c073f..8906c9f 100644
@@ -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);

Signature · 1 line

2.9.3

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 <[email protected]>

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

fs/f2fs/dir.c +0 -18

index 8d5c62b..058c4f3 100644
@@ -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)
 {

fs/f2fs/dir.c +1 -2

+	}
 
 	/*
 	 * 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);

fs/f2fs/f2fs.h +0 -2

index 7edb3be..5dbc0c0 100644
@@ -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);

fs/f2fs/file.c +0 -8

index 4ec764e..8c7923f 100644
@@ -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;

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +0 -22

index 25c073f..8906c9f 100644
@@ -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:

Quoted 60 lines · 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 <[email protected]>

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

fs/f2fs/dir.c +0 -18

index 8d5c62b..058c4f3 100644
@@ -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);
 	}
 

fs/f2fs/dir.c struct page *init_inode_metadata(struct inode *inode, struct inode *dir, · line 426

Quoted 5 lines

fs/f2fs/dir.c +3 -1

-	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,

Quoted 118 lines

fs/f2fs/dir.c +1 -2

+	}
 
 	/*
 	 * 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);

fs/f2fs/f2fs.h +0 -2

index 7edb3be..5dbc0c0 100644
@@ -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);

fs/f2fs/file.c +0 -8

index 4ec764e..8c7923f 100644
@@ -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;

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +0 -25

index 25c073f..8906c9f 100644
@@ -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

fs/f2fs/namei.c static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry, · line 956

Quoted 8 lines

fs/f2fs/namei.c +0 -4

-	 */
-	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);

Quoted 61 lines · Chao Yu

Quoted 60 lines · 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 <[email protected]>

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

fs/f2fs/dir.c +0 -18

index 8d5c62b..058c4f3 100644
@@ -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);
 	}
 

Quoted 9 lines

fs/f2fs/dir.c struct page *init_inode_metadata(struct inode *inode, struct inode *dir, · line 426

Quoted 5 lines

fs/f2fs/dir.c +3 -1

-	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

Quoted 123 lines

Other parts look good to me.

Thanks,

Quoted 118 lines

fs/f2fs/dir.c +1 -2

+	}
 
 	/*
 	 * 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);

fs/f2fs/f2fs.h +0 -2

index 7edb3be..5dbc0c0 100644
@@ -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);

fs/f2fs/file.c +0 -8

index 4ec764e..8c7923f 100644
@@ -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;

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +0 -25

index 25c073f..8906c9f 100644
@@ -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

Quoted 10 lines

fs/f2fs/namei.c static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry, · line 956

Quoted 8 lines

fs/f2fs/namei.c +0 -4

-	 */
-	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);

.

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 <[email protected]>

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

fs/f2fs/dir.c +85 -6

index 295a223..b1bb0b1 100644
@@ -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);

fs/f2fs/f2fs.h +3 -0

index 7e29249..ce24fe9 100644
@@ -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);

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +2 -11

index 3231a0a..57050ff 100644
@@ -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);

Quoted 7 lines · 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.

Quoted 2 lines

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,

Quoted 205 lines

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 <[email protected]>

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

fs/f2fs/dir.c +85 -6

index 295a223..b1bb0b1 100644
@@ -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);

fs/f2fs/f2fs.h +3 -0

index 7e29249..ce24fe9 100644
@@ -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);

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +2 -11

index 3231a0a..57050ff 100644
@@ -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);
 

Quoted 6 lines

fs/f2fs/namei.c +0 -0

 	/* 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);

fs/f2fs/namei.c static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry, · line 968

Quoted 10 lines

fs/f2fs/namei.c +0 -6

 	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:

Quoted 11 lines · Chao Yu

Quoted 9 lines · 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,

Quoted 4 lines

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

Quoted 208 lines

Thanks,

Quoted 205 lines

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 <[email protected]>

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

fs/f2fs/dir.c +85 -6

index 295a223..b1bb0b1 100644
@@ -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);

fs/f2fs/f2fs.h +3 -0

index 7e29249..ce24fe9 100644
@@ -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);

fs/f2fs/inline.c +0 -2

index e32a9e5..41fe27d 100644
@@ -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);

fs/f2fs/namei.c +2 -11

index 3231a0a..57050ff 100644
@@ -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);
 

Quoted 18 lines

Quoted 6 lines

fs/f2fs/namei.c +0 -0

 	/* 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);

fs/f2fs/namei.c static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry, · line 968

Quoted 10 lines

fs/f2fs/namei.c +0 -6

 	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:

.

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 <[email protected]>

---
 fs/f2fs/namei.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

fs/f2fs/namei.c +6 -4

index 3231a0a..db2ab7f 100644
@@ -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);

Quoted 2 lines · 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).

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,

Quoted 54 lines

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 <[email protected]>

---
 fs/f2fs/namei.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

fs/f2fs/namei.c +6 -4

index 3231a0a..db2ab7f 100644
@@ -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,
 

fs/f2fs/namei.c static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry, · line 966

Quoted 8 lines

fs/f2fs/namei.c +2 -0

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