[f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Chao Yu]

Now if we call fsync() after we update the xattr date belongs to the file, f2fs
will do checkpoint to keep data.
This can cause low performance because checkpoint block most operation and write
lots of blocks. So we'd better to avoid doing checkpoint by writing modified
xattr node page to warm node segment, and then it can be recovered when we mount
this device later on.

Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
 fs/f2fs/f2fs.h     |  3 +--
 fs/f2fs/file.c     |  3 ---
 fs/f2fs/node.c     | 28 ++++++++++++++++------------
 fs/f2fs/node.h     |  2 +-
 fs/f2fs/recovery.c |  4 +++-
 fs/f2fs/xattr.c    |  3 ---
 6 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index c48847e..dfbdd64 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -301,7 +301,6 @@ struct f2fs_inode_info {
 	f2fs_hash_t chash;		/* hash value of given file name */
 	unsigned int clevel;		/* maximum level of given file name */
 	nid_t i_xattr_nid;		/* node id that contains xattrs */
-	unsigned long long xattr_ver;	/* cp version of xattr modification */
 	struct extent_info ext;		/* in-memory extent cache entry */
 	struct inode_entry *dirty_dir;	/* the pointer of dirty dir */
 
@@ -1391,7 +1390,7 @@ bool alloc_nid(struct f2fs_sb_info *, nid_t *);
 void alloc_nid_done(struct f2fs_sb_info *, nid_t);
 void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
 void recover_inline_xattr(struct inode *, struct page *);
-void recover_xattr_data(struct inode *, struct page *, block_t);
+int recover_xattr_data(struct inode *, struct page *, block_t);
 int recover_inode_page(struct f2fs_sb_info *, struct page *);
 int restore_node_summary(struct f2fs_sb_info *, unsigned int,
 				struct f2fs_summary_block *);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f172ddc4..fb26a43 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -129,8 +129,6 @@ static inline bool need_do_checkpoint(struct inode *inode)
 		need_cp = true;
 	else if (!is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
 		need_cp = true;
-	else if (F2FS_I(inode)->xattr_ver == cur_cp_version(F2FS_CKPT(sbi)))
-		need_cp = true;
 	else if (test_opt(sbi, FASTBOOT))
 		need_cp = true;
 	else if (sbi->active_logs == 2)
@@ -156,7 +154,6 @@ static void try_to_fix_pino(struct inode *inode)
 	nid_t pino;
 
 	down_write(&fi->i_sem);
-	fi->xattr_ver = 0;
 	if (file_wrong_pino(inode) && inode->i_nlink == 1 &&
 			get_parent_ino(inode, &pino)) {
 		fi->i_pino = pino;
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index a7cb0db..675fa65 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -863,9 +863,6 @@ int truncate_xattr_node(struct inode *inode, struct page *page)
 
 	F2FS_I(inode)->i_xattr_nid = 0;
 
-	/* need to do checkpoint during fsync */
-	F2FS_I(inode)->xattr_ver = cur_cp_version(F2FS_CKPT(sbi));
-
 	set_new_dnode(&dn, inode, page, npage, nid);
 
 	if (page)
@@ -1655,12 +1652,13 @@ update_inode:
 	f2fs_put_page(ipage, 1);
 }
 
-void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
+int recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
 	nid_t new_xnid = nid_of_node(page);
 	struct node_info ni;
+	struct page *xpage;
 
 	/* 1: invalidate the previous xattr nid */
 	if (!prev_xnid)
@@ -1674,21 +1672,27 @@ void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
 	set_node_addr(sbi, &ni, NULL_ADDR, false);
 
 recover_xnid:
-	/* 2: allocate new xattr nid */
+	/* 2: update xattr nid in inode */
+	remove_free_nid(NM_I(sbi), new_xnid);
+	F2FS_I(inode)->i_xattr_nid = new_xnid;
 	if (unlikely(!inc_valid_node_count(sbi, inode)))
 		f2fs_bug_on(sbi, 1);
+	update_inode_page(inode);
+
+	/* 3: update and make xattr node page dirty */
+	xpage = grab_cache_page(NODE_MAPPING(sbi), new_xnid);
+	if (!xpage)
+		return -ENOMEM;
+
+	memcpy(F2FS_NODE(xpage), F2FS_NODE(page), PAGE_CACHE_SIZE);
 
-	remove_free_nid(NM_I(sbi), new_xnid);
 	get_node_info(sbi, new_xnid, &ni);
 	ni.ino = inode->i_ino;
 	set_node_addr(sbi, &ni, NEW_ADDR, false);
-	F2FS_I(inode)->i_xattr_nid = new_xnid;
+	set_page_dirty(xpage);
+	f2fs_put_page(xpage, 1);
 
-	/* 3: update xattr blkaddr */
-	refresh_sit_entry(sbi, NEW_ADDR, blkaddr);
-	set_node_addr(sbi, &ni, blkaddr, false);
-
-	update_inode_page(inode);
+	return 0;
 }
 
 int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
index cac8a3d..55eb1e7 100644
--- a/fs/f2fs/node.h
+++ b/fs/f2fs/node.h
@@ -300,7 +300,7 @@ static inline bool IS_DNODE(struct page *node_page)
 	unsigned int ofs = ofs_of_node(node_page);
 
 	if (f2fs_has_xattr_block(ofs))
-		return false;
+		return true;
 
 	if (ofs == 3 || ofs == 4 + NIDS_PER_BLOCK ||
 			ofs == 5 + 2 * NIDS_PER_BLOCK)
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 9160a37..4f2c197 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -346,7 +346,9 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	if (IS_INODE(page)) {
 		recover_inline_xattr(inode, page);
 	} else if (f2fs_has_xattr_block(ofs_of_node(page))) {
-		recover_xattr_data(inode, page, blkaddr);
+		err = recover_xattr_data(inode, page, blkaddr);
+		if (!err)
+			recovered++;
 		goto out;
 	}
 
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 5072bf9..3d13ca2 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -391,9 +391,6 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
 						sizeof(struct node_footer));
 	set_page_dirty(xpage);
 	f2fs_put_page(xpage, 1);
