[f2fs-dev] [PATCH RFC v4] f2fs: flush cp pack except cp pack 2 page at first

[f2fs-dev] [PATCH RFC v4] f2fs: flush cp pack except cp pack 2 page at first

[Gaoxiang (OS)]

Previously, we attempt to flush the whole cp pack in a single bio,
however, when suddenly powering off at this time, we could get into
an extreme scenario that cp pack 1 page and cp pack 2 page are updated
and latest, but payload or current summaries are still partially
outdated. (see reliable write in the UFS specification)

This patch submits the whole cp pack except cp pack 2 page at first,
and then writes the cp pack 2 page with an extra independent
bio with pre-io barrier.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
---
Change log from v3:
  - further review comments are applied from Jaegeuk and Chao
  - Tested on this patch (without multiple-device): mount, boot Android with f2fs userdata and make fragment
  - If any problem with this patch or I miss something, please kindly share your comments, thanks :)
Change log from v2:
  - Apply the review comments from Chao
Change log from v1:
  - Apply the review comments from Chao
  - time data from "finish block_ops" to " finish checkpoint" (tested on ARM64 with TOSHIBA 128GB UFS):
     Before patch: 0.002273  0.001973  0.002789  0.005159  0.002050
     After patch: 0.002502  0.001624  0.002487  0.003049  0.002696
 fs/f2fs/checkpoint.c | 67 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 21 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 14d2fed..916dc72 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1158,6 +1158,39 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	spin_unlock_irqrestore(&sbi->cp_lock, flags);
 }
 
+static void commit_checkpoint(struct f2fs_sb_info *sbi,
+	void *src, block_t blk_addr)
+{
+	struct writeback_control wbc = {
+		.for_reclaim = 0,
+	};
+
+	/*
+	 * pagevec_lookup_tag and lock_page again will take
+	 * some extra time. Therefore, update_meta_pages and
+	 * sync_meta_pages are combined in this function.
+	 */
+	struct page *page = grab_meta_page(sbi, blk_addr);
+	int err;
+
+	memcpy(page_address(page), src, PAGE_SIZE);
+	set_page_dirty(page);
+
+	f2fs_wait_on_page_writeback(page, META, true);
+	f2fs_bug_on(sbi, PageWriteback(page));
+	if (unlikely(!clear_page_dirty_for_io(page)))
+		f2fs_bug_on(sbi, 1);
+
+	/* writeout cp pack 2 page */
+	err = __f2fs_write_meta_page(page, &wbc, FS_CP_META_IO);
+	f2fs_bug_on(sbi, err);
+
+	f2fs_put_page(page, 0);
+
+	/* submit checkpoint (with barrier if NOBARRIER is not set) */
+	f2fs_submit_merged_write(sbi, META_FLUSH);
+}
+
 static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 {
 	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
@@ -1260,16 +1293,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		}
 	}
 
-	/* need to wait for end_io results */
-	wait_on_all_pages_writeback(sbi);
-	if (unlikely(f2fs_cp_error(sbi)))
-		return -EIO;
-
-	/* flush all device cache */
-	err = f2fs_flush_device_cache(sbi);
-	if (err)
-		return err;
-
 	/* write out checkpoint buffer at block 0 */
 	update_meta_page(sbi, ckpt, start_blk++);
 
@@ -1297,15 +1320,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		start_blk += NR_CURSEG_NODE_TYPE;
 	}
 
-	/* writeout checkpoint block */
-	update_meta_page(sbi, ckpt, start_blk);
-
-	/* wait for previous submitted node/meta pages writeback */
-	wait_on_all_pages_writeback(sbi);
-
-	if (unlikely(f2fs_cp_error(sbi)))
-		return -EIO;
-
 	filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
 	filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);
 
@@ -1313,12 +1327,23 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	sbi->last_valid_block_count = sbi->total_valid_block_count;
 	percpu_counter_set(&sbi->alloc_valid_block_count, 0);
 
-	/* Here, we only have one bio having CP pack */
-	sync_meta_pages(sbi, META_FLUSH, LONG_MAX, FS_CP_META_IO);
+	/* Here, we have one bio having CP pack except cp pack 2 page */
+	sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
+
+	/* flush all device cache */
+	err = f2fs_flush_device_cache(sbi);
+	if (err)
+		return err;
 
 	/* wait for previous submitted meta pages writeback */
 	wait_on_all_pages_writeback(sbi);
 
+	if (unlikely(f2fs_cp_error(sbi)))
+		return -EIO;
+
+	/* barrier and flush checkpoint cp pack 2 page if it can */
+	commit_checkpoint(sbi, ckpt, start_blk);
+
 	release_ino_entry(sbi, false);
 
 	if (unlikely(f2fs_cp_error(sbi)))
