[PATCH] f2fs: don't leave the hashed inode while it's unlinked

[PATCH] f2fs: don't leave the hashed inode while it's unlinked

[Jaegeuk Kim]

f2fs_symlink()
1. f2fs_new_inode
2. f2fs_add_link
3. page_symlink
4. flush dirty pages and or checkpoint

Step 4 is nice to succeed, which doesn't become a reason to roll back
the created symlink. OTOH, if we get an error till step 3, let's roll
back and remove the cached inode.

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

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 37897f4321c0..7b318cee2ed6 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -704,14 +704,16 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	f2fs_alloc_nid_done(sbi, inode->i_ino);
 
 	err = fscrypt_encrypt_symlink(inode, symname, len, &disk_link);
-	if (err)
-		goto err_out;
-
-	err = page_symlink(inode, disk_link.name, disk_link.len);
+	if (!err)
+		err = page_symlink(inode, disk_link.name, disk_link.len);
 
-err_out:
 	d_instantiate_new(dentry, inode);
 
+	if (err) {
+		f2fs_unlink(dir, dentry);
+		goto out_f2fs_handle_failed_inode;
+	}
+
 	/*
 	 * Let's flush symlink data in order to avoid broken symlink as much as
 	 * possible. Nevertheless, fsyncing is the best way, but there is no
@@ -721,16 +723,10 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	 * If the symlink path is stored into inline_data, there is no
 	 * performance regression.
 	 */
-	if (!err) {
-		err = filemap_write_and_wait_range(inode->i_mapping, 0,
-						   disk_link.len - 1);
-
-		if (!err && IS_DIRSYNC(dir))
-			err = f2fs_sync_fs(sbi->sb, 1);
-	}
-
-	if (err)
-		f2fs_unlink(dir, dentry);
+	err = filemap_write_and_wait_range(inode->i_mapping, 0,
+					   disk_link.len - 1);
+	if (!err && IS_DIRSYNC(dir))
+		f2fs_sync_fs(sbi->sb, 1);
 
 	f2fs_balance_fs(sbi, true);
 	goto out_free_encrypted_link;
-- 
2.55.0.737.g08866a6d13-goog

[f2fs-dev] [PATCH] f2fs: don't leave the hashed inode while it's unlinked

[Jaegeuk Kim via Linux-f2fs-devel]

f2fs_symlink()
1. f2fs_new_inode
2. f2fs_add_link
3. page_symlink
4. flush dirty pages and or checkpoint

Step 4 is nice to succeed, which doesn't become a reason to roll back
the created symlink. OTOH, if we get an error till step 3, let's roll
back and remove the cached inode.

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

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 37897f4321c0..7b318cee2ed6 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -704,14 +704,16 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	f2fs_alloc_nid_done(sbi, inode->i_ino);
 
 	err = fscrypt_encrypt_symlink(inode, symname, len, &disk_link);
-	if (err)
-		goto err_out;
-
-	err = page_symlink(inode, disk_link.name, disk_link.len);
+	if (!err)
+		err = page_symlink(inode, disk_link.name, disk_link.len);
 
-err_out:
 	d_instantiate_new(dentry, inode);
 
+	if (err) {
+		f2fs_unlink(dir, dentry);
+		goto out_f2fs_handle_failed_inode;
+	}
+
 	/*
 	 * Let's flush symlink data in order to avoid broken symlink as much as
 	 * possible. Nevertheless, fsyncing is the best way, but there is no
@@ -721,16 +723,10 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	 * If the symlink path is stored into inline_data, there is no
 	 * performance regression.
 	 */
-	if (!err) {
-		err = filemap_write_and_wait_range(inode->i_mapping, 0,
-						   disk_link.len - 1);
-
-		if (!err && IS_DIRSYNC(dir))
-			err = f2fs_sync_fs(sbi->sb, 1);
-	}
-
-	if (err)
-		f2fs_unlink(dir, dentry);
+	err = filemap_write_and_wait_range(inode->i_mapping, 0,
+					   disk_link.len - 1);
+	if (!err && IS_DIRSYNC(dir))
+		f2fs_sync_fs(sbi->sb, 1);
 
 	f2fs_balance_fs(sbi, true);
 	goto out_free_encrypted_link;
-- 
2.55.0.737.g08866a6d13-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: don't leave the hashed inode while it's unlinked

[Chao Yu via Linux-f2fs-devel]

On 8/19/26 04:01, Jaegeuk Kim via Linux-f2fs-devel wrote:
> f2fs_symlink()
> 1. f2fs_new_inode
> 2. f2fs_add_link
> 3. page_symlink
> 4. flush dirty pages and or checkpoint
> 
> Step 4 is nice to succeed, which doesn't become a reason to roll back
> the created symlink. OTOH, if we get an error till step 3, let's roll
> back and remove the cached inode.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/namei.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 37897f4321c0..7b318cee2ed6 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -704,14 +704,16 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	f2fs_alloc_nid_done(sbi, inode->i_ino);
>  
>  	err = fscrypt_encrypt_symlink(inode, symname, len, &disk_link);
> -	if (err)
> -		goto err_out;
> -
> -	err = page_symlink(inode, disk_link.name, disk_link.len);
> +	if (!err)
> +		err = page_symlink(inode, disk_link.name, disk_link.len);
>  
> -err_out:
>  	d_instantiate_new(dentry, inode);

IIUC, Al means we'd better not call d_instantiate_new() before every thing is
done? IOW, do not call d_instantiate_new() if we're in error handling.

Let me know if I missed something.

Thanks,

>  
> +	if (err) {
> +		f2fs_unlink(dir, dentry);
> +		goto out_f2fs_handle_failed_inode;
> +	}
> +
>  	/*
>  	 * Let's flush symlink data in order to avoid broken symlink as much as
>  	 * possible. Nevertheless, fsyncing is the best way, but there is no
> @@ -721,16 +723,10 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	 * If the symlink path is stored into inline_data, there is no
>  	 * performance regression.
>  	 */
> -	if (!err) {
> -		err = filemap_write_and_wait_range(inode->i_mapping, 0,
> -						   disk_link.len - 1);
> -
> -		if (!err && IS_DIRSYNC(dir))
> -			err = f2fs_sync_fs(sbi->sb, 1);
> -	}
> -
> -	if (err)
> -		f2fs_unlink(dir, dentry);
> +	err = filemap_write_and_wait_range(inode->i_mapping, 0,
> +					   disk_link.len - 1);
> +	if (!err && IS_DIRSYNC(dir))
> +		f2fs_sync_fs(sbi->sb, 1);
>  
>  	f2fs_balance_fs(sbi, true);
>  	goto out_free_encrypted_link;



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: don't leave the hashed inode while it's unlinked

[Chao Yu]

On 8/19/26 04:01, Jaegeuk Kim via Linux-f2fs-devel wrote:
> f2fs_symlink()
> 1. f2fs_new_inode
> 2. f2fs_add_link
> 3. page_symlink
> 4. flush dirty pages and or checkpoint
> 
> Step 4 is nice to succeed, which doesn't become a reason to roll back
> the created symlink. OTOH, if we get an error till step 3, let's roll
> back and remove the cached inode.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/namei.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 37897f4321c0..7b318cee2ed6 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -704,14 +704,16 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	f2fs_alloc_nid_done(sbi, inode->i_ino);
>  
>  	err = fscrypt_encrypt_symlink(inode, symname, len, &disk_link);
> -	if (err)
> -		goto err_out;
> -
> -	err = page_symlink(inode, disk_link.name, disk_link.len);
> +	if (!err)
> +		err = page_symlink(inode, disk_link.name, disk_link.len);
>  
> -err_out:
>  	d_instantiate_new(dentry, inode);

IIUC, Al means we'd better not call d_instantiate_new() before every thing is
done? IOW, do not call d_instantiate_new() if we're in error handling.

Let me know if I missed something.

Thanks,

>  
> +	if (err) {
> +		f2fs_unlink(dir, dentry);
> +		goto out_f2fs_handle_failed_inode;
> +	}
> +
>  	/*
>  	 * Let's flush symlink data in order to avoid broken symlink as much as
>  	 * possible. Nevertheless, fsyncing is the best way, but there is no
> @@ -721,16 +723,10 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	 * If the symlink path is stored into inline_data, there is no
>  	 * performance regression.
>  	 */
> -	if (!err) {
> -		err = filemap_write_and_wait_range(inode->i_mapping, 0,
> -						   disk_link.len - 1);
> -
> -		if (!err && IS_DIRSYNC(dir))
> -			err = f2fs_sync_fs(sbi->sb, 1);
> -	}
> -
> -	if (err)
> -		f2fs_unlink(dir, dentry);
> +	err = filemap_write_and_wait_range(inode->i_mapping, 0,
> +					   disk_link.len - 1);
> +	if (!err && IS_DIRSYNC(dir))
> +		f2fs_sync_fs(sbi->sb, 1);
>  
>  	f2fs_balance_fs(sbi, true);
>  	goto out_free_encrypted_link;

Re: [f2fs-dev] [PATCH v2] f2fs: don't leave the hashed inode while it's unlinked

[Jaegeuk Kim via Linux-f2fs-devel]

f2fs_symlink()
1. f2fs_new_inode
2. f2fs_add_link
3. write_being|end to fill the symlink path
4. flush dirty pages and or checkpoint

Step 4 is nice to succeed, which doesn't become a reason to roll back
the created symlink. OTOH, if we get an error till step 3, don't leave
its dentry and its inode.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---

 Change log from v1:
  - fix bugs

 fs/f2fs/f2fs.h  |   3 +-
 fs/f2fs/inode.c |   6 ++-
 fs/f2fs/namei.c | 101 +++++++++++++++++++++++++-----------------------
 3 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a1f5f375045a..b0a9c14de595 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3878,7 +3878,8 @@ void f2fs_update_inode_page(struct inode *inode);
 int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
 void f2fs_remove_donate_inode(struct inode *inode);
 void f2fs_evict_inode(struct inode *inode);
-void f2fs_handle_failed_inode(struct inode *inode, struct f2fs_lock_context *lc);
+void f2fs_handle_failed_inode(struct inode *inode,
+		struct f2fs_lock_context *lc, bool add_orphan);
 int f2fs_init_evict_inode_work(void);
 void f2fs_destroy_evict_inode_work(void);
 
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index bac1e360d966..96cc0e777567 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -1107,7 +1107,8 @@ void f2fs_evict_inode(struct inode *inode)
 }
 
 /* caller should call f2fs_lock_op() */
-void f2fs_handle_failed_inode(struct inode *inode, struct f2fs_lock_context *lc)
+void f2fs_handle_failed_inode(struct inode *inode,
+		struct f2fs_lock_context *lc, bool orphan_free)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct node_info ni;
@@ -1129,6 +1130,9 @@ void f2fs_handle_failed_inode(struct inode *inode, struct f2fs_lock_context *lc)
 	/* don't make bad inode, since it becomes a regular file. */
 	unlock_new_inode(inode);
 
+	if (!orphan_free)
+		goto out;
+
 	/*
 	 * Note: we should add inode to orphan list before f2fs_unlock_op()
 	 * so we can prevent losing this orphan when encoutering checkpoint
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 37897f4321c0..4971872edc5f 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -411,7 +411,7 @@ static int f2fs_create(struct mnt_idmap *idmap, struct inode *dir,
 	f2fs_balance_fs(sbi, true);
 	return 0;
 out:
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, true);
 	return err;
 }
 
@@ -566,38 +566,31 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 	return ERR_PTR(err);
 }
 
-static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
+static int __do_unlink(struct inode *dir, struct inode *inode,
+			const struct qstr *name)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
-	struct inode *inode = d_inode(dentry);
 	struct f2fs_dir_entry *de;
 	struct f2fs_lock_context lc;
 	struct folio *folio;
 	int err;
 
-	trace_f2fs_unlink_enter(dir, dentry);
-
 	if (IS_DEVICE_ALIASING(inode))
 		return -EPERM;
 
-	if (unlikely(f2fs_cp_error(sbi))) {
-		err = -EIO;
-		goto out;
-	}
+	if (unlikely(f2fs_cp_error(sbi)))
+		return -EIO;
 
 	err = f2fs_dquot_initialize(dir);
 	if (err)
-		goto out;
+		return err;
 	err = f2fs_dquot_initialize(inode);
 	if (err)
-		goto out;
+		return err;
 
-	de = f2fs_find_entry(dir, &dentry->d_name, &folio);
-	if (!de) {
-		if (IS_ERR(folio))
-			err = PTR_ERR(folio);
-		goto out;
-	}
+	de = f2fs_find_entry(dir, name, &folio);
+	if (!de)
+		return IS_ERR(folio) ? PTR_ERR(folio) : 0;
 
 	if (unlikely(inode->i_nlink == 0)) {
 		f2fs_warn(sbi, "%s: inode (ino=%llx) has zero i_nlink",
@@ -615,11 +608,28 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
 	err = f2fs_acquire_orphan_inode(sbi);
 	if (err) {
 		f2fs_unlock_op(sbi, &lc);
-		f2fs_folio_put(folio, false);
-		goto out;
+		goto err_out;
 	}
 	f2fs_delete_entry(de, folio, dir, inode);
 	f2fs_unlock_op(sbi, &lc);
+	return 0;
+
+corrupted:
+	err = -EFSCORRUPTED;
+	set_sbi_flag(sbi, SBI_NEED_FSCK);
+err_out:
+	f2fs_folio_put(folio, false);
+	return err;
+}
+
+static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
+{
+	int err;
+
+	trace_f2fs_unlink_enter(dir, dentry);
+	err = __do_unlink(dir, d_inode(dentry), &dentry->d_name);
+	if (err)
+		goto out;
 
 	/* VFS negative dentries are incompatible with Encoding and
 	 * Case-insensitiveness. Eventually we'll want avoid
@@ -630,19 +640,10 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
 	if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
 		d_invalidate(dentry);
 
-	if (IS_DIRSYNC(dir)) {
-		err = f2fs_sync_fs(sbi->sb, 1);
-		if (err)
-			goto out;
-	}
-
-	goto out;
-corrupted:
-	err = -EFSCORRUPTED;
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
-	f2fs_folio_put(folio, false);
+	if (IS_DIRSYNC(dir))
+		err = f2fs_sync_fs(F2FS_I_SB(dir)->sb, 1);
 out:
-	trace_f2fs_unlink_exit(inode, err);
+	trace_f2fs_unlink_exit(d_inode(dentry), err);
 	return err;
 }
 
@@ -669,7 +670,8 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	struct inode *inode;
 	size_t len = strlen(symname);
 	struct fscrypt_str disk_link;
-	int err;
+	bool orphan_free = true;
+	int err, ret;
 
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
@@ -703,13 +705,16 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	f2fs_unlock_op(sbi, &lc);
 	f2fs_alloc_nid_done(sbi, inode->i_ino);
 
+	/* Write the symlink path to the new inode. */
 	err = fscrypt_encrypt_symlink(inode, symname, len, &disk_link);
 	if (err)
-		goto err_out;
+		goto unlink_out_f2fs_handle_failed_inode;
 
 	err = page_symlink(inode, disk_link.name, disk_link.len);
+	if (err)
+		goto unlink_out_f2fs_handle_failed_inode;
 
-err_out:
+give_up:
 	d_instantiate_new(dentry, inode);
 
 	/*
@@ -721,22 +726,22 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	 * If the symlink path is stored into inline_data, there is no
 	 * performance regression.
 	 */
-	if (!err) {
-		err = filemap_write_and_wait_range(inode->i_mapping, 0,
-						   disk_link.len - 1);
-
-		if (!err && IS_DIRSYNC(dir))
-			err = f2fs_sync_fs(sbi->sb, 1);
-	}
-
-	if (err)
-		f2fs_unlink(dir, dentry);
+	ret = filemap_write_and_wait_range(inode->i_mapping, 0,
+					   disk_link.len - 1);
+	if (!ret && IS_DIRSYNC(dir))
+		err = f2fs_sync_fs(sbi->sb, 1);
 
 	f2fs_balance_fs(sbi, true);
 	goto out_free_encrypted_link;
 
