[PATCH] f2fs: fix the panic in do_checkpoint()

[PATCH] f2fs: fix the panic in do_checkpoint()

[Sahitya Tummala]

There could be a scenario where f2fs_sync_meta_pages() will not
ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
resulting in the below panic in do_checkpoint() -

f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
				!f2fs_cp_error(sbi));

This can happen in a low-memory condition, where shrinker could
also be doing the writepage operation (stack shown below)
at the same time when checkpoint is running on another core.

schedule
down_write
f2fs_submit_page_write -> by this time, this page in page cache is tagged
			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
			is cleared, due to which f2fs_sync_meta_pages()
			cannot sync this page in do_checkpoint() path.
f2fs_do_write_meta_page
__f2fs_write_meta_page
f2fs_write_meta_page
shrink_page_list
shrink_inactive_list
shrink_node_memcg
shrink_node
kswapd

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
---
 fs/f2fs/checkpoint.c | 16 ++++++++--------
 fs/f2fs/f2fs.h       |  2 +-
 fs/f2fs/super.c      |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index ffdaba0..2b651a3 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
 	f2fs_unlock_all(sbi);
 }
 
-void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
+void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
 {
 	DEFINE_WAIT(wait);
 
 	for (;;) {
 		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
 
-		if (!get_pages(sbi, F2FS_WB_CP_DATA))
+		if (!get_pages(sbi, type))
 			break;
 
 		if (unlikely(f2fs_cp_error(sbi)))
@@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* Flush all the NAT/SIT pages */
 	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
-	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
-					!f2fs_cp_error(sbi));
+	/* Wait for all dirty meta pages to be submitted for IO */
+	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
 
 	/*
 	 * modify checkpoint
@@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* Here, we have one bio having CP pack except cp pack 2 page */
 	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
-	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
-					!f2fs_cp_error(sbi));
+	/* Wait for all dirty meta pages to be submitted for IO */
+	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
 
 	/* wait for previous submitted meta pages writeback */
-	f2fs_wait_on_all_pages_writeback(sbi);
+	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
 
 	/* flush all device cache */
 	err = f2fs_flush_device_cache(sbi);
@@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* barrier and flush checkpoint cp pack 2 page if it can */
 	commit_checkpoint(sbi, ckpt, start_blk);
-	f2fs_wait_on_all_pages_writeback(sbi);
+	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
 
 	/*
 	 * invalidate intermediate page cache borrowed from meta inode
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 5a888a0..b0e0535 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
 void f2fs_update_dirty_page(struct inode *inode, struct page *page);
 void f2fs_remove_dirty_inode(struct inode *inode);
 int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
-void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
+void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
 int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
 void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
 int __init f2fs_create_checkpoint_caches(void);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 5111e1f..084633b 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
 	/* our cp_error case, we can wait for any writeback page */
 	f2fs_flush_merged_writes(sbi);
 
-	f2fs_wait_on_all_pages_writeback(sbi);
+	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
 
 	f2fs_bug_on(sbi, sbi->fsync_node_num);
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[f2fs-dev] [PATCH] f2fs: fix the panic in do_checkpoint()

[Sahitya Tummala]

There could be a scenario where f2fs_sync_meta_pages() will not
ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
resulting in the below panic in do_checkpoint() -

f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
				!f2fs_cp_error(sbi));

This can happen in a low-memory condition, where shrinker could
also be doing the writepage operation (stack shown below)
at the same time when checkpoint is running on another core.

schedule
down_write
f2fs_submit_page_write -> by this time, this page in page cache is tagged
			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
			is cleared, due to which f2fs_sync_meta_pages()
			cannot sync this page in do_checkpoint() path.
f2fs_do_write_meta_page
__f2fs_write_meta_page
f2fs_write_meta_page
shrink_page_list
shrink_inactive_list
shrink_node_memcg
shrink_node
kswapd

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
---
 fs/f2fs/checkpoint.c | 16 ++++++++--------
 fs/f2fs/f2fs.h       |  2 +-
 fs/f2fs/super.c      |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index ffdaba0..2b651a3 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
 	f2fs_unlock_all(sbi);
 }
 
-void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
+void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
 {
 	DEFINE_WAIT(wait);
 
 	for (;;) {
 		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
 
-		if (!get_pages(sbi, F2FS_WB_CP_DATA))
+		if (!get_pages(sbi, type))
 			break;
 
 		if (unlikely(f2fs_cp_error(sbi)))
@@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* Flush all the NAT/SIT pages */
 	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
-	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
-					!f2fs_cp_error(sbi));
+	/* Wait for all dirty meta pages to be submitted for IO */
+	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
 
 	/*
 	 * modify checkpoint
@@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* Here, we have one bio having CP pack except cp pack 2 page */
 	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
-	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
-					!f2fs_cp_error(sbi));
+	/* Wait for all dirty meta pages to be submitted for IO */
+	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
 
 	/* wait for previous submitted meta pages writeback */
-	f2fs_wait_on_all_pages_writeback(sbi);
+	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
 
 	/* flush all device cache */
 	err = f2fs_flush_device_cache(sbi);
@@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* barrier and flush checkpoint cp pack 2 page if it can */
 	commit_checkpoint(sbi, ckpt, start_blk);
-	f2fs_wait_on_all_pages_writeback(sbi);
+	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
 
 	/*
 	 * invalidate intermediate page cache borrowed from meta inode
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 5a888a0..b0e0535 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
 void f2fs_update_dirty_page(struct inode *inode, struct page *page);
 void f2fs_remove_dirty_inode(struct inode *inode);
 int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
-void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
+void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
 int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
 void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
 int __init f2fs_create_checkpoint_caches(void);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 5111e1f..084633b 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
 	/* our cp_error case, we can wait for any writeback page */
 	f2fs_flush_merged_writes(sbi);
 
-	f2fs_wait_on_all_pages_writeback(sbi);
+	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
 
 	f2fs_bug_on(sbi, sbi->fsync_node_num);
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: fix the panic in do_checkpoint()

[Chao Yu]

Hi Sahitya,

