[PATCH 1/4] f2fs: convert inline_dir early before starting rename

[PATCH 1/4] f2fs: convert inline_dir early before starting rename

[Jaegeuk Kim]

If we hit an error during rename, we'll get two dentries in different
directories.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/inline.c | 30 ++++++++++++++++++++++++++++--
 fs/f2fs/namei.c  | 36 +++++++++++++-----------------------
 3 files changed, 42 insertions(+), 25 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index dbc20d33d0e1..8d64525743cb 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3490,6 +3490,7 @@ void f2fs_truncate_inline_inode(struct inode *inode,
 int f2fs_read_inline_data(struct inode *inode, struct page *page);
 int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page);
 int f2fs_convert_inline_inode(struct inode *inode);
+int f2fs_convert_inline_dir(struct inode *dir);
 int f2fs_write_inline_data(struct inode *inode, struct page *page);
 bool f2fs_recover_inline_data(struct inode *inode, struct page *npage);
 struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir,
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 52f85ed07a15..f82c3d9cf333 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -530,7 +530,7 @@ static int f2fs_move_rehashed_dirents(struct inode *dir, struct page *ipage,
 	return err;
 }
 
-static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
+static int do_convert_inline_dir(struct inode *dir, struct page *ipage,
 							void *inline_dentry)
 {
 	if (!F2FS_I(dir)->i_dir_level)
@@ -539,6 +539,32 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
 		return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry);
 }
 
+int f2fs_convert_inline_dir(struct inode *dir)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
+	struct page *ipage;
+	void *inline_dentry = NULL;
+	int err;
+
+	if (!f2fs_has_inline_dentry(dir))
+		return 0;
+
+	f2fs_lock_op(sbi);
+
+	ipage = f2fs_get_node_page(sbi, dir->i_ino);
+	if (IS_ERR(ipage))
+		return PTR_ERR(ipage);
+
+	inline_dentry = inline_data_addr(dir, ipage);
+
+	err = do_convert_inline_dir(dir, ipage, inline_dentry);
+	if (!err)
+		f2fs_put_page(ipage, 1);
+
+	f2fs_unlock_op(sbi);
+	return err;
+}
+
 int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
 				const struct qstr *orig_name,
 				struct inode *inode, nid_t ino, umode_t mode)
@@ -562,7 +588,7 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
 
 	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
 	if (bit_pos >= d.max) {
-		err = f2fs_convert_inline_dir(dir, ipage, inline_dentry);
+		err = do_convert_inline_dir(dir, ipage, inline_dentry);
 		if (err)
 			return err;
 		err = -EAGAIN;
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 5d9584281935..61615ab466c2 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -855,7 +855,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct f2fs_dir_entry *old_dir_entry = NULL;
 	struct f2fs_dir_entry *old_entry;
 	struct f2fs_dir_entry *new_entry;
-	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
 	int err;
 
 	if (unlikely(f2fs_cp_error(sbi)))
@@ -868,6 +867,19 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			F2FS_I(old_dentry->d_inode)->i_projid)))
 		return -EXDEV;
 
+	/*
+	 * old entry and new entry can locate in the same inline
+	 * dentry in inode, when attaching new entry in inline dentry,
+	 * it could force inline dentry conversion, after that,
+	 * old_entry and old_page will point to wrong address, in
+	 * order to avoid this, let's do the check and update here.
+	 */
+	if (old_dir == new_dir && !new_inode) {
+		err = f2fs_convert_inline_dir(old_dir);
+		if (err)
+			return err;
+	}
+
 	if (flags & RENAME_WHITEOUT) {
 		err = f2fs_create_whiteout(old_dir, &whiteout);
 		if (err)
@@ -954,28 +966,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 		if (old_dir_entry)
 			f2fs_i_links_write(new_dir, true);
-
-		/*
-		 * old entry and new entry can locate in the same inline
-		 * dentry in inode, when attaching new entry in inline dentry,
-		 * it could force inline dentry conversion, after that,
-		 * old_entry and old_page will point to wrong address, in
-		 * order to avoid this, let's do the check and update here.
-		 */
-		if (is_old_inline && !f2fs_has_inline_dentry(old_dir)) {
-			f2fs_put_page(old_page, 0);
-			old_page = NULL;
-
-			old_entry = f2fs_find_entry(old_dir,
-						&old_dentry->d_name, &old_page);
-			if (!old_entry) {
-				err = -ENOENT;
-				if (IS_ERR(old_page))
-					err = PTR_ERR(old_page);
-				f2fs_unlock_op(sbi);
-				goto out_dir;
-			}
-		}
 	}
 
 	down_write(&F2FS_I(old_inode)->i_sem);
-- 
2.24.0.525.g8f36a354ae-goog

[PATCH 2/4] f2fs: don't put new_page twice in f2fs_rename

[Jaegeuk Kim]

In f2fs_rename(), new_page is gone after f2fs_set_link(), but it tries
to put again when whiteout is failed and jumped to put_out_dir.

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

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 61615ab466c2..91e7b8360d2a 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -850,7 +850,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct inode *old_inode = d_inode(old_dentry);
 	struct inode *new_inode = d_inode(new_dentry);
 	struct inode *whiteout = NULL;
-	struct page *old_dir_page;
+	struct page *old_dir_page = NULL;
 	struct page *old_page, *new_page = NULL;
 	struct f2fs_dir_entry *old_dir_entry = NULL;
 	struct f2fs_dir_entry *old_entry;
@@ -941,6 +941,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			goto put_out_dir;
 
 		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
