[PATCH 01/13] f2fs: should convert inline_data during the mkwrite

[PATCH 01/13] f2fs: should convert inline_data during the mkwrite

[Jaegeuk Kim]

If mkwrite is called to an inode having inline_data, it can overwrite the data
index space as NEW_ADDR. (e.g., the first 4 bytes are coincidently zero)

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c   |  2 +-
 fs/f2fs/f2fs.h   |  2 +-
 fs/f2fs/file.c   | 11 ++++++++---
 fs/f2fs/inline.c | 20 ++++++++++++--------
 4 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7aef28d..ac3ccc2 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -946,7 +946,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 
 	f2fs_balance_fs(sbi);
 repeat:
-	err = f2fs_convert_inline_data(inode, pos + len);
+	err = f2fs_convert_inline_data(inode, pos + len, NULL);
 	if (err)
 		goto fail;
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 790a073..c8288c9 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1439,7 +1439,7 @@ extern const struct inode_operations f2fs_special_inode_operations;
  */
 bool f2fs_may_inline(struct inode *);
 int f2fs_read_inline_data(struct inode *, struct page *);
-int f2fs_convert_inline_data(struct inode *, pgoff_t);
+int f2fs_convert_inline_data(struct inode *, pgoff_t, struct page *);
 int f2fs_write_inline_data(struct inode *, struct page *, unsigned int);
 void truncate_inline_data(struct inode *, u64);
 int recover_inline_data(struct inode *, struct page *);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 87cdac4..ecbdf6a 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -41,6 +41,11 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct *vma,
 
 	sb_start_pagefault(inode->i_sb);
 
+	/* force to convert with normal data indices */
+	err = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1, page);
+	if (err)
+		goto out;
+
 	/* block allocation */
 	f2fs_lock_op(sbi);
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
@@ -533,7 +538,7 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
 
 	if ((attr->ia_valid & ATTR_SIZE) &&
 			attr->ia_size != i_size_read(inode)) {
-		err = f2fs_convert_inline_data(inode, attr->ia_size);
+		err = f2fs_convert_inline_data(inode, attr->ia_size, NULL);
 		if (err)
 			return err;
 
@@ -622,7 +627,7 @@ static int punch_hole(struct inode *inode, loff_t offset, loff_t len)
 	loff_t off_start, off_end;
 	int ret = 0;
 
-	ret = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1);
+	ret = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1, NULL);
 	if (ret)
 		return ret;
 
@@ -678,7 +683,7 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
 	if (ret)
 		return ret;
 
-	ret = f2fs_convert_inline_data(inode, offset + len);
+	ret = f2fs_convert_inline_data(inode, offset + len, NULL);
 	if (ret)
 		return ret;
 
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 5beecce..1ec512d 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -124,9 +124,10 @@ out:
 	return err;
 }
 
-int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size)
+int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size,
+						struct page *page)
 {
-	struct page *page;
+	struct page *new_page = page;
 	int err;
 
 	if (!f2fs_has_inline_data(inode))
@@ -134,17 +135,20 @@ int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size)
 	else if (to_size <= MAX_INLINE_DATA)
 		return 0;
 
-	page = grab_cache_page(inode->i_mapping, 0);
-	if (!page)
-		return -ENOMEM;
+	if (!page || page->index != 0) {
+		new_page = grab_cache_page(inode->i_mapping, 0);
+		if (!new_page)
+			return -ENOMEM;
+	}
 
-	err = __f2fs_convert_inline_data(inode, page);
-	f2fs_put_page(page, 1);
+	err = __f2fs_convert_inline_data(inode, new_page);
+	if (!page || page->index != 0)
+		f2fs_put_page(new_page, 1);
 	return err;
 }
 
 int f2fs_write_inline_data(struct inode *inode,
-			   struct page *page, unsigned size)
+				struct page *page, unsigned size)
 {
 	void *src_addr, *dst_addr;
 	struct page *ipage;
-- 
1.8.5.2 (Apple Git-48)

[PATCH 01/13] f2fs: should convert inline_data during the mkwrite

[Jaegeuk Kim]

If mkwrite is called to an inode having inline_data, it can overwrite the data
index space as NEW_ADDR. (e.g., the first 4 bytes are coincidently zero)

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c   |  2 +-
 fs/f2fs/f2fs.h   |  2 +-
 fs/f2fs/file.c   | 11 ++++++++---
 fs/f2fs/inline.c | 20 ++++++++++++--------
 4 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7aef28d..ac3ccc2 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -946,7 +946,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 
 	f2fs_balance_fs(sbi);
 repeat:
-	err = f2fs_convert_inline_data(inode, pos + len);
+	err = f2fs_convert_inline_data(inode, pos + len, NULL);
 	if (err)
 		goto fail;
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 790a073..c8288c9 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1439,7 +1439,7 @@ extern const struct inode_operations f2fs_special_inode_operations;
  */
 bool f2fs_may_inline(struct inode *);
 int f2fs_read_inline_data(struct inode *, struct page *);
-int f2fs_convert_inline_data(struct inode *, pgoff_t);
+int f2fs_convert_inline_data(struct inode *, pgoff_t, struct page *);
 int f2fs_write_inline_data(struct inode *, struct page *, unsigned int);
 void truncate_inline_data(struct inode *, u64);
 int recover_inline_data(struct inode *, struct page *);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 87cdac4..ecbdf6a 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -41,6 +41,11 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct *vma,
 
 	sb_start_pagefault(inode->i_sb);
 
+	/* force to convert with normal data indices */
+	err = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1, page);
+	if (err)
+		goto out;
+
 	/* block allocation */
 	f2fs_lock_op(sbi);
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
@@ -533,7 +538,7 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
 
 	if ((attr->ia_valid & ATTR_SIZE) &&
 			attr->ia_size != i_size_read(inode)) {
-		err = f2fs_convert_inline_data(inode, attr->ia_size);
+		err = f2fs_convert_inline_data(inode, attr->ia_size, NULL);
 		if (err)
 			return err;
 
@@ -622,7 +627,7 @@ static int punch_hole(struct inode *inode, loff_t offset, loff_t len)
 	loff_t off_start, off_end;
 	int ret = 0;
 
-	ret = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1);
+	ret = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1, NULL);
 	if (ret)
 		return ret;
 
@@ -678,7 +683,7 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
 	if (ret)
 		return ret;
 
-	ret = f2fs_convert_inline_data(inode, offset + len);
+	ret = f2fs_convert_inline_data(inode, offset + len, NULL);
 	if (ret)
 		return ret;
 
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 5beecce..1ec512d 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -124,9 +124,10 @@ out:
 	return err;
 }
 
-int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size)
+int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size,
+						struct page *page)
 {
-	struct page *page;
+	struct page *new_page = page;
 	int err;
 
 	if (!f2fs_has_inline_data(inode))
@@ -134,17 +135,20 @@ int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size)
 	else if (to_size <= MAX_INLINE_DATA)
 		return 0;
 
-	page = grab_cache_page(inode->i_mapping, 0);
-	if (!page)
-		return -ENOMEM;
+	if (!page || page->index != 0) {
+		new_page = grab_cache_page(inode->i_mapping, 0);
+		if (!new_page)
+			return -ENOMEM;
+	}
 
-	err = __f2fs_convert_inline_data(inode, page);
-	f2fs_put_page(page, 1);
+	err = __f2fs_convert_inline_data(inode, new_page);
+	if (!page || page->index != 0)
+		f2fs_put_page(new_page, 1);
 	return err;
 }
 
 int f2fs_write_inline_data(struct inode *inode,
-			   struct page *page, unsigned size)
+				struct page *page, unsigned size)
 {
 	void *src_addr, *dst_addr;
 	struct page *ipage;
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

[PATCH 02/13] f2fs: make clear on test condition and return types

[Jaegeuk Kim]

This patch adds a parentheses to make clear for condition check.
And also it changes the return type for better meanings.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h   |  2 +-
 fs/f2fs/inline.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index c8288c9..2e4aa3a 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1442,5 +1442,5 @@ int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_data(struct inode *, pgoff_t, struct page *);
 int f2fs_write_inline_data(struct inode *, struct page *, unsigned int);
 void truncate_inline_data(struct inode *, u64);
-int recover_inline_data(struct inode *, struct page *);
+bool recover_inline_data(struct inode *, struct page *);
 #endif
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 1ec512d..520758b 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -203,7 +203,7 @@ void truncate_inline_data(struct inode *inode, u64 from)
 	f2fs_put_page(ipage, 1);
 }
 
-int recover_inline_data(struct inode *inode, struct page *npage)
+bool recover_inline_data(struct inode *inode, struct page *npage)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 	struct f2fs_inode *ri = NULL;
@@ -222,7 +222,7 @@ int recover_inline_data(struct inode *inode, struct page *npage)
 		ri = F2FS_INODE(npage);
 
 	if (f2fs_has_inline_data(inode) &&
-			ri && ri->i_inline & F2FS_INLINE_DATA) {
+			ri && (ri->i_inline & F2FS_INLINE_DATA)) {
 process_inline:
 		ipage = get_node_page(sbi, inode->i_ino);
 		f2fs_bug_on(IS_ERR(ipage));
@@ -234,7 +234,7 @@ process_inline:
 		memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
 		update_inode(inode, ipage);
 		f2fs_put_page(ipage, 1);
-		return -1;
+		return true;
 	}
 
 	if (f2fs_has_inline_data(inode)) {
@@ -246,10 +246,10 @@ process_inline:
 		clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
 		update_inode(inode, ipage);
 		f2fs_put_page(ipage, 1);
-	} else if (ri && ri->i_inline & F2FS_INLINE_DATA) {
+	} else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) {
 		truncate_blocks(inode, 0);
 		set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
 		goto process_inline;
 	}
-	return 0;
+	return false;
 }
-- 
1.8.5.2 (Apple Git-48)

[PATCH 02/13] f2fs: make clear on test condition and return types

[Jaegeuk Kim]

This patch adds a parentheses to make clear for condition check.
And also it changes the return type for better meanings.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h   |  2 +-
 fs/f2fs/inline.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index c8288c9..2e4aa3a 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1442,5 +1442,5 @@ int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_data(struct inode *, pgoff_t, struct page *);
 int f2fs_write_inline_data(struct inode *, struct page *, unsigned int);
 void truncate_inline_data(struct inode *, u64);
-int recover_inline_data(struct inode *, struct page *);
+bool recover_inline_data(struct inode *, struct page *);
 #endif
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 1ec512d..520758b 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -203,7 +203,7 @@ void truncate_inline_data(struct inode *inode, u64 from)
 	f2fs_put_page(ipage, 1);
 }
 
-int recover_inline_data(struct inode *inode, struct page *npage)
+bool recover_inline_data(struct inode *inode, struct page *npage)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 	struct f2fs_inode *ri = NULL;
@@ -222,7 +222,7 @@ int recover_inline_data(struct inode *inode, struct page *npage)
 		ri = F2FS_INODE(npage);
 
 	if (f2fs_has_inline_data(inode) &&
-			ri && ri->i_inline & F2FS_INLINE_DATA) {
+			ri && (ri->i_inline & F2FS_INLINE_DATA)) {
 process_inline:
 		ipage = get_node_page(sbi, inode->i_ino);
 		f2fs_bug_on(IS_ERR(ipage));
@@ -234,7 +234,7 @@ process_inline:
 		memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
 		update_inode(inode, ipage);
 		f2fs_put_page(ipage, 1);
-		return -1;
+		return true;
 	}
 
 	if (f2fs_has_inline_data(inode)) {
@@ -246,10 +246,10 @@ process_inline:
 		clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
 		update_inode(inode, ipage);
 		f2fs_put_page(ipage, 1);
-	} else if (ri && ri->i_inline & F2FS_INLINE_DATA) {
+	} else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) {
 		truncate_blocks(inode, 0);
 		set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
 		goto process_inline;
 	}
-	return 0;
+	return false;
 }
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

[PATCH 03/13] f2fs: fix the initial inode page for recovery

[Jaegeuk Kim]

If a new inode page is needed for recover_dentry, we should assing i_inline
as zero.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 1f33299..093d799 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1651,6 +1651,7 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
 	dst->i_blocks = cpu_to_le64(1);
 	dst->i_links = cpu_to_le32(1);
 	dst->i_xattr_nid = 0;
+	dst->i_inline = src->i_inline & F2FS_INLINE_XATTR;
 
 	new_ni = old_ni;
 	new_ni.ino = ino;
@@ -1659,6 +1660,7 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
 		WARN_ON(1);
 	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
 	inc_valid_inode_count(sbi);
+	set_page_dirty(ipage);
 	f2fs_put_page(ipage, 1);
 	return 0;
 }
-- 
1.8.5.2 (Apple Git-48)

[PATCH 03/13] f2fs: fix the initial inode page for recovery

[Jaegeuk Kim]

If a new inode page is needed for recover_dentry, we should assing i_inline
as zero.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 1f33299..093d799 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1651,6 +1651,7 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
 	dst->i_blocks = cpu_to_le64(1);
 	dst->i_links = cpu_to_le32(1);
 	dst->i_xattr_nid = 0;
+	dst->i_inline = src->i_inline & F2FS_INLINE_XATTR;
 
 	new_ni = old_ni;
 	new_ni.ino = ino;
@@ -1659,6 +1660,7 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
 		WARN_ON(1);
 	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
 	inc_valid_inode_count(sbi);
+	set_page_dirty(ipage);
 	f2fs_put_page(ipage, 1);
 	return 0;
 }
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

RE: [f2fs-dev] [PATCH 03/13] f2fs: fix the initial inode page for recovery

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 03/13] f2fs: fix the initial inode page for recovery
> 
> If a new inode page is needed for recover_dentry, we should assing i_inline
> as zero.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

> ---
>  fs/f2fs/node.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 1f33299..093d799 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1651,6 +1651,7 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
>  	dst->i_blocks = cpu_to_le64(1);
>  	dst->i_links = cpu_to_le32(1);
>  	dst->i_xattr_nid = 0;
> +	dst->i_inline = src->i_inline & F2FS_INLINE_XATTR;
> 
>  	new_ni = old_ni;
>  	new_ni.ino = ino;
> @@ -1659,6 +1660,7 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
>  		WARN_ON(1);
>  	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
>  	inc_valid_inode_count(sbi);
> +	set_page_dirty(ipage);
>  	f2fs_put_page(ipage, 1);
>  	return 0;
>  }
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 03/13] f2fs: fix the initial inode page for recovery

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 03/13] f2fs: fix the initial inode page for recovery
> 
> If a new inode page is needed for recover_dentry, we should assing i_inline
> as zero.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

