[PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[Yunlong Song]

If some files are opened with atomic flag and have not commited yet, at
the same time, if all the target victim segments have at least one page
of these atomic files, then f2fs gc will fail to do gc and hangs in the
process of go to gc_more, since gc_date_segment will not move any data
and get_valid_blocks will never be 0, then do_garbage_collect will
always return 0.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/gc.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 5d5bba4..3fdcd04 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
 	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
 		goto out;
 
-	if (f2fs_is_atomic_file(inode))
-		goto out;
-
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
 	err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE);
 	if (err)
@@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
 	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
 		goto out;
 
-	if (f2fs_is_atomic_file(inode))
-		goto out;
-
 	if (gc_type == BG_GC) {
 		if (PageWriteback(page))
 			goto out;
-- 
1.8.5.2

Re: [PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[Chao Yu]

On 2017/11/8 10:34, Yunlong Song wrote:
> If some files are opened with atomic flag and have not commited yet, at
> the same time, if all the target victim segments have at least one page
> of these atomic files, then f2fs gc will fail to do gc and hangs in the
> process of go to gc_more, since gc_date_segment will not move any data
> and get_valid_blocks will never be 0, then do_garbage_collect will
> always return 0.

Oh, I added this judgment condition to avoid ruining atomic write by data
GC, could we find another way to solve this issue? BTW, if there is direct
IO, we will also skip data segment GC.

Thanks,

> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/gc.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 5d5bba4..3fdcd04 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
>  	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>  		goto out;
>  
> -	if (f2fs_is_atomic_file(inode))
> -		goto out;
> -
>  	set_new_dnode(&dn, inode, NULL, NULL, 0);
>  	err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE);
>  	if (err)
> @@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>  		goto out;
>  
> -	if (f2fs_is_atomic_file(inode))
> -		goto out;
> -
>  	if (gc_type == BG_GC) {
>  		if (PageWriteback(page))
>  			goto out;
> 

Re: [PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[Yunlong Song]

Is there any problem if just deleting the judgement condition in this patch?

On 2017/11/8 17:28, Chao Yu wrote:
> On 2017/11/8 10:34, Yunlong Song wrote:
>> If some files are opened with atomic flag and have not commited yet, at
>> the same time, if all the target victim segments have at least one page
>> of these atomic files, then f2fs gc will fail to do gc and hangs in the
>> process of go to gc_more, since gc_date_segment will not move any data
>> and get_valid_blocks will never be 0, then do_garbage_collect will
>> always return 0.
> Oh, I added this judgment condition to avoid ruining atomic write by data
> GC, could we find another way to solve this issue? BTW, if there is direct
> IO, we will also skip data segment GC.
>
> Thanks,
>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> ---
>>   fs/f2fs/gc.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 5d5bba4..3fdcd04 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
>>   	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>   		goto out;
>>   
>> -	if (f2fs_is_atomic_file(inode))
>> -		goto out;
>> -
>>   	set_new_dnode(&dn, inode, NULL, NULL, 0);
>>   	err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE);
>>   	if (err)
>> @@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>   	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>   		goto out;
>>   
>> -	if (f2fs_is_atomic_file(inode))
>> -		goto out;
>> -
>>   	if (gc_type == BG_GC) {
>>   		if (PageWriteback(page))
>>   			goto out;
>>
>
> .
>

-- 
Thanks,
Yunlong Song

Re: [PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[Chao Yu]

On 2017/11/17 8:58, Yunlong Song wrote:
> Is there any problem if just deleting the judgement condition in this patch?

IIRC, dirty node comes from data segment GC can be writebacked & flushed during
atomic commit, but related data will still be in inner bio cache, after later
SPOR, data would be inconsistent.

Thanks,

> 
> On 2017/11/8 17:28, Chao Yu wrote:
>> On 2017/11/8 10:34, Yunlong Song wrote:
>>> If some files are opened with atomic flag and have not commited yet, at
>>> the same time, if all the target victim segments have at least one page
>>> of these atomic files, then f2fs gc will fail to do gc and hangs in the
>>> process of go to gc_more, since gc_date_segment will not move any data
>>> and get_valid_blocks will never be 0, then do_garbage_collect will
>>> always return 0.
>> Oh, I added this judgment condition to avoid ruining atomic write by data
>> GC, could we find another way to solve this issue? BTW, if there is direct
>> IO, we will also skip data segment GC.
>>
>> Thanks,
>>
>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>> ---
>>>   fs/f2fs/gc.c | 6 ------
>>>   1 file changed, 6 deletions(-)
>>>
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index 5d5bba4..3fdcd04 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
>>>   	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>   		goto out;
>>>   
>>> -	if (f2fs_is_atomic_file(inode))
>>> -		goto out;
>>> -
>>>   	set_new_dnode(&dn, inode, NULL, NULL, 0);
>>>   	err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE);
>>>   	if (err)
>>> @@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>   	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>   		goto out;
>>>   
>>> -	if (f2fs_is_atomic_file(inode))
>>> -		goto out;
>>> -
>>>   	if (gc_type == BG_GC) {
>>>   		if (PageWriteback(page))
>>>   			goto out;
>>>
>>
>> .
>>
> 

Re: [PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[Yunlong Song]

The atomic commit will trigger:
     -f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true)
         -file_write_and_wait_range(file, 0, LLONG_MAX)
         -fsync_node_pages
             -__write_node_page
                 -REQ_PREFLUSH | REQ_FUA

So data is flushed to non-volatile before  last node write with 
REQ_PREFLUSH | REQ_FUA,
we do not need to worry about the inconsistent problem. Right?

On 2017/11/17 10:49, Chao Yu wrote:
> On 2017/11/17 8:58, Yunlong Song wrote:
>> Is there any problem if just deleting the judgement condition in this patch?
> IIRC, dirty node comes from data segment GC can be writebacked & flushed during
> atomic commit, but related data will still be in inner bio cache, after later
> SPOR, data would be inconsistent.
>
> Thanks,
>
>> On 2017/11/8 17:28, Chao Yu wrote:
>>> On 2017/11/8 10:34, Yunlong Song wrote:
>>>> If some files are opened with atomic flag and have not commited yet, at
>>>> the same time, if all the target victim segments have at least one page
>>>> of these atomic files, then f2fs gc will fail to do gc and hangs in the
>>>> process of go to gc_more, since gc_date_segment will not move any data
>>>> and get_valid_blocks will never be 0, then do_garbage_collect will
>>>> always return 0.
>>> Oh, I added this judgment condition to avoid ruining atomic write by data
>>> GC, could we find another way to solve this issue? BTW, if there is direct
>>> IO, we will also skip data segment GC.
>>>
>>> Thanks,
>>>
>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>> ---
>>>>    fs/f2fs/gc.c | 6 ------
>>>>    1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>> index 5d5bba4..3fdcd04 100644
>>>> --- a/fs/f2fs/gc.c
>>>> +++ b/fs/f2fs/gc.c
>>>> @@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
>>>>    	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>    		goto out;
>>>>    
>>>> -	if (f2fs_is_atomic_file(inode))
>>>> -		goto out;
>>>> -
>>>>    	set_new_dnode(&dn, inode, NULL, NULL, 0);
>>>>    	err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE);
>>>>    	if (err)
>>>> @@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>>    	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>    		goto out;
>>>>    
>>>> -	if (f2fs_is_atomic_file(inode))
>>>> -		goto out;
>>>> -
>>>>    	if (gc_type == BG_GC) {
>>>>    		if (PageWriteback(page))
>>>>    			goto out;
>>>>
>>> .
>>>
>
> .
>

-- 
Thanks,
Yunlong Song

Re: [PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[Chao Yu]

On 2017/11/17 11:04, Yunlong Song wrote:
> The atomic commit will trigger:
>      -f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true)
>          -file_write_and_wait_range(file, 0, LLONG_MAX)
>          -fsync_node_pages
>              -__write_node_page
>                  -REQ_PREFLUSH | REQ_FUA
> 
> So data is flushed to non-volatile before  last node write with > REQ_PREFLUSH | REQ_FUA,

I mean GCed data.

- file_write_and_wait_range
					- move_data_block
					 - f2fs_submit_page_write
					  - f2fs_update_data_blkaddr
					   - set_page_dirty
 - fsync_node_pages

Thanks,

> we do not need to worry about the inconsistent problem. Right?
> 
> On 2017/11/17 10:49, Chao Yu wrote:
>> On 2017/11/17 8:58, Yunlong Song wrote:
>>> Is there any problem if just deleting the judgement condition in this patch?
>> IIRC, dirty node comes from data segment GC can be writebacked & flushed during
>> atomic commit, but related data will still be in inner bio cache, after later
>> SPOR, data would be inconsistent.
>>
>> Thanks,
>>
>>> On 2017/11/8 17:28, Chao Yu wrote:
>>>> On 2017/11/8 10:34, Yunlong Song wrote:
>>>>> If some files are opened with atomic flag and have not commited yet, at
>>>>> the same time, if all the target victim segments have at least one page
>>>>> of these atomic files, then f2fs gc will fail to do gc and hangs in the
>>>>> process of go to gc_more, since gc_date_segment will not move any data
>>>>> and get_valid_blocks will never be 0, then do_garbage_collect will
>>>>> always return 0.
>>>> Oh, I added this judgment condition to avoid ruining atomic write by data
>>>> GC, could we find another way to solve this issue? BTW, if there is direct
>>>> IO, we will also skip data segment GC.
>>>>
>>>> Thanks,
>>>>
>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>> ---
>>>>>    fs/f2fs/gc.c | 6 ------
>>>>>    1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>> index 5d5bba4..3fdcd04 100644
>>>>> --- a/fs/f2fs/gc.c
>>>>> +++ b/fs/f2fs/gc.c
>>>>> @@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
>>>>>    	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>>    		goto out;
>>>>>    
>>>>> -	if (f2fs_is_atomic_file(inode))
>>>>> -		goto out;
>>>>> -
>>>>>    	set_new_dnode(&dn, inode, NULL, NULL, 0);
>>>>>    	err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE);
>>>>>    	if (err)
>>>>> @@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>>>    	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>>    		goto out;
>>>>>    
>>>>> -	if (f2fs_is_atomic_file(inode))
>>>>> -		goto out;
>>>>> -
>>>>>    	if (gc_type == BG_GC) {
>>>>>    		if (PageWriteback(page))
>>>>>    			goto out;
>>>>>
>>>> .
>>>>
>>
>> .
>>
> 

Re: [PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[Yunlong Song]

How about add file_write_and_wait_range in __write_node_page as following:
if (atomic && !test_opt(sbi, NOBARRIER)) {
     file_write_and_wait_range(file, 0, LLONG_MAX);
     fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
}

The all the GCed data will be flushed to non-volatile before last node 
write with REQ_PREFLUSH | REQ_FUA.

On 2017/11/17 11:20, Chao Yu wrote:
> On 2017/11/17 11:04, Yunlong Song wrote:
>> The atomic commit will trigger:
>>       -f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true)
>>           -file_write_and_wait_range(file, 0, LLONG_MAX)
>>           -fsync_node_pages
>>               -__write_node_page
>>                   -REQ_PREFLUSH | REQ_FUA
>>
>> So data is flushed to non-volatile before  last node write with > REQ_PREFLUSH | REQ_FUA,
> I mean GCed data.
>
> - file_write_and_wait_range
> 					- move_data_block
> 					 - f2fs_submit_page_write
> 					  - f2fs_update_data_blkaddr
> 					   - set_page_dirty
>   - fsync_node_pages
>
> Thanks,
>
>> we do not need to worry about the inconsistent problem. Right?
>>
>> On 2017/11/17 10:49, Chao Yu wrote:
>>> On 2017/11/17 8:58, Yunlong Song wrote:
>>>> Is there any problem if just deleting the judgement condition in this patch?
>>> IIRC, dirty node comes from data segment GC can be writebacked & flushed during
>>> atomic commit, but related data will still be in inner bio cache, after later
>>> SPOR, data would be inconsistent.
>>>
>>> Thanks,
>>>
>>>> On 2017/11/8 17:28, Chao Yu wrote:
>>>>> On 2017/11/8 10:34, Yunlong Song wrote:
>>>>>> If some files are opened with atomic flag and have not commited yet, at
>>>>>> the same time, if all the target victim segments have at least one page
>>>>>> of these atomic files, then f2fs gc will fail to do gc and hangs in the
>>>>>> process of go to gc_more, since gc_date_segment will not move any data
>>>>>> and get_valid_blocks will never be 0, then do_garbage_collect will
>>>>>> always return 0.
>>>>> Oh, I added this judgment condition to avoid ruining atomic write by data
>>>>> GC, could we find another way to solve this issue? BTW, if there is direct
>>>>> IO, we will also skip data segment GC.
>>>>>
>>>>> Thanks,
>>>>>
>>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>>> ---
>>>>>>     fs/f2fs/gc.c | 6 ------
>>>>>>     1 file changed, 6 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>> index 5d5bba4..3fdcd04 100644
>>>>>> --- a/fs/f2fs/gc.c
>>>>>> +++ b/fs/f2fs/gc.c
>>>>>> @@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
>>>>>>     	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>>>     		goto out;
>>>>>>     
>>>>>> -	if (f2fs_is_atomic_file(inode))
>>>>>> -		goto out;
>>>>>> -
>>>>>>     	set_new_dnode(&dn, inode, NULL, NULL, 0);
>>>>>>     	err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE);
>>>>>>     	if (err)
>>>>>> @@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>>>>     	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>>>     		goto out;
>>>>>>     
>>>>>> -	if (f2fs_is_atomic_file(inode))
>>>>>> -		goto out;
>>>>>> -
>>>>>>     	if (gc_type == BG_GC) {
>>>>>>     		if (PageWriteback(page))
>>>>>>     			goto out;
>>>>>>
>>>>> .
>>>>>
>>> .
>>>
>
> .
>

-- 
Thanks,
Yunlong Song

Re: [PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc

[Chao Yu]

On 2017/11/17 11:30, Yunlong Song wrote:
> How about add file_write_and_wait_range in __write_node_page as following:
> if (atomic && !test_opt(sbi, NOBARRIER)) {
>      file_write_and_wait_range(file, 0, LLONG_MAX);

Nope, GCed encrypted data wouldn't be cached in inode page cache.

I don't think adding raw code to flush data here is a good implementation,
it incurs complicated lock dependency relation. Instead, IMO, it will be
better to use inmem_lock to avoid race in between GC and atomic commit flow.

Thanks,

>      fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
> }
> 
> The all the GCed data will be flushed to non-volatile before last node 
> write with REQ_PREFLUSH | REQ_FUA.
> 
> On 2017/11/17 11:20, Chao Yu wrote:
>> On 2017/11/17 11:04, Yunlong Song wrote:
>>> The atomic commit will trigger:
>>>       -f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true)
>>>           -file_write_and_wait_range(file, 0, LLONG_MAX)
>>>           -fsync_node_pages
>>>               -__write_node_page
>>>                   -REQ_PREFLUSH | REQ_FUA
>>>
>>> So data is flushed to non-volatile before  last node write with > REQ_PREFLUSH | REQ_FUA,
>> I mean GCed data.
>>
>> - file_write_and_wait_range
>> 					- move_data_block
>> 					 - f2fs_submit_page_write
>> 					  - f2fs_update_data_blkaddr
>> 					   - set_page_dirty
>>   - fsync_node_pages
>>
>> Thanks,
>>
>>> we do not need to worry about the inconsistent problem. Right?
>>>
>>> On 2017/11/17 10:49, Chao Yu wrote:
>>>> On 2017/11/17 8:58, Yunlong Song wrote:
>>>>> Is there any problem if just deleting the judgement condition in this patch?
>>>> IIRC, dirty node comes from data segment GC can be writebacked & flushed during
>>>> atomic commit, but related data will still be in inner bio cache, after later
>>>> SPOR, data would be inconsistent.
>>>>
>>>> Thanks,
>>>>
>>>>> On 2017/11/8 17:28, Chao Yu wrote:
>>>>>> On 2017/11/8 10:34, Yunlong Song wrote:
>>>>>>> If some files are opened with atomic flag and have not commited yet, at
>>>>>>> the same time, if all the target victim segments have at least one page
>>>>>>> of these atomic files, then f2fs gc will fail to do gc and hangs in the
>>>>>>> process of go to gc_more, since gc_date_segment will not move any data
>>>>>>> and get_valid_blocks will never be 0, then do_garbage_collect will
>>>>>>> always return 0.
>>>>>> Oh, I added this judgment condition to avoid ruining atomic write by data
>>>>>> GC, could we find another way to solve this issue? BTW, if there is direct
>>>>>> IO, we will also skip data segment GC.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>>>> ---
>>>>>>>     fs/f2fs/gc.c | 6 ------
>>>>>>>     1 file changed, 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>>> index 5d5bba4..3fdcd04 100644
>>>>>>> --- a/fs/f2fs/gc.c
>>>>>>> +++ b/fs/f2fs/gc.c
>>>>>>> @@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
>>>>>>>     	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>>>>     		goto out;
>>>>>>>     
>>>>>>> -	if (f2fs_is_atomic_file(inode))
>>>>>>> -		goto out;
>>>>>>> -
>>>>>>>     	set_new_dnode(&dn, inode, NULL, NULL, 0);
>>>>>>>     	err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE);
>>>>>>>     	if (err)
>>>>>>> @@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>>>>>     	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>>>>     		goto out;
>>>>>>>     
>>>>>>> -	if (f2fs_is_atomic_file(inode))
>>>>>>> -		goto out;
>>>>>>> -
>>>>>>>     	if (gc_type == BG_GC) {
>>>>>>>     		if (PageWriteback(page))
>>>>>>>     			goto out;
>>>>>>>
>>>>>> .
>>>>>>
>>>> .
>>>>
>>
>> .
>>
>