-
-	/* need to checkpoint during fsync */
-	F2FS_I(inode)->xattr_ver = cur_cp_version(F2FS_CKPT(sbi));
 	return 0;
 }
 
-- 
2.2.1

Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Jaegeuk Kim]

Hi Chao,

On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> will do checkpoint to keep data.
> This can cause low performance because checkpoint block most operation and write
> lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> xattr node page to warm node segment, and then it can be recovered when we mount
> this device later on.

You're trying to change the writing policy as xattr blocks are written into
WARM_NODE area instead of COLD_NODE area.
I don't think xattrs are frequently changed between each fsync calls.

How do you think?

Thanks,

> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  fs/f2fs/f2fs.h     |  3 +--
>  fs/f2fs/file.c     |  3 ---
>  fs/f2fs/node.c     | 28 ++++++++++++++++------------
>  fs/f2fs/node.h     |  2 +-
>  fs/f2fs/recovery.c |  4 +++-
>  fs/f2fs/xattr.c    |  3 ---
>  6 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index c48847e..dfbdd64 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -301,7 +301,6 @@ struct f2fs_inode_info {
>  	f2fs_hash_t chash;		/* hash value of given file name */
>  	unsigned int clevel;		/* maximum level of given file name */
>  	nid_t i_xattr_nid;		/* node id that contains xattrs */
> -	unsigned long long xattr_ver;	/* cp version of xattr modification */
>  	struct extent_info ext;		/* in-memory extent cache entry */
>  	struct inode_entry *dirty_dir;	/* the pointer of dirty dir */
>  
> @@ -1391,7 +1390,7 @@ bool alloc_nid(struct f2fs_sb_info *, nid_t *);
>  void alloc_nid_done(struct f2fs_sb_info *, nid_t);
>  void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
>  void recover_inline_xattr(struct inode *, struct page *);
> -void recover_xattr_data(struct inode *, struct page *, block_t);
> +int recover_xattr_data(struct inode *, struct page *, block_t);
>  int recover_inode_page(struct f2fs_sb_info *, struct page *);
>  int restore_node_summary(struct f2fs_sb_info *, unsigned int,
>  				struct f2fs_summary_block *);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index f172ddc4..fb26a43 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -129,8 +129,6 @@ static inline bool need_do_checkpoint(struct inode *inode)
>  		need_cp = true;
>  	else if (!is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
>  		need_cp = true;
> -	else if (F2FS_I(inode)->xattr_ver == cur_cp_version(F2FS_CKPT(sbi)))
> -		need_cp = true;
>  	else if (test_opt(sbi, FASTBOOT))
>  		need_cp = true;
>  	else if (sbi->active_logs == 2)
> @@ -156,7 +154,6 @@ static void try_to_fix_pino(struct inode *inode)
>  	nid_t pino;
>  
>  	down_write(&fi->i_sem);
> -	fi->xattr_ver = 0;
>  	if (file_wrong_pino(inode) && inode->i_nlink == 1 &&
>  			get_parent_ino(inode, &pino)) {
>  		fi->i_pino = pino;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index a7cb0db..675fa65 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -863,9 +863,6 @@ int truncate_xattr_node(struct inode *inode, struct page *page)
>  
>  	F2FS_I(inode)->i_xattr_nid = 0;
>  
> -	/* need to do checkpoint during fsync */
> -	F2FS_I(inode)->xattr_ver = cur_cp_version(F2FS_CKPT(sbi));
> -
>  	set_new_dnode(&dn, inode, page, npage, nid);
>  
>  	if (page)
> @@ -1655,12 +1652,13 @@ update_inode:
>  	f2fs_put_page(ipage, 1);
>  }
>  
> -void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
> +int recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
>  	nid_t new_xnid = nid_of_node(page);
>  	struct node_info ni;
> +	struct page *xpage;
>  
>  	/* 1: invalidate the previous xattr nid */
>  	if (!prev_xnid)
> @@ -1674,21 +1672,27 @@ void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
>  	set_node_addr(sbi, &ni, NULL_ADDR, false);
>  
>  recover_xnid:
> -	/* 2: allocate new xattr nid */
> +	/* 2: update xattr nid in inode */
> +	remove_free_nid(NM_I(sbi), new_xnid);
> +	F2FS_I(inode)->i_xattr_nid = new_xnid;
>  	if (unlikely(!inc_valid_node_count(sbi, inode)))
>  		f2fs_bug_on(sbi, 1);
> +	update_inode_page(inode);
> +
> +	/* 3: update and make xattr node page dirty */
> +	xpage = grab_cache_page(NODE_MAPPING(sbi), new_xnid);
> +	if (!xpage)
> +		return -ENOMEM;
> +
> +	memcpy(F2FS_NODE(xpage), F2FS_NODE(page), PAGE_CACHE_SIZE);
>  
> -	remove_free_nid(NM_I(sbi), new_xnid);
>  	get_node_info(sbi, new_xnid, &ni);
>  	ni.ino = inode->i_ino;
>  	set_node_addr(sbi, &ni, NEW_ADDR, false);
> -	F2FS_I(inode)->i_xattr_nid = new_xnid;
> +	set_page_dirty(xpage);
> +	f2fs_put_page(xpage, 1);
>  
> -	/* 3: update xattr blkaddr */
> -	refresh_sit_entry(sbi, NEW_ADDR, blkaddr);
> -	set_node_addr(sbi, &ni, blkaddr, false);
> -
> -	update_inode_page(inode);
> +	return 0;
>  }
>  
>  int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index cac8a3d..55eb1e7 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -300,7 +300,7 @@ static inline bool IS_DNODE(struct page *node_page)
>  	unsigned int ofs = ofs_of_node(node_page);
>  
>  	if (f2fs_has_xattr_block(ofs))
> -		return false;
> +		return true;
>  
>  	if (ofs == 3 || ofs == 4 + NIDS_PER_BLOCK ||
>  			ofs == 5 + 2 * NIDS_PER_BLOCK)
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 9160a37..4f2c197 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -346,7 +346,9 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>  	if (IS_INODE(page)) {
>  		recover_inline_xattr(inode, page);
>  	} else if (f2fs_has_xattr_block(ofs_of_node(page))) {
> -		recover_xattr_data(inode, page, blkaddr);
> +		err = recover_xattr_data(inode, page, blkaddr);
> +		if (!err)
> +			recovered++;
>  		goto out;
>  	}
>  
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 5072bf9..3d13ca2 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -391,9 +391,6 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>  						sizeof(struct node_footer));
>  	set_page_dirty(xpage);
>  	f2fs_put_page(xpage, 1);
> -
> -	/* need to checkpoint during fsync */
> -	F2FS_I(inode)->xattr_ver = cur_cp_version(F2FS_CKPT(sbi));
>  	return 0;
>  }
>  
> -- 
> 2.2.1