> ---
>  fs/f2fs/node.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 1f33299..093d799 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1651,6 +1651,7 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
>  	dst->i_blocks = cpu_to_le64(1);
>  	dst->i_links = cpu_to_le32(1);
>  	dst->i_xattr_nid = 0;
> +	dst->i_inline = src->i_inline & F2FS_INLINE_XATTR;
> 
>  	new_ni = old_ni;
>  	new_ni.ino = ino;
> @@ -1659,6 +1660,7 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
>  		WARN_ON(1);
>  	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
>  	inc_valid_inode_count(sbi);
> +	set_page_dirty(ipage);
>  	f2fs_put_page(ipage, 1);
>  	return 0;
>  }
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


------------------------------------------------------------------------------

[PATCH 04/13] f2fs: clear FI_INC_LINK during the recovery

[Jaegeuk Kim]

If an inode are fsynced multiple times with fsync & dent marks, this inode will
set FI_INC_LINK at find_fsync_dnodes during the recovery.
But, in recover_inode, recover_dentry doesn't clear that flag when multiple hits
were occurred.

So this patch removes the flag for the further consistency.

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

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index fe1c6d9..cfb2aa9 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -62,8 +62,10 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 	}
 retry:
 	de = f2fs_find_entry(dir, &name, &page);
-	if (de && inode->i_ino == le32_to_cpu(de->ino))
+	if (de && inode->i_ino == le32_to_cpu(de->ino)) {
+		clear_inode_flag(F2FS_I(inode), FI_INC_LINK);
 		goto out_unmap_put;
+	}
 	if (de) {
 		einode = f2fs_iget(inode->i_sb, le32_to_cpu(de->ino));
 		if (IS_ERR(einode)) {
-- 
1.8.5.2 (Apple Git-48)

[PATCH 04/13] f2fs: clear FI_INC_LINK during the recovery

[Jaegeuk Kim]

If an inode are fsynced multiple times with fsync & dent marks, this inode will
set FI_INC_LINK at find_fsync_dnodes during the recovery.
But, in recover_inode, recover_dentry doesn't clear that flag when multiple hits
were occurred.

So this patch removes the flag for the further consistency.

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

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index fe1c6d9..cfb2aa9 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -62,8 +62,10 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 	}
 retry:
 	de = f2fs_find_entry(dir, &name, &page);
-	if (de && inode->i_ino == le32_to_cpu(de->ino))
+	if (de && inode->i_ino == le32_to_cpu(de->ino)) {
+		clear_inode_flag(F2FS_I(inode), FI_INC_LINK);
 		goto out_unmap_put;
+	}
 	if (de) {
 		einode = f2fs_iget(inode->i_sb, le32_to_cpu(de->ino));
 		if (IS_ERR(einode)) {
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

RE: [f2fs-dev] [PATCH 04/13] f2fs: clear FI_INC_LINK during the recovery

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 04/13] f2fs: clear FI_INC_LINK during the recovery
> 
> If an inode are fsynced multiple times with fsync & dent marks, this inode will
> set FI_INC_LINK at find_fsync_dnodes during the recovery.
> But, in recover_inode, recover_dentry doesn't clear that flag when multiple hits
> were occurred.
> 
> So this patch removes the flag for the further consistency.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

> ---
>  fs/f2fs/recovery.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index fe1c6d9..cfb2aa9 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -62,8 +62,10 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
>  	}
>  retry:
>  	de = f2fs_find_entry(dir, &name, &page);
> -	if (de && inode->i_ino == le32_to_cpu(de->ino))
> +	if (de && inode->i_ino == le32_to_cpu(de->ino)) {
> +		clear_inode_flag(F2FS_I(inode), FI_INC_LINK);
>  		goto out_unmap_put;
> +	}
>  	if (de) {
>  		einode = f2fs_iget(inode->i_sb, le32_to_cpu(de->ino));
>  		if (IS_ERR(einode)) {
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 04/13] f2fs: clear FI_INC_LINK during the recovery

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 04/13] f2fs: clear FI_INC_LINK during the recovery
> 
> If an inode are fsynced multiple times with fsync & dent marks, this inode will
> set FI_INC_LINK at find_fsync_dnodes during the recovery.
> But, in recover_inode, recover_dentry doesn't clear that flag when multiple hits
> were occurred.
> 
> So this patch removes the flag for the further consistency.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

> ---
>  fs/f2fs/recovery.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index fe1c6d9..cfb2aa9 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -62,8 +62,10 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
>  	}
>  retry:
>  	de = f2fs_find_entry(dir, &name, &page);
> -	if (de && inode->i_ino == le32_to_cpu(de->ino))
> +	if (de && inode->i_ino == le32_to_cpu(de->ino)) {
> +		clear_inode_flag(F2FS_I(inode), FI_INC_LINK);
>  		goto out_unmap_put;
> +	}
>  	if (de) {
>  		einode = f2fs_iget(inode->i_sb, le32_to_cpu(de->ino));
>  		if (IS_ERR(einode)) {
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


------------------------------------------------------------------------------

[PATCH 05/13] f2fs: should clear the inline_xattr flag

[Jaegeuk Kim]

During the recovery, we should clear the inline_xattr flag if its xattr node
block is recovered.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 093d799..151045f 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1557,26 +1557,25 @@ void recover_inline_xattr(struct inode *inode, struct page *page)
 	struct page *ipage;
 	struct f2fs_inode *ri;
 
-	if (!f2fs_has_inline_xattr(inode))
-		return;
-
 	if (!IS_INODE(page))
 		return;
 
-	ri = F2FS_INODE(page);
-	if (!(ri->i_inline & F2FS_INLINE_XATTR))
-		return;
-
 	ipage = get_node_page(sbi, inode->i_ino);
 	f2fs_bug_on(IS_ERR(ipage));
 
+	ri = F2FS_INODE(page);
+	if (!(ri->i_inline & F2FS_INLINE_XATTR)) {
+		clear_inode_flag(F2FS_I(inode), FI_INLINE_XATTR);
+		goto update_inode;
+	}
+
 	dst_addr = inline_xattr_addr(ipage);
 	src_addr = inline_xattr_addr(page);
 	inline_size = inline_xattr_size(inode);
 
 	f2fs_wait_on_page_writeback(ipage, NODE);
 	memcpy(dst_addr, src_addr, inline_size);
-
+update_inode:
 	update_inode(inode, ipage);
 	f2fs_put_page(ipage, 1);
 }
-- 
1.8.5.2 (Apple Git-48)

[PATCH 05/13] f2fs: should clear the inline_xattr flag

[Jaegeuk Kim]

During the recovery, we should clear the inline_xattr flag if its xattr node
block is recovered.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 093d799..151045f 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1557,26 +1557,25 @@ void recover_inline_xattr(struct inode *inode, struct page *page)
 	struct page *ipage;
 	struct f2fs_inode *ri;
 
-	if (!f2fs_has_inline_xattr(inode))
-		return;
-
 	if (!IS_INODE(page))
 		return;
 
-	ri = F2FS_INODE(page);
-	if (!(ri->i_inline & F2FS_INLINE_XATTR))
-		return;
-
 	ipage = get_node_page(sbi, inode->i_ino);
 	f2fs_bug_on(IS_ERR(ipage));
 
+	ri = F2FS_INODE(page);
+	if (!(ri->i_inline & F2FS_INLINE_XATTR)) {
+		clear_inode_flag(F2FS_I(inode), FI_INLINE_XATTR);
+		goto update_inode;
+	}
+
 	dst_addr = inline_xattr_addr(ipage);
 	src_addr = inline_xattr_addr(page);
 	inline_size = inline_xattr_size(inode);
 
 	f2fs_wait_on_page_writeback(ipage, NODE);
 	memcpy(dst_addr, src_addr, inline_size);
-
+update_inode:
 	update_inode(inode, ipage);
 	f2fs_put_page(ipage, 1);
 }
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

RE: [f2fs-dev] [PATCH 05/13] f2fs: should clear the inline_xattr flag

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 05/13] f2fs: should clear the inline_xattr flag
> 
> During the recovery, we should clear the inline_xattr flag if its xattr node
> block is recovered.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

> ---
>  fs/f2fs/node.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 093d799..151045f 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1557,26 +1557,25 @@ void recover_inline_xattr(struct inode *inode, struct page *page)
>  	struct page *ipage;
>  	struct f2fs_inode *ri;
> 
> -	if (!f2fs_has_inline_xattr(inode))
> -		return;
> -
>  	if (!IS_INODE(page))
>  		return;
> 
> -	ri = F2FS_INODE(page);
> -	if (!(ri->i_inline & F2FS_INLINE_XATTR))
> -		return;
> -
>  	ipage = get_node_page(sbi, inode->i_ino);
>  	f2fs_bug_on(IS_ERR(ipage));
> 
> +	ri = F2FS_INODE(page);
> +	if (!(ri->i_inline & F2FS_INLINE_XATTR)) {
> +		clear_inode_flag(F2FS_I(inode), FI_INLINE_XATTR);
> +		goto update_inode;
> +	}
> +
>  	dst_addr = inline_xattr_addr(ipage);
>  	src_addr = inline_xattr_addr(page);
>  	inline_size = inline_xattr_size(inode);
> 
>  	f2fs_wait_on_page_writeback(ipage, NODE);
>  	memcpy(dst_addr, src_addr, inline_size);
> -
> +update_inode:
>  	update_inode(inode, ipage);
>  	f2fs_put_page(ipage, 1);
>  }
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH 06/13] f2fs: fix to recover inline_xattr/data and blocks

[Jaegeuk Kim]

This patch fixes not to skip xattr recovery and inline xattr/data recovery
order.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h     |  2 +-
 fs/f2fs/node.c     |  9 +--------
 fs/f2fs/recovery.c | 13 +++++++++----
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 2e4aa3a..cc5ead1 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1205,7 +1205,7 @@ void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
 void recover_node_page(struct f2fs_sb_info *, struct page *,
 		struct f2fs_summary *, struct node_info *, block_t);
 void recover_inline_xattr(struct inode *, struct page *);
-bool recover_xattr_data(struct inode *, struct page *, block_t);
+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 151045f..c80e3d5 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1557,9 +1557,6 @@ void recover_inline_xattr(struct inode *inode, struct page *page)
 	struct page *ipage;
 	struct f2fs_inode *ri;
 
-	if (!IS_INODE(page))
-		return;
-
 	ipage = get_node_page(sbi, inode->i_ino);
 	f2fs_bug_on(IS_ERR(ipage));
 
@@ -1580,16 +1577,13 @@ update_inode:
 	f2fs_put_page(ipage, 1);
 }
 
-bool recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
+void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
 	nid_t new_xnid = nid_of_node(page);
 	struct node_info ni;
 
-	if (!f2fs_has_xattr_block(ofs_of_node(page)))
-		return false;
-
 	/* 1: invalidate the previous xattr nid */
 	if (!prev_xnid)
 		goto recover_xnid;
@@ -1617,7 +1611,6 @@ recover_xnid:
 	set_node_addr(sbi, &ni, blkaddr, false);
 
 	update_inode_page(inode);
-	return true;
 }
 
 int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index cfb2aa9..d7b67b8 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -302,14 +302,19 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	struct node_info ni;
 	int err = 0, recovered = 0;
 
-	recover_inline_xattr(inode, page);
-
-	if (recover_inline_data(inode, page))
+	/* step 1: recover 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;
+	}
 
-	if (recover_xattr_data(inode, page, blkaddr))
+	/* step 2: recover inline data */
+	if (recover_inline_data(inode, page))
 		goto out;
 
+	/* step 3: recover data indices */
 	start = start_bidx_of_node(ofs_of_node(page), fi);
 	end = start + ADDRS_PER_PAGE(page, fi);
 
-- 
1.8.5.2 (Apple Git-48)

[PATCH 06/13] f2fs: fix to recover inline_xattr/data and blocks

[Jaegeuk Kim]

This patch fixes not to skip xattr recovery and inline xattr/data recovery
order.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h     |  2 +-
 fs/f2fs/node.c     |  9 +--------
 fs/f2fs/recovery.c | 13 +++++++++----
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 2e4aa3a..cc5ead1 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1205,7 +1205,7 @@ void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
 void recover_node_page(struct f2fs_sb_info *, struct page *,
 		struct f2fs_summary *, struct node_info *, block_t);
 void recover_inline_xattr(struct inode *, struct page *);
-bool recover_xattr_data(struct inode *, struct page *, block_t);
+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 151045f..c80e3d5 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1557,9 +1557,6 @@ void recover_inline_xattr(struct inode *inode, struct page *page)
 	struct page *ipage;
 	struct f2fs_inode *ri;
 
-	if (!IS_INODE(page))
-		return;
-
 	ipage = get_node_page(sbi, inode->i_ino);
 	f2fs_bug_on(IS_ERR(ipage));
 
@@ -1580,16 +1577,13 @@ update_inode:
 	f2fs_put_page(ipage, 1);
 }
 
-bool recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
+void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
 	nid_t new_xnid = nid_of_node(page);
 	struct node_info ni;
 
-	if (!f2fs_has_xattr_block(ofs_of_node(page)))
-		return false;
-
 	/* 1: invalidate the previous xattr nid */
 	if (!prev_xnid)
 		goto recover_xnid;
@@ -1617,7 +1611,6 @@ recover_xnid:
 	set_node_addr(sbi, &ni, blkaddr, false);
 
 	update_inode_page(inode);
-	return true;
 }
 
 int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index cfb2aa9..d7b67b8 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -302,14 +302,19 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	struct node_info ni;
 	int err = 0, recovered = 0;
 
-	recover_inline_xattr(inode, page);
-
-	if (recover_inline_data(inode, page))
+	/* step 1: recover 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;
+	}
 
-	if (recover_xattr_data(inode, page, blkaddr))
+	/* step 2: recover inline data */
+	if (recover_inline_data(inode, page))
 		goto out;
 
+	/* step 3: recover data indices */
 	start = start_bidx_of_node(ofs_of_node(page), fi);
 	end = start + ADDRS_PER_PAGE(page, fi);
 
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

RE: [f2fs-dev] [PATCH 06/13] f2fs: fix to recover inline_xattr/data and blocks

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 06/13] f2fs: fix to recover inline_xattr/data and blocks
> 
> This patch fixes not to skip xattr recovery and inline xattr/data recovery
> order.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

