f2fs: optimize restore_node_summary slightly

PATCH In review Gu Zheng 3 participants 6 messages Mar 7, 2014 – Mar 10, 2014 F2FS file system

Share & tools

Gu Zheng

author

2014-03-07 10:52 UTC #0 lore raw

PATCH 4/5 f2fs: optimize restore_node_summary slightly

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 <[email protected]>

---
 fs/f2fs/node.c    |   44 ++++++++++++++++++++------------------------
 fs/f2fs/segment.c |    7 +++++--
 2 files changed, 25 insertions(+), 26 deletions(-)

fs/f2fs/node.c +20 ~~-24~~

index 8787469..4b7861d 100644
@@ -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;
 }

fs/f2fs/segment.c +5 -2

index 199c964..b3f8431 100644
@@ -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;
 			}
 		}
 	}

Signature · 1 line

1.7.7

Jaegeuk Kim

2014-03-10 04:45 UTC #1 lore raw

Hi Gu,

2014-03-07 (금), 18:43 +0800, Gu Zheng:

Quoted 51 lines

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 <[email protected]>

---
 fs/f2fs/node.c    |   44 ++++++++++++++++++++------------------------
 fs/f2fs/segment.c |    7 +++++--
 2 files changed, 25 insertions(+), 26 deletions(-)

fs/f2fs/node.c +4 ~~-10~~

index 8787469..4b7861d 100644
@@ -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);

fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1638

Quoted 10 lines