On 2020/2/12 18:34, Sahitya Tummala wrote:
> There could be a scenario where f2fs_sync_meta_pages() will not
> ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
> resulting in the below panic in do_checkpoint() -
> 
> f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> 				!f2fs_cp_error(sbi));
> 
> This can happen in a low-memory condition, where shrinker could
> also be doing the writepage operation (stack shown below)
> at the same time when checkpoint is running on another core.
> 
> schedule
> down_write
> f2fs_submit_page_write -> by this time, this page in page cache is tagged
> 			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
> 			is cleared, due to which f2fs_sync_meta_pages()
> 			cannot sync this page in do_checkpoint() path.
> f2fs_do_write_meta_page
> __f2fs_write_meta_page
> f2fs_write_meta_page
> shrink_page_list
> shrink_inactive_list
> shrink_node_memcg
> shrink_node
> kswapd

IMO, there may be one more simple fix here:

-	f2fs_do_write_meta_page(sbi, page, io_type);
	dec_page_count(sbi, F2FS_DIRTY_META);

+	f2fs_do_write_meta_page(sbi, page, io_type);

If we can remove F2FS_DIRTY_META reference count before we clear
PAGECACHE_TAG_DIRTY, we can avoid this race condition.

- dec_page_count(sbi, F2FS_DIRTY_META);
- f2fs_do_write_meta_page
 - set_page_writeback
  - __test_set_page_writeback
   - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);

Thoughts?

> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> ---
>  fs/f2fs/checkpoint.c | 16 ++++++++--------
>  fs/f2fs/f2fs.h       |  2 +-
>  fs/f2fs/super.c      |  2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index ffdaba0..2b651a3 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
>  	f2fs_unlock_all(sbi);
>  }
>  
> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
>  {
>  	DEFINE_WAIT(wait);
>  
>  	for (;;) {
>  		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
>  
> -		if (!get_pages(sbi, F2FS_WB_CP_DATA))
> +		if (!get_pages(sbi, type))
>  			break;
>  
>  		if (unlikely(f2fs_cp_error(sbi)))
> @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  
>  	/* Flush all the NAT/SIT pages */
>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> -					!f2fs_cp_error(sbi));
> +	/* Wait for all dirty meta pages to be submitted for IO */
> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);

I'm afraid calling f2fs_wait_on_all_pages() after we call
f2fs_sync_meta_pages() is low efficient, as we only want to write out
dirty meta pages instead of wait for writebacking them to device cache.

Thanks,

>  
>  	/*
>  	 * modify checkpoint
> @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  
>  	/* Here, we have one bio having CP pack except cp pack 2 page */
>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> -					!f2fs_cp_error(sbi));
> +	/* Wait for all dirty meta pages to be submitted for IO */
> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>  
>  	/* wait for previous submitted meta pages writeback */
> -	f2fs_wait_on_all_pages_writeback(sbi);
> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>  
>  	/* flush all device cache */
>  	err = f2fs_flush_device_cache(sbi);
> @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  
>  	/* barrier and flush checkpoint cp pack 2 page if it can */
>  	commit_checkpoint(sbi, ckpt, start_blk);
> -	f2fs_wait_on_all_pages_writeback(sbi);
> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>  
>  	/*
>  	 * invalidate intermediate page cache borrowed from meta inode
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 5a888a0..b0e0535 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
>  void f2fs_update_dirty_page(struct inode *inode, struct page *page);
>  void f2fs_remove_dirty_inode(struct inode *inode);
>  int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
>  int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>  void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
>  int __init f2fs_create_checkpoint_caches(void);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 5111e1f..084633b 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
>  	/* our cp_error case, we can wait for any writeback page */
>  	f2fs_flush_merged_writes(sbi);
>  
> -	f2fs_wait_on_all_pages_writeback(sbi);
> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>  
>  	f2fs_bug_on(sbi, sbi->fsync_node_num);
>  
> 

Re: [f2fs-dev] [PATCH] f2fs: fix the panic in do_checkpoint()

[Chao Yu]

Hi Sahitya,

On 2020/2/12 18:34, Sahitya Tummala wrote:
> There could be a scenario where f2fs_sync_meta_pages() will not
> ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
> resulting in the below panic in do_checkpoint() -
> 
> f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> 				!f2fs_cp_error(sbi));
> 
> This can happen in a low-memory condition, where shrinker could
> also be doing the writepage operation (stack shown below)
> at the same time when checkpoint is running on another core.
> 
> schedule
> down_write
> f2fs_submit_page_write -> by this time, this page in page cache is tagged
> 			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
> 			is cleared, due to which f2fs_sync_meta_pages()
> 			cannot sync this page in do_checkpoint() path.
> f2fs_do_write_meta_page
> __f2fs_write_meta_page
> f2fs_write_meta_page
> shrink_page_list
> shrink_inactive_list
> shrink_node_memcg
> shrink_node
> kswapd

IMO, there may be one more simple fix here:

-	f2fs_do_write_meta_page(sbi, page, io_type);
	dec_page_count(sbi, F2FS_DIRTY_META);

+	f2fs_do_write_meta_page(sbi, page, io_type);

If we can remove F2FS_DIRTY_META reference count before we clear
PAGECACHE_TAG_DIRTY, we can avoid this race condition.

- dec_page_count(sbi, F2FS_DIRTY_META);
- f2fs_do_write_meta_page
 - set_page_writeback
  - __test_set_page_writeback
   - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);

Thoughts?

> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> ---
>  fs/f2fs/checkpoint.c | 16 ++++++++--------
>  fs/f2fs/f2fs.h       |  2 +-
>  fs/f2fs/super.c      |  2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index ffdaba0..2b651a3 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
>  	f2fs_unlock_all(sbi);
>  }
>  
> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
>  {
>  	DEFINE_WAIT(wait);
>  
>  	for (;;) {
>  		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
>  
> -		if (!get_pages(sbi, F2FS_WB_CP_DATA))
> +		if (!get_pages(sbi, type))
>  			break;
>  
>  		if (unlikely(f2fs_cp_error(sbi)))
> @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  
>  	/* Flush all the NAT/SIT pages */
>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> -					!f2fs_cp_error(sbi));
> +	/* Wait for all dirty meta pages to be submitted for IO */
> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);

I'm afraid calling f2fs_wait_on_all_pages() after we call
f2fs_sync_meta_pages() is low efficient, as we only want to write out
dirty meta pages instead of wait for writebacking them to device cache.

Thanks,