> ---
>  fs/f2fs/f2fs.h     |  2 +-
>  fs/f2fs/node.c     |  9 +--------
>  fs/f2fs/recovery.c | 13 +++++++++----
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 2e4aa3a..cc5ead1 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1205,7 +1205,7 @@ void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
>  void recover_node_page(struct f2fs_sb_info *, struct page *,
>  		struct f2fs_summary *, struct node_info *, block_t);
>  void recover_inline_xattr(struct inode *, struct page *);
> -bool recover_xattr_data(struct inode *, struct page *, block_t);
> +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 151045f..c80e3d5 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1557,9 +1557,6 @@ void recover_inline_xattr(struct inode *inode, struct page *page)
>  	struct page *ipage;
>  	struct f2fs_inode *ri;
> 
> -	if (!IS_INODE(page))
> -		return;
> -
>  	ipage = get_node_page(sbi, inode->i_ino);
>  	f2fs_bug_on(IS_ERR(ipage));
> 
> @@ -1580,16 +1577,13 @@ update_inode:
>  	f2fs_put_page(ipage, 1);
>  }
> 
> -bool recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
> +void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
>  	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
>  	nid_t new_xnid = nid_of_node(page);
>  	struct node_info ni;
> 
> -	if (!f2fs_has_xattr_block(ofs_of_node(page)))
> -		return false;
> -
>  	/* 1: invalidate the previous xattr nid */
>  	if (!prev_xnid)
>  		goto recover_xnid;
> @@ -1617,7 +1611,6 @@ recover_xnid:
>  	set_node_addr(sbi, &ni, blkaddr, false);
> 
>  	update_inode_page(inode);
> -	return true;
>  }
> 
>  int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index cfb2aa9..d7b67b8 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -302,14 +302,19 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>  	struct node_info ni;
>  	int err = 0, recovered = 0;
> 
> -	recover_inline_xattr(inode, page);
> -
> -	if (recover_inline_data(inode, page))
> +	/* step 1: recover 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;
> +	}
> 
> -	if (recover_xattr_data(inode, page, blkaddr))
> +	/* step 2: recover inline data */
> +	if (recover_inline_data(inode, page))
>  		goto out;
> 
> +	/* step 3: recover data indices */
>  	start = start_bidx_of_node(ofs_of_node(page), fi);
>  	end = start + ADDRS_PER_PAGE(page, fi);
> 
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 06/13] f2fs: fix to recover inline_xattr/data and blocks

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 06/13] f2fs: fix to recover inline_xattr/data and blocks
> 
> This patch fixes not to skip xattr recovery and inline xattr/data recovery
> order.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

> ---
>  fs/f2fs/f2fs.h     |  2 +-
>  fs/f2fs/node.c     |  9 +--------
>  fs/f2fs/recovery.c | 13 +++++++++----
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 2e4aa3a..cc5ead1 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1205,7 +1205,7 @@ void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
>  void recover_node_page(struct f2fs_sb_info *, struct page *,
>  		struct f2fs_summary *, struct node_info *, block_t);
>  void recover_inline_xattr(struct inode *, struct page *);
> -bool recover_xattr_data(struct inode *, struct page *, block_t);
> +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 151045f..c80e3d5 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1557,9 +1557,6 @@ void recover_inline_xattr(struct inode *inode, struct page *page)
>  	struct page *ipage;
>  	struct f2fs_inode *ri;
> 
> -	if (!IS_INODE(page))
> -		return;
> -
>  	ipage = get_node_page(sbi, inode->i_ino);
>  	f2fs_bug_on(IS_ERR(ipage));
> 
> @@ -1580,16 +1577,13 @@ update_inode:
>  	f2fs_put_page(ipage, 1);
>  }
> 
> -bool recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
> +void recover_xattr_data(struct inode *inode, struct page *page, block_t blkaddr)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
>  	nid_t prev_xnid = F2FS_I(inode)->i_xattr_nid;
>  	nid_t new_xnid = nid_of_node(page);
>  	struct node_info ni;
> 
> -	if (!f2fs_has_xattr_block(ofs_of_node(page)))
> -		return false;
> -
>  	/* 1: invalidate the previous xattr nid */
>  	if (!prev_xnid)
>  		goto recover_xnid;
> @@ -1617,7 +1611,6 @@ recover_xnid:
>  	set_node_addr(sbi, &ni, blkaddr, false);
> 
>  	update_inode_page(inode);
> -	return true;
>  }
> 
>  int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index cfb2aa9..d7b67b8 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -302,14 +302,19 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>  	struct node_info ni;
>  	int err = 0, recovered = 0;
> 
> -	recover_inline_xattr(inode, page);
> -
> -	if (recover_inline_data(inode, page))
> +	/* step 1: recover 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;
> +	}
> 
> -	if (recover_xattr_data(inode, page, blkaddr))
> +	/* step 2: recover inline data */
> +	if (recover_inline_data(inode, page))
>  		goto out;
> 
> +	/* step 3: recover data indices */
>  	start = start_bidx_of_node(ofs_of_node(page), fi);
>  	end = start + ADDRS_PER_PAGE(page, fi);
> 
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


------------------------------------------------------------------------------

[PATCH 07/13] f2fs: avoid bug_on when error is occurred

[Jaegeuk Kim]

During the recovery, if an error like EIO or ENOMEM, f2fs_bug_on should skip.

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

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index d7b67b8..7ca7aad 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -472,7 +472,8 @@ int recover_fsync_data(struct f2fs_sb_info *sbi)
 
 	/* step #2: recover data */
 	err = recover_data(sbi, &inode_list, CURSEG_WARM_NODE);
-	f2fs_bug_on(!list_empty(&inode_list));
+	if (!err)
+		f2fs_bug_on(!list_empty(&inode_list));
 out:
 	destroy_fsync_dnodes(&inode_list);
 	kmem_cache_destroy(fsync_entry_slab);
-- 
1.8.5.2 (Apple Git-48)

[PATCH 07/13] f2fs: avoid bug_on when error is occurred

[Jaegeuk Kim]

During the recovery, if an error like EIO or ENOMEM, f2fs_bug_on should skip.

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

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index d7b67b8..7ca7aad 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -472,7 +472,8 @@ int recover_fsync_data(struct f2fs_sb_info *sbi)
 
 	/* step #2: recover data */
 	err = recover_data(sbi, &inode_list, CURSEG_WARM_NODE);
-	f2fs_bug_on(!list_empty(&inode_list));
+	if (!err)
+		f2fs_bug_on(!list_empty(&inode_list));
 out:
 	destroy_fsync_dnodes(&inode_list);
 	kmem_cache_destroy(fsync_entry_slab);
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

[PATCH 08/13] f2fs: do checkpoint at f2fs_put_super

[Jaegeuk Kim]

The generic_shutdown_super calls sync_filesystem, evict_inode, and then
f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
so we should release them at f2fs_put_super.

But normally, it's more reasonable to set its superblock as dirty when
evict_inode is called.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/inode.c | 1 +
 fs/f2fs/super.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 2c39999..eeaf5aa 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -303,6 +303,7 @@ no_delete:
 		add_dirty_inode(sbi, inode->i_ino, APPEND_INO);
 	if (is_inode_flag_set(F2FS_I(inode), FI_UPDATE_WRITE))
 		add_dirty_inode(sbi, inode->i_ino, UPDATE_INO);
+	F2FS_SET_SB_DIRT(sbi);
 out_clear:
 	clear_inode(inode);
 }
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 633315a..60e3554 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
 	stop_gc_thread(sbi);
 
 	/* We don't need to do checkpoint when it's clean */
-	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
+	if (sbi->s_dirty)
 		write_checkpoint(sbi, true);
 
 	iput(sbi->node_inode);
-- 
1.8.5.2 (Apple Git-48)

[PATCH 08/13] f2fs: do checkpoint at f2fs_put_super

[Jaegeuk Kim]

The generic_shutdown_super calls sync_filesystem, evict_inode, and then
f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
so we should release them at f2fs_put_super.

But normally, it's more reasonable to set its superblock as dirty when
evict_inode is called.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/inode.c | 1 +
 fs/f2fs/super.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 2c39999..eeaf5aa 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -303,6 +303,7 @@ no_delete:
 		add_dirty_inode(sbi, inode->i_ino, APPEND_INO);
 	if (is_inode_flag_set(F2FS_I(inode), FI_UPDATE_WRITE))
 		add_dirty_inode(sbi, inode->i_ino, UPDATE_INO);
+	F2FS_SET_SB_DIRT(sbi);
 out_clear:
 	clear_inode(inode);
 }
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 633315a..60e3554 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
 	stop_gc_thread(sbi);
 
 	/* We don't need to do checkpoint when it's clean */
-	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
+	if (sbi->s_dirty)
 		write_checkpoint(sbi, true);
 
 	iput(sbi->node_inode);
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

RE: [f2fs-dev] [PATCH 08/13] f2fs: do checkpoint at f2fs_put_super

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 08/13] f2fs: do checkpoint at f2fs_put_super
> 
> The generic_shutdown_super calls sync_filesystem, evict_inode, and then
> f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
> so we should release them at f2fs_put_super.
> 
> But normally, it's more reasonable to set its superblock as dirty when
> evict_inode is called.

After applying this patch, when we mount and then umount f2fs image without
modification, we will write checkpoint in f2fs_put_super even though we have
no more dirty data to sync, it's not needed.

"some dirty inode information" you mentioned is inode information in ->ino_root of
sbi, right?

Regards,
Yu

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/inode.c | 1 +
>  fs/f2fs/super.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 2c39999..eeaf5aa 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -303,6 +303,7 @@ no_delete:
>  		add_dirty_inode(sbi, inode->i_ino, APPEND_INO);
>  	if (is_inode_flag_set(F2FS_I(inode), FI_UPDATE_WRITE))
>  		add_dirty_inode(sbi, inode->i_ino, UPDATE_INO);
> +	F2FS_SET_SB_DIRT(sbi);
>  out_clear:
>  	clear_inode(inode);
>  }
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 633315a..60e3554 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
>  	stop_gc_thread(sbi);
> 
>  	/* We don't need to do checkpoint when it's clean */
> -	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
> +	if (sbi->s_dirty)
>  		write_checkpoint(sbi, true);
> 
>  	iput(sbi->node_inode);
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 08/13] f2fs: do checkpoint at f2fs_put_super

[Jaegeuk Kim]

Hi Chao,

On Thu, Aug 14, 2014 at 10:57:10AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Wednesday, August 13, 2014 3:49 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 08/13] f2fs: do checkpoint at f2fs_put_super
> > 
> > The generic_shutdown_super calls sync_filesystem, evict_inode, and then
> > f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
> > so we should release them at f2fs_put_super.
> > 
> > But normally, it's more reasonable to set its superblock as dirty when
> > evict_inode is called.
> 
> After applying this patch, when we mount and then umount f2fs image without
> modification, we will write checkpoint in f2fs_put_super even though we have
> no more dirty data to sync, it's not needed.
> 
> "some dirty inode information" you mentioned is inode information in ->ino_root of
> sbi, right?

Actually this patch is intended to call release_dirty_inode() in
write_checkpoint, since evict_inode remains information by calling
add_dirty_inode().

Oh, it'd better set sb dirty only when add_dirty_inode is called.
Let me send v2 for this. :)

Thanks,

> 
> Regards,
> Yu
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/inode.c | 1 +
> >  fs/f2fs/super.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 2c39999..eeaf5aa 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -303,6 +303,7 @@ no_delete:
> >  		add_dirty_inode(sbi, inode->i_ino, APPEND_INO);
> >  	if (is_inode_flag_set(F2FS_I(inode), FI_UPDATE_WRITE))
> >  		add_dirty_inode(sbi, inode->i_ino, UPDATE_INO);
> > +	F2FS_SET_SB_DIRT(sbi);
> >  out_clear:
> >  	clear_inode(inode);
> >  }
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 633315a..60e3554 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
> >  	stop_gc_thread(sbi);
> > 
> >  	/* We don't need to do checkpoint when it's clean */
> > -	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
> > +	if (sbi->s_dirty)
> >  		write_checkpoint(sbi, true);
> > 
> >  	iput(sbi->node_inode);
> > --
> > 1.8.5.2 (Apple Git-48)
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super

[Jaegeuk Kim]

Change log from v1:
 o relocate F2FS_SET_SB_DIRT to avoid unnecessary checkpoints

The generic_shutdown_super calls sync_filesystem, evict_inode, and then
f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
so we should release them at f2fs_put_super.

But normally, it's more reasonable to set its superblock as dirty when
evict_inode is called.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 1 +
 fs/f2fs/super.c      | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 7e1c13b..01e1796 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -330,6 +330,7 @@ void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
 {
 	/* add new dirty ino entry into list */
 	__add_ino_entry(sbi, ino, type);
+	F2FS_SET_SB_DIRT(sbi);
 }
 
 void remove_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 633315a..60e3554 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
 	stop_gc_thread(sbi);
 
 	/* We don't need to do checkpoint when it's clean */
-	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
+	if (sbi->s_dirty)
 		write_checkpoint(sbi, true);
 
 	iput(sbi->node_inode);
-- 
1.8.5.2 (Apple Git-48)