+		new_page = NULL;
 
 		new_inode->i_ctime = current_time(new_inode);
 		down_write(&F2FS_I(new_inode)->i_sem);
@@ -980,6 +981,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	f2fs_mark_inode_dirty_sync(old_inode, false);
 
 	f2fs_delete_entry(old_entry, old_page, old_dir, NULL);
+	old_page = NULL;
 
 	if (whiteout) {
 		set_inode_flag(whiteout, FI_INC_LINK);
@@ -1015,8 +1017,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 put_out_dir:
 	f2fs_unlock_op(sbi);
-	if (new_page)
-		f2fs_put_page(new_page, 0);
+	f2fs_put_page(new_page, 0);
 out_dir:
 	if (old_dir_entry)
 		f2fs_put_page(old_dir_page, 0);
-- 
2.24.0.525.g8f36a354ae-goog

[PATCH 3/4] f2fs: declare nested quota_sem and remove unnecessary sems

[Jaegeuk Kim]

1.
f2fs_quota_sync
 -> down_read(&sbi->quota_sem)
 -> dquot_writeback_dquots
  -> f2fs_dquot_commit
   -> down_read(&sbi->quota_sem)

2.
f2fs_quota_sync
 -> down_read(&sbi->quota_sem)
  -> f2fs_write_data_pages
   -> f2fs_write_single_data_page
    -> down_write(&F2FS_I(inode)->i_sem)

f2fs_mkdir
 -> f2fs_do_add_link
   -> down_write(&F2FS_I(inode)->i_sem)
   -> f2fs_init_inode_metadata
    -> f2fs_new_node_page
     -> dquot_alloc_inode
      -> f2fs_dquot_mark_dquot_dirty
       -> down_read(&sbi->quota_sem)

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

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 9e85894d12c9..e6fb4b52eb91 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2158,7 +2158,7 @@ static int f2fs_dquot_commit(struct dquot *dquot)
 	struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb);
 	int ret;
 
-	down_read(&sbi->quota_sem);
+	down_read_nested(&sbi->quota_sem, SINGLE_DEPTH_NESTING);
 	ret = dquot_commit(dquot);
 	if (ret < 0)
 		set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
@@ -2182,13 +2182,10 @@ static int f2fs_dquot_acquire(struct dquot *dquot)
 static int f2fs_dquot_release(struct dquot *dquot)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb);
-	int ret;
+	int ret = dquot_release(dquot);
 
-	down_read(&sbi->quota_sem);
-	ret = dquot_release(dquot);
 	if (ret < 0)
 		set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
-	up_read(&sbi->quota_sem);
 	return ret;
 }
 
@@ -2196,29 +2193,22 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot)
 {
 	struct super_block *sb = dquot->dq_sb;
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
-	int ret;
-
-	down_read(&sbi->quota_sem);
-	ret = dquot_mark_dquot_dirty(dquot);
+	int ret = dquot_mark_dquot_dirty(dquot);
 
 	/* if we are using journalled quota */
 	if (is_journalled_quota(sbi))
 		set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH);
 
-	up_read(&sbi->quota_sem);
 	return ret;
 }
 
 static int f2fs_dquot_commit_info(struct super_block *sb, int type)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
-	int ret;
+	int ret = dquot_commit_info(sb, type);
 
-	down_read(&sbi->quota_sem);
-	ret = dquot_commit_info(sb, type);
 	if (ret < 0)
 		set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
-	up_read(&sbi->quota_sem);
 	return ret;
 }
 
-- 
2.24.0.525.g8f36a354ae-goog

[PATCH 4/4] f2fs: free sysfs kobject

[Jaegeuk Kim]

Detected kmemleak.

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

diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 70945ceb9c0c..5963316f391a 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -786,4 +786,5 @@ void f2fs_unregister_sysfs(struct f2fs_sb_info *sbi)
 		remove_proc_entry(sbi->sb->s_id, f2fs_proc_root);
 	}
 	kobject_del(&sbi->s_kobj);
+	kobject_put(&sbi->s_kobj);
 }
-- 
2.24.0.525.g8f36a354ae-goog

Re: [f2fs-dev] [PATCH 1/4] f2fs: convert inline_dir early before starting rename

[Chao Yu]

On 2019/12/19 4:09, Jaegeuk Kim wrote:
> If we hit an error during rename, we'll get two dentries in different
> directories.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/f2fs.h   |  1 +
>  fs/f2fs/inline.c | 30 ++++++++++++++++++++++++++++--
>  fs/f2fs/namei.c  | 36 +++++++++++++-----------------------
>  3 files changed, 42 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index dbc20d33d0e1..8d64525743cb 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3490,6 +3490,7 @@ void f2fs_truncate_inline_inode(struct inode *inode,
>  int f2fs_read_inline_data(struct inode *inode, struct page *page);
>  int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page);
>  int f2fs_convert_inline_inode(struct inode *inode);
> +int f2fs_convert_inline_dir(struct inode *dir);
>  int f2fs_write_inline_data(struct inode *inode, struct page *page);
>  bool f2fs_recover_inline_data(struct inode *inode, struct page *npage);
>  struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir,
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index 52f85ed07a15..f82c3d9cf333 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -530,7 +530,7 @@ static int f2fs_move_rehashed_dirents(struct inode *dir, struct page *ipage,
>  	return err;
>  }
>  
> -static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
> +static int do_convert_inline_dir(struct inode *dir, struct page *ipage,
>  							void *inline_dentry)
>  {
>  	if (!F2FS_I(dir)->i_dir_level)
> @@ -539,6 +539,32 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
>  		return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry);
>  }
>  
> +int f2fs_convert_inline_dir(struct inode *dir)
> +{
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> +	struct page *ipage;
> +	void *inline_dentry = NULL;
> +	int err;
> +
> +	if (!f2fs_has_inline_dentry(dir))
> +		return 0;
> +
> +	f2fs_lock_op(sbi);
> +
> +	ipage = f2fs_get_node_page(sbi, dir->i_ino);
> +	if (IS_ERR(ipage))
> +		return PTR_ERR(ipage);
> +
> +	inline_dentry = inline_data_addr(dir, ipage);
> +
> +	err = do_convert_inline_dir(dir, ipage, inline_dentry);
> +	if (!err)
> +		f2fs_put_page(ipage, 1);
> +
> +	f2fs_unlock_op(sbi);
> +	return err;
> +}
> +
>  int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>  				const struct qstr *orig_name,
>  				struct inode *inode, nid_t ino, umode_t mode)
> @@ -562,7 +588,7 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>  
>  	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
>  	if (bit_pos >= d.max) {
> -		err = f2fs_convert_inline_dir(dir, ipage, inline_dentry);
> +		err = do_convert_inline_dir(dir, ipage, inline_dentry);
>  		if (err)
>  			return err;
>  		err = -EAGAIN;
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 5d9584281935..61615ab466c2 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -855,7 +855,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	struct f2fs_dir_entry *old_dir_entry = NULL;
>  	struct f2fs_dir_entry *old_entry;
>  	struct f2fs_dir_entry *new_entry;
> -	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
>  	int err;
>  
>  	if (unlikely(f2fs_cp_error(sbi)))
> @@ -868,6 +867,19 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  			F2FS_I(old_dentry->d_inode)->i_projid)))
>  		return -EXDEV;
>  
> +	/*
> +	 * old entry and new entry can locate in the same inline
> +	 * dentry in inode, when attaching new entry in inline dentry,
> +	 * it could force inline dentry conversion, after that,
> +	 * old_entry and old_page will point to wrong address, in
> +	 * order to avoid this, let's do the check and update here.
> +	 */

The comment is out-of-update here...

If there is enough room for the new dentry in old directory's inline space,
how about just keeping inline dir state for old directory?


---
 fs/f2fs/dir.c    | 14 ++++++++++++++
 fs/f2fs/f2fs.h   |  4 +++-
 fs/f2fs/inline.c | 22 +++++++++++++++++-----
 fs/f2fs/namei.c  |  2 +-
 4 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index c967cacf979e..b56f6060c1a6 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -578,6 +578,20 @@ int f2fs_room_for_filename(const void *bitmap, int slots, int max_slots)
 	goto next;
 }

+bool f2fs_has_enough_room(struct inode *dir, struct page *ipage,
+					struct fscrypt_name *fname)
+{
+	struct f2fs_dentry_ptr d;
+	unsigned int bit_pos;
+	int slots = GET_DENTRY_SLOTS(fname_len(fname));
+
+	make_dentry_ptr_inline(dir, &d, inline_data_addr(dir, ipage));
+
+	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
+
+	return bit_pos < d.max;
+}
+
 void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
 				const struct qstr *name, f2fs_hash_t name_hash,
 				unsigned int bit_pos)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 5d55cef66410..ce4ce33a40ce 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3120,6 +3120,8 @@ 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);
+bool f2fs_has_enough_room(struct inode *dir, struct page *ipage,
+					struct fscrypt_name *fname);
 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);
@@ -3662,7 +3664,7 @@ void f2fs_truncate_inline_inode(struct inode *inode,
 int f2fs_read_inline_data(struct inode *inode, struct page *page);
 int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page);
 int f2fs_convert_inline_inode(struct inode *inode);
-int f2fs_convert_inline_dir(struct inode *dir);
+int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry);
 int f2fs_write_inline_data(struct inode *inode, struct page *page);
 bool f2fs_recover_inline_data(struct inode *inode, struct page *npage);
 struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir,
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index f82c3d9cf333..4167e5408151 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -539,28 +539,40 @@ static int do_convert_inline_dir(struct inode *dir, struct page *ipage,
 		return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry);
 }

-int f2fs_convert_inline_dir(struct inode *dir)
+int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
 	struct page *ipage;
+	struct fscrypt_name fname;
 	void *inline_dentry = NULL;
-	int err;
+	int err = 0;

 	if (!f2fs_has_inline_dentry(dir))
 		return 0;

 	f2fs_lock_op(sbi);

+	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &fname);
+	if (err)
+		goto out;
+
 	ipage = f2fs_get_node_page(sbi, dir->i_ino);
-	if (IS_ERR(ipage))
-		return PTR_ERR(ipage);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto out;
+	}
+
+	if (f2fs_has_enough_room(dir, ipage, &fname)) {
+		f2fs_put_page(ipage, 1);
+		goto out;
+	}

 	inline_dentry = inline_data_addr(dir, ipage);

 	err = do_convert_inline_dir(dir, ipage, inline_dentry);
 	if (!err)
 		f2fs_put_page(ipage, 1);
-
+out:
 	f2fs_unlock_op(sbi);
 	return err;
 }
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 1daa54c8d8d8..856f0f984549 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -927,7 +927,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	 * order to avoid this, let's do the check and update here.
 	 */
 	if (old_dir == new_dir && !new_inode) {
-		err = f2fs_convert_inline_dir(old_dir);
+		err = f2fs_try_convert_inline_dir(old_dir, new_dentry);
 		if (err)
 			return err;
 	}
-- 
2.18.0.rc1

Thanks,

