* [PATCH 4/5] f2fs: optimize restore_node_summary slightly
@ 2014-03-07 10:43 Gu Zheng
2014-03-10 4:45 ` Jaegeuk Kim
0 siblings, 1 reply; 6+ messages in thread
From: Gu Zheng @ 2014-03-07 10:43 UTC (permalink / raw)
To: Kim; +Cc: linux-kernel, f2fs
Previously, we ra_sum_pages to pre-read contiguous pages as more
as possible, and if we fail to alloc more pages, an ENOMEM error
will be reported upstream, even though we have alloced some pages
yet. In fact, we can use the available pages to do the job partly,
and continue the rest in the following circle. Only reporting ENOMEM
upstream if we really can not alloc any available page.
And another fix is ignoring dealing with the following pages if an
EIO occurs when reading page from page_list.
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
fs/f2fs/node.c | 44 ++++++++++++++++++++------------------------
fs/f2fs/segment.c | 7 +++++--
2 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 8787469..4b7861d 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head *pages,
for (; page_idx < start + nrpages; page_idx++) {
/* alloc temporal page for read node summary info*/
page = alloc_page(GFP_F2FS_ZERO);
- if (!page) {
- struct page *tmp;
- list_for_each_entry_safe(page, tmp, pages, lru) {
- list_del(&page->lru);
- unlock_page(page);
- __free_pages(page, 0);
- }
- return -ENOMEM;
- }
+ if (!page)
+ break;
lock_page(page);
page->index = page_idx;
@@ -1607,7 +1600,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head *pages,
f2fs_submit_page_mbio(sbi, page, page->index, &fio);
f2fs_submit_merged_bio(sbi, META, READ);
- return 0;
+
+ return page_idx - start;
}
int restore_node_summary(struct f2fs_sb_info *sbi,
@@ -1630,28 +1624,30 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
nrpages = min(last_offset - i, bio_blocks);
/* read ahead node pages */
- err = ra_sum_pages(sbi, &page_list, addr, nrpages);
- if (err)
- return err;
+ nrpages = ra_sum_pages(sbi, &page_list, addr, nrpages);
+ if (!nrpages)
+ return -ENOMEM;
list_for_each_entry_safe(page, tmp, &page_list, lru) {
-
- lock_page(page);
- if (unlikely(!PageUptodate(page))) {
- err = -EIO;
- } else {
- rn = F2FS_NODE(page);
- sum_entry->nid = rn->footer.nid;
- sum_entry->version = 0;
- sum_entry->ofs_in_node = 0;
- sum_entry++;
+ if (!err) {
+ lock_page(page);
+ if (unlikely(!PageUptodate(page))) {
+ err = -EIO;
+ } else {
+ rn = F2FS_NODE(page);
+ sum_entry->nid = rn->footer.nid;
+ sum_entry->version = 0;
+ sum_entry->ofs_in_node = 0;
+ sum_entry++;
+ }
+ unlock_page(page);
}
list_del(&page->lru);
- unlock_page(page);
__free_pages(page, 0);
}
}
+
return err;
}
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 199c964..b3f8431 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1160,9 +1160,12 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type)
ns->ofs_in_node = 0;
}
} else {
- if (restore_node_summary(sbi, segno, sum)) {
+ int err;
+
+ err = restore_node_summary(sbi, segno, sum);
+ if (err) {
f2fs_put_page(new, 1);
- return -EINVAL;
+ return err;
}
}
}
--
1.7.7
------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works.
Faster operations. Version large binaries. Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] f2fs: optimize restore_node_summary slightly
2014-03-07 10:43 [PATCH 4/5] f2fs: optimize restore_node_summary slightly Gu Zheng
@ 2014-03-10 4:45 ` Jaegeuk Kim
2014-03-10 5:13 ` Chao Yu
2014-03-10 5:38 ` Gu Zheng
0 siblings, 2 replies; 6+ messages in thread
From: Jaegeuk Kim @ 2014-03-10 4:45 UTC (permalink / raw)
To: Gu Zheng; +Cc: f2fs, linux-kernel
Hi Gu,
2014-03-07 (금), 18:43 +0800, Gu Zheng:
> Previously, we ra_sum_pages to pre-read contiguous pages as more
> as possible, and if we fail to alloc more pages, an ENOMEM error
> will be reported upstream, even though we have alloced some pages
> yet. In fact, we can use the available pages to do the job partly,
> and continue the rest in the following circle. Only reporting ENOMEM
> upstream if we really can not alloc any available page.
>
> And another fix is ignoring dealing with the following pages if an
> EIO occurs when reading page from page_list.
>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
> fs/f2fs/node.c | 44 ++++++++++++++++++++------------------------
> fs/f2fs/segment.c | 7 +++++--
> 2 files changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 8787469..4b7861d 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head *pages,
> for (; page_idx < start + nrpages; page_idx++) {
> /* alloc temporal page for read node summary info*/
> page = alloc_page(GFP_F2FS_ZERO);
> - if (!page) {
> - struct page *tmp;
> - list_for_each_entry_safe(page, tmp, pages, lru) {
> - list_del(&page->lru);
> - unlock_page(page);
> - __free_pages(page, 0);
> - }
> - return -ENOMEM;
> - }
> + if (!page)
> + break;
>
> lock_page(page);
> page->index = page_idx;
> @@ -1607,7 +1600,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head *pages,
> f2fs_submit_page_mbio(sbi, page, page->index, &fio);
>
> f2fs_submit_merged_bio(sbi, META, READ);
> - return 0;
> +
> + return page_idx - start;
> }
>
> int restore_node_summary(struct f2fs_sb_info *sbi,
> @@ -1630,28 +1624,30 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
> nrpages = min(last_offset - i, bio_blocks);
>
> /* read ahead node pages */
> - err = ra_sum_pages(sbi, &page_list, addr, nrpages);
> - if (err)
> - return err;
> + nrpages = ra_sum_pages(sbi, &page_list, addr, nrpages);
> + if (!nrpages)
> + return -ENOMEM;
>
> list_for_each_entry_safe(page, tmp, &page_list, lru) {
> -
Here we can just add:
if (err)
goto skip;
lock_page();
...
unlock_page();
skip:
list_del();
__free_pages();
IMO, it's more neat, so if you have any objection, let me know.
Otherwise, I'll handle this by myself. :)
Thanks,
> - lock_page(page);
> - if (unlikely(!PageUptodate(page))) {
> - err = -EIO;
> - } else {
> - rn = F2FS_NODE(page);
> - sum_entry->nid = rn->footer.nid;
> - sum_entry->version = 0;
> - sum_entry->ofs_in_node = 0;
> - sum_entry++;
> + if (!err) {
> + lock_page(page);
> + if (unlikely(!PageUptodate(page))) {
> + err = -EIO;
> + } else {
> + rn = F2FS_NODE(page);
> + sum_entry->nid = rn->footer.nid;
> + sum_entry->version = 0;
> + sum_entry->ofs_in_node = 0;
> + sum_entry++;
> + }
> + unlock_page(page);
> }
>
> list_del(&page->lru);
> - unlock_page(page);
> __free_pages(page, 0);
> }
> }
> +
> return err;
> }
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 199c964..b3f8431 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1160,9 +1160,12 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type)
> ns->ofs_in_node = 0;
> }
> } else {
> - if (restore_node_summary(sbi, segno, sum)) {
> + int err;
> +
> + err = restore_node_summary(sbi, segno, sum);
> + if (err) {
> f2fs_put_page(new, 1);
> - return -EINVAL;
> + return err;
> }
> }
> }
--
Jaegeuk Kim
Samsung
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] f2fs: optimize restore_node_summary slightly
2014-03-10 4:45 ` Jaegeuk Kim
@ 2014-03-10 5:13 ` Chao Yu
2014-03-10 5:37 ` Jaegeuk Kim
2014-03-10 5:38 ` Gu Zheng
1 sibling, 1 reply; 6+ messages in thread
From: Chao Yu @ 2014-03-10 5:13 UTC (permalink / raw)
To: 'Gu Zheng', jaegeuk.kim; +Cc: 'linux-kernel', 'f2fs'
Hi Gu, Kim:
One more comment.
> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk.kim@samsung.com]
> Sent: Monday, March 10, 2014 12:46 PM
> To: Gu Zheng
> Cc: linux-kernel; f2fs
> Subject: Re: [f2fs-dev] [PATCH 4/5] f2fs: optimize restore_node_summary slightly
>
> Hi Gu,
>
> 2014-03-07 (금), 18:43 +0800, Gu Zheng:
> > Previously, we ra_sum_pages to pre-read contiguous pages as more
> > as possible, and if we fail to alloc more pages, an ENOMEM error
> > will be reported upstream, even though we have alloced some pages
> > yet. In fact, we can use the available pages to do the job partly,
> > and continue the rest in the following circle. Only reporting ENOMEM
> > upstream if we really can not alloc any available page.
> >
> > And another fix is ignoring dealing with the following pages if an
> > EIO occurs when reading page from page_list.
> >
> > Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Reviewed-by: Chao Yu <chao2.yu@samsung.com>
> > ---
> > fs/f2fs/node.c | 44 ++++++++++++++++++++------------------------
> > fs/f2fs/segment.c | 7 +++++--
> > 2 files changed, 25 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 8787469..4b7861d 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
> *pages,
> > for (; page_idx < start + nrpages; page_idx++) {
> > /* alloc temporal page for read node summary info*/
> > page = alloc_page(GFP_F2FS_ZERO);
> > - if (!page) {
> > - struct page *tmp;
> > - list_for_each_entry_safe(page, tmp, pages, lru) {
> > - list_del(&page->lru);
> > - unlock_page(page);
> > - __free_pages(page, 0);
> > - }
> > - return -ENOMEM;
> > - }
> > + if (!page)
> > + break;
> >
> > lock_page(page);
> > page->index = page_idx;
> > @@ -1607,7 +1600,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
> *pages,
> > f2fs_submit_page_mbio(sbi, page, page->index, &fio);
> >
> > f2fs_submit_merged_bio(sbi, META, READ);
> > - return 0;
> > +
> > + return page_idx - start;
> > }
> >
> > int restore_node_summary(struct f2fs_sb_info *sbi,
> > @@ -1630,28 +1624,30 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
> > nrpages = min(last_offset - i, bio_blocks);
> >
> > /* read ahead node pages */
> > - err = ra_sum_pages(sbi, &page_list, addr, nrpages);
> > - if (err)
> > - return err;
> > + nrpages = ra_sum_pages(sbi, &page_list, addr, nrpages);
> > + if (!nrpages)
> > + return -ENOMEM;
> >
> > list_for_each_entry_safe(page, tmp, &page_list, lru) {
> > -
>
> Here we can just add:
> if (err)
> goto skip;
> lock_page();
> ...
> unlock_page();
> skip:
> list_del();
> __free_pages();
>
> IMO, it's more neat, so if you have any objection, let me know.
> Otherwise, I'll handle this by myself. :)
> Thanks,
>
> > - lock_page(page);
> > - if (unlikely(!PageUptodate(page))) {
> > - err = -EIO;
> > - } else {
> > - rn = F2FS_NODE(page);
> > - sum_entry->nid = rn->footer.nid;
> > - sum_entry->version = 0;
> > - sum_entry->ofs_in_node = 0;
> > - sum_entry++;
> > + if (!err) {
If we skip here, next round we will fill these summary page entries with
wrong info because we skip the code 'sum_entry++;'.
> > + lock_page(page);
> > + if (unlikely(!PageUptodate(page))) {
> > + err = -EIO;
> > + } else {
> > + rn = F2FS_NODE(page);
> > + sum_entry->nid = rn->footer.nid;
> > + sum_entry->version = 0;
> > + sum_entry->ofs_in_node = 0;
> > + sum_entry++;
> > + }
> > + unlock_page(page);
> > }
> >
> > list_del(&page->lru);
> > - unlock_page(page);
> > __free_pages(page, 0);
> > }
Maybe we should add code here.
if (err)
return err;
> > }
> > +
> > return err;
> > }
> >
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 199c964..b3f8431 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1160,9 +1160,12 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type)
> > ns->ofs_in_node = 0;
> > }
> > } else {
> > - if (restore_node_summary(sbi, segno, sum)) {
> > + int err;
> > +
> > + err = restore_node_summary(sbi, segno, sum);
> > + if (err) {
> > f2fs_put_page(new, 1);
> > - return -EINVAL;
> > + return err;
> > }
> > }
> > }
>
> --
> Jaegeuk Kim
> Samsung
>
>
> ------------------------------------------------------------------------------
> Learn Graph Databases - Download FREE O'Reilly Book
> "Graph Databases" is the definitive new guide to graph databases and their
> applications. Written by three acclaimed leaders in the field,
> this first edition is now available. Download your free book today!
> http://p.sf.net/sfu/13534_NeoTech
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] f2fs: optimize restore_node_summary slightly
2014-03-10 5:13 ` Chao Yu
@ 2014-03-10 5:37 ` Jaegeuk Kim
2014-03-10 6:12 ` Chao Yu
0 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2014-03-10 5:37 UTC (permalink / raw)
To: Chao Yu; +Cc: 'Gu Zheng', 'linux-kernel', 'f2fs'
Hi,
2014-03-10 (월), 13:13 +0800, Chao Yu:
> Hi Gu, Kim:
>
> One more comment.
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk.kim@samsung.com]
> > Sent: Monday, March 10, 2014 12:46 PM
> > To: Gu Zheng
> > Cc: linux-kernel; f2fs
> > Subject: Re: [f2fs-dev] [PATCH 4/5] f2fs: optimize restore_node_summary slightly
> >
> > Hi Gu,
> >
> > 2014-03-07 (금), 18:43 +0800, Gu Zheng:
> > > Previously, we ra_sum_pages to pre-read contiguous pages as more
> > > as possible, and if we fail to alloc more pages, an ENOMEM error
> > > will be reported upstream, even though we have alloced some pages
> > > yet. In fact, we can use the available pages to do the job partly,
> > > and continue the rest in the following circle. Only reporting ENOMEM
> > > upstream if we really can not alloc any available page.
> > >
> > > And another fix is ignoring dealing with the following pages if an
> > > EIO occurs when reading page from page_list.
> > >
> > > Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>
> Reviewed-by: Chao Yu <chao2.yu@samsung.com>
>
> > > ---
> > > fs/f2fs/node.c | 44 ++++++++++++++++++++------------------------
> > > fs/f2fs/segment.c | 7 +++++--
> > > 2 files changed, 25 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > > index 8787469..4b7861d 100644
> > > --- a/fs/f2fs/node.c
> > > +++ b/fs/f2fs/node.c
> > > @@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
> > *pages,
> > > for (; page_idx < start + nrpages; page_idx++) {
> > > /* alloc temporal page for read node summary info*/
> > > page = alloc_page(GFP_F2FS_ZERO);
> > > - if (!page) {
> > > - struct page *tmp;
> > > - list_for_each_entry_safe(page, tmp, pages, lru) {
> > > - list_del(&page->lru);
> > > - unlock_page(page);
> > > - __free_pages(page, 0);
> > > - }
> > > - return -ENOMEM;
> > > - }
> > > + if (!page)
> > > + break;
> > >
> > > lock_page(page);
> > > page->index = page_idx;
> > > @@ -1607,7 +1600,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
> > *pages,
> > > f2fs_submit_page_mbio(sbi, page, page->index, &fio);
> > >
> > > f2fs_submit_merged_bio(sbi, META, READ);
> > > - return 0;
> > > +
> > > + return page_idx - start;
> > > }
> > >
> > > int restore_node_summary(struct f2fs_sb_info *sbi,
> > > @@ -1630,28 +1624,30 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
> > > nrpages = min(last_offset - i, bio_blocks);
> > >
> > > /* read ahead node pages */
> > > - err = ra_sum_pages(sbi, &page_list, addr, nrpages);
> > > - if (err)
> > > - return err;
> > > + nrpages = ra_sum_pages(sbi, &page_list, addr, nrpages);
> > > + if (!nrpages)
> > > + return -ENOMEM;
> > >
> > > list_for_each_entry_safe(page, tmp, &page_list, lru) {
> > > -
> >
> > Here we can just add:
> > if (err)
> > goto skip;
> > lock_page();
> > ...
> > unlock_page();
> > skip:
> > list_del();
> > __free_pages();
> >
> > IMO, it's more neat, so if you have any objection, let me know.
> > Otherwise, I'll handle this by myself. :)
> > Thanks,
> >
> > > - lock_page(page);
> > > - if (unlikely(!PageUptodate(page))) {
> > > - err = -EIO;
> > > - } else {
> > > - rn = F2FS_NODE(page);
> > > - sum_entry->nid = rn->footer.nid;
> > > - sum_entry->version = 0;
> > > - sum_entry->ofs_in_node = 0;
> > > - sum_entry++;
> > > + if (!err) {
>
> If we skip here, next round we will fill these summary page entries with
> wrong info because we skip the code 'sum_entry++;'.
There is no next round. Once err = -EIO, there's no route to make err =
0.
>
> > > + lock_page(page);
> > > + if (unlikely(!PageUptodate(page))) {
> > > + err = -EIO;
> > > + } else {
> > > + rn = F2FS_NODE(page);
> > > + sum_entry->nid = rn->footer.nid;
> > > + sum_entry->version = 0;
> > > + sum_entry->ofs_in_node = 0;
> > > + sum_entry++;
> > > + }
> > > + unlock_page(page);
> > > }
> > >
> > > list_del(&page->lru);
> > > - unlock_page(page);
> > > __free_pages(page, 0);
> > > }
>
> Maybe we should add code here.
> if (err)
> return err;
This can reduce unnecessary loop executions.
I'll add this.
Thanks,
>
> > > }
> > > +
> > > return err;
> > > }
> > >
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index 199c964..b3f8431 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -1160,9 +1160,12 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type)
> > > ns->ofs_in_node = 0;
> > > }
> > > } else {
> > > - if (restore_node_summary(sbi, segno, sum)) {
> > > + int err;
> > > +
> > > + err = restore_node_summary(sbi, segno, sum);
> > > + if (err) {
> > > f2fs_put_page(new, 1);
> > > - return -EINVAL;
> > > + return err;
> > > }
> > > }
> > > }
> >
> > --
> > Jaegeuk Kim
> > Samsung
> >
> >
> > ------------------------------------------------------------------------------
> > Learn Graph Databases - Download FREE O'Reilly Book
> > "Graph Databases" is the definitive new guide to graph databases and their
> > applications. Written by three acclaimed leaders in the field,
> > this first edition is now available. Download your free book today!
> > http://p.sf.net/sfu/13534_NeoTech
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
--
Jaegeuk Kim
Samsung
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] f2fs: optimize restore_node_summary slightly
2014-03-10 4:45 ` Jaegeuk Kim
2014-03-10 5:13 ` Chao Yu
@ 2014-03-10 5:38 ` Gu Zheng
1 sibling, 0 replies; 6+ messages in thread
From: Gu Zheng @ 2014-03-10 5:38 UTC (permalink / raw)
To: jaegeuk.kim; +Cc: linux-kernel, f2fs
Hi Kim,
On 03/10/2014 12:45 PM, Jaegeuk Kim wrote:
> Hi Gu,
>
> 2014-03-07 (금), 18:43 +0800, Gu Zheng:
>> Previously, we ra_sum_pages to pre-read contiguous pages as more
>> as possible, and if we fail to alloc more pages, an ENOMEM error
>> will be reported upstream, even though we have alloced some pages
>> yet. In fact, we can use the available pages to do the job partly,
>> and continue the rest in the following circle. Only reporting ENOMEM
>> upstream if we really can not alloc any available page.
>>
>> And another fix is ignoring dealing with the following pages if an
>> EIO occurs when reading page from page_list.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>> fs/f2fs/node.c | 44 ++++++++++++++++++++------------------------
>> fs/f2fs/segment.c | 7 +++++--
>> 2 files changed, 25 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index 8787469..4b7861d 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head *pages,
>> for (; page_idx < start + nrpages; page_idx++) {
>> /* alloc temporal page for read node summary info*/
>> page = alloc_page(GFP_F2FS_ZERO);
>> - if (!page) {
>> - struct page *tmp;
>> - list_for_each_entry_safe(page, tmp, pages, lru) {
>> - list_del(&page->lru);
>> - unlock_page(page);
>> - __free_pages(page, 0);
>> - }
>> - return -ENOMEM;
>> - }
>> + if (!page)
>> + break;
>>
>> lock_page(page);
>> page->index = page_idx;
>> @@ -1607,7 +1600,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head *pages,
>> f2fs_submit_page_mbio(sbi, page, page->index, &fio);
>>
>> f2fs_submit_merged_bio(sbi, META, READ);
>> - return 0;
>> +
>> + return page_idx - start;
>> }
>>
>> int restore_node_summary(struct f2fs_sb_info *sbi,
>> @@ -1630,28 +1624,30 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
>> nrpages = min(last_offset - i, bio_blocks);
>>
>> /* read ahead node pages */
>> - err = ra_sum_pages(sbi, &page_list, addr, nrpages);
>> - if (err)
>> - return err;
>> + nrpages = ra_sum_pages(sbi, &page_list, addr, nrpages);
>> + if (!nrpages)
>> + return -ENOMEM;
>>
>> list_for_each_entry_safe(page, tmp, &page_list, lru) {
>> -
>
> Here we can just add:
> if (err)
> goto skip;
> lock_page();
> ...
> unlock_page();
> skip:
> list_del();
> __free_pages();
>
> IMO, it's more neat, so if you have any objection, let me know.
> Otherwise, I'll handle this by myself. :)
Thanks very much.
Regards,
Gu
> Thanks,
>
>> - lock_page(page);
>> - if (unlikely(!PageUptodate(page))) {
>> - err = -EIO;
>> - } else {
>> - rn = F2FS_NODE(page);
>> - sum_entry->nid = rn->footer.nid;
>> - sum_entry->version = 0;
>> - sum_entry->ofs_in_node = 0;
>> - sum_entry++;
>> + if (!err) {
>> + lock_page(page);
>> + if (unlikely(!PageUptodate(page))) {
>> + err = -EIO;
>> + } else {
>> + rn = F2FS_NODE(page);
>> + sum_entry->nid = rn->footer.nid;
>> + sum_entry->version = 0;
>> + sum_entry->ofs_in_node = 0;
>> + sum_entry++;
>> + }
>> + unlock_page(page);
>> }
>>
>> list_del(&page->lru);
>> - unlock_page(page);
>> __free_pages(page, 0);
>> }
>> }
>> +
>> return err;
>> }
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 199c964..b3f8431 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -1160,9 +1160,12 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type)
>> ns->ofs_in_node = 0;
>> }
>> } else {
>> - if (restore_node_summary(sbi, segno, sum)) {
>> + int err;
>> +
>> + err = restore_node_summary(sbi, segno, sum);
>> + if (err) {
>> f2fs_put_page(new, 1);
>> - return -EINVAL;
>> + return err;
>> }
>> }
>> }
>
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] f2fs: optimize restore_node_summary slightly
2014-03-10 5:37 ` Jaegeuk Kim
@ 2014-03-10 6:12 ` Chao Yu
0 siblings, 0 replies; 6+ messages in thread
From: Chao Yu @ 2014-03-10 6:12 UTC (permalink / raw)
To: jaegeuk.kim; +Cc: 'Gu Zheng', 'linux-kernel', 'f2fs'
Hi,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk.kim@samsung.com]
> Sent: Monday, March 10, 2014 1:38 PM
> To: Chao Yu
> Cc: 'Gu Zheng'; 'linux-kernel'; 'f2fs'
> Subject: RE: [f2fs-dev] [PATCH 4/5] f2fs: optimize restore_node_summary slightly
>
> Hi,
>
> 2014-03-10 (월), 13:13 +0800, Chao Yu:
> > Hi Gu, Kim:
> >
> > One more comment.
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk.kim@samsung.com]
> > > Sent: Monday, March 10, 2014 12:46 PM
> > > To: Gu Zheng
> > > Cc: linux-kernel; f2fs
> > > Subject: Re: [f2fs-dev] [PATCH 4/5] f2fs: optimize restore_node_summary slightly
> > >
> > > Hi Gu,
> > >
> > > 2014-03-07 (금), 18:43 +0800, Gu Zheng:
> > > > Previously, we ra_sum_pages to pre-read contiguous pages as more
> > > > as possible, and if we fail to alloc more pages, an ENOMEM error
> > > > will be reported upstream, even though we have alloced some pages
> > > > yet. In fact, we can use the available pages to do the job partly,
> > > > and continue the rest in the following circle. Only reporting ENOMEM
> > > > upstream if we really can not alloc any available page.
> > > >
> > > > And another fix is ignoring dealing with the following pages if an
> > > > EIO occurs when reading page from page_list.
> > > >
> > > > Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> >
> > Reviewed-by: Chao Yu <chao2.yu@samsung.com>
> >
> > > > ---
> > > > fs/f2fs/node.c | 44 ++++++++++++++++++++------------------------
> > > > fs/f2fs/segment.c | 7 +++++--
> > > > 2 files changed, 25 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > > > index 8787469..4b7861d 100644
> > > > --- a/fs/f2fs/node.c
> > > > +++ b/fs/f2fs/node.c
> > > > @@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
> > > *pages,
> > > > for (; page_idx < start + nrpages; page_idx++) {
> > > > /* alloc temporal page for read node summary info*/
> > > > page = alloc_page(GFP_F2FS_ZERO);
> > > > - if (!page) {
> > > > - struct page *tmp;
> > > > - list_for_each_entry_safe(page, tmp, pages, lru) {
> > > > - list_del(&page->lru);
> > > > - unlock_page(page);
> > > > - __free_pages(page, 0);
> > > > - }
> > > > - return -ENOMEM;
> > > > - }
> > > > + if (!page)
> > > > + break;
> > > >
> > > > lock_page(page);
> > > > page->index = page_idx;
> > > > @@ -1607,7 +1600,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
> > > *pages,
> > > > f2fs_submit_page_mbio(sbi, page, page->index, &fio);
> > > >
> > > > f2fs_submit_merged_bio(sbi, META, READ);
> > > > - return 0;
> > > > +
> > > > + return page_idx - start;
> > > > }
> > > >
> > > > int restore_node_summary(struct f2fs_sb_info *sbi,
> > > > @@ -1630,28 +1624,30 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
> > > > nrpages = min(last_offset - i, bio_blocks);
> > > >
> > > > /* read ahead node pages */
> > > > - err = ra_sum_pages(sbi, &page_list, addr, nrpages);
> > > > - if (err)
> > > > - return err;
> > > > + nrpages = ra_sum_pages(sbi, &page_list, addr, nrpages);
> > > > + if (!nrpages)
> > > > + return -ENOMEM;
> > > >
> > > > list_for_each_entry_safe(page, tmp, &page_list, lru) {
> > > > -
> > >
> > > Here we can just add:
> > > if (err)
> > > goto skip;
> > > lock_page();
> > > ...
> > > unlock_page();
> > > skip:
> > > list_del();
> > > __free_pages();
> > >
> > > IMO, it's more neat, so if you have any objection, let me know.
> > > Otherwise, I'll handle this by myself. :)
> > > Thanks,
> > >
> > > > - lock_page(page);
> > > > - if (unlikely(!PageUptodate(page))) {
> > > > - err = -EIO;
> > > > - } else {
> > > > - rn = F2FS_NODE(page);
> > > > - sum_entry->nid = rn->footer.nid;
> > > > - sum_entry->version = 0;
> > > > - sum_entry->ofs_in_node = 0;
> > > > - sum_entry++;
> > > > + if (!err) {
> >
> > If we skip here, next round we will fill these summary page entries with
> > wrong info because we skip the code 'sum_entry++;'.
>
> There is no next round. Once err = -EIO, there's no route to make err =
> 0.
Ah, you're right. Only old code has this problem, this new patch can fix it well.
Thanks for the mention. :)
Thanks.
>
> >
> > > > + lock_page(page);
> > > > + if (unlikely(!PageUptodate(page))) {
> > > > + err = -EIO;
> > > > + } else {
> > > > + rn = F2FS_NODE(page);
> > > > + sum_entry->nid = rn->footer.nid;
> > > > + sum_entry->version = 0;
> > > > + sum_entry->ofs_in_node = 0;
> > > > + sum_entry++;
> > > > + }
> > > > + unlock_page(page);
> > > > }
> > > >
> > > > list_del(&page->lru);
> > > > - unlock_page(page);
> > > > __free_pages(page, 0);
> > > > }
> >
> > Maybe we should add code here.
> > if (err)
> > return err;
>
> This can reduce unnecessary loop executions.
> I'll add this.
> Thanks,
>
> >
> > > > }
> > > > +
> > > > return err;
> > > > }
> > > >
> > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > > index 199c964..b3f8431 100644
> > > > --- a/fs/f2fs/segment.c
> > > > +++ b/fs/f2fs/segment.c
> > > > @@ -1160,9 +1160,12 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int
> type)
> > > > ns->ofs_in_node = 0;
> > > > }
> > > > } else {
> > > > - if (restore_node_summary(sbi, segno, sum)) {
> > > > + int err;
> > > > +
> > > > + err = restore_node_summary(sbi, segno, sum);
> > > > + if (err) {
> > > > f2fs_put_page(new, 1);
> > > > - return -EINVAL;
> > > > + return err;
> > > > }
> > > > }
> > > > }
> > >
> > > --
> > > Jaegeuk Kim
> > > Samsung
> > >
> > >
> > > ------------------------------------------------------------------------------
> > > Learn Graph Databases - Download FREE O'Reilly Book
> > > "Graph Databases" is the definitive new guide to graph databases and their
> > > applications. Written by three acclaimed leaders in the field,
> > > this first edition is now available. Download your free book today!
> > > http://p.sf.net/sfu/13534_NeoTech
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
>
> --
> Jaegeuk Kim
> Samsung
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-10 6:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-07 10:43 [PATCH 4/5] f2fs: optimize restore_node_summary slightly Gu Zheng
2014-03-10 4:45 ` Jaegeuk Kim
2014-03-10 5:13 ` Chao Yu
2014-03-10 5:37 ` Jaegeuk Kim
2014-03-10 6:12 ` Chao Yu
2014-03-10 5:38 ` Gu Zheng
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).