[PATCH 1/2] f2fs: report ENOENT correct in f2fs_rename

[PATCH 1/2] f2fs: report ENOENT correct in f2fs_rename

[Jaegeuk Kim]

This fixes wrong error report in f2fs_rename.

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

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 98d3ab7c3ce6..d653be777529 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -833,7 +833,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct f2fs_dir_entry *old_entry;
 	struct f2fs_dir_entry *new_entry;
 	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
-	int err = -ENOENT;
+	int err;
 
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
@@ -854,6 +854,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (err)
 		goto out;
 
+	err = -ENOENT;
 	if (new_inode) {
 		err = dquot_initialize(new_inode);
 		if (err)
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 2/2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
xfstests/generic/475.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c |  2 +-
 fs/f2fs/gc.c         |  2 ++
 fs/f2fs/node.c       | 12 ++++++++++--
 fs/f2fs/recovery.c   |  2 ++
 fs/f2fs/segment.c    |  3 +++
 5 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 01e0d8f5bbbe..6ce3cb6502dd 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -121,7 +121,7 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
 			goto retry;
 
 		f2fs_stop_checkpoint(sbi, false);
-		f2fs_bug_on(sbi, 1);
+		return NULL;
 	}
 
 	return page;
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 4bcc8a59fdef..d049865887cf 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1070,6 +1070,8 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
 	/* reference all summary page */
 	while (segno < end_segno) {
 		sum_page = f2fs_get_sum_page(sbi, segno++);
+		if (!sum_page)
+			return -EIO;
 		unlock_page(sum_page);
 	}
 
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index fa2381c0bc47..b3595522c35b 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
 
 	/* get current nat block page with lock */
 	src_page = get_current_nat_page(sbi, nid);
+	if (!src_page)
+		return NULL;
 	dst_page = f2fs_grab_meta_page(sbi, dst_off);
 	f2fs_bug_on(sbi, PageDirty(src_page));
 
@@ -2265,8 +2267,12 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
 						nm_i->nat_block_bitmap)) {
 			struct page *page = get_current_nat_page(sbi, nid);
 
-			ret = scan_nat_page(sbi, page, nid);
-			f2fs_put_page(page, 1);
+			if (page) {
+				ret = scan_nat_page(sbi, page, nid);
+				f2fs_put_page(page, 1);
+			} else {
+				ret = -EIO;
+			}
 
 			if (ret) {
 				up_read(&nm_i->nat_tree_lock);
@@ -2724,6 +2730,8 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 		down_write(&curseg->journal_rwsem);
 	} else {
 		page = get_next_nat_page(sbi, start_nid);
+		if (!page)
+			return;
 		nat_blk = page_address(page);
 		f2fs_bug_on(sbi, !nat_blk);
 	}
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 56d34193a74b..a3dce16bfd6c 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
 	}
 
 	sum_page = f2fs_get_sum_page(sbi, segno);
+	if (!sum_page)
+		return -EIO;
 	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
 	sum = sum_node->entries[blkoff];
 	f2fs_put_page(sum_page, 1);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index aa96a371aaf8..cfc9eb492da1 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
 	__next_free_blkoff(sbi, curseg, 0);
 
 	sum_page = f2fs_get_sum_page(sbi, new_segno);
+	f2fs_bug_on(sbi, !sum_page);
 	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
 	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
 	f2fs_put_page(sum_page, 1);
@@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
 
 			se = &sit_i->sentries[start];
 			page = get_current_sit_page(sbi, start);
+			if (!page)
+				return err;
 			sit_blk = (struct f2fs_sit_block *)page_address(page);
 			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
 			f2fs_put_page(page, 1);
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018/9/18 10:18, Jaegeuk Kim wrote:
> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> xfstests/generic/475.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/checkpoint.c |  2 +-
>  fs/f2fs/gc.c         |  2 ++
>  fs/f2fs/node.c       | 12 ++++++++++--
>  fs/f2fs/recovery.c   |  2 ++
>  fs/f2fs/segment.c    |  3 +++
>  5 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 01e0d8f5bbbe..6ce3cb6502dd 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -121,7 +121,7 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
>  			goto retry;
>  
>  		f2fs_stop_checkpoint(sbi, false);
> -		f2fs_bug_on(sbi, 1);
> +		return NULL;

How about propagate PTR_ERR(page) to caller?

>  	}
>  
>  	return page;
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 4bcc8a59fdef..d049865887cf 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1070,6 +1070,8 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>  	/* reference all summary page */
>  	while (segno < end_segno) {
>  		sum_page = f2fs_get_sum_page(sbi, segno++);
> +		if (!sum_page)
> +			return -EIO;

Well, for large section, we need to release all referenced sum page by
f2fs_put_page().

>  		unlock_page(sum_page);
>  	}
>  
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index fa2381c0bc47..b3595522c35b 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
>  
>  	/* get current nat block page with lock */
>  	src_page = get_current_nat_page(sbi, nid);
> +	if (!src_page)
> +		return NULL;
>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
>  	f2fs_bug_on(sbi, PageDirty(src_page));
>  
> @@ -2265,8 +2267,12 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>  						nm_i->nat_block_bitmap)) {
>  			struct page *page = get_current_nat_page(sbi, nid);
>  
> -			ret = scan_nat_page(sbi, page, nid);
> -			f2fs_put_page(page, 1);
> +			if (page) {
> +				ret = scan_nat_page(sbi, page, nid);
> +				f2fs_put_page(page, 1);
> +			} else {
> +				ret = -EIO;
> +			}
>  
>  			if (ret) {
>  				up_read(&nm_i->nat_tree_lock);

Should propagate the error to f2fs_alloc_nid()?

> @@ -2724,6 +2730,8 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>  		down_write(&curseg->journal_rwsem);
>  	} else {
>  		page = get_next_nat_page(sbi, start_nid);
> +		if (!page)
> +			return;

Ditto, propagate such error to write_checkpoint()?

>  		nat_blk = page_address(page);
>  		f2fs_bug_on(sbi, !nat_blk);
>  	}
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 56d34193a74b..a3dce16bfd6c 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
>  	}
>  
>  	sum_page = f2fs_get_sum_page(sbi, segno);
> +	if (!sum_page)
> +		return -EIO;
>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>  	sum = sum_node->entries[blkoff];
>  	f2fs_put_page(sum_page, 1);
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index aa96a371aaf8..cfc9eb492da1 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>  	__next_free_blkoff(sbi, curseg, 0);
>  
>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> +	f2fs_bug_on(sbi, !sum_page);

Well, next time we may panic here...

In product, for EIO case, usually we just reboot cell phone directly to avoid
potential data loss later.

So I just set DEFAULT_RETRY_IO_COUNT to 32 temporarily to pass xfstest IO error
injection cases.

Thanks,

>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
>  	f2fs_put_page(sum_page, 1);
> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
>  
>  			se = &sit_i->sentries[start];
>  			page = get_current_sit_page(sbi, start);
> +			if (!page)
> +				return err;
>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
>  			f2fs_put_page(page, 1);
> 

Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

On 09/18, Chao Yu wrote:
> On 2018/9/18 10:18, Jaegeuk Kim wrote:
> > This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> > xfstests/generic/475.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/checkpoint.c |  2 +-
> >  fs/f2fs/gc.c         |  2 ++
> >  fs/f2fs/node.c       | 12 ++++++++++--
> >  fs/f2fs/recovery.c   |  2 ++
> >  fs/f2fs/segment.c    |  3 +++
> >  5 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 01e0d8f5bbbe..6ce3cb6502dd 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -121,7 +121,7 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
> >  			goto retry;
> >  
> >  		f2fs_stop_checkpoint(sbi, false);
> > -		f2fs_bug_on(sbi, 1);
> > +		return NULL;
> 
> How about propagate PTR_ERR(page) to caller?

Done.

> 
> >  	}
> >  
> >  	return page;
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 4bcc8a59fdef..d049865887cf 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -1070,6 +1070,8 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >  	/* reference all summary page */
> >  	while (segno < end_segno) {
> >  		sum_page = f2fs_get_sum_page(sbi, segno++);
> > +		if (!sum_page)
> > +			return -EIO;
> 
> Well, for large section, we need to release all referenced sum page by
> f2fs_put_page().

Done.

> 
> >  		unlock_page(sum_page);
> >  	}
> >  
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index fa2381c0bc47..b3595522c35b 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
> >  
> >  	/* get current nat block page with lock */
> >  	src_page = get_current_nat_page(sbi, nid);
> > +	if (!src_page)
> > +		return NULL;
> >  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
> >  	f2fs_bug_on(sbi, PageDirty(src_page));
> >  
> > @@ -2265,8 +2267,12 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> >  						nm_i->nat_block_bitmap)) {
> >  			struct page *page = get_current_nat_page(sbi, nid);
> >  
> > -			ret = scan_nat_page(sbi, page, nid);
> > -			f2fs_put_page(page, 1);
> > +			if (page) {
> > +				ret = scan_nat_page(sbi, page, nid);
> > +				f2fs_put_page(page, 1);
> > +			} else {
> > +				ret = -EIO;
> > +			}
> >  
> >  			if (ret) {
> >  				up_read(&nm_i->nat_tree_lock);
> 
> Should propagate the error to f2fs_alloc_nid()?

Done.

> 
> > @@ -2724,6 +2730,8 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >  		down_write(&curseg->journal_rwsem);
> >  	} else {
> >  		page = get_next_nat_page(sbi, start_nid);
> > +		if (!page)
> > +			return;
> 
> Ditto, propagate such error to write_checkpoint()?

Done.

> 
> >  		nat_blk = page_address(page);
> >  		f2fs_bug_on(sbi, !nat_blk);
> >  	}
> > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> > index 56d34193a74b..a3dce16bfd6c 100644
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
> >  	}
> >  
> >  	sum_page = f2fs_get_sum_page(sbi, segno);
> > +	if (!sum_page)
> > +		return -EIO;
> >  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >  	sum = sum_node->entries[blkoff];
> >  	f2fs_put_page(sum_page, 1);
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index aa96a371aaf8..cfc9eb492da1 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
> >  	__next_free_blkoff(sbi, curseg, 0);
> >  
> >  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> > +	f2fs_bug_on(sbi, !sum_page);
> 
> Well, next time we may panic here...

This is the same as before, and it's almost impossible to hit anyway in
production.

Thanks,

> 
> In product, for EIO case, usually we just reboot cell phone directly to avoid
> potential data loss later.
> 
> So I just set DEFAULT_RETRY_IO_COUNT to 32 temporarily to pass xfstest IO error
> injection cases.
> 
> Thanks,
> 
> >  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
> >  	f2fs_put_page(sum_page, 1);
> > @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
> >  
> >  			se = &sit_i->sentries[start];
> >  			page = get_current_sit_page(sbi, start);
> > +			if (!page)
> > +				return err;
> >  			sit_blk = (struct f2fs_sit_block *)page_address(page);
> >  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
> >  			f2fs_put_page(page, 1);
> > 

Re: [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
xfstests/generic/475.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
Change log from v1:
 - propagate errno
 - drop sum_pages

 fs/f2fs/checkpoint.c | 10 +++++-----
 fs/f2fs/f2fs.h       |  2 +-
 fs/f2fs/gc.c         |  9 +++++++++
 fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
 fs/f2fs/recovery.c   |  2 ++
 fs/f2fs/segment.c    |  3 +++
 6 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 01e0d8f5bbbe..1ddf332ce4b2 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
 		if (PTR_ERR(page) == -EIO &&
 				++count <= DEFAULT_RETRY_IO_COUNT)
 			goto retry;
-
 		f2fs_stop_checkpoint(sbi, false);
-		f2fs_bug_on(sbi, 1);
 	}
-
 	return page;
 }
 
@@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
 
 	/* write cached NAT/SIT entries to NAT/SIT area */
-	f2fs_flush_nat_entries(sbi, cpc);
+	err = f2fs_flush_nat_entries(sbi, cpc);
+	if (err)
+		goto stop;
+
 	f2fs_flush_sit_entries(sbi, cpc);
 
 	/* unlock all the fs_lock[] in do_checkpoint() */
@@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		f2fs_release_discard_addrs(sbi);
 	else
 		f2fs_clear_prefree_segments(sbi, cpc);
-
+stop:
 	unblock_operations(sbi);
 	stat_inc_cp_count(sbi->stat_info);
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a0c868127a7c..29021dbc8f2a 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
 int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
 int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
 			unsigned int segno, struct f2fs_summary_block *sum);
-void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
+int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
 int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
 void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
 int __init f2fs_create_node_manager_caches(void);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 4bcc8a59fdef..c7d14282dbf4 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
 	/* reference all summary page */
 	while (segno < end_segno) {
 		sum_page = f2fs_get_sum_page(sbi, segno++);
+		if (IS_ERR(sum_page)) {
+			end_segno = segno - 1;
+			for (segno = start_segno; segno < end_segno; segno++) {
+				sum_page = find_get_page(META_MAPPING(sbi),
+						GET_SUM_BLOCK(sbi, segno));
+				f2fs_put_page(sum_page, 0);
+			}
+			return PTR_ERR(sum_page);
+		}
 		unlock_page(sum_page);
 	}
 
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index fa2381c0bc47..79b6fee354f7 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
 
 	/* get current nat block page with lock */
 	src_page = get_current_nat_page(sbi, nid);
+	if (IS_ERR(src_page))
+		return src_page;
 	dst_page = f2fs_grab_meta_page(sbi, dst_off);
 	f2fs_bug_on(sbi, PageDirty(src_page));
 
@@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
 						nm_i->nat_block_bitmap)) {
 			struct page *page = get_current_nat_page(sbi, nid);
 
-			ret = scan_nat_page(sbi, page, nid);
-			f2fs_put_page(page, 1);
+			if (IS_ERR(page)) {
+				ret = PTR_ERR(page);
+			} else {
+				ret = scan_nat_page(sbi, page, nid);
+				f2fs_put_page(page, 1);
+			}
 
 			if (ret) {
 				up_read(&nm_i->nat_tree_lock);
 				f2fs_bug_on(sbi, !mount);
 				f2fs_msg(sbi->sb, KERN_ERR,
 					"NAT is corrupt, run fsck to fix it");
-				return -EINVAL;
+				return ret;
 			}
 		}
 
@@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
 		__clear_bit_le(nat_index, nm_i->full_nat_bits);
 }
 
-static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
+static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 		struct nat_entry_set *set, struct cp_control *cpc)
 {
 	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
@@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 		down_write(&curseg->journal_rwsem);
 	} else {
 		page = get_next_nat_page(sbi, start_nid);
+		if (IS_ERR(page))
+			return PTR_ERR(page);
+
 		nat_blk = page_address(page);
 		f2fs_bug_on(sbi, !nat_blk);
 	}
@@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
 		kmem_cache_free(nat_entry_set_slab, set);
 	}
+	return 0;
 }
 
 /*
  * This function is called during the checkpointing process.
  */
-void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
+int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 {
 	struct f2fs_nm_info *nm_i = NM_I(sbi);
 	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
@@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	unsigned int found;
 	nid_t set_idx = 0;
 	LIST_HEAD(sets);
+	int err = 0;
 
 	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
 	if (enabled_nat_bits(sbi, cpc)) {
@@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	}
 
 	if (!nm_i->dirty_nat_cnt)
-		return;
+		return 0;
 
 	down_write(&nm_i->nat_tree_lock);
 
@@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	}
 
 	/* flush dirty nats in nat entry set */
-	list_for_each_entry_safe(set, tmp, &sets, set_list)
-		__flush_nat_entry_set(sbi, set, cpc);
+	list_for_each_entry_safe(set, tmp, &sets, set_list) {
+		err = __flush_nat_entry_set(sbi, set, cpc);
+		if (err)
+			break;
+	}
 
 	up_write(&nm_i->nat_tree_lock);
 	/* Allow dirty nats by node block allocation in write_begin */
+
+	return err;
 }
 
 static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 56d34193a74b..ba678efe7cad 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
 	}
 
 	sum_page = f2fs_get_sum_page(sbi, segno);
+	if (IS_ERR(sum_page))
+		return PTR_ERR(sum_page);
 	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
 	sum = sum_node->entries[blkoff];
 	f2fs_put_page(sum_page, 1);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index aa96a371aaf8..ab7386a7e9d3 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
 	__next_free_blkoff(sbi, curseg, 0);
 
 	sum_page = f2fs_get_sum_page(sbi, new_segno);
+	f2fs_bug_on(sbi, IS_ERR(sum_page));
 	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
 	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
 	f2fs_put_page(sum_page, 1);
@@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
 
 			se = &sit_i->sentries[start];
 			page = get_current_sit_page(sbi, start);