Re: [PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Jaegeuk Kim]

Hi Chao,

On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> will do checkpoint to keep data.
> This can cause low performance because checkpoint block most operation and write
> lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> xattr node page to warm node segment, and then it can be recovered when we mount
> this device later on.

You're trying to change the writing policy as xattr blocks are written into
WARM_NODE area instead of COLD_NODE area.
I don't think xattrs are frequently changed between each fsync calls.

How do you think?

Thanks,

> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  fs/f2fs/f2fs.h     |  3 +--
>  fs/f2fs/file.c     |  3 ---
>  fs/f2fs/node.c     | 28 ++++++++++++++++------------
>  fs/f2fs/node.h     |  2 +-
>  fs/f2fs/recovery.c |  4 +++-
>  fs/f2fs/xattr.c    |  3 ---
>  6 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index c48847e..dfbdd64 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -301,7 +301,6 @@ struct f2fs_inode_info {
>  	f2fs_hash_t chash;		/* hash value of given file name */
>  	unsigned int clevel;		/* maximum level of given file name */
>  	nid_t i_xattr_nid;		/* node id that contains xattrs */
> -	unsigned long long xattr_ver;	/* cp version of xattr modification */
>  	struct extent_info ext;		/* in-memory extent cache entry */
>  	struct inode_entry *dirty_dir;	/* the pointer of dirty dir */
>  
> @@ -1391,7 +1390,7 @@ bool alloc_nid(struct f2fs_sb_info *, nid_t *);
>  void alloc_nid_done(struct f2fs_sb_info *, nid_t);
>  void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
>  void recover_inline_xattr(struct inode *, struct page *);
> -void recover_xattr_data(struct inode *, struct page *, block_t);
> +int recover_xattr_data(struct inode *, struct page *, block_t);
>  int recover_inode_page(struct f2fs_sb_info *, struct page *);
>  int restore_node_summary(struct f2fs_sb_info *, unsigned int,
>  				struct f2fs_summary_block *);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index f172ddc4..fb26a43 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -129,8 +129,6 @@ static inline bool need_do_checkpoint(struct inode *inode)
>  		need_cp = true;
>  	else if (!is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
>  		need_cp = true;
> -	else if (F2FS_I(inode)->xattr_ver == cur_cp_version(F2FS_CKPT(sbi)))
> -		need_cp = true;
>  	else if (test_opt(sbi, FASTBOOT))
>  		need_cp = true;
>  	else if (sbi->active_logs == 2)
> @@ -156,7 +154,6 @@ static void try_to_fix_pino(struct inode *inode)
>  	nid_t pino;
>  
>  	down_write(&fi->i_sem);
> -	fi->xattr_ver = 0;
>  	if (file_wrong_pino(inode) && inode->i_nlink == 1 &&
>  			get_parent_ino(inode, &pino)) {
>  		fi->i_pino = pino;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index a7cb0db..675fa65 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -863,9 +863,6 @@ int truncate_xattr_node(struct inode *inode, struct page *page)
>  
>  	F2FS_I(inode)->i_xattr_nid = 0;
>  
> -	/* need to do checkpoint during fsync */
> -	F2FS_I(inode)->xattr_ver = cur_cp_version(F2FS_CKPT(sbi));
> -
>  	set_new_dnode(&dn, inode, page, npage, nid);
>  
>  	if (page)
> @@ -1655,12 +1652,13 @@ update_inode:
>  	f2fs_put_page(ipage, 1);
>  }
>  
> -void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
> +int recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
>  	nid_t new_xnid = nid_of_node(page);
>  	struct node_info ni;
> +	struct page *xpage;
>  
>  	/* 1: invalidate the previous xattr nid */
>  	if (!prev_xnid)
> @@ -1674,21 +1672,27 @@ void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
>  	set_node_addr(sbi, &ni, NULL_ADDR, false);
>  
>  recover_xnid:
> -	/* 2: allocate new xattr nid */
> +	/* 2: update xattr nid in inode */
> +	remove_free_nid(NM_I(sbi), new_xnid);
> +	F2FS_I(inode)->i_xattr_nid = new_xnid;
>  	if (unlikely(!inc_valid_node_count(sbi, inode)))
>  		f2fs_bug_on(sbi, 1);
> +	update_inode_page(inode);
> +
> +	/* 3: update and make xattr node page dirty */
> +	xpage = grab_cache_page(NODE_MAPPING(sbi), new_xnid);
> +	if (!xpage)
> +		return -ENOMEM;
> +
> +	memcpy(F2FS_NODE(xpage), F2FS_NODE(page), PAGE_CACHE_SIZE);
>  
> -	remove_free_nid(NM_I(sbi), new_xnid);
>  	get_node_info(sbi, new_xnid, &ni);
>  	ni.ino = inode->i_ino;
>  	set_node_addr(sbi, &ni, NEW_ADDR, false);
> -	F2FS_I(inode)->i_xattr_nid = new_xnid;
> +	set_page_dirty(xpage);
> +	f2fs_put_page(xpage, 1);
>  
> -	/* 3: update xattr blkaddr */
> -	refresh_sit_entry(sbi, NEW_ADDR, blkaddr);
> -	set_node_addr(sbi, &ni, blkaddr, false);
> -
> -	update_inode_page(inode);
> +	return 0;
>  }
>  
>  int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index cac8a3d..55eb1e7 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -300,7 +300,7 @@ static inline bool IS_DNODE(struct page *node_page)
>  	unsigned int ofs = ofs_of_node(node_page);
>  
>  	if (f2fs_has_xattr_block(ofs))
> -		return false;
> +		return true;
>  
>  	if (ofs == 3 || ofs == 4 + NIDS_PER_BLOCK ||
>  			ofs == 5 + 2 * NIDS_PER_BLOCK)
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 9160a37..4f2c197 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -346,7 +346,9 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>  	if (IS_INODE(page)) {
>  		recover_inline_xattr(inode, page);
>  	} else if (f2fs_has_xattr_block(ofs_of_node(page))) {
> -		recover_xattr_data(inode, page, blkaddr);
> +		err = recover_xattr_data(inode, page, blkaddr);
> +		if (!err)
> +			recovered++;
>  		goto out;
>  	}
>  
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 5072bf9..3d13ca2 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -391,9 +391,6 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
>  						sizeof(struct node_footer));
>  	set_page_dirty(xpage);
>  	f2fs_put_page(xpage, 1);
> -
> -	/* need to checkpoint during fsync */
> -	F2FS_I(inode)->xattr_ver = cur_cp_version(F2FS_CKPT(sbi));
>  	return 0;
>  }
>  
> -- 
> 2.2.1