-- 
2.1.4

Re: [f2fs-dev] [PATCH RFC v4] f2fs: flush cp pack except cp pack 2 page at first

[Chao Yu]

On 2018/1/31 14:39, Gaoxiang (OS) wrote:
> Previously, we attempt to flush the whole cp pack in a single bio,
> however, when suddenly powering off at this time, we could get into
> an extreme scenario that cp pack 1 page and cp pack 2 page are updated
> and latest, but payload or current summaries are still partially
> outdated. (see reliable write in the UFS specification)
> 
> This patch submits the whole cp pack except cp pack 2 page at first,
> and then writes the cp pack 2 page with an extra independent
> bio with pre-io barrier.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> ---
> Change log from v3:
>   - further review comments are applied from Jaegeuk and Chao
>   - Tested on this patch (without multiple-device): mount, boot Android with f2fs userdata and make fragment
>   - If any problem with this patch or I miss something, please kindly share your comments, thanks :)
> Change log from v2:
>   - Apply the review comments from Chao
> Change log from v1:
>   - Apply the review comments from Chao
>   - time data from "finish block_ops" to " finish checkpoint" (tested on ARM64 with TOSHIBA 128GB UFS):
>      Before patch: 0.002273  0.001973  0.002789  0.005159  0.002050
>      After patch: 0.002502  0.001624  0.002487  0.003049  0.002696
>  fs/f2fs/checkpoint.c | 67 ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 14d2fed..916dc72 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1158,6 +1158,39 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	spin_unlock_irqrestore(&sbi->cp_lock, flags);
>  }
>  
> +static void commit_checkpoint(struct f2fs_sb_info *sbi,
> +	void *src, block_t blk_addr)
> +{
> +	struct writeback_control wbc = {
> +		.for_reclaim = 0,
> +	};
> +
> +	/*
> +	 * pagevec_lookup_tag and lock_page again will take
> +	 * some extra time. Therefore, update_meta_pages and
> +	 * sync_meta_pages are combined in this function.
> +	 */
> +	struct page *page = grab_meta_page(sbi, blk_addr);
> +	int err;
> +
> +	memcpy(page_address(page), src, PAGE_SIZE);
> +	set_page_dirty(page);
> +
> +	f2fs_wait_on_page_writeback(page, META, true);
> +	f2fs_bug_on(sbi, PageWriteback(page));
> +	if (unlikely(!clear_page_dirty_for_io(page)))
> +		f2fs_bug_on(sbi, 1);
> +
> +	/* writeout cp pack 2 page */
> +	err = __f2fs_write_meta_page(page, &wbc, FS_CP_META_IO);
> +	f2fs_bug_on(sbi, err);
> +
> +	f2fs_put_page(page, 0);
> +
> +	/* submit checkpoint (with barrier if NOBARRIER is not set) */
> +	f2fs_submit_merged_write(sbi, META_FLUSH);
> +}
> +
>  static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  {
>  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
> @@ -1260,16 +1293,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  		}
>  	}
>  
> -	/* need to wait for end_io results */
> -	wait_on_all_pages_writeback(sbi);
> -	if (unlikely(f2fs_cp_error(sbi)))
> -		return -EIO;
> -
> -	/* flush all device cache */
> -	err = f2fs_flush_device_cache(sbi);
> -	if (err)
> -		return err;
> -
>  	/* write out checkpoint buffer at block 0 */
>  	update_meta_page(sbi, ckpt, start_blk++);
>  
> @@ -1297,15 +1320,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  		start_blk += NR_CURSEG_NODE_TYPE;
>  	}
>  
> -	/* writeout checkpoint block */
> -	update_meta_page(sbi, ckpt, start_blk);
> -
> -	/* wait for previous submitted node/meta pages writeback */
> -	wait_on_all_pages_writeback(sbi);
> -
> -	if (unlikely(f2fs_cp_error(sbi)))
> -		return -EIO;
> -
>  	filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
>  	filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);
>  
> @@ -1313,12 +1327,23 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	sbi->last_valid_block_count = sbi->total_valid_block_count;
>  	percpu_counter_set(&sbi->alloc_valid_block_count, 0);
>  
> -	/* Here, we only have one bio having CP pack */
> -	sync_meta_pages(sbi, META_FLUSH, LONG_MAX, FS_CP_META_IO);
> +	/* Here, we have one bio having CP pack except cp pack 2 page */
> +	sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> +
> +	/* flush all device cache */
> +	err = f2fs_flush_device_cache(sbi);
> +	if (err)
> +		return err;
>  
>  	/* wait for previous submitted meta pages writeback */
>  	wait_on_all_pages_writeback(sbi);