Re: [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super

[Jaegeuk Kim]

Change log from v1:
 o relocate F2FS_SET_SB_DIRT to avoid unnecessary checkpoints

The generic_shutdown_super calls sync_filesystem, evict_inode, and then
f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
so we should release them at f2fs_put_super.

But normally, it's more reasonable to set its superblock as dirty when
evict_inode is called.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 1 +
 fs/f2fs/super.c      | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 7e1c13b..01e1796 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -330,6 +330,7 @@ void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
 {
 	/* add new dirty ino entry into list */
 	__add_ino_entry(sbi, ino, type);
+	F2FS_SET_SB_DIRT(sbi);
 }
 
 void remove_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 633315a..60e3554 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
 	stop_gc_thread(sbi);
 
 	/* We don't need to do checkpoint when it's clean */
-	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
+	if (sbi->s_dirty)
 		write_checkpoint(sbi, true);
 
 	iput(sbi->node_inode);
-- 
1.8.5.2 (Apple Git-48)



------------------------------------------------------------------------------

RE: [f2fs-dev] [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Saturday, August 16, 2014 5:58 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super
> 
> Change log from v1:
>  o relocate F2FS_SET_SB_DIRT to avoid unnecessary checkpoints

In umount, we will encounter memory leak as we may skip releasing dirty inode
when IO error occurred in cp. Why not just invoking release_dirty_inode directly
in f2fs_put_super?

Thanks,
Yu

> 
> The generic_shutdown_super calls sync_filesystem, evict_inode, and then
> f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
> so we should release them at f2fs_put_super.
> 
> But normally, it's more reasonable to set its superblock as dirty when
> evict_inode is called.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/checkpoint.c | 1 +
>  fs/f2fs/super.c      | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 7e1c13b..01e1796 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -330,6 +330,7 @@ void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
>  {
>  	/* add new dirty ino entry into list */
>  	__add_ino_entry(sbi, ino, type);
> +	F2FS_SET_SB_DIRT(sbi);
>  }
> 
>  void remove_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 633315a..60e3554 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
>  	stop_gc_thread(sbi);
> 
>  	/* We don't need to do checkpoint when it's clean */
> -	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
> +	if (sbi->s_dirty)
>  		write_checkpoint(sbi, true);
> 
>  	iput(sbi->node_inode);
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Saturday, August 16, 2014 5:58 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super
> 
> Change log from v1:
>  o relocate F2FS_SET_SB_DIRT to avoid unnecessary checkpoints

In umount, we will encounter memory leak as we may skip releasing dirty inode
when IO error occurred in cp. Why not just invoking release_dirty_inode directly
in f2fs_put_super?

Thanks,
Yu

> 
> The generic_shutdown_super calls sync_filesystem, evict_inode, and then
> f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
> so we should release them at f2fs_put_super.
> 
> But normally, it's more reasonable to set its superblock as dirty when
> evict_inode is called.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/checkpoint.c | 1 +
>  fs/f2fs/super.c      | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 7e1c13b..01e1796 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -330,6 +330,7 @@ void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
>  {
>  	/* add new dirty ino entry into list */
>  	__add_ino_entry(sbi, ino, type);
> +	F2FS_SET_SB_DIRT(sbi);
>  }
> 
>  void remove_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 633315a..60e3554 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
>  	stop_gc_thread(sbi);
> 
>  	/* We don't need to do checkpoint when it's clean */
> -	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
> +	if (sbi->s_dirty)
>  		write_checkpoint(sbi, true);
> 
>  	iput(sbi->node_inode);
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


------------------------------------------------------------------------------

Re: [f2fs-dev] [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super

[Jaegeuk Kim]

On Tue, Aug 19, 2014 at 02:41:44PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Saturday, August 16, 2014 5:58 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Subject: Re: [f2fs-dev] [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super
> > 
> > Change log from v1:
> >  o relocate F2FS_SET_SB_DIRT to avoid unnecessary checkpoints
> 
> In umount, we will encounter memory leak as we may skip releasing dirty inode
> when IO error occurred in cp. Why not just invoking release_dirty_inode directly
> in f2fs_put_super?

Correct. The release_dirty_inode call will be added to handle -EIO through the
following patch.

Ok, so I need to drop this patch and rewrite another patch to make the issue
clear.
Actually the patch will include:
-	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
+	if (sbi->s_dirty)

Thanks,

> 
> Thanks,
> Yu
> 
> > 
> > The generic_shutdown_super calls sync_filesystem, evict_inode, and then
> > f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
> > so we should release them at f2fs_put_super.
> > 
> > But normally, it's more reasonable to set its superblock as dirty when
> > evict_inode is called.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/checkpoint.c | 1 +
> >  fs/f2fs/super.c      | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 7e1c13b..01e1796 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -330,6 +330,7 @@ void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
> >  {
> >  	/* add new dirty ino entry into list */
> >  	__add_ino_entry(sbi, ino, type);
> > +	F2FS_SET_SB_DIRT(sbi);
> >  }
> > 
> >  void remove_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 633315a..60e3554 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
> >  	stop_gc_thread(sbi);
> > 
> >  	/* We don't need to do checkpoint when it's clean */
> > -	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
> > +	if (sbi->s_dirty)
> >  		write_checkpoint(sbi, true);
> > 
> >  	iput(sbi->node_inode);
> > --
> > 1.8.5.2 (Apple Git-48)
> > 
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super

[Jaegeuk Kim]

On Tue, Aug 19, 2014 at 02:41:44PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Saturday, August 16, 2014 5:58 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Subject: Re: [f2fs-dev] [PATCH 08/13 v2] f2fs: do checkpoint at f2fs_put_super
> > 
> > Change log from v1:
> >  o relocate F2FS_SET_SB_DIRT to avoid unnecessary checkpoints
> 
> In umount, we will encounter memory leak as we may skip releasing dirty inode
> when IO error occurred in cp. Why not just invoking release_dirty_inode directly
> in f2fs_put_super?

Correct. The release_dirty_inode call will be added to handle -EIO through the
following patch.

Ok, so I need to drop this patch and rewrite another patch to make the issue
clear.
Actually the patch will include:
-	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
+	if (sbi->s_dirty)

Thanks,

> 
> Thanks,
> Yu
> 
> > 
> > The generic_shutdown_super calls sync_filesystem, evict_inode, and then
> > f2fs_put_super. In f2fs_evict_inode, we remain some dirty inode information
> > so we should release them at f2fs_put_super.
> > 
> > But normally, it's more reasonable to set its superblock as dirty when
> > evict_inode is called.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/checkpoint.c | 1 +
> >  fs/f2fs/super.c      | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 7e1c13b..01e1796 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -330,6 +330,7 @@ void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
> >  {
> >  	/* add new dirty ino entry into list */
> >  	__add_ino_entry(sbi, ino, type);
> > +	F2FS_SET_SB_DIRT(sbi);
> >  }
> > 
> >  void remove_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 633315a..60e3554 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -432,7 +432,7 @@ static void f2fs_put_super(struct super_block *sb)
> >  	stop_gc_thread(sbi);
> > 
> >  	/* We don't need to do checkpoint when it's clean */
> > -	if (sbi->s_dirty && get_pages(sbi, F2FS_DIRTY_NODES))
> > +	if (sbi->s_dirty)
> >  		write_checkpoint(sbi, true);
> > 
> >  	iput(sbi->node_inode);
> > --
> > 1.8.5.2 (Apple Git-48)
> > 
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------

[PATCH 09/13] f2fs: give a chance to mount again when encountering errors

[Jaegeuk Kim]

This patch gives another chance to try mount process when we encounter an error.
This makes an effect on the roll-forward recovery failures as well.

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

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 60e3554..2856020 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -899,8 +899,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	struct buffer_head *raw_super_buf;
 	struct inode *root;
 	long err = -EINVAL;
+	bool retry = true;
 	int i;
 
+try_onemore:
 	/* allocate memory for f2fs-specific super block info */
 	sbi = kzalloc(sizeof(struct f2fs_sb_info), GFP_KERNEL);
 	if (!sbi)
@@ -1080,9 +1082,11 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	/* recover fsynced data */
 	if (!test_opt(sbi, DISABLE_ROLL_FORWARD)) {
 		err = recover_fsync_data(sbi);
-		if (err)
+		if (err) {
 			f2fs_msg(sb, KERN_ERR,
 				"Cannot recover all fsync data errno=%ld", err);
+			goto free_kobj;
+		}
 	}
 
 	/*
@@ -1123,6 +1127,13 @@ free_sb_buf:
 	brelse(raw_super_buf);
 free_sbi:
 	kfree(sbi);
+
+	/* give only one another chance */
+	if (retry) {
+		retry = !retry;
+		shrink_dcache_sb(sb);
+		goto try_onemore;
+	}
 	return err;
 }
 
-- 
1.8.5.2 (Apple Git-48)

[PATCH 09/13] f2fs: give a chance to mount again when encountering errors

[Jaegeuk Kim]

This patch gives another chance to try mount process when we encounter an error.
This makes an effect on the roll-forward recovery failures as well.

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

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 60e3554..2856020 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -899,8 +899,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	struct buffer_head *raw_super_buf;
 	struct inode *root;
 	long err = -EINVAL;
+	bool retry = true;
 	int i;
 
+try_onemore:
 	/* allocate memory for f2fs-specific super block info */
 	sbi = kzalloc(sizeof(struct f2fs_sb_info), GFP_KERNEL);
 	if (!sbi)
@@ -1080,9 +1082,11 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	/* recover fsynced data */
 	if (!test_opt(sbi, DISABLE_ROLL_FORWARD)) {
 		err = recover_fsync_data(sbi);
-		if (err)
+		if (err) {
 			f2fs_msg(sb, KERN_ERR,
 				"Cannot recover all fsync data errno=%ld", err);
+			goto free_kobj;
+		}
 	}
 
 	/*
@@ -1123,6 +1127,13 @@ free_sb_buf:
 	brelse(raw_super_buf);
 free_sbi:
 	kfree(sbi);
+
+	/* give only one another chance */
+	if (retry) {
+		retry = !retry;
+		shrink_dcache_sb(sb);
+		goto try_onemore;
+	}
 	return err;
 }
 
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

[PATCH 10/13] f2fs: introduce f2fs_cp_error for readability

[Jaegeuk Kim]

This patch adds f2fs_cp_error for readability.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 4 ++--
 fs/f2fs/f2fs.h       | 5 +++++
 fs/f2fs/gc.c         | 2 +-
 fs/f2fs/super.c      | 2 +-
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 7e1c13b..1161d2a 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -162,7 +162,7 @@ static int f2fs_write_meta_page(struct page *page,
 		goto redirty_out;
 
 	/* Should not write any meta pages, if any IO error was occurred */
-	if (unlikely(is_set_ckpt_flags(F2FS_CKPT(sbi), CP_ERROR_FLAG)))
+	if (unlikely(f2fs_cp_error(sbi)))
 		goto no_write;
 
 	f2fs_wait_on_page_writeback(page, META);
@@ -934,7 +934,7 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 	/* Here, we only have one bio having CP pack */
 	sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
 
-	if (!is_set_ckpt_flags(ckpt, CP_ERROR_FLAG)) {
+	if (!f2fs_cp_error(sbi)) {
 		clear_prefree_segments(sbi);
 		release_dirty_inode(sbi);
 		F2FS_RESET_SB_DIRT(sbi);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index cc5ead1..46ceb88 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1096,6 +1096,11 @@ static inline int f2fs_readonly(struct super_block *sb)
 	return sb->s_flags & MS_RDONLY;
 }
 
+static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi)
+{
+	return is_set_ckpt_flags(sbi->ckpt, CP_ERROR_FLAG);
+}
+
 static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi)
 {
 	set_ckpt_flags(sbi->ckpt, CP_ERROR_FLAG);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 87c50c5..e8507b1 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -693,7 +693,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi)
 gc_more:
 	if (unlikely(!(sbi->sb->s_flags & MS_ACTIVE)))
 		goto stop;
-	if (unlikely(is_set_ckpt_flags(F2FS_CKPT(sbi), CP_ERROR_FLAG)))
+	if (unlikely(f2fs_cp_error(sbi)))
 		goto stop;
 
 	if (gc_type == BG_GC && has_not_enough_free_secs(sbi, nfree)) {
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 2856020..7e4b631 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -812,7 +812,7 @@ static int sanity_check_ckpt(struct f2fs_sb_info *sbi)
 	if (unlikely(fsmeta >= total))
 		return 1;
 
-	if (unlikely(is_set_ckpt_flags(ckpt, CP_ERROR_FLAG))) {
+	if (unlikely(f2fs_cp_error(sbi))) {
 		f2fs_msg(sbi->sb, KERN_ERR, "A bug case: need to run fsck");
 		return 1;
 	}
-- 
1.8.5.2 (Apple Git-48)

[PATCH 11/13] f2fs: unlock_page when node page is redirtied out

[Jaegeuk Kim]

This patch fixes missing unlock_page when a node page is redirtied out.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index c80e3d5..9f126f8 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1129,8 +1129,11 @@ continue_unlock:
 				set_fsync_mark(page, 0);
 				set_dentry_mark(page, 0);
 			}
-			NODE_MAPPING(sbi)->a_ops->writepage(page, wbc);
-			wrote++;
+
+			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
+				unlock_page(page);
+			else
+				wrote++;
 
 			if (--wbc->nr_to_write == 0)
 				break;
-- 
1.8.5.2 (Apple Git-48)

[PATCH 11/13] f2fs: unlock_page when node page is redirtied out

[Jaegeuk Kim]

This patch fixes missing unlock_page when a node page is redirtied out.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index c80e3d5..9f126f8 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1129,8 +1129,11 @@ continue_unlock:
 				set_fsync_mark(page, 0);
 				set_dentry_mark(page, 0);
 			}
-			NODE_MAPPING(sbi)->a_ops->writepage(page, wbc);
-			wrote++;
+
+			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
+				unlock_page(page);
+			else
+				wrote++;
 
 			if (--wbc->nr_to_write == 0)
 				break;
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

[PATCH 12/13] f2fs: check s_dirty under cp_mutex

[Jaegeuk Kim]

It needs to check s_dirty under cp_mutex, since s_dirty is reset under that
mutex.
And previous condition was not correct, since we can omit doing checkpoint
when checkpoint was done followed by all the node pages were written back.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 8 ++++++--
 fs/f2fs/super.c      | 3 ---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 1161d2a..d074e27 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -952,6 +952,10 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "start block_ops");
 
 	mutex_lock(&sbi->cp_mutex);
+
+	if (!sbi->s_dirty)
+		goto out;
+
 	block_operations(sbi);
 
 	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish block_ops");
@@ -976,9 +980,9 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 	do_checkpoint(sbi, is_umount);
 
 	unblock_operations(sbi);
-	mutex_unlock(&sbi->cp_mutex);
-
 	stat_inc_cp_count(sbi->stat_info);
+out:
+	mutex_unlock(&sbi->cp_mutex);
 	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish checkpoint");
 }
 
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7e4b631..c4f90cd 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -457,9 +457,6 @@ int f2fs_sync_fs(struct super_block *sb, int sync)
 
 	trace_f2fs_sync_fs(sb, sync);
 