------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net

RE: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, January 07, 2015 3:44 AM
> To: Chao Yu
> Cc: Changman Lee; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after
> operating xattr
> 
> Hi Chao,
> 
> On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> > Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> > will do checkpoint to keep data.
> > This can cause low performance because checkpoint block most operation and write
> > lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> > xattr node page to warm node segment, and then it can be recovered when we mount
> > this device later on.
> 
> You're trying to change the writing policy as xattr blocks are written into
> WARM_NODE area instead of COLD_NODE area.
> I don't think xattrs are frequently changed between each fsync calls.
> 
> How do you think?

I'm not sure whether there is a scenario that setxattr and fsync are invoked
alternately, but if there is, our performance will decrease obviously.

If you don't want to change writing policy, how about writing xattr node with
fsync flag into cold node segment when fsync() is called, then try to recover
it from cold node chain when recovery after abnormally pow-cut, this way can
avoid cp frequently in above scenario.

Thanks,
Yu

Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Jaegeuk Kim]

On Sat, Jan 10, 2015 at 08:08:33PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Wednesday, January 07, 2015 3:44 AM
> > To: Chao Yu
> > Cc: Changman Lee; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after
> > operating xattr
> > 
> > Hi Chao,
> > 
> > On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> > > Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> > > will do checkpoint to keep data.
> > > This can cause low performance because checkpoint block most operation and write
> > > lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> > > xattr node page to warm node segment, and then it can be recovered when we mount
> > > this device later on.
> > 
> > You're trying to change the writing policy as xattr blocks are written into
> > WARM_NODE area instead of COLD_NODE area.
> > I don't think xattrs are frequently changed between each fsync calls.
> > 
> > How do you think?
> 
> I'm not sure whether there is a scenario that setxattr and fsync are invoked
> alternately, but if there is, our performance will decrease obviously.
> 
> If you don't want to change writing policy, how about writing xattr node with
> fsync flag into cold node segment when fsync() is called, then try to recover
> it from cold node chain when recovery after abnormally pow-cut, this way can
> avoid cp frequently in above scenario.

Firt of all, I don't think this scenario is frequent enough that we have to
break the exisiting writing and recovery procedures.
Moreover, if xattr entries are covered by inline_xattr, it doesn't trigger
checkpoint.

Let me know, if I'm missing something.

If you try to change the recovery procedure, it needs to think about the
disk full condition. (i.e., space_for_roll_forward())
And, I don't want to search cold node chain.

Thanks,

> 
> Thanks,
> Yu