+			if (IS_ERR(page))
+				return PTR_ERR(page);
 			sit_blk = (struct f2fs_sit_block *)page_address(page);
 			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
 			f2fs_put_page(page, 1);
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018/9/19 1:56, Jaegeuk Kim wrote:
> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> xfstests/generic/475.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> Change log from v1:
>  - propagate errno
>  - drop sum_pages
> 
>  fs/f2fs/checkpoint.c | 10 +++++-----
>  fs/f2fs/f2fs.h       |  2 +-
>  fs/f2fs/gc.c         |  9 +++++++++
>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
>  fs/f2fs/recovery.c   |  2 ++
>  fs/f2fs/segment.c    |  3 +++
>  6 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 01e0d8f5bbbe..1ddf332ce4b2 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
>  		if (PTR_ERR(page) == -EIO &&
>  				++count <= DEFAULT_RETRY_IO_COUNT)
>  			goto retry;
> -
>  		f2fs_stop_checkpoint(sbi, false);
> -		f2fs_bug_on(sbi, 1);
>  	}
> -
>  	return page;
>  }
>  
> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
>  
>  	/* write cached NAT/SIT entries to NAT/SIT area */
> -	f2fs_flush_nat_entries(sbi, cpc);
> +	err = f2fs_flush_nat_entries(sbi, cpc);
> +	if (err)
> +		goto stop;
> +
>  	f2fs_flush_sit_entries(sbi, cpc);
>  
>  	/* unlock all the fs_lock[] in do_checkpoint() */
> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  		f2fs_release_discard_addrs(sbi);
>  	else
>  		f2fs_clear_prefree_segments(sbi, cpc);
> -
> +stop:
>  	unblock_operations(sbi);
>  	stat_inc_cp_count(sbi->stat_info);
>  
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index a0c868127a7c..29021dbc8f2a 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
>  			unsigned int segno, struct f2fs_summary_block *sum);
> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
>  int __init f2fs_create_node_manager_caches(void);
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 4bcc8a59fdef..c7d14282dbf4 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>  	/* reference all summary page */
>  	while (segno < end_segno) {
>  		sum_page = f2fs_get_sum_page(sbi, segno++);
> +		if (IS_ERR(sum_page)) {
> +			end_segno = segno - 1;
> +			for (segno = start_segno; segno < end_segno; segno++) {
> +				sum_page = find_get_page(META_MAPPING(sbi),
> +						GET_SUM_BLOCK(sbi, segno));
> +				f2fs_put_page(sum_page, 0);
> +			}
> +			return PTR_ERR(sum_page);
> +		}
>  		unlock_page(sum_page);
>  	}
>  
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index fa2381c0bc47..79b6fee354f7 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
>  
>  	/* get current nat block page with lock */
>  	src_page = get_current_nat_page(sbi, nid);
> +	if (IS_ERR(src_page))
> +		return src_page;
>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
>  	f2fs_bug_on(sbi, PageDirty(src_page));
>  
> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>  						nm_i->nat_block_bitmap)) {
>  			struct page *page = get_current_nat_page(sbi, nid);
>  
> -			ret = scan_nat_page(sbi, page, nid);
> -			f2fs_put_page(page, 1);
> +			if (IS_ERR(page)) {
> +				ret = PTR_ERR(page);
> +			} else {
> +				ret = scan_nat_page(sbi, page, nid);
> +				f2fs_put_page(page, 1);
> +			}
>  
>  			if (ret) {
>  				up_read(&nm_i->nat_tree_lock);
>  				f2fs_bug_on(sbi, !mount);
>  				f2fs_msg(sbi->sb, KERN_ERR,
>  					"NAT is corrupt, run fsck to fix it");
> -				return -EINVAL;
> +				return ret;
>  			}

I mean we can propagate the error to caller of f2fs_alloc_nid(), otherwise
we may loop forever in f2fs_alloc_nid() if we fail to load meta page.

f2fs_alloc_nid()

	err = f2fs_build_free_nids();
	if (err)
		return false;
	goto retry;

Thanks,

>  		}
>  
> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
>  }
>  
> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>  		struct nat_entry_set *set, struct cp_control *cpc)
>  {
>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>  		down_write(&curseg->journal_rwsem);
>  	} else {
>  		page = get_next_nat_page(sbi, start_nid);
> +		if (IS_ERR(page))
> +			return PTR_ERR(page);
> +
>  		nat_blk = page_address(page);
>  		f2fs_bug_on(sbi, !nat_blk);
>  	}
> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
>  		kmem_cache_free(nat_entry_set_slab, set);
>  	}
> +	return 0;
>  }
>  
>  /*
>   * This function is called during the checkpointing process.
>   */
> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  {
>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	unsigned int found;
>  	nid_t set_idx = 0;
>  	LIST_HEAD(sets);
> +	int err = 0;
>  
>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
>  	if (enabled_nat_bits(sbi, cpc)) {
> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	}
>  
>  	if (!nm_i->dirty_nat_cnt)
> -		return;
> +		return 0;
>  
>  	down_write(&nm_i->nat_tree_lock);
>  
> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	}
>  
>  	/* flush dirty nats in nat entry set */
> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
> -		__flush_nat_entry_set(sbi, set, cpc);
> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
> +		err = __flush_nat_entry_set(sbi, set, cpc);
> +		if (err)
> +			break;
> +	}
>  
>  	up_write(&nm_i->nat_tree_lock);
>  	/* Allow dirty nats by node block allocation in write_begin */
> +
> +	return err;
>  }
>  
>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 56d34193a74b..ba678efe7cad 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
>  	}
>  
>  	sum_page = f2fs_get_sum_page(sbi, segno);
> +	if (IS_ERR(sum_page))
> +		return PTR_ERR(sum_page);
>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>  	sum = sum_node->entries[blkoff];
>  	f2fs_put_page(sum_page, 1);
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index aa96a371aaf8..ab7386a7e9d3 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>  	__next_free_blkoff(sbi, curseg, 0);
>  
>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
>  	f2fs_put_page(sum_page, 1);
> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
>  
>  			se = &sit_i->sentries[start];
>  			page = get_current_sit_page(sbi, start);
> +			if (IS_ERR(page))
> +				return PTR_ERR(page);
>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
>  			f2fs_put_page(page, 1);
> 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

On 09/19, Chao Yu wrote:
> On 2018/9/19 1:56, Jaegeuk Kim wrote:
> > This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> > xfstests/generic/475.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > Change log from v1:
> >  - propagate errno
> >  - drop sum_pages
> > 
> >  fs/f2fs/checkpoint.c | 10 +++++-----
> >  fs/f2fs/f2fs.h       |  2 +-
> >  fs/f2fs/gc.c         |  9 +++++++++
> >  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
> >  fs/f2fs/recovery.c   |  2 ++
> >  fs/f2fs/segment.c    |  3 +++
> >  6 files changed, 44 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 01e0d8f5bbbe..1ddf332ce4b2 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
> >  		if (PTR_ERR(page) == -EIO &&
> >  				++count <= DEFAULT_RETRY_IO_COUNT)
> >  			goto retry;
> > -
> >  		f2fs_stop_checkpoint(sbi, false);
> > -		f2fs_bug_on(sbi, 1);
> >  	}
> > -
> >  	return page;
> >  }
> >  
> > @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
> >  
> >  	/* write cached NAT/SIT entries to NAT/SIT area */
> > -	f2fs_flush_nat_entries(sbi, cpc);
> > +	err = f2fs_flush_nat_entries(sbi, cpc);
> > +	if (err)
> > +		goto stop;
> > +
> >  	f2fs_flush_sit_entries(sbi, cpc);
> >  
> >  	/* unlock all the fs_lock[] in do_checkpoint() */
> > @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  		f2fs_release_discard_addrs(sbi);
> >  	else
> >  		f2fs_clear_prefree_segments(sbi, cpc);
> > -
> > +stop:
> >  	unblock_operations(sbi);
> >  	stat_inc_cp_count(sbi->stat_info);
> >  
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index a0c868127a7c..29021dbc8f2a 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
> >  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
> >  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
> >  			unsigned int segno, struct f2fs_summary_block *sum);
> > -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> > +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
> >  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
> >  int __init f2fs_create_node_manager_caches(void);
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 4bcc8a59fdef..c7d14282dbf4 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >  	/* reference all summary page */
> >  	while (segno < end_segno) {
> >  		sum_page = f2fs_get_sum_page(sbi, segno++);
> > +		if (IS_ERR(sum_page)) {
> > +			end_segno = segno - 1;
> > +			for (segno = start_segno; segno < end_segno; segno++) {
> > +				sum_page = find_get_page(META_MAPPING(sbi),
> > +						GET_SUM_BLOCK(sbi, segno));
> > +				f2fs_put_page(sum_page, 0);
> > +			}
> > +			return PTR_ERR(sum_page);
> > +		}
> >  		unlock_page(sum_page);
> >  	}
> >  
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index fa2381c0bc47..79b6fee354f7 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
> >  
> >  	/* get current nat block page with lock */
> >  	src_page = get_current_nat_page(sbi, nid);
> > +	if (IS_ERR(src_page))
> > +		return src_page;
> >  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
> >  	f2fs_bug_on(sbi, PageDirty(src_page));
> >  
> > @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> >  						nm_i->nat_block_bitmap)) {
> >  			struct page *page = get_current_nat_page(sbi, nid);
> >  
> > -			ret = scan_nat_page(sbi, page, nid);
> > -			f2fs_put_page(page, 1);
> > +			if (IS_ERR(page)) {
> > +				ret = PTR_ERR(page);
> > +			} else {
> > +				ret = scan_nat_page(sbi, page, nid);
> > +				f2fs_put_page(page, 1);
> > +			}
> >  
> >  			if (ret) {
> >  				up_read(&nm_i->nat_tree_lock);
> >  				f2fs_bug_on(sbi, !mount);
> >  				f2fs_msg(sbi->sb, KERN_ERR,
> >  					"NAT is corrupt, run fsck to fix it");
> > -				return -EINVAL;
> > +				return ret;
> >  			}
> 
> I mean we can propagate the error to caller of f2fs_alloc_nid(), otherwise
> we may loop forever in f2fs_alloc_nid() if we fail to load meta page.
> 
> f2fs_alloc_nid()
> 
> 	err = f2fs_build_free_nids();
> 	if (err)
> 		return false;
> 	goto retry;

It looks like an another bug. Could you check another patch that I sent?

Thanks,

> 
> Thanks,
> 
> >  		}
> >  
> > @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
> >  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
> >  }
> >  
> > -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> > +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >  		struct nat_entry_set *set, struct cp_control *cpc)
> >  {
> >  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> > @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >  		down_write(&curseg->journal_rwsem);
> >  	} else {
> >  		page = get_next_nat_page(sbi, start_nid);
> > +		if (IS_ERR(page))
> > +			return PTR_ERR(page);
> > +
> >  		nat_blk = page_address(page);
> >  		f2fs_bug_on(sbi, !nat_blk);
> >  	}
> > @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
> >  		kmem_cache_free(nat_entry_set_slab, set);
> >  	}
> > +	return 0;
> >  }
> >  
> >  /*
> >   * This function is called during the checkpointing process.
> >   */
> > -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> > +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  {
> >  	struct f2fs_nm_info *nm_i = NM_I(sbi);
> >  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> > @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	unsigned int found;
> >  	nid_t set_idx = 0;
> >  	LIST_HEAD(sets);
> > +	int err = 0;
> >  
> >  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
> >  	if (enabled_nat_bits(sbi, cpc)) {
> > @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	}
> >  
> >  	if (!nm_i->dirty_nat_cnt)
> > -		return;
> > +		return 0;
> >  
> >  	down_write(&nm_i->nat_tree_lock);
> >  
> > @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	}
> >  
> >  	/* flush dirty nats in nat entry set */
> > -	list_for_each_entry_safe(set, tmp, &sets, set_list)
> > -		__flush_nat_entry_set(sbi, set, cpc);
> > +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
> > +		err = __flush_nat_entry_set(sbi, set, cpc);
> > +		if (err)
> > +			break;
> > +	}
> >  
> >  	up_write(&nm_i->nat_tree_lock);
> >  	/* Allow dirty nats by node block allocation in write_begin */
> > +
> > +	return err;
> >  }
> >  
> >  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
> > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> > index 56d34193a74b..ba678efe7cad 100644
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
> >  	}
> >  
> >  	sum_page = f2fs_get_sum_page(sbi, segno);
> > +	if (IS_ERR(sum_page))
> > +		return PTR_ERR(sum_page);
> >  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >  	sum = sum_node->entries[blkoff];
> >  	f2fs_put_page(sum_page, 1);
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index aa96a371aaf8..ab7386a7e9d3 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
> >  	__next_free_blkoff(sbi, curseg, 0);
> >  
> >  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> > +	f2fs_bug_on(sbi, IS_ERR(sum_page));
> >  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
> >  	f2fs_put_page(sum_page, 1);
> > @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
> >  
> >  			se = &sit_i->sentries[start];
> >  			page = get_current_sit_page(sbi, start);
> > +			if (IS_ERR(page))
> > +				return PTR_ERR(page);
> >  			sit_blk = (struct f2fs_sit_block *)page_address(page);
> >  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
> >  			f2fs_put_page(page, 1);
> > 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018/9/19 1:56, Jaegeuk Kim wrote:
> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> xfstests/generic/475.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> Change log from v1:
>  - propagate errno
>  - drop sum_pages
> 
>  fs/f2fs/checkpoint.c | 10 +++++-----
>  fs/f2fs/f2fs.h       |  2 +-
>  fs/f2fs/gc.c         |  9 +++++++++
>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
>  fs/f2fs/recovery.c   |  2 ++
>  fs/f2fs/segment.c    |  3 +++
>  6 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 01e0d8f5bbbe..1ddf332ce4b2 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
>  		if (PTR_ERR(page) == -EIO &&
>  				++count <= DEFAULT_RETRY_IO_COUNT)
>  			goto retry;
> -
>  		f2fs_stop_checkpoint(sbi, false);
> -		f2fs_bug_on(sbi, 1);
>  	}
> -
>  	return page;
>  }
>  
> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
>  
>  	/* write cached NAT/SIT entries to NAT/SIT area */
> -	f2fs_flush_nat_entries(sbi, cpc);
> +	err = f2fs_flush_nat_entries(sbi, cpc);
> +	if (err)
> +		goto stop;

To make sure, if partial NAT pages become dirty, flush them back to device
outside checkpoint() is not harmful, right?

> +
>  	f2fs_flush_sit_entries(sbi, cpc);
>  
>  	/* unlock all the fs_lock[] in do_checkpoint() */
> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  		f2fs_release_discard_addrs(sbi);
>  	else
>  		f2fs_clear_prefree_segments(sbi, cpc);
> -
> +stop:
>  	unblock_operations(sbi);
>  	stat_inc_cp_count(sbi->stat_info);
>  
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index a0c868127a7c..29021dbc8f2a 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
>  			unsigned int segno, struct f2fs_summary_block *sum);
> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
>  int __init f2fs_create_node_manager_caches(void);
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 4bcc8a59fdef..c7d14282dbf4 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>  	/* reference all summary page */
>  	while (segno < end_segno) {
>  		sum_page = f2fs_get_sum_page(sbi, segno++);
> +		if (IS_ERR(sum_page)) {
> +			end_segno = segno - 1;
> +			for (segno = start_segno; segno < end_segno; segno++) {
> +				sum_page = find_get_page(META_MAPPING(sbi),
> +						GET_SUM_BLOCK(sbi, segno));
> +				f2fs_put_page(sum_page, 0);

find_get_page() will add one more reference on page, so we need to call
f2fs_put_page(sum_page, 0) twice.

Thanks,

> +			}
> +			return PTR_ERR(sum_page);
> +		}
>  		unlock_page(sum_page);
>  	}
>  
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index fa2381c0bc47..79b6fee354f7 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
>  
>  	/* get current nat block page with lock */
>  	src_page = get_current_nat_page(sbi, nid);
> +	if (IS_ERR(src_page))
> +		return src_page;
>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
>  	f2fs_bug_on(sbi, PageDirty(src_page));
>  
> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>  						nm_i->nat_block_bitmap)) {
>  			struct page *page = get_current_nat_page(sbi, nid);
>  
> -			ret = scan_nat_page(sbi, page, nid);
> -			f2fs_put_page(page, 1);
> +			if (IS_ERR(page)) {
> +				ret = PTR_ERR(page);
> +			} else {
> +				ret = scan_nat_page(sbi, page, nid);
> +				f2fs_put_page(page, 1);
> +			}
>  
>  			if (ret) {
>  				up_read(&nm_i->nat_tree_lock);
>  				f2fs_bug_on(sbi, !mount);
>  				f2fs_msg(sbi->sb, KERN_ERR,
>  					"NAT is corrupt, run fsck to fix it");
> -				return -EINVAL;
> +				return ret;
>  			}
>  		}
>  
> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
>  }
>  
> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>  		struct nat_entry_set *set, struct cp_control *cpc)
>  {
>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>  		down_write(&curseg->journal_rwsem);
>  	} else {
>  		page = get_next_nat_page(sbi, start_nid);
> +		if (IS_ERR(page))
> +			return PTR_ERR(page);
> +
>  		nat_blk = page_address(page);
>  		f2fs_bug_on(sbi, !nat_blk);
>  	}
> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
>  		kmem_cache_free(nat_entry_set_slab, set);
>  	}
> +	return 0;
>  }
>  
>  /*
>   * This function is called during the checkpointing process.
>   */
> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  {
>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	unsigned int found;
>  	nid_t set_idx = 0;
>  	LIST_HEAD(sets);
> +	int err = 0;
>  
>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
>  	if (enabled_nat_bits(sbi, cpc)) {
> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	}
>  
>  	if (!nm_i->dirty_nat_cnt)
> -		return;
> +		return 0;
>  
>  	down_write(&nm_i->nat_tree_lock);
>  
> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	}
>  
>  	/* flush dirty nats in nat entry set */
> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
> -		__flush_nat_entry_set(sbi, set, cpc);
> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
> +		err = __flush_nat_entry_set(sbi, set, cpc);
> +		if (err)
> +			break;
> +	}
>  
>  	up_write(&nm_i->nat_tree_lock);
>  	/* Allow dirty nats by node block allocation in write_begin */
> +
> +	return err;
>  }
>  
>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 56d34193a74b..ba678efe7cad 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
>  	}
>  
>  	sum_page = f2fs_get_sum_page(sbi, segno);
> +	if (IS_ERR(sum_page))
> +		return PTR_ERR(sum_page);
>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>  	sum = sum_node->entries[blkoff];
>  	f2fs_put_page(sum_page, 1);
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index aa96a371aaf8..ab7386a7e9d3 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>  	__next_free_blkoff(sbi, curseg, 0);
>  
>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
>  	f2fs_put_page(sum_page, 1);
> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
>  
>  			se = &sit_i->sentries[start];
>  			page = get_current_sit_page(sbi, start);
> +			if (IS_ERR(page))
> +				return PTR_ERR(page);
>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
>  			f2fs_put_page(page, 1);
> 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