-	if (!sbi->s_dirty && !get_pages(sbi, F2FS_DIRTY_NODES))
-		return 0;
-
 	if (sync) {
 		mutex_lock(&sbi->gc_mutex);
 		write_checkpoint(sbi, false);
-- 
1.8.5.2 (Apple Git-48)

[PATCH 13/13] f2fs: handle EIO not to break fs consistency

[Jaegeuk Kim]

There are two rules when EIO is occurred.
1. don't write any checkpoint data to preserve the previous checkpoint
2. don't lose the cached data/node/meta pages

So, at first, this patch adds set_page_dirty in f2fs_write_end_io's failure.
Then, writing checkpoint/dentry/node blocks is not allowed.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 47 +++++++++++++++++++++++++++++++++--------------
 fs/f2fs/data.c       | 10 +++++++++-
 fs/f2fs/f2fs.h       |  1 +
 fs/f2fs/node.c       |  2 ++
 fs/f2fs/super.c      |  3 +++
 5 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index d074e27..b365c64 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -160,14 +160,11 @@ static int f2fs_write_meta_page(struct page *page,
 		goto redirty_out;
 	if (wbc->for_reclaim)
 		goto redirty_out;
-
-	/* Should not write any meta pages, if any IO error was occurred */
 	if (unlikely(f2fs_cp_error(sbi)))
-		goto no_write;
+		goto redirty_out;
 
 	f2fs_wait_on_page_writeback(page, META);
 	write_meta_page(sbi, page);
-no_write:
 	dec_page_count(sbi, F2FS_DIRTY_META);
 	unlock_page(page);
 	return 0;
@@ -348,7 +345,7 @@ bool exist_written_data(struct f2fs_sb_info *sbi, nid_t ino, int mode)
 	return e ? true : false;
 }
 
-static void release_dirty_inode(struct f2fs_sb_info *sbi)
+void release_dirty_inode(struct f2fs_sb_info *sbi)
 {
 	struct ino_entry *e, *tmp;
 	int i;
@@ -737,7 +734,7 @@ retry:
 /*
  * Freeze all the FS-operations for checkpoint.
  */
-static void block_operations(struct f2fs_sb_info *sbi)
+static int block_operations(struct f2fs_sb_info *sbi)
 {
 	struct writeback_control wbc = {
 		.sync_mode = WB_SYNC_ALL,
@@ -745,6 +742,7 @@ static void block_operations(struct f2fs_sb_info *sbi)
 		.for_reclaim = 0,
 	};
 	struct blk_plug plug;
+	int err = 0;
 
 	blk_start_plug(&plug);
 
@@ -754,6 +752,10 @@ retry_flush_dents:
 	if (get_pages(sbi, F2FS_DIRTY_DENTS)) {
 		f2fs_unlock_all(sbi);
 		sync_dirty_dir_inodes(sbi);
+		if (unlikely(f2fs_cp_error(sbi))) {
+			err = -EIO;
+			goto out;
+		}
 		goto retry_flush_dents;
 	}
 
@@ -767,9 +769,16 @@ retry_flush_nodes:
 	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
 		up_write(&sbi->node_write);
 		sync_node_pages(sbi, 0, &wbc);
+		if (unlikely(f2fs_cp_error(sbi))) {
+			f2fs_unlock_all(sbi);
+			err = -EIO;
+			goto out;
+		}
 		goto retry_flush_nodes;
 	}
+out:
 	blk_finish_plug(&plug);
+	return err;
 }
 
 static void unblock_operations(struct f2fs_sb_info *sbi)
@@ -813,8 +822,11 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 	discard_next_dnode(sbi, NEXT_FREE_BLKADDR(sbi, curseg));
 
 	/* Flush all the NAT/SIT pages */
-	while (get_pages(sbi, F2FS_DIRTY_META))
+	while (get_pages(sbi, F2FS_DIRTY_META)) {
 		sync_meta_pages(sbi, META, LONG_MAX);
+		if (unlikely(f2fs_cp_error(sbi)))
+			return;
+	}
 
 	next_free_nid(sbi, &last_nid);
 
@@ -931,14 +943,19 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 	sbi->last_valid_block_count = sbi->total_valid_block_count;
 	sbi->alloc_valid_block_count = 0;
 
+	if (unlikely(f2fs_cp_error(sbi)))
+		return;
+
 	/* Here, we only have one bio having CP pack */
 	sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
 
-	if (!f2fs_cp_error(sbi)) {
-		clear_prefree_segments(sbi);
-		release_dirty_inode(sbi);
-		F2FS_RESET_SB_DIRT(sbi);
-	}
+	release_dirty_inode(sbi);
+
+	if (unlikely(f2fs_cp_error(sbi)))
+		return;
+
+	clear_prefree_segments(sbi);
+	F2FS_RESET_SB_DIRT(sbi);
 }
 
 /*
@@ -955,8 +972,10 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 
 	if (!sbi->s_dirty)
 		goto out;
-
-	block_operations(sbi);
+	if (unlikely(f2fs_cp_error(sbi)))
+		goto out;
+	if (block_operations(sbi))
+		goto out;
 
 	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish block_ops");
 
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index ac3ccc2..68834e2 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -53,7 +53,7 @@ static void f2fs_write_end_io(struct bio *bio, int err)
 		struct page *page = bvec->bv_page;
 
 		if (unlikely(err)) {
-			SetPageError(page);
+			set_page_dirty(page);
 			set_bit(AS_EIO, &page->mapping->flags);
 			f2fs_stop_checkpoint(sbi);
 		}
@@ -836,10 +836,18 @@ write:
 
 	/* Dentry blocks are controlled by checkpoint */
 	if (S_ISDIR(inode->i_mode)) {
+		if (unlikely(f2fs_cp_error(sbi)))
+			goto redirty_out;
 		err = do_write_data_page(page, &fio);
 		goto done;
 	}
 
+	if (unlikely(f2fs_cp_error(sbi))) {
+		SetPageError(page);
+		unlock_page(page);
+		return 0;
+	}
+
 	if (!wbc->for_reclaim)
 		need_balance_fs = true;
 	else if (has_not_enough_free_secs(sbi, 0))
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 46ceb88..2d009ae 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1267,6 +1267,7 @@ int ra_meta_pages(struct f2fs_sb_info *, int, int, int);
 long sync_meta_pages(struct f2fs_sb_info *, enum page_type, long);
 void add_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
 void remove_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
+void release_dirty_inode(struct f2fs_sb_info *);
 bool exist_written_data(struct f2fs_sb_info *, nid_t, int);
 int acquire_orphan_inode(struct f2fs_sb_info *);
 void release_orphan_inode(struct f2fs_sb_info *);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 9f126f8..d2f7842 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1215,6 +1215,8 @@ static int f2fs_write_node_page(struct page *page,
 
 	if (unlikely(sbi->por_doing))
 		goto redirty_out;
+	if (unlikely(f2fs_cp_error(sbi)))
+		goto redirty_out;
 
 	f2fs_wait_on_page_writeback(page, NODE);
 
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index c4f90cd..c999d67 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -435,6 +435,9 @@ static void f2fs_put_super(struct super_block *sb)
 	if (sbi->s_dirty)
 		write_checkpoint(sbi, true);
 
+	/* EIO will skip do checkpoint, so here we need to release this */
+	release_dirty_inode(sbi);
+
 	iput(sbi->node_inode);
 	iput(sbi->meta_inode);
 
-- 
1.8.5.2 (Apple Git-48)

[PATCH 13/13] f2fs: handle EIO not to break fs consistency

[Jaegeuk Kim]

There are two rules when EIO is occurred.
1. don't write any checkpoint data to preserve the previous checkpoint
2. don't lose the cached data/node/meta pages

So, at first, this patch adds set_page_dirty in f2fs_write_end_io's failure.
Then, writing checkpoint/dentry/node blocks is not allowed.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 47 +++++++++++++++++++++++++++++++++--------------
 fs/f2fs/data.c       | 10 +++++++++-
 fs/f2fs/f2fs.h       |  1 +
 fs/f2fs/node.c       |  2 ++
 fs/f2fs/super.c      |  3 +++
 5 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index d074e27..b365c64 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -160,14 +160,11 @@ static int f2fs_write_meta_page(struct page *page,
 		goto redirty_out;
 	if (wbc->for_reclaim)
 		goto redirty_out;
-
-	/* Should not write any meta pages, if any IO error was occurred */
 	if (unlikely(f2fs_cp_error(sbi)))
-		goto no_write;
+		goto redirty_out;
 
 	f2fs_wait_on_page_writeback(page, META);
 	write_meta_page(sbi, page);
-no_write:
 	dec_page_count(sbi, F2FS_DIRTY_META);
 	unlock_page(page);
 	return 0;
@@ -348,7 +345,7 @@ bool exist_written_data(struct f2fs_sb_info *sbi, nid_t ino, int mode)
 	return e ? true : false;
 }
 
-static void release_dirty_inode(struct f2fs_sb_info *sbi)
+void release_dirty_inode(struct f2fs_sb_info *sbi)
 {
 	struct ino_entry *e, *tmp;
 	int i;
@@ -737,7 +734,7 @@ retry:
 /*
  * Freeze all the FS-operations for checkpoint.
  */
-static void block_operations(struct f2fs_sb_info *sbi)
+static int block_operations(struct f2fs_sb_info *sbi)
 {
 	struct writeback_control wbc = {
 		.sync_mode = WB_SYNC_ALL,
@@ -745,6 +742,7 @@ static void block_operations(struct f2fs_sb_info *sbi)
 		.for_reclaim = 0,
 	};
 	struct blk_plug plug;
+	int err = 0;
 
 	blk_start_plug(&plug);
 
@@ -754,6 +752,10 @@ retry_flush_dents:
 	if (get_pages(sbi, F2FS_DIRTY_DENTS)) {
 		f2fs_unlock_all(sbi);
 		sync_dirty_dir_inodes(sbi);
+		if (unlikely(f2fs_cp_error(sbi))) {
+			err = -EIO;
+			goto out;
+		}
 		goto retry_flush_dents;
 	}
 
@@ -767,9 +769,16 @@ retry_flush_nodes:
 	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
 		up_write(&sbi->node_write);
 		sync_node_pages(sbi, 0, &wbc);
+		if (unlikely(f2fs_cp_error(sbi))) {
+			f2fs_unlock_all(sbi);
+			err = -EIO;
+			goto out;
+		}
 		goto retry_flush_nodes;
 	}
+out:
 	blk_finish_plug(&plug);
+	return err;
 }
 
 static void unblock_operations(struct f2fs_sb_info *sbi)
@@ -813,8 +822,11 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 	discard_next_dnode(sbi, NEXT_FREE_BLKADDR(sbi, curseg));
 
 	/* Flush all the NAT/SIT pages */
-	while (get_pages(sbi, F2FS_DIRTY_META))
+	while (get_pages(sbi, F2FS_DIRTY_META)) {
 		sync_meta_pages(sbi, META, LONG_MAX);
+		if (unlikely(f2fs_cp_error(sbi)))
+			return;
+	}
 
 	next_free_nid(sbi, &last_nid);
 
@@ -931,14 +943,19 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 	sbi->last_valid_block_count = sbi->total_valid_block_count;
 	sbi->alloc_valid_block_count = 0;
 
+	if (unlikely(f2fs_cp_error(sbi)))
+		return;
+
 	/* Here, we only have one bio having CP pack */
 	sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
 
-	if (!f2fs_cp_error(sbi)) {
-		clear_prefree_segments(sbi);
-		release_dirty_inode(sbi);
-		F2FS_RESET_SB_DIRT(sbi);
-	}
+	release_dirty_inode(sbi);
+
+	if (unlikely(f2fs_cp_error(sbi)))
+		return;
+
+	clear_prefree_segments(sbi);
+	F2FS_RESET_SB_DIRT(sbi);
 }
 
 /*
@@ -955,8 +972,10 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 
 	if (!sbi->s_dirty)
 		goto out;
-
-	block_operations(sbi);
+	if (unlikely(f2fs_cp_error(sbi)))
+		goto out;
+	if (block_operations(sbi))
+		goto out;
 
 	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish block_ops");
 
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index ac3ccc2..68834e2 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -53,7 +53,7 @@ static void f2fs_write_end_io(struct bio *bio, int err)
 		struct page *page = bvec->bv_page;
 
 		if (unlikely(err)) {
-			SetPageError(page);
+			set_page_dirty(page);
 			set_bit(AS_EIO, &page->mapping->flags);
 			f2fs_stop_checkpoint(sbi);
 		}
@@ -836,10 +836,18 @@ write:
 
 	/* Dentry blocks are controlled by checkpoint */
 	if (S_ISDIR(inode->i_mode)) {
+		if (unlikely(f2fs_cp_error(sbi)))
+			goto redirty_out;
 		err = do_write_data_page(page, &fio);
 		goto done;
 	}
 
+	if (unlikely(f2fs_cp_error(sbi))) {
+		SetPageError(page);
+		unlock_page(page);
+		return 0;
+	}
+
 	if (!wbc->for_reclaim)
 		need_balance_fs = true;
 	else if (has_not_enough_free_secs(sbi, 0))
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 46ceb88..2d009ae 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1267,6 +1267,7 @@ int ra_meta_pages(struct f2fs_sb_info *, int, int, int);
 long sync_meta_pages(struct f2fs_sb_info *, enum page_type, long);
 void add_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
 void remove_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