RE: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Sunday, January 11, 2015 1:32 PM
> To: Chao Yu
> Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after
> operating xattr
> 
> On Sat, Jan 10, 2015 at 08:08:33PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Wednesday, January 07, 2015 3:44 AM
> > > To: Chao Yu
> > > Cc: Changman Lee; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync
> after
> > > operating xattr
> > >
> > > Hi Chao,
> > >
> > > On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> > > > Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> > > > will do checkpoint to keep data.
> > > > This can cause low performance because checkpoint block most operation and write
> > > > lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> > > > xattr node page to warm node segment, and then it can be recovered when we mount
> > > > this device later on.
> > >
> > > You're trying to change the writing policy as xattr blocks are written into
> > > WARM_NODE area instead of COLD_NODE area.
> > > I don't think xattrs are frequently changed between each fsync calls.
> > >
> > > How do you think?
> >
> > I'm not sure whether there is a scenario that setxattr and fsync are invoked
> > alternately, but if there is, our performance will decrease obviously.
> >
> > If you don't want to change writing policy, how about writing xattr node with
> > fsync flag into cold node segment when fsync() is called, then try to recover
> > it from cold node chain when recovery after abnormally pow-cut, this way can
> > avoid cp frequently in above scenario.
> 
> Firt of all, I don't think this scenario is frequent enough that we have to
> break the exisiting writing and recovery procedures.
> Moreover, if xattr entries are covered by inline_xattr, it doesn't trigger
> checkpoint.

Agree, that's a good solution.

> 
> Let me know, if I'm missing something.
> 
> If you try to change the recovery procedure, it needs to think about the
> disk full condition. (i.e., space_for_roll_forward())
> And, I don't want to search cold node chain.

OK, if we keep writing policy and recovery procedure as it is, then, shouldn't our
recover_xattr_data be dropped because it will be not used from any call path?
How do you think of below patch?

>From 4fee238292aa0198a1530492afbd529e84e83141 Mon Sep 17 00:00:00 2001
From: Chao Yu <chao2.yu@samsung.com>
Date: Mon, 12 Jan 2015 17:25:44 +0800
Subject: [PATCH] f2fs: drop recover_xattr_data

Because xattr node will be written to cold node segment, so in current procedure
it's wrong that we try to find and recover it from warm node chain in
recover_xattr_data.

Let's drop this related function and invoking.

Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
 fs/f2fs/f2fs.h     |  1 -
 fs/f2fs/node.c     | 36 ------------------------------------
 fs/f2fs/recovery.c |  8 ++------
 3 files changed, 2 insertions(+), 43 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 209532c..3b18072 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1392,7 +1392,6 @@ bool alloc_nid(struct f2fs_sb_info *, nid_t *);
 void alloc_nid_done(struct f2fs_sb_info *, nid_t);
 void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
 void recover_inline_xattr(struct inode *, struct page *);
-void recover_xattr_data(struct inode *, struct page *, block_t);
 int recover_inode_page(struct f2fs_sb_info *, struct page *);
 int restore_node_summary(struct f2fs_sb_info *, unsigned int,
 				struct f2fs_summary_block *);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index d7c1436..75aa233 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1656,42 +1656,6 @@ update_inode:
 	f2fs_put_page(ipage, 1);
 }
 
-void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
-{
-	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
-	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
-	nid_t new_xnid = nid_of_node(page);
-	struct node_info ni;
-
-	/* 1: invalidate the previous xattr nid */
-	if (!prev_xnid)
-		goto recover_xnid;
-
-	/* Deallocate node address */
-	get_node_info(sbi, prev_xnid, &ni);
-	f2fs_bug_on(sbi, ni.blk_addr == NULL_ADDR);
-	invalidate_blocks(sbi, ni.blk_addr);
-	dec_valid_node_count(sbi, inode);
-	set_node_addr(sbi, &ni, NULL_ADDR, false);
-
-recover_xnid:
-	/* 2: allocate new xattr nid */
-	if (unlikely(!inc_valid_node_count(sbi, inode)))
-		f2fs_bug_on(sbi, 1);
-
-	remove_free_nid(NM_I(sbi), new_xnid);
-	get_node_info(sbi, new_xnid, &ni);
-	ni.ino = inode->i_ino;
-	set_node_addr(sbi, &ni, NEW_ADDR, false);
-	F2FS_I(inode)->i_xattr_nid = new_xnid;
-
-	/* 3: update xattr blkaddr */
-	refresh_sit_entry(sbi, NEW_ADDR, blkaddr);
-	set_node_addr(sbi, &ni, blkaddr, false);
-
-	update_inode_page(inode);
-}
-
 int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
 {
 	struct f2fs_inode *src, *dst;
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index c4211a5..f70c2e7 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -342,13 +342,9 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	struct node_info ni;
 	int err = 0, recovered = 0;
 
-	/* step 1: recover xattr */
-	if (IS_INODE(page)) {
+	/* step 1: recover inline xattr */
+	if (IS_INODE(page))
 		recover_inline_xattr(inode, page);
-	} else if (f2fs_has_xattr_block(ofs_of_node(page))) {
-		recover_xattr_data(inode, page, blkaddr);
-		goto out;
-	}
 
 	/* step 2: recover inline data */
 	if (recover_inline_data(inode, page))
-- 
2.2.1


> 
> Thanks,
> 
> >
> > Thanks,
> > Yu

Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Jaegeuk Kim]

