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 <[email protected]>
---
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.
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 <[email protected]>
> ---
> 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);
>
>
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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>
>>> ---
>>> 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);
>>>
>>>
>
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 <[email protected]>
>>>> ---
>>>> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>
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 <[email protected]>
> >>> ---
> >>> 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.