>  
>  	/*
>  	 * modify checkpoint
> @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  
>  	/* Here, we have one bio having CP pack except cp pack 2 page */
>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> -					!f2fs_cp_error(sbi));
> +	/* Wait for all dirty meta pages to be submitted for IO */
> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>  
>  	/* wait for previous submitted meta pages writeback */
> -	f2fs_wait_on_all_pages_writeback(sbi);
> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>  
>  	/* flush all device cache */
>  	err = f2fs_flush_device_cache(sbi);
> @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  
>  	/* barrier and flush checkpoint cp pack 2 page if it can */
>  	commit_checkpoint(sbi, ckpt, start_blk);
> -	f2fs_wait_on_all_pages_writeback(sbi);
> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>  
>  	/*
>  	 * invalidate intermediate page cache borrowed from meta inode
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 5a888a0..b0e0535 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
>  void f2fs_update_dirty_page(struct inode *inode, struct page *page);
>  void f2fs_remove_dirty_inode(struct inode *inode);
>  int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
>  int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>  void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
>  int __init f2fs_create_checkpoint_caches(void);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 5111e1f..084633b 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
>  	/* our cp_error case, we can wait for any writeback page */
>  	f2fs_flush_merged_writes(sbi);
>  
> -	f2fs_wait_on_all_pages_writeback(sbi);
> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>  
>  	f2fs_bug_on(sbi, sbi->fsync_node_num);
>  
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: fix the panic in do_checkpoint()

[Sahitya Tummala]

Hi Chao,

On Fri, Feb 14, 2020 at 10:26:18AM +0800, Chao Yu wrote:
> Hi Sahitya,
> 
> On 2020/2/12 18:34, Sahitya Tummala wrote:
> > There could be a scenario where f2fs_sync_meta_pages() will not
> > ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
> > resulting in the below panic in do_checkpoint() -
> > 
> > f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> > 				!f2fs_cp_error(sbi));
> > 
> > This can happen in a low-memory condition, where shrinker could
> > also be doing the writepage operation (stack shown below)
> > at the same time when checkpoint is running on another core.
> > 
> > schedule
> > down_write
> > f2fs_submit_page_write -> by this time, this page in page cache is tagged
> > 			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
> > 			is cleared, due to which f2fs_sync_meta_pages()
> > 			cannot sync this page in do_checkpoint() path.
> > f2fs_do_write_meta_page
> > __f2fs_write_meta_page
> > f2fs_write_meta_page
> > shrink_page_list
> > shrink_inactive_list
> > shrink_node_memcg
> > shrink_node
> > kswapd
> 
> IMO, there may be one more simple fix here:
> 
> -	f2fs_do_write_meta_page(sbi, page, io_type);
> 	dec_page_count(sbi, F2FS_DIRTY_META);
> 
> +	f2fs_do_write_meta_page(sbi, page, io_type);
> 
> If we can remove F2FS_DIRTY_META reference count before we clear
> PAGECACHE_TAG_DIRTY, we can avoid this race condition.
> 
> - dec_page_count(sbi, F2FS_DIRTY_META);
> - f2fs_do_write_meta_page
>  - set_page_writeback
>   - __test_set_page_writeback
>    - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
> 
> Thoughts?

I believe it will be a problem because let's say the shrinker path is 
still in the below stack -

f2fs_submit_page_write();
->down_write(&io->io_rwsem);

Then, the current f2fs_wait_on_all_pages_writeback() will not wait for
that page as it is not yet tagged as F2FS_WB_CP_DATA. Hence, the checkpoint
may proceed without waiting for this meta page to be written to disk.

> 
> > 
> > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > ---
> >  fs/f2fs/checkpoint.c | 16 ++++++++--------
> >  fs/f2fs/f2fs.h       |  2 +-
> >  fs/f2fs/super.c      |  2 +-
> >  3 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index ffdaba0..2b651a3 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
> >  	f2fs_unlock_all(sbi);
> >  }
> >  
> > -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
> > +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
> >  {
> >  	DEFINE_WAIT(wait);
> >  
> >  	for (;;) {
> >  		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
> >  
> > -		if (!get_pages(sbi, F2FS_WB_CP_DATA))
> > +		if (!get_pages(sbi, type))
> >  			break;
> >  
> >  		if (unlikely(f2fs_cp_error(sbi)))
> > @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  
> >  	/* Flush all the NAT/SIT pages */
> >  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> > -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> > -					!f2fs_cp_error(sbi));
> > +	/* Wait for all dirty meta pages to be submitted for IO */
> > +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
> 
> I'm afraid calling f2fs_wait_on_all_pages() after we call
> f2fs_sync_meta_pages() is low efficient, as we only want to write out
> dirty meta pages instead of wait for writebacking them to device cache.
> 

I have modified the existing function f2fs_wait_on_all_pages_writeback() to
a generic one f2fs_wait_on_all_pages(), where it will wait according to the
requested type. In this case, it will only wait for dirty F2FS_DIRTY_META pages
but not for the writeback to device cache.

Thanks,

> Thanks,
> 
> >  
> >  	/*
> >  	 * modify checkpoint
> > @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  
> >  	/* Here, we have one bio having CP pack except cp pack 2 page */
> >  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> > -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> > -					!f2fs_cp_error(sbi));
> > +	/* Wait for all dirty meta pages to be submitted for IO */
> > +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
> >  
> >  	/* wait for previous submitted meta pages writeback */
> > -	f2fs_wait_on_all_pages_writeback(sbi);
> > +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >  
> >  	/* flush all device cache */
> >  	err = f2fs_flush_device_cache(sbi);
> > @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  
> >  	/* barrier and flush checkpoint cp pack 2 page if it can */
> >  	commit_checkpoint(sbi, ckpt, start_blk);
> > -	f2fs_wait_on_all_pages_writeback(sbi);
> > +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >  
> >  	/*
> >  	 * invalidate intermediate page cache borrowed from meta inode
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 5a888a0..b0e0535 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
> >  void f2fs_update_dirty_page(struct inode *inode, struct page *page);
> >  void f2fs_remove_dirty_inode(struct inode *inode);
> >  int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
> > -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
> > +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
> >  int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >  void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
> >  int __init f2fs_create_checkpoint_caches(void);
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 5111e1f..084633b 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
> >  	/* our cp_error case, we can wait for any writeback page */
> >  	f2fs_flush_merged_writes(sbi);
> >  
> > -	f2fs_wait_on_all_pages_writeback(sbi);
> > +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >  
> >  	f2fs_bug_on(sbi, sbi->fsync_node_num);
> >  
> > 