Move f2fs_flush_device_cache here? since meta area can cross the multiple
devices, we should make sure all metadata were in device cache at least, and
then trigger the flush.

>  
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		return -EIO;
> +
> +	/* barrier and flush checkpoint cp pack 2 page if it can */
> +	commit_checkpoint(sbi, ckpt, start_blk);

Jaegeuk, are we really allow to make critical do_checkpoint which is on path of
fsync()/sync() be asynchronous?

Thanks,

> +
>  	release_ino_entry(sbi, false);
>  
>  	if (unlikely(f2fs_cp_error(sbi)))
> 

Re: [f2fs-dev] [PATCH RFC v4] f2fs: flush cp pack except cp pack 2 page at first

[Jaegeuk Kim]

On 01/31, Chao Yu wrote:
> On 2018/1/31 14:39, Gaoxiang (OS) wrote:
> > Previously, we attempt to flush the whole cp pack in a single bio,
> > however, when suddenly powering off at this time, we could get into
> > an extreme scenario that cp pack 1 page and cp pack 2 page are updated
> > and latest, but payload or current summaries are still partially
> > outdated. (see reliable write in the UFS specification)
> > 
> > This patch submits the whole cp pack except cp pack 2 page at first,
> > and then writes the cp pack 2 page with an extra independent
> > bio with pre-io barrier.
> > 
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> > ---
> > Change log from v3:
> >   - further review comments are applied from Jaegeuk and Chao
> >   - Tested on this patch (without multiple-device): mount, boot Android with f2fs userdata and make fragment
> >   - If any problem with this patch or I miss something, please kindly share your comments, thanks :)
> > Change log from v2:
> >   - Apply the review comments from Chao
> > Change log from v1:
> >   - Apply the review comments from Chao
> >   - time data from "finish block_ops" to " finish checkpoint" (tested on ARM64 with TOSHIBA 128GB UFS):
> >      Before patch: 0.002273  0.001973  0.002789  0.005159  0.002050
> >      After patch: 0.002502  0.001624  0.002487  0.003049  0.002696
> >  fs/f2fs/checkpoint.c | 67 ++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 46 insertions(+), 21 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 14d2fed..916dc72 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1158,6 +1158,39 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	spin_unlock_irqrestore(&sbi->cp_lock, flags);
> >  }
> >  
> > +static void commit_checkpoint(struct f2fs_sb_info *sbi,
> > +	void *src, block_t blk_addr)
> > +{
> > +	struct writeback_control wbc = {
> > +		.for_reclaim = 0,
> > +	};
> > +
> > +	/*
> > +	 * pagevec_lookup_tag and lock_page again will take
> > +	 * some extra time. Therefore, update_meta_pages and
> > +	 * sync_meta_pages are combined in this function.
> > +	 */
> > +	struct page *page = grab_meta_page(sbi, blk_addr);
> > +	int err;
> > +
> > +	memcpy(page_address(page), src, PAGE_SIZE);
> > +	set_page_dirty(page);
> > +
> > +	f2fs_wait_on_page_writeback(page, META, true);
> > +	f2fs_bug_on(sbi, PageWriteback(page));
> > +	if (unlikely(!clear_page_dirty_for_io(page)))
> > +		f2fs_bug_on(sbi, 1);
> > +
> > +	/* writeout cp pack 2 page */
> > +	err = __f2fs_write_meta_page(page, &wbc, FS_CP_META_IO);
> > +	f2fs_bug_on(sbi, err);
> > +
> > +	f2fs_put_page(page, 0);
> > +
> > +	/* submit checkpoint (with barrier if NOBARRIER is not set) */
> > +	f2fs_submit_merged_write(sbi, META_FLUSH);
> > +}
> > +
> >  static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  {
> >  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
> > @@ -1260,16 +1293,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  		}
> >  	}
> >  
> > -	/* need to wait for end_io results */
> > -	wait_on_all_pages_writeback(sbi);
> > -	if (unlikely(f2fs_cp_error(sbi)))
> > -		return -EIO;
> > -
> > -	/* flush all device cache */
> > -	err = f2fs_flush_device_cache(sbi);
> > -	if (err)
> > -		return err;
> > -
> >  	/* write out checkpoint buffer at block 0 */
> >  	update_meta_page(sbi, ckpt, start_blk++);
> >  
> > @@ -1297,15 +1320,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  		start_blk += NR_CURSEG_NODE_TYPE;
> >  	}
> >  
> > -	/* writeout checkpoint block */
> > -	update_meta_page(sbi, ckpt, start_blk);
> > -
> > -	/* wait for previous submitted node/meta pages writeback */
> > -	wait_on_all_pages_writeback(sbi);
> > -
> > -	if (unlikely(f2fs_cp_error(sbi)))
> > -		return -EIO;
> > -
> >  	filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
> >  	filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);

 - remove

