Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1412800 > unrolled thread
| Started by | Jaegeuk Kim <jaegeuk@kernel.org> |
|---|---|
| First post | 2016-06-03 07:10 +0200 |
| Last post | 2016-06-03 07:20 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] f2fs: fix to redirty page if fail to gc data page Jaegeuk Kim <jaegeuk@kernel.org> - 2016-06-03 07:10 +0200
Re: [PATCH] f2fs: fix to redirty page if fail to gc data page Jaegeuk Kim <jaegeuk@kernel.org> - 2016-06-03 07:20 +0200
Re: [PATCH] f2fs: fix to redirty page if fail to gc data page Chao Yu <yuchao0@huawei.com> - 2016-06-03 08:00 +0200
Re: [PATCH] f2fs: fix to redirty page if fail to gc data page Jaegeuk Kim <jaegeuk@kernel.org> - 2016-06-03 19:40 +0200
Re: [PATCH] f2fs: fix to redirty page if fail to gc data page Chao Yu <yuchao0@huawei.com> - 2016-06-03 07:20 +0200
| From | Jaegeuk Kim <jaegeuk@kernel.org> |
|---|---|
| Date | 2016-06-03 07:10 +0200 |
| Subject | Re: [PATCH] f2fs: fix to redirty page if fail to gc data page |
| Message-ID | <rFPwR-7pG-3@gated-at.bofh.it> |
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 > > . > >
[toc] | [next] | [standalone]
| From | Jaegeuk Kim <jaegeuk@kernel.org> |
|---|---|
| Date | 2016-06-03 07:20 +0200 |
| Message-ID | <rFPGx-7sZ-1@gated-at.bofh.it> |
| In reply to | #1412800 |
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 > >>> . > >>> > > . > >
[toc] | [prev] | [next] | [standalone]
| From | Chao Yu <yuchao0@huawei.com> |
|---|---|
| Date | 2016-06-03 08:00 +0200 |
| Message-ID | <rFQjf-7FB-3@gated-at.bofh.it> |
| In reply to | #1412801 |
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 >>>>> . >>>>> >>> . >>> > . >
[toc] | [prev] | [next] | [standalone]
| From | Jaegeuk Kim <jaegeuk@kernel.org> |
|---|---|
| Date | 2016-06-03 19:40 +0200 |
| Message-ID | <rG1eG-6eI-17@gated-at.bofh.it> |
| In reply to | #1412825 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Chao Yu <yuchao0@huawei.com> |
|---|---|
| Date | 2016-06-03 07:20 +0200 |
| Message-ID | <rFPGx-7sZ-3@gated-at.bofh.it> |
| In reply to | #1412800 |
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 >>> . >>> > . >
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web