-- 
--
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] f2fs: fix the panic in do_checkpoint()

[Chao Yu]

Hi Sahitya,

On 2020/2/14 11:54, Sahitya Tummala wrote:
> Hi Chao,
> 
> On Fri, Feb 14, 2020 at 10:26:18AM +0800, Chao Yu wrote:
>> Hi Sahitya,
>>
>> On 2020/2/12 18:34, Sahitya Tummala wrote:
>>> There could be a scenario where f2fs_sync_meta_pages() will not
>>> ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
>>> resulting in the below panic in do_checkpoint() -
>>>
>>> f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>> 				!f2fs_cp_error(sbi));
>>>
>>> This can happen in a low-memory condition, where shrinker could
>>> also be doing the writepage operation (stack shown below)
>>> at the same time when checkpoint is running on another core.
>>>
>>> schedule
>>> down_write
>>> f2fs_submit_page_write -> by this time, this page in page cache is tagged
>>> 			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
>>> 			is cleared, due to which f2fs_sync_meta_pages()
>>> 			cannot sync this page in do_checkpoint() path.
>>> f2fs_do_write_meta_page
>>> __f2fs_write_meta_page
>>> f2fs_write_meta_page
>>> shrink_page_list
>>> shrink_inactive_list
>>> shrink_node_memcg
>>> shrink_node
>>> kswapd
>>
>> IMO, there may be one more simple fix here:
>>
>> -	f2fs_do_write_meta_page(sbi, page, io_type);
>> 	dec_page_count(sbi, F2FS_DIRTY_META);
>>
>> +	f2fs_do_write_meta_page(sbi, page, io_type);
>>
>> If we can remove F2FS_DIRTY_META reference count before we clear
>> PAGECACHE_TAG_DIRTY, we can avoid this race condition.
>>
>> - dec_page_count(sbi, F2FS_DIRTY_META);
>> - f2fs_do_write_meta_page
>>  - set_page_writeback
>>   - __test_set_page_writeback
>>    - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
>>
>> Thoughts?
> 
> I believe it will be a problem because let's say the shrinker path is 
> still in the below stack -
> 
> f2fs_submit_page_write();
> ->down_write(&io->io_rwsem);
> 
> Then, the current f2fs_wait_on_all_pages_writeback() will not wait for
> that page as it is not yet tagged as F2FS_WB_CP_DATA. Hence, the checkpoint
> may proceed without waiting for this meta page to be written to disk.

Oh, you're right.

It looks the race can happen for data/node pages? like in fsync() procedure.

> 
>>
>>>
>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> ---
>>>  fs/f2fs/checkpoint.c | 16 ++++++++--------
>>>  fs/f2fs/f2fs.h       |  2 +-
>>>  fs/f2fs/super.c      |  2 +-
>>>  3 files changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index ffdaba0..2b651a3 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
>>>  	f2fs_unlock_all(sbi);
>>>  }
>>>  
>>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
>>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
>>>  {
>>>  	DEFINE_WAIT(wait);
>>>  
>>>  	for (;;) {
>>>  		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
>>>  
>>> -		if (!get_pages(sbi, F2FS_WB_CP_DATA))
>>> +		if (!get_pages(sbi, type))
>>>  			break;
>>>  
>>>  		if (unlikely(f2fs_cp_error(sbi)))
>>> @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  
>>>  	/* Flush all the NAT/SIT pages */
>>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>> -					!f2fs_cp_error(sbi));
>>> +	/* Wait for all dirty meta pages to be submitted for IO */
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>>
>> I'm afraid calling f2fs_wait_on_all_pages() after we call
>> f2fs_sync_meta_pages() is low efficient, as we only want to write out
>> dirty meta pages instead of wait for writebacking them to device cache.
>>
> 
> I have modified the existing function f2fs_wait_on_all_pages_writeback() to
> a generic one f2fs_wait_on_all_pages(), where it will wait according to the
> requested type. In this case, it will only wait for dirty F2FS_DIRTY_META pages
> but not for the writeback to device cache.

Oh, I see, as last dirty reference count decreaser won't wake up waiter, we need
to wait for timeout, right? Can we decrease timeout count, maybe HZ/50 as we did
for congestion.

io_schedule_timeout(5*HZ);

Thanks,

> 
> Thanks,
> 
>> Thanks,
>>
>>>  
>>>  	/*
>>>  	 * modify checkpoint
>>> @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  
>>>  	/* Here, we have one bio having CP pack except cp pack 2 page */
>>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>> -					!f2fs_cp_error(sbi));
>>> +	/* Wait for all dirty meta pages to be submitted for IO */
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>>>  
>>>  	/* wait for previous submitted meta pages writeback */
>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>  
>>>  	/* flush all device cache */
>>>  	err = f2fs_flush_device_cache(sbi);
>>> @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  
>>>  	/* barrier and flush checkpoint cp pack 2 page if it can */
>>>  	commit_checkpoint(sbi, ckpt, start_blk);
>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>  
>>>  	/*
>>>  	 * invalidate intermediate page cache borrowed from meta inode
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 5a888a0..b0e0535 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
>>>  void f2fs_update_dirty_page(struct inode *inode, struct page *page);
>>>  void f2fs_remove_dirty_inode(struct inode *inode);
>>>  int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
>>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
>>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
>>>  int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>  void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
>>>  int __init f2fs_create_checkpoint_caches(void);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 5111e1f..084633b 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
>>>  	/* our cp_error case, we can wait for any writeback page */
>>>  	f2fs_flush_merged_writes(sbi);
>>>  
>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>  
>>>  	f2fs_bug_on(sbi, sbi->fsync_node_num);
>>>  
>>>
> 

Re: [f2fs-dev] [PATCH] f2fs: fix the panic in do_checkpoint()

[Chao Yu]

Hi Sahitya,