On 09/20, Chao Yu wrote:
> On 2018/9/19 1:56, Jaegeuk Kim wrote:
> > This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> > xfstests/generic/475.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > Change log from v1:
> >  - propagate errno
> >  - drop sum_pages
> > 
> >  fs/f2fs/checkpoint.c | 10 +++++-----
> >  fs/f2fs/f2fs.h       |  2 +-
> >  fs/f2fs/gc.c         |  9 +++++++++
> >  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
> >  fs/f2fs/recovery.c   |  2 ++
> >  fs/f2fs/segment.c    |  3 +++
> >  6 files changed, 44 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 01e0d8f5bbbe..1ddf332ce4b2 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
> >  		if (PTR_ERR(page) == -EIO &&
> >  				++count <= DEFAULT_RETRY_IO_COUNT)
> >  			goto retry;
> > -
> >  		f2fs_stop_checkpoint(sbi, false);
> > -		f2fs_bug_on(sbi, 1);
> >  	}
> > -
> >  	return page;
> >  }
> >  
> > @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
> >  
> >  	/* write cached NAT/SIT entries to NAT/SIT area */
> > -	f2fs_flush_nat_entries(sbi, cpc);
> > +	err = f2fs_flush_nat_entries(sbi, cpc);
> > +	if (err)
> > +		goto stop;
> 
> To make sure, if partial NAT pages become dirty, flush them back to device
> outside checkpoint() is not harmful, right?

I think so.

> 
> > +
> >  	f2fs_flush_sit_entries(sbi, cpc);
> >  
> >  	/* unlock all the fs_lock[] in do_checkpoint() */
> > @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  		f2fs_release_discard_addrs(sbi);
> >  	else
> >  		f2fs_clear_prefree_segments(sbi, cpc);
> > -
> > +stop:
> >  	unblock_operations(sbi);
> >  	stat_inc_cp_count(sbi->stat_info);
> >  
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index a0c868127a7c..29021dbc8f2a 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
> >  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
> >  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
> >  			unsigned int segno, struct f2fs_summary_block *sum);
> > -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> > +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
> >  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
> >  int __init f2fs_create_node_manager_caches(void);
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 4bcc8a59fdef..c7d14282dbf4 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >  	/* reference all summary page */
> >  	while (segno < end_segno) {
> >  		sum_page = f2fs_get_sum_page(sbi, segno++);
> > +		if (IS_ERR(sum_page)) {
> > +			end_segno = segno - 1;
> > +			for (segno = start_segno; segno < end_segno; segno++) {
> > +				sum_page = find_get_page(META_MAPPING(sbi),
> > +						GET_SUM_BLOCK(sbi, segno));
> > +				f2fs_put_page(sum_page, 0);
> 
> find_get_page() will add one more reference on page, so we need to call
> f2fs_put_page(sum_page, 0) twice.

Done.

> 
> Thanks,
> 
> > +			}
> > +			return PTR_ERR(sum_page);
> > +		}
> >  		unlock_page(sum_page);
> >  	}
> >  
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index fa2381c0bc47..79b6fee354f7 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
> >  
> >  	/* get current nat block page with lock */
> >  	src_page = get_current_nat_page(sbi, nid);
> > +	if (IS_ERR(src_page))
> > +		return src_page;
> >  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
> >  	f2fs_bug_on(sbi, PageDirty(src_page));
> >  
> > @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> >  						nm_i->nat_block_bitmap)) {
> >  			struct page *page = get_current_nat_page(sbi, nid);
> >  
> > -			ret = scan_nat_page(sbi, page, nid);
> > -			f2fs_put_page(page, 1);
> > +			if (IS_ERR(page)) {
> > +				ret = PTR_ERR(page);
> > +			} else {
> > +				ret = scan_nat_page(sbi, page, nid);
> > +				f2fs_put_page(page, 1);
> > +			}
> >  
> >  			if (ret) {
> >  				up_read(&nm_i->nat_tree_lock);
> >  				f2fs_bug_on(sbi, !mount);
> >  				f2fs_msg(sbi->sb, KERN_ERR,
> >  					"NAT is corrupt, run fsck to fix it");
> > -				return -EINVAL;
> > +				return ret;
> >  			}
> >  		}
> >  
> > @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
> >  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
> >  }
> >  
> > -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> > +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >  		struct nat_entry_set *set, struct cp_control *cpc)
> >  {
> >  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> > @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >  		down_write(&curseg->journal_rwsem);
> >  	} else {
> >  		page = get_next_nat_page(sbi, start_nid);
> > +		if (IS_ERR(page))
> > +			return PTR_ERR(page);
> > +
> >  		nat_blk = page_address(page);
> >  		f2fs_bug_on(sbi, !nat_blk);
> >  	}
> > @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
> >  		kmem_cache_free(nat_entry_set_slab, set);
> >  	}
> > +	return 0;
> >  }
> >  
> >  /*
> >   * This function is called during the checkpointing process.
> >   */
> > -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> > +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  {
> >  	struct f2fs_nm_info *nm_i = NM_I(sbi);
> >  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> > @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	unsigned int found;
> >  	nid_t set_idx = 0;
> >  	LIST_HEAD(sets);
> > +	int err = 0;
> >  
> >  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
> >  	if (enabled_nat_bits(sbi, cpc)) {
> > @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	}
> >  
> >  	if (!nm_i->dirty_nat_cnt)
> > -		return;
> > +		return 0;
> >  
> >  	down_write(&nm_i->nat_tree_lock);
> >  
> > @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	}
> >  
> >  	/* flush dirty nats in nat entry set */
> > -	list_for_each_entry_safe(set, tmp, &sets, set_list)
> > -		__flush_nat_entry_set(sbi, set, cpc);
> > +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
> > +		err = __flush_nat_entry_set(sbi, set, cpc);
> > +		if (err)
> > +			break;
> > +	}
> >  
> >  	up_write(&nm_i->nat_tree_lock);
> >  	/* Allow dirty nats by node block allocation in write_begin */
> > +
> > +	return err;
> >  }
> >  
> >  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
> > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> > index 56d34193a74b..ba678efe7cad 100644
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
> >  	}
> >  
> >  	sum_page = f2fs_get_sum_page(sbi, segno);
> > +	if (IS_ERR(sum_page))
> > +		return PTR_ERR(sum_page);
> >  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >  	sum = sum_node->entries[blkoff];
> >  	f2fs_put_page(sum_page, 1);
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index aa96a371aaf8..ab7386a7e9d3 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
> >  	__next_free_blkoff(sbi, curseg, 0);
> >  
> >  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> > +	f2fs_bug_on(sbi, IS_ERR(sum_page));
> >  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
> >  	f2fs_put_page(sum_page, 1);
> > @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
> >  
> >  			se = &sit_i->sentries[start];
> >  			page = get_current_sit_page(sbi, start);
> > +			if (IS_ERR(page))
> > +				return PTR_ERR(page);
> >  			sit_blk = (struct f2fs_sit_block *)page_address(page);
> >  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
> >  			f2fs_put_page(page, 1);
> > 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018/9/21 5:46, Jaegeuk Kim wrote:
> On 09/20, Chao Yu wrote:
>> On 2018/9/19 1:56, Jaegeuk Kim wrote:
>>> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
>>> xfstests/generic/475.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>> Change log from v1:
>>>  - propagate errno
>>>  - drop sum_pages
>>>
>>>  fs/f2fs/checkpoint.c | 10 +++++-----
>>>  fs/f2fs/f2fs.h       |  2 +-
>>>  fs/f2fs/gc.c         |  9 +++++++++
>>>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
>>>  fs/f2fs/recovery.c   |  2 ++
>>>  fs/f2fs/segment.c    |  3 +++
>>>  6 files changed, 44 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 01e0d8f5bbbe..1ddf332ce4b2 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
>>>  		if (PTR_ERR(page) == -EIO &&
>>>  				++count <= DEFAULT_RETRY_IO_COUNT)
>>>  			goto retry;
>>> -
>>>  		f2fs_stop_checkpoint(sbi, false);
>>> -		f2fs_bug_on(sbi, 1);
>>>  	}
>>> -
>>>  	return page;
>>>  }
>>>  
>>> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
>>>  
>>>  	/* write cached NAT/SIT entries to NAT/SIT area */
>>> -	f2fs_flush_nat_entries(sbi, cpc);
>>> +	err = f2fs_flush_nat_entries(sbi, cpc);
>>> +	if (err)
>>> +		goto stop;
>>
>> To make sure, if partial NAT pages become dirty, flush them back to device
>> outside checkpoint() is not harmful, right?
> 
> I think so.

Oh, one more case, now we use NAT #0, if partial NAT pages were
writebacked, then we start to use NAT #1, later, in another checkpoint(),
we update those NAT pages again, we will write them to NAT #0, which is
belong to last valid checkpoint(), it's may cause corrupted checkpoint in
scenario of SPO. So it's harmfull, right?

Thanks,

> 
>>
>>> +
>>>  	f2fs_flush_sit_entries(sbi, cpc);
>>>  
>>>  	/* unlock all the fs_lock[] in do_checkpoint() */
>>> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  		f2fs_release_discard_addrs(sbi);
>>>  	else
>>>  		f2fs_clear_prefree_segments(sbi, cpc);
>>> -
>>> +stop:
>>>  	unblock_operations(sbi);
>>>  	stat_inc_cp_count(sbi->stat_info);
>>>  
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index a0c868127a7c..29021dbc8f2a 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
>>>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
>>>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
>>>  			unsigned int segno, struct f2fs_summary_block *sum);
>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
>>>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
>>>  int __init f2fs_create_node_manager_caches(void);
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index 4bcc8a59fdef..c7d14282dbf4 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>>>  	/* reference all summary page */
>>>  	while (segno < end_segno) {
>>>  		sum_page = f2fs_get_sum_page(sbi, segno++);
>>> +		if (IS_ERR(sum_page)) {
>>> +			end_segno = segno - 1;
>>> +			for (segno = start_segno; segno < end_segno; segno++) {
>>> +				sum_page = find_get_page(META_MAPPING(sbi),
>>> +						GET_SUM_BLOCK(sbi, segno));
>>> +				f2fs_put_page(sum_page, 0);
>>
>> find_get_page() will add one more reference on page, so we need to call
>> f2fs_put_page(sum_page, 0) twice.
> 
> Done.
> 
>>
>> Thanks,
>>
>>> +			}
>>> +			return PTR_ERR(sum_page);
>>> +		}
>>>  		unlock_page(sum_page);
>>>  	}
>>>  
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index fa2381c0bc47..79b6fee354f7 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
>>>  
>>>  	/* get current nat block page with lock */
>>>  	src_page = get_current_nat_page(sbi, nid);
>>> +	if (IS_ERR(src_page))
>>> +		return src_page;
>>>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
>>>  	f2fs_bug_on(sbi, PageDirty(src_page));
>>>  
>>> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>>>  						nm_i->nat_block_bitmap)) {
>>>  			struct page *page = get_current_nat_page(sbi, nid);
>>>  
>>> -			ret = scan_nat_page(sbi, page, nid);
>>> -			f2fs_put_page(page, 1);
>>> +			if (IS_ERR(page)) {
>>> +				ret = PTR_ERR(page);
>>> +			} else {
>>> +				ret = scan_nat_page(sbi, page, nid);
>>> +				f2fs_put_page(page, 1);
>>> +			}
>>>  
>>>  			if (ret) {
>>>  				up_read(&nm_i->nat_tree_lock);
>>>  				f2fs_bug_on(sbi, !mount);
>>>  				f2fs_msg(sbi->sb, KERN_ERR,
>>>  					"NAT is corrupt, run fsck to fix it");
>>> -				return -EINVAL;
>>> +				return ret;
>>>  			}
>>>  		}
>>>  
>>> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
>>>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
>>>  }
>>>  
>>> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>  		struct nat_entry_set *set, struct cp_control *cpc)
>>>  {
>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>  		down_write(&curseg->journal_rwsem);
>>>  	} else {
>>>  		page = get_next_nat_page(sbi, start_nid);
>>> +		if (IS_ERR(page))
>>> +			return PTR_ERR(page);
>>> +
>>>  		nat_blk = page_address(page);
>>>  		f2fs_bug_on(sbi, !nat_blk);
>>>  	}
>>> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
>>>  		kmem_cache_free(nat_entry_set_slab, set);
>>>  	}
>>> +	return 0;
>>>  }
>>>  
>>>  /*
>>>   * This function is called during the checkpointing process.
>>>   */
>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  {
>>>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	unsigned int found;
>>>  	nid_t set_idx = 0;
>>>  	LIST_HEAD(sets);
>>> +	int err = 0;
>>>  
>>>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
>>>  	if (enabled_nat_bits(sbi, cpc)) {
>>> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	}
>>>  
>>>  	if (!nm_i->dirty_nat_cnt)
>>> -		return;
>>> +		return 0;
>>>  
>>>  	down_write(&nm_i->nat_tree_lock);
>>>  
>>> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	}
>>>  
>>>  	/* flush dirty nats in nat entry set */
>>> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
>>> -		__flush_nat_entry_set(sbi, set, cpc);
>>> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
>>> +		err = __flush_nat_entry_set(sbi, set, cpc);
>>> +		if (err)
>>> +			break;
>>> +	}
>>>  
>>>  	up_write(&nm_i->nat_tree_lock);
>>>  	/* Allow dirty nats by node block allocation in write_begin */
>>> +
>>> +	return err;
>>>  }
>>>  
>>>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>> index 56d34193a74b..ba678efe7cad 100644
>>> --- a/fs/f2fs/recovery.c
>>> +++ b/fs/f2fs/recovery.c
>>> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
>>>  	}
>>>  
>>>  	sum_page = f2fs_get_sum_page(sbi, segno);
>>> +	if (IS_ERR(sum_page))
>>> +		return PTR_ERR(sum_page);
>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>  	sum = sum_node->entries[blkoff];
>>>  	f2fs_put_page(sum_page, 1);
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index aa96a371aaf8..ab7386a7e9d3 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>>>  	__next_free_blkoff(sbi, curseg, 0);
>>>  
>>>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
>>> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
>>>  	f2fs_put_page(sum_page, 1);
>>> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
>>>  
>>>  			se = &sit_i->sentries[start];
>>>  			page = get_current_sit_page(sbi, start);
>>> +			if (IS_ERR(page))
>>> +				return PTR_ERR(page);
>>>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
>>>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
>>>  			f2fs_put_page(page, 1);
>>>
> 
> .
> 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

On 09/21, Chao Yu wrote:
> On 2018/9/21 5:46, Jaegeuk Kim wrote:
> > On 09/20, Chao Yu wrote:
> >> On 2018/9/19 1:56, Jaegeuk Kim wrote:
> >>> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> >>> xfstests/generic/475.
> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>> ---
> >>> Change log from v1:
> >>>  - propagate errno
> >>>  - drop sum_pages
> >>>
> >>>  fs/f2fs/checkpoint.c | 10 +++++-----
> >>>  fs/f2fs/f2fs.h       |  2 +-
> >>>  fs/f2fs/gc.c         |  9 +++++++++
> >>>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
> >>>  fs/f2fs/recovery.c   |  2 ++
> >>>  fs/f2fs/segment.c    |  3 +++
> >>>  6 files changed, 44 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index 01e0d8f5bbbe..1ddf332ce4b2 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
> >>>  		if (PTR_ERR(page) == -EIO &&
> >>>  				++count <= DEFAULT_RETRY_IO_COUNT)
> >>>  			goto retry;
> >>> -
> >>>  		f2fs_stop_checkpoint(sbi, false);
> >>> -		f2fs_bug_on(sbi, 1);
> >>>  	}
> >>> -
> >>>  	return page;
> >>>  }
> >>>  
> >>> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
> >>>  
> >>>  	/* write cached NAT/SIT entries to NAT/SIT area */
> >>> -	f2fs_flush_nat_entries(sbi, cpc);
> >>> +	err = f2fs_flush_nat_entries(sbi, cpc);
> >>> +	if (err)
> >>> +		goto stop;
> >>
> >> To make sure, if partial NAT pages become dirty, flush them back to device
> >> outside checkpoint() is not harmful, right?
> > 
> > I think so.
> 
> Oh, one more case, now we use NAT #0, if partial NAT pages were
> writebacked, then we start to use NAT #1, later, in another checkpoint(),
> we update those NAT pages again, we will write them to NAT #0, which is
> belong to last valid checkpoint(), it's may cause corrupted checkpoint in
> scenario of SPO. So it's harmfull, right?

We already stopped checkpoint anymore, so there'd be no way to proceed another
checkpoint. Am I missing something?