+unlink_out_f2fs_handle_failed_inode:
+	err = __do_unlink(dir, inode, &dentry->d_name);
+	if (err)
+		goto give_up;
+	orphan_free = false;
+	f2fs_lock_op(sbi, &lc);
 out_f2fs_handle_failed_inode:
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, orphan_free);
 out_free_encrypted_link:
 	if (disk_link.name != (unsigned char *)symname)
 		kfree(disk_link.name);
@@ -789,7 +794,7 @@ static struct dentry *f2fs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 
 out_fail:
 	clear_inode_flag(inode, FI_INC_LINK);
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, true);
 	return ERR_PTR(err);
 }
 
@@ -845,7 +850,7 @@ static int f2fs_mknod(struct mnt_idmap *idmap, struct inode *dir,
 	f2fs_balance_fs(sbi, true);
 	return 0;
 out:
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, true);
 	return err;
 }
 
@@ -916,7 +921,7 @@ static int __f2fs_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
 release_out:
 	f2fs_release_orphan_inode(sbi);
 out:
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, true);
 	return err;
 }
 
-- 
2.55.0.766.g2966f0265a-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2] f2fs: don't leave the hashed inode while it's unlinked

[Jaegeuk Kim]

f2fs_symlink()
1. f2fs_new_inode
2. f2fs_add_link
3. write_being|end to fill the symlink path
4. flush dirty pages and or checkpoint

Step 4 is nice to succeed, which doesn't become a reason to roll back
the created symlink. OTOH, if we get an error till step 3, don't leave
its dentry and its inode.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---

 Change log from v1:
  - fix bugs

 fs/f2fs/f2fs.h  |   3 +-
 fs/f2fs/inode.c |   6 ++-
 fs/f2fs/namei.c | 101 +++++++++++++++++++++++++-----------------------
 3 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a1f5f375045a..b0a9c14de595 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3878,7 +3878,8 @@ void f2fs_update_inode_page(struct inode *inode);
 int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
 void f2fs_remove_donate_inode(struct inode *inode);
 void f2fs_evict_inode(struct inode *inode);
-void f2fs_handle_failed_inode(struct inode *inode, struct f2fs_lock_context *lc);
+void f2fs_handle_failed_inode(struct inode *inode,
+		struct f2fs_lock_context *lc, bool add_orphan);
 int f2fs_init_evict_inode_work(void);
 void f2fs_destroy_evict_inode_work(void);
 
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index bac1e360d966..96cc0e777567 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -1107,7 +1107,8 @@ void f2fs_evict_inode(struct inode *inode)
 }
 
 /* caller should call f2fs_lock_op() */
-void f2fs_handle_failed_inode(struct inode *inode, struct f2fs_lock_context *lc)
+void f2fs_handle_failed_inode(struct inode *inode,
+		struct f2fs_lock_context *lc, bool orphan_free)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct node_info ni;
@@ -1129,6 +1130,9 @@ void f2fs_handle_failed_inode(struct inode *inode, struct f2fs_lock_context *lc)
 	/* don't make bad inode, since it becomes a regular file. */
 	unlock_new_inode(inode);
 