On 2020/2/14 11:54, Sahitya Tummala wrote:
> Hi Chao,
> 
> On Fri, Feb 14, 2020 at 10:26:18AM +0800, Chao Yu wrote:
>> Hi Sahitya,
>>
>> On 2020/2/12 18:34, Sahitya Tummala wrote:
>>> There could be a scenario where f2fs_sync_meta_pages() will not
>>> ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
>>> resulting in the below panic in do_checkpoint() -
>>>
>>> f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>> 				!f2fs_cp_error(sbi));
>>>
>>> This can happen in a low-memory condition, where shrinker could
>>> also be doing the writepage operation (stack shown below)
>>> at the same time when checkpoint is running on another core.
>>>
>>> schedule
>>> down_write
>>> f2fs_submit_page_write -> by this time, this page in page cache is tagged
>>> 			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
>>> 			is cleared, due to which f2fs_sync_meta_pages()
>>> 			cannot sync this page in do_checkpoint() path.
>>> f2fs_do_write_meta_page
>>> __f2fs_write_meta_page
>>> f2fs_write_meta_page
>>> shrink_page_list
>>> shrink_inactive_list
>>> shrink_node_memcg
>>> shrink_node
>>> kswapd
>>
>> IMO, there may be one more simple fix here:
>>
>> -	f2fs_do_write_meta_page(sbi, page, io_type);
>> 	dec_page_count(sbi, F2FS_DIRTY_META);
>>
>> +	f2fs_do_write_meta_page(sbi, page, io_type);
>>
>> If we can remove F2FS_DIRTY_META reference count before we clear
>> PAGECACHE_TAG_DIRTY, we can avoid this race condition.
>>
>> - dec_page_count(sbi, F2FS_DIRTY_META);
>> - f2fs_do_write_meta_page
>>  - set_page_writeback
>>   - __test_set_page_writeback
>>    - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
>>
>> Thoughts?
> 
> I believe it will be a problem because let's say the shrinker path is 
> still in the below stack -
> 
> f2fs_submit_page_write();
> ->down_write(&io->io_rwsem);
> 
> Then, the current f2fs_wait_on_all_pages_writeback() will not wait for
> that page as it is not yet tagged as F2FS_WB_CP_DATA. Hence, the checkpoint
> may proceed without waiting for this meta page to be written to disk.

Oh, you're right.

It looks the race can happen for data/node pages? like in fsync() procedure.

> 
>>
>>>
>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> ---
>>>  fs/f2fs/checkpoint.c | 16 ++++++++--------
>>>  fs/f2fs/f2fs.h       |  2 +-
>>>  fs/f2fs/super.c      |  2 +-
>>>  3 files changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index ffdaba0..2b651a3 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
>>>  	f2fs_unlock_all(sbi);
>>>  }
>>>  
>>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
>>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
>>>  {
>>>  	DEFINE_WAIT(wait);
>>>  
>>>  	for (;;) {
>>>  		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
>>>  
>>> -		if (!get_pages(sbi, F2FS_WB_CP_DATA))
>>> +		if (!get_pages(sbi, type))
>>>  			break;
>>>  
>>>  		if (unlikely(f2fs_cp_error(sbi)))
>>> @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  
>>>  	/* Flush all the NAT/SIT pages */
>>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>> -					!f2fs_cp_error(sbi));
>>> +	/* Wait for all dirty meta pages to be submitted for IO */
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>>
>> I'm afraid calling f2fs_wait_on_all_pages() after we call
>> f2fs_sync_meta_pages() is low efficient, as we only want to write out
>> dirty meta pages instead of wait for writebacking them to device cache.
>>
> 
> I have modified the existing function f2fs_wait_on_all_pages_writeback() to
> a generic one f2fs_wait_on_all_pages(), where it will wait according to the
> requested type. In this case, it will only wait for dirty F2FS_DIRTY_META pages
> but not for the writeback to device cache.

Oh, I see, as last dirty reference count decreaser won't wake up waiter, we need
to wait for timeout, right? Can we decrease timeout count, maybe HZ/50 as we did
for congestion.

io_schedule_timeout(5*HZ);

Thanks,

> 
> Thanks,
> 
>> Thanks,
>>
>>>  
>>>  	/*
>>>  	 * modify checkpoint
>>> @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  
>>>  	/* Here, we have one bio having CP pack except cp pack 2 page */
>>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>> -					!f2fs_cp_error(sbi));
>>> +	/* Wait for all dirty meta pages to be submitted for IO */
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>>>  
>>>  	/* wait for previous submitted meta pages writeback */
>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>  
>>>  	/* flush all device cache */
>>>  	err = f2fs_flush_device_cache(sbi);
>>> @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  
>>>  	/* barrier and flush checkpoint cp pack 2 page if it can */
>>>  	commit_checkpoint(sbi, ckpt, start_blk);
>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>  
>>>  	/*
>>>  	 * invalidate intermediate page cache borrowed from meta inode
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 5a888a0..b0e0535 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
>>>  void f2fs_update_dirty_page(struct inode *inode, struct page *page);
>>>  void f2fs_remove_dirty_inode(struct inode *inode);
>>>  int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
>>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
>>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
>>>  int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>  void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
>>>  int __init f2fs_create_checkpoint_caches(void);
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 5111e1f..084633b 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
>>>  	/* our cp_error case, we can wait for any writeback page */
>>>  	f2fs_flush_merged_writes(sbi);
>>>  
>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>  
>>>  	f2fs_bug_on(sbi, sbi->fsync_node_num);
>>>  
>>>
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: fix the panic in do_checkpoint()

[Chao Yu]

On 2020/2/14 15:16, Chao Yu wrote:
> Hi Sahitya,
> 
> On 2020/2/14 11:54, Sahitya Tummala wrote:
>> Hi Chao,
>>
>> On Fri, Feb 14, 2020 at 10:26:18AM +0800, Chao Yu wrote:
>>> Hi Sahitya,
>>>
>>> On 2020/2/12 18:34, Sahitya Tummala wrote:
>>>> There could be a scenario where f2fs_sync_meta_pages() will not
>>>> ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
>>>> resulting in the below panic in do_checkpoint() -
>>>>
>>>> f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>>> 				!f2fs_cp_error(sbi));
>>>>
>>>> This can happen in a low-memory condition, where shrinker could
>>>> also be doing the writepage operation (stack shown below)
>>>> at the same time when checkpoint is running on another core.
>>>>
>>>> schedule
>>>> down_write
>>>> f2fs_submit_page_write -> by this time, this page in page cache is tagged
>>>> 			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
>>>> 			is cleared, due to which f2fs_sync_meta_pages()
>>>> 			cannot sync this page in do_checkpoint() path.
>>>> f2fs_do_write_meta_page
>>>> __f2fs_write_meta_page
>>>> f2fs_write_meta_page
>>>> shrink_page_list
>>>> shrink_inactive_list
>>>> shrink_node_memcg
>>>> shrink_node
>>>> kswapd
>>>
>>> IMO, there may be one more simple fix here:
>>>
>>> -	f2fs_do_write_meta_page(sbi, page, io_type);
>>> 	dec_page_count(sbi, F2FS_DIRTY_META);
>>>
>>> +	f2fs_do_write_meta_page(sbi, page, io_type);
>>>
>>> If we can remove F2FS_DIRTY_META reference count before we clear
>>> PAGECACHE_TAG_DIRTY, we can avoid this race condition.
>>>
>>> - dec_page_count(sbi, F2FS_DIRTY_META);
>>> - f2fs_do_write_meta_page
>>>  - set_page_writeback
>>>   - __test_set_page_writeback
>>>    - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
>>>
>>> Thoughts?
>>
>> I believe it will be a problem because let's say the shrinker path is 
>> still in the below stack -
>>
>> f2fs_submit_page_write();
>> ->down_write(&io->io_rwsem);
>>
>> Then, the current f2fs_wait_on_all_pages_writeback() will not wait for
>> that page as it is not yet tagged as F2FS_WB_CP_DATA. Hence, the checkpoint
>> may proceed without waiting for this meta page to be written to disk.
> 
> Oh, you're right.
> 
> It looks the race can happen for data/node pages? like in fsync() procedure.

