[PATCH] f2fs: fix a infinite loop in gc_node_segment

[PATCH] f2fs: fix a infinite loop in gc_node_segment

[Yunlei He]

Similar as this problem:
	f2fs: relax node version check for victim data in gc

But this patch fix wrong SSA nid.

Signed-off-by: Yunlei He <heyunlei@huawei.com>
---
 fs/f2fs/gc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index c52656c..1d0fd04 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -499,6 +499,25 @@ static void gc_node_segment(struct f2fs_sb_info *sbi,
 		get_node_info(sbi, nid, &ni);
 		if (ni.blk_addr != start_addr + off) {
 			f2fs_put_page(node_page, 1);
+			node_page = pagecache_get_page(META_MAPPING(sbi), start_addr + off,
+								FGP_LOCK | FGP_CREAT, GFP_NOFS);
+			if (!node_page)
+				continue;
+
+			lock_page(node_page);
+			if (unlikely(node_page->mapping != META_MAPPING(sbi))
+							|| unlikely(!PageUptodate(node_page))) {
+				f2fs_put_page(node_page, 1);
+				continue;
+			}
+
+			get_node_info(sbi, nid_of_node(node_page), &ni);
+			if (ni.blk_addr == start_addr + off) {
+				entry->nid = cpu_to_le32(ni.nid);
+			else
+				f2fs_bug_on(sbi, 1);
+
+			f2fs_put_page(node_page, 1);
 			continue;
 		}
 
-- 
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] f2fs: fix a infinite loop in gc_node_segment

[Jaegeuk Kim]

Hi Yunlei,

On 03/31, Yunlei He wrote:
> Similar as this problem:
> 	f2fs: relax node version check for victim data in gc
> 
> But this patch fix wrong SSA nid.

What was the problem and how does this fix it?

> 
> Signed-off-by: Yunlei He <heyunlei@huawei.com>
> ---
>  fs/f2fs/gc.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index c52656c..1d0fd04 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -499,6 +499,25 @@ static void gc_node_segment(struct f2fs_sb_info *sbi,
>  		get_node_info(sbi, nid, &ni);
>  		if (ni.blk_addr != start_addr + off) {
>  			f2fs_put_page(node_page, 1);
> +			node_page = pagecache_get_page(META_MAPPING(sbi), start_addr + off,
> +								FGP_LOCK | FGP_CREAT, GFP_NOFS);

Why do you think node_page can be cached in meta_inode?

> +			if (!node_page)
> +				continue;
> +
> +			lock_page(node_page);

Locking the page again? Since FGP_LOCK gave a locked page.

Thanks,

> +			if (unlikely(node_page->mapping != META_MAPPING(sbi))
> +							|| unlikely(!PageUptodate(node_page))) {
> +				f2fs_put_page(node_page, 1);
> +				continue;
> +			}
> +
> +			get_node_info(sbi, nid_of_node(node_page), &ni);
> +			if (ni.blk_addr == start_addr + off) {
> +				entry->nid = cpu_to_le32(ni.nid);
> +			else
> +				f2fs_bug_on(sbi, 1);
> +
> +			f2fs_put_page(node_page, 1);
>  			continue;
>  		}
>  
> -- 
> 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] f2fs: fix a infinite loop in gc_node_segment

[heyunlei]

Hi Jaegeuk,

On 2017/4/4 1:39, Jaegeuk Kim wrote:
> Hi Yunlei,
>
> On 03/31, Yunlei He wrote:
>> Similar as this problem:
>> 	f2fs: relax node version check for victim data in gc
>>
>> But this patch fix wrong SSA nid.
>
> What was the problem and how does this fix it?

- has_not_enough_free_secs
           - f2fs_gc
              - get_victim_by_default
                 - do_garbage_collect
		  - gc_node_segment
                     - ni.blk_addr != start_addr + off
				blk_addr = 2668744,  node_footer->nid = 10659, sum_entry->nid = 5431
- gc_more select same section

Here, something wrong with sum_entry of this block. Can we fix this error online, or just
adding a bug_on here?

>
>>
>> Signed-off-by: Yunlei He <heyunlei@huawei.com>
>> ---
>>  fs/f2fs/gc.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index c52656c..1d0fd04 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -499,6 +499,25 @@ static void gc_node_segment(struct f2fs_sb_info *sbi,
>>  		get_node_info(sbi, nid, &ni);
>>  		if (ni.blk_addr != start_addr + off) {
>>  			f2fs_put_page(node_page, 1);
>> +			node_page = pagecache_get_page(META_MAPPING(sbi), start_addr + off,
>> +								FGP_LOCK | FGP_CREAT, GFP_NOFS);
>
> Why do you think node_page can be cached in meta_inode?
>
>> +			if (!node_page)
>> +				continue;
>> +
>> +			lock_page(node_page);
>
> Locking the page again? Since FGP_LOCK gave a locked page.
>
> Thanks,
>
>> +			if (unlikely(node_page->mapping != META_MAPPING(sbi))
>> +							|| unlikely(!PageUptodate(node_page))) {
>> +				f2fs_put_page(node_page, 1);
>> +				continue;
>> +			}
>> +
>> +			get_node_info(sbi, nid_of_node(node_page), &ni);
>> +			if (ni.blk_addr == start_addr + off) {
>> +				entry->nid = cpu_to_le32(ni.nid);
>> +			else
>> +				f2fs_bug_on(sbi, 1);
>> +
>> +			f2fs_put_page(node_page, 1);
>>  			continue;
>>  		}
>>
>> --
>> 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] f2fs: fix a infinite loop in gc_node_segment

[Jaegeuk Kim]

On 04/05, heyunlei wrote:
> Hi Jaegeuk,
> 
> On 2017/4/4 1:39, Jaegeuk Kim wrote:
> > Hi Yunlei,
> > 
> > On 03/31, Yunlei He wrote:
> > > Similar as this problem:
> > > 	f2fs: relax node version check for victim data in gc
> > > 
> > > But this patch fix wrong SSA nid.
> > 
> > What was the problem and how does this fix it?
> 
> - has_not_enough_free_secs
>           - f2fs_gc
>              - get_victim_by_default
>                 - do_garbage_collect
> 		  - gc_node_segment
>                     - ni.blk_addr != start_addr + off
> 				blk_addr = 2668744,  node_footer->nid = 10659, sum_entry->nid = 5431

Can you check the node block written in 2668744?
Does the problem come from wrong sum_entry only?

Thanks,

> - gc_more select same section
> 
> Here, something wrong with sum_entry of this block. Can we fix this error online, or just
> adding a bug_on here?
> 
> > 
> > > 
> > > Signed-off-by: Yunlei He <heyunlei@huawei.com>
> > > ---
> > >  fs/f2fs/gc.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > index c52656c..1d0fd04 100644
> > > --- a/fs/f2fs/gc.c
> > > +++ b/fs/f2fs/gc.c
> > > @@ -499,6 +499,25 @@ static void gc_node_segment(struct f2fs_sb_info *sbi,
> > >  		get_node_info(sbi, nid, &ni);
> > >  		if (ni.blk_addr != start_addr + off) {
> > >  			f2fs_put_page(node_page, 1);
> > > +			node_page = pagecache_get_page(META_MAPPING(sbi), start_addr + off,
> > > +								FGP_LOCK | FGP_CREAT, GFP_NOFS);
> > 
> > Why do you think node_page can be cached in meta_inode?
> > 
> > > +			if (!node_page)
> > > +				continue;
> > > +
> > > +			lock_page(node_page);
> > 
> > Locking the page again? Since FGP_LOCK gave a locked page.
> > 
> > Thanks,
> > 
> > > +			if (unlikely(node_page->mapping != META_MAPPING(sbi))
> > > +							|| unlikely(!PageUptodate(node_page))) {
> > > +				f2fs_put_page(node_page, 1);
> > > +				continue;
> > > +			}
> > > +
> > > +			get_node_info(sbi, nid_of_node(node_page), &ni);
> > > +			if (ni.blk_addr == start_addr + off) {
> > > +				entry->nid = cpu_to_le32(ni.nid);
> > > +			else
> > > +				f2fs_bug_on(sbi, 1);
> > > +
> > > +			f2fs_put_page(node_page, 1);
> > >  			continue;
> > >  		}
> > > 
> > > --
> > > 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] f2fs: fix a infinite loop in gc_node_segment

[heyunlei]

Hi Jaegeuk,

On 2017/4/6 2:22, Jaegeuk Kim wrote:
> On 04/05, heyunlei wrote:
>> Hi Jaegeuk,
>>
>> On 2017/4/4 1:39, Jaegeuk Kim wrote:
>>> Hi Yunlei,
>>>
>>> On 03/31, Yunlei He wrote:
>>>> Similar as this problem:
>>>> 	f2fs: relax node version check for victim data in gc
>>>>
>>>> But this patch fix wrong SSA nid.
>>>
>>> What was the problem and how does this fix it?
>>
>> - has_not_enough_free_secs
>>           - f2fs_gc
>>              - get_victim_by_default
>>                 - do_garbage_collect
>> 		  - gc_node_segment
>>                     - ni.blk_addr != start_addr + off
>> 				blk_addr = 2668744,  node_footer->nid = 10659, sum_entry->nid = 5431
>
> Can you check the node block written in 2668744?
> Does the problem come from wrong sum_entry only?

Yeso<\f
node_footer->nid = 10659, block addr in nat_entry 10659 is 2668744,

fsck just fix the wrong sum_entry nid.

Thanks,

>
> Thanks,
>
>> - gc_more select same section
>>
>> Here, something wrong with sum_entry of this block. Can we fix this error online, or just
>> adding a bug_on here?
>>
>>>
>>>>
>>>> Signed-off-by: Yunlei He <heyunlei@huawei.com>
>>>> ---
>>>>  fs/f2fs/gc.c | 19 +++++++++++++++++++
>>>>  1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>> index c52656c..1d0fd04 100644
>>>> --- a/fs/f2fs/gc.c
>>>> +++ b/fs/f2fs/gc.c
>>>> @@ -499,6 +499,25 @@ static void gc_node_segment(struct f2fs_sb_info *sbi,
>>>>  		get_node_info(sbi, nid, &ni);
>>>>  		if (ni.blk_addr != start_addr + off) {
>>>>  			f2fs_put_page(node_page, 1);
>>>> +			node_page = pagecache_get_page(META_MAPPING(sbi), start_addr + off,
>>>> +								FGP_LOCK | FGP_CREAT, GFP_NOFS);
>>>
>>> Why do you think node_page can be cached in meta_inode?
>>>
>>>> +			if (!node_page)
>>>> +				continue;
>>>> +
>>>> +			lock_page(node_page);
>>>
>>> Locking the page again? Since FGP_LOCK gave a locked page.
>>>
>>> Thanks,
>>>
>>>> +			if (unlikely(node_page->mapping != META_MAPPING(sbi))
>>>> +							|| unlikely(!PageUptodate(node_page))) {
>>>> +				f2fs_put_page(node_page, 1);
>>>> +				continue;
>>>> +			}
>>>> +
>>>> +			get_node_info(sbi, nid_of_node(node_page), &ni);
>>>> +			if (ni.blk_addr == start_addr + off) {
>>>> +				entry->nid = cpu_to_le32(ni.nid);
>>>> +			else
>>>> +				f2fs_bug_on(sbi, 1);
>>>> +
>>>> +			f2fs_put_page(node_page, 1);
>>>>  			continue;
>>>>  		}
>>>>
>>>> --
>>>> 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