> >  
> > @@ -1313,12 +1327,23 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	sbi->last_valid_block_count = sbi->total_valid_block_count;
> >  	percpu_counter_set(&sbi->alloc_valid_block_count, 0);
> >  
> > -	/* Here, we only have one bio having CP pack */
> > -	sync_meta_pages(sbi, META_FLUSH, LONG_MAX, FS_CP_META_IO);
> > +	/* Here, we have one bio having CP pack except cp pack 2 page */
> > +	sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> > +
> > +	/* flush all device cache */
> > +	err = f2fs_flush_device_cache(sbi);
> > +	if (err)
> > +		return err;
> >  
> >  	/* wait for previous submitted meta pages writeback */
> >  	wait_on_all_pages_writeback(sbi);
> 
> Move f2fs_flush_device_cache here? since meta area can cross the multiple
> devices, we should make sure all metadata were in device cache at least, and
> then trigger the flush.

Agreed, and need to flush, only if we have multiple devices.

> 
> >  
> > +	if (unlikely(f2fs_cp_error(sbi)))
> > +		return -EIO;
> > +
> > +	/* barrier and flush checkpoint cp pack 2 page if it can */
> > +	commit_checkpoint(sbi, ckpt, start_blk);
> 
> Jaegeuk, are we really allow to make critical do_checkpoint which is on path of
> fsync()/sync() be asynchronous?

Yeah, so we need to wait end_io on synchronous paths like f2fs_sync_fs(1).

> 
> Thanks,
> 
> > +
> >  	release_ino_entry(sbi, false);
> >  
> >  	if (unlikely(f2fs_cp_error(sbi)))
> > 

Re: [f2fs-dev] [PATCH RFC v4] f2fs: flush cp pack except cp pack 2 page at first

[Gao Xiang]

Hi Jaegeuk and Chao,

On 2018/2/1 6:28, Jaegeuk Kim wrote:
> On 01/31, Chao Yu wrote:
>> On 2018/1/31 14:39, Gaoxiang (OS) wrote:
>>> Previously, we attempt to flush the whole cp pack in a single bio,
>>> however, when suddenly powering off at this time, we could get into
>>> an extreme scenario that cp pack 1 page and cp pack 2 page are updated
>>> and latest, but payload or current summaries are still partially
>>> outdated. (see reliable write in the UFS specification)
>>>
>>> This patch submits the whole cp pack except cp pack 2 page at first,
>>> and then writes the cp pack 2 page with an extra independent
>>> bio with pre-io barrier.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>>> ---
>>> Change log from v3:
>>>    - further review comments are applied from Jaegeuk and Chao
>>>    - Tested on this patch (without multiple-device): mount, boot Android with f2fs userdata and make fragment
>>>    - If any problem with this patch or I miss something, please kindly share your comments, thanks :)
>>> Change log from v2:
>>>    - Apply the review comments from Chao
>>> Change log from v1:
>>>    - Apply the review comments from Chao
>>>    - time data from "finish block_ops" to " finish checkpoint" (tested on ARM64 with TOSHIBA 128GB UFS):
>>>       Before patch: 0.002273  0.001973  0.002789  0.005159  0.002050
>>>       After patch: 0.002502  0.001624  0.002487  0.003049  0.002696
>>>   fs/f2fs/checkpoint.c | 67 ++++++++++++++++++++++++++++++++++++----------------
>>>   1 file changed, 46 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 14d2fed..916dc72 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1158,6 +1158,39 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>   	spin_unlock_irqrestore(&sbi->cp_lock, flags);
>>>   }
>>>   
>>> +static void commit_checkpoint(struct f2fs_sb_info *sbi,
>>> +	void *src, block_t blk_addr)
>>> +{
>>> +	struct writeback_control wbc = {
>>> +		.for_reclaim = 0,
>>> +	};
>>> +
>>> +	/*
>>> +	 * pagevec_lookup_tag and lock_page again will take
>>> +	 * some extra time. Therefore, update_meta_pages and
>>> +	 * sync_meta_pages are combined in this function.
>>> +	 */
>>> +	struct page *page = grab_meta_page(sbi, blk_addr);
>>> +	int err;
>>> +
>>> +	memcpy(page_address(page), src, PAGE_SIZE);
>>> +	set_page_dirty(page);
>>> +
>>> +	f2fs_wait_on_page_writeback(page, META, true);
>>> +	f2fs_bug_on(sbi, PageWriteback(page));
>>> +	if (unlikely(!clear_page_dirty_for_io(page)))
>>> +		f2fs_bug_on(sbi, 1);
>>> +
>>> +	/* writeout cp pack 2 page */
>>> +	err = __f2fs_write_meta_page(page, &wbc, FS_CP_META_IO);
>>> +	f2fs_bug_on(sbi, err);
>>> +
>>> +	f2fs_put_page(page, 0);
>>> +
>>> +	/* submit checkpoint (with barrier if NOBARRIER is not set) */
>>> +	f2fs_submit_merged_write(sbi, META_FLUSH);
>>> +}
>>> +
>>>   static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>   {
>>>   	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
>>> @@ -1260,16 +1293,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>   		}
>>>   	}
>>>   
>>> -	/* need to wait for end_io results */
>>> -	wait_on_all_pages_writeback(sbi);
>>> -	if (unlikely(f2fs_cp_error(sbi)))
>>> -		return -EIO;
>>> -
>>> -	/* flush all device cache */
>>> -	err = f2fs_flush_device_cache(sbi);
>>> -	if (err)
>>> -		return err;
>>> -
>>>   	/* write out checkpoint buffer at block 0 */
>>>   	update_meta_page(sbi, ckpt, start_blk++);
>>>   
>>> @@ -1297,15 +1320,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>   		start_blk += NR_CURSEG_NODE_TYPE;
>>>   	}
>>>   
>>> -	/* writeout checkpoint block */
>>> -	update_meta_page(sbi, ckpt, start_blk);
>>> -
>>> -	/* wait for previous submitted node/meta pages writeback */
>>> -	wait_on_all_pages_writeback(sbi);
>>> -
>>> -	if (unlikely(f2fs_cp_error(sbi)))
>>> -		return -EIO;
>>> -
>>>   	filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
>>>   	filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);
> 
>   - remove
> 
You mean remove
filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
and
filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);
or remove
filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);