I mean something like this:

- fsync()				- shrink
 - f2fs_do_sync_file
					 - __write_node_page
					  - set_page_writeback(page#0)
					  : remove DIRTY/TOWRITE flag
  - f2fs_fsync_node_pages
  : won't find page #0 as TOWRITE flag was removeD
  - f2fs_wait_on_node_pages_writeback
  : wont' wait page #0 writeback as it was not in fsync_node_list list.
					   - f2fs_add_fsync_node_entry

Thanks,

> 
>>
>>>
>>>>
>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>> ---
>>>>  fs/f2fs/checkpoint.c | 16 ++++++++--------
>>>>  fs/f2fs/f2fs.h       |  2 +-
>>>>  fs/f2fs/super.c      |  2 +-
>>>>  3 files changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>> index ffdaba0..2b651a3 100644
>>>> --- a/fs/f2fs/checkpoint.c
>>>> +++ b/fs/f2fs/checkpoint.c
>>>> @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
>>>>  	f2fs_unlock_all(sbi);
>>>>  }
>>>>  
>>>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
>>>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
>>>>  {
>>>>  	DEFINE_WAIT(wait);
>>>>  
>>>>  	for (;;) {
>>>>  		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
>>>>  
>>>> -		if (!get_pages(sbi, F2FS_WB_CP_DATA))
>>>> +		if (!get_pages(sbi, type))
>>>>  			break;
>>>>  
>>>>  		if (unlikely(f2fs_cp_error(sbi)))
>>>> @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>  
>>>>  	/* Flush all the NAT/SIT pages */
>>>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>>> -					!f2fs_cp_error(sbi));
>>>> +	/* Wait for all dirty meta pages to be submitted for IO */
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>>>
>>> I'm afraid calling f2fs_wait_on_all_pages() after we call
>>> f2fs_sync_meta_pages() is low efficient, as we only want to write out
>>> dirty meta pages instead of wait for writebacking them to device cache.
>>>
>>
>> I have modified the existing function f2fs_wait_on_all_pages_writeback() to
>> a generic one f2fs_wait_on_all_pages(), where it will wait according to the
>> requested type. In this case, it will only wait for dirty F2FS_DIRTY_META pages
>> but not for the writeback to device cache.
> 
> Oh, I see, as last dirty reference count decreaser won't wake up waiter, we need
> to wait for timeout, right? Can we decrease timeout count, maybe HZ/50 as we did
> for congestion.
> 
> io_schedule_timeout(5*HZ);
> 
> Thanks,
> 
>>
>> Thanks,
>>
>>> Thanks,
>>>
>>>>  
>>>>  	/*
>>>>  	 * modify checkpoint
>>>> @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>  
>>>>  	/* Here, we have one bio having CP pack except cp pack 2 page */
>>>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>>> -					!f2fs_cp_error(sbi));
>>>> +	/* Wait for all dirty meta pages to be submitted for IO */
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>>>>  
>>>>  	/* wait for previous submitted meta pages writeback */
>>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>>  
>>>>  	/* flush all device cache */
>>>>  	err = f2fs_flush_device_cache(sbi);
>>>> @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>  
>>>>  	/* barrier and flush checkpoint cp pack 2 page if it can */
>>>>  	commit_checkpoint(sbi, ckpt, start_blk);
>>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>>  
>>>>  	/*
>>>>  	 * invalidate intermediate page cache borrowed from meta inode
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index 5a888a0..b0e0535 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
>>>>  void f2fs_update_dirty_page(struct inode *inode, struct page *page);
>>>>  void f2fs_remove_dirty_inode(struct inode *inode);
>>>>  int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
>>>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
>>>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
>>>>  int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>  void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
>>>>  int __init f2fs_create_checkpoint_caches(void);
>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>> index 5111e1f..084633b 100644
>>>> --- a/fs/f2fs/super.c
>>>> +++ b/fs/f2fs/super.c
>>>> @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
>>>>  	/* our cp_error case, we can wait for any writeback page */
>>>>  	f2fs_flush_merged_writes(sbi);
>>>>  
>>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>>  
>>>>  	f2fs_bug_on(sbi, sbi->fsync_node_num);
>>>>  
>>>>
>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: fix the panic in do_checkpoint()

[Chao Yu]

