[PATCH 1/3] f2fs: attach inline_data after setting compression

[PATCH 1/3] f2fs: attach inline_data after setting compression

[Jaegeuk Kim]

This fixes the below corruption.

[345393.335389] F2FS-fs (vdb): sanity_check_inode: inode (ino=6d0, mode=33206) should not have inline_data, run fsck to fix

Cc: <stable@vger.kernel.org>
Fixes: 677a82b44ebf ("f2fs: fix to do sanity check for inline inode")
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/namei.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index c549acb52ac4..a841abe6a071 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -89,8 +89,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
 	if (test_opt(sbi, INLINE_XATTR))
 		set_inode_flag(inode, FI_INLINE_XATTR);
 
-	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
-		set_inode_flag(inode, FI_INLINE_DATA);
 	if (f2fs_may_inline_dentry(inode))
 		set_inode_flag(inode, FI_INLINE_DENTRY);
 
@@ -107,10 +105,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
 
 	f2fs_init_extent_tree(inode, NULL);
 
-	stat_inc_inline_xattr(inode);
-	stat_inc_inline_inode(inode);
-	stat_inc_inline_dir(inode);
-
 	F2FS_I(inode)->i_flags =
 		f2fs_mask_flags(mode, F2FS_I(dir)->i_flags & F2FS_FL_INHERITED);
 
@@ -127,6 +121,14 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
 			set_compress_context(inode);
 	}
 
+	/* Should enable inline_data after compression set */
+	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
+		set_inode_flag(inode, FI_INLINE_DATA);
+
+	stat_inc_inline_xattr(inode);
+	stat_inc_inline_inode(inode);
+	stat_inc_inline_dir(inode);
+
 	f2fs_set_inode_flags(inode);
 
 	trace_f2fs_new_inode(inode, 0);
@@ -325,6 +327,8 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
 		if (!is_extension_exist(name, ext[i], false))
 			continue;
 
+		/* Do not use inline_data with compression */
+		clear_inode_flag(inode, FI_INLINE_DATA);
 		set_compress_context(inode);
 		return;
 	}
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH 2/3] f2fs: run GCs synchronously given user requests

[Jaegeuk Kim]

When users set GC_URGENT or GC_MID, they expected to do GCs right away.
But, there's a condition to bypass it. Let's indicate we need to do now
in the thread.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/gc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index d5fb426e0747..f4aa3c88118b 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -37,7 +37,6 @@ static int gc_thread_func(void *data)
 	unsigned int wait_ms;
 	struct f2fs_gc_control gc_control = {
 		.victim_segno = NULL_SEGNO,
-		.should_migrate_blocks = false,
 		.err_gc_skipped = false };
 
 	wait_ms = gc_th->min_sleep_time;
@@ -113,7 +112,10 @@ static int gc_thread_func(void *data)
 				sbi->gc_mode == GC_URGENT_MID) {
 			wait_ms = gc_th->urgent_sleep_time;
 			f2fs_down_write(&sbi->gc_lock);
+			gc_control.should_migrate_blocks = true;
 			goto do_gc;
+		} else {
+			gc_control.should_migrate_blocks = false;
 		}
 
 		if (foreground) {
@@ -139,7 +141,9 @@ static int gc_thread_func(void *data)
 		if (!foreground)
 			stat_inc_bggc_count(sbi->stat_info);
 
-		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC;
+		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC ||
+				sbi->gc_mode == GC_URGENT_HIGH ||
+				sbi->gc_mode == GC_URGENT_MID;
 
 		/* foreground GC was been triggered via f2fs_balance_fs() */
 		if (foreground)
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH 3/3] f2fs: do not skip updating inode when retrying to flush node page

[Jaegeuk Kim]

Let's try to flush dirty inode again to improve subtle i_blocks mismatch.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/node.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 836c79a20afc..4181d03a7ef7 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1943,7 +1943,6 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi,
 		for (i = 0; i < nr_pages; i++) {
 			struct page *page = pvec.pages[i];
 			bool submitted = false;
-			bool may_dirty = true;
 
 			/* give a priority to WB_SYNC threads */
 			if (atomic_read(&sbi->wb_sync_req[NODE]) &&
@@ -1996,11 +1995,8 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi,
 			}
 
 			/* flush dirty inode */
-			if (IS_INODE(page) && may_dirty) {
-				may_dirty = false;
-				if (flush_dirty_inode(page))
-					goto lock_node;
-			}
+			if (IS_INODE(page) && flush_dirty_inode(page))
+				goto lock_node;
 write_node:
 			f2fs_wait_on_page_writeback(page, NODE, true, true);
 
-- 
2.36.1.476.g0c4daa206d-goog

Re: [f2fs-dev] [PATCH 1/3] f2fs: attach inline_data after setting compression

[Chao Yu]

On 2022/6/18 6:31, Jaegeuk Kim wrote:
> This fixes the below corruption.
> 
> [345393.335389] F2FS-fs (vdb): sanity_check_inode: inode (ino=6d0, mode=33206) should not have inline_data, run fsck to fix
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 677a82b44ebf ("f2fs: fix to do sanity check for inline inode")
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>   fs/f2fs/namei.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index c549acb52ac4..a841abe6a071 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -89,8 +89,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>   	if (test_opt(sbi, INLINE_XATTR))
>   		set_inode_flag(inode, FI_INLINE_XATTR);
>   
> -	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> -		set_inode_flag(inode, FI_INLINE_DATA);
>   	if (f2fs_may_inline_dentry(inode))
>   		set_inode_flag(inode, FI_INLINE_DENTRY);
>   
> @@ -107,10 +105,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>   
>   	f2fs_init_extent_tree(inode, NULL);
>   
> -	stat_inc_inline_xattr(inode);
> -	stat_inc_inline_inode(inode);
> -	stat_inc_inline_dir(inode);
> -
>   	F2FS_I(inode)->i_flags =
>   		f2fs_mask_flags(mode, F2FS_I(dir)->i_flags & F2FS_FL_INHERITED);
>   
> @@ -127,6 +121,14 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>   			set_compress_context(inode);
>   	}
>   
> +	/* Should enable inline_data after compression set */
> +	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> +		set_inode_flag(inode, FI_INLINE_DATA);
> +
> +	stat_inc_inline_xattr(inode);
> +	stat_inc_inline_inode(inode);
> +	stat_inc_inline_dir(inode);
> +
>   	f2fs_set_inode_flags(inode);
>   
>   	trace_f2fs_new_inode(inode, 0);
> @@ -325,6 +327,8 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>   		if (!is_extension_exist(name, ext[i], false))
>   			continue;
>   
> +		/* Do not use inline_data with compression */
> +		clear_inode_flag(inode, FI_INLINE_DATA);