Actually, I have no idea why do these two filemap_fdatawait_range stay 
here and what are these used and waited for in this place,
however I found it was modified recently and for many times, I guess 
they have some use.

>>>   
>>> @@ -1313,12 +1327,23 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>   	sbi->last_valid_block_count = sbi->total_valid_block_count;
>>>   	percpu_counter_set(&sbi->alloc_valid_block_count, 0);
>>>   
>>> -	/* Here, we only have one bio having CP pack */
>>> -	sync_meta_pages(sbi, META_FLUSH, LONG_MAX, FS_CP_META_IO);
>>> +	/* Here, we have one bio having CP pack except cp pack 2 page */
>>> +	sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>> +
>>> +	/* flush all device cache */
>>> +	err = f2fs_flush_device_cache(sbi);
>>> +	if (err)
>>> +		return err;
>>>   
>>>   	/* wait for previous submitted meta pages writeback */
>>>   	wait_on_all_pages_writeback(sbi);
>>
>> Move f2fs_flush_device_cache here? since meta area can cross the multiple
>> devices, we should make sure all metadata were in device cache at least, and
>> then trigger the flush.
> 
> Agreed, and need to flush, only if we have multiple devices.
> 

OK, and it seems that
- current f2fs_flush_device_cache
    - for (i = 1; i < sbi->s_ndevs; i++) {
it would skip if it only has only one devices?

>>
>>>   
>>> +	if (unlikely(f2fs_cp_error(sbi)))
>>> +		return -EIO;
>>> +
>>> +	/* barrier and flush checkpoint cp pack 2 page if it can */
>>> +	commit_checkpoint(sbi, ckpt, start_blk);
>>
>> Jaegeuk, are we really allow to make critical do_checkpoint which is on path of
>> fsync()/sync() be asynchronous?
> 
> Yeah, so we need to wait end_io on synchronous paths like f2fs_sync_fs(1).
>

How about adding a CP_SYNC (or CP_ASYNC) control flag in cp_control.reason?
and which paths could be synchronous paths:
     sync, umount, recovery - synchronous, right?

Thanks,

>>
>> Thanks,
>>
>>> +
>>>   	release_ino_entry(sbi, false);
>>>   
>>>   	if (unlikely(f2fs_cp_error(sbi)))
>>>

Re: [f2fs-dev] [PATCH RFC v4] f2fs: flush cp pack except cp pack 2 page at first

[Jaegeuk Kim]

On 02/01, Gao Xiang wrote:
> Hi Jaegeuk and Chao,
> 
> On 2018/2/1 6:28, Jaegeuk Kim wrote:
> > On 01/31, Chao Yu wrote:
> > > On 2018/1/31 14:39, Gaoxiang (OS) wrote:
> > > > Previously, we attempt to flush the whole cp pack in a single bio,
> > > > however, when suddenly powering off at this time, we could get into
> > > > an extreme scenario that cp pack 1 page and cp pack 2 page are updated
> > > > and latest, but payload or current summaries are still partially
> > > > outdated. (see reliable write in the UFS specification)
> > > > 
> > > > This patch submits the whole cp pack except cp pack 2 page at first,
> > > > and then writes the cp pack 2 page with an extra independent
> > > > bio with pre-io barrier.
> > > > 
> > > > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > > > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> > > > ---
> > > > Change log from v3:
> > > >    - further review comments are applied from Jaegeuk and Chao
> > > >    - Tested on this patch (without multiple-device): mount, boot Android with f2fs userdata and make fragment
> > > >    - If any problem with this patch or I miss something, please kindly share your comments, thanks :)
> > > > Change log from v2:
> > > >    - Apply the review comments from Chao
> > > > Change log from v1:
> > > >    - Apply the review comments from Chao
> > > >    - time data from "finish block_ops" to " finish checkpoint" (tested on ARM64 with TOSHIBA 128GB UFS):
> > > >       Before patch: 0.002273  0.001973  0.002789  0.005159  0.002050
> > > >       After patch: 0.002502  0.001624  0.002487  0.003049  0.002696
> > > >   fs/f2fs/checkpoint.c | 67 ++++++++++++++++++++++++++++++++++++----------------
> > > >   1 file changed, 46 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > > > index 14d2fed..916dc72 100644
> > > > --- a/fs/f2fs/checkpoint.c
> > > > +++ b/fs/f2fs/checkpoint.c
> > > > @@ -1158,6 +1158,39 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> > > >   	spin_unlock_irqrestore(&sbi->cp_lock, flags);
> > > >   }
> > > > +static void commit_checkpoint(struct f2fs_sb_info *sbi,
> > > > +	void *src, block_t blk_addr)
> > > > +{
> > > > +	struct writeback_control wbc = {
> > > > +		.for_reclaim = 0,
> > > > +	};
> > > > +
> > > > +	/*
> > > > +	 * pagevec_lookup_tag and lock_page again will take
> > > > +	 * some extra time. Therefore, update_meta_pages and
> > > > +	 * sync_meta_pages are combined in this function.
> > > > +	 */
> > > > +	struct page *page = grab_meta_page(sbi, blk_addr);
> > > > +	int err;
> > > > +
> > > > +	memcpy(page_address(page), src, PAGE_SIZE);
> > > > +	set_page_dirty(page);
> > > > +
> > > > +	f2fs_wait_on_page_writeback(page, META, true);
> > > > +	f2fs_bug_on(sbi, PageWriteback(page));
> > > > +	if (unlikely(!clear_page_dirty_for_io(page)))
> > > > +		f2fs_bug_on(sbi, 1);
> > > > +
> > > > +	/* writeout cp pack 2 page */
> > > > +	err = __f2fs_write_meta_page(page, &wbc, FS_CP_META_IO);
> > > > +	f2fs_bug_on(sbi, err);
> > > > +
> > > > +	f2fs_put_page(page, 0);
> > > > +
> > > > +	/* submit checkpoint (with barrier if NOBARRIER is not set) */
> > > > +	f2fs_submit_merged_write(sbi, META_FLUSH);
> > > > +}
> > > > +
> > > >   static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> > > >   {
> > > >   	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
> > > > @@ -1260,16 +1293,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> > > >   		}
> > > >   	}
> > > > -	/* need to wait for end_io results */
> > > > -	wait_on_all_pages_writeback(sbi);
> > > > -	if (unlikely(f2fs_cp_error(sbi)))
> > > > -		return -EIO;
> > > > -
> > > > -	/* flush all device cache */
> > > > -	err = f2fs_flush_device_cache(sbi);
> > > > -	if (err)
> > > > -		return err;
> > > > -
> > > >   	/* write out checkpoint buffer at block 0 */
> > > >   	update_meta_page(sbi, ckpt, start_blk++);
> > > > @@ -1297,15 +1320,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> > > >   		start_blk += NR_CURSEG_NODE_TYPE;
> > > >   	}
> > > > -	/* writeout checkpoint block */
> > > > -	update_meta_page(sbi, ckpt, start_blk);
> > > > -
> > > > -	/* wait for previous submitted node/meta pages writeback */
> > > > -	wait_on_all_pages_writeback(sbi);
> > > > -
> > > > -	if (unlikely(f2fs_cp_error(sbi)))
> > > > -		return -EIO;
> > > > -
> > > >   	filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
> > > >   	filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);
> > 
> >   - remove
> > 
> You mean remove
> filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
> and
> filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);
> or remove
> filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);