On Mon, Jan 12, 2015 at 05:40:28PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Sunday, January 11, 2015 1:32 PM
> > To: Chao Yu
> > Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after
> > operating xattr
> > 
> > On Sat, Jan 10, 2015 at 08:08:33PM +0800, Chao Yu wrote:
> > > Hi Jaegeuk,
> > >
> > > > -----Original Message-----
> > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > Sent: Wednesday, January 07, 2015 3:44 AM
> > > > To: Chao Yu
> > > > Cc: Changman Lee; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync
> > after
> > > > operating xattr
> > > >
> > > > Hi Chao,
> > > >
> > > > On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> > > > > Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> > > > > will do checkpoint to keep data.
> > > > > This can cause low performance because checkpoint block most operation and write
> > > > > lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> > > > > xattr node page to warm node segment, and then it can be recovered when we mount
> > > > > this device later on.
> > > >
> > > > You're trying to change the writing policy as xattr blocks are written into
> > > > WARM_NODE area instead of COLD_NODE area.
> > > > I don't think xattrs are frequently changed between each fsync calls.
> > > >
> > > > How do you think?
> > >
> > > I'm not sure whether there is a scenario that setxattr and fsync are invoked
> > > alternately, but if there is, our performance will decrease obviously.
> > >
> > > If you don't want to change writing policy, how about writing xattr node with
> > > fsync flag into cold node segment when fsync() is called, then try to recover
> > > it from cold node chain when recovery after abnormally pow-cut, this way can
> > > avoid cp frequently in above scenario.
> > 
> > Firt of all, I don't think this scenario is frequent enough that we have to
> > break the exisiting writing and recovery procedures.
> > Moreover, if xattr entries are covered by inline_xattr, it doesn't trigger
> > checkpoint.
> 
> Agree, that's a good solution.
> 
> > 
> > Let me know, if I'm missing something.
> > 
> > If you try to change the recovery procedure, it needs to think about the
> > disk full condition. (i.e., space_for_roll_forward())
> > And, I don't want to search cold node chain.
> 
> OK, if we keep writing policy and recovery procedure as it is, then, shouldn't our
> recover_xattr_data be dropped because it will be not used from any call path?
> How do you think of below patch?

Hi Chao,

Nice catch.
But, IIRC, this code was remained for backward compatibility, since long time
ago, xattr blocks were written into the warm node chain.
So, I couldn't remove this.

Thanks,

> 
> >From 4fee238292aa0198a1530492afbd529e84e83141 Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao2.yu@samsung.com>
> Date: Mon, 12 Jan 2015 17:25:44 +0800
> Subject: [PATCH] f2fs: drop recover_xattr_data
> 
> Because xattr node will be written to cold node segment, so in current procedure
> it's wrong that we try to find and recover it from warm node chain in
> recover_xattr_data.
> 
> Let's drop this related function and invoking.
> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  fs/f2fs/f2fs.h     |  1 -
>  fs/f2fs/node.c     | 36 ------------------------------------
>  fs/f2fs/recovery.c |  8 ++------
>  3 files changed, 2 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 209532c..3b18072 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1392,7 +1392,6 @@ bool alloc_nid(struct f2fs_sb_info *, nid_t *);
>  void alloc_nid_done(struct f2fs_sb_info *, nid_t);
>  void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
>  void recover_inline_xattr(struct inode *, struct page *);
> -void recover_xattr_data(struct inode *, struct page *, block_t);
>  int recover_inode_page(struct f2fs_sb_info *, struct page *);
>  int restore_node_summary(struct f2fs_sb_info *, unsigned int,
>  				struct f2fs_summary_block *);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index d7c1436..75aa233 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1656,42 +1656,6 @@ update_inode:
>  	f2fs_put_page(ipage, 1);
>  }
>  
> -void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
> -{
> -	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> -	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
> -	nid_t new_xnid = nid_of_node(page);
> -	struct node_info ni;
> -
> -	/* 1: invalidate the previous xattr nid */
> -	if (!prev_xnid)
> -		goto recover_xnid;
> -
> -	/* Deallocate node address */
> -	get_node_info(sbi, prev_xnid, &ni);
> -	f2fs_bug_on(sbi, ni.blk_addr == NULL_ADDR);
> -	invalidate_blocks(sbi, ni.blk_addr);
> -	dec_valid_node_count(sbi, inode);
> -	set_node_addr(sbi, &ni, NULL_ADDR, false);
> -
> -recover_xnid:
> -	/* 2: allocate new xattr nid */
> -	if (unlikely(!inc_valid_node_count(sbi, inode)))
> -		f2fs_bug_on(sbi, 1);
> -
> -	remove_free_nid(NM_I(sbi), new_xnid);
> -	get_node_info(sbi, new_xnid, &ni);
> -	ni.ino = inode->i_ino;
> -	set_node_addr(sbi, &ni, NEW_ADDR, false);
> -	F2FS_I(inode)->i_xattr_nid = new_xnid;
> -
> -	/* 3: update xattr blkaddr */
> -	refresh_sit_entry(sbi, NEW_ADDR, blkaddr);
> -	set_node_addr(sbi, &ni, blkaddr, false);
> -
> -	update_inode_page(inode);
> -}
> -
>  int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
>  {
>  	struct f2fs_inode *src, *dst;
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index c4211a5..f70c2e7 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -342,13 +342,9 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>  	struct node_info ni;
>  	int err = 0, recovered = 0;
>  
> -	/* step 1: recover xattr */
> -	if (IS_INODE(page)) {
> +	/* step 1: recover inline xattr */
> +	if (IS_INODE(page))
>  		recover_inline_xattr(inode, page);
> -	} else if (f2fs_has_xattr_block(ofs_of_node(page))) {
> -		recover_xattr_data(inode, page, blkaddr);
> -		goto out;
> -	}
>  
>  	/* step 2: recover inline data */
>  	if (recover_inline_data(inode, page))
> -- 
> 2.2.1
> 
> 
> > 
> > Thanks,
> > 
> > >
> > > Thanks,
> > > Yu

RE: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, January 14, 2015 8:22 AM
> To: Chao Yu
> Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after
> operating xattr
> 
> On Mon, Jan 12, 2015 at 05:40:28PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Sunday, January 11, 2015 1:32 PM
> > > To: Chao Yu
> > > Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync
> after
> > > operating xattr
> > >
> > > On Sat, Jan 10, 2015 at 08:08:33PM +0800, Chao Yu wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > > Sent: Wednesday, January 07, 2015 3:44 AM
> > > > > To: Chao Yu
> > > > > Cc: Changman Lee; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > > > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when
> fsync
> > > after
> > > > > operating xattr
> > > > >
> > > > > Hi Chao,
> > > > >
> > > > > On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> > > > > > Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> > > > > > will do checkpoint to keep data.
> > > > > > This can cause low performance because checkpoint block most operation and write
> > > > > > lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> > > > > > xattr node page to warm node segment, and then it can be recovered when we mount
> > > > > > this device later on.
> > > > >
> > > > > You're trying to change the writing policy as xattr blocks are written into
> > > > > WARM_NODE area instead of COLD_NODE area.
> > > > > I don't think xattrs are frequently changed between each fsync calls.
> > > > >
> > > > > How do you think?
> > > >
> > > > I'm not sure whether there is a scenario that setxattr and fsync are invoked
> > > > alternately, but if there is, our performance will decrease obviously.
> > > >
> > > > If you don't want to change writing policy, how about writing xattr node with
> > > > fsync flag into cold node segment when fsync() is called, then try to recover
> > > > it from cold node chain when recovery after abnormally pow-cut, this way can
> > > > avoid cp frequently in above scenario.
> > >
> > > Firt of all, I don't think this scenario is frequent enough that we have to
> > > break the exisiting writing and recovery procedures.
> > > Moreover, if xattr entries are covered by inline_xattr, it doesn't trigger
> > > checkpoint.
> >
> > Agree, that's a good solution.
> >
> > >
> > > Let me know, if I'm missing something.
> > >
> > > If you try to change the recovery procedure, it needs to think about the
> > > disk full condition. (i.e., space_for_roll_forward())
> > > And, I don't want to search cold node chain.
> >
> > OK, if we keep writing policy and recovery procedure as it is, then, shouldn't our
> > recover_xattr_data be dropped because it will be not used from any call path?
> > How do you think of below patch?
> 
> Hi Chao,
> 
> Nice catch.
> But, IIRC, this code was remained for backward compatibility, since long time
> ago, xattr blocks were written into the warm node chain.

Ah, I got it, thanks for your explanation! :)
How do you think of adding some comments on these codes, because this can help
developers understand it well and not to submit the wrong fix patch like me again.