+void release_dirty_inode(struct f2fs_sb_info *);
 bool exist_written_data(struct f2fs_sb_info *, nid_t, int);
 int acquire_orphan_inode(struct f2fs_sb_info *);
 void release_orphan_inode(struct f2fs_sb_info *);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 9f126f8..d2f7842 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1215,6 +1215,8 @@ static int f2fs_write_node_page(struct page *page,
 
 	if (unlikely(sbi->por_doing))
 		goto redirty_out;
+	if (unlikely(f2fs_cp_error(sbi)))
+		goto redirty_out;
 
 	f2fs_wait_on_page_writeback(page, NODE);
 
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index c4f90cd..c999d67 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -435,6 +435,9 @@ static void f2fs_put_super(struct super_block *sb)
 	if (sbi->s_dirty)
 		write_checkpoint(sbi, true);
 
+	/* EIO will skip do checkpoint, so here we need to release this */
+	release_dirty_inode(sbi);
+
 	iput(sbi->node_inode);
 	iput(sbi->meta_inode);
 
-- 
1.8.5.2 (Apple Git-48)


------------------------------------------------------------------------------

RE: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency
> 
> There are two rules when EIO is occurred.
> 1. don't write any checkpoint data to preserve the previous checkpoint
> 2. don't lose the cached data/node/meta pages
> 
> So, at first, this patch adds set_page_dirty in f2fs_write_end_io's failure.
> Then, writing checkpoint/dentry/node blocks is not allowed.

do_checkpoint()
	/* wait for previous submitted node/meta pages writeback */
	wait_on_all_pages_writeback(sbi);

	if (unlikely(f2fs_cp_error(sbi)))
		return;
add here to prevent unneeded syncing for meta/node pages.

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/checkpoint.c | 47 +++++++++++++++++++++++++++++++++--------------
>  fs/f2fs/data.c       | 10 +++++++++-
>  fs/f2fs/f2fs.h       |  1 +
>  fs/f2fs/node.c       |  2 ++
>  fs/f2fs/super.c      |  3 +++
>  5 files changed, 48 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index d074e27..b365c64 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -160,14 +160,11 @@ static int f2fs_write_meta_page(struct page *page,
>  		goto redirty_out;
>  	if (wbc->for_reclaim)
>  		goto redirty_out;
> -
> -	/* Should not write any meta pages, if any IO error was occurred */
>  	if (unlikely(f2fs_cp_error(sbi)))
> -		goto no_write;
> +		goto redirty_out;
> 
>  	f2fs_wait_on_page_writeback(page, META);
>  	write_meta_page(sbi, page);
> -no_write:
>  	dec_page_count(sbi, F2FS_DIRTY_META);
>  	unlock_page(page);
>  	return 0;
> @@ -348,7 +345,7 @@ bool exist_written_data(struct f2fs_sb_info *sbi, nid_t ino, int mode)
>  	return e ? true : false;
>  }
> 
> -static void release_dirty_inode(struct f2fs_sb_info *sbi)
> +void release_dirty_inode(struct f2fs_sb_info *sbi)
>  {
>  	struct ino_entry *e, *tmp;
>  	int i;
> @@ -737,7 +734,7 @@ retry:
>  /*
>   * Freeze all the FS-operations for checkpoint.
>   */
> -static void block_operations(struct f2fs_sb_info *sbi)
> +static int block_operations(struct f2fs_sb_info *sbi)
>  {
>  	struct writeback_control wbc = {
>  		.sync_mode = WB_SYNC_ALL,
> @@ -745,6 +742,7 @@ static void block_operations(struct f2fs_sb_info *sbi)
>  		.for_reclaim = 0,
>  	};
>  	struct blk_plug plug;
> +	int err = 0;
> 
>  	blk_start_plug(&plug);
> 
> @@ -754,6 +752,10 @@ retry_flush_dents:
>  	if (get_pages(sbi, F2FS_DIRTY_DENTS)) {
>  		f2fs_unlock_all(sbi);
>  		sync_dirty_dir_inodes(sbi);
> +		if (unlikely(f2fs_cp_error(sbi))) {
> +			err = -EIO;
> +			goto out;
> +		}
>  		goto retry_flush_dents;
>  	}
> 
> @@ -767,9 +769,16 @@ retry_flush_nodes:
>  	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
>  		up_write(&sbi->node_write);
>  		sync_node_pages(sbi, 0, &wbc);
> +		if (unlikely(f2fs_cp_error(sbi))) {
> +			f2fs_unlock_all(sbi);
> +			err = -EIO;
> +			goto out;
> +		}
>  		goto retry_flush_nodes;
>  	}
> +out:
>  	blk_finish_plug(&plug);
> +	return err;
>  }
> 
>  static void unblock_operations(struct f2fs_sb_info *sbi)
> @@ -813,8 +822,11 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
>  	discard_next_dnode(sbi, NEXT_FREE_BLKADDR(sbi, curseg));
> 
>  	/* Flush all the NAT/SIT pages */
> -	while (get_pages(sbi, F2FS_DIRTY_META))
> +	while (get_pages(sbi, F2FS_DIRTY_META)) {
>  		sync_meta_pages(sbi, META, LONG_MAX);
> +		if (unlikely(f2fs_cp_error(sbi)))
> +			return;
> +	}
> 
>  	next_free_nid(sbi, &last_nid);
> 
> @@ -931,14 +943,19 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
>  	sbi->last_valid_block_count = sbi->total_valid_block_count;
>  	sbi->alloc_valid_block_count = 0;
> 
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		return;
> +
>  	/* Here, we only have one bio having CP pack */
>  	sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
> 
> -	if (!f2fs_cp_error(sbi)) {
> -		clear_prefree_segments(sbi);
> -		release_dirty_inode(sbi);
> -		F2FS_RESET_SB_DIRT(sbi);
> -	}
> +	release_dirty_inode(sbi);
> +
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		return;
> +
> +	clear_prefree_segments(sbi);
> +	F2FS_RESET_SB_DIRT(sbi);
>  }
> 
>  /*
> @@ -955,8 +972,10 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> 
>  	if (!sbi->s_dirty)
>  		goto out;
> -
> -	block_operations(sbi);
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		goto out;
> +	if (block_operations(sbi))
> +		goto out;
> 
>  	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish block_ops");
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index ac3ccc2..68834e2 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -53,7 +53,7 @@ static void f2fs_write_end_io(struct bio *bio, int err)
>  		struct page *page = bvec->bv_page;
> 
>  		if (unlikely(err)) {
> -			SetPageError(page);
> +			set_page_dirty(page);
>  			set_bit(AS_EIO, &page->mapping->flags);
>  			f2fs_stop_checkpoint(sbi);
>  		}
> @@ -836,10 +836,18 @@ write:
> 
>  	/* Dentry blocks are controlled by checkpoint */
>  	if (S_ISDIR(inode->i_mode)) {
> +		if (unlikely(f2fs_cp_error(sbi)))
> +			goto redirty_out;
>  		err = do_write_data_page(page, &fio);
>  		goto done;
>  	}
> 
> +	if (unlikely(f2fs_cp_error(sbi))) {
> +		SetPageError(page);
> +		unlock_page(page);
> +		return 0;

Without redirtying it, we may lose cached page here which against the
second rule.

Thanks,
Yu


> +	}
> +
>  	if (!wbc->for_reclaim)
>  		need_balance_fs = true;
>  	else if (has_not_enough_free_secs(sbi, 0))
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 46ceb88..2d009ae 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1267,6 +1267,7 @@ int ra_meta_pages(struct f2fs_sb_info *, int, int, int);
>  long sync_meta_pages(struct f2fs_sb_info *, enum page_type, long);
>  void add_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
>  void remove_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
> +void release_dirty_inode(struct f2fs_sb_info *);
>  bool exist_written_data(struct f2fs_sb_info *, nid_t, int);
>  int acquire_orphan_inode(struct f2fs_sb_info *);
>  void release_orphan_inode(struct f2fs_sb_info *);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 9f126f8..d2f7842 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1215,6 +1215,8 @@ static int f2fs_write_node_page(struct page *page,
> 
>  	if (unlikely(sbi->por_doing))
>  		goto redirty_out;
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		goto redirty_out;
> 
>  	f2fs_wait_on_page_writeback(page, NODE);
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index c4f90cd..c999d67 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -435,6 +435,9 @@ static void f2fs_put_super(struct super_block *sb)
>  	if (sbi->s_dirty)
>  		write_checkpoint(sbi, true);
> 
> +	/* EIO will skip do checkpoint, so here we need to release this */
> +	release_dirty_inode(sbi);
> +
>  	iput(sbi->node_inode);
>  	iput(sbi->meta_inode);
> 
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 13/13] f2fs: handle EIO not to break fs consistency

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency
> 
> There are two rules when EIO is occurred.
> 1. don't write any checkpoint data to preserve the previous checkpoint
> 2. don't lose the cached data/node/meta pages
> 
> So, at first, this patch adds set_page_dirty in f2fs_write_end_io's failure.
> Then, writing checkpoint/dentry/node blocks is not allowed.

do_checkpoint()
	/* wait for previous submitted node/meta pages writeback */
	wait_on_all_pages_writeback(sbi);

	if (unlikely(f2fs_cp_error(sbi)))
		return;
add here to prevent unneeded syncing for meta/node pages.

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/checkpoint.c | 47 +++++++++++++++++++++++++++++++++--------------
>  fs/f2fs/data.c       | 10 +++++++++-
>  fs/f2fs/f2fs.h       |  1 +
>  fs/f2fs/node.c       |  2 ++
>  fs/f2fs/super.c      |  3 +++
>  5 files changed, 48 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index d074e27..b365c64 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -160,14 +160,11 @@ static int f2fs_write_meta_page(struct page *page,
>  		goto redirty_out;
>  	if (wbc->for_reclaim)
>  		goto redirty_out;
> -
> -	/* Should not write any meta pages, if any IO error was occurred */
>  	if (unlikely(f2fs_cp_error(sbi)))
> -		goto no_write;
> +		goto redirty_out;
> 
>  	f2fs_wait_on_page_writeback(page, META);
>  	write_meta_page(sbi, page);
> -no_write:
>  	dec_page_count(sbi, F2FS_DIRTY_META);
>  	unlock_page(page);
>  	return 0;
> @@ -348,7 +345,7 @@ bool exist_written_data(struct f2fs_sb_info *sbi, nid_t ino, int mode)
>  	return e ? true : false;
>  }
> 
> -static void release_dirty_inode(struct f2fs_sb_info *sbi)
> +void release_dirty_inode(struct f2fs_sb_info *sbi)
>  {
>  	struct ino_entry *e, *tmp;
>  	int i;
> @@ -737,7 +734,7 @@ retry:
>  /*
>   * Freeze all the FS-operations for checkpoint.
>   */
> -static void block_operations(struct f2fs_sb_info *sbi)
> +static int block_operations(struct f2fs_sb_info *sbi)
>  {
>  	struct writeback_control wbc = {
>  		.sync_mode = WB_SYNC_ALL,
> @@ -745,6 +742,7 @@ static void block_operations(struct f2fs_sb_info *sbi)
>  		.for_reclaim = 0,
>  	};
>  	struct blk_plug plug;
> +	int err = 0;
> 
>  	blk_start_plug(&plug);
> 
> @@ -754,6 +752,10 @@ retry_flush_dents:
>  	if (get_pages(sbi, F2FS_DIRTY_DENTS)) {
>  		f2fs_unlock_all(sbi);
>  		sync_dirty_dir_inodes(sbi);
> +		if (unlikely(f2fs_cp_error(sbi))) {
> +			err = -EIO;
> +			goto out;
> +		}
>  		goto retry_flush_dents;
>  	}
> 
> @@ -767,9 +769,16 @@ retry_flush_nodes:
>  	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
>  		up_write(&sbi->node_write);
>  		sync_node_pages(sbi, 0, &wbc);
> +		if (unlikely(f2fs_cp_error(sbi))) {
> +			f2fs_unlock_all(sbi);
> +			err = -EIO;
> +			goto out;
> +		}
>  		goto retry_flush_nodes;
>  	}
> +out:
>  	blk_finish_plug(&plug);
> +	return err;
>  }
> 
>  static void unblock_operations(struct f2fs_sb_info *sbi)
> @@ -813,8 +822,11 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
>  	discard_next_dnode(sbi, NEXT_FREE_BLKADDR(sbi, curseg));
> 
>  	/* Flush all the NAT/SIT pages */
> -	while (get_pages(sbi, F2FS_DIRTY_META))
> +	while (get_pages(sbi, F2FS_DIRTY_META)) {
>  		sync_meta_pages(sbi, META, LONG_MAX);
> +		if (unlikely(f2fs_cp_error(sbi)))
> +			return;
> +	}
> 
>  	next_free_nid(sbi, &last_nid);
> 
> @@ -931,14 +943,19 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
>  	sbi->last_valid_block_count = sbi->total_valid_block_count;
>  	sbi->alloc_valid_block_count = 0;
> 
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		return;
> +
>  	/* Here, we only have one bio having CP pack */
>  	sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
> 
> -	if (!f2fs_cp_error(sbi)) {
> -		clear_prefree_segments(sbi);
> -		release_dirty_inode(sbi);
> -		F2FS_RESET_SB_DIRT(sbi);
> -	}
> +	release_dirty_inode(sbi);
> +
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		return;
> +
> +	clear_prefree_segments(sbi);
> +	F2FS_RESET_SB_DIRT(sbi);
>  }
> 
>  /*
> @@ -955,8 +972,10 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> 
>  	if (!sbi->s_dirty)
>  		goto out;
> -
> -	block_operations(sbi);
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		goto out;
> +	if (block_operations(sbi))
> +		goto out;
> 
>  	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish block_ops");
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index ac3ccc2..68834e2 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -53,7 +53,7 @@ static void f2fs_write_end_io(struct bio *bio, int err)
>  		struct page *page = bvec->bv_page;
> 
>  		if (unlikely(err)) {
> -			SetPageError(page);
> +			set_page_dirty(page);
>  			set_bit(AS_EIO, &page->mapping->flags);
>  			f2fs_stop_checkpoint(sbi);
>  		}
> @@ -836,10 +836,18 @@ write:
> 
>  	/* Dentry blocks are controlled by checkpoint */
>  	if (S_ISDIR(inode->i_mode)) {
> +		if (unlikely(f2fs_cp_error(sbi)))
> +			goto redirty_out;
>  		err = do_write_data_page(page, &fio);
>  		goto done;
>  	}
> 
> +	if (unlikely(f2fs_cp_error(sbi))) {
> +		SetPageError(page);
> +		unlock_page(page);
> +		return 0;

Without redirtying it, we may lose cached page here which against the
second rule.

Thanks,
Yu


> +	}
> +
>  	if (!wbc->for_reclaim)
>  		need_balance_fs = true;
>  	else if (has_not_enough_free_secs(sbi, 0))
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 46ceb88..2d009ae 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1267,6 +1267,7 @@ int ra_meta_pages(struct f2fs_sb_info *, int, int, int);
>  long sync_meta_pages(struct f2fs_sb_info *, enum page_type, long);
>  void add_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
>  void remove_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
> +void release_dirty_inode(struct f2fs_sb_info *);
>  bool exist_written_data(struct f2fs_sb_info *, nid_t, int);
>  int acquire_orphan_inode(struct f2fs_sb_info *);
>  void release_orphan_inode(struct f2fs_sb_info *);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 9f126f8..d2f7842 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1215,6 +1215,8 @@ static int f2fs_write_node_page(struct page *page,
> 
>  	if (unlikely(sbi->por_doing))
>  		goto redirty_out;
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		goto redirty_out;
> 
>  	f2fs_wait_on_page_writeback(page, NODE);
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index c4f90cd..c999d67 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -435,6 +435,9 @@ static void f2fs_put_super(struct super_block *sb)
>  	if (sbi->s_dirty)
>  		write_checkpoint(sbi, true);
> 
> +	/* EIO will skip do checkpoint, so here we need to release this */
> +	release_dirty_inode(sbi);
> +
>  	iput(sbi->node_inode);
>  	iput(sbi->meta_inode);
> 
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

Re: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency

[Jaegeuk Kim]

On Wed, Aug 20, 2014 at 06:35:18PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Wednesday, August 13, 2014 3:49 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency
> > 
> > There are two rules when EIO is occurred.
> > 1. don't write any checkpoint data to preserve the previous checkpoint
> > 2. don't lose the cached data/node/meta pages
> > 
> > So, at first, this patch adds set_page_dirty in f2fs_write_end_io's failure.
> > Then, writing checkpoint/dentry/node blocks is not allowed.
> 
> do_checkpoint()
> 	/* wait for previous submitted node/meta pages writeback */
> 	wait_on_all_pages_writeback(sbi);
> 
> 	if (unlikely(f2fs_cp_error(sbi)))
> 		return;
> add here to prevent unneeded syncing for meta/node pages.

Well, there is one below. But it's not a big deal to change the location.

> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/checkpoint.c | 47 +++++++++++++++++++++++++++++++++--------------
> >  fs/f2fs/data.c       | 10 +++++++++-
> >  fs/f2fs/f2fs.h       |  1 +
> >  fs/f2fs/node.c       |  2 ++
> >  fs/f2fs/super.c      |  3 +++
> >  5 files changed, 48 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index d074e27..b365c64 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -160,14 +160,11 @@ static int f2fs_write_meta_page(struct page *page,
> >  		goto redirty_out;
> >  	if (wbc->for_reclaim)
> >  		goto redirty_out;
> > -
> > -	/* Should not write any meta pages, if any IO error was occurred */
> >  	if (unlikely(f2fs_cp_error(sbi)))
> > -		goto no_write;
> > +		goto redirty_out;
> > 
> >  	f2fs_wait_on_page_writeback(page, META);
> >  	write_meta_page(sbi, page);
> > -no_write:
> >  	dec_page_count(sbi, F2FS_DIRTY_META);
> >  	unlock_page(page);
> >  	return 0;
> > @@ -348,7 +345,7 @@ bool exist_written_data(struct f2fs_sb_info *sbi, nid_t ino, int mode)
> >  	return e ? true : false;
> >  }
> > 
> > -static void release_dirty_inode(struct f2fs_sb_info *sbi)
> > +void release_dirty_inode(struct f2fs_sb_info *sbi)
> >  {
> >  	struct ino_entry *e, *tmp;
> >  	int i;
> > @@ -737,7 +734,7 @@ retry:
> >  /*
> >   * Freeze all the FS-operations for checkpoint.
> >   */
> > -static void block_operations(struct f2fs_sb_info *sbi)
> > +static int block_operations(struct f2fs_sb_info *sbi)
> >  {
> >  	struct writeback_control wbc = {
> >  		.sync_mode = WB_SYNC_ALL,
> > @@ -745,6 +742,7 @@ static void block_operations(struct f2fs_sb_info *sbi)
> >  		.for_reclaim = 0,
> >  	};
> >  	struct blk_plug plug;
> > +	int err = 0;
> > 
> >  	blk_start_plug(&plug);
> > 
> > @@ -754,6 +752,10 @@ retry_flush_dents:
> >  	if (get_pages(sbi, F2FS_DIRTY_DENTS)) {
> >  		f2fs_unlock_all(sbi);
> >  		sync_dirty_dir_inodes(sbi);
> > +		if (unlikely(f2fs_cp_error(sbi))) {
> > +			err = -EIO;
> > +			goto out;
> > +		}
> >  		goto retry_flush_dents;
> >  	}
> > 
> > @@ -767,9 +769,16 @@ retry_flush_nodes:
> >  	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
> >  		up_write(&sbi->node_write);
> >  		sync_node_pages(sbi, 0, &wbc);
> > +		if (unlikely(f2fs_cp_error(sbi))) {
> > +			f2fs_unlock_all(sbi);
> > +			err = -EIO;
> > +			goto out;
> > +		}
> >  		goto retry_flush_nodes;
> >  	}
> > +out:
> >  	blk_finish_plug(&plug);
> > +	return err;
> >  }
> > 
> >  static void unblock_operations(struct f2fs_sb_info *sbi)
> > @@ -813,8 +822,11 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> >  	discard_next_dnode(sbi, NEXT_FREE_BLKADDR(sbi, curseg));
> > 
> >  	/* Flush all the NAT/SIT pages */
> > -	while (get_pages(sbi, F2FS_DIRTY_META))
> > +	while (get_pages(sbi, F2FS_DIRTY_META)) {
> >  		sync_meta_pages(sbi, META, LONG_MAX);
> > +		if (unlikely(f2fs_cp_error(sbi)))
> > +			return;
> > +	}
> > 
> >  	next_free_nid(sbi, &last_nid);
> > 
> > @@ -931,14 +943,19 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> >  	sbi->last_valid_block_count = sbi->total_valid_block_count;
> >  	sbi->alloc_valid_block_count = 0;
> > 
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		return;
> > +
> >  	/* Here, we only have one bio having CP pack */
> >  	sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
> > 
> > -	if (!f2fs_cp_error(sbi)) {
> > -		clear_prefree_segments(sbi);
> > -		release_dirty_inode(sbi);
> > -		F2FS_RESET_SB_DIRT(sbi);
> > -	}
> > +	release_dirty_inode(sbi);
> > +
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		return;
> > +
> > +	clear_prefree_segments(sbi);
> > +	F2FS_RESET_SB_DIRT(sbi);
> >  }
> > 
> >  /*
> > @@ -955,8 +972,10 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> > 
> >  	if (!sbi->s_dirty)
> >  		goto out;
> > -
> > -	block_operations(sbi);
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		goto out;
> > +	if (block_operations(sbi))
> > +		goto out;
> > 
> >  	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish block_ops");
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index ac3ccc2..68834e2 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -53,7 +53,7 @@ static void f2fs_write_end_io(struct bio *bio, int err)
> >  		struct page *page = bvec->bv_page;
> > 
> >  		if (unlikely(err)) {
> > -			SetPageError(page);
> > +			set_page_dirty(page);
> >  			set_bit(AS_EIO, &page->mapping->flags);
> >  			f2fs_stop_checkpoint(sbi);
> >  		}
> > @@ -836,10 +836,18 @@ write:
> > 
> >  	/* Dentry blocks are controlled by checkpoint */
> >  	if (S_ISDIR(inode->i_mode)) {
> > +		if (unlikely(f2fs_cp_error(sbi)))
> > +			goto redirty_out;
> >  		err = do_write_data_page(page, &fio);
> >  		goto done;
> >  	}
> > 
> > +	if (unlikely(f2fs_cp_error(sbi))) {
> > +		SetPageError(page);
> > +		unlock_page(page);
> > +		return 0;
> 
> Without redirtying it, we may lose cached page here which against the
> second rule.

Ya, I need to change the description for this. If we call redirty_out for all
the dirty data pages, kworker cannot finish the job, resulting in 98% of cpu to
retry flushes. This can be found through xfstests/019.
So, for that reason, we need to drop user data for now.
It might be worth to do something to support removable devices later.

Thanks,

> 
> Thanks,
> Yu
> 
> 
> > +	}
> > +
> >  	if (!wbc->for_reclaim)
> >  		need_balance_fs = true;
> >  	else if (has_not_enough_free_secs(sbi, 0))
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 46ceb88..2d009ae 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -1267,6 +1267,7 @@ int ra_meta_pages(struct f2fs_sb_info *, int, int, int);
> >  long sync_meta_pages(struct f2fs_sb_info *, enum page_type, long);
> >  void add_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
> >  void remove_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
> > +void release_dirty_inode(struct f2fs_sb_info *);
> >  bool exist_written_data(struct f2fs_sb_info *, nid_t, int);
> >  int acquire_orphan_inode(struct f2fs_sb_info *);
> >  void release_orphan_inode(struct f2fs_sb_info *);
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 9f126f8..d2f7842 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -1215,6 +1215,8 @@ static int f2fs_write_node_page(struct page *page,
> > 
> >  	if (unlikely(sbi->por_doing))
> >  		goto redirty_out;
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		goto redirty_out;
> > 
> >  	f2fs_wait_on_page_writeback(page, NODE);
> > 
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index c4f90cd..c999d67 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -435,6 +435,9 @@ static void f2fs_put_super(struct super_block *sb)
> >  	if (sbi->s_dirty)
> >  		write_checkpoint(sbi, true);
> > 
> > +	/* EIO will skip do checkpoint, so here we need to release this */
> > +	release_dirty_inode(sbi);
> > +
> >  	iput(sbi->node_inode);
> >  	iput(sbi->meta_inode);
> > 
> > --
> > 1.8.5.2 (Apple Git-48)
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 13/13] f2fs: handle EIO not to break fs consistency