On 2020/2/14 15:16, Chao Yu wrote:
> Hi Sahitya,
> 
> On 2020/2/14 11:54, Sahitya Tummala wrote:
>> Hi Chao,
>>
>> On Fri, Feb 14, 2020 at 10:26:18AM +0800, Chao Yu wrote:
>>> Hi Sahitya,
>>>
>>> On 2020/2/12 18:34, Sahitya Tummala wrote:
>>>> There could be a scenario where f2fs_sync_meta_pages() will not
>>>> ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
>>>> resulting in the below panic in do_checkpoint() -
>>>>
>>>> f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>>> 				!f2fs_cp_error(sbi));
>>>>
>>>> This can happen in a low-memory condition, where shrinker could
>>>> also be doing the writepage operation (stack shown below)
>>>> at the same time when checkpoint is running on another core.
>>>>
>>>> schedule
>>>> down_write
>>>> f2fs_submit_page_write -> by this time, this page in page cache is tagged
>>>> 			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
>>>> 			is cleared, due to which f2fs_sync_meta_pages()
>>>> 			cannot sync this page in do_checkpoint() path.
>>>> f2fs_do_write_meta_page
>>>> __f2fs_write_meta_page
>>>> f2fs_write_meta_page
>>>> shrink_page_list
>>>> shrink_inactive_list
>>>> shrink_node_memcg
>>>> shrink_node
>>>> kswapd
>>>
>>> IMO, there may be one more simple fix here:
>>>
>>> -	f2fs_do_write_meta_page(sbi, page, io_type);
>>> 	dec_page_count(sbi, F2FS_DIRTY_META);
>>>
>>> +	f2fs_do_write_meta_page(sbi, page, io_type);
>>>
>>> If we can remove F2FS_DIRTY_META reference count before we clear
>>> PAGECACHE_TAG_DIRTY, we can avoid this race condition.
>>>
>>> - dec_page_count(sbi, F2FS_DIRTY_META);
>>> - f2fs_do_write_meta_page
>>>  - set_page_writeback
>>>   - __test_set_page_writeback
>>>    - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
>>>
>>> Thoughts?
>>
>> I believe it will be a problem because let's say the shrinker path is 
>> still in the below stack -
>>
>> f2fs_submit_page_write();
>> ->down_write(&io->io_rwsem);
>>
>> Then, the current f2fs_wait_on_all_pages_writeback() will not wait for
>> that page as it is not yet tagged as F2FS_WB_CP_DATA. Hence, the checkpoint
>> may proceed without waiting for this meta page to be written to disk.
> 
> Oh, you're right.
> 
> It looks the race can happen for data/node pages? like in fsync() procedure.

I mean something like this:

- fsync()				- shrink
 - f2fs_do_sync_file
					 - __write_node_page
					  - set_page_writeback(page#0)
					  : remove DIRTY/TOWRITE flag
  - f2fs_fsync_node_pages
  : won't find page #0 as TOWRITE flag was removeD
  - f2fs_wait_on_node_pages_writeback
  : wont' wait page #0 writeback as it was not in fsync_node_list list.
					   - f2fs_add_fsync_node_entry

Thanks,

> 
>>
>>>
>>>>
>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>> ---
>>>>  fs/f2fs/checkpoint.c | 16 ++++++++--------
>>>>  fs/f2fs/f2fs.h       |  2 +-
>>>>  fs/f2fs/super.c      |  2 +-
>>>>  3 files changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>> index ffdaba0..2b651a3 100644
>>>> --- a/fs/f2fs/checkpoint.c
>>>> +++ b/fs/f2fs/checkpoint.c
>>>> @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
>>>>  	f2fs_unlock_all(sbi);
>>>>  }
>>>>  
>>>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
>>>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
>>>>  {
>>>>  	DEFINE_WAIT(wait);
>>>>  
>>>>  	for (;;) {
>>>>  		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
>>>>  
>>>> -		if (!get_pages(sbi, F2FS_WB_CP_DATA))
>>>> +		if (!get_pages(sbi, type))
>>>>  			break;
>>>>  
>>>>  		if (unlikely(f2fs_cp_error(sbi)))
>>>> @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>  
>>>>  	/* Flush all the NAT/SIT pages */
>>>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>>> -					!f2fs_cp_error(sbi));
>>>> +	/* Wait for all dirty meta pages to be submitted for IO */
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>>>
>>> I'm afraid calling f2fs_wait_on_all_pages() after we call
>>> f2fs_sync_meta_pages() is low efficient, as we only want to write out
>>> dirty meta pages instead of wait for writebacking them to device cache.
>>>
>>
>> I have modified the existing function f2fs_wait_on_all_pages_writeback() to
>> a generic one f2fs_wait_on_all_pages(), where it will wait according to the
>> requested type. In this case, it will only wait for dirty F2FS_DIRTY_META pages
>> but not for the writeback to device cache.
> 
> Oh, I see, as last dirty reference count decreaser won't wake up waiter, we need
> to wait for timeout, right? Can we decrease timeout count, maybe HZ/50 as we did
> for congestion.
> 
> io_schedule_timeout(5*HZ);
> 
> Thanks,
> 
>>
>> Thanks,
>>
>>> Thanks,
>>>
>>>>  
>>>>  	/*
>>>>  	 * modify checkpoint
>>>> @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>  
>>>>  	/* Here, we have one bio having CP pack except cp pack 2 page */
>>>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
>>>> -					!f2fs_cp_error(sbi));
>>>> +	/* Wait for all dirty meta pages to be submitted for IO */
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
>>>>  
>>>>  	/* wait for previous submitted meta pages writeback */
>>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>>  
>>>>  	/* flush all device cache */
>>>>  	err = f2fs_flush_device_cache(sbi);
>>>> @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>  
>>>>  	/* barrier and flush checkpoint cp pack 2 page if it can */
>>>>  	commit_checkpoint(sbi, ckpt, start_blk);
>>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>>  
>>>>  	/*
>>>>  	 * invalidate intermediate page cache borrowed from meta inode
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index 5a888a0..b0e0535 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
>>>>  void f2fs_update_dirty_page(struct inode *inode, struct page *page);
>>>>  void f2fs_remove_dirty_inode(struct inode *inode);
>>>>  int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
>>>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
>>>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
>>>>  int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
>>>>  void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
>>>>  int __init f2fs_create_checkpoint_caches(void);
>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>> index 5111e1f..084633b 100644
>>>> --- a/fs/f2fs/super.c
>>>> +++ b/fs/f2fs/super.c
>>>> @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
>>>>  	/* our cp_error case, we can wait for any writeback page */
>>>>  	f2fs_flush_merged_writes(sbi);
>>>>  
>>>> -	f2fs_wait_on_all_pages_writeback(sbi);
>>>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
>>>>  
>>>>  	f2fs_bug_on(sbi, sbi->fsync_node_num);
>>>>  
>>>>
>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: fix the panic in do_checkpoint()

[Sahitya Tummala]