Thanks,

> So, I couldn't remove this.
> 
> Thanks,

Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Jaegeuk Kim]

Hi Chao,

On Wed, Jan 14, 2015 at 01:01:13PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Wednesday, January 14, 2015 8:22 AM
> > To: Chao Yu
> > Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after
> > operating xattr
> > 
> > On Mon, Jan 12, 2015 at 05:40:28PM +0800, Chao Yu wrote:
> > > Hi Jaegeuk,
> > >
> > > > -----Original Message-----
> > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > Sent: Sunday, January 11, 2015 1:32 PM
> > > > To: Chao Yu
> > > > Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync
> > after
> > > > operating xattr
> > > >
> > > > On Sat, Jan 10, 2015 at 08:08:33PM +0800, Chao Yu wrote:
> > > > > Hi Jaegeuk,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > > > Sent: Wednesday, January 07, 2015 3:44 AM
> > > > > > To: Chao Yu
> > > > > > Cc: Changman Lee; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > > > > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when
> > fsync
> > > > after
> > > > > > operating xattr
> > > > > >
> > > > > > Hi Chao,
> > > > > >
> > > > > > On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> > > > > > > Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> > > > > > > will do checkpoint to keep data.
> > > > > > > This can cause low performance because checkpoint block most operation and write
> > > > > > > lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> > > > > > > xattr node page to warm node segment, and then it can be recovered when we mount
> > > > > > > this device later on.
> > > > > >
> > > > > > You're trying to change the writing policy as xattr blocks are written into
> > > > > > WARM_NODE area instead of COLD_NODE area.
> > > > > > I don't think xattrs are frequently changed between each fsync calls.
> > > > > >
> > > > > > How do you think?
> > > > >
> > > > > I'm not sure whether there is a scenario that setxattr and fsync are invoked
> > > > > alternately, but if there is, our performance will decrease obviously.
> > > > >
> > > > > If you don't want to change writing policy, how about writing xattr node with
> > > > > fsync flag into cold node segment when fsync() is called, then try to recover
> > > > > it from cold node chain when recovery after abnormally pow-cut, this way can
> > > > > avoid cp frequently in above scenario.
> > > >
> > > > Firt of all, I don't think this scenario is frequent enough that we have to
> > > > break the exisiting writing and recovery procedures.
> > > > Moreover, if xattr entries are covered by inline_xattr, it doesn't trigger
> > > > checkpoint.
> > >
> > > Agree, that's a good solution.
> > >
> > > >
> > > > Let me know, if I'm missing something.
> > > >
> > > > If you try to change the recovery procedure, it needs to think about the
> > > > disk full condition. (i.e., space_for_roll_forward())
> > > > And, I don't want to search cold node chain.
> > >
> > > OK, if we keep writing policy and recovery procedure as it is, then, shouldn't our
> > > recover_xattr_data be dropped because it will be not used from any call path?
> > > How do you think of below patch?
> > 
> > Hi Chao,
> > 
> > Nice catch.
> > But, IIRC, this code was remained for backward compatibility, since long time
> > ago, xattr blocks were written into the warm node chain.
> 
> Ah, I got it, thanks for your explanation! :)
> How do you think of adding some comments on these codes, because this can help
> developers understand it well and not to submit the wrong fix patch like me again.