if (is_inode_set_flag()) {
	clear_inode_flag();
	stat_dec_inline_inode();
}

Thanks,

>   		set_compress_context(inode);
>   		return;
>   	}

Re: [f2fs-dev] [PATCH 2/3] f2fs: run GCs synchronously given user requests

[Chao Yu]

On 2022/6/18 6:31, Jaegeuk Kim wrote:
> When users set GC_URGENT or GC_MID, they expected to do GCs right away.
> But, there's a condition to bypass it. Let's indicate we need to do now
> in the thread.

.should_migrate_blocks is used to force migrating blocks in full
section, so what is the condition here? GC should not never select
a full section, right?

Thanks,

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>   fs/f2fs/gc.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index d5fb426e0747..f4aa3c88118b 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -37,7 +37,6 @@ static int gc_thread_func(void *data)
>   	unsigned int wait_ms;
>   	struct f2fs_gc_control gc_control = {
>   		.victim_segno = NULL_SEGNO,
> -		.should_migrate_blocks = false,
>   		.err_gc_skipped = false };
>   
>   	wait_ms = gc_th->min_sleep_time;
> @@ -113,7 +112,10 @@ static int gc_thread_func(void *data)
>   				sbi->gc_mode == GC_URGENT_MID) {
>   			wait_ms = gc_th->urgent_sleep_time;
>   			f2fs_down_write(&sbi->gc_lock);
> +			gc_control.should_migrate_blocks = true;
>   			goto do_gc;
> +		} else {
> +			gc_control.should_migrate_blocks = false;
>   		}
>   
>   		if (foreground) {
> @@ -139,7 +141,9 @@ static int gc_thread_func(void *data)
>   		if (!foreground)
>   			stat_inc_bggc_count(sbi->stat_info);
>   
> -		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC;
> +		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC ||
> +				sbi->gc_mode == GC_URGENT_HIGH ||
> +				sbi->gc_mode == GC_URGENT_MID;
>   
>   		/* foreground GC was been triggered via f2fs_balance_fs() */
>   		if (foreground)

Re: [f2fs-dev] [PATCH 3/3] f2fs: do not skip updating inode when retrying to flush node page

[Chao Yu]

On 2022/6/18 6:31, Jaegeuk Kim wrote:
> Let's try to flush dirty inode again to improve subtle i_blocks mismatch.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [f2fs-dev] [PATCH 2/3] f2fs: run GCs synchronously given user requests

[Jaegeuk Kim]

On 06/19, Chao Yu wrote:
> On 2022/6/18 6:31, Jaegeuk Kim wrote:
> > When users set GC_URGENT or GC_MID, they expected to do GCs right away.
> > But, there's a condition to bypass it. Let's indicate we need to do now
> > in the thread.
> 
> .should_migrate_blocks is used to force migrating blocks in full
> section, so what is the condition here? GC should not never select
> a full section, right?

I think it'll move a full section given .victim_segno is not NULL_SEGNO,
as __get_victim will give a dirty segment all the time. wdyt?

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >   fs/f2fs/gc.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index d5fb426e0747..f4aa3c88118b 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -37,7 +37,6 @@ static int gc_thread_func(void *data)
> >   	unsigned int wait_ms;
> >   	struct f2fs_gc_control gc_control = {
> >   		.victim_segno = NULL_SEGNO,
> > -		.should_migrate_blocks = false,
> >   		.err_gc_skipped = false };
> >   	wait_ms = gc_th->min_sleep_time;
> > @@ -113,7 +112,10 @@ static int gc_thread_func(void *data)
> >   				sbi->gc_mode == GC_URGENT_MID) {
> >   			wait_ms = gc_th->urgent_sleep_time;
> >   			f2fs_down_write(&sbi->gc_lock);
> > +			gc_control.should_migrate_blocks = true;
> >   			goto do_gc;
> > +		} else {
> > +			gc_control.should_migrate_blocks = false;
> >   		}
> >   		if (foreground) {
> > @@ -139,7 +141,9 @@ static int gc_thread_func(void *data)
> >   		if (!foreground)
> >   			stat_inc_bggc_count(sbi->stat_info);
> > -		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC;
> > +		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC ||
> > +				sbi->gc_mode == GC_URGENT_HIGH ||
> > +				sbi->gc_mode == GC_URGENT_MID;
> >   		/* foreground GC was been triggered via f2fs_balance_fs() */
> >   		if (foreground)

Re: [f2fs-dev] [PATCH 2/3] f2fs: run GCs synchronously given user requests

[Chao Yu]

On 2022/6/20 6:34, Jaegeuk Kim wrote:
> On 06/19, Chao Yu wrote:
>> On 2022/6/18 6:31, Jaegeuk Kim wrote:
>>> When users set GC_URGENT or GC_MID, they expected to do GCs right away.
>>> But, there's a condition to bypass it. Let's indicate we need to do now
>>> in the thread.
>>
>> .should_migrate_blocks is used to force migrating blocks in full
>> section, so what is the condition here? GC should not never select
>> a full section, right?
> 
> I think it'll move a full section given .victim_segno is not NULL_SEGNO,
> as __get_victim will give a dirty segment all the time. wdyt?

However, in gc_thread_fun() victim_segno is NULL_SEGNO all the time.

I guess .should_migrate_blocks should only be set to true for
F2FS_IOC_FLUSH_DEVICE/F2FS_IOC_RESIZE_FS case? rather than GC_URGENT or GC_MID
case? See commit 7dede88659df ("f2fs: fix to allow migrating fully valid segment").

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>    fs/f2fs/gc.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index d5fb426e0747..f4aa3c88118b 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -37,7 +37,6 @@ static int gc_thread_func(void *data)
>>>    	unsigned int wait_ms;
>>>    	struct f2fs_gc_control gc_control = {
>>>    		.victim_segno = NULL_SEGNO,
>>> -		.should_migrate_blocks = false,
>>>    		.err_gc_skipped = false };
>>>    	wait_ms = gc_th->min_sleep_time;
>>> @@ -113,7 +112,10 @@ static int gc_thread_func(void *data)
>>>    				sbi->gc_mode == GC_URGENT_MID) {
>>>    			wait_ms = gc_th->urgent_sleep_time;
>>>    			f2fs_down_write(&sbi->gc_lock);
>>> +			gc_control.should_migrate_blocks = true;
>>>    			goto do_gc;
>>> +		} else {
>>> +			gc_control.should_migrate_blocks = false;
>>>    		}
>>>    		if (foreground) {
>>> @@ -139,7 +141,9 @@ static int gc_thread_func(void *data)
>>>    		if (!foreground)
>>>    			stat_inc_bggc_count(sbi->stat_info);
>>> -		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC;
>>> +		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC ||
>>> +				sbi->gc_mode == GC_URGENT_HIGH ||
>>> +				sbi->gc_mode == GC_URGENT_MID;
>>>    		/* foreground GC was been triggered via f2fs_balance_fs() */
>>>    		if (foreground)

Re: [f2fs-dev] [PATCH 1/3] f2fs: attach inline_data after setting compression

[Chao Yu]

On 2022/6/19 8:35, Chao Yu wrote:
> On 2022/6/18 6:31, Jaegeuk Kim wrote:
>> This fixes the below corruption.
>>
>> [345393.335389] F2FS-fs (vdb): sanity_check_inode: inode (ino=6d0, mode=33206) should not have inline_data, run fsck to fix
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: 677a82b44ebf ("f2fs: fix to do sanity check for inline inode")
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>> ---
>>    fs/f2fs/namei.c | 16 ++++++++++------
>>    1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index c549acb52ac4..a841abe6a071 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -89,8 +89,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>>    	if (test_opt(sbi, INLINE_XATTR))
>>    		set_inode_flag(inode, FI_INLINE_XATTR);
>>    
>> -	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
>> -		set_inode_flag(inode, FI_INLINE_DATA);
>>    	if (f2fs_may_inline_dentry(inode))
>>    		set_inode_flag(inode, FI_INLINE_DENTRY);
>>    
>> @@ -107,10 +105,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>>    
>>    	f2fs_init_extent_tree(inode, NULL);
>>    
>> -	stat_inc_inline_xattr(inode);
>> -	stat_inc_inline_inode(inode);
>> -	stat_inc_inline_dir(inode);
>> -
>>    	F2FS_I(inode)->i_flags =
>>    		f2fs_mask_flags(mode, F2FS_I(dir)->i_flags & F2FS_FL_INHERITED);
>>    
>> @@ -127,6 +121,14 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>>    			set_compress_context(inode);
>>    	}
>>    
>> +	/* Should enable inline_data after compression set */
>> +	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
>> +		set_inode_flag(inode, FI_INLINE_DATA);
>> +
>> +	stat_inc_inline_xattr(inode);
>> +	stat_inc_inline_inode(inode);
>> +	stat_inc_inline_dir(inode);
>> +
>>    	f2fs_set_inode_flags(inode);
>>    
>>    	trace_f2fs_new_inode(inode, 0);
>> @@ -325,6 +327,8 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>>    		if (!is_extension_exist(name, ext[i], false))
>>    			continue;
>>    
>> +		/* Do not use inline_data with compression */
>> +		clear_inode_flag(inode, FI_INLINE_DATA);
> 
> if (is_inode_set_flag()) {
> 	clear_inode_flag();
> 	stat_dec_inline_inode();
> }