> +	if (old_dir == new_dir && !new_inode) {
> +		err = f2fs_convert_inline_dir(old_dir);
> +		if (err)
> +			return err;
> +	}
> +
>  	if (flags & RENAME_WHITEOUT) {
>  		err = f2fs_create_whiteout(old_dir, &whiteout);
>  		if (err)
> @@ -954,28 +966,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  		if (old_dir_entry)
>  			f2fs_i_links_write(new_dir, true);
> -
> -		/*
> -		 * old entry and new entry can locate in the same inline
> -		 * dentry in inode, when attaching new entry in inline dentry,
> -		 * it could force inline dentry conversion, after that,
> -		 * old_entry and old_page will point to wrong address, in
> -		 * order to avoid this, let's do the check and update here.
> -		 */
> -		if (is_old_inline && !f2fs_has_inline_dentry(old_dir)) {
> -			f2fs_put_page(old_page, 0);
> -			old_page = NULL;
> -
> -			old_entry = f2fs_find_entry(old_dir,
> -						&old_dentry->d_name, &old_page);
> -			if (!old_entry) {
> -				err = -ENOENT;
> -				if (IS_ERR(old_page))
> -					err = PTR_ERR(old_page);
> -				f2fs_unlock_op(sbi);
> -				goto out_dir;
> -			}
> -		}
>  	}
>  
>  	down_write(&F2FS_I(old_inode)->i_sem);
> 

Re: [f2fs-dev] [PATCH 2/4] f2fs: don't put new_page twice in f2fs_rename

[Chao Yu]

On 2019/12/19 4:09, Jaegeuk Kim wrote:
> In f2fs_rename(), new_page is gone after f2fs_set_link(), but it tries
> to put again when whiteout is failed and jumped to put_out_dir.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

Thanks,

Re: [f2fs-dev] [PATCH 4/4] f2fs: free sysfs kobject

[Chao Yu]

On 2019/12/19 4:09, Jaegeuk Kim wrote:
> Detected kmemleak.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

Thanks,

Re: [f2fs-dev] [PATCH 1/4] f2fs: convert inline_dir early before starting rename

[Jaegeuk Kim]

On 12/28, Chao Yu wrote:
> On 2019/12/19 4:09, Jaegeuk Kim wrote:
> > If we hit an error during rename, we'll get two dentries in different
> > directories.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/f2fs.h   |  1 +
> >  fs/f2fs/inline.c | 30 ++++++++++++++++++++++++++++--
> >  fs/f2fs/namei.c  | 36 +++++++++++++-----------------------
> >  3 files changed, 42 insertions(+), 25 deletions(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index dbc20d33d0e1..8d64525743cb 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -3490,6 +3490,7 @@ void f2fs_truncate_inline_inode(struct inode *inode,
> >  int f2fs_read_inline_data(struct inode *inode, struct page *page);
> >  int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page);
> >  int f2fs_convert_inline_inode(struct inode *inode);
> > +int f2fs_convert_inline_dir(struct inode *dir);
> >  int f2fs_write_inline_data(struct inode *inode, struct page *page);
> >  bool f2fs_recover_inline_data(struct inode *inode, struct page *npage);
> >  struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir,
> > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> > index 52f85ed07a15..f82c3d9cf333 100644
> > --- a/fs/f2fs/inline.c
> > +++ b/fs/f2fs/inline.c
> > @@ -530,7 +530,7 @@ static int f2fs_move_rehashed_dirents(struct inode *dir, struct page *ipage,
> >  	return err;
> >  }
> >  
> > -static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
> > +static int do_convert_inline_dir(struct inode *dir, struct page *ipage,
> >  							void *inline_dentry)
> >  {
> >  	if (!F2FS_I(dir)->i_dir_level)
> > @@ -539,6 +539,32 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
> >  		return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry);
> >  }
> >  
> > +int f2fs_convert_inline_dir(struct inode *dir)
> > +{
> > +	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
> > +	struct page *ipage;
> > +	void *inline_dentry = NULL;
> > +	int err;
> > +
> > +	if (!f2fs_has_inline_dentry(dir))
> > +		return 0;
> > +
> > +	f2fs_lock_op(sbi);
> > +
> > +	ipage = f2fs_get_node_page(sbi, dir->i_ino);
> > +	if (IS_ERR(ipage))
> > +		return PTR_ERR(ipage);
> > +
> > +	inline_dentry = inline_data_addr(dir, ipage);
> > +
> > +	err = do_convert_inline_dir(dir, ipage, inline_dentry);
> > +	if (!err)
> > +		f2fs_put_page(ipage, 1);
> > +
> > +	f2fs_unlock_op(sbi);
> > +	return err;
> > +}
> > +
> >  int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
> >  				const struct qstr *orig_name,
> >  				struct inode *inode, nid_t ino, umode_t mode)
> > @@ -562,7 +588,7 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
> >  
> >  	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
> >  	if (bit_pos >= d.max) {
> > -		err = f2fs_convert_inline_dir(dir, ipage, inline_dentry);
> > +		err = do_convert_inline_dir(dir, ipage, inline_dentry);
> >  		if (err)
> >  			return err;
> >  		err = -EAGAIN;
> > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> > index 5d9584281935..61615ab466c2 100644
> > --- a/fs/f2fs/namei.c
> > +++ b/fs/f2fs/namei.c
> > @@ -855,7 +855,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >  	struct f2fs_dir_entry *old_dir_entry = NULL;
> >  	struct f2fs_dir_entry *old_entry;
> >  	struct f2fs_dir_entry *new_entry;
> > -	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
> >  	int err;
> >  
> >  	if (unlikely(f2fs_cp_error(sbi)))
> > @@ -868,6 +867,19 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >  			F2FS_I(old_dentry->d_inode)->i_projid)))
> >  		return -EXDEV;
> >  
> > +	/*
> > +	 * old entry and new entry can locate in the same inline
> > +	 * dentry in inode, when attaching new entry in inline dentry,
> > +	 * it could force inline dentry conversion, after that,
> > +	 * old_entry and old_page will point to wrong address, in
> > +	 * order to avoid this, let's do the check and update here.
> > +	 */
> 
> The comment is out-of-update here...
> 
> If there is enough room for the new dentry in old directory's inline space,
> how about just keeping inline dir state for old directory?

It'd work, but how can we deal with race conditions?

> 
> 
> ---
>  fs/f2fs/dir.c    | 14 ++++++++++++++
>  fs/f2fs/f2fs.h   |  4 +++-
>  fs/f2fs/inline.c | 22 +++++++++++++++++-----
>  fs/f2fs/namei.c  |  2 +-
>  4 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index c967cacf979e..b56f6060c1a6 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -578,6 +578,20 @@ int f2fs_room_for_filename(const void *bitmap, int slots, int max_slots)
>  	goto next;
>  }
> 
> +bool f2fs_has_enough_room(struct inode *dir, struct page *ipage,
> +					struct fscrypt_name *fname)
> +{
> +	struct f2fs_dentry_ptr d;
> +	unsigned int bit_pos;
> +	int slots = GET_DENTRY_SLOTS(fname_len(fname));
> +
> +	make_dentry_ptr_inline(dir, &d, inline_data_addr(dir, ipage));
> +
> +	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
> +
> +	return bit_pos < d.max;
> +}
> +
>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>  				const struct qstr *name, f2fs_hash_t name_hash,
>  				unsigned int bit_pos)
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 5d55cef66410..ce4ce33a40ce 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3120,6 +3120,8 @@ 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);
> +bool f2fs_has_enough_room(struct inode *dir, struct page *ipage,
> +					struct fscrypt_name *fname);
>  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);
> @@ -3662,7 +3664,7 @@ void f2fs_truncate_inline_inode(struct inode *inode,
>  int f2fs_read_inline_data(struct inode *inode, struct page *page);
>  int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page);
>  int f2fs_convert_inline_inode(struct inode *inode);
> -int f2fs_convert_inline_dir(struct inode *dir);
> +int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry);
>  int f2fs_write_inline_data(struct inode *inode, struct page *page);
>  bool f2fs_recover_inline_data(struct inode *inode, struct page *npage);
>  struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir,
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index f82c3d9cf333..4167e5408151 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -539,28 +539,40 @@ static int do_convert_inline_dir(struct inode *dir, struct page *ipage,
>  		return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry);
>  }
> 
> -int f2fs_convert_inline_dir(struct inode *dir)
> +int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
>  	struct page *ipage;
> +	struct fscrypt_name fname;
>  	void *inline_dentry = NULL;
> -	int err;
> +	int err = 0;
> 
>  	if (!f2fs_has_inline_dentry(dir))
>  		return 0;
> 
>  	f2fs_lock_op(sbi);
> 
> +	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &fname);
> +	if (err)
> +		goto out;
> +
>  	ipage = f2fs_get_node_page(sbi, dir->i_ino);
> -	if (IS_ERR(ipage))
> -		return PTR_ERR(ipage);
> +	if (IS_ERR(ipage)) {
> +		err = PTR_ERR(ipage);
> +		goto out;
> +	}
> +
> +	if (f2fs_has_enough_room(dir, ipage, &fname)) {
> +		f2fs_put_page(ipage, 1);
> +		goto out;
> +	}
> 
>  	inline_dentry = inline_data_addr(dir, ipage);
> 
>  	err = do_convert_inline_dir(dir, ipage, inline_dentry);
>  	if (!err)
>  		f2fs_put_page(ipage, 1);
> -
> +out:
>  	f2fs_unlock_op(sbi);
>  	return err;
>  }
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 1daa54c8d8d8..856f0f984549 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -927,7 +927,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	 * order to avoid this, let's do the check and update here.
>  	 */
>  	if (old_dir == new_dir && !new_inode) {
> -		err = f2fs_convert_inline_dir(old_dir);
> +		err = f2fs_try_convert_inline_dir(old_dir, new_dentry);
>  		if (err)
>  			return err;
>  	}
> -- 
> 2.18.0.rc1
> 
> Thanks,
> 
> > +	if (old_dir == new_dir && !new_inode) {
> > +		err = f2fs_convert_inline_dir(old_dir);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> >  	if (flags & RENAME_WHITEOUT) {
> >  		err = f2fs_create_whiteout(old_dir, &whiteout);
> >  		if (err)
> > @@ -954,28 +966,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >  
> >  		if (old_dir_entry)
> >  			f2fs_i_links_write(new_dir, true);
> > -
> > -		/*
> > -		 * old entry and new entry can locate in the same inline
> > -		 * dentry in inode, when attaching new entry in inline dentry,
> > -		 * it could force inline dentry conversion, after that,
> > -		 * old_entry and old_page will point to wrong address, in
> > -		 * order to avoid this, let's do the check and update here.
> > -		 */
> > -		if (is_old_inline && !f2fs_has_inline_dentry(old_dir)) {
> > -			f2fs_put_page(old_page, 0);
> > -			old_page = NULL;
> > -
> > -			old_entry = f2fs_find_entry(old_dir,
> > -						&old_dentry->d_name, &old_page);
> > -			if (!old_entry) {
> > -				err = -ENOENT;
> > -				if (IS_ERR(old_page))
> > -					err = PTR_ERR(old_page);
> > -				f2fs_unlock_op(sbi);
> > -				goto out_dir;
> > -			}
> > -		}
> >  	}
> >  
> >  	down_write(&F2FS_I(old_inode)->i_sem);
> > 

Re: [f2fs-dev] [PATCH 1/4] f2fs: convert inline_dir early before starting rename

[Chao Yu]

On 2019/12/31 8:06, Jaegeuk Kim wrote:
> On 12/28, Chao Yu wrote:
>> On 2019/12/19 4:09, Jaegeuk Kim wrote:
>>> If we hit an error during rename, we'll get two dentries in different
>>> directories.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/f2fs.h   |  1 +
>>>  fs/f2fs/inline.c | 30 ++++++++++++++++++++++++++++--
>>>  fs/f2fs/namei.c  | 36 +++++++++++++-----------------------
>>>  3 files changed, 42 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index dbc20d33d0e1..8d64525743cb 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -3490,6 +3490,7 @@ void f2fs_truncate_inline_inode(struct inode *inode,
>>>  int f2fs_read_inline_data(struct inode *inode, struct page *page);
>>>  int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page);
>>>  int f2fs_convert_inline_inode(struct inode *inode);
>>> +int f2fs_convert_inline_dir(struct inode *dir);
>>>  int f2fs_write_inline_data(struct inode *inode, struct page *page);
>>>  bool f2fs_recover_inline_data(struct inode *inode, struct page *npage);
>>>  struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir,
>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>>> index 52f85ed07a15..f82c3d9cf333 100644
>>> --- a/fs/f2fs/inline.c
>>> +++ b/fs/f2fs/inline.c
>>> @@ -530,7 +530,7 @@ static int f2fs_move_rehashed_dirents(struct inode *dir, struct page *ipage,
>>>  	return err;
>>>  }
>>>  
>>> -static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
>>> +static int do_convert_inline_dir(struct inode *dir, struct page *ipage,
>>>  							void *inline_dentry)
>>>  {
>>>  	if (!F2FS_I(dir)->i_dir_level)
>>> @@ -539,6 +539,32 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
>>>  		return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry);
>>>  }
>>>  
>>> +int f2fs_convert_inline_dir(struct inode *dir)
>>> +{
>>> +	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
>>> +	struct page *ipage;
>>> +	void *inline_dentry = NULL;
>>> +	int err;
>>> +
>>> +	if (!f2fs_has_inline_dentry(dir))
>>> +		return 0;
>>> +
>>> +	f2fs_lock_op(sbi);
>>> +
>>> +	ipage = f2fs_get_node_page(sbi, dir->i_ino);
>>> +	if (IS_ERR(ipage))
>>> +		return PTR_ERR(ipage);
>>> +
>>> +	inline_dentry = inline_data_addr(dir, ipage);
>>> +
>>> +	err = do_convert_inline_dir(dir, ipage, inline_dentry);
>>> +	if (!err)
>>> +		f2fs_put_page(ipage, 1);
>>> +
>>> +	f2fs_unlock_op(sbi);
>>> +	return err;
>>> +}
>>> +
>>>  int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>>>  				const struct qstr *orig_name,
>>>  				struct inode *inode, nid_t ino, umode_t mode)
>>> @@ -562,7 +588,7 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
>>>  
>>>  	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
>>>  	if (bit_pos >= d.max) {
>>> -		err = f2fs_convert_inline_dir(dir, ipage, inline_dentry);
>>> +		err = do_convert_inline_dir(dir, ipage, inline_dentry);
>>>  		if (err)
>>>  			return err;
>>>  		err = -EAGAIN;
>>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>>> index 5d9584281935..61615ab466c2 100644
>>> --- a/fs/f2fs/namei.c
>>> +++ b/fs/f2fs/namei.c
>>> @@ -855,7 +855,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  	struct f2fs_dir_entry *old_dir_entry = NULL;
>>>  	struct f2fs_dir_entry *old_entry;
>>>  	struct f2fs_dir_entry *new_entry;
>>> -	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
>>>  	int err;
>>>  
>>>  	if (unlikely(f2fs_cp_error(sbi)))
>>> @@ -868,6 +867,19 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  			F2FS_I(old_dentry->d_inode)->i_projid)))
>>>  		return -EXDEV;
>>>  
>>> +	/*
>>> +	 * old entry and new entry can locate in the same inline
>>> +	 * dentry in inode, when attaching new entry in inline dentry,
>>> +	 * it could force inline dentry conversion, after that,
>>> +	 * old_entry and old_page will point to wrong address, in
>>> +	 * order to avoid this, let's do the check and update here.
>>> +	 */
>>
>> The comment is out-of-update here...
>>
>> If there is enough room for the new dentry in old directory's inline space,
>> how about just keeping inline dir state for old directory?
> 
> It'd work, but how can we deal with race conditions?

In rename() syscall, both src/dst's parent inode lock should has been hold to
prevent race condition.

Thanks,

> 
>>
>>
>> ---
>>  fs/f2fs/dir.c    | 14 ++++++++++++++
>>  fs/f2fs/f2fs.h   |  4 +++-
>>  fs/f2fs/inline.c | 22 +++++++++++++++++-----
>>  fs/f2fs/namei.c  |  2 +-
>>  4 files changed, 35 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index c967cacf979e..b56f6060c1a6 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -578,6 +578,20 @@ int f2fs_room_for_filename(const void *bitmap, int slots, int max_slots)
>>  	goto next;
>>  }
>>
>> +bool f2fs_has_enough_room(struct inode *dir, struct page *ipage,
>> +					struct fscrypt_name *fname)
>> +{
>> +	struct f2fs_dentry_ptr d;
>> +	unsigned int bit_pos;
>> +	int slots = GET_DENTRY_SLOTS(fname_len(fname));
>> +
>> +	make_dentry_ptr_inline(dir, &d, inline_data_addr(dir, ipage));
>> +
>> +	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
>> +
>> +	return bit_pos < d.max;
>> +}
>> +
>>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
>>  				const struct qstr *name, f2fs_hash_t name_hash,
>>  				unsigned int bit_pos)
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 5d55cef66410..ce4ce33a40ce 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -3120,6 +3120,8 @@ 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);
>> +bool f2fs_has_enough_room(struct inode *dir, struct page *ipage,
>> +					struct fscrypt_name *fname);
>>  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);
>> @@ -3662,7 +3664,7 @@ void f2fs_truncate_inline_inode(struct inode *inode,
>>  int f2fs_read_inline_data(struct inode *inode, struct page *page);
>>  int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page);
>>  int f2fs_convert_inline_inode(struct inode *inode);
>> -int f2fs_convert_inline_dir(struct inode *dir);
>> +int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry);
>>  int f2fs_write_inline_data(struct inode *inode, struct page *page);
>>  bool f2fs_recover_inline_data(struct inode *inode, struct page *npage);
>>  struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir,
>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>> index f82c3d9cf333..4167e5408151 100644
>> --- a/fs/f2fs/inline.c
>> +++ b/fs/f2fs/inline.c
>> @@ -539,28 +539,40 @@ static int do_convert_inline_dir(struct inode *dir, struct page *ipage,
>>  		return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry);
>>  }
>>
>> -int f2fs_convert_inline_dir(struct inode *dir)
>> +int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry)
>>  {
>>  	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
>>  	struct page *ipage;
>> +	struct fscrypt_name fname;
>>  	void *inline_dentry = NULL;
>> -	int err;
>> +	int err = 0;
>>
>>  	if (!f2fs_has_inline_dentry(dir))
>>  		return 0;
>>
>>  	f2fs_lock_op(sbi);
>>
>> +	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &fname);
>> +	if (err)
>> +		goto out;
>> +
>>  	ipage = f2fs_get_node_page(sbi, dir->i_ino);
>> -	if (IS_ERR(ipage))
>> -		return PTR_ERR(ipage);
>> +	if (IS_ERR(ipage)) {
>> +		err = PTR_ERR(ipage);
>> +		goto out;
>> +	}
>> +
>> +	if (f2fs_has_enough_room(dir, ipage, &fname)) {
>> +		f2fs_put_page(ipage, 1);
>> +		goto out;
>> +	}
>>
>>  	inline_dentry = inline_data_addr(dir, ipage);
>>
>>  	err = do_convert_inline_dir(dir, ipage, inline_dentry);
>>  	if (!err)
>>  		f2fs_put_page(ipage, 1);
>> -
>> +out:
>>  	f2fs_unlock_op(sbi);
>>  	return err;
>>  }
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index 1daa54c8d8d8..856f0f984549 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -927,7 +927,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  	 * order to avoid this, let's do the check and update here.
>>  	 */
>>  	if (old_dir == new_dir && !new_inode) {
>> -		err = f2fs_convert_inline_dir(old_dir);
>> +		err = f2fs_try_convert_inline_dir(old_dir, new_dentry);
>>  		if (err)
>>  			return err;
>>  	}
>> -- 
>> 2.18.0.rc1
>>
>> Thanks,
>>
>>> +	if (old_dir == new_dir && !new_inode) {
>>> +		err = f2fs_convert_inline_dir(old_dir);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>> +
>>>  	if (flags & RENAME_WHITEOUT) {
>>>  		err = f2fs_create_whiteout(old_dir, &whiteout);
>>>  		if (err)
>>> @@ -954,28 +966,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  
>>>  		if (old_dir_entry)
>>>  			f2fs_i_links_write(new_dir, true);
>>> -
>>> -		/*
>>> -		 * old entry and new entry can locate in the same inline
>>> -		 * dentry in inode, when attaching new entry in inline dentry,
>>> -		 * it could force inline dentry conversion, after that,
>>> -		 * old_entry and old_page will point to wrong address, in
>>> -		 * order to avoid this, let's do the check and update here.
>>> -		 */
>>> -		if (is_old_inline && !f2fs_has_inline_dentry(old_dir)) {
>>> -			f2fs_put_page(old_page, 0);
>>> -			old_page = NULL;
>>> -
>>> -			old_entry = f2fs_find_entry(old_dir,
>>> -						&old_dentry->d_name, &old_page);
>>> -			if (!old_entry) {
>>> -				err = -ENOENT;
>>> -				if (IS_ERR(old_page))
>>> -					err = PTR_ERR(old_page);
>>> -				f2fs_unlock_op(sbi);
>>> -				goto out_dir;
>>> -			}
>>> -		}
>>>  	}
>>>  
>>>  	down_write(&F2FS_I(old_inode)->i_sem);
>>>
> .
> 

Re: [PATCH 1/4 v2] f2fs: convert inline_dir early before starting rename

[Jaegeuk Kim]

If we hit an error during rename, we'll get two dentries in different
directories.

Chao adds to check the room in inline_dir which can avoid needless
inversion. This should be done by inode_lock(&old_dir).

Signed-off-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c    | 14 ++++++++++++++
 fs/f2fs/f2fs.h   |  3 +++
 fs/f2fs/inline.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 fs/f2fs/namei.c  | 37 ++++++++++++++-----------------------
 4 files changed, 71 insertions(+), 25 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index c967cacf979e..b56f6060c1a6 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -578,6 +578,20 @@ int f2fs_room_for_filename(const void *bitmap, int slots, int max_slots)
 	goto next;
 }
 
+bool f2fs_has_enough_room(struct inode *dir, struct page *ipage,
+					struct fscrypt_name *fname)
+{
+	struct f2fs_dentry_ptr d;
+	unsigned int bit_pos;
+	int slots = GET_DENTRY_SLOTS(fname_len(fname));
+
+	make_dentry_ptr_inline(dir, &d, inline_data_addr(dir, ipage));
+
+	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
+
+	return bit_pos < d.max;
+}
+
 void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
 				const struct qstr *name, f2fs_hash_t name_hash,
 				unsigned int bit_pos)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 740e4f11bd1f..0164c8279037 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3121,6 +3121,8 @@ 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);
+bool f2fs_has_enough_room(struct inode *dir, struct page *ipage,
+			struct fscrypt_name *fname);
 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);
@@ -3663,6 +3665,7 @@ void f2fs_truncate_inline_inode(struct inode *inode,
 int f2fs_read_inline_data(struct inode *inode, struct page *page);
 int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page);
 int f2fs_convert_inline_inode(struct inode *inode);
+int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry);
 int f2fs_write_inline_data(struct inode *inode, struct page *page);
 bool f2fs_recover_inline_data(struct inode *inode, struct page *npage);
 struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir,
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 52f85ed07a15..4167e5408151 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -530,7 +530,7 @@ static int f2fs_move_rehashed_dirents(struct inode *dir, struct page *ipage,
 	return err;
 }
 
-static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
+static int do_convert_inline_dir(struct inode *dir, struct page *ipage,
 							void *inline_dentry)
 {
 	if (!F2FS_I(dir)->i_dir_level)
@@ -539,6 +539,44 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
 		return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry);
 }
 
+int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
+	struct page *ipage;
+	struct fscrypt_name fname;
+	void *inline_dentry = NULL;
+	int err = 0;
+
+	if (!f2fs_has_inline_dentry(dir))
+		return 0;
+
+	f2fs_lock_op(sbi);
+
+	err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &fname);
+	if (err)
+		goto out;
+
+	ipage = f2fs_get_node_page(sbi, dir->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto out;
+	}
+
+	if (f2fs_has_enough_room(dir, ipage, &fname)) {
+		f2fs_put_page(ipage, 1);
+		goto out;
+	}
+
+	inline_dentry = inline_data_addr(dir, ipage);
+
+	err = do_convert_inline_dir(dir, ipage, inline_dentry);
+	if (!err)
+		f2fs_put_page(ipage, 1);
+out:
+	f2fs_unlock_op(sbi);
+	return err;
+}
+
 int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
 				const struct qstr *orig_name,
 				struct inode *inode, nid_t ino, umode_t mode)
@@ -562,7 +600,7 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
 
 	bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max);
 	if (bit_pos >= d.max) {
-		err = f2fs_convert_inline_dir(dir, ipage, inline_dentry);
+		err = do_convert_inline_dir(dir, ipage, inline_dentry);
 		if (err)
 			return err;
 		err = -EAGAIN;
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index d479b91f9ca0..2aa035422c0f 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -906,7 +906,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct f2fs_dir_entry *old_dir_entry = NULL;
 	struct f2fs_dir_entry *old_entry;
 	struct f2fs_dir_entry *new_entry;
-	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
 	int err;
 
 	if (unlikely(f2fs_cp_error(sbi)))
@@ -919,6 +918,20 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			F2FS_I(old_dentry->d_inode)->i_projid)))
 		return -EXDEV;
 
+	/*
+	 * If new_inode is null, the below renaming flow will
+	 * add a link in old_dir which can conver inline_dir.
+	 * After then, if we failed to get the entry due to other
+	 * reasons like ENOMEM, we had to remove the new entry.
+	 * Instead of adding such the error handling routine, let's
+	 * simply convert first here.
+	 */
+	if (old_dir == new_dir && !new_inode) {
+		err = f2fs_try_convert_inline_dir(old_dir, new_dentry);
+		if (err)
+			return err;
+	}
+
 	if (flags & RENAME_WHITEOUT) {
 		err = f2fs_create_whiteout(old_dir, &whiteout);
 		if (err)
@@ -1006,28 +1019,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 		if (old_dir_entry)
 			f2fs_i_links_write(new_dir, true);
-
-		/*
-		 * old entry and new entry can locate in the same inline
-		 * dentry in inode, when attaching new entry in inline dentry,
-		 * it could force inline dentry conversion, after that,
-		 * old_entry and old_page will point to wrong address, in
-		 * order to avoid this, let's do the check and update here.
-		 */
-		if (is_old_inline && !f2fs_has_inline_dentry(old_dir)) {
-			f2fs_put_page(old_page, 0);
-			old_page = NULL;
-
-			old_entry = f2fs_find_entry(old_dir,
-						&old_dentry->d_name, &old_page);
-			if (!old_entry) {
-				err = -ENOENT;
-				if (IS_ERR(old_page))
-					err = PTR_ERR(old_page);
-				f2fs_unlock_op(sbi);
-				goto out_dir;
-			}
-		}
 	}
 
 	down_write(&F2FS_I(old_inode)->i_sem);
-- 
2.24.0.525.g8f36a354ae-goog