Something like this?

>From 6b609421e4f9f52de26554300aae62de33e0703a Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Thu, 22 Jan 2015 14:48:28 -0800
Subject: [PATCH] f2fs: leave comment for code readability

During the recovery, any xattr blocks should not be found, since they are
written into cold log, not the warm node chain.

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

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index c4211a5..57603a7 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -346,6 +346,10 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	if (IS_INODE(page)) {
 		recover_inline_xattr(inode, page);
 	} else if (f2fs_has_xattr_block(ofs_of_node(page))) {
+		/*
+		 * Deprecated; xattr blocks should be found from cold log.
+		 * But, we should remain this for backward compatibility.
+		 */
 		recover_xattr_data(inode, page, blkaddr);
 		goto out;
 	}
-- 
2.1.1

RE: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after operating xattr

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, January 23, 2015 7:13 AM
> To: Chao Yu
> Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after
> operating xattr
> 
> Hi Chao,
> 
> On Wed, Jan 14, 2015 at 01:01:13PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Wednesday, January 14, 2015 8:22 AM
> > > To: Chao Yu
> > > Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync
> after
> > > operating xattr
> > >
> > > On Mon, Jan 12, 2015 at 05:40:28PM +0800, Chao Yu wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > > Sent: Sunday, January 11, 2015 1:32 PM
> > > > > To: Chao Yu
> > > > > Cc: 'Changman Lee'; linux-f2fs-devel@lists.sourceforge.net;
> linux-kernel@vger.kernel.org
> > > > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when
> fsync
> > > after
> > > > > operating xattr
> > > > >
> > > > > On Sat, Jan 10, 2015 at 08:08:33PM +0800, Chao Yu wrote:
> > > > > > Hi Jaegeuk,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > > > > Sent: Wednesday, January 07, 2015 3:44 AM
> > > > > > > To: Chao Yu
> > > > > > > Cc: Changman Lee; linux-f2fs-devel@lists.sourceforge.net;
> linux-kernel@vger.kernel.org
> > > > > > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when
> > > fsync
> > > > > after
> > > > > > > operating xattr
> > > > > > >
> > > > > > > Hi Chao,
> > > > > > >
> > > > > > > On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> > > > > > > > Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> > > > > > > > will do checkpoint to keep data.
> > > > > > > > This can cause low performance because checkpoint block most operation and write
> > > > > > > > lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> > > > > > > > xattr node page to warm node segment, and then it can be recovered when we mount
> > > > > > > > this device later on.
> > > > > > >
> > > > > > > You're trying to change the writing policy as xattr blocks are written into
> > > > > > > WARM_NODE area instead of COLD_NODE area.
> > > > > > > I don't think xattrs are frequently changed between each fsync calls.
> > > > > > >
> > > > > > > How do you think?
> > > > > >
> > > > > > I'm not sure whether there is a scenario that setxattr and fsync are invoked
> > > > > > alternately, but if there is, our performance will decrease obviously.
> > > > > >
> > > > > > If you don't want to change writing policy, how about writing xattr node with
> > > > > > fsync flag into cold node segment when fsync() is called, then try to recover
> > > > > > it from cold node chain when recovery after abnormally pow-cut, this way can
> > > > > > avoid cp frequently in above scenario.
> > > > >
> > > > > Firt of all, I don't think this scenario is frequent enough that we have to
> > > > > break the exisiting writing and recovery procedures.
> > > > > Moreover, if xattr entries are covered by inline_xattr, it doesn't trigger
> > > > > checkpoint.
> > > >
> > > > Agree, that's a good solution.
> > > >
> > > > >
> > > > > Let me know, if I'm missing something.
> > > > >
> > > > > If you try to change the recovery procedure, it needs to think about the
> > > > > disk full condition. (i.e., space_for_roll_forward())
> > > > > And, I don't want to search cold node chain.
> > > >
> > > > OK, if we keep writing policy and recovery procedure as it is, then, shouldn't our
> > > > recover_xattr_data be dropped because it will be not used from any call path?
> > > > How do you think of below patch?
> > >
> > > Hi Chao,
> > >
> > > Nice catch.
> > > But, IIRC, this code was remained for backward compatibility, since long time
> > > ago, xattr blocks were written into the warm node chain.
> >
> > Ah, I got it, thanks for your explanation! :)
> > How do you think of adding some comments on these codes, because this can help
> > developers understand it well and not to submit the wrong fix patch like me again.
> 
> Something like this?

It's good, this is what I wanted.
Thanks for your work! :)

> 
> From 6b609421e4f9f52de26554300aae62de33e0703a Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> Date: Thu, 22 Jan 2015 14:48:28 -0800
> Subject: [PATCH] f2fs: leave comment for code readability
> 
> During the recovery, any xattr blocks should not be found, since they are
> written into cold log, not the warm node chain.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao2.yu@samsung.com>

Thanks,
Yu

> ---
>  fs/f2fs/recovery.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index c4211a5..57603a7 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -346,6 +346,10 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>  	if (IS_INODE(page)) {
>  		recover_inline_xattr(inode, page);
>  	} else if (f2fs_has_xattr_block(ofs_of_node(page))) {
> +		/*
> +		 * Deprecated; xattr blocks should be found from cold log.
> +		 * But, we should remain this for backward compatibility.
> +		 */
>  		recover_xattr_data(inode, page, blkaddr);
>  		goto out;
>  	}
> --
> 2.1.1