Missed to send new version to mailing list?

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=4cde00d50707c2ef6647b9b96b2cb40b6eb24397

Thanks,

> 
> Thanks,
> 
>>    		set_compress_context(inode);
>>    		return;
>>    	}
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/3] f2fs: attach inline_data after setting compression

[Jaegeuk Kim]

On 06/22, Chao Yu wrote:
> On 2022/6/19 8:35, Chao Yu wrote:
> > On 2022/6/18 6:31, Jaegeuk Kim wrote:
> > > This fixes the below corruption.
> > > 
> > > [345393.335389] F2FS-fs (vdb): sanity_check_inode: inode (ino=6d0, mode=33206) should not have inline_data, run fsck to fix
> > > 
> > > Cc: <stable@vger.kernel.org>
> > > Fixes: 677a82b44ebf ("f2fs: fix to do sanity check for inline inode")
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >    fs/f2fs/namei.c | 16 ++++++++++------
> > >    1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> > > index c549acb52ac4..a841abe6a071 100644
> > > --- a/fs/f2fs/namei.c
> > > +++ b/fs/f2fs/namei.c
> > > @@ -89,8 +89,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > >    	if (test_opt(sbi, INLINE_XATTR))
> > >    		set_inode_flag(inode, FI_INLINE_XATTR);
> > > -	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> > > -		set_inode_flag(inode, FI_INLINE_DATA);
> > >    	if (f2fs_may_inline_dentry(inode))
> > >    		set_inode_flag(inode, FI_INLINE_DENTRY);
> > > @@ -107,10 +105,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > >    	f2fs_init_extent_tree(inode, NULL);
> > > -	stat_inc_inline_xattr(inode);
> > > -	stat_inc_inline_inode(inode);
> > > -	stat_inc_inline_dir(inode);
> > > -
> > >    	F2FS_I(inode)->i_flags =
> > >    		f2fs_mask_flags(mode, F2FS_I(dir)->i_flags & F2FS_FL_INHERITED);
> > > @@ -127,6 +121,14 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
> > >    			set_compress_context(inode);
> > >    	}
> > > +	/* Should enable inline_data after compression set */
> > > +	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> > > +		set_inode_flag(inode, FI_INLINE_DATA);
> > > +
> > > +	stat_inc_inline_xattr(inode);
> > > +	stat_inc_inline_inode(inode);
> > > +	stat_inc_inline_dir(inode);
> > > +
> > >    	f2fs_set_inode_flags(inode);
> > >    	trace_f2fs_new_inode(inode, 0);
> > > @@ -325,6 +327,8 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
> > >    		if (!is_extension_exist(name, ext[i], false))
> > >    			continue;
> > > +		/* Do not use inline_data with compression */
> > > +		clear_inode_flag(inode, FI_INLINE_DATA);
> > 
> > if (is_inode_set_flag()) {
> > 	clear_inode_flag();
> > 	stat_dec_inline_inode();
> > }
> 
> Missed to send new version to mailing list?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=4cde00d50707c2ef6647b9b96b2cb40b6eb24397