fs/f2fs/node.c +3 -4

 		/* 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,

Quoted 51 lines

fs/f2fs/node.c +13 ~~-10~~

-			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;
 }

fs/f2fs/segment.c +5 -2

index 199c964..b3f8431 100644
@@ -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;
 			}
 		}
 	}

Signature · 2 lines

Jaegeuk Kim
Samsung

Chao Yu

Reviewed-by

2014-03-10 05:14 UTC #2 lore raw

Hi Gu, Kim:

One more comment.

Quoted 7 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
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

Quoted 14 lines

Hi Gu,
2014-03-07 (금), 18:43 +0800, Gu Zheng:
Quoted 11 lines
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 <[email protected]>

Reviewed-by Chao Yu <[email protected]>

Quoted 67 lines

fs/f2fs/node.c static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head

Quoted 10 lines

---
 fs/f2fs/node.c    |   44 ++++++++++++++++++++------------------------
 fs/f2fs/segment.c |    7 +++++--
 2 files changed, 25 insertions(+), 26 deletions(-)

fs/f2fs/node.c +0 -0

index 8787469..4b7861d 100644
@@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head

*pages,

fs/f2fs/node.c static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head

Quoted 18 lines

fs/f2fs/node.c +2 -9

 	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,

Quoted 12 lines

fs/f2fs/node.c +2 -1

 		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);

fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1638

Quoted 10 lines

fs/f2fs/node.c +3 -4

 		/* 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,

Quoted 10 lines

fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1632

Quoted 10 lines

fs/f2fs/node.c +1 -9

-			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++;'.

Quoted 13 lines

fs/f2fs/node.c +11 -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);
 			}

Quoted 4 lines

fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1648
Quoted 4 lines
fs/f2fs/node.c +0 -1
 			list_del(&page->lru);
-			unlock_page(page);
 			__free_pages(page, 0);
 		}

Maybe we should add code here.

			if (err)
				return err;

Quoted 40 lines

Quoted 14 lines
fs/f2fs/node.c +1 -0
 	}
+
 	return err;
 }
 
fs/f2fs/segment.c +0 -1
index 199c964..b3f8431 100644
@@ -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)) {
fs/f2fs/segment.c static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) · line 1171
Quoted 10 lines
fs/f2fs/segment.c +5 -1
+			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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

2014-03-10 05:39 UTC #3 lore raw

Hi,

2014-03-10 (월), 13:13 +0800, Chao Yu:

Quoted 95 lines

Hi Gu, Kim:
One more comment.
Quoted 7 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
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
Quoted 14 lines
Hi Gu,
2014-03-07 (금), 18:43 +0800, Gu Zheng:
Quoted 11 lines
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 <[email protected]>
Reviewed-by Chao Yu <[email protected]>
Quoted 67 lines
fs/f2fs/node.c static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
Quoted 10 lines
---
 fs/f2fs/node.c    |   44 ++++++++++++++++++++------------------------
 fs/f2fs/segment.c |    7 +++++--
 2 files changed, 25 insertions(+), 26 deletions(-)
fs/f2fs/node.c +0 -0
index 8787469..4b7861d 100644
@@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
*pages,
fs/f2fs/node.c static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
Quoted 18 lines
fs/f2fs/node.c +2 -9
 	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,
Quoted 12 lines
fs/f2fs/node.c +2 -1
 		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);
 
fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1638
Quoted 10 lines
fs/f2fs/node.c +3 -4
 		/* 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,

Quoted 13 lines

Quoted 10 lines
fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1632
Quoted 10 lines
fs/f2fs/node.c +1 -9
-			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.

Quoted 14 lines

fs/f2fs/node.c +11 -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);
 			}

Quoted 8 lines

Quoted 4 lines
fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1648
Quoted 4 lines
fs/f2fs/node.c +0 -1
 			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,

Quoted 42 lines

Quoted 41 lines
Quoted 15 lines
fs/f2fs/node.c +1 -0
 	}
+
 	return err;
 }
 
fs/f2fs/segment.c +0 -1
index 199c964..b3f8431 100644
@@ -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)) {
fs/f2fs/segment.c static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) · line 1171
Quoted 10 lines
fs/f2fs/segment.c +5 -1
+			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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Signature · 2 lines

Jaegeuk Kim
Samsung

Gu Zheng

author

2014-03-10 05:47 UTC #5 lore raw

In reply to Jaegeuk Kim

Hi Kim,

Quoted 75 lines · Jaegeuk Kim

Hi Gu,

2014-03-07 (금), 18:43 +0800, Gu Zheng:

Quoted 51 lines

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 <[email protected]>

---
 fs/f2fs/node.c    |   44 ++++++++++++++++++++------------------------
 fs/f2fs/segment.c |    7 +++++--
 2 files changed, 25 insertions(+), 26 deletions(-)

fs/f2fs/node.c +4 ~~-10~~

index 8787469..4b7861d 100644
@@ -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);

fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1638

Quoted 10 lines

fs/f2fs/node.c +3 -4

 		/* 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();

Quoted 2 lines

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

Quoted 45 lines

Thanks,

Quoted 43 lines

fs/f2fs/node.c +13 ~~-10~~

-			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;
 }

fs/f2fs/segment.c +2 -1

index 199c964..b3f8431 100644
@@ -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;
+

Quoted 9 lines

fs/f2fs/segment.c static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) · line 1171
Quoted 8 lines
fs/f2fs/segment.c +3 -1
+			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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

2014-03-10 06:13 UTC #4 lore raw

In reply to Jaegeuk Kim

Hi,

Quoted 119 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
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:
Quoted 95 lines
Hi Gu, Kim:
One more comment.
Quoted 7 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
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
Quoted 14 lines
Hi Gu,
2014-03-07 (금), 18:43 +0800, Gu Zheng:
Quoted 11 lines
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 <[email protected]>
Reviewed-by Chao Yu <[email protected]>
Quoted 67 lines
fs/f2fs/node.c static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
Quoted 10 lines
---
 fs/f2fs/node.c    |   44 ++++++++++++++++++++------------------------
 fs/f2fs/segment.c |    7 +++++--
 2 files changed, 25 insertions(+), 26 deletions(-)
fs/f2fs/node.c +0 -0
index 8787469..4b7861d 100644
@@ -1588,15 +1588,8 @@ static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
*pages,
fs/f2fs/node.c static int ra_sum_pages(struct f2fs_sb_info *sbi, struct list_head
Quoted 18 lines
fs/f2fs/node.c +2 -9
 	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,
Quoted 12 lines
fs/f2fs/node.c +2 -1
 		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);
 
fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1638
Quoted 10 lines
fs/f2fs/node.c +3 -4
 		/* 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,
Quoted 13 lines
Quoted 10 lines
fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1632
Quoted 10 lines
fs/f2fs/node.c +1 -9
-			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++;'.

Quoted 2 lines

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.

Quoted 75 lines

Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
fs/f2fs/node.c +11 -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);
 			}
 
Quoted 8 lines
Quoted 4 lines
fs/f2fs/node.c int restore_node_summary(struct f2fs_sb_info *sbi, · line 1648
Quoted 4 lines
fs/f2fs/node.c +0 -1
 			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,
Quoted 11 lines
Quoted 11 lines
fs/f2fs/segment.c static int read_normal_summaries(struct f2fs_sb_info *sbi, int
Quoted 11 lines
fs/f2fs/node.c +1 -0
 	}
+
 	return err;
 }
 
fs/f2fs/segment.c +0 -0
index 199c964..b3f8431 100644
@@ -1160,9 +1160,12 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int
type)
Quoted 20 lines
Quoted 20 lines
Quoted 4 lines
fs/f2fs/segment.c +0 -1
 				ns->ofs_in_node = 0;
 			}
 		} else {
-			if (restore_node_summary(sbi, segno, sum)) {
fs/f2fs/segment.c static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) · line 1171
Quoted 10 lines
fs/f2fs/segment.c +5 -1
+			int err;
+
+			err = restore_node_summary(sbi, segno, sum);
+			if (err) {
 				f2fs_put_page(new, 1);
-				return -EINVAL;
+				return err;
 			}
 		}
 	}
--
Jaegeuk Kim
Samsung
Quoted 11 lines
Quoted 10 lines
------------------------------------------------------------------------------
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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
--
Jaegeuk Kim
Samsung