* [PATCH] f2fs: fix to redirty page if fail to gc data page @ 2016-05-21 5:19 Chao Yu 2016-05-28 0:54 ` Chao Yu 2016-05-30 2:37 ` Jaegeuk Kim 0 siblings, 2 replies; 9+ messages in thread From: Chao Yu @ 2016-05-21 5:19 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu From: Chao Yu <yuchao0@huawei.com> If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer. Signed-off-by: Chao Yu <yuchao0@huawei.com> --- fs/f2fs/gc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 38d56f6..ee213a8 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type) .page = page, .encrypted_page = NULL, }; + bool is_dirty = PageDirty(page); + set_page_dirty(page); f2fs_wait_on_page_writeback(page, DATA, true); if (clear_page_dirty_for_io(page)) inode_dec_dirty_pages(inode); set_cold_data(page); - do_write_data_page(&fio); + if (do_write_data_page(&fio) && is_dirty) + set_page_dirty(page); clear_cold_data(page); } out: -- 2.7.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] f2fs: fix to redirty page if fail to gc data page 2016-05-21 5:19 [PATCH] f2fs: fix to redirty page if fail to gc data page Chao Yu @ 2016-05-28 0:54 ` Chao Yu 2016-05-30 2:37 ` Jaegeuk Kim 1 sibling, 0 replies; 9+ messages in thread From: Chao Yu @ 2016-05-28 0:54 UTC (permalink / raw) To: Chao Yu, jaegeuk; +Cc: linux-f2fs-devel, linux-kernel Ping, On 2016/5/21 13:19, Chao Yu wrote: > From: Chao Yu <yuchao0@huawei.com> > > If we fail to move data page during foreground GC, we should give another > chance to writeback that page which was set dirty previously by writer. > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > --- > fs/f2fs/gc.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 38d56f6..ee213a8 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type) > .page = page, > .encrypted_page = NULL, > }; > + bool is_dirty = PageDirty(page); > + > set_page_dirty(page); > f2fs_wait_on_page_writeback(page, DATA, true); > if (clear_page_dirty_for_io(page)) > inode_dec_dirty_pages(inode); > set_cold_data(page); > - do_write_data_page(&fio); > + if (do_write_data_page(&fio) && is_dirty) > + set_page_dirty(page); > clear_cold_data(page); > } > out: > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] f2fs: fix to redirty page if fail to gc data page 2016-05-21 5:19 [PATCH] f2fs: fix to redirty page if fail to gc data page Chao Yu 2016-05-28 0:54 ` Chao Yu @ 2016-05-30 2:37 ` Jaegeuk Kim 2016-05-31 6:10 ` Chao Yu 1 sibling, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2016-05-30 2:37 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu Hi Chao, On Sat, May 21, 2016 at 01:19:11PM +0800, Chao Yu wrote: > From: Chao Yu <yuchao0@huawei.com> > > If we fail to move data page during foreground GC, we should give another > chance to writeback that page which was set dirty previously by writer. > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > --- > fs/f2fs/gc.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 38d56f6..ee213a8 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type) > .page = page, > .encrypted_page = NULL, > }; > + bool is_dirty = PageDirty(page); > + > set_page_dirty(page); > f2fs_wait_on_page_writeback(page, DATA, true); > if (clear_page_dirty_for_io(page)) > inode_dec_dirty_pages(inode); > set_cold_data(page); > - do_write_data_page(&fio); > + if (do_write_data_page(&fio) && is_dirty) > + set_page_dirty(page); If this page is truncated with -ENOENT, we don't need to set it dirty again. I expect that, if we get an error here, do_garbage_collect() would retry FG_GC again. Thanks, > clear_cold_data(page); > } > out: > -- > 2.7.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] f2fs: fix to redirty page if fail to gc data page 2016-05-30 2:37 ` Jaegeuk Kim @ 2016-05-31 6:10 ` Chao Yu 2016-06-03 5:08 ` Jaegeuk Kim 0 siblings, 1 reply; 9+ messages in thread From: Chao Yu @ 2016-05-31 6:10 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel Hi Jaegeuk, On 2016/5/30 10:37, Jaegeuk Kim wrote: > Hi Chao, > > On Sat, May 21, 2016 at 01:19:11PM +0800, Chao Yu wrote: >> From: Chao Yu <yuchao0@huawei.com> >> >> If we fail to move data page during foreground GC, we should give another >> chance to writeback that page which was set dirty previously by writer. >> >> Signed-off-by: Chao Yu <yuchao0@huawei.com> >> --- >> fs/f2fs/gc.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >> index 38d56f6..ee213a8 100644 >> --- a/fs/f2fs/gc.c >> +++ b/fs/f2fs/gc.c >> @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type) >> .page = page, >> .encrypted_page = NULL, >> }; >> + bool is_dirty = PageDirty(page); >> + >> set_page_dirty(page); >> f2fs_wait_on_page_writeback(page, DATA, true); >> if (clear_page_dirty_for_io(page)) >> inode_dec_dirty_pages(inode); >> set_cold_data(page); >> - do_write_data_page(&fio); >> + if (do_write_data_page(&fio) && is_dirty) >> + set_page_dirty(page); > > If this page is truncated with -ENOENT, we don't need to set it dirty again. Agree > I expect that, if we get an error here, do_garbage_collect() would retry FG_GC IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying do the movement just one time. Here, I think if there are just few of blocks are failed to be moved, we can give one more time for retrying. How do you think? > again. > > Thanks, > >> clear_cold_data(page); >> } >> out: >> -- >> 2.7.2 > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] f2fs: fix to redirty page if fail to gc data page 2016-05-31 6:10 ` Chao Yu @ 2016-06-03 5:08 ` Jaegeuk Kim 2016-06-03 5:13 ` Chao Yu 0 siblings, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2016-06-03 5:08 UTC (permalink / raw) To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel On Tue, May 31, 2016 at 02:10:50PM +0800, Chao Yu wrote: > Hi Jaegeuk, > > On 2016/5/30 10:37, Jaegeuk Kim wrote: > > Hi Chao, > > > > On Sat, May 21, 2016 at 01:19:11PM +0800, Chao Yu wrote: > >> From: Chao Yu <yuchao0@huawei.com> > >> > >> If we fail to move data page during foreground GC, we should give another > >> chance to writeback that page which was set dirty previously by writer. > >> > >> Signed-off-by: Chao Yu <yuchao0@huawei.com> > >> --- > >> fs/f2fs/gc.c | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > >> index 38d56f6..ee213a8 100644 > >> --- a/fs/f2fs/gc.c > >> +++ b/fs/f2fs/gc.c > >> @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type) > >> .page = page, > >> .encrypted_page = NULL, > >> }; > >> + bool is_dirty = PageDirty(page); > >> + > >> set_page_dirty(page); > >> f2fs_wait_on_page_writeback(page, DATA, true); > >> if (clear_page_dirty_for_io(page)) > >> inode_dec_dirty_pages(inode); > >> set_cold_data(page); > >> - do_write_data_page(&fio); > >> + if (do_write_data_page(&fio) && is_dirty) > >> + set_page_dirty(page); > > > > If this page is truncated with -ENOENT, we don't need to set it dirty again. > > Agree > > > I expect that, if we get an error here, do_garbage_collect() would retry FG_GC > > IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying > do the movement just one time. Here, I think if there are just few of blocks are > failed to be moved, we can give one more time for retrying. How do you think? Mostly I expected here -ENOENT caused by race condition. Do we have another expectation? Thanks, > > > again. > > > > Thanks, > > > >> clear_cold_data(page); > >> } > >> out: > >> -- > >> 2.7.2 > > . > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] f2fs: fix to redirty page if fail to gc data page 2016-06-03 5:08 ` Jaegeuk Kim @ 2016-06-03 5:13 ` Chao Yu 2016-06-03 5:17 ` Jaegeuk Kim 0 siblings, 1 reply; 9+ messages in thread From: Chao Yu @ 2016-06-03 5:13 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel On 2016/6/3 13:08, Jaegeuk Kim wrote: > On Tue, May 31, 2016 at 02:10:50PM +0800, Chao Yu wrote: >> Hi Jaegeuk, >> >> On 2016/5/30 10:37, Jaegeuk Kim wrote: >>> Hi Chao, >>> >>> On Sat, May 21, 2016 at 01:19:11PM +0800, Chao Yu wrote: >>>> From: Chao Yu <yuchao0@huawei.com> >>>> >>>> If we fail to move data page during foreground GC, we should give another >>>> chance to writeback that page which was set dirty previously by writer. >>>> >>>> Signed-off-by: Chao Yu <yuchao0@huawei.com> >>>> --- >>>> fs/f2fs/gc.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>>> index 38d56f6..ee213a8 100644 >>>> --- a/fs/f2fs/gc.c >>>> +++ b/fs/f2fs/gc.c >>>> @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type) >>>> .page = page, >>>> .encrypted_page = NULL, >>>> }; >>>> + bool is_dirty = PageDirty(page); >>>> + >>>> set_page_dirty(page); >>>> f2fs_wait_on_page_writeback(page, DATA, true); >>>> if (clear_page_dirty_for_io(page)) >>>> inode_dec_dirty_pages(inode); >>>> set_cold_data(page); >>>> - do_write_data_page(&fio); >>>> + if (do_write_data_page(&fio) && is_dirty) >>>> + set_page_dirty(page); >>> >>> If this page is truncated with -ENOENT, we don't need to set it dirty again. >> >> Agree >> >>> I expect that, if we get an error here, do_garbage_collect() would retry FG_GC >> >> IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying >> do the movement just one time. Here, I think if there are just few of blocks are >> failed to be moved, we can give one more time for retrying. How do you think? > > Mostly I expected here -ENOENT caused by race condition. If we hit ENOENT case, we can pass get_valid_blocks check, so we don't need to worry about this case, right? > Do we have another expectation? ENOMEM or EIO? Thanks, > > Thanks, > >> >>> again. >>> >>> Thanks, >>> >>>> clear_cold_data(page); >>>> } >>>> out: >>>> -- >>>> 2.7.2 >>> . >>> > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] f2fs: fix to redirty page if fail to gc data page 2016-06-03 5:13 ` Chao Yu @ 2016-06-03 5:17 ` Jaegeuk Kim 2016-06-03 5:59 ` Chao Yu 0 siblings, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2016-06-03 5:17 UTC (permalink / raw) To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel On Fri, Jun 03, 2016 at 01:13:21PM +0800, Chao Yu wrote: > On 2016/6/3 13:08, Jaegeuk Kim wrote: > > On Tue, May 31, 2016 at 02:10:50PM +0800, Chao Yu wrote: > >> Hi Jaegeuk, > >> > >> On 2016/5/30 10:37, Jaegeuk Kim wrote: > >>> Hi Chao, > >>> > >>> On Sat, May 21, 2016 at 01:19:11PM +0800, Chao Yu wrote: > >>>> From: Chao Yu <yuchao0@huawei.com> > >>>> > >>>> If we fail to move data page during foreground GC, we should give another > >>>> chance to writeback that page which was set dirty previously by writer. > >>>> > >>>> Signed-off-by: Chao Yu <yuchao0@huawei.com> > >>>> --- > >>>> fs/f2fs/gc.c | 5 ++++- > >>>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > >>>> index 38d56f6..ee213a8 100644 > >>>> --- a/fs/f2fs/gc.c > >>>> +++ b/fs/f2fs/gc.c > >>>> @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type) > >>>> .page = page, > >>>> .encrypted_page = NULL, > >>>> }; > >>>> + bool is_dirty = PageDirty(page); > >>>> + > >>>> set_page_dirty(page); > >>>> f2fs_wait_on_page_writeback(page, DATA, true); > >>>> if (clear_page_dirty_for_io(page)) > >>>> inode_dec_dirty_pages(inode); > >>>> set_cold_data(page); > >>>> - do_write_data_page(&fio); > >>>> + if (do_write_data_page(&fio) && is_dirty) > >>>> + set_page_dirty(page); > >>> > >>> If this page is truncated with -ENOENT, we don't need to set it dirty again. > >> > >> Agree > >> > >>> I expect that, if we get an error here, do_garbage_collect() would retry FG_GC > >> > >> IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying > >> do the movement just one time. Here, I think if there are just few of blocks are > >> failed to be moved, we can give one more time for retrying. How do you think? > > > > Mostly I expected here -ENOENT caused by race condition. > > If we hit ENOENT case, we can pass get_valid_blocks check, so we don't need to > worry about this case, right? > > > Do we have another expectation? > > ENOMEM or EIO? EIO will stop everything. ENOMEM would be better to wait for a while from page reclaim? > > Thanks, > > > > > Thanks, > > > >> > >>> again. > >>> > >>> Thanks, > >>> > >>>> clear_cold_data(page); > >>>> } > >>>> out: > >>>> -- > >>>> 2.7.2 > >>> . > >>> > > . > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] f2fs: fix to redirty page if fail to gc data page 2016-06-03 5:17 ` Jaegeuk Kim @ 2016-06-03 5:59 ` Chao Yu 2016-06-03 17:36 ` Jaegeuk Kim 0 siblings, 1 reply; 9+ messages in thread From: Chao Yu @ 2016-06-03 5:59 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel On 2016/6/3 13:17, Jaegeuk Kim wrote: > On Fri, Jun 03, 2016 at 01:13:21PM +0800, Chao Yu wrote: >> On 2016/6/3 13:08, Jaegeuk Kim wrote: >>> On Tue, May 31, 2016 at 02:10:50PM +0800, Chao Yu wrote: >>>> Hi Jaegeuk, >>>> >>>> On 2016/5/30 10:37, Jaegeuk Kim wrote: >>>>> Hi Chao, >>>>> >>>>> On Sat, May 21, 2016 at 01:19:11PM +0800, Chao Yu wrote: >>>>>> From: Chao Yu <yuchao0@huawei.com> >>>>>> >>>>>> If we fail to move data page during foreground GC, we should give another >>>>>> chance to writeback that page which was set dirty previously by writer. >>>>>> >>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com> >>>>>> --- >>>>>> fs/f2fs/gc.c | 5 ++++- >>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>>>>> index 38d56f6..ee213a8 100644 >>>>>> --- a/fs/f2fs/gc.c >>>>>> +++ b/fs/f2fs/gc.c >>>>>> @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type) >>>>>> .page = page, >>>>>> .encrypted_page = NULL, >>>>>> }; >>>>>> + bool is_dirty = PageDirty(page); >>>>>> + >>>>>> set_page_dirty(page); >>>>>> f2fs_wait_on_page_writeback(page, DATA, true); >>>>>> if (clear_page_dirty_for_io(page)) >>>>>> inode_dec_dirty_pages(inode); >>>>>> set_cold_data(page); >>>>>> - do_write_data_page(&fio); >>>>>> + if (do_write_data_page(&fio) && is_dirty) >>>>>> + set_page_dirty(page); >>>>> >>>>> If this page is truncated with -ENOENT, we don't need to set it dirty again. >>>> >>>> Agree >>>> >>>>> I expect that, if we get an error here, do_garbage_collect() would retry FG_GC >>>> >>>> IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying >>>> do the movement just one time. Here, I think if there are just few of blocks are >>>> failed to be moved, we can give one more time for retrying. How do you think? >>> >>> Mostly I expected here -ENOENT caused by race condition. >> >> If we hit ENOENT case, we can pass get_valid_blocks check, so we don't need to >> worry about this case, right? >> >>> Do we have another expectation? >> >> ENOMEM or EIO? > > EIO will stop everything. > ENOMEM would be better to wait for a while from page reclaim? Agree, but for ioctl path, IMO, we don't need to let user waiting for ENOMEM case looping. > >> >> Thanks, >> >>> >>> Thanks, >>> >>>> >>>>> again. >>>>> >>>>> Thanks, >>>>> >>>>>> clear_cold_data(page); >>>>>> } >>>>>> out: >>>>>> -- >>>>>> 2.7.2 >>>>> . >>>>> >>> . >>> > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] f2fs: fix to redirty page if fail to gc data page 2016-06-03 5:59 ` Chao Yu @ 2016-06-03 17:36 ` Jaegeuk Kim 0 siblings, 0 replies; 9+ messages in thread From: Jaegeuk Kim @ 2016-06-03 17:36 UTC (permalink / raw) To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel On Fri, Jun 03, 2016 at 01:59:15PM +0800, Chao Yu wrote: ... > >>> Do we have another expectation? > >> > >> ENOMEM or EIO? > > > > EIO will stop everything. > > ENOMEM would be better to wait for a while from page reclaim? > > Agree, but for ioctl path, IMO, we don't need to let user waiting for ENOMEM > case looping. Well, if user wanted to do a synchronous gc, we need that, IMO. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-06-03 17:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-21 5:19 [PATCH] f2fs: fix to redirty page if fail to gc data page Chao Yu 2016-05-28 0:54 ` Chao Yu 2016-05-30 2:37 ` Jaegeuk Kim 2016-05-31 6:10 ` Chao Yu 2016-06-03 5:08 ` Jaegeuk Kim 2016-06-03 5:13 ` Chao Yu 2016-06-03 5:17 ` Jaegeuk Kim 2016-06-03 5:59 ` Chao Yu 2016-06-03 17:36 ` Jaegeuk Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).