I'm testing the new version.

> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > >    		set_compress_context(inode);
> > >    		return;
> > >    	}
> > 
> > 
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 1/3 v2] f2fs: attach inline_data after setting compression

[Jaegeuk Kim]

This fixes the below corruption.

[345393.335389] F2FS-fs (vdb): sanity_check_inode: inode (ino=6d0, mode=33206) should not have inline_data, run fsck to fix

Cc: <stable@vger.kernel.org>
Fixes: 677a82b44ebf ("f2fs: fix to do sanity check for inline inode")
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/namei.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index c549acb52ac4..bf00d5057abb 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -89,8 +89,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
 	if (test_opt(sbi, INLINE_XATTR))
 		set_inode_flag(inode, FI_INLINE_XATTR);
 
-	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
-		set_inode_flag(inode, FI_INLINE_DATA);
 	if (f2fs_may_inline_dentry(inode))
 		set_inode_flag(inode, FI_INLINE_DENTRY);
 
@@ -107,10 +105,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
 
 	f2fs_init_extent_tree(inode, NULL);
 
-	stat_inc_inline_xattr(inode);
-	stat_inc_inline_inode(inode);
-	stat_inc_inline_dir(inode);
-
 	F2FS_I(inode)->i_flags =
 		f2fs_mask_flags(mode, F2FS_I(dir)->i_flags & F2FS_FL_INHERITED);
 
@@ -127,6 +121,14 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
 			set_compress_context(inode);
 	}
 
+	/* Should enable inline_data after compression set */
+	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
+		set_inode_flag(inode, FI_INLINE_DATA);
+
+	stat_inc_inline_xattr(inode);
+	stat_inc_inline_inode(inode);
+	stat_inc_inline_dir(inode);
+
 	f2fs_set_inode_flags(inode);
 
 	trace_f2fs_new_inode(inode, 0);
@@ -325,6 +327,9 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
 		if (!is_extension_exist(name, ext[i], false))
 			continue;
 
+		/* Do not use inline_data with compression */
+		stat_dec_inline_inode(inode);
+		clear_inode_flag(inode, FI_INLINE_DATA);
 		set_compress_context(inode);
 		return;
 	}
-- 
2.37.0.rc0.104.g0611611a94-goog

Re: [f2fs-dev] [PATCH 2/3] f2fs: run GCs synchronously given user requests

[Jaegeuk Kim]

On 06/22, Chao Yu wrote:
> On 2022/6/20 6:34, Jaegeuk Kim wrote:
> > On 06/19, Chao Yu wrote:
> > > On 2022/6/18 6:31, Jaegeuk Kim wrote:
> > > > When users set GC_URGENT or GC_MID, they expected to do GCs right away.
> > > > But, there's a condition to bypass it. Let's indicate we need to do now
> > > > in the thread.
> > > 
> > > .should_migrate_blocks is used to force migrating blocks in full
> > > section, so what is the condition here? GC should not never select
> > > a full section, right?
> > 
> > I think it'll move a full section given .victim_segno is not NULL_SEGNO,
> > as __get_victim will give a dirty segment all the time. wdyt?
> 
> However, in gc_thread_fun() victim_segno is NULL_SEGNO all the time.

What do you mean? The gc_thread thread sets NULL_SEGNO, which prevents
from selecting the full section. But, f2fs_ioc_flush_device will set the
segno to move, and f2fs_resize_fs calls do_garbage_collect directly.

> 
> I guess .should_migrate_blocks should only be set to true for
> F2FS_IOC_FLUSH_DEVICE/F2FS_IOC_RESIZE_FS case? rather than GC_URGENT or GC_MID
> case? See commit 7dede88659df ("f2fs: fix to allow migrating fully valid segment").
> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > ---
> > > >    fs/f2fs/gc.c | 8 ++++++--
> > > >    1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > > index d5fb426e0747..f4aa3c88118b 100644
> > > > --- a/fs/f2fs/gc.c
> > > > +++ b/fs/f2fs/gc.c
> > > > @@ -37,7 +37,6 @@ static int gc_thread_func(void *data)
> > > >    	unsigned int wait_ms;
> > > >    	struct f2fs_gc_control gc_control = {
> > > >    		.victim_segno = NULL_SEGNO,
> > > > -		.should_migrate_blocks = false,
> > > >    		.err_gc_skipped = false };
> > > >    	wait_ms = gc_th->min_sleep_time;
> > > > @@ -113,7 +112,10 @@ static int gc_thread_func(void *data)
> > > >    				sbi->gc_mode == GC_URGENT_MID) {
> > > >    			wait_ms = gc_th->urgent_sleep_time;
> > > >    			f2fs_down_write(&sbi->gc_lock);
> > > > +			gc_control.should_migrate_blocks = true;
> > > >    			goto do_gc;
> > > > +		} else {
> > > > +			gc_control.should_migrate_blocks = false;
> > > >    		}
> > > >    		if (foreground) {
> > > > @@ -139,7 +141,9 @@ static int gc_thread_func(void *data)
> > > >    		if (!foreground)
> > > >    			stat_inc_bggc_count(sbi->stat_info);
> > > > -		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC;
> > > > +		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC ||
> > > > +				sbi->gc_mode == GC_URGENT_HIGH ||
> > > > +				sbi->gc_mode == GC_URGENT_MID;
> > > >    		/* foreground GC was been triggered via f2fs_balance_fs() */
> > > >    		if (foreground)

Re: [f2fs-dev] [PATCH 1/3 v2] f2fs: attach inline_data after setting compression

[Chao Yu]

On 2022/6/23 0:53, Jaegeuk Kim wrote:
> This fixes the below corruption.
> 
> [345393.335389] F2FS-fs (vdb): sanity_check_inode: inode (ino=6d0, mode=33206) should not have inline_data, run fsck to fix
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 677a82b44ebf ("f2fs: fix to do sanity check for inline inode")
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>   fs/f2fs/namei.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index c549acb52ac4..bf00d5057abb 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -89,8 +89,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>   	if (test_opt(sbi, INLINE_XATTR))
>   		set_inode_flag(inode, FI_INLINE_XATTR);
>   
> -	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> -		set_inode_flag(inode, FI_INLINE_DATA);
>   	if (f2fs_may_inline_dentry(inode))
>   		set_inode_flag(inode, FI_INLINE_DENTRY);
>   
> @@ -107,10 +105,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>   
>   	f2fs_init_extent_tree(inode, NULL);
>   
> -	stat_inc_inline_xattr(inode);
> -	stat_inc_inline_inode(inode);
> -	stat_inc_inline_dir(inode);
> -
>   	F2FS_I(inode)->i_flags =
>   		f2fs_mask_flags(mode, F2FS_I(dir)->i_flags & F2FS_FL_INHERITED);
>   
> @@ -127,6 +121,14 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>   			set_compress_context(inode);
>   	}
>   
> +	/* Should enable inline_data after compression set */
> +	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
> +		set_inode_flag(inode, FI_INLINE_DATA);
> +
> +	stat_inc_inline_xattr(inode);
> +	stat_inc_inline_inode(inode);
> +	stat_inc_inline_dir(inode);
> +
>   	f2fs_set_inode_flags(inode);
>   
>   	trace_f2fs_new_inode(inode, 0);
> @@ -325,6 +327,9 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>   		if (!is_extension_exist(name, ext[i], false))
>   			continue;
>   
> +		/* Do not use inline_data with compression */
> +		stat_dec_inline_inode(inode);
> +		clear_inode_flag(inode, FI_INLINE_DATA);

It looks we don't need to dirty inode if there is no inline_data flag.

Thanks,

>   		set_compress_context(inode);
>   		return;
>   	}

Re: [f2fs-dev] [PATCH 1/3 v2] f2fs: attach inline_data after setting compression

[Chao Yu]