On Fri, Feb 14, 2020 at 03:16:59PM +0800, Chao Yu wrote:
> Hi Sahitya,
> 
> On 2020/2/14 11:54, Sahitya Tummala wrote:
> > Hi Chao,
> > 
> > On Fri, Feb 14, 2020 at 10:26:18AM +0800, Chao Yu wrote:
> >> Hi Sahitya,
> >>
> >> On 2020/2/12 18:34, Sahitya Tummala wrote:
> >>> There could be a scenario where f2fs_sync_meta_pages() will not
> >>> ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
> >>> resulting in the below panic in do_checkpoint() -
> >>>
> >>> f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> >>> 				!f2fs_cp_error(sbi));
> >>>
> >>> This can happen in a low-memory condition, where shrinker could
> >>> also be doing the writepage operation (stack shown below)
> >>> at the same time when checkpoint is running on another core.
> >>>
> >>> schedule
> >>> down_write
> >>> f2fs_submit_page_write -> by this time, this page in page cache is tagged
> >>> 			as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
> >>> 			is cleared, due to which f2fs_sync_meta_pages()
> >>> 			cannot sync this page in do_checkpoint() path.
> >>> f2fs_do_write_meta_page
> >>> __f2fs_write_meta_page
> >>> f2fs_write_meta_page
> >>> shrink_page_list
> >>> shrink_inactive_list
> >>> shrink_node_memcg
> >>> shrink_node
> >>> kswapd
> >>
> >> IMO, there may be one more simple fix here:
> >>
> >> -	f2fs_do_write_meta_page(sbi, page, io_type);
> >> 	dec_page_count(sbi, F2FS_DIRTY_META);
> >>
> >> +	f2fs_do_write_meta_page(sbi, page, io_type);
> >>
> >> If we can remove F2FS_DIRTY_META reference count before we clear
> >> PAGECACHE_TAG_DIRTY, we can avoid this race condition.
> >>
> >> - dec_page_count(sbi, F2FS_DIRTY_META);
> >> - f2fs_do_write_meta_page
> >>  - set_page_writeback
> >>   - __test_set_page_writeback
> >>    - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
> >>
> >> Thoughts?
> > 
> > I believe it will be a problem because let's say the shrinker path is 
> > still in the below stack -
> > 
> > f2fs_submit_page_write();
> > ->down_write(&io->io_rwsem);
> > 
> > Then, the current f2fs_wait_on_all_pages_writeback() will not wait for
> > that page as it is not yet tagged as F2FS_WB_CP_DATA. Hence, the checkpoint
> > may proceed without waiting for this meta page to be written to disk.
> 
> Oh, you're right.
> 
> It looks the race can happen for data/node pages? like in fsync() procedure.
> 
Yes, it can happen. I see that you have already put a patch to fix that :).

> > 
> >>
> >>>
> >>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> >>> ---
> >>>  fs/f2fs/checkpoint.c | 16 ++++++++--------
> >>>  fs/f2fs/f2fs.h       |  2 +-
> >>>  fs/f2fs/super.c      |  2 +-
> >>>  3 files changed, 10 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index ffdaba0..2b651a3 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
> >>>  	f2fs_unlock_all(sbi);
> >>>  }
> >>>  
> >>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
> >>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
> >>>  {
> >>>  	DEFINE_WAIT(wait);
> >>>  
> >>>  	for (;;) {
> >>>  		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
> >>>  
> >>> -		if (!get_pages(sbi, F2FS_WB_CP_DATA))
> >>> +		if (!get_pages(sbi, type))
> >>>  			break;
> >>>  
> >>>  		if (unlikely(f2fs_cp_error(sbi)))
> >>> @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  
> >>>  	/* Flush all the NAT/SIT pages */
> >>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> >>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> >>> -					!f2fs_cp_error(sbi));
> >>> +	/* Wait for all dirty meta pages to be submitted for IO */
> >>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
> >>
> >> I'm afraid calling f2fs_wait_on_all_pages() after we call
> >> f2fs_sync_meta_pages() is low efficient, as we only want to write out
> >> dirty meta pages instead of wait for writebacking them to device cache.
> >>
> > 
> > I have modified the existing function f2fs_wait_on_all_pages_writeback() to
> > a generic one f2fs_wait_on_all_pages(), where it will wait according to the
> > requested type. In this case, it will only wait for dirty F2FS_DIRTY_META pages
> > but not for the writeback to device cache.
> 
> Oh, I see, as last dirty reference count decreaser won't wake up waiter, we need
> to wait for timeout, right? Can we decrease timeout count, maybe HZ/50 as we did
> for congestion.

Yes, I agree that it is better to decrease the timeout to HZ/50.

Thanks,

> 
> io_schedule_timeout(5*HZ);
> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> >> Thanks,
> >>
> >>>  
> >>>  	/*
> >>>  	 * modify checkpoint
> >>> @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  
> >>>  	/* Here, we have one bio having CP pack except cp pack 2 page */
> >>>  	f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> >>> -	f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> >>> -					!f2fs_cp_error(sbi));
> >>> +	/* Wait for all dirty meta pages to be submitted for IO */
> >>> +	f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
> >>>  
> >>>  	/* wait for previous submitted meta pages writeback */
> >>> -	f2fs_wait_on_all_pages_writeback(sbi);
> >>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >>>  
> >>>  	/* flush all device cache */
> >>>  	err = f2fs_flush_device_cache(sbi);
> >>> @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  
> >>>  	/* barrier and flush checkpoint cp pack 2 page if it can */
> >>>  	commit_checkpoint(sbi, ckpt, start_blk);
> >>> -	f2fs_wait_on_all_pages_writeback(sbi);
> >>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >>>  
> >>>  	/*
> >>>  	 * invalidate intermediate page cache borrowed from meta inode
> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>> index 5a888a0..b0e0535 100644
> >>> --- a/fs/f2fs/f2fs.h
> >>> +++ b/fs/f2fs/f2fs.h
> >>> @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
> >>>  void f2fs_update_dirty_page(struct inode *inode, struct page *page);
> >>>  void f2fs_remove_dirty_inode(struct inode *inode);
> >>>  int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
> >>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
> >>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
> >>>  int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>>  void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
> >>>  int __init f2fs_create_checkpoint_caches(void);
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index 5111e1f..084633b 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
> >>>  	/* our cp_error case, we can wait for any writeback page */
> >>>  	f2fs_flush_merged_writes(sbi);
> >>>  
> >>> -	f2fs_wait_on_all_pages_writeback(sbi);
> >>> +	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >>>  
> >>>  	f2fs_bug_on(sbi, sbi->fsync_node_num);
> >>>  
> >>>
> > 

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