+	if (!orphan_free)
+		goto out;
+
 	/*
 	 * Note: we should add inode to orphan list before f2fs_unlock_op()
 	 * so we can prevent losing this orphan when encoutering checkpoint
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 37897f4321c0..4971872edc5f 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -411,7 +411,7 @@ static int f2fs_create(struct mnt_idmap *idmap, struct inode *dir,
 	f2fs_balance_fs(sbi, true);
 	return 0;
 out:
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, true);
 	return err;
 }
 
@@ -566,38 +566,31 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 	return ERR_PTR(err);
 }
 
-static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
+static int __do_unlink(struct inode *dir, struct inode *inode,
+			const struct qstr *name)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
-	struct inode *inode = d_inode(dentry);
 	struct f2fs_dir_entry *de;
 	struct f2fs_lock_context lc;
 	struct folio *folio;
 	int err;
 
-	trace_f2fs_unlink_enter(dir, dentry);
-
 	if (IS_DEVICE_ALIASING(inode))
 		return -EPERM;
 
-	if (unlikely(f2fs_cp_error(sbi))) {
-		err = -EIO;
-		goto out;
-	}
+	if (unlikely(f2fs_cp_error(sbi)))
+		return -EIO;
 
 	err = f2fs_dquot_initialize(dir);
 	if (err)
-		goto out;
+		return err;
 	err = f2fs_dquot_initialize(inode);
 	if (err)
-		goto out;
+		return err;
 
-	de = f2fs_find_entry(dir, &dentry->d_name, &folio);
-	if (!de) {
-		if (IS_ERR(folio))
-			err = PTR_ERR(folio);
-		goto out;
-	}
+	de = f2fs_find_entry(dir, name, &folio);
+	if (!de)
+		return IS_ERR(folio) ? PTR_ERR(folio) : 0;
 
 	if (unlikely(inode->i_nlink == 0)) {
 		f2fs_warn(sbi, "%s: inode (ino=%llx) has zero i_nlink",
@@ -615,11 +608,28 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
 	err = f2fs_acquire_orphan_inode(sbi);
 	if (err) {
 		f2fs_unlock_op(sbi, &lc);
-		f2fs_folio_put(folio, false);
-		goto out;
+		goto err_out;
 	}
 	f2fs_delete_entry(de, folio, dir, inode);
 	f2fs_unlock_op(sbi, &lc);
+	return 0;
+
+corrupted:
+	err = -EFSCORRUPTED;
+	set_sbi_flag(sbi, SBI_NEED_FSCK);
+err_out:
+	f2fs_folio_put(folio, false);
+	return err;
+}
+
+static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
+{
+	int err;
+
+	trace_f2fs_unlink_enter(dir, dentry);
+	err = __do_unlink(dir, d_inode(dentry), &dentry->d_name);
+	if (err)
+		goto out;
 
 	/* VFS negative dentries are incompatible with Encoding and
 	 * Case-insensitiveness. Eventually we'll want avoid
@@ -630,19 +640,10 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
 	if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
 		d_invalidate(dentry);
 
-	if (IS_DIRSYNC(dir)) {
-		err = f2fs_sync_fs(sbi->sb, 1);
-		if (err)
-			goto out;
-	}
-
-	goto out;
-corrupted:
-	err = -EFSCORRUPTED;
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
-	f2fs_folio_put(folio, false);
+	if (IS_DIRSYNC(dir))
+		err = f2fs_sync_fs(F2FS_I_SB(dir)->sb, 1);
 out:
-	trace_f2fs_unlink_exit(inode, err);
+	trace_f2fs_unlink_exit(d_inode(dentry), err);
 	return err;
 }
 
@@ -669,7 +670,8 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	struct inode *inode;
 	size_t len = strlen(symname);
 	struct fscrypt_str disk_link;
-	int err;
+	bool orphan_free = true;
+	int err, ret;
 
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
@@ -703,13 +705,16 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	f2fs_unlock_op(sbi, &lc);
 	f2fs_alloc_nid_done(sbi, inode->i_ino);
 
+	/* Write the symlink path to the new inode. */
 	err = fscrypt_encrypt_symlink(inode, symname, len, &disk_link);
 	if (err)
-		goto err_out;
+		goto unlink_out_f2fs_handle_failed_inode;
 
 	err = page_symlink(inode, disk_link.name, disk_link.len);
+	if (err)
+		goto unlink_out_f2fs_handle_failed_inode;
 
-err_out:
+give_up:
 	d_instantiate_new(dentry, inode);
 
 	/*
@@ -721,22 +726,22 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	 * If the symlink path is stored into inline_data, there is no
 	 * performance regression.
 	 */
-	if (!err) {
-		err = filemap_write_and_wait_range(inode->i_mapping, 0,
-						   disk_link.len - 1);
-
-		if (!err && IS_DIRSYNC(dir))
-			err = f2fs_sync_fs(sbi->sb, 1);
-	}
-
-	if (err)
-		f2fs_unlink(dir, dentry);
+	ret = filemap_write_and_wait_range(inode->i_mapping, 0,
+					   disk_link.len - 1);
+	if (!ret && IS_DIRSYNC(dir))
+		err = f2fs_sync_fs(sbi->sb, 1);
 
 	f2fs_balance_fs(sbi, true);
 	goto out_free_encrypted_link;
 
+unlink_out_f2fs_handle_failed_inode:
+	err = __do_unlink(dir, inode, &dentry->d_name);
+	if (err)
+		goto give_up;
+	orphan_free = false;
+	f2fs_lock_op(sbi, &lc);
 out_f2fs_handle_failed_inode:
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, orphan_free);
 out_free_encrypted_link:
 	if (disk_link.name != (unsigned char *)symname)
 		kfree(disk_link.name);
@@ -789,7 +794,7 @@ static struct dentry *f2fs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 
 out_fail:
 	clear_inode_flag(inode, FI_INC_LINK);
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, true);
 	return ERR_PTR(err);
 }
 
@@ -845,7 +850,7 @@ static int f2fs_mknod(struct mnt_idmap *idmap, struct inode *dir,
 	f2fs_balance_fs(sbi, true);
 	return 0;
 out:
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, true);
 	return err;
 }
 
@@ -916,7 +921,7 @@ static int __f2fs_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
 release_out:
 	f2fs_release_orphan_inode(sbi);
 out:
-	f2fs_handle_failed_inode(inode, &lc);
+	f2fs_handle_failed_inode(inode, &lc, true);
 	return err;
 }
 
-- 
2.55.0.766.g2966f0265a-goog

Re: [f2fs-dev] [PATCH v2] f2fs: don't leave the hashed inode while it's unlinked

[Chao Yu via Linux-f2fs-devel]

On 8/21/26 23:23, Jaegeuk Kim via Linux-f2fs-devel wrote:
> f2fs_symlink()
> 1. f2fs_new_inode
> 2. f2fs_add_link
> 3. write_being|end to fill the symlink path
> 4. flush dirty pages and or checkpoint
> 
> Step 4 is nice to succeed, which doesn't become a reason to roll back
> the created symlink. OTOH, if we get an error till step 3, don't leave
> its dentry and its inode.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2] f2fs: don't leave the hashed inode while it's unlinked

[Chao Yu]

On 8/21/26 23:23, Jaegeuk Kim via Linux-f2fs-devel wrote:
> f2fs_symlink()
> 1. f2fs_new_inode
> 2. f2fs_add_link
> 3. write_being|end to fill the symlink path
> 4. flush dirty pages and or checkpoint
> 
> Step 4 is nice to succeed, which doesn't become a reason to roll back
> the created symlink. OTOH, if we get an error till step 3, don't leave
> its dentry and its inode.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [f2fs-dev] [PATCH v2] f2fs: don't leave the hashed inode while it's unlinked

[Wenjie Qi]

Hi Jaegeuk,

It looks like the original symlink construction error is lost here:

	err = __do_unlink(dir, inode, &dentry->d_name);

If fscrypt_encrypt_symlink() or page_symlink() fails but __do_unlink()
succeeds, err becomes zero. f2fs_handle_failed_inode() then releases the
inode, and f2fs_symlink() returns success even though the directory entry
has been removed.

Is this intended? Should the rollback result be stored separately, for
example in ret, while preserving the original construction error in err?

There seems to be a similar issue in the give_up path. If rollback fails
in a DIRSYNC directory, a successful f2fs_sync_fs() can overwrite the
existing error with zero.

Regards,
Wenjie


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v2] f2fs: don't leave the hashed inode while it's unlinked

[Wenjie Qi]

Hi Jaegeuk,

It looks like the original symlink construction error is lost here:

	err = __do_unlink(dir, inode, &dentry->d_name);

If fscrypt_encrypt_symlink() or page_symlink() fails but __do_unlink()
succeeds, err becomes zero. f2fs_handle_failed_inode() then releases the
inode, and f2fs_symlink() returns success even though the directory entry
has been removed.

Is this intended? Should the rollback result be stored separately, for
example in ret, while preserving the original construction error in err?

There seems to be a similar issue in the give_up path. If rollback fails
in a DIRSYNC directory, a successful f2fs_sync_fs() can overwrite the
existing error with zero.

Regards,
Wenjie