[Jaegeuk Kim]

On Wed, Aug 20, 2014 at 06:35:18PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Wednesday, August 13, 2014 3:49 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency
> > 
> > There are two rules when EIO is occurred.
> > 1. don't write any checkpoint data to preserve the previous checkpoint
> > 2. don't lose the cached data/node/meta pages
> > 
> > So, at first, this patch adds set_page_dirty in f2fs_write_end_io's failure.
> > Then, writing checkpoint/dentry/node blocks is not allowed.
> 
> do_checkpoint()
> 	/* wait for previous submitted node/meta pages writeback */
> 	wait_on_all_pages_writeback(sbi);
> 
> 	if (unlikely(f2fs_cp_error(sbi)))
> 		return;
> add here to prevent unneeded syncing for meta/node pages.

Well, there is one below. But it's not a big deal to change the location.

> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/checkpoint.c | 47 +++++++++++++++++++++++++++++++++--------------
> >  fs/f2fs/data.c       | 10 +++++++++-
> >  fs/f2fs/f2fs.h       |  1 +
> >  fs/f2fs/node.c       |  2 ++
> >  fs/f2fs/super.c      |  3 +++
> >  5 files changed, 48 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index d074e27..b365c64 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -160,14 +160,11 @@ static int f2fs_write_meta_page(struct page *page,
> >  		goto redirty_out;
> >  	if (wbc->for_reclaim)
> >  		goto redirty_out;
> > -
> > -	/* Should not write any meta pages, if any IO error was occurred */
> >  	if (unlikely(f2fs_cp_error(sbi)))
> > -		goto no_write;
> > +		goto redirty_out;
> > 
> >  	f2fs_wait_on_page_writeback(page, META);
> >  	write_meta_page(sbi, page);
> > -no_write:
> >  	dec_page_count(sbi, F2FS_DIRTY_META);
> >  	unlock_page(page);
> >  	return 0;
> > @@ -348,7 +345,7 @@ bool exist_written_data(struct f2fs_sb_info *sbi, nid_t ino, int mode)
> >  	return e ? true : false;
> >  }
> > 
> > -static void release_dirty_inode(struct f2fs_sb_info *sbi)
> > +void release_dirty_inode(struct f2fs_sb_info *sbi)
> >  {
> >  	struct ino_entry *e, *tmp;
> >  	int i;
> > @@ -737,7 +734,7 @@ retry:
> >  /*
> >   * Freeze all the FS-operations for checkpoint.
> >   */
> > -static void block_operations(struct f2fs_sb_info *sbi)
> > +static int block_operations(struct f2fs_sb_info *sbi)
> >  {
> >  	struct writeback_control wbc = {
> >  		.sync_mode = WB_SYNC_ALL,
> > @@ -745,6 +742,7 @@ static void block_operations(struct f2fs_sb_info *sbi)
> >  		.for_reclaim = 0,
> >  	};
> >  	struct blk_plug plug;
> > +	int err = 0;
> > 
> >  	blk_start_plug(&plug);
> > 
> > @@ -754,6 +752,10 @@ retry_flush_dents:
> >  	if (get_pages(sbi, F2FS_DIRTY_DENTS)) {
> >  		f2fs_unlock_all(sbi);
> >  		sync_dirty_dir_inodes(sbi);
> > +		if (unlikely(f2fs_cp_error(sbi))) {
> > +			err = -EIO;
> > +			goto out;
> > +		}
> >  		goto retry_flush_dents;
> >  	}
> > 
> > @@ -767,9 +769,16 @@ retry_flush_nodes:
> >  	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
> >  		up_write(&sbi->node_write);
> >  		sync_node_pages(sbi, 0, &wbc);
> > +		if (unlikely(f2fs_cp_error(sbi))) {
> > +			f2fs_unlock_all(sbi);
> > +			err = -EIO;
> > +			goto out;
> > +		}
> >  		goto retry_flush_nodes;
> >  	}
> > +out:
> >  	blk_finish_plug(&plug);
> > +	return err;
> >  }
> > 
> >  static void unblock_operations(struct f2fs_sb_info *sbi)
> > @@ -813,8 +822,11 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> >  	discard_next_dnode(sbi, NEXT_FREE_BLKADDR(sbi, curseg));
> > 
> >  	/* Flush all the NAT/SIT pages */
> > -	while (get_pages(sbi, F2FS_DIRTY_META))
> > +	while (get_pages(sbi, F2FS_DIRTY_META)) {
> >  		sync_meta_pages(sbi, META, LONG_MAX);
> > +		if (unlikely(f2fs_cp_error(sbi)))
> > +			return;
> > +	}
> > 
> >  	next_free_nid(sbi, &last_nid);
> > 
> > @@ -931,14 +943,19 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> >  	sbi->last_valid_block_count = sbi->total_valid_block_count;
> >  	sbi->alloc_valid_block_count = 0;
> > 
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		return;
> > +
> >  	/* Here, we only have one bio having CP pack */
> >  	sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
> > 
> > -	if (!f2fs_cp_error(sbi)) {
> > -		clear_prefree_segments(sbi);
> > -		release_dirty_inode(sbi);
> > -		F2FS_RESET_SB_DIRT(sbi);
> > -	}
> > +	release_dirty_inode(sbi);
> > +
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		return;
> > +
> > +	clear_prefree_segments(sbi);
> > +	F2FS_RESET_SB_DIRT(sbi);
> >  }
> > 
> >  /*
> > @@ -955,8 +972,10 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> > 
> >  	if (!sbi->s_dirty)
> >  		goto out;
> > -
> > -	block_operations(sbi);
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		goto out;
> > +	if (block_operations(sbi))
> > +		goto out;
> > 
> >  	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish block_ops");
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index ac3ccc2..68834e2 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -53,7 +53,7 @@ static void f2fs_write_end_io(struct bio *bio, int err)
> >  		struct page *page = bvec->bv_page;
> > 
> >  		if (unlikely(err)) {
> > -			SetPageError(page);
> > +			set_page_dirty(page);
> >  			set_bit(AS_EIO, &page->mapping->flags);
> >  			f2fs_stop_checkpoint(sbi);
> >  		}
> > @@ -836,10 +836,18 @@ write:
> > 
> >  	/* Dentry blocks are controlled by checkpoint */
> >  	if (S_ISDIR(inode->i_mode)) {
> > +		if (unlikely(f2fs_cp_error(sbi)))
> > +			goto redirty_out;
> >  		err = do_write_data_page(page, &fio);
> >  		goto done;
> >  	}
> > 
> > +	if (unlikely(f2fs_cp_error(sbi))) {
> > +		SetPageError(page);
> > +		unlock_page(page);
> > +		return 0;
> 
> Without redirtying it, we may lose cached page here which against the
> second rule.

Ya, I need to change the description for this. If we call redirty_out for all
the dirty data pages, kworker cannot finish the job, resulting in 98% of cpu to
retry flushes. This can be found through xfstests/019.
So, for that reason, we need to drop user data for now.
It might be worth to do something to support removable devices later.

Thanks,

> 
> Thanks,
> Yu
> 
> 
> > +	}
> > +
> >  	if (!wbc->for_reclaim)
> >  		need_balance_fs = true;
> >  	else if (has_not_enough_free_secs(sbi, 0))
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 46ceb88..2d009ae 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -1267,6 +1267,7 @@ int ra_meta_pages(struct f2fs_sb_info *, int, int, int);
> >  long sync_meta_pages(struct f2fs_sb_info *, enum page_type, long);
> >  void add_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
> >  void remove_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
> > +void release_dirty_inode(struct f2fs_sb_info *);
> >  bool exist_written_data(struct f2fs_sb_info *, nid_t, int);
> >  int acquire_orphan_inode(struct f2fs_sb_info *);
> >  void release_orphan_inode(struct f2fs_sb_info *);
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 9f126f8..d2f7842 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -1215,6 +1215,8 @@ static int f2fs_write_node_page(struct page *page,
> > 
> >  	if (unlikely(sbi->por_doing))
> >  		goto redirty_out;
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		goto redirty_out;
> > 
> >  	f2fs_wait_on_page_writeback(page, NODE);
> > 
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index c4f90cd..c999d67 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -435,6 +435,9 @@ static void f2fs_put_super(struct super_block *sb)
> >  	if (sbi->s_dirty)
> >  		write_checkpoint(sbi, true);
> > 
> > +	/* EIO will skip do checkpoint, so here we need to release this */
> > +	release_dirty_inode(sbi);
> > +
> >  	iput(sbi->node_inode);
> >  	iput(sbi->meta_inode);
> > 
> > --
> > 1.8.5.2 (Apple Git-48)
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

RE: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, August 22, 2014 4:34 AM
> To: Chao Yu
> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency
> 
> On Wed, Aug 20, 2014 at 06:35:18PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Wednesday, August 13, 2014 3:49 AM
> > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > > linux-f2fs-devel@lists.sourceforge.net
> > > Cc: Jaegeuk Kim
> > > Subject: [f2fs-dev] [PATCH 13/13] f2fs: handle EIO not to break fs consistency
> > >
> > > There are two rules when EIO is occurred.
> > > 1. don't write any checkpoint data to preserve the previous checkpoint
> > > 2. don't lose the cached data/node/meta pages
> > >
> > > So, at first, this patch adds set_page_dirty in f2fs_write_end_io's failure.
> > > Then, writing checkpoint/dentry/node blocks is not allowed.
> >
> > do_checkpoint()
> > 	/* wait for previous submitted node/meta pages writeback */
> > 	wait_on_all_pages_writeback(sbi);
> >
> > 	if (unlikely(f2fs_cp_error(sbi)))
> > 		return;
> > add here to prevent unneeded syncing for meta/node pages.
> 
> Well, there is one below. But it's not a big deal to change the location.

I saw the last code in git respository, it's not like what I thought.
What I meant is to add one additionally here, not moving from below to here.
It's my mistaken that having expressed unclearly, but like what you said,
it's not a big deal, let's pass it.

> 
> >
> > >
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/checkpoint.c | 47 +++++++++++++++++++++++++++++++++--------------
> > >  fs/f2fs/data.c       | 10 +++++++++-
> > >  fs/f2fs/f2fs.h       |  1 +
> > >  fs/f2fs/node.c       |  2 ++
> > >  fs/f2fs/super.c      |  3 +++
> > >  5 files changed, 48 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > > index d074e27..b365c64 100644
> > > --- a/fs/f2fs/checkpoint.c
> > > +++ b/fs/f2fs/checkpoint.c
> > > @@ -160,14 +160,11 @@ static int f2fs_write_meta_page(struct page *page,
> > >  		goto redirty_out;
> > >  	if (wbc->for_reclaim)
> > >  		goto redirty_out;
> > > -
> > > -	/* Should not write any meta pages, if any IO error was occurred */
> > >  	if (unlikely(f2fs_cp_error(sbi)))
> > > -		goto no_write;
> > > +		goto redirty_out;
> > >
> > >  	f2fs_wait_on_page_writeback(page, META);
> > >  	write_meta_page(sbi, page);
> > > -no_write:
> > >  	dec_page_count(sbi, F2FS_DIRTY_META);
> > >  	unlock_page(page);
> > >  	return 0;
> > > @@ -348,7 +345,7 @@ bool exist_written_data(struct f2fs_sb_info *sbi, nid_t ino, int mode)
> > >  	return e ? true : false;
> > >  }
> > >
> > > -static void release_dirty_inode(struct f2fs_sb_info *sbi)
> > > +void release_dirty_inode(struct f2fs_sb_info *sbi)
> > >  {
> > >  	struct ino_entry *e, *tmp;
> > >  	int i;
> > > @@ -737,7 +734,7 @@ retry:
> > >  /*
> > >   * Freeze all the FS-operations for checkpoint.
> > >   */
> > > -static void block_operations(struct f2fs_sb_info *sbi)
> > > +static int block_operations(struct f2fs_sb_info *sbi)
> > >  {
> > >  	struct writeback_control wbc = {
> > >  		.sync_mode = WB_SYNC_ALL,
> > > @@ -745,6 +742,7 @@ static void block_operations(struct f2fs_sb_info *sbi)
> > >  		.for_reclaim = 0,
> > >  	};
> > >  	struct blk_plug plug;
> > > +	int err = 0;
> > >
> > >  	blk_start_plug(&plug);
> > >
> > > @@ -754,6 +752,10 @@ retry_flush_dents:
> > >  	if (get_pages(sbi, F2FS_DIRTY_DENTS)) {
> > >  		f2fs_unlock_all(sbi);
> > >  		sync_dirty_dir_inodes(sbi);
> > > +		if (unlikely(f2fs_cp_error(sbi))) {
> > > +			err = -EIO;
> > > +			goto out;
> > > +		}
> > >  		goto retry_flush_dents;
> > >  	}
> > >
> > > @@ -767,9 +769,16 @@ retry_flush_nodes:
> > >  	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
> > >  		up_write(&sbi->node_write);
> > >  		sync_node_pages(sbi, 0, &wbc);
> > > +		if (unlikely(f2fs_cp_error(sbi))) {
> > > +			f2fs_unlock_all(sbi);
> > > +			err = -EIO;
> > > +			goto out;
> > > +		}
> > >  		goto retry_flush_nodes;
> > >  	}
> > > +out:
> > >  	blk_finish_plug(&plug);
> > > +	return err;
> > >  }
> > >
> > >  static void unblock_operations(struct f2fs_sb_info *sbi)
> > > @@ -813,8 +822,11 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> > >  	discard_next_dnode(sbi, NEXT_FREE_BLKADDR(sbi, curseg));
> > >
> > >  	/* Flush all the NAT/SIT pages */
> > > -	while (get_pages(sbi, F2FS_DIRTY_META))
> > > +	while (get_pages(sbi, F2FS_DIRTY_META)) {
> > >  		sync_meta_pages(sbi, META, LONG_MAX);
> > > +		if (unlikely(f2fs_cp_error(sbi)))
> > > +			return;
> > > +	}
> > >
> > >  	next_free_nid(sbi, &last_nid);
> > >
> > > @@ -931,14 +943,19 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> > >  	sbi->last_valid_block_count = sbi->total_valid_block_count;
> > >  	sbi->alloc_valid_block_count = 0;
> > >
> > > +	if (unlikely(f2fs_cp_error(sbi)))
> > > +		return;
> > > +
> > >  	/* Here, we only have one bio having CP pack */
> > >  	sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
> > >
> > > -	if (!f2fs_cp_error(sbi)) {
> > > -		clear_prefree_segments(sbi);
> > > -		release_dirty_inode(sbi);
> > > -		F2FS_RESET_SB_DIRT(sbi);
> > > -	}
> > > +	release_dirty_inode(sbi);
> > > +
> > > +	if (unlikely(f2fs_cp_error(sbi)))
> > > +		return;
> > > +
> > > +	clear_prefree_segments(sbi);
> > > +	F2FS_RESET_SB_DIRT(sbi);
> > >  }
> > >
> > >  /*
> > > @@ -955,8 +972,10 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> > >
> > >  	if (!sbi->s_dirty)
> > >  		goto out;
> > > -
> > > -	block_operations(sbi);
> > > +	if (unlikely(f2fs_cp_error(sbi)))
> > > +		goto out;
> > > +	if (block_operations(sbi))
> > > +		goto out;
> > >
> > >  	trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish block_ops");
> > >
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index ac3ccc2..68834e2 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -53,7 +53,7 @@ static void f2fs_write_end_io(struct bio *bio, int err)
> > >  		struct page *page = bvec->bv_page;
> > >
> > >  		if (unlikely(err)) {
> > > -			SetPageError(page);
> > > +			set_page_dirty(page);
> > >  			set_bit(AS_EIO, &page->mapping->flags);
> > >  			f2fs_stop_checkpoint(sbi);
> > >  		}
> > > @@ -836,10 +836,18 @@ write:
> > >
> > >  	/* Dentry blocks are controlled by checkpoint */
> > >  	if (S_ISDIR(inode->i_mode)) {
> > > +		if (unlikely(f2fs_cp_error(sbi)))
> > > +			goto redirty_out;
> > >  		err = do_write_data_page(page, &fio);
> > >  		goto done;
> > >  	}
> > >
> > > +	if (unlikely(f2fs_cp_error(sbi))) {
> > > +		SetPageError(page);
> > > +		unlock_page(page);
> > > +		return 0;
> >
> > Without redirtying it, we may lose cached page here which against the
> > second rule.
> 
> Ya, I need to change the description for this. If we call redirty_out for all
> the dirty data pages, kworker cannot finish the job, resulting in 98% of cpu to
> retry flushes. This can be found through xfstests/019.
> So, for that reason, we need to drop user data for now.
> It might be worth to do something to support removable devices later.

Oh, got it, and thanks for your explanation.

Regards,
Yu

> 
> Thanks,
> 
> >
> > Thanks,
> > Yu
> >
> >
> > > +	}
> > > +
> > >  	if (!wbc->for_reclaim)
> > >  		need_balance_fs = true;
> > >  	else if (has_not_enough_free_secs(sbi, 0))
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 46ceb88..2d009ae 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -1267,6 +1267,7 @@ int ra_meta_pages(struct f2fs_sb_info *, int, int, int);
> > >  long sync_meta_pages(struct f2fs_sb_info *, enum page_type, long);
> > >  void add_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
> > >  void remove_dirty_inode(struct f2fs_sb_info *, nid_t, int type);
> > > +void release_dirty_inode(struct f2fs_sb_info *);
> > >  bool exist_written_data(struct f2fs_sb_info *, nid_t, int);
> > >  int acquire_orphan_inode(struct f2fs_sb_info *);
> > >  void release_orphan_inode(struct f2fs_sb_info *);
> > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > > index 9f126f8..d2f7842 100644
> > > --- a/fs/f2fs/node.c
> > > +++ b/fs/f2fs/node.c
> > > @@ -1215,6 +1215,8 @@ static int f2fs_write_node_page(struct page *page,
> > >
> > >  	if (unlikely(sbi->por_doing))
> > >  		goto redirty_out;
> > > +	if (unlikely(f2fs_cp_error(sbi)))
> > > +		goto redirty_out;
> > >
> > >  	f2fs_wait_on_page_writeback(page, NODE);
> > >
> > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > index c4f90cd..c999d67 100644
> > > --- a/fs/f2fs/super.c
> > > +++ b/fs/f2fs/super.c
> > > @@ -435,6 +435,9 @@ static void f2fs_put_super(struct super_block *sb)
> > >  	if (sbi->s_dirty)
> > >  		write_checkpoint(sbi, true);
> > >
> > > +	/* EIO will skip do checkpoint, so here we need to release this */
> > > +	release_dirty_inode(sbi);
> > > +
> > >  	iput(sbi->node_inode);
> > >  	iput(sbi->meta_inode);
> > >
> > > --
> > > 1.8.5.2 (Apple Git-48)
> > >
> > >
> > > ------------------------------------------------------------------------------
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

RE: [f2fs-dev] [PATCH 01/13] f2fs: should convert inline_data during the mkwrite

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 13, 2014 3:49 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 01/13] f2fs: should convert inline_data during the mkwrite
> 
> If mkwrite is called to an inode having inline_data, it can overwrite the data
> index space as NEW_ADDR. (e.g., the first 4 bytes are coincidently zero)