> 
> Thanks,
> 
> > 
> >>
> >>> +
> >>>  	f2fs_flush_sit_entries(sbi, cpc);
> >>>  
> >>>  	/* unlock all the fs_lock[] in do_checkpoint() */
> >>> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  		f2fs_release_discard_addrs(sbi);
> >>>  	else
> >>>  		f2fs_clear_prefree_segments(sbi, cpc);
> >>> -
> >>> +stop:
> >>>  	unblock_operations(sbi);
> >>>  	stat_inc_cp_count(sbi->stat_info);
> >>>  
> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>> index a0c868127a7c..29021dbc8f2a 100644
> >>> --- a/fs/f2fs/f2fs.h
> >>> +++ b/fs/f2fs/f2fs.h
> >>> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
> >>>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
> >>>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
> >>>  			unsigned int segno, struct f2fs_summary_block *sum);
> >>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
> >>>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
> >>>  int __init f2fs_create_node_manager_caches(void);
> >>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> >>> index 4bcc8a59fdef..c7d14282dbf4 100644
> >>> --- a/fs/f2fs/gc.c
> >>> +++ b/fs/f2fs/gc.c
> >>> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >>>  	/* reference all summary page */
> >>>  	while (segno < end_segno) {
> >>>  		sum_page = f2fs_get_sum_page(sbi, segno++);
> >>> +		if (IS_ERR(sum_page)) {
> >>> +			end_segno = segno - 1;
> >>> +			for (segno = start_segno; segno < end_segno; segno++) {
> >>> +				sum_page = find_get_page(META_MAPPING(sbi),
> >>> +						GET_SUM_BLOCK(sbi, segno));
> >>> +				f2fs_put_page(sum_page, 0);
> >>
> >> find_get_page() will add one more reference on page, so we need to call
> >> f2fs_put_page(sum_page, 0) twice.
> > 
> > Done.
> > 
> >>
> >> Thanks,
> >>
> >>> +			}
> >>> +			return PTR_ERR(sum_page);
> >>> +		}
> >>>  		unlock_page(sum_page);
> >>>  	}
> >>>  
> >>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >>> index fa2381c0bc47..79b6fee354f7 100644
> >>> --- a/fs/f2fs/node.c
> >>> +++ b/fs/f2fs/node.c
> >>> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
> >>>  
> >>>  	/* get current nat block page with lock */
> >>>  	src_page = get_current_nat_page(sbi, nid);
> >>> +	if (IS_ERR(src_page))
> >>> +		return src_page;
> >>>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
> >>>  	f2fs_bug_on(sbi, PageDirty(src_page));
> >>>  
> >>> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> >>>  						nm_i->nat_block_bitmap)) {
> >>>  			struct page *page = get_current_nat_page(sbi, nid);
> >>>  
> >>> -			ret = scan_nat_page(sbi, page, nid);
> >>> -			f2fs_put_page(page, 1);
> >>> +			if (IS_ERR(page)) {
> >>> +				ret = PTR_ERR(page);
> >>> +			} else {
> >>> +				ret = scan_nat_page(sbi, page, nid);
> >>> +				f2fs_put_page(page, 1);
> >>> +			}
> >>>  
> >>>  			if (ret) {
> >>>  				up_read(&nm_i->nat_tree_lock);
> >>>  				f2fs_bug_on(sbi, !mount);
> >>>  				f2fs_msg(sbi->sb, KERN_ERR,
> >>>  					"NAT is corrupt, run fsck to fix it");
> >>> -				return -EINVAL;
> >>> +				return ret;
> >>>  			}
> >>>  		}
> >>>  
> >>> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
> >>>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
> >>>  }
> >>>  
> >>> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>  		struct nat_entry_set *set, struct cp_control *cpc)
> >>>  {
> >>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> >>> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>  		down_write(&curseg->journal_rwsem);
> >>>  	} else {
> >>>  		page = get_next_nat_page(sbi, start_nid);
> >>> +		if (IS_ERR(page))
> >>> +			return PTR_ERR(page);
> >>> +
> >>>  		nat_blk = page_address(page);
> >>>  		f2fs_bug_on(sbi, !nat_blk);
> >>>  	}
> >>> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
> >>>  		kmem_cache_free(nat_entry_set_slab, set);
> >>>  	}
> >>> +	return 0;
> >>>  }
> >>>  
> >>>  /*
> >>>   * This function is called during the checkpointing process.
> >>>   */
> >>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  {
> >>>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
> >>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> >>> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  	unsigned int found;
> >>>  	nid_t set_idx = 0;
> >>>  	LIST_HEAD(sets);
> >>> +	int err = 0;
> >>>  
> >>>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
> >>>  	if (enabled_nat_bits(sbi, cpc)) {
> >>> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  	}
> >>>  
> >>>  	if (!nm_i->dirty_nat_cnt)
> >>> -		return;
> >>> +		return 0;
> >>>  
> >>>  	down_write(&nm_i->nat_tree_lock);
> >>>  
> >>> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  	}
> >>>  
> >>>  	/* flush dirty nats in nat entry set */
> >>> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
> >>> -		__flush_nat_entry_set(sbi, set, cpc);
> >>> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
> >>> +		err = __flush_nat_entry_set(sbi, set, cpc);
> >>> +		if (err)
> >>> +			break;
> >>> +	}
> >>>  
> >>>  	up_write(&nm_i->nat_tree_lock);
> >>>  	/* Allow dirty nats by node block allocation in write_begin */
> >>> +
> >>> +	return err;
> >>>  }
> >>>  
> >>>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
> >>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> >>> index 56d34193a74b..ba678efe7cad 100644
> >>> --- a/fs/f2fs/recovery.c
> >>> +++ b/fs/f2fs/recovery.c
> >>> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
> >>>  	}
> >>>  
> >>>  	sum_page = f2fs_get_sum_page(sbi, segno);
> >>> +	if (IS_ERR(sum_page))
> >>> +		return PTR_ERR(sum_page);
> >>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >>>  	sum = sum_node->entries[blkoff];
> >>>  	f2fs_put_page(sum_page, 1);
> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>> index aa96a371aaf8..ab7386a7e9d3 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
> >>>  	__next_free_blkoff(sbi, curseg, 0);
> >>>  
> >>>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> >>> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
> >>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >>>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
> >>>  	f2fs_put_page(sum_page, 1);
> >>> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
> >>>  
> >>>  			se = &sit_i->sentries[start];
> >>>  			page = get_current_sit_page(sbi, start);
> >>> +			if (IS_ERR(page))
> >>> +				return PTR_ERR(page);
> >>>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
> >>>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
> >>>  			f2fs_put_page(page, 1);
> >>>
> > 
> > .
> > 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018/9/26 8:18, Jaegeuk Kim wrote:
> On 09/21, Chao Yu wrote:
>> On 2018/9/21 5:46, Jaegeuk Kim wrote:
>>> On 09/20, Chao Yu wrote:
>>>> On 2018/9/19 1:56, Jaegeuk Kim wrote:
>>>>> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
>>>>> xfstests/generic/475.
>>>>>
>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>> ---
>>>>> Change log from v1:
>>>>>  - propagate errno
>>>>>  - drop sum_pages
>>>>>
>>>>>  fs/f2fs/checkpoint.c | 10 +++++-----
>>>>>  fs/f2fs/f2fs.h       |  2 +-
>>>>>  fs/f2fs/gc.c         |  9 +++++++++
>>>>>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
>>>>>  fs/f2fs/recovery.c   |  2 ++
>>>>>  fs/f2fs/segment.c    |  3 +++
>>>>>  6 files changed, 44 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>> index 01e0d8f5bbbe..1ddf332ce4b2 100644
>>>>> --- a/fs/f2fs/checkpoint.c
>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
>>>>>  		if (PTR_ERR(page) == -EIO &&
>>>>>  				++count <= DEFAULT_RETRY_IO_COUNT)
>>>>>  			goto retry;
>>>>> -
>>>>>  		f2fs_stop_checkpoint(sbi, false);
>>>>> -		f2fs_bug_on(sbi, 1);
>>>>>  	}
>>>>> -
>>>>>  	return page;
>>>>>  }
>>>>>  
>>>>> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
>>>>>  
>>>>>  	/* write cached NAT/SIT entries to NAT/SIT area */
>>>>> -	f2fs_flush_nat_entries(sbi, cpc);
>>>>> +	err = f2fs_flush_nat_entries(sbi, cpc);
>>>>> +	if (err)
>>>>> +		goto stop;
>>>>
>>>> To make sure, if partial NAT pages become dirty, flush them back to device
>>>> outside checkpoint() is not harmful, right?
>>>
>>> I think so.
>>
>> Oh, one more case, now we use NAT #0, if partial NAT pages were
>> writebacked, then we start to use NAT #1, later, in another checkpoint(),
>> we update those NAT pages again, we will write them to NAT #0, which is
>> belong to last valid checkpoint(), it's may cause corrupted checkpoint in
>> scenario of SPO. So it's harmfull, right?
> 
> We already stopped checkpoint anymore, so there'd be no way to proceed another
> checkpoint. Am I missing something?

You're right, we have called f2fs_stop_checkpoint(), so we are safe now. :)

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>>> +
>>>>>  	f2fs_flush_sit_entries(sbi, cpc);
>>>>>  
>>>>>  	/* unlock all the fs_lock[] in do_checkpoint() */
>>>>> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>  		f2fs_release_discard_addrs(sbi);
>>>>>  	else
>>>>>  		f2fs_clear_prefree_segments(sbi, cpc);
>>>>> -
>>>>> +stop:
>>>>>  	unblock_operations(sbi);
>>>>>  	stat_inc_cp_count(sbi->stat_info);
>>>>>  
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index a0c868127a7c..29021dbc8f2a 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
>>>>>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
>>>>>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
>>>>>  			unsigned int segno, struct f2fs_summary_block *sum);
>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
>>>>>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
>>>>>  int __init f2fs_create_node_manager_caches(void);
>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>> index 4bcc8a59fdef..c7d14282dbf4 100644
>>>>> --- a/fs/f2fs/gc.c
>>>>> +++ b/fs/f2fs/gc.c
>>>>> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>>>>>  	/* reference all summary page */
>>>>>  	while (segno < end_segno) {
>>>>>  		sum_page = f2fs_get_sum_page(sbi, segno++);
>>>>> +		if (IS_ERR(sum_page)) {
>>>>> +			end_segno = segno - 1;
>>>>> +			for (segno = start_segno; segno < end_segno; segno++) {
>>>>> +				sum_page = find_get_page(META_MAPPING(sbi),
>>>>> +						GET_SUM_BLOCK(sbi, segno));
>>>>> +				f2fs_put_page(sum_page, 0);
>>>>
>>>> find_get_page() will add one more reference on page, so we need to call
>>>> f2fs_put_page(sum_page, 0) twice.
>>>
>>> Done.
>>>
>>>>
>>>> Thanks,
>>>>
>>>>> +			}
>>>>> +			return PTR_ERR(sum_page);
>>>>> +		}
>>>>>  		unlock_page(sum_page);
>>>>>  	}
>>>>>  
>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>>> index fa2381c0bc47..79b6fee354f7 100644
>>>>> --- a/fs/f2fs/node.c
>>>>> +++ b/fs/f2fs/node.c
>>>>> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
>>>>>  
>>>>>  	/* get current nat block page with lock */
>>>>>  	src_page = get_current_nat_page(sbi, nid);
>>>>> +	if (IS_ERR(src_page))
>>>>> +		return src_page;
>>>>>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
>>>>>  	f2fs_bug_on(sbi, PageDirty(src_page));
>>>>>  
>>>>> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>>>>>  						nm_i->nat_block_bitmap)) {
>>>>>  			struct page *page = get_current_nat_page(sbi, nid);
>>>>>  
>>>>> -			ret = scan_nat_page(sbi, page, nid);
>>>>> -			f2fs_put_page(page, 1);
>>>>> +			if (IS_ERR(page)) {
>>>>> +				ret = PTR_ERR(page);
>>>>> +			} else {
>>>>> +				ret = scan_nat_page(sbi, page, nid);
>>>>> +				f2fs_put_page(page, 1);
>>>>> +			}
>>>>>  
>>>>>  			if (ret) {
>>>>>  				up_read(&nm_i->nat_tree_lock);
>>>>>  				f2fs_bug_on(sbi, !mount);
>>>>>  				f2fs_msg(sbi->sb, KERN_ERR,
>>>>>  					"NAT is corrupt, run fsck to fix it");
>>>>> -				return -EINVAL;
>>>>> +				return ret;
>>>>>  			}
>>>>>  		}
>>>>>  
>>>>> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
>>>>>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
>>>>>  }
>>>>>  
>>>>> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>  		struct nat_entry_set *set, struct cp_control *cpc)
>>>>>  {
>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>>>> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>  		down_write(&curseg->journal_rwsem);
>>>>>  	} else {
>>>>>  		page = get_next_nat_page(sbi, start_nid);
>>>>> +		if (IS_ERR(page))
>>>>> +			return PTR_ERR(page);
>>>>> +
>>>>>  		nat_blk = page_address(page);
>>>>>  		f2fs_bug_on(sbi, !nat_blk);
>>>>>  	}
>>>>> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
>>>>>  		kmem_cache_free(nat_entry_set_slab, set);
>>>>>  	}
>>>>> +	return 0;
>>>>>  }
>>>>>  
>>>>>  /*
>>>>>   * This function is called during the checkpointing process.
>>>>>   */
>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>  {
>>>>>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>>>> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>  	unsigned int found;
>>>>>  	nid_t set_idx = 0;
>>>>>  	LIST_HEAD(sets);
>>>>> +	int err = 0;
>>>>>  
>>>>>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
>>>>>  	if (enabled_nat_bits(sbi, cpc)) {
>>>>> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>  	}
>>>>>  
>>>>>  	if (!nm_i->dirty_nat_cnt)
>>>>> -		return;
>>>>> +		return 0;
>>>>>  
>>>>>  	down_write(&nm_i->nat_tree_lock);
>>>>>  
>>>>> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>  	}
>>>>>  
>>>>>  	/* flush dirty nats in nat entry set */
>>>>> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
>>>>> -		__flush_nat_entry_set(sbi, set, cpc);
>>>>> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
>>>>> +		err = __flush_nat_entry_set(sbi, set, cpc);
>>>>> +		if (err)
>>>>> +			break;
>>>>> +	}
>>>>>  
>>>>>  	up_write(&nm_i->nat_tree_lock);
>>>>>  	/* Allow dirty nats by node block allocation in write_begin */
>>>>> +
>>>>> +	return err;
>>>>>  }
>>>>>  
>>>>>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
>>>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>>>> index 56d34193a74b..ba678efe7cad 100644
>>>>> --- a/fs/f2fs/recovery.c
>>>>> +++ b/fs/f2fs/recovery.c
>>>>> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
>>>>>  	}
>>>>>  
>>>>>  	sum_page = f2fs_get_sum_page(sbi, segno);
>>>>> +	if (IS_ERR(sum_page))
>>>>> +		return PTR_ERR(sum_page);
>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>>>  	sum = sum_node->entries[blkoff];
>>>>>  	f2fs_put_page(sum_page, 1);
>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>> index aa96a371aaf8..ab7386a7e9d3 100644
>>>>> --- a/fs/f2fs/segment.c
>>>>> +++ b/fs/f2fs/segment.c
>>>>> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>>>>>  	__next_free_blkoff(sbi, curseg, 0);
>>>>>  
>>>>>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
>>>>> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>>>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
>>>>>  	f2fs_put_page(sum_page, 1);
>>>>> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
>>>>>  
>>>>>  			se = &sit_i->sentries[start];
>>>>>  			page = get_current_sit_page(sbi, start);
>>>>> +			if (IS_ERR(page))
>>>>> +				return PTR_ERR(page);
>>>>>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
>>>>>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
>>>>>  			f2fs_put_page(page, 1);
>>>>>
>>>
>>> .
>>>
> 
> .
> 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018/9/26 9:10, Chao Yu wrote:
> On 2018/9/26 8:18, Jaegeuk Kim wrote:
>> On 09/21, Chao Yu wrote:
>>> On 2018/9/21 5:46, Jaegeuk Kim wrote:
>>>> On 09/20, Chao Yu wrote:
>>>>> On 2018/9/19 1:56, Jaegeuk Kim wrote:
>>>>>> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
>>>>>> xfstests/generic/475.
>>>>>>
>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>>> ---
>>>>>> Change log from v1:
>>>>>>  - propagate errno
>>>>>>  - drop sum_pages
>>>>>>
>>>>>>  fs/f2fs/checkpoint.c | 10 +++++-----
>>>>>>  fs/f2fs/f2fs.h       |  2 +-
>>>>>>  fs/f2fs/gc.c         |  9 +++++++++
>>>>>>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
>>>>>>  fs/f2fs/recovery.c   |  2 ++
>>>>>>  fs/f2fs/segment.c    |  3 +++
>>>>>>  6 files changed, 44 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>>> index 01e0d8f5bbbe..1ddf332ce4b2 100644
>>>>>> --- a/fs/f2fs/checkpoint.c
>>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>>> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
>>>>>>  		if (PTR_ERR(page) == -EIO &&
>>>>>>  				++count <= DEFAULT_RETRY_IO_COUNT)
>>>>>>  			goto retry;
>>>>>> -
>>>>>>  		f2fs_stop_checkpoint(sbi, false);
>>>>>> -		f2fs_bug_on(sbi, 1);
>>>>>>  	}
>>>>>> -
>>>>>>  	return page;
>>>>>>  }
>>>>>>  
>>>>>> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
>>>>>>  
>>>>>>  	/* write cached NAT/SIT entries to NAT/SIT area */
>>>>>> -	f2fs_flush_nat_entries(sbi, cpc);
>>>>>> +	err = f2fs_flush_nat_entries(sbi, cpc);
>>>>>> +	if (err)
>>>>>> +		goto stop;
>>>>>
>>>>> To make sure, if partial NAT pages become dirty, flush them back to device
>>>>> outside checkpoint() is not harmful, right?
>>>>
>>>> I think so.
>>>
>>> Oh, one more case, now we use NAT #0, if partial NAT pages were
>>> writebacked, then we start to use NAT #1, later, in another checkpoint(),
>>> we update those NAT pages again, we will write them to NAT #0, which is
>>> belong to last valid checkpoint(), it's may cause corrupted checkpoint in
>>> scenario of SPO. So it's harmfull, right?
>>
>> We already stopped checkpoint anymore, so there'd be no way to proceed another
>> checkpoint. Am I missing something?
> 
> You're right, we have called f2fs_stop_checkpoint(), so we are safe now. :)

Can we allow to writeback fs metadata (NAT/SIT/SSA) in prior to checkpoint
to mitigate IO stress of checkpoint?

Maybe below conditions can be consider to trigger writebacking:
a) dirty count of meta (NAT/SIT/SSA) exceeds threshold
b) exceed time threshold

Thanks,