On 2022/6/28 15:46, Chao Yu wrote:
> On 2022/6/23 0:53, Jaegeuk Kim wrote:
>> This fixes the below corruption.
>>
>> [345393.335389] F2FS-fs (vdb): sanity_check_inode: inode (ino=6d0, mode=33206) should not have inline_data, run fsck to fix
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: 677a82b44ebf ("f2fs: fix to do sanity check for inline inode")
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>> ---
>>   fs/f2fs/namei.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index c549acb52ac4..bf00d5057abb 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -89,8 +89,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>>       if (test_opt(sbi, INLINE_XATTR))
>>           set_inode_flag(inode, FI_INLINE_XATTR);
>> -    if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
>> -        set_inode_flag(inode, FI_INLINE_DATA);
>>       if (f2fs_may_inline_dentry(inode))
>>           set_inode_flag(inode, FI_INLINE_DENTRY);
>> @@ -107,10 +105,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>>       f2fs_init_extent_tree(inode, NULL);
>> -    stat_inc_inline_xattr(inode);
>> -    stat_inc_inline_inode(inode);
>> -    stat_inc_inline_dir(inode);
>> -
>>       F2FS_I(inode)->i_flags =
>>           f2fs_mask_flags(mode, F2FS_I(dir)->i_flags & F2FS_FL_INHERITED);
>> @@ -127,6 +121,14 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns,
>>               set_compress_context(inode);
>>       }
>> +    /* Should enable inline_data after compression set */
>> +    if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
>> +        set_inode_flag(inode, FI_INLINE_DATA);
>> +
>> +    stat_inc_inline_xattr(inode);
>> +    stat_inc_inline_inode(inode);
>> +    stat_inc_inline_dir(inode);
>> +
>>       f2fs_set_inode_flags(inode);
>>       trace_f2fs_new_inode(inode, 0);
>> @@ -325,6 +327,9 @@ static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode,
>>           if (!is_extension_exist(name, ext[i], false))
>>               continue;
>> +        /* Do not use inline_data with compression */
>> +        stat_dec_inline_inode(inode);
>> +        clear_inode_flag(inode, FI_INLINE_DATA);
> 
> It looks we don't need to dirty inode if there is no inline_data flag.

Oh, it looks set_compress_context() will dirty inode anyway.... :P

Thanks,

> 
> Thanks,
> 
>>           set_compress_context(inode);
>>           return;
>>       }
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/3] f2fs: run GCs synchronously given user requests

[Chao Yu]

On 2022/6/23 0:58, Jaegeuk Kim wrote:
> On 06/22, Chao Yu wrote:
>> On 2022/6/20 6:34, Jaegeuk Kim wrote:
>>> On 06/19, Chao Yu wrote:
>>>> On 2022/6/18 6:31, Jaegeuk Kim wrote:
>>>>> When users set GC_URGENT or GC_MID, they expected to do GCs right away.
>>>>> But, there's a condition to bypass it. Let's indicate we need to do now
>>>>> in the thread.
>>>>
>>>> .should_migrate_blocks is used to force migrating blocks in full
>>>> section, so what is the condition here? GC should not never select
>>>> a full section, right?
>>>
>>> I think it'll move a full section given .victim_segno is not NULL_SEGNO,
>>> as __get_victim will give a dirty segment all the time. wdyt?
>>
>> However, in gc_thread_fun() victim_segno is NULL_SEGNO all the time.
> 
> What do you mean? The gc_thread thread sets NULL_SEGNO, which prevents
> from selecting the full section. But, f2fs_ioc_flush_device will set the
> segno to move, and f2fs_resize_fs calls do_garbage_collect directly.

Yes, but I didn't get why this patch updates .should_migrate_blocks for
gc_thread_func() case? If this is added to avoid breaking from below condition,
I guess it's not necessary, since victim selected from gc_thread_func() will
always be dirty, so get_valid_blocks(sbi, segno, true) == BLKS_PER_SEC(sbi)
will be false anyway? or am I missing something?

		/*
		 * stop BG_GC if there is not enough free sections.
		 * Or, stop GC if the segment becomes fully valid caused by
		 * race condition along with SSR block allocation.
		 */
		if ((gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) ||
			(!force_migrate && get_valid_blocks(sbi, segno, true) ==
							BLKS_PER_SEC(sbi)))
			return submitted;

Thanks,

> 
>>
>> I guess .should_migrate_blocks should only be set to true for
>> F2FS_IOC_FLUSH_DEVICE/F2FS_IOC_RESIZE_FS case? rather than GC_URGENT or GC_MID
>> case? See commit 7dede88659df ("f2fs: fix to allow migrating fully valid segment").
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>> ---
>>>>>     fs/f2fs/gc.c | 8 ++++++--
>>>>>     1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>> index d5fb426e0747..f4aa3c88118b 100644
>>>>> --- a/fs/f2fs/gc.c
>>>>> +++ b/fs/f2fs/gc.c
>>>>> @@ -37,7 +37,6 @@ static int gc_thread_func(void *data)
>>>>>     	unsigned int wait_ms;
>>>>>     	struct f2fs_gc_control gc_control = {
>>>>>     		.victim_segno = NULL_SEGNO,
>>>>> -		.should_migrate_blocks = false,
>>>>>     		.err_gc_skipped = false };
>>>>>     	wait_ms = gc_th->min_sleep_time;
>>>>> @@ -113,7 +112,10 @@ static int gc_thread_func(void *data)
>>>>>     				sbi->gc_mode == GC_URGENT_MID) {
>>>>>     			wait_ms = gc_th->urgent_sleep_time;
>>>>>     			f2fs_down_write(&sbi->gc_lock);
>>>>> +			gc_control.should_migrate_blocks = true;
>>>>>     			goto do_gc;
>>>>> +		} else {
>>>>> +			gc_control.should_migrate_blocks = false;
>>>>>     		}
>>>>>     		if (foreground) {
>>>>> @@ -139,7 +141,9 @@ static int gc_thread_func(void *data)
>>>>>     		if (!foreground)
>>>>>     			stat_inc_bggc_count(sbi->stat_info);
>>>>> -		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC;
>>>>> +		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC ||
>>>>> +				sbi->gc_mode == GC_URGENT_HIGH ||
>>>>> +				sbi->gc_mode == GC_URGENT_MID;
>>>>>     		/* foreground GC was been triggered via f2fs_balance_fs() */
>>>>>     		if (foreground)

Re: [f2fs-dev] [PATCH 2/3] f2fs: run GCs synchronously given user requests

[Jaegeuk Kim]

On 06/28, Chao Yu wrote:
> On 2022/6/23 0:58, Jaegeuk Kim wrote:
> > On 06/22, Chao Yu wrote:
> > > On 2022/6/20 6:34, Jaegeuk Kim wrote:
> > > > On 06/19, Chao Yu wrote:
> > > > > On 2022/6/18 6:31, Jaegeuk Kim wrote:
> > > > > > When users set GC_URGENT or GC_MID, they expected to do GCs right away.
> > > > > > But, there's a condition to bypass it. Let's indicate we need to do now
> > > > > > in the thread.
> > > > > 
> > > > > .should_migrate_blocks is used to force migrating blocks in full
> > > > > section, so what is the condition here? GC should not never select
> > > > > a full section, right?
> > > > 
> > > > I think it'll move a full section given .victim_segno is not NULL_SEGNO,
> > > > as __get_victim will give a dirty segment all the time. wdyt?
> > > 
> > > However, in gc_thread_fun() victim_segno is NULL_SEGNO all the time.
> > 
> > What do you mean? The gc_thread thread sets NULL_SEGNO, which prevents
> > from selecting the full section. But, f2fs_ioc_flush_device will set the
> > segno to move, and f2fs_resize_fs calls do_garbage_collect directly.
> 
> Yes, but I didn't get why this patch updates .should_migrate_blocks for
> gc_thread_func() case? If this is added to avoid breaking from below condition,
> I guess it's not necessary, since victim selected from gc_thread_func() will
> always be dirty, so get_valid_blocks(sbi, segno, true) == BLKS_PER_SEC(sbi)
> will be false anyway? or am I missing something?

I lost the previous test. :( IIRC, once I set GC_URGENT_HIGH, I've seen that
f2fs_gc couldn't migrate blocks quickly, even or never succeeded. And, I
also suspected the below condition tho, if I give force_migrate, it
simply worked. I just realized that that may be caused by large sections
having non-2MB-aligned zone capacity. Will check it again, as I found
some buggy flows in that case.

> 
> 		/*
> 		 * stop BG_GC if there is not enough free sections.
> 		 * Or, stop GC if the segment becomes fully valid caused by
> 		 * race condition along with SSR block allocation.
> 		 */
> 		if ((gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) ||
> 			(!force_migrate && get_valid_blocks(sbi, segno, true) ==
> 							BLKS_PER_SEC(sbi)))
> 			return submitted;
> 
> Thanks,
> 
> > 
> > > 
> > > I guess .should_migrate_blocks should only be set to true for
> > > F2FS_IOC_FLUSH_DEVICE/F2FS_IOC_RESIZE_FS case? rather than GC_URGENT or GC_MID
> > > case? See commit 7dede88659df ("f2fs: fix to allow migrating fully valid segment").
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > > > ---
> > > > > >     fs/f2fs/gc.c | 8 ++++++--
> > > > > >     1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > > > > index d5fb426e0747..f4aa3c88118b 100644
> > > > > > --- a/fs/f2fs/gc.c
> > > > > > +++ b/fs/f2fs/gc.c
> > > > > > @@ -37,7 +37,6 @@ static int gc_thread_func(void *data)
> > > > > >     	unsigned int wait_ms;
> > > > > >     	struct f2fs_gc_control gc_control = {
> > > > > >     		.victim_segno = NULL_SEGNO,
> > > > > > -		.should_migrate_blocks = false,
> > > > > >     		.err_gc_skipped = false };
> > > > > >     	wait_ms = gc_th->min_sleep_time;
> > > > > > @@ -113,7 +112,10 @@ static int gc_thread_func(void *data)
> > > > > >     				sbi->gc_mode == GC_URGENT_MID) {
> > > > > >     			wait_ms = gc_th->urgent_sleep_time;
> > > > > >     			f2fs_down_write(&sbi->gc_lock);
> > > > > > +			gc_control.should_migrate_blocks = true;
> > > > > >     			goto do_gc;
> > > > > > +		} else {
> > > > > > +			gc_control.should_migrate_blocks = false;
> > > > > >     		}
> > > > > >     		if (foreground) {
> > > > > > @@ -139,7 +141,9 @@ static int gc_thread_func(void *data)
> > > > > >     		if (!foreground)
> > > > > >     			stat_inc_bggc_count(sbi->stat_info);
> > > > > > -		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC;
> > > > > > +		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC ||
> > > > > > +				sbi->gc_mode == GC_URGENT_HIGH ||
> > > > > > +				sbi->gc_mode == GC_URGENT_MID;
> > > > > >     		/* foreground GC was been triggered via f2fs_balance_fs() */
> > > > > >     		if (foreground)

Re: [f2fs-dev] [PATCH 2/3] f2fs: run GCs synchronously given user requests

[Jaegeuk Kim]

On 06/28, Jaegeuk Kim wrote:
> On 06/28, Chao Yu wrote:
> > On 2022/6/23 0:58, Jaegeuk Kim wrote:
> > > On 06/22, Chao Yu wrote:
> > > > On 2022/6/20 6:34, Jaegeuk Kim wrote:
> > > > > On 06/19, Chao Yu wrote:
> > > > > > On 2022/6/18 6:31, Jaegeuk Kim wrote:
> > > > > > > When users set GC_URGENT or GC_MID, they expected to do GCs right away.
> > > > > > > But, there's a condition to bypass it. Let's indicate we need to do now
> > > > > > > in the thread.
> > > > > > 
> > > > > > .should_migrate_blocks is used to force migrating blocks in full
> > > > > > section, so what is the condition here? GC should not never select
> > > > > > a full section, right?
> > > > > 
> > > > > I think it'll move a full section given .victim_segno is not NULL_SEGNO,
> > > > > as __get_victim will give a dirty segment all the time. wdyt?
> > > > 
> > > > However, in gc_thread_fun() victim_segno is NULL_SEGNO all the time.
> > > 
> > > What do you mean? The gc_thread thread sets NULL_SEGNO, which prevents
> > > from selecting the full section. But, f2fs_ioc_flush_device will set the
> > > segno to move, and f2fs_resize_fs calls do_garbage_collect directly.
> > 
> > Yes, but I didn't get why this patch updates .should_migrate_blocks for
> > gc_thread_func() case? If this is added to avoid breaking from below condition,
> > I guess it's not necessary, since victim selected from gc_thread_func() will
> > always be dirty, so get_valid_blocks(sbi, segno, true) == BLKS_PER_SEC(sbi)
> > will be false anyway? or am I missing something?
> 
> I lost the previous test. :( IIRC, once I set GC_URGENT_HIGH, I've seen that
> f2fs_gc couldn't migrate blocks quickly, even or never succeeded. And, I
> also suspected the below condition tho, if I give force_migrate, it
> simply worked. I just realized that that may be caused by large sections
> having non-2MB-aligned zone capacity. Will check it again, as I found
> some buggy flows in that case.

Let me drop this patch, since [1] series could fix the GC issue.

[1] https://lore.kernel.org/linux-f2fs-devel/20220628234733.3330502-1-jaegeuk@kernel.org/T/#t

> 
> > 
> > 		/*
> > 		 * stop BG_GC if there is not enough free sections.
> > 		 * Or, stop GC if the segment becomes fully valid caused by
> > 		 * race condition along with SSR block allocation.
> > 		 */
> > 		if ((gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) ||
> > 			(!force_migrate && get_valid_blocks(sbi, segno, true) ==
> > 							BLKS_PER_SEC(sbi)))
> > 			return submitted;
> > 
> > Thanks,
> > 
> > > 
> > > > 
> > > > I guess .should_migrate_blocks should only be set to true for
> > > > F2FS_IOC_FLUSH_DEVICE/F2FS_IOC_RESIZE_FS case? rather than GC_URGENT or GC_MID
> > > > case? See commit 7dede88659df ("f2fs: fix to allow migrating fully valid segment").
> > > > 
> > > > Thanks,
> > > > 
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > > > > ---
> > > > > > >     fs/f2fs/gc.c | 8 ++++++--
> > > > > > >     1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > > > > > index d5fb426e0747..f4aa3c88118b 100644
> > > > > > > --- a/fs/f2fs/gc.c
> > > > > > > +++ b/fs/f2fs/gc.c
> > > > > > > @@ -37,7 +37,6 @@ static int gc_thread_func(void *data)
> > > > > > >     	unsigned int wait_ms;
> > > > > > >     	struct f2fs_gc_control gc_control = {
> > > > > > >     		.victim_segno = NULL_SEGNO,
> > > > > > > -		.should_migrate_blocks = false,
> > > > > > >     		.err_gc_skipped = false };
> > > > > > >     	wait_ms = gc_th->min_sleep_time;
> > > > > > > @@ -113,7 +112,10 @@ static int gc_thread_func(void *data)
> > > > > > >     				sbi->gc_mode == GC_URGENT_MID) {
> > > > > > >     			wait_ms = gc_th->urgent_sleep_time;
> > > > > > >     			f2fs_down_write(&sbi->gc_lock);
> > > > > > > +			gc_control.should_migrate_blocks = true;
> > > > > > >     			goto do_gc;
> > > > > > > +		} else {
> > > > > > > +			gc_control.should_migrate_blocks = false;
> > > > > > >     		}
> > > > > > >     		if (foreground) {
> > > > > > > @@ -139,7 +141,9 @@ static int gc_thread_func(void *data)
> > > > > > >     		if (!foreground)
> > > > > > >     			stat_inc_bggc_count(sbi->stat_info);
> > > > > > > -		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC;
> > > > > > > +		sync_mode = F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_SYNC ||
> > > > > > > +				sbi->gc_mode == GC_URGENT_HIGH ||
> > > > > > > +				sbi->gc_mode == GC_URGENT_MID;
> > > > > > >     		/* foreground GC was been triggered via f2fs_balance_fs() */
> > > > > > >     		if (foreground)
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel