Re: [PATCH 2/3] f2fs: remove unsafe bitmap checking

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 02/25, Chao Yu wrote:
> On 2017/2/25 10:03, Jaegeuk Kim wrote:
> > On 02/25, Chao Yu wrote:
> >> On 2017/2/25 1:45, Jaegeuk Kim wrote:
> >>> On 02/24, Chao Yu wrote:
> >>>> On 2017/2/23 17:18, Hou Pengyang wrote:
> >>>>> proc A:                      proc B:
> >>>>> - writeback_sb_inodes
> >>>>> - __writeback_single_inode   
> >>>>> - do_writepages
> >>>>> - f2fs_write_node_pages       
> >>>>> - f2fs_balance_fs_bg         - write_checkpoint
> >>>>> - build_free_nids            - flush_nat_entries
> >>>>> - __build_free_nids          - __flush_nat_entry_set
> >>>>> - ra_meta_pages              - get_next_nat_page
> >>>>> - current_nat_addr           - set_to_next_nat
> >>>>> [do nat_bitmap checking]     - f2fs_change_bit
> >>>>
> >>>> Both flows were protected by nat_tree_lock, so we don't need to worry about such
> >>>> case?
> >>>
> >>> The nat_tree_lock doesn't cover ra_meta_pages in proc A.
> >>
> >> Can we cover ra_meta_pages in __build_free_nid with nat_tree_lock?
> > 
> > I don't think we need to do this only for the consistency check.
> 
> For improving efficiency of NAT block readahead, otherwise we will wast IO for
> wrong NAT block in race condition.

That should be an another topic about the hit ratio of all the ra_meta_pages()
not limited nat pages here.

Thanks,

> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> >>
> >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >> index 819032961218..17ae737a958d 100644
> >> --- a/fs/f2fs/node.c
> >> +++ b/fs/f2fs/node.c
> >> @@ -1997,12 +1997,12 @@ static void __build_free_nids(struct f2fs_sb_info *sbi,
> >> bool sync, bool mount)
> >>                         nid = idx * NAT_ENTRY_PER_BLOCK;
> >>         }
> >>
> >> +       down_read(&nm_i->nat_tree_lock);
> >> +
> >>         /* readahead nat pages to be scanned */
> >>         ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nid), FREE_NID_PAGES,
> >>                                                         META_NAT, true);
> >>
> >> -       down_read(&nm_i->nat_tree_lock);
> >> -
> >>         while (1) {
> >>                 struct page *page = get_current_nat_page(sbi, nid);
> >>
> >> @@ -2033,10 +2033,10 @@ static void __build_free_nids(struct f2fs_sb_info *sbi,
> >> bool sync, bool mount)
> >>                         remove_free_nid(sbi, nid);
> >>         }
> >>         up_read(&curseg->journal_rwsem);
> >> -       up_read(&nm_i->nat_tree_lock);
> >>
> >>         ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nm_i->next_scan_nid),
> >>                                         nm_i->ra_nid_pages, META_NAT, false);
> >> +       up_read(&nm_i->nat_tree_lock);
> >>  }
> >>
> >>  void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount)
> >>
> >>
> >>>
> >>> Thanks,
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>
> >>>>> For proc A, nat_bitmap and nat_bitmap_mir would be compared without lock_op and 
> >>>>> nm_i->nat_tree_lock, while proc B is changing nat_bitmap/nat_bitmap_ver in cp.
> >>>>>
> >>>>> So it is normal for nat_bitmap/nat_bitmap diffrence under such scenario.
> >>>>>
> >>>>> This patch fix this by removing the monitoring point.
> >>>>>
> >>>>> [Fix: 599a09b f2fs: check in-memory nat version bitmap]
> >>>>> Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
> >>>>> ---
> >>>>>  fs/f2fs/node.h | 6 ------
> >>>>>  1 file changed, 6 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> >>>>> index d3d2893..3fc9c4b 100644
> >>>>> --- a/fs/f2fs/node.h
> >>>>> +++ b/fs/f2fs/node.h
> >>>>> @@ -209,12 +209,6 @@ static inline pgoff_t current_nat_addr(struct f2fs_sb_info *sbi, nid_t start)
> >>>>>  		(seg_off << sbi->log_blocks_per_seg << 1) +
> >>>>>  		(block_off & (sbi->blocks_per_seg - 1)));
> >>>>>  
> >>>>> -#ifdef CONFIG_F2FS_CHECK_FS
> >>>>> -	if (f2fs_test_bit(block_off, nm_i->nat_bitmap) !=
> >>>>> -			f2fs_test_bit(block_off, nm_i->nat_bitmap_mir))
> >>>>> -		f2fs_bug_on(sbi, 1);
> >>>>> -#endif
> >>>>> -
> >>>>>  	if (f2fs_test_bit(block_off, nm_i->nat_bitmap))
> >>>>>  		block_addr += sbi->blocks_per_seg;
> >>>>>  
> >>>>>
> >>>>
> >>>>
> >>>> ------------------------------------------------------------------------------
> >>>> Check out the vibrant tech community on one of the world's most
> >>>> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> >>>> _______________________________________________
> >>>> Linux-f2fs-devel mailing list
> >>>> Linux-f2fs-devel@lists.sourceforge.net
> >>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>
> >>> .
> >>>
> > 
> > .
> > 
> 
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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