> 
> Thanks,
> 
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>>> +
>>>>>>  	f2fs_flush_sit_entries(sbi, cpc);
>>>>>>  
>>>>>>  	/* unlock all the fs_lock[] in do_checkpoint() */
>>>>>> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>  		f2fs_release_discard_addrs(sbi);
>>>>>>  	else
>>>>>>  		f2fs_clear_prefree_segments(sbi, cpc);
>>>>>> -
>>>>>> +stop:
>>>>>>  	unblock_operations(sbi);
>>>>>>  	stat_inc_cp_count(sbi->stat_info);
>>>>>>  
>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>> index a0c868127a7c..29021dbc8f2a 100644
>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
>>>>>>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
>>>>>>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
>>>>>>  			unsigned int segno, struct f2fs_summary_block *sum);
>>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>>>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
>>>>>>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
>>>>>>  int __init f2fs_create_node_manager_caches(void);
>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>> index 4bcc8a59fdef..c7d14282dbf4 100644
>>>>>> --- a/fs/f2fs/gc.c
>>>>>> +++ b/fs/f2fs/gc.c
>>>>>> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>>>>>>  	/* reference all summary page */
>>>>>>  	while (segno < end_segno) {
>>>>>>  		sum_page = f2fs_get_sum_page(sbi, segno++);
>>>>>> +		if (IS_ERR(sum_page)) {
>>>>>> +			end_segno = segno - 1;
>>>>>> +			for (segno = start_segno; segno < end_segno; segno++) {
>>>>>> +				sum_page = find_get_page(META_MAPPING(sbi),
>>>>>> +						GET_SUM_BLOCK(sbi, segno));
>>>>>> +				f2fs_put_page(sum_page, 0);
>>>>>
>>>>> find_get_page() will add one more reference on page, so we need to call
>>>>> f2fs_put_page(sum_page, 0) twice.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>> +			}
>>>>>> +			return PTR_ERR(sum_page);
>>>>>> +		}
>>>>>>  		unlock_page(sum_page);
>>>>>>  	}
>>>>>>  
>>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>>>> index fa2381c0bc47..79b6fee354f7 100644
>>>>>> --- a/fs/f2fs/node.c
>>>>>> +++ b/fs/f2fs/node.c
>>>>>> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
>>>>>>  
>>>>>>  	/* get current nat block page with lock */
>>>>>>  	src_page = get_current_nat_page(sbi, nid);
>>>>>> +	if (IS_ERR(src_page))
>>>>>> +		return src_page;
>>>>>>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
>>>>>>  	f2fs_bug_on(sbi, PageDirty(src_page));
>>>>>>  
>>>>>> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>>>>>>  						nm_i->nat_block_bitmap)) {
>>>>>>  			struct page *page = get_current_nat_page(sbi, nid);
>>>>>>  
>>>>>> -			ret = scan_nat_page(sbi, page, nid);
>>>>>> -			f2fs_put_page(page, 1);
>>>>>> +			if (IS_ERR(page)) {
>>>>>> +				ret = PTR_ERR(page);
>>>>>> +			} else {
>>>>>> +				ret = scan_nat_page(sbi, page, nid);
>>>>>> +				f2fs_put_page(page, 1);
>>>>>> +			}
>>>>>>  
>>>>>>  			if (ret) {
>>>>>>  				up_read(&nm_i->nat_tree_lock);
>>>>>>  				f2fs_bug_on(sbi, !mount);
>>>>>>  				f2fs_msg(sbi->sb, KERN_ERR,
>>>>>>  					"NAT is corrupt, run fsck to fix it");
>>>>>> -				return -EINVAL;
>>>>>> +				return ret;
>>>>>>  			}
>>>>>>  		}
>>>>>>  
>>>>>> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
>>>>>>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
>>>>>>  }
>>>>>>  
>>>>>> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>  		struct nat_entry_set *set, struct cp_control *cpc)
>>>>>>  {
>>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>>>>> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>  		down_write(&curseg->journal_rwsem);
>>>>>>  	} else {
>>>>>>  		page = get_next_nat_page(sbi, start_nid);
>>>>>> +		if (IS_ERR(page))
>>>>>> +			return PTR_ERR(page);
>>>>>> +
>>>>>>  		nat_blk = page_address(page);
>>>>>>  		f2fs_bug_on(sbi, !nat_blk);
>>>>>>  	}
>>>>>> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
>>>>>>  		kmem_cache_free(nat_entry_set_slab, set);
>>>>>>  	}
>>>>>> +	return 0;
>>>>>>  }
>>>>>>  
>>>>>>  /*
>>>>>>   * This function is called during the checkpointing process.
>>>>>>   */
>>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>  {
>>>>>>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
>>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>>>>> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>  	unsigned int found;
>>>>>>  	nid_t set_idx = 0;
>>>>>>  	LIST_HEAD(sets);
>>>>>> +	int err = 0;
>>>>>>  
>>>>>>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
>>>>>>  	if (enabled_nat_bits(sbi, cpc)) {
>>>>>> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>  	}
>>>>>>  
>>>>>>  	if (!nm_i->dirty_nat_cnt)
>>>>>> -		return;
>>>>>> +		return 0;
>>>>>>  
>>>>>>  	down_write(&nm_i->nat_tree_lock);
>>>>>>  
>>>>>> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>  	}
>>>>>>  
>>>>>>  	/* flush dirty nats in nat entry set */
>>>>>> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
>>>>>> -		__flush_nat_entry_set(sbi, set, cpc);
>>>>>> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
>>>>>> +		err = __flush_nat_entry_set(sbi, set, cpc);
>>>>>> +		if (err)
>>>>>> +			break;
>>>>>> +	}
>>>>>>  
>>>>>>  	up_write(&nm_i->nat_tree_lock);
>>>>>>  	/* Allow dirty nats by node block allocation in write_begin */
>>>>>> +
>>>>>> +	return err;
>>>>>>  }
>>>>>>  
>>>>>>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
>>>>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>>>>> index 56d34193a74b..ba678efe7cad 100644
>>>>>> --- a/fs/f2fs/recovery.c
>>>>>> +++ b/fs/f2fs/recovery.c
>>>>>> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
>>>>>>  	}
>>>>>>  
>>>>>>  	sum_page = f2fs_get_sum_page(sbi, segno);
>>>>>> +	if (IS_ERR(sum_page))
>>>>>> +		return PTR_ERR(sum_page);
>>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>>>>  	sum = sum_node->entries[blkoff];
>>>>>>  	f2fs_put_page(sum_page, 1);
>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>>> index aa96a371aaf8..ab7386a7e9d3 100644
>>>>>> --- a/fs/f2fs/segment.c
>>>>>> +++ b/fs/f2fs/segment.c
>>>>>> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>>>>>>  	__next_free_blkoff(sbi, curseg, 0);
>>>>>>  
>>>>>>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
>>>>>> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
>>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>>>>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
>>>>>>  	f2fs_put_page(sum_page, 1);
>>>>>> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
>>>>>>  
>>>>>>  			se = &sit_i->sentries[start];
>>>>>>  			page = get_current_sit_page(sbi, start);
>>>>>> +			if (IS_ERR(page))
>>>>>> +				return PTR_ERR(page);
>>>>>>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
>>>>>>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
>>>>>>  			f2fs_put_page(page, 1);
>>>>>>
>>>>
>>>> .
>>>>
>>
>> .
>>
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> .
> 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

On 09/26, Chao Yu wrote:
> On 2018/9/26 9:10, Chao Yu wrote:
> > On 2018/9/26 8:18, Jaegeuk Kim wrote:
> >> On 09/21, Chao Yu wrote:
> >>> On 2018/9/21 5:46, Jaegeuk Kim wrote:
> >>>> On 09/20, Chao Yu wrote:
> >>>>> On 2018/9/19 1:56, Jaegeuk Kim wrote:
> >>>>>> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> >>>>>> xfstests/generic/475.
> >>>>>>
> >>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>>>> ---
> >>>>>> Change log from v1:
> >>>>>>  - propagate errno
> >>>>>>  - drop sum_pages
> >>>>>>
> >>>>>>  fs/f2fs/checkpoint.c | 10 +++++-----
> >>>>>>  fs/f2fs/f2fs.h       |  2 +-
> >>>>>>  fs/f2fs/gc.c         |  9 +++++++++
> >>>>>>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
> >>>>>>  fs/f2fs/recovery.c   |  2 ++
> >>>>>>  fs/f2fs/segment.c    |  3 +++
> >>>>>>  6 files changed, 44 insertions(+), 14 deletions(-)
> >>>>>>
> >>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>>>>> index 01e0d8f5bbbe..1ddf332ce4b2 100644
> >>>>>> --- a/fs/f2fs/checkpoint.c
> >>>>>> +++ b/fs/f2fs/checkpoint.c
> >>>>>> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
> >>>>>>  		if (PTR_ERR(page) == -EIO &&
> >>>>>>  				++count <= DEFAULT_RETRY_IO_COUNT)
> >>>>>>  			goto retry;
> >>>>>> -
> >>>>>>  		f2fs_stop_checkpoint(sbi, false);
> >>>>>> -		f2fs_bug_on(sbi, 1);
> >>>>>>  	}
> >>>>>> -
> >>>>>>  	return page;
> >>>>>>  }
> >>>>>>  
> >>>>>> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
> >>>>>>  
> >>>>>>  	/* write cached NAT/SIT entries to NAT/SIT area */
> >>>>>> -	f2fs_flush_nat_entries(sbi, cpc);
> >>>>>> +	err = f2fs_flush_nat_entries(sbi, cpc);
> >>>>>> +	if (err)
> >>>>>> +		goto stop;
> >>>>>
> >>>>> To make sure, if partial NAT pages become dirty, flush them back to device
> >>>>> outside checkpoint() is not harmful, right?
> >>>>
> >>>> I think so.
> >>>
> >>> Oh, one more case, now we use NAT #0, if partial NAT pages were
> >>> writebacked, then we start to use NAT #1, later, in another checkpoint(),
> >>> we update those NAT pages again, we will write them to NAT #0, which is
> >>> belong to last valid checkpoint(), it's may cause corrupted checkpoint in
> >>> scenario of SPO. So it's harmfull, right?
> >>
> >> We already stopped checkpoint anymore, so there'd be no way to proceed another
> >> checkpoint. Am I missing something?
> > 
> > You're right, we have called f2fs_stop_checkpoint(), so we are safe now. :)
> 
> Can we allow to writeback fs metadata (NAT/SIT/SSA) in prior to checkpoint
> to mitigate IO stress of checkpoint?

What's the point related to this patch?
Anyway, do you mean f2fs_write_meta_pages is not enough?

> 
> Maybe below conditions can be consider to trigger writebacking:
> a) dirty count of meta (NAT/SIT/SSA) exceeds threshold
> b) exceed time threshold
> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> >>
> >>>
> >>> Thanks,
> >>>
> >>>>
> >>>>>
> >>>>>> +
> >>>>>>  	f2fs_flush_sit_entries(sbi, cpc);
> >>>>>>  
> >>>>>>  	/* unlock all the fs_lock[] in do_checkpoint() */
> >>>>>> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>  		f2fs_release_discard_addrs(sbi);
> >>>>>>  	else
> >>>>>>  		f2fs_clear_prefree_segments(sbi, cpc);
> >>>>>> -
> >>>>>> +stop:
> >>>>>>  	unblock_operations(sbi);
> >>>>>>  	stat_inc_cp_count(sbi->stat_info);
> >>>>>>  
> >>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>>>>> index a0c868127a7c..29021dbc8f2a 100644
> >>>>>> --- a/fs/f2fs/f2fs.h
> >>>>>> +++ b/fs/f2fs/f2fs.h
> >>>>>> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
> >>>>>>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
> >>>>>>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
> >>>>>>  			unsigned int segno, struct f2fs_summary_block *sum);
> >>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>>>>>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
> >>>>>>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
> >>>>>>  int __init f2fs_create_node_manager_caches(void);
> >>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> >>>>>> index 4bcc8a59fdef..c7d14282dbf4 100644
> >>>>>> --- a/fs/f2fs/gc.c
> >>>>>> +++ b/fs/f2fs/gc.c
> >>>>>> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >>>>>>  	/* reference all summary page */
> >>>>>>  	while (segno < end_segno) {
> >>>>>>  		sum_page = f2fs_get_sum_page(sbi, segno++);
> >>>>>> +		if (IS_ERR(sum_page)) {
> >>>>>> +			end_segno = segno - 1;
> >>>>>> +			for (segno = start_segno; segno < end_segno; segno++) {
> >>>>>> +				sum_page = find_get_page(META_MAPPING(sbi),
> >>>>>> +						GET_SUM_BLOCK(sbi, segno));
> >>>>>> +				f2fs_put_page(sum_page, 0);
> >>>>>
> >>>>> find_get_page() will add one more reference on page, so we need to call
> >>>>> f2fs_put_page(sum_page, 0) twice.
> >>>>
> >>>> Done.
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>>> +			}
> >>>>>> +			return PTR_ERR(sum_page);
> >>>>>> +		}
> >>>>>>  		unlock_page(sum_page);
> >>>>>>  	}
> >>>>>>  
> >>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >>>>>> index fa2381c0bc47..79b6fee354f7 100644
> >>>>>> --- a/fs/f2fs/node.c
> >>>>>> +++ b/fs/f2fs/node.c
> >>>>>> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
> >>>>>>  
> >>>>>>  	/* get current nat block page with lock */
> >>>>>>  	src_page = get_current_nat_page(sbi, nid);
> >>>>>> +	if (IS_ERR(src_page))
> >>>>>> +		return src_page;
> >>>>>>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
> >>>>>>  	f2fs_bug_on(sbi, PageDirty(src_page));
> >>>>>>  
> >>>>>> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> >>>>>>  						nm_i->nat_block_bitmap)) {
> >>>>>>  			struct page *page = get_current_nat_page(sbi, nid);
> >>>>>>  
> >>>>>> -			ret = scan_nat_page(sbi, page, nid);
> >>>>>> -			f2fs_put_page(page, 1);
> >>>>>> +			if (IS_ERR(page)) {
> >>>>>> +				ret = PTR_ERR(page);
> >>>>>> +			} else {
> >>>>>> +				ret = scan_nat_page(sbi, page, nid);
> >>>>>> +				f2fs_put_page(page, 1);
> >>>>>> +			}
> >>>>>>  
> >>>>>>  			if (ret) {
> >>>>>>  				up_read(&nm_i->nat_tree_lock);
> >>>>>>  				f2fs_bug_on(sbi, !mount);
> >>>>>>  				f2fs_msg(sbi->sb, KERN_ERR,
> >>>>>>  					"NAT is corrupt, run fsck to fix it");
> >>>>>> -				return -EINVAL;
> >>>>>> +				return ret;
> >>>>>>  			}
> >>>>>>  		}
> >>>>>>  
> >>>>>> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
> >>>>>>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
> >>>>>>  }
> >>>>>>  
> >>>>>> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>>>> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>>>>  		struct nat_entry_set *set, struct cp_control *cpc)
> >>>>>>  {
> >>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> >>>>>> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>>>>  		down_write(&curseg->journal_rwsem);
> >>>>>>  	} else {
> >>>>>>  		page = get_next_nat_page(sbi, start_nid);
> >>>>>> +		if (IS_ERR(page))
> >>>>>> +			return PTR_ERR(page);
> >>>>>> +
> >>>>>>  		nat_blk = page_address(page);
> >>>>>>  		f2fs_bug_on(sbi, !nat_blk);
> >>>>>>  	}
> >>>>>> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>>>>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
> >>>>>>  		kmem_cache_free(nat_entry_set_slab, set);
> >>>>>>  	}
> >>>>>> +	return 0;
> >>>>>>  }
> >>>>>>  
> >>>>>>  /*
> >>>>>>   * This function is called during the checkpointing process.
> >>>>>>   */
> >>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>  {
> >>>>>>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
> >>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> >>>>>> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>  	unsigned int found;
> >>>>>>  	nid_t set_idx = 0;
> >>>>>>  	LIST_HEAD(sets);
> >>>>>> +	int err = 0;
> >>>>>>  
> >>>>>>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
> >>>>>>  	if (enabled_nat_bits(sbi, cpc)) {
> >>>>>> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>  	}
> >>>>>>  
> >>>>>>  	if (!nm_i->dirty_nat_cnt)
> >>>>>> -		return;
> >>>>>> +		return 0;
> >>>>>>  
> >>>>>>  	down_write(&nm_i->nat_tree_lock);
> >>>>>>  
> >>>>>> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>  	}
> >>>>>>  
> >>>>>>  	/* flush dirty nats in nat entry set */
> >>>>>> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
> >>>>>> -		__flush_nat_entry_set(sbi, set, cpc);
> >>>>>> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
> >>>>>> +		err = __flush_nat_entry_set(sbi, set, cpc);
> >>>>>> +		if (err)
> >>>>>> +			break;
> >>>>>> +	}
> >>>>>>  
> >>>>>>  	up_write(&nm_i->nat_tree_lock);
> >>>>>>  	/* Allow dirty nats by node block allocation in write_begin */
> >>>>>> +
> >>>>>> +	return err;
> >>>>>>  }
> >>>>>>  
> >>>>>>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
> >>>>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> >>>>>> index 56d34193a74b..ba678efe7cad 100644
> >>>>>> --- a/fs/f2fs/recovery.c
> >>>>>> +++ b/fs/f2fs/recovery.c
> >>>>>> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
> >>>>>>  	}
> >>>>>>  
> >>>>>>  	sum_page = f2fs_get_sum_page(sbi, segno);
> >>>>>> +	if (IS_ERR(sum_page))
> >>>>>> +		return PTR_ERR(sum_page);
> >>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >>>>>>  	sum = sum_node->entries[blkoff];
> >>>>>>  	f2fs_put_page(sum_page, 1);
> >>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>>>>> index aa96a371aaf8..ab7386a7e9d3 100644
> >>>>>> --- a/fs/f2fs/segment.c
> >>>>>> +++ b/fs/f2fs/segment.c
> >>>>>> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
> >>>>>>  	__next_free_blkoff(sbi, curseg, 0);
> >>>>>>  
> >>>>>>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> >>>>>> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
> >>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >>>>>>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
> >>>>>>  	f2fs_put_page(sum_page, 1);
> >>>>>> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
> >>>>>>  
> >>>>>>  			se = &sit_i->sentries[start];
> >>>>>>  			page = get_current_sit_page(sbi, start);
> >>>>>> +			if (IS_ERR(page))
> >>>>>> +				return PTR_ERR(page);
> >>>>>>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
> >>>>>>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
> >>>>>>  			f2fs_put_page(page, 1);
> >>>>>>
> >>>>
> >>>> .
> >>>>
> >>
> >> .
> >>
> > 
> > 
> > 
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > 
> > .
> > 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018/9/27 3:49, Jaegeuk Kim wrote:
> On 09/26, Chao Yu wrote:
>> On 2018/9/26 9:10, Chao Yu wrote:
>>> On 2018/9/26 8:18, Jaegeuk Kim wrote:
>>>> On 09/21, Chao Yu wrote:
>>>>> On 2018/9/21 5:46, Jaegeuk Kim wrote:
>>>>>> On 09/20, Chao Yu wrote:
>>>>>>> On 2018/9/19 1:56, Jaegeuk Kim wrote:
>>>>>>>> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
>>>>>>>> xfstests/generic/475.
>>>>>>>>
>>>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>>>>> ---
>>>>>>>> Change log from v1:
>>>>>>>>  - propagate errno
>>>>>>>>  - drop sum_pages
>>>>>>>>
>>>>>>>>  fs/f2fs/checkpoint.c | 10 +++++-----
>>>>>>>>  fs/f2fs/f2fs.h       |  2 +-
>>>>>>>>  fs/f2fs/gc.c         |  9 +++++++++
>>>>>>>>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
>>>>>>>>  fs/f2fs/recovery.c   |  2 ++
>>>>>>>>  fs/f2fs/segment.c    |  3 +++
>>>>>>>>  6 files changed, 44 insertions(+), 14 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>>>>> index 01e0d8f5bbbe..1ddf332ce4b2 100644
>>>>>>>> --- a/fs/f2fs/checkpoint.c
>>>>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>>>>> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
>>>>>>>>  		if (PTR_ERR(page) == -EIO &&
>>>>>>>>  				++count <= DEFAULT_RETRY_IO_COUNT)
>>>>>>>>  			goto retry;
>>>>>>>> -
>>>>>>>>  		f2fs_stop_checkpoint(sbi, false);
>>>>>>>> -		f2fs_bug_on(sbi, 1);
>>>>>>>>  	}
>>>>>>>> -
>>>>>>>>  	return page;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
>>>>>>>>  
>>>>>>>>  	/* write cached NAT/SIT entries to NAT/SIT area */
>>>>>>>> -	f2fs_flush_nat_entries(sbi, cpc);
>>>>>>>> +	err = f2fs_flush_nat_entries(sbi, cpc);
>>>>>>>> +	if (err)
>>>>>>>> +		goto stop;
>>>>>>>
>>>>>>> To make sure, if partial NAT pages become dirty, flush them back to device
>>>>>>> outside checkpoint() is not harmful, right?
>>>>>>
>>>>>> I think so.
>>>>>
>>>>> Oh, one more case, now we use NAT #0, if partial NAT pages were
>>>>> writebacked, then we start to use NAT #1, later, in another checkpoint(),
>>>>> we update those NAT pages again, we will write them to NAT #0, which is
>>>>> belong to last valid checkpoint(), it's may cause corrupted checkpoint in
>>>>> scenario of SPO. So it's harmfull, right?
>>>>
>>>> We already stopped checkpoint anymore, so there'd be no way to proceed another
>>>> checkpoint. Am I missing something?
>>>
>>> You're right, we have called f2fs_stop_checkpoint(), so we are safe now. :)
>>
>> Can we allow to writeback fs metadata (NAT/SIT/SSA) in prior to checkpoint
>> to mitigate IO stress of checkpoint?
> 
> What's the point related to this patch?

It's not related to this patch.

> Anyway, do you mean f2fs_write_meta_pages is not enough?

Yup, f2fs_write_meta_pages can only flush summary pages, I guess we can
allow to flush dirty nat/sit entries to NAT/SIT blocks in some conditions,
and then flush those block in background.

Thanks,

> 
>>
>> Maybe below conditions can be consider to trigger writebacking:
>> a) dirty count of meta (NAT/SIT/SSA) exceeds threshold
>> b) exceed time threshold
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>>  	f2fs_flush_sit_entries(sbi, cpc);
>>>>>>>>  
>>>>>>>>  	/* unlock all the fs_lock[] in do_checkpoint() */
>>>>>>>> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>  		f2fs_release_discard_addrs(sbi);
>>>>>>>>  	else
>>>>>>>>  		f2fs_clear_prefree_segments(sbi, cpc);
>>>>>>>> -
>>>>>>>> +stop:
>>>>>>>>  	unblock_operations(sbi);
>>>>>>>>  	stat_inc_cp_count(sbi->stat_info);
>>>>>>>>  
>>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>>>> index a0c868127a7c..29021dbc8f2a 100644
>>>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>>>> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
>>>>>>>>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
>>>>>>>>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
>>>>>>>>  			unsigned int segno, struct f2fs_summary_block *sum);
>>>>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>>>>>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
>>>>>>>>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
>>>>>>>>  int __init f2fs_create_node_manager_caches(void);
>>>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>>>> index 4bcc8a59fdef..c7d14282dbf4 100644
>>>>>>>> --- a/fs/f2fs/gc.c
>>>>>>>> +++ b/fs/f2fs/gc.c
>>>>>>>> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>>>>>>>>  	/* reference all summary page */
>>>>>>>>  	while (segno < end_segno) {
>>>>>>>>  		sum_page = f2fs_get_sum_page(sbi, segno++);
>>>>>>>> +		if (IS_ERR(sum_page)) {
>>>>>>>> +			end_segno = segno - 1;
>>>>>>>> +			for (segno = start_segno; segno < end_segno; segno++) {
>>>>>>>> +				sum_page = find_get_page(META_MAPPING(sbi),
>>>>>>>> +						GET_SUM_BLOCK(sbi, segno));
>>>>>>>> +				f2fs_put_page(sum_page, 0);
>>>>>>>
>>>>>>> find_get_page() will add one more reference on page, so we need to call
>>>>>>> f2fs_put_page(sum_page, 0) twice.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>> +			}
>>>>>>>> +			return PTR_ERR(sum_page);
>>>>>>>> +		}
>>>>>>>>  		unlock_page(sum_page);
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>>>>>> index fa2381c0bc47..79b6fee354f7 100644
>>>>>>>> --- a/fs/f2fs/node.c
>>>>>>>> +++ b/fs/f2fs/node.c
>>>>>>>> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
>>>>>>>>  
>>>>>>>>  	/* get current nat block page with lock */
>>>>>>>>  	src_page = get_current_nat_page(sbi, nid);
>>>>>>>> +	if (IS_ERR(src_page))
>>>>>>>> +		return src_page;
>>>>>>>>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
>>>>>>>>  	f2fs_bug_on(sbi, PageDirty(src_page));
>>>>>>>>  
>>>>>>>> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>>>>>>>>  						nm_i->nat_block_bitmap)) {
>>>>>>>>  			struct page *page = get_current_nat_page(sbi, nid);
>>>>>>>>  
>>>>>>>> -			ret = scan_nat_page(sbi, page, nid);
>>>>>>>> -			f2fs_put_page(page, 1);
>>>>>>>> +			if (IS_ERR(page)) {
>>>>>>>> +				ret = PTR_ERR(page);
>>>>>>>> +			} else {
>>>>>>>> +				ret = scan_nat_page(sbi, page, nid);
>>>>>>>> +				f2fs_put_page(page, 1);
>>>>>>>> +			}
>>>>>>>>  
>>>>>>>>  			if (ret) {
>>>>>>>>  				up_read(&nm_i->nat_tree_lock);
>>>>>>>>  				f2fs_bug_on(sbi, !mount);
>>>>>>>>  				f2fs_msg(sbi->sb, KERN_ERR,
>>>>>>>>  					"NAT is corrupt, run fsck to fix it");
>>>>>>>> -				return -EINVAL;
>>>>>>>> +				return ret;
>>>>>>>>  			}
>>>>>>>>  		}
>>>>>>>>  
>>>>>>>> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
>>>>>>>>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>>> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>>>  		struct nat_entry_set *set, struct cp_control *cpc)
>>>>>>>>  {
>>>>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>>>>>>> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>>>  		down_write(&curseg->journal_rwsem);
>>>>>>>>  	} else {
>>>>>>>>  		page = get_next_nat_page(sbi, start_nid);
>>>>>>>> +		if (IS_ERR(page))
>>>>>>>> +			return PTR_ERR(page);
>>>>>>>> +
>>>>>>>>  		nat_blk = page_address(page);
>>>>>>>>  		f2fs_bug_on(sbi, !nat_blk);
>>>>>>>>  	}
>>>>>>>> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>>>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
>>>>>>>>  		kmem_cache_free(nat_entry_set_slab, set);
>>>>>>>>  	}
>>>>>>>> +	return 0;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>>  /*
>>>>>>>>   * This function is called during the checkpointing process.
>>>>>>>>   */
>>>>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>  {
>>>>>>>>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
>>>>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>>>>>>> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>  	unsigned int found;
>>>>>>>>  	nid_t set_idx = 0;
>>>>>>>>  	LIST_HEAD(sets);
>>>>>>>> +	int err = 0;
>>>>>>>>  
>>>>>>>>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
>>>>>>>>  	if (enabled_nat_bits(sbi, cpc)) {
>>>>>>>> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>>  	if (!nm_i->dirty_nat_cnt)
>>>>>>>> -		return;
>>>>>>>> +		return 0;
>>>>>>>>  
>>>>>>>>  	down_write(&nm_i->nat_tree_lock);
>>>>>>>>  
>>>>>>>> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>>  	/* flush dirty nats in nat entry set */
>>>>>>>> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
>>>>>>>> -		__flush_nat_entry_set(sbi, set, cpc);
>>>>>>>> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
>>>>>>>> +		err = __flush_nat_entry_set(sbi, set, cpc);
>>>>>>>> +		if (err)
>>>>>>>> +			break;
>>>>>>>> +	}
>>>>>>>>  
>>>>>>>>  	up_write(&nm_i->nat_tree_lock);
>>>>>>>>  	/* Allow dirty nats by node block allocation in write_begin */
>>>>>>>> +
>>>>>>>> +	return err;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
>>>>>>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>>>>>>> index 56d34193a74b..ba678efe7cad 100644
>>>>>>>> --- a/fs/f2fs/recovery.c
>>>>>>>> +++ b/fs/f2fs/recovery.c
>>>>>>>> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>>  	sum_page = f2fs_get_sum_page(sbi, segno);
>>>>>>>> +	if (IS_ERR(sum_page))
>>>>>>>> +		return PTR_ERR(sum_page);
>>>>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>>>>>>  	sum = sum_node->entries[blkoff];
>>>>>>>>  	f2fs_put_page(sum_page, 1);
>>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>>>>> index aa96a371aaf8..ab7386a7e9d3 100644
>>>>>>>> --- a/fs/f2fs/segment.c
>>>>>>>> +++ b/fs/f2fs/segment.c
>>>>>>>> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>>>>>>>>  	__next_free_blkoff(sbi, curseg, 0);
>>>>>>>>  
>>>>>>>>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
>>>>>>>> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
>>>>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>>>>>>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
>>>>>>>>  	f2fs_put_page(sum_page, 1);
>>>>>>>> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
>>>>>>>>  
>>>>>>>>  			se = &sit_i->sentries[start];
>>>>>>>>  			page = get_current_sit_page(sbi, start);
>>>>>>>> +			if (IS_ERR(page))
>>>>>>>> +				return PTR_ERR(page);
>>>>>>>>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
>>>>>>>>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
>>>>>>>>  			f2fs_put_page(page, 1);
>>>>>>>>
>>>>>>
>>>>>> .
>>>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>
>>> .
>>>
> 
> .
> 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

On 09/27, Chao Yu wrote:
> On 2018/9/27 3:49, Jaegeuk Kim wrote:
> > On 09/26, Chao Yu wrote:
> >> On 2018/9/26 9:10, Chao Yu wrote:
> >>> On 2018/9/26 8:18, Jaegeuk Kim wrote:
> >>>> On 09/21, Chao Yu wrote:
> >>>>> On 2018/9/21 5:46, Jaegeuk Kim wrote:
> >>>>>> On 09/20, Chao Yu wrote:
> >>>>>>> On 2018/9/19 1:56, Jaegeuk Kim wrote:
> >>>>>>>> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> >>>>>>>> xfstests/generic/475.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>>>>>> ---
> >>>>>>>> Change log from v1:
> >>>>>>>>  - propagate errno
> >>>>>>>>  - drop sum_pages
> >>>>>>>>
> >>>>>>>>  fs/f2fs/checkpoint.c | 10 +++++-----
> >>>>>>>>  fs/f2fs/f2fs.h       |  2 +-
> >>>>>>>>  fs/f2fs/gc.c         |  9 +++++++++
> >>>>>>>>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
> >>>>>>>>  fs/f2fs/recovery.c   |  2 ++
> >>>>>>>>  fs/f2fs/segment.c    |  3 +++
> >>>>>>>>  6 files changed, 44 insertions(+), 14 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>>>>>>> index 01e0d8f5bbbe..1ddf332ce4b2 100644
> >>>>>>>> --- a/fs/f2fs/checkpoint.c
> >>>>>>>> +++ b/fs/f2fs/checkpoint.c
> >>>>>>>> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
> >>>>>>>>  		if (PTR_ERR(page) == -EIO &&
> >>>>>>>>  				++count <= DEFAULT_RETRY_IO_COUNT)
> >>>>>>>>  			goto retry;
> >>>>>>>> -
> >>>>>>>>  		f2fs_stop_checkpoint(sbi, false);
> >>>>>>>> -		f2fs_bug_on(sbi, 1);
> >>>>>>>>  	}
> >>>>>>>> -
> >>>>>>>>  	return page;
> >>>>>>>>  }
> >>>>>>>>  
> >>>>>>>> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>>>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
> >>>>>>>>  
> >>>>>>>>  	/* write cached NAT/SIT entries to NAT/SIT area */
> >>>>>>>> -	f2fs_flush_nat_entries(sbi, cpc);
> >>>>>>>> +	err = f2fs_flush_nat_entries(sbi, cpc);
> >>>>>>>> +	if (err)
> >>>>>>>> +		goto stop;
> >>>>>>>
> >>>>>>> To make sure, if partial NAT pages become dirty, flush them back to device
> >>>>>>> outside checkpoint() is not harmful, right?
> >>>>>>
> >>>>>> I think so.
> >>>>>
> >>>>> Oh, one more case, now we use NAT #0, if partial NAT pages were
> >>>>> writebacked, then we start to use NAT #1, later, in another checkpoint(),
> >>>>> we update those NAT pages again, we will write them to NAT #0, which is
> >>>>> belong to last valid checkpoint(), it's may cause corrupted checkpoint in
> >>>>> scenario of SPO. So it's harmfull, right?
> >>>>
> >>>> We already stopped checkpoint anymore, so there'd be no way to proceed another
> >>>> checkpoint. Am I missing something?
> >>>
> >>> You're right, we have called f2fs_stop_checkpoint(), so we are safe now. :)
> >>
> >> Can we allow to writeback fs metadata (NAT/SIT/SSA) in prior to checkpoint
> >> to mitigate IO stress of checkpoint?
> > 
> > What's the point related to this patch?
> 
> It's not related to this patch.
> 
> > Anyway, do you mean f2fs_write_meta_pages is not enough?
> 
> Yup, f2fs_write_meta_pages can only flush summary pages, I guess we can
> allow to flush dirty nat/sit entries to NAT/SIT blocks in some conditions,
> and then flush those block in background.

I see, then NAT/SIT will be dirtied and flushed during checkpoint. Do you
mean flushing some of them partially in background, even if we don't know
it will be flushed again? We may need to gather some data first.

> 
> Thanks,
> 
> > 
> >>
> >> Maybe below conditions can be consider to trigger writebacking:
> >> a) dirty count of meta (NAT/SIT/SSA) exceeds threshold
> >> b) exceed time threshold
> >>
> >> Thanks,
> >>
> >>>
> >>> Thanks,
> >>>
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>> +
> >>>>>>>>  	f2fs_flush_sit_entries(sbi, cpc);
> >>>>>>>>  
> >>>>>>>>  	/* unlock all the fs_lock[] in do_checkpoint() */
> >>>>>>>> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>>>  		f2fs_release_discard_addrs(sbi);
> >>>>>>>>  	else
> >>>>>>>>  		f2fs_clear_prefree_segments(sbi, cpc);
> >>>>>>>> -
> >>>>>>>> +stop:
> >>>>>>>>  	unblock_operations(sbi);
> >>>>>>>>  	stat_inc_cp_count(sbi->stat_info);
> >>>>>>>>  
> >>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>>>>>>> index a0c868127a7c..29021dbc8f2a 100644
> >>>>>>>> --- a/fs/f2fs/f2fs.h
> >>>>>>>> +++ b/fs/f2fs/f2fs.h
> >>>>>>>> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
> >>>>>>>>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
> >>>>>>>>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
> >>>>>>>>  			unsigned int segno, struct f2fs_summary_block *sum);
> >>>>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>>>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>>>>>>>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
> >>>>>>>>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
> >>>>>>>>  int __init f2fs_create_node_manager_caches(void);
> >>>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> >>>>>>>> index 4bcc8a59fdef..c7d14282dbf4 100644
> >>>>>>>> --- a/fs/f2fs/gc.c
> >>>>>>>> +++ b/fs/f2fs/gc.c
> >>>>>>>> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >>>>>>>>  	/* reference all summary page */
> >>>>>>>>  	while (segno < end_segno) {
> >>>>>>>>  		sum_page = f2fs_get_sum_page(sbi, segno++);
> >>>>>>>> +		if (IS_ERR(sum_page)) {
> >>>>>>>> +			end_segno = segno - 1;
> >>>>>>>> +			for (segno = start_segno; segno < end_segno; segno++) {
> >>>>>>>> +				sum_page = find_get_page(META_MAPPING(sbi),
> >>>>>>>> +						GET_SUM_BLOCK(sbi, segno));
> >>>>>>>> +				f2fs_put_page(sum_page, 0);
> >>>>>>>
> >>>>>>> find_get_page() will add one more reference on page, so we need to call
> >>>>>>> f2fs_put_page(sum_page, 0) twice.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>>
> >>>>>>>> +			}
> >>>>>>>> +			return PTR_ERR(sum_page);
> >>>>>>>> +		}
> >>>>>>>>  		unlock_page(sum_page);
> >>>>>>>>  	}
> >>>>>>>>  
> >>>>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >>>>>>>> index fa2381c0bc47..79b6fee354f7 100644
> >>>>>>>> --- a/fs/f2fs/node.c
> >>>>>>>> +++ b/fs/f2fs/node.c
> >>>>>>>> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
> >>>>>>>>  
> >>>>>>>>  	/* get current nat block page with lock */
> >>>>>>>>  	src_page = get_current_nat_page(sbi, nid);
> >>>>>>>> +	if (IS_ERR(src_page))
> >>>>>>>> +		return src_page;
> >>>>>>>>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
> >>>>>>>>  	f2fs_bug_on(sbi, PageDirty(src_page));
> >>>>>>>>  
> >>>>>>>> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> >>>>>>>>  						nm_i->nat_block_bitmap)) {
> >>>>>>>>  			struct page *page = get_current_nat_page(sbi, nid);
> >>>>>>>>  
> >>>>>>>> -			ret = scan_nat_page(sbi, page, nid);
> >>>>>>>> -			f2fs_put_page(page, 1);
> >>>>>>>> +			if (IS_ERR(page)) {
> >>>>>>>> +				ret = PTR_ERR(page);
> >>>>>>>> +			} else {
> >>>>>>>> +				ret = scan_nat_page(sbi, page, nid);
> >>>>>>>> +				f2fs_put_page(page, 1);
> >>>>>>>> +			}
> >>>>>>>>  
> >>>>>>>>  			if (ret) {
> >>>>>>>>  				up_read(&nm_i->nat_tree_lock);
> >>>>>>>>  				f2fs_bug_on(sbi, !mount);
> >>>>>>>>  				f2fs_msg(sbi->sb, KERN_ERR,
> >>>>>>>>  					"NAT is corrupt, run fsck to fix it");
> >>>>>>>> -				return -EINVAL;
> >>>>>>>> +				return ret;
> >>>>>>>>  			}
> >>>>>>>>  		}
> >>>>>>>>  
> >>>>>>>> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
> >>>>>>>>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
> >>>>>>>>  }
> >>>>>>>>  
> >>>>>>>> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>>>>>> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>>>>>>  		struct nat_entry_set *set, struct cp_control *cpc)
> >>>>>>>>  {
> >>>>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> >>>>>>>> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>>>>>>  		down_write(&curseg->journal_rwsem);
> >>>>>>>>  	} else {
> >>>>>>>>  		page = get_next_nat_page(sbi, start_nid);
> >>>>>>>> +		if (IS_ERR(page))
> >>>>>>>> +			return PTR_ERR(page);
> >>>>>>>> +
> >>>>>>>>  		nat_blk = page_address(page);
> >>>>>>>>  		f2fs_bug_on(sbi, !nat_blk);
> >>>>>>>>  	}
> >>>>>>>> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> >>>>>>>>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
> >>>>>>>>  		kmem_cache_free(nat_entry_set_slab, set);
> >>>>>>>>  	}
> >>>>>>>> +	return 0;
> >>>>>>>>  }
> >>>>>>>>  
> >>>>>>>>  /*
> >>>>>>>>   * This function is called during the checkpointing process.
> >>>>>>>>   */
> >>>>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>>>  {
> >>>>>>>>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
> >>>>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
> >>>>>>>> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>>>  	unsigned int found;
> >>>>>>>>  	nid_t set_idx = 0;
> >>>>>>>>  	LIST_HEAD(sets);
> >>>>>>>> +	int err = 0;
> >>>>>>>>  
> >>>>>>>>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
> >>>>>>>>  	if (enabled_nat_bits(sbi, cpc)) {
> >>>>>>>> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>>>  	}
> >>>>>>>>  
> >>>>>>>>  	if (!nm_i->dirty_nat_cnt)
> >>>>>>>> -		return;
> >>>>>>>> +		return 0;
> >>>>>>>>  
> >>>>>>>>  	down_write(&nm_i->nat_tree_lock);
> >>>>>>>>  
> >>>>>>>> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>>>  	}
> >>>>>>>>  
> >>>>>>>>  	/* flush dirty nats in nat entry set */
> >>>>>>>> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
> >>>>>>>> -		__flush_nat_entry_set(sbi, set, cpc);
> >>>>>>>> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
> >>>>>>>> +		err = __flush_nat_entry_set(sbi, set, cpc);
> >>>>>>>> +		if (err)
> >>>>>>>> +			break;
> >>>>>>>> +	}
> >>>>>>>>  
> >>>>>>>>  	up_write(&nm_i->nat_tree_lock);
> >>>>>>>>  	/* Allow dirty nats by node block allocation in write_begin */
> >>>>>>>> +
> >>>>>>>> +	return err;
> >>>>>>>>  }
> >>>>>>>>  
> >>>>>>>>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
> >>>>>>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> >>>>>>>> index 56d34193a74b..ba678efe7cad 100644
> >>>>>>>> --- a/fs/f2fs/recovery.c
> >>>>>>>> +++ b/fs/f2fs/recovery.c
> >>>>>>>> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
> >>>>>>>>  	}
> >>>>>>>>  
> >>>>>>>>  	sum_page = f2fs_get_sum_page(sbi, segno);
> >>>>>>>> +	if (IS_ERR(sum_page))
> >>>>>>>> +		return PTR_ERR(sum_page);
> >>>>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >>>>>>>>  	sum = sum_node->entries[blkoff];
> >>>>>>>>  	f2fs_put_page(sum_page, 1);
> >>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>>>>>>> index aa96a371aaf8..ab7386a7e9d3 100644
> >>>>>>>> --- a/fs/f2fs/segment.c
> >>>>>>>> +++ b/fs/f2fs/segment.c
> >>>>>>>> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
> >>>>>>>>  	__next_free_blkoff(sbi, curseg, 0);
> >>>>>>>>  
> >>>>>>>>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
> >>>>>>>> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
> >>>>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> >>>>>>>>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
> >>>>>>>>  	f2fs_put_page(sum_page, 1);
> >>>>>>>> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
> >>>>>>>>  
> >>>>>>>>  			se = &sit_i->sentries[start];
> >>>>>>>>  			page = get_current_sit_page(sbi, start);
> >>>>>>>> +			if (IS_ERR(page))
> >>>>>>>> +				return PTR_ERR(page);
> >>>>>>>>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
> >>>>>>>>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
> >>>>>>>>  			f2fs_put_page(page, 1);
> >>>>>>>>
> >>>>>>
> >>>>>> .
> >>>>>>
> >>>>
> >>>> .
> >>>>
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> Linux-f2fs-devel mailing list
> >>> Linux-f2fs-devel@lists.sourceforge.net
> >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>
> >>> .
> >>>
> > 
> > .
> > 

Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018/9/27 12:58, Jaegeuk Kim wrote:
> On 09/27, Chao Yu wrote:
>> On 2018/9/27 3:49, Jaegeuk Kim wrote:
>>> On 09/26, Chao Yu wrote:
>>>> On 2018/9/26 9:10, Chao Yu wrote:
>>>>> On 2018/9/26 8:18, Jaegeuk Kim wrote:
>>>>>> On 09/21, Chao Yu wrote:
>>>>>>> On 2018/9/21 5:46, Jaegeuk Kim wrote:
>>>>>>>> On 09/20, Chao Yu wrote:
>>>>>>>>> On 2018/9/19 1:56, Jaegeuk Kim wrote:
>>>>>>>>>> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
>>>>>>>>>> xfstests/generic/475.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>>>>>>> ---
>>>>>>>>>> Change log from v1:
>>>>>>>>>>  - propagate errno
>>>>>>>>>>  - drop sum_pages
>>>>>>>>>>
>>>>>>>>>>  fs/f2fs/checkpoint.c | 10 +++++-----
>>>>>>>>>>  fs/f2fs/f2fs.h       |  2 +-
>>>>>>>>>>  fs/f2fs/gc.c         |  9 +++++++++
>>>>>>>>>>  fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
>>>>>>>>>>  fs/f2fs/recovery.c   |  2 ++
>>>>>>>>>>  fs/f2fs/segment.c    |  3 +++
>>>>>>>>>>  6 files changed, 44 insertions(+), 14 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>>>>>>> index 01e0d8f5bbbe..1ddf332ce4b2 100644
>>>>>>>>>> --- a/fs/f2fs/checkpoint.c
>>>>>>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>>>>>>> @@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
>>>>>>>>>>  		if (PTR_ERR(page) == -EIO &&
>>>>>>>>>>  				++count <= DEFAULT_RETRY_IO_COUNT)
>>>>>>>>>>  			goto retry;
>>>>>>>>>> -
>>>>>>>>>>  		f2fs_stop_checkpoint(sbi, false);
>>>>>>>>>> -		f2fs_bug_on(sbi, 1);
>>>>>>>>>>  	}
>>>>>>>>>> -
>>>>>>>>>>  	return page;
>>>>>>>>>>  }
>>>>>>>>>>  
>>>>>>>>>> @@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>>>  	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
>>>>>>>>>>  
>>>>>>>>>>  	/* write cached NAT/SIT entries to NAT/SIT area */
>>>>>>>>>> -	f2fs_flush_nat_entries(sbi, cpc);
>>>>>>>>>> +	err = f2fs_flush_nat_entries(sbi, cpc);
>>>>>>>>>> +	if (err)
>>>>>>>>>> +		goto stop;
>>>>>>>>>
>>>>>>>>> To make sure, if partial NAT pages become dirty, flush them back to device
>>>>>>>>> outside checkpoint() is not harmful, right?
>>>>>>>>
>>>>>>>> I think so.
>>>>>>>
>>>>>>> Oh, one more case, now we use NAT #0, if partial NAT pages were
>>>>>>> writebacked, then we start to use NAT #1, later, in another checkpoint(),
>>>>>>> we update those NAT pages again, we will write them to NAT #0, which is
>>>>>>> belong to last valid checkpoint(), it's may cause corrupted checkpoint in
>>>>>>> scenario of SPO. So it's harmfull, right?
>>>>>>
>>>>>> We already stopped checkpoint anymore, so there'd be no way to proceed another
>>>>>> checkpoint. Am I missing something?
>>>>>
>>>>> You're right, we have called f2fs_stop_checkpoint(), so we are safe now. :)
>>>>
>>>> Can we allow to writeback fs metadata (NAT/SIT/SSA) in prior to checkpoint
>>>> to mitigate IO stress of checkpoint?
>>>
>>> What's the point related to this patch?
>>
>> It's not related to this patch.
>>
>>> Anyway, do you mean f2fs_write_meta_pages is not enough?
>>
>> Yup, f2fs_write_meta_pages can only flush summary pages, I guess we can
>> allow to flush dirty nat/sit entries to NAT/SIT blocks in some conditions,
>> and then flush those block in background.
> 
> I see, then NAT/SIT will be dirtied and flushed during checkpoint. Do you
> mean flushing some of them partially in background, even if we don't know
> it will be flushed again? We may need to gather some data first.

Yes, agreed, let me check that.

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Maybe below conditions can be consider to trigger writebacking:
>>>> a) dirty count of meta (NAT/SIT/SSA) exceeds threshold
>>>> b) exceed time threshold
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>>  	f2fs_flush_sit_entries(sbi, cpc);
>>>>>>>>>>  
>>>>>>>>>>  	/* unlock all the fs_lock[] in do_checkpoint() */
>>>>>>>>>> @@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>>>  		f2fs_release_discard_addrs(sbi);
>>>>>>>>>>  	else
>>>>>>>>>>  		f2fs_clear_prefree_segments(sbi, cpc);
>>>>>>>>>> -
>>>>>>>>>> +stop:
>>>>>>>>>>  	unblock_operations(sbi);
>>>>>>>>>>  	stat_inc_cp_count(sbi->stat_info);
>>>>>>>>>>  
>>>>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>>>>>> index a0c868127a7c..29021dbc8f2a 100644
>>>>>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>>>>>> @@ -2895,7 +2895,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
>>>>>>>>>>  int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
>>>>>>>>>>  int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
>>>>>>>>>>  			unsigned int segno, struct f2fs_summary_block *sum);
>>>>>>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>>>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>>>>>>>  int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
>>>>>>>>>>  void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
>>>>>>>>>>  int __init f2fs_create_node_manager_caches(void);
>>>>>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>>>>>> index 4bcc8a59fdef..c7d14282dbf4 100644
>>>>>>>>>> --- a/fs/f2fs/gc.c
>>>>>>>>>> +++ b/fs/f2fs/gc.c
>>>>>>>>>> @@ -1070,6 +1070,15 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>>>>>>>>>>  	/* reference all summary page */
>>>>>>>>>>  	while (segno < end_segno) {
>>>>>>>>>>  		sum_page = f2fs_get_sum_page(sbi, segno++);
>>>>>>>>>> +		if (IS_ERR(sum_page)) {
>>>>>>>>>> +			end_segno = segno - 1;
>>>>>>>>>> +			for (segno = start_segno; segno < end_segno; segno++) {
>>>>>>>>>> +				sum_page = find_get_page(META_MAPPING(sbi),
>>>>>>>>>> +						GET_SUM_BLOCK(sbi, segno));
>>>>>>>>>> +				f2fs_put_page(sum_page, 0);
>>>>>>>>>
>>>>>>>>> find_get_page() will add one more reference on page, so we need to call
>>>>>>>>> f2fs_put_page(sum_page, 0) twice.
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>>> +			}
>>>>>>>>>> +			return PTR_ERR(sum_page);
>>>>>>>>>> +		}
>>>>>>>>>>  		unlock_page(sum_page);
>>>>>>>>>>  	}
>>>>>>>>>>  
>>>>>>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>>>>>>>> index fa2381c0bc47..79b6fee354f7 100644
>>>>>>>>>> --- a/fs/f2fs/node.c
>>>>>>>>>> +++ b/fs/f2fs/node.c
>>>>>>>>>> @@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
>>>>>>>>>>  
>>>>>>>>>>  	/* get current nat block page with lock */
>>>>>>>>>>  	src_page = get_current_nat_page(sbi, nid);
>>>>>>>>>> +	if (IS_ERR(src_page))
>>>>>>>>>> +		return src_page;
>>>>>>>>>>  	dst_page = f2fs_grab_meta_page(sbi, dst_off);
>>>>>>>>>>  	f2fs_bug_on(sbi, PageDirty(src_page));
>>>>>>>>>>  
>>>>>>>>>> @@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>>>>>>>>>>  						nm_i->nat_block_bitmap)) {
>>>>>>>>>>  			struct page *page = get_current_nat_page(sbi, nid);
>>>>>>>>>>  
>>>>>>>>>> -			ret = scan_nat_page(sbi, page, nid);
>>>>>>>>>> -			f2fs_put_page(page, 1);
>>>>>>>>>> +			if (IS_ERR(page)) {
>>>>>>>>>> +				ret = PTR_ERR(page);
>>>>>>>>>> +			} else {
>>>>>>>>>> +				ret = scan_nat_page(sbi, page, nid);
>>>>>>>>>> +				f2fs_put_page(page, 1);
>>>>>>>>>> +			}
>>>>>>>>>>  
>>>>>>>>>>  			if (ret) {
>>>>>>>>>>  				up_read(&nm_i->nat_tree_lock);
>>>>>>>>>>  				f2fs_bug_on(sbi, !mount);
>>>>>>>>>>  				f2fs_msg(sbi->sb, KERN_ERR,
>>>>>>>>>>  					"NAT is corrupt, run fsck to fix it");
>>>>>>>>>> -				return -EINVAL;
>>>>>>>>>> +				return ret;
>>>>>>>>>>  			}
>>>>>>>>>>  		}
>>>>>>>>>>  
>>>>>>>>>> @@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
>>>>>>>>>>  		__clear_bit_le(nat_index, nm_i->full_nat_bits);
>>>>>>>>>>  }
>>>>>>>>>>  
>>>>>>>>>> -static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>>>>> +static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>>>>>  		struct nat_entry_set *set, struct cp_control *cpc)
>>>>>>>>>>  {
>>>>>>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>>>>>>>>> @@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>>>>>  		down_write(&curseg->journal_rwsem);
>>>>>>>>>>  	} else {
>>>>>>>>>>  		page = get_next_nat_page(sbi, start_nid);
>>>>>>>>>> +		if (IS_ERR(page))
>>>>>>>>>> +			return PTR_ERR(page);
>>>>>>>>>> +
>>>>>>>>>>  		nat_blk = page_address(page);
>>>>>>>>>>  		f2fs_bug_on(sbi, !nat_blk);
>>>>>>>>>>  	}
>>>>>>>>>> @@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>>>>>>>>>  		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
>>>>>>>>>>  		kmem_cache_free(nat_entry_set_slab, set);
>>>>>>>>>>  	}
>>>>>>>>>> +	return 0;
>>>>>>>>>>  }
>>>>>>>>>>  
>>>>>>>>>>  /*
>>>>>>>>>>   * This function is called during the checkpointing process.
>>>>>>>>>>   */
>>>>>>>>>> -void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>>> +int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>>>  {
>>>>>>>>>>  	struct f2fs_nm_info *nm_i = NM_I(sbi);
>>>>>>>>>>  	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
>>>>>>>>>> @@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>>>  	unsigned int found;
>>>>>>>>>>  	nid_t set_idx = 0;
>>>>>>>>>>  	LIST_HEAD(sets);
>>>>>>>>>> +	int err = 0;
>>>>>>>>>>  
>>>>>>>>>>  	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
>>>>>>>>>>  	if (enabled_nat_bits(sbi, cpc)) {
>>>>>>>>>> @@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>>>  	}
>>>>>>>>>>  
>>>>>>>>>>  	if (!nm_i->dirty_nat_cnt)
>>>>>>>>>> -		return;
>>>>>>>>>> +		return 0;
>>>>>>>>>>  
>>>>>>>>>>  	down_write(&nm_i->nat_tree_lock);
>>>>>>>>>>  
>>>>>>>>>> @@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>>>  	}
>>>>>>>>>>  
>>>>>>>>>>  	/* flush dirty nats in nat entry set */
>>>>>>>>>> -	list_for_each_entry_safe(set, tmp, &sets, set_list)
>>>>>>>>>> -		__flush_nat_entry_set(sbi, set, cpc);
>>>>>>>>>> +	list_for_each_entry_safe(set, tmp, &sets, set_list) {
>>>>>>>>>> +		err = __flush_nat_entry_set(sbi, set, cpc);
>>>>>>>>>> +		if (err)
>>>>>>>>>> +			break;
>>>>>>>>>> +	}
>>>>>>>>>>  
>>>>>>>>>>  	up_write(&nm_i->nat_tree_lock);
>>>>>>>>>>  	/* Allow dirty nats by node block allocation in write_begin */
>>>>>>>>>> +
>>>>>>>>>> +	return err;
>>>>>>>>>>  }
>>>>>>>>>>  
>>>>>>>>>>  static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
>>>>>>>>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>>>>>>>>> index 56d34193a74b..ba678efe7cad 100644
>>>>>>>>>> --- a/fs/f2fs/recovery.c
>>>>>>>>>> +++ b/fs/f2fs/recovery.c
>>>>>>>>>> @@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
>>>>>>>>>>  	}
>>>>>>>>>>  
>>>>>>>>>>  	sum_page = f2fs_get_sum_page(sbi, segno);
>>>>>>>>>> +	if (IS_ERR(sum_page))
>>>>>>>>>> +		return PTR_ERR(sum_page);
>>>>>>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>>>>>>>>  	sum = sum_node->entries[blkoff];
>>>>>>>>>>  	f2fs_put_page(sum_page, 1);
>>>>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>>>>>>> index aa96a371aaf8..ab7386a7e9d3 100644
>>>>>>>>>> --- a/fs/f2fs/segment.c
>>>>>>>>>> +++ b/fs/f2fs/segment.c
>>>>>>>>>> @@ -2487,6 +2487,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>>>>>>>>>>  	__next_free_blkoff(sbi, curseg, 0);
>>>>>>>>>>  
>>>>>>>>>>  	sum_page = f2fs_get_sum_page(sbi, new_segno);
>>>>>>>>>> +	f2fs_bug_on(sbi, IS_ERR(sum_page));
>>>>>>>>>>  	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
>>>>>>>>>>  	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
>>>>>>>>>>  	f2fs_put_page(sum_page, 1);
>>>>>>>>>> @@ -3971,6 +3972,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
>>>>>>>>>>  
>>>>>>>>>>  			se = &sit_i->sentries[start];
>>>>>>>>>>  			page = get_current_sit_page(sbi, start);
>>>>>>>>>> +			if (IS_ERR(page))
>>>>>>>>>> +				return PTR_ERR(page);
>>>>>>>>>>  			sit_blk = (struct f2fs_sit_block *)page_address(page);
>>>>>>>>>>  			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
>>>>>>>>>>  			f2fs_put_page(page, 1);
>>>>>>>>>>
>>>>>>>>
>>>>>>>> .
>>>>>>>>
>>>>>>
>>>>>> .
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Linux-f2fs-devel mailing list
>>>>> Linux-f2fs-devel@lists.sourceforge.net
>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>>
>>>>> .
>>>>>
>>>
>>> .
>>>
> 
> .
> 

Re: [f2fs-dev] [PATCH 2/2 v3] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Jaegeuk Kim]

This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
xfstests/generic/475.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
Change log from v2:
 - fix sum_page error report
 - fix missing put_page for sum_page

 fs/f2fs/checkpoint.c | 10 +++++-----
 fs/f2fs/f2fs.h       |  2 +-
 fs/f2fs/gc.c         | 12 ++++++++++++
 fs/f2fs/node.c       | 32 ++++++++++++++++++++++++--------
 fs/f2fs/recovery.c   |  2 ++
 fs/f2fs/segment.c    |  3 +++
 6 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 01e0d8f5bbbe..1ddf332ce4b2 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -119,11 +119,8 @@ struct page *f2fs_get_meta_page_nofail(struct f2fs_sb_info *sbi, pgoff_t index)
 		if (PTR_ERR(page) == -EIO &&
 				++count <= DEFAULT_RETRY_IO_COUNT)
 			goto retry;
-
 		f2fs_stop_checkpoint(sbi, false);
-		f2fs_bug_on(sbi, 1);
 	}
-
 	return page;
 }
 
@@ -1512,7 +1509,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	ckpt->checkpoint_ver = cpu_to_le64(++ckpt_ver);
 
 	/* write cached NAT/SIT entries to NAT/SIT area */
-	f2fs_flush_nat_entries(sbi, cpc);
+	err = f2fs_flush_nat_entries(sbi, cpc);
+	if (err)
+		goto stop;
+
 	f2fs_flush_sit_entries(sbi, cpc);
 
 	/* unlock all the fs_lock[] in do_checkpoint() */
@@ -1521,7 +1521,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		f2fs_release_discard_addrs(sbi);
 	else
 		f2fs_clear_prefree_segments(sbi, cpc);
-
+stop:
 	unblock_operations(sbi);
 	stat_inc_cp_count(sbi->stat_info);
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b18e8a44eea1..caf3b755503c 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2919,7 +2919,7 @@ int f2fs_recover_xattr_data(struct inode *inode, struct page *page);
 int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page);
 int f2fs_restore_node_summary(struct f2fs_sb_info *sbi,
 			unsigned int segno, struct f2fs_summary_block *sum);
-void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
+int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc);
 int f2fs_build_node_manager(struct f2fs_sb_info *sbi);
 void f2fs_destroy_node_manager(struct f2fs_sb_info *sbi);
 int __init f2fs_create_node_manager_caches(void);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 519af97e1f3f..e7c69a143bee 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1070,6 +1070,18 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
 	/* reference all summary page */
 	while (segno < end_segno) {
 		sum_page = f2fs_get_sum_page(sbi, segno++);
+		if (IS_ERR(sum_page)) {
+			int err = PTR_ERR(sum_page);
+
+			end_segno = segno - 1;
+			for (segno = start_segno; segno < end_segno; segno++) {
+				sum_page = find_get_page(META_MAPPING(sbi),
+						GET_SUM_BLOCK(sbi, segno));
+				f2fs_put_page(sum_page, 0);
+				f2fs_put_page(sum_page, 0);
+			}
+			return err;
+		}
 		unlock_page(sum_page);
 	}
 
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index fa2381c0bc47..79b6fee354f7 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -126,6 +126,8 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid)
 
 	/* get current nat block page with lock */
 	src_page = get_current_nat_page(sbi, nid);
+	if (IS_ERR(src_page))
+		return src_page;
 	dst_page = f2fs_grab_meta_page(sbi, dst_off);
 	f2fs_bug_on(sbi, PageDirty(src_page));
 
@@ -2265,15 +2267,19 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
 						nm_i->nat_block_bitmap)) {
 			struct page *page = get_current_nat_page(sbi, nid);
 
-			ret = scan_nat_page(sbi, page, nid);
-			f2fs_put_page(page, 1);
+			if (IS_ERR(page)) {
+				ret = PTR_ERR(page);
+			} else {
+				ret = scan_nat_page(sbi, page, nid);
+				f2fs_put_page(page, 1);
+			}
 
 			if (ret) {
 				up_read(&nm_i->nat_tree_lock);
 				f2fs_bug_on(sbi, !mount);
 				f2fs_msg(sbi->sb, KERN_ERR,
 					"NAT is corrupt, run fsck to fix it");
-				return -EINVAL;
+				return ret;
 			}
 		}
 
@@ -2700,7 +2706,7 @@ static void __update_nat_bits(struct f2fs_sb_info *sbi, nid_t start_nid,
 		__clear_bit_le(nat_index, nm_i->full_nat_bits);
 }
 
-static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
+static int __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 		struct nat_entry_set *set, struct cp_control *cpc)
 {
 	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
@@ -2724,6 +2730,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 		down_write(&curseg->journal_rwsem);
 	} else {
 		page = get_next_nat_page(sbi, start_nid);
+		if (IS_ERR(page))
+			return PTR_ERR(page);
+
 		nat_blk = page_address(page);
 		f2fs_bug_on(sbi, !nat_blk);
 	}
@@ -2769,12 +2778,13 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 		radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
 		kmem_cache_free(nat_entry_set_slab, set);
 	}
+	return 0;
 }
 
 /*
  * This function is called during the checkpointing process.
  */
-void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
+int f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 {
 	struct f2fs_nm_info *nm_i = NM_I(sbi);
 	struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA);
@@ -2784,6 +2794,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	unsigned int found;
 	nid_t set_idx = 0;
 	LIST_HEAD(sets);
+	int err = 0;
 
 	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
 	if (enabled_nat_bits(sbi, cpc)) {
@@ -2793,7 +2804,7 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	}
 
 	if (!nm_i->dirty_nat_cnt)
-		return;
+		return 0;
 
 	down_write(&nm_i->nat_tree_lock);
 
@@ -2816,11 +2827,16 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	}
 
 	/* flush dirty nats in nat entry set */
-	list_for_each_entry_safe(set, tmp, &sets, set_list)
-		__flush_nat_entry_set(sbi, set, cpc);
+	list_for_each_entry_safe(set, tmp, &sets, set_list) {
+		err = __flush_nat_entry_set(sbi, set, cpc);
+		if (err)
+			break;
+	}
 
 	up_write(&nm_i->nat_tree_lock);
 	/* Allow dirty nats by node block allocation in write_begin */
+
+	return err;
 }
 
 static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 56d34193a74b..ba678efe7cad 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -355,6 +355,8 @@ static int check_index_in_prev_nodes(struct f2fs_sb_info *sbi,
 	}
 
 	sum_page = f2fs_get_sum_page(sbi, segno);
+	if (IS_ERR(sum_page))
+		return PTR_ERR(sum_page);
 	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
 	sum = sum_node->entries[blkoff];
 	f2fs_put_page(sum_page, 1);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index c18081034be0..6f7b45853dc5 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2483,6 +2483,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
 	__next_free_blkoff(sbi, curseg, 0);
 
 	sum_page = f2fs_get_sum_page(sbi, new_segno);
+	f2fs_bug_on(sbi, IS_ERR(sum_page));
 	sum_node = (struct f2fs_summary_block *)page_address(sum_page);
 	memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
 	f2fs_put_page(sum_page, 1);
@@ -3967,6 +3968,8 @@ static int build_sit_entries(struct f2fs_sb_info *sbi)
 
 			se = &sit_i->sentries[start];
 			page = get_current_sit_page(sbi, start);
+			if (IS_ERR(page))
+				return PTR_ERR(page);
 			sit_blk = (struct f2fs_sit_block *)page_address(page);
 			sit = sit_blk->entries[SIT_ENTRY_OFFSET(sit_i, start)];
 			f2fs_put_page(page, 1);
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev] [PATCH 2/2 v3] f2fs: avoid f2fs_bug_on if f2fs_get_meta_page_nofail got EIO

[Chao Yu]

On 2018-9-21 5:48, Jaegeuk Kim wrote:
> This patch avoids BUG_ON when f2fs_get_meta_page_nofail got EIO during
> xfstests/generic/475.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [f2fs-dev] [PATCH 1/2] f2fs: report ENOENT correct in f2fs_rename

[Chao Yu]

On 2018/9/18 10:18, Jaegeuk Kim wrote:
> This fixes wrong error report in f2fs_rename.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/namei.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 98d3ab7c3ce6..d653be777529 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -833,7 +833,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	struct f2fs_dir_entry *old_entry;
>  	struct f2fs_dir_entry *new_entry;
>  	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
> -	int err = -ENOENT;
> +	int err;
>  
>  	if (unlikely(f2fs_cp_error(sbi)))
>  		return -EIO;
> @@ -854,6 +854,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (err)
>  		goto out;
>  
> +	err = -ENOENT;

I didn't get what's the difference after moving initialization of 'err' here...

Thanks,

>  	if (new_inode) {
>  		err = dquot_initialize(new_inode);
>  		if (err)
> 

Re: [f2fs-dev] [PATCH 1/2] f2fs: report ENOENT correct in f2fs_rename

[Jaegeuk Kim]

On 09/18, Chao Yu wrote:
> On 2018/9/18 10:18, Jaegeuk Kim wrote:
> > This fixes wrong error report in f2fs_rename.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/namei.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> > index 98d3ab7c3ce6..d653be777529 100644
> > --- a/fs/f2fs/namei.c
> > +++ b/fs/f2fs/namei.c
> > @@ -833,7 +833,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >  	struct f2fs_dir_entry *old_entry;
> >  	struct f2fs_dir_entry *new_entry;
> >  	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
> > -	int err = -ENOENT;
> > +	int err;
> >  
> >  	if (unlikely(f2fs_cp_error(sbi)))
> >  		return -EIO;
> > @@ -854,6 +854,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >  	if (err)
> >  		goto out;
> >  
> > +	err = -ENOENT;
> 
> I didn't get what's the difference after moving initialization of 'err' here...

One example,

        old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
        if (!old_entry) {
                if (IS_ERR(old_page))
                        err = PTR_ERR(old_page);

--> need to return ENOENT instead of 0.

                goto out;
        }

> 
> Thanks,
> 
> >  	if (new_inode) {
> >  		err = dquot_initialize(new_inode);
> >  		if (err)
> > 

Re: [f2fs-dev] [PATCH 1/2] f2fs: report ENOENT correct in f2fs_rename

[Chao Yu]

On 2018/9/19 1:59, Jaegeuk Kim wrote:
> On 09/18, Chao Yu wrote:
>> On 2018/9/18 10:18, Jaegeuk Kim wrote:
>>> This fixes wrong error report in f2fs_rename.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/namei.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>>> index 98d3ab7c3ce6..d653be777529 100644
>>> --- a/fs/f2fs/namei.c
>>> +++ b/fs/f2fs/namei.c
>>> @@ -833,7 +833,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  	struct f2fs_dir_entry *old_entry;
>>>  	struct f2fs_dir_entry *new_entry;
>>>  	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
>>> -	int err = -ENOENT;
>>> +	int err;
>>>  
>>>  	if (unlikely(f2fs_cp_error(sbi)))
>>>  		return -EIO;
>>> @@ -854,6 +854,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>  	if (err)
>>>  		goto out;
>>>  
>>> +	err = -ENOENT;
>>
>> I didn't get what's the difference after moving initialization of 'err' here...
> 
> One example,
> 
>         old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
>         if (!old_entry) {
>                 if (IS_ERR(old_page))
>                         err = PTR_ERR(old_page);
> 
> --> need to return ENOENT instead of 0.

Clear for me now, thanks for your explanation, I just miss that case. ;)

It would be better to add this into commit log.

I agree with Sahitya, we need to cover rename2 as well.

Thanks,

> 
>                 goto out;
>         }
> 
>>
>> Thanks,
>>
>>>  	if (new_inode) {
>>>  		err = dquot_initialize(new_inode);
>>>  		if (err)
>>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> 

Re: [f2fs-dev] [PATCH 1/2] f2fs: report ENOENT correct in f2fs_rename

[Sahitya Tummala]

On Mon, Sep 17, 2018 at 07:18:04PM -0700, Jaegeuk Kim wrote:
> This fixes wrong error report in f2fs_rename.

I think f2fs_cross_rename() path also needs the same fix?

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/namei.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 98d3ab7c3ce6..d653be777529 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -833,7 +833,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	struct f2fs_dir_entry *old_entry;
>  	struct f2fs_dir_entry *new_entry;
>  	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
> -	int err = -ENOENT;
> +	int err;
>  
>  	if (unlikely(f2fs_cp_error(sbi)))
>  		return -EIO;
> @@ -854,6 +854,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (err)
>  		goto out;
>  
> +	err = -ENOENT;
>  	if (new_inode) {
>  		err = dquot_initialize(new_inode);
>  		if (err)
> -- 
> 2.17.0.441.gb46fe60e1d-goog
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

-- 
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/2 v2] f2fs: report ENOENT correct in f2fs_rename

[Jaegeuk Kim]

This fixes wrong error report in f2fs_rename.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
Change log from v1:
 - cover rename2 case

 fs/f2fs/namei.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 98d3ab7c3ce6..48f919469151 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -833,7 +833,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct f2fs_dir_entry *old_entry;
 	struct f2fs_dir_entry *new_entry;
 	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
-	int err = -ENOENT;
+	int err;
 
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
@@ -854,6 +854,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (err)
 		goto out;
 
+	err = -ENOENT;
 	if (new_inode) {
 		err = dquot_initialize(new_inode);
 		if (err)
@@ -1049,6 +1050,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (err)
 		goto out;
 
+	err = -ENOENT;
 	old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
 	if (!old_entry) {
 		if (IS_ERR(old_page))
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev] [PATCH 1/2 v2] f2fs: report ENOENT correct in f2fs_rename

[Chao Yu]

On 2018/9/20 6:50, Jaegeuk Kim wrote:
> This fixes wrong error report in f2fs_rename.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> Change log from v1:
>  - cover rename2 case
> 
>  fs/f2fs/namei.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 98d3ab7c3ce6..48f919469151 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -833,7 +833,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	struct f2fs_dir_entry *old_entry;
>  	struct f2fs_dir_entry *new_entry;
>  	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
> -	int err = -ENOENT;
> +	int err;
>  
>  	if (unlikely(f2fs_cp_error(sbi)))
>  		return -EIO;
> @@ -854,6 +854,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (err)
>  		goto out;
>  
> +	err = -ENOENT;

It should be assigned after dquot_initialize(), otherwise, err's value can
still be reset to 0 due to dquot_initialize().

>  	if (new_inode) {
>  		err = dquot_initialize(new_inode);
>  		if (err)
> @@ -1049,6 +1050,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (err)
>  		goto out;
>  
> +	err = -ENOENT;

We can remove err initialization like f2fs_rename()

int err;

Thanks,

>  	old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
>  	if (!old_entry) {
>  		if (IS_ERR(old_page))
> 

Re: [f2fs-dev] [PATCH 1/2 v3] f2fs: report ENOENT correct in f2fs_rename

[Jaegeuk Kim]

This fixes wrong error report in f2fs_rename.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 Change log from v2:
  - remove needless err assignment
  - relocate err precisely

 fs/f2fs/namei.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 98d3ab7c3ce6..abbad9610fad 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -833,7 +833,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct f2fs_dir_entry *old_entry;
 	struct f2fs_dir_entry *new_entry;
 	bool is_old_inline = f2fs_has_inline_dentry(old_dir);
-	int err = -ENOENT;
+	int err;
 
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
@@ -860,6 +860,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			goto out;
 	}
 
+	err = -ENOENT;
 	old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
 	if (!old_entry) {
 		if (IS_ERR(old_page))
@@ -1025,7 +1026,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct f2fs_dir_entry *old_dir_entry = NULL, *new_dir_entry = NULL;
 	struct f2fs_dir_entry *old_entry, *new_entry;
 	int old_nlink = 0, new_nlink = 0;
-	int err = -ENOENT;
+	int err;
 
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
@@ -1049,6 +1050,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (err)
 		goto out;
 
+	err = -ENOENT;
 	old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
 	if (!old_entry) {
 		if (IS_ERR(old_page))
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev] [PATCH 1/2 v3] f2fs: report ENOENT correct in f2fs_rename

[Chao Yu]

On 2018/9/21 5:35, Jaegeuk Kim wrote:
> This fixes wrong error report in f2fs_rename.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

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

Thanks,