Good catch!

As I did some simple tests, I found that thread will receive SIGBUS when
operating mmaped page exceed file size, so here, we will meet a inline data
inode only when we mkwrite the first page.
Please correct me if I'm wrong.

In this case, why not just do f2fs_read_inline_data & skip f2fs_reserve_block,
instead of converting inline_data and following f2fs_write_inline_data in
->writepage().

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c   |  2 +-
>  fs/f2fs/f2fs.h   |  2 +-
>  fs/f2fs/file.c   | 11 ++++++++---
>  fs/f2fs/inline.c | 20 ++++++++++++--------
>  4 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 7aef28d..ac3ccc2 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -946,7 +946,7 @@ static int f2fs_write_begin(struct file *file, struct address_space
> *mapping,
> 
>  	f2fs_balance_fs(sbi);
>  repeat:
> -	err = f2fs_convert_inline_data(inode, pos + len);
> +	err = f2fs_convert_inline_data(inode, pos + len, NULL);
>  	if (err)
>  		goto fail;
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 790a073..c8288c9 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1439,7 +1439,7 @@ extern const struct inode_operations f2fs_special_inode_operations;
>   */
>  bool f2fs_may_inline(struct inode *);
>  int f2fs_read_inline_data(struct inode *, struct page *);
> -int f2fs_convert_inline_data(struct inode *, pgoff_t);
> +int f2fs_convert_inline_data(struct inode *, pgoff_t, struct page *);
>  int f2fs_write_inline_data(struct inode *, struct page *, unsigned int);
>  void truncate_inline_data(struct inode *, u64);
>  int recover_inline_data(struct inode *, struct page *);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 87cdac4..ecbdf6a 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -41,6 +41,11 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct *vma,
> 
>  	sb_start_pagefault(inode->i_sb);
> 
> +	/* force to convert with normal data indices */
> +	err = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1, page);
> +	if (err)
> +		goto out;
> +
>  	/* block allocation */
>  	f2fs_lock_op(sbi);
>  	set_new_dnode(&dn, inode, NULL, NULL, 0);
> @@ -533,7 +538,7 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
> 
>  	if ((attr->ia_valid & ATTR_SIZE) &&
>  			attr->ia_size != i_size_read(inode)) {
> -		err = f2fs_convert_inline_data(inode, attr->ia_size);
> +		err = f2fs_convert_inline_data(inode, attr->ia_size, NULL);
>  		if (err)
>  			return err;
> 
> @@ -622,7 +627,7 @@ static int punch_hole(struct inode *inode, loff_t offset, loff_t len)
>  	loff_t off_start, off_end;
>  	int ret = 0;
> 
> -	ret = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1);
> +	ret = f2fs_convert_inline_data(inode, MAX_INLINE_DATA + 1, NULL);
>  	if (ret)
>  		return ret;
> 
> @@ -678,7 +683,7 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
>  	if (ret)
>  		return ret;
> 
> -	ret = f2fs_convert_inline_data(inode, offset + len);
> +	ret = f2fs_convert_inline_data(inode, offset + len, NULL);
>  	if (ret)
>  		return ret;
> 
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index 5beecce..1ec512d 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -124,9 +124,10 @@ out:
>  	return err;
>  }
> 
> -int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size)
> +int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size,
> +						struct page *page)
>  {
> -	struct page *page;
> +	struct page *new_page = page;
>  	int err;
> 
>  	if (!f2fs_has_inline_data(inode))
> @@ -134,17 +135,20 @@ int f2fs_convert_inline_data(struct inode *inode, pgoff_t to_size)
>  	else if (to_size <= MAX_INLINE_DATA)
>  		return 0;
> 
> -	page = grab_cache_page(inode->i_mapping, 0);
> -	if (!page)
> -		return -ENOMEM;
> +	if (!page || page->index != 0) {
> +		new_page = grab_cache_page(inode->i_mapping, 0);
> +		if (!new_page)
> +			return -ENOMEM;
> +	}
> 
> -	err = __f2fs_convert_inline_data(inode, page);
> -	f2fs_put_page(page, 1);
> +	err = __f2fs_convert_inline_data(inode, new_page);
> +	if (!page || page->index != 0)
> +		f2fs_put_page(new_page, 1);
>  	return err;
>  }
> 
>  int f2fs_write_inline_data(struct inode *inode,
> -			   struct page *page, unsigned size)
> +				struct page *page, unsigned size)
>  {
>  	void *src_addr, *dst_addr;
>  	struct page *ipage;
> --
> 1.8.5.2 (Apple Git-48)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel