* Re: [f2fs-dev] [PATCH] f2fs: don't leave the hashed inode while it's unlinked
2026-08-18 20:01 [PATCH] f2fs: don't leave the hashed inode while it's unlinked Jaegeuk Kim
@ 2026-08-19 2:56 ` Chao Yu
2026-08-21 15:23 ` [f2fs-dev] [PATCH v2] " Jaegeuk Kim
1 sibling, 0 replies; 4+ messages in thread
From: Chao Yu @ 2026-08-19 2:56 UTC (permalink / raw)
To: Jaegeuk Kim, Alexander Viro; +Cc: chao, linux-kernel, 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;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [f2fs-dev] [PATCH v2] f2fs: don't leave the hashed inode while it's unlinked
2026-08-18 20:01 [PATCH] f2fs: don't leave the hashed inode while it's unlinked Jaegeuk Kim
2026-08-19 2:56 ` [f2fs-dev] " Chao Yu
@ 2026-08-21 15:23 ` Jaegeuk Kim
2026-08-22 7:37 ` Chao Yu
1 sibling, 1 reply; 4+ messages in thread
From: Jaegeuk Kim @ 2026-08-21 15:23 UTC (permalink / raw)
To: linux-kernel, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread