[PATCH 1/3] f2fs: init local extent_info to avoid stale stack info in tp

[PATCH 1/3] f2fs: init local extent_info to avoid stale stack info in tp

[Hou Pengyang]

To avoid such stale(fops, blk, len) info in f2fs_lookup_extent_tree_end tp

dio-23095 [005] ...1 17878.856859: f2fs_lookup_extent_tree_end: 
			dev = (259,30), ino = 856, pgofs = 0, 
			ext_info(fofs: 3441207040, blk: 4294967232, len: 3481143808)

Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
---
 fs/f2fs/data.c | 8 ++++----
 fs/f2fs/file.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 5f3bc98..f72493d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -511,7 +511,7 @@ int f2fs_reserve_block(struct dnode_of_data *dn, pgoff_t index)
 
 int f2fs_get_block(struct dnode_of_data *dn, pgoff_t index)
 {
-	struct extent_info ei;
+	struct extent_info ei  = {0,0,0};
 	struct inode *inode = dn->inode;
 
 	if (f2fs_lookup_extent_cache(inode, index, &ei)) {
@@ -528,7 +528,7 @@ struct page *get_read_data_page(struct inode *inode, pgoff_t index,
 	struct address_space *mapping = inode->i_mapping;
 	struct dnode_of_data dn;
 	struct page *page;
-	struct extent_info ei;
+	struct extent_info ei = {0,0,0};
 	int err;
 	struct f2fs_io_info fio = {
 		.sbi = F2FS_I_SB(inode),
@@ -803,7 +803,7 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
 	int err = 0, ofs = 1;
 	unsigned int ofs_in_node, last_ofs_in_node;
 	blkcnt_t prealloc;
-	struct extent_info ei;
+	struct extent_info ei = {0,0,0};
 	block_t blkaddr;
 
 	if (!maxblocks)
@@ -1667,7 +1667,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
 	struct dnode_of_data dn;
 	struct page *ipage;
 	bool locked = false;
-	struct extent_info ei;
+	struct extent_info ei = {0,0,0};
 	int err = 0;
 
 	/*
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 1201648..36c1565 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1877,7 +1877,7 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
 {
 	struct inode *inode = file_inode(filp);
 	struct f2fs_map_blocks map = { .m_next_pgofs = NULL };
-	struct extent_info ei;
+	struct extent_info ei = {0,0,0};
 	pgoff_t pg_start, pg_end;
 	unsigned int blk_per_seg = sbi->blocks_per_seg;
 	unsigned int total = 0, sec_num;
-- 
2.10.1


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

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

[Hou Pengyang]

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

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;
 
-- 
2.10.1


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

[PATCH 3/3] f2fs: skip dirty bitmap scanning when no dirty segments

[Hou Pengyang]

Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
---
 fs/f2fs/gc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 6c996e3..eaf9e68 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -293,13 +293,22 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
 	unsigned int secno, last_victim;
 	unsigned int last_segment = MAIN_SEGS(sbi);
 	unsigned int nsearched = 0;
+	int nr_dirty;
 
 	mutex_lock(&dirty_i->seglist_lock);
 
 	p.alloc_mode = alloc_mode;
-	select_policy(sbi, gc_type, type, &p);
-
 	p.min_segno = NULL_SEGNO;
+
+	if (alloc_mode == SSR)
+		nr_dirty = dirty_i->nr_dirty[type];
+	else
+		nr_dirty = dirty_i->nr_dirty[DIRTY];
+
+	if (!nr_dirty)
+		goto out;
+
+	select_policy(sbi, gc_type, type, &p);
 	p.min_cost = get_max_cost(sbi, &p);
 
 	if (p.max_search == 0)
-- 
2.10.1


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

Re: [PATCH 3/3] f2fs: skip dirty bitmap scanning when no dirty segments

[Jaegeuk Kim]

Hi Pengyang,

On 02/23, Hou Pengyang wrote:
> Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
> ---
>  fs/f2fs/gc.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 6c996e3..eaf9e68 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -293,13 +293,22 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>  	unsigned int secno, last_victim;
>  	unsigned int last_segment = MAIN_SEGS(sbi);
>  	unsigned int nsearched = 0;
> +	int nr_dirty;
>  
>  	mutex_lock(&dirty_i->seglist_lock);
>  
>  	p.alloc_mode = alloc_mode;
> -	select_policy(sbi, gc_type, type, &p);
> -
>  	p.min_segno = NULL_SEGNO;
> +
> +	if (alloc_mode == SSR)
> +		nr_dirty = dirty_i->nr_dirty[type];
> +	else
> +		nr_dirty = dirty_i->nr_dirty[DIRTY];
> +
> +	if (!nr_dirty)
> +		goto out;
> +
> +	select_policy(sbi, gc_type, type, &p);
>  	p.min_cost = get_max_cost(sbi, &p);
>  
>  	if (p.max_search == 0)

Here, max_search will return if there is no candidate.

Thanks,

> -- 
> 2.10.1
> 
> 
> ------------------------------------------------------------------------------
> 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

Re: [PATCH 3/3] f2fs: skip dirty bitmap scanning when no dirty segments

[Hou Pengyang]

On 2017/2/24 3:03, Jaegeuk Kim wrote:
> Hi Pengyang,
>
> On 02/23, Hou Pengyang wrote:
>> Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
>> ---
>>   fs/f2fs/gc.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 6c996e3..eaf9e68 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -293,13 +293,22 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>   	unsigned int secno, last_victim;
>>   	unsigned int last_segment = MAIN_SEGS(sbi);
>>   	unsigned int nsearched = 0;
>> +	int nr_dirty;
>>
>>   	mutex_lock(&dirty_i->seglist_lock);
>>
>>   	p.alloc_mode = alloc_mode;
>> -	select_policy(sbi, gc_type, type, &p);
>> -
>>   	p.min_segno = NULL_SEGNO;
>> +
>> +	if (alloc_mode == SSR)
>> +		nr_dirty = dirty_i->nr_dirty[type];
>> +	else
>> +		nr_dirty = dirty_i->nr_dirty[DIRTY];
>> +
>> +	if (!nr_dirty)
>> +		goto out;
>> +
>> +	select_policy(sbi, gc_type, type, &p);
>>   	p.min_cost = get_max_cost(sbi, &p);
>>
>>   	if (p.max_search == 0)
>
> Here, max_search will return if there is no candidate.

Got it :)
Thanks,

>
> Thanks,
>
>> --
>> 2.10.1
>>
>>
>> ------------------------------------------------------------------------------
>> 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

Re: [PATCH 1/3] f2fs: init local extent_info to avoid stale stack info in tp

[Chao Yu]

On 2017/2/23 17:18, Hou Pengyang wrote:
> To avoid such stale(fops, blk, len) info in f2fs_lookup_extent_tree_end tp
> 
> dio-23095 [005] ...1 17878.856859: f2fs_lookup_extent_tree_end: 
> 			dev = (259,30), ino = 856, pgofs = 0, 
> 			ext_info(fofs: 3441207040, blk: 4294967232, len: 3481143808)
> 
> Signed-off-by: Hou Pengyang <houpengyang@huawei.com>

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


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

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

[Chao Yu]

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?

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

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

[Jaegeuk Kim]

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.

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

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

[Chao Yu]

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?

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

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

[Jaegeuk Kim]

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.

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

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

[Chao Yu]

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.

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

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

[Jaegeuk Kim]

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