Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 2019/9/10 20:09, Jaegeuk Kim wrote: >> The lock is used to protect F2FS_I(inode)->inmem_pages list... it should be >> kept? > Urg.. yup. I added. Reviewed-by: Chao Yu Thanks, >
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 09/10, Chao Yu wrote:
> On 2019/9/10 19:58, Jaegeuk Kim wrote:
> > On 09/10, Chao Yu wrote:
> >> On 2019/9/9 22:34, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> On 2019/9/9 16:38, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> On 2019/9/9 16:21, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> On 2019/9/9 16:01, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> On 2019/9/9 15:30, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> 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.
> >>>
> >>> f2fs_commit_inmem_pages() should have flushed them.
> >>
> >> Oh, we failed to flush its nodes.
> >>
> >> However we won't clear such info if we failed to flush inmen
> >> pages, it looks
> >> inconsistent.
> >>
> >> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag
> >> in that two
> >> error path? I'm not very clear how sqlite
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 2019/9/10 19:58, Jaegeuk Kim wrote:
> On 09/10, Chao Yu wrote:
>> On 2019/9/9 22:34, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
On 2019/9/9 16:38, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> On 2019/9/9 16:21, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
On 2019/9/9 16:01, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> On 2019/9/9 15:30, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
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.
>>>
>>> f2fs_commit_inmem_pages() should have flushed them.
>>
>> Oh, we failed to flush its nodes.
>>
>> However we won't clear such info if we failed to flush inmen pages,
>> it looks
>> inconsistent.
>>
>> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag
>> in that two
>> error path? I'm not very clear how sqlite handle such error.
>
> f2fs_drop_inmem_pages() did that, but not in this case.
What I mean is, for any error returned from atomic_commit() interface,
should
userspace application handle it with consistent way, like trigger
f2fs_drop_inmem_pages(), so we don't need
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 09/10, Chao Yu wrote:
> On 2019/9/9 22:34, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> On 2019/9/9 16:38, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> On 2019/9/9 16:21, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> On 2019/9/9 16:01, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> On 2019/9/9 15:30, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> 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.
> >
> > f2fs_commit_inmem_pages() should have flushed them.
>
> Oh, we failed to flush its nodes.
>
> However we won't clear such info if we failed to flush inmen pages,
> it looks
> inconsistent.
>
> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag
> in that two
> error path? I'm not very clear how sqlite handle such error.
> >>>
> >>> f2fs_drop_inmem_pages() did that, but not in this case.
> >>
> >> What I mean is, for any error returned from atomic_commit() interface,
> >> should
> >> userspace application handle it with consistent way, like trigger
> >> f2fs_drop_inmem_pages(), so we don't need to handle it inside
> >>
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 2019/9/9 22:34, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> On 2019/9/9 16:38, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
On 2019/9/9 16:21, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> On 2019/9/9 16:01, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
On 2019/9/9 15:30, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> 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.
>
> f2fs_commit_inmem_pages() should have flushed them.
Oh, we failed to flush its nodes.
However we won't clear such info if we failed to flush inmen pages, it
looks
inconsistent.
Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in
that two
error path? I'm not very clear how sqlite handle such error.
>>>
>>> f2fs_drop_inmem_pages() did that, but not in this case.
>>
>> What I mean is, for any error returned from atomic_commit() interface,
>> should
>> userspace application handle it with consistent way, like trigger
>> f2fs_drop_inmem_pages(), so we don't need to handle it inside
>> atomic_commit().
>
> f2fs_ioc_abort_volatile_write() will be triggered.
If userspace can do this, we can get rid of this patch, or am I missing
sth?
>>>
>>> We don't know when that will come. And, other threads are waiting for GC
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 09/09, Chao Yu wrote:
> On 2019/9/9 16:38, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> On 2019/9/9 16:21, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> On 2019/9/9 16:01, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> On 2019/9/9 15:30, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> 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.
> >>>
> >>> f2fs_commit_inmem_pages() should have flushed them.
> >>
> >> Oh, we failed to flush its nodes.
> >>
> >> However we won't clear such info if we failed to flush inmen pages, it
> >> looks
> >> inconsistent.
> >>
> >> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in
> >> that two
> >> error path? I'm not very clear how sqlite handle such error.
> >
> > f2fs_drop_inmem_pages() did that, but not in this case.
>
> What I mean is, for any error returned from atomic_commit() interface,
> should
> userspace application handle it with consistent way, like trigger
> f2fs_drop_inmem_pages(), so we don't need to handle it inside
> atomic_commit().
> >>>
> >>> f2fs_ioc_abort_volatile_write() will be triggered.
> >>
> >> If userspace can do this, we can get rid of this patch, or am I missing
> >> sth?
> >
> > We don't know when that will come. And, other threads are waiting for GC
> > here.
>
> Yes, however, even
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 2019/9/9 16:38, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> On 2019/9/9 16:21, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
On 2019/9/9 16:01, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> On 2019/9/9 15:30, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
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.
>>>
>>> f2fs_commit_inmem_pages() should have flushed them.
>>
>> Oh, we failed to flush its nodes.
>>
>> However we won't clear such info if we failed to flush inmen pages, it
>> looks
>> inconsistent.
>>
>> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in
>> that two
>> error path? I'm not very clear how sqlite handle such error.
>
> f2fs_drop_inmem_pages() did that, but not in this case.
What I mean is, for any error returned from atomic_commit() interface,
should
userspace application handle it with consistent way, like trigger
f2fs_drop_inmem_pages(), so we don't need to handle it inside
atomic_commit().
>>>
>>> f2fs_ioc_abort_volatile_write() will be triggered.
>>
>> If userspace can do this, we can get rid of this patch, or am I missing sth?
>
> We don't know when that will come. And, other threads are waiting for GC here.
Yes, however, even atomic_write won't be called sometimes... that's why we add
handling logic in f2fs_gc().
>
>>
>> - f2fs_ioc_abort_volatile_write
>> - f2fs_drop_inmem_pages
>> - clear_inode_flag(inode, FI_ATOMIC_FILE);
>> - fi->i_gc_failures[GC_FAILURE_ATOMIC] = 0;
>>
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 09/09, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
> > On 2019/9/9 16:21, Jaegeuk Kim wrote:
> > > On 09/09, Chao Yu wrote:
> > >> On 2019/9/9 16:01, Jaegeuk Kim wrote:
> > >>> On 09/09, Chao Yu wrote:
> > On 2019/9/9 15:30, Jaegeuk Kim wrote:
> > > On 09/09, Chao Yu wrote:
> > >> 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.
> > >
> > > f2fs_commit_inmem_pages() should have flushed them.
> >
> > Oh, we failed to flush its nodes.
> >
> > However we won't clear such info if we failed to flush inmen pages, it
> > looks
> > inconsistent.
> >
> > Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in
> > that two
> > error path? I'm not very clear how sqlite handle such error.
> > >>>
> > >>> f2fs_drop_inmem_pages() did that, but not in this case.
> > >>
> > >> What I mean is, for any error returned from atomic_commit() interface,
> > >> should
> > >> userspace application handle it with consistent way, like trigger
> > >> f2fs_drop_inmem_pages(), so we don't need to handle it inside
> > >> atomic_commit().
> > >
> > > f2fs_ioc_abort_volatile_write() will be triggered.
> >
> > If userspace can do this, we can get rid of this patch, or am I missing sth?
>
> We don't know when that will come. And, other threads are waiting for GC here.
>
Actually, we can call this.
---
fs/f2fs/file.c | 6 +-
1 file changed, 1
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 09/09, Chao Yu wrote:
> On 2019/9/9 16:21, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> On 2019/9/9 16:01, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> On 2019/9/9 15:30, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> 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.
> >
> > f2fs_commit_inmem_pages() should have flushed them.
>
> Oh, we failed to flush its nodes.
>
> However we won't clear such info if we failed to flush inmen pages, it
> looks
> inconsistent.
>
> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in
> that two
> error path? I'm not very clear how sqlite handle such error.
> >>>
> >>> f2fs_drop_inmem_pages() did that, but not in this case.
> >>
> >> What I mean is, for any error returned from atomic_commit() interface,
> >> should
> >> userspace application handle it with consistent way, like trigger
> >> f2fs_drop_inmem_pages(), so we don't need to handle it inside
> >> atomic_commit().
> >
> > f2fs_ioc_abort_volatile_write() will be triggered.
>
> If userspace can do this, we can get rid of this patch, or am I missing sth?
We don't know when that will come. And, other threads are waiting for GC here.
>
> - f2fs_ioc_abort_volatile_write
> - f2fs_drop_inmem_pages
> - clear_inode_flag(inode, FI_ATOMIC_FILE);
> - fi->i_gc_failures[GC_FAILURE_ATOMIC] = 0;
> - stat_dec_atomic_write(inode);
>
> >
> >>
> >>>
>
> Thanks,
>
> >
> >>
> >> So my question is why below logic didn't handle such
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 2019/9/9 16:21, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> On 2019/9/9 16:01, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
On 2019/9/9 15:30, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> 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.
>
> f2fs_commit_inmem_pages() should have flushed them.
Oh, we failed to flush its nodes.
However we won't clear such info if we failed to flush inmen pages, it
looks
inconsistent.
Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in that
two
error path? I'm not very clear how sqlite handle such error.
>>>
>>> f2fs_drop_inmem_pages() did that, but not in this case.
>>
>> What I mean is, for any error returned from atomic_commit() interface, should
>> userspace application handle it with consistent way, like trigger
>> f2fs_drop_inmem_pages(), so we don't need to handle it inside
>> atomic_commit().
>
> f2fs_ioc_abort_volatile_write() will be triggered.
If userspace can do this, we can get rid of this patch, or am I missing sth?
- f2fs_ioc_abort_volatile_write
- f2fs_drop_inmem_pages
- clear_inode_flag(inode, FI_ATOMIC_FILE);
- fi->i_gc_failures[GC_FAILURE_ATOMIC] = 0;
- stat_dec_atomic_write(inode);
>
>>
>>>
Thanks,
>
>>
>> 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;
>>
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 09/09, Chao Yu wrote:
> On 2019/9/9 16:01, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> On 2019/9/9 15:30, Jaegeuk Kim wrote:
> >>> On 09/09, Chao Yu wrote:
> 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.
> >>>
> >>> f2fs_commit_inmem_pages() should have flushed them.
> >>
> >> Oh, we failed to flush its nodes.
> >>
> >> However we won't clear such info if we failed to flush inmen pages, it
> >> looks
> >> inconsistent.
> >>
> >> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in that
> >> two
> >> error path? I'm not very clear how sqlite handle such error.
> >
> > f2fs_drop_inmem_pages() did that, but not in this case.
>
> What I mean is, for any error returned from atomic_commit() interface, should
> userspace application handle it with consistent way, like trigger
> f2fs_drop_inmem_pages(), so we don't need to handle it inside atomic_commit().
f2fs_ioc_abort_volatile_write() will be triggered.
>
> >
> >>
> >> Thanks,
> >>
> >>>
>
> 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);
> >>>
> >>> This is doing nothing, since
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 2019/9/9 16:01, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> On 2019/9/9 15:30, Jaegeuk Kim wrote:
>>> On 09/09, Chao Yu wrote:
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.
>>>
>>> f2fs_commit_inmem_pages() should have flushed them.
>>
>> Oh, we failed to flush its nodes.
>>
>> However we won't clear such info if we failed to flush inmen pages, it looks
>> inconsistent.
>>
>> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in that
>> two
>> error path? I'm not very clear how sqlite handle such error.
>
> f2fs_drop_inmem_pages() did that, but not in this case.
What I mean is, for any error returned from atomic_commit() interface, should
userspace application handle it with consistent way, like trigger
f2fs_drop_inmem_pages(), so we don't need to handle it inside atomic_commit().
>
>>
>> Thanks,
>>
>>>
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);
>>>
>>> This is doing nothing, since f2fs_commit_inmem_pages() removed the inode
>>> from inmem list.
>>>
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,
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 09/09, Chao Yu wrote:
> On 2019/9/9 15:30, Jaegeuk Kim wrote:
> > On 09/09, Chao Yu wrote:
> >> 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.
> >
> > f2fs_commit_inmem_pages() should have flushed them.
>
> Oh, we failed to flush its nodes.
>
> However we won't clear such info if we failed to flush inmen pages, it looks
> inconsistent.
>
> Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in that two
> error path? I'm not very clear how sqlite handle such error.
f2fs_drop_inmem_pages() did that, but not in this case.
>
> Thanks,
>
> >
> >>
> >> 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);
> >
> > This is doing nothing, since f2fs_commit_inmem_pages() removed the inode
> > from inmem list.
> >
> >>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);
> >>> }
> >>>
> > .
> >
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 2019/9/9 15:30, Jaegeuk Kim wrote:
> On 09/09, Chao Yu wrote:
>> 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.
>
> f2fs_commit_inmem_pages() should have flushed them.
Oh, we failed to flush its nodes.
However we won't clear such info if we failed to flush inmen pages, it looks
inconsistent.
Any interface needed to drop inmem pages or clear ATOMIC_FILE flag in that two
error path? I'm not very clear how sqlite handle such error.
Thanks,
>
>>
>> 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);
>
> This is doing nothing, since f2fs_commit_inmem_pages() removed the inode
> from inmem list.
>
>> 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 2/2] f2fs: avoid infinite GC loop due to stale atomic files
On 09/09, Chao Yu wrote:
> 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.
f2fs_commit_inmem_pages() should have flushed them.
>
> 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);
This is doing nothing, since f2fs_commit_inmem_pages() removed the inode
from inmem list.
> 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);
> > }
> >
Re: [f2fs-dev] [PATCH 2/2] f2fs: avoid infinite GC loop due to stale atomic files
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
