Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files

2019-09-08 Thread Chao Yu 
On 2019/9/9 9:25, Jaegeuk Kim wrote:
> If committing atomic pages is failed when doing f2fs_do_sync_file(), we can
> get commited pages but atomic_file being still set like:
> 
> - inmem:0, atomic IO:4 (Max.   10), volatile IO:0 (Max.0)
> 
> If GC selects this block, we can get an infinite loop like this:
> 
> f2fs_submit_page_bio: dev = (253,7), ino = 2, page_index = 0x2359a8, oldaddr 
> = 0x2359a8, newaddr = 0x2359a8, rw = READ(), type = COLD_DATA
> f2fs_submit_read_bio: dev = (253,7)/(253,7), rw = READ(), DATA, sector = 
> 18533696, size = 4096
> f2fs_get_victim: dev = (253,7), type = No TYPE, policy = (Foreground GC, 
> LFS-mode, Greedy), victim = 4355, cost = 1, ofs_unit = 1, pre_victim_secno = 
> 4355, prefree = 0, free = 234
> f2fs_iget: dev = (253,7), ino = 6247, pino = 5845, i_mode = 0x81b0, i_size = 
> 319488, i_nlink = 1, i_blocks = 624, i_advise = 0x2c
> f2fs_submit_page_bio: dev = (253,7), ino = 2, page_index = 0x2359a8, oldaddr 
> = 0x2359a8, newaddr = 0x2359a8, rw = READ(), type = COLD_DATA
> f2fs_submit_read_bio: dev = (253,7)/(253,7), rw = READ(), DATA, sector = 
> 18533696, size = 4096
> f2fs_get_victim: dev = (253,7), type = No TYPE, policy = (Foreground GC, 
> LFS-mode, Greedy), victim = 4355, cost = 1, ofs_unit = 1, pre_victim_secno = 
> 4355, prefree = 0, free = 234
> f2fs_iget: dev = (253,7), ino = 6247, pino = 5845, i_mode = 0x81b0, i_size = 
> 319488, i_nlink = 1, i_blocks = 624, i_advise = 0x2c
> 
> In that moment, we can observe:
> 
> [Before]
> Try to move 5084219 blocks (BG: 384508)
>   - data blocks : 4962373 (274483)
>   - node blocks : 121846 (110025)
> Skipped : atomic write 4534686 (10)
> 
> [After]
> Try to move 5088973 blocks (BG: 384508)
>   - data blocks : 4967127 (274483)
>   - node blocks : 121846 (110025)
> Skipped : atomic write 4539440 (10)
> 
> Signed-off-by: Jaegeuk Kim 
> ---
>  fs/f2fs/file.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 7ae2f3bd8c2f..68b6da734e5f 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1997,11 +1997,11 @@ static int f2fs_ioc_commit_atomic_write(struct file 
> *filp)
>   goto err_out;
>  
>   ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
> - if (!ret) {
> - clear_inode_flag(inode, FI_ATOMIC_FILE);
> - F2FS_I(inode)->i_gc_failures[GC_FAILURE_ATOMIC] = 0;
> - stat_dec_atomic_write(inode);
> - }
> +
> + /* doesn't need to check error */
> + clear_inode_flag(inode, FI_ATOMIC_FILE);
> + F2FS_I(inode)->i_gc_failures[GC_FAILURE_ATOMIC] = 0;
> + stat_dec_atomic_write(inode);

If there are still valid atomic write pages linked in .inmem_pages, it may cause
memory leak when we just clear FI_ATOMIC_FILE flag.

So my question is why below logic didn't handle such condition well?

f2fs_gc()

if (has_not_enough_free_secs(sbi, sec_freed, 0)) {
if (skipped_round <= MAX_SKIP_GC_COUNT ||
skipped_round * 2 < round) {
segno = NULL_SEGNO;
goto gc_more;
}

if (first_skipped < last_skipped &&
(last_skipped - first_skipped) >
sbi->skipped_gc_rwsem) {
f2fs_drop_inmem_pages_all(sbi, true);
segno = NULL_SEGNO;
goto gc_more;
}
if (gc_type == FG_GC && !is_sbi_flag_set(sbi, SBI_CP_DISABLED))
ret = f2fs_write_checkpoint(sbi, );
}

>   } else {
>   ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
>   }
> 


___
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/2] f2fs: do not select same victim right again

2019-09-08 Thread Chao Yu 
On 2019/9/9 9:25, Jaegeuk Kim wrote:
> GC must avoid select the same victim again.

Blocks in previous victim will occupy addition free segment, I doubt after this
change, FGGC may encounter out-of-free space issue more frequently.

Thanks,

> 
> Signed-off-by: Jaegeuk Kim 
> ---
>  fs/f2fs/gc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index e88f98ddf396..15ca8bbb0b22 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -274,6 +274,9 @@ static unsigned int get_cb_cost(struct f2fs_sb_info *sbi, 
> unsigned int segno)
>  static inline unsigned int get_gc_cost(struct f2fs_sb_info *sbi,
>   unsigned int segno, struct victim_sel_policy *p)
>  {
> + if (sbi->cur_victim_sec == GET_SEC_FROM_SEG(sbi, segno))
> + return UINT_MAX;
> +
>   if (p->alloc_mode == SSR)
>   return get_seg_entry(sbi, segno)->ckpt_valid_blocks;
>  
> @@ -1326,9 +1329,6 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>   round++;
>   }
>  
> - if (gc_type == FG_GC)
> - sbi->cur_victim_sec = NULL_SEGNO;
> -
>   if (sync)
>   goto stop;
>  
>

[f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files

2019-09-08 Thread Jaegeuk Kim 
If committing atomic pages is failed when doing f2fs_do_sync_file(), we can
get commited pages but atomic_file being still set like:

- inmem:0, atomic IO:4 (Max.   10), volatile IO:0 (Max.0)

If GC selects this block, we can get an infinite loop like this:

f2fs_submit_page_bio: dev = (253,7), ino = 2, page_index = 0x2359a8, oldaddr = 
0x2359a8, newaddr = 0x2359a8, rw = READ(), type = COLD_DATA
f2fs_submit_read_bio: dev = (253,7)/(253,7), rw = READ(), DATA, sector = 
18533696, size = 4096
f2fs_get_victim: dev = (253,7), type = No TYPE, policy = (Foreground GC, 
LFS-mode, Greedy), victim = 4355, cost = 1, ofs_unit = 1, pre_victim_secno = 
4355, prefree = 0, free = 234
f2fs_iget: dev = (253,7), ino = 6247, pino = 5845, i_mode = 0x81b0, i_size = 
319488, i_nlink = 1, i_blocks = 624, i_advise = 0x2c
f2fs_submit_page_bio: dev = (253,7), ino = 2, page_index = 0x2359a8, oldaddr = 
0x2359a8, newaddr = 0x2359a8, rw = READ(), type = COLD_DATA
f2fs_submit_read_bio: dev = (253,7)/(253,7), rw = READ(), DATA, sector = 
18533696, size = 4096
f2fs_get_victim: dev = (253,7), type = No TYPE, policy = (Foreground GC, 
LFS-mode, Greedy), victim = 4355, cost = 1, ofs_unit = 1, pre_victim_secno = 
4355, prefree = 0, free = 234
f2fs_iget: dev = (253,7), ino = 6247, pino = 5845, i_mode = 0x81b0, i_size = 
319488, i_nlink = 1, i_blocks = 624, i_advise = 0x2c

In that moment, we can observe:

[Before]
Try to move 5084219 blocks (BG: 384508)
  - data blocks : 4962373 (274483)
  - node blocks : 121846 (110025)
Skipped : atomic write 4534686 (10)

[After]
Try to move 5088973 blocks (BG: 384508)
  - data blocks : 4967127 (274483)
  - node blocks : 121846 (110025)
Skipped : atomic write 4539440 (10)

Signed-off-by: Jaegeuk Kim 
---
 fs/f2fs/file.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 7ae2f3bd8c2f..68b6da734e5f 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1997,11 +1997,11 @@ static int f2fs_ioc_commit_atomic_write(struct file 
*filp)
goto err_out;
 
ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
-   if (!ret) {
-   clear_inode_flag(inode, FI_ATOMIC_FILE);
-   F2FS_I(inode)->i_gc_failures[GC_FAILURE_ATOMIC] = 0;
-   stat_dec_atomic_write(inode);
-   }
+
+   /* doesn't need to check error */
+   clear_inode_flag(inode, FI_ATOMIC_FILE);
+   F2FS_I(inode)->i_gc_failures[GC_FAILURE_ATOMIC] = 0;
+   stat_dec_atomic_write(inode);
} else {
ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
}
-- 
2.19.0.605.g01d371f741-goog



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH 1/2] f2fs: do not select same victim right again

2019-09-08 Thread Jaegeuk Kim 
GC must avoid select the same victim again.

Signed-off-by: Jaegeuk Kim 
---
 fs/f2fs/gc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index e88f98ddf396..15ca8bbb0b22 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -274,6 +274,9 @@ static unsigned int get_cb_cost(struct f2fs_sb_info *sbi, 
unsigned int segno)
 static inline unsigned int get_gc_cost(struct f2fs_sb_info *sbi,
unsigned int segno, struct victim_sel_policy *p)
 {
+   if (sbi->cur_victim_sec == GET_SEC_FROM_SEG(sbi, segno))
+   return UINT_MAX;
+
if (p->alloc_mode == SSR)
return get_seg_entry(sbi, segno)->ckpt_valid_blocks;
 
@@ -1326,9 +1329,6 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
round++;
}
 
-   if (gc_type == FG_GC)
-   sbi->cur_victim_sec = NULL_SEGNO;
-
if (sync)
goto stop;
 
-- 
2.19.0.605.g01d371f741-goog