Yup, both of them.

> 
> Actually, I have no idea why do these two filemap_fdatawait_range stay here
> and what are these used and waited for in this place,
> however I found it was modified recently and for many times, I guess they
> have some use.
> 
> > > > @@ -1313,12 +1327,23 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> > > >   	sbi->last_valid_block_count = sbi->total_valid_block_count;
> > > >   	percpu_counter_set(&sbi->alloc_valid_block_count, 0);
> > > > -	/* Here, we only have one bio having CP pack */
> > > > -	sync_meta_pages(sbi, META_FLUSH, LONG_MAX, FS_CP_META_IO);
> > > > +	/* Here, we have one bio having CP pack except cp pack 2 page */
> > > > +	sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> > > > +
> > > > +	/* flush all device cache */
> > > > +	err = f2fs_flush_device_cache(sbi);
> > > > +	if (err)
> > > > +		return err;
> > > >   	/* wait for previous submitted meta pages writeback */
> > > >   	wait_on_all_pages_writeback(sbi);
> > > 
> > > Move f2fs_flush_device_cache here? since meta area can cross the multiple
> > > devices, we should make sure all metadata were in device cache at least, and
> > > then trigger the flush.
> > 
> > Agreed, and need to flush, only if we have multiple devices.
> > 
> 
> OK, and it seems that
> - current f2fs_flush_device_cache
>    - for (i = 1; i < sbi->s_ndevs; i++) {
> it would skip if it only has only one devices?

Yup, right.

> 
> > > 
> > > > +	if (unlikely(f2fs_cp_error(sbi)))
> > > > +		return -EIO;
> > > > +
> > > > +	/* barrier and flush checkpoint cp pack 2 page if it can */
> > > > +	commit_checkpoint(sbi, ckpt, start_blk);
> > > 
> > > Jaegeuk, are we really allow to make critical do_checkpoint which is on path of
> > > fsync()/sync() be asynchronous?
> > 
> > Yeah, so we need to wait end_io on synchronous paths like f2fs_sync_fs(1).
> > 
> 
> How about adding a CP_SYNC (or CP_ASYNC) control flag in cp_control.reason?
> and which paths could be synchronous paths:
>     sync, umount, recovery - synchronous, right?
> 
> Thanks,
> 
> > > 
> > > Thanks,
> > > 
> > > > +
> > > >   	release_ino_entry(sbi, false);
> > > >   	if (unlikely(f2fs_cp_error(sbi)))
> > > > 

Re: [PATCH RFC v4] f2fs: flush cp pack except cp pack 2 page at first

[Chao Yu]

On 2018/2/1 6:28, Jaegeuk Kim wrote:
> On 01/31, Chao Yu wrote:
>> On 2018/1/31 14:39, Gaoxiang (OS) wrote:
>>> Previously, we attempt to flush the whole cp pack in a single bio,
>>> however, when suddenly powering off at this time, we could get into
>>> an extreme scenario that cp pack 1 page and cp pack 2 page are updated
>>> and latest, but payload or current summaries are still partially
>>> outdated. (see reliable write in the UFS specification)
>>>
>>> This patch submits the whole cp pack except cp pack 2 page at first,
>>> and then writes the cp pack 2 page with an extra independent
>>> bio with pre-io barrier.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>>> ---
>>> Change log from v3:
>>>   - further review comments are applied from Jaegeuk and Chao
>>>   - Tested on this patch (without multiple-device): mount, boot Android with f2fs userdata and make fragment
>>>   - If any problem with this patch or I miss something, please kindly share your comments, thanks :)
>>> Change log from v2:
>>>   - Apply the review comments from Chao
>>> Change log from v1:
>>>   - Apply the review comments from Chao
>>>   - time data from "finish block_ops" to " finish checkpoint" (tested on ARM64 with TOSHIBA 128GB UFS):
>>>      Before patch: 0.002273  0.001973  0.002789  0.005159  0.002050
>>>      After patch: 0.002502  0.001624  0.002487  0.003049  0.002696
>>>  fs/f2fs/checkpoint.c | 67 ++++++++++++++++++++++++++++++++++++----------------
>>>  1 file changed, 46 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 14d2fed..916dc72 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1158,6 +1158,39 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	spin_unlock_irqrestore(&sbi->cp_lock, flags);
>>>  }
>>>  
>>> +static void commit_checkpoint(struct f2fs_sb_info *sbi,
>>> +	void *src, block_t blk_addr)
>>> +{
>>> +	struct writeback_control wbc = {
>>> +		.for_reclaim = 0,
>>> +	};
>>> +
>>> +	/*
>>> +	 * pagevec_lookup_tag and lock_page again will take
>>> +	 * some extra time. Therefore, update_meta_pages and
>>> +	 * sync_meta_pages are combined in this function.
>>> +	 */
>>> +	struct page *page = grab_meta_page(sbi, blk_addr);
>>> +	int err;
>>> +
>>> +	memcpy(page_address(page), src, PAGE_SIZE);
>>> +	set_page_dirty(page);
>>> +
>>> +	f2fs_wait_on_page_writeback(page, META, true);
>>> +	f2fs_bug_on(sbi, PageWriteback(page));
>>> +	if (unlikely(!clear_page_dirty_for_io(page)))
>>> +		f2fs_bug_on(sbi, 1);
>>> +
>>> +	/* writeout cp pack 2 page */
>>> +	err = __f2fs_write_meta_page(page, &wbc, FS_CP_META_IO);
>>> +	f2fs_bug_on(sbi, err);
>>> +
>>> +	f2fs_put_page(page, 0);
>>> +
>>> +	/* submit checkpoint (with barrier if NOBARRIER is not set) */
>>> +	f2fs_submit_merged_write(sbi, META_FLUSH);
>>> +}
>>> +
>>>  static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  {
>>>  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
>>> @@ -1260,16 +1293,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  		}
>>>  	}
>>>  
>>> -	/* need to wait for end_io results */
>>> -	wait_on_all_pages_writeback(sbi);
>>> -	if (unlikely(f2fs_cp_error(sbi)))
>>> -		return -EIO;
>>> -
>>> -	/* flush all device cache */
>>> -	err = f2fs_flush_device_cache(sbi);
>>> -	if (err)
>>> -		return err;
>>> -
>>>  	/* write out checkpoint buffer at block 0 */
>>>  	update_meta_page(sbi, ckpt, start_blk++);
>>>  
>>> @@ -1297,15 +1320,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  		start_blk += NR_CURSEG_NODE_TYPE;
>>>  	}
>>>  
>>> -	/* writeout checkpoint block */
>>> -	update_meta_page(sbi, ckpt, start_blk);
>>> -
>>> -	/* wait for previous submitted node/meta pages writeback */
>>> -	wait_on_all_pages_writeback(sbi);
>>> -
>>> -	if (unlikely(f2fs_cp_error(sbi)))
>>> -		return -EIO;
>>> -
>>>  	filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
>>>  	filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);
> 
>  - remove

Agreed.

> 
>>>  
>>> @@ -1313,12 +1327,23 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	sbi->last_valid_block_count = sbi->total_valid_block_count;
>>>  	percpu_counter_set(&sbi->alloc_valid_block_count, 0);
>>>  
>>> -	/* Here, we only have one bio having CP pack */
>>> -	sync_meta_pages(sbi, META_FLUSH, LONG_MAX, FS_CP_META_IO);
>>> +	/* Here, we have one bio having CP pack except cp pack 2 page */
>>> +	sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>> +
>>> +	/* flush all device cache */
>>> +	err = f2fs_flush_device_cache(sbi);
>>> +	if (err)
>>> +		return err;
>>>  
>>>  	/* wait for previous submitted meta pages writeback */
>>>  	wait_on_all_pages_writeback(sbi);
>>
>> Move f2fs_flush_device_cache here? since meta area can cross the multiple
>> devices, we should make sure all metadata were in device cache at least, and
>> then trigger the flush.
> 
> Agreed, and need to flush, only if we have multiple devices.

f2fs_flush_device_cache is designed as only flushing devices except the first
one when f2fs enables multiple device. Calling it directly will be OK. :)

> 
>>
>>>  
>>> +	if (unlikely(f2fs_cp_error(sbi)))
>>> +		return -EIO;
>>> +
>>> +	/* barrier and flush checkpoint cp pack 2 page if it can */
>>> +	commit_checkpoint(sbi, ckpt, start_blk);
>>
>> Jaegeuk, are we really allow to make critical do_checkpoint which is on path of
>> fsync()/sync() be asynchronous?
> 
> Yeah, so we need to wait end_io on synchronous paths like f2fs_sync_fs(1).

I think we should consider the case Xiang mentioned:

1. write async checkpoint #1 from gc;
2. write sync checkpoint #2 from sync_fs, end_io finished w/ error; -> cp #2 becomes corrupted
3. checkpoint #1's end_io finished w/ error; -> cp #1 becomes corrupted

Then, entire filesystem becomes corrupted now.

Thanks,

> 
>>
>> Thanks,
>>
>>> +
>>>  	release_ino_entry(sbi, false);
>>>  
>>>  	if (unlikely(f2fs_cp_error(sbi)))
>>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot