f2fs: check the page status filled from disk

PATCH Chao Yu 2 participants 2 messages Jan 6, 2016 VFS

lore

Share & tools

lore

Chao Yu

author

2016-01-06 03:15 UTC #0 lore raw

Hi Jaegeuk,

Quoted 149 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 10:30 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
Hi,
Quoted 122 lines · Chao Yu
Hi Jaegeuk,
Quoted 44 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 1:49 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
Hi Chao,
Quoted 23 lines · Chao Yu
Hi Jaegeuk,
Quoted 21 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
After reading a page, we need to check whether there is any error.
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 8 ++++++++
 1 file changed, 8 insertions(+)
fs/f2fs/data.c +0 -0
index 89a978c..11b2111 100644
@@ -448,6 +448,14 @@ repeat:
 
Quoted 9 lines
fs/f2fs/data.c repeat: · line 453
Quoted 5 lines
fs/f2fs/data.c +3 -0
 		/* wait for read completion */
 		lock_page(page);
+		if (unlikely(!PageUptodate(page))) {
+			f2fs_put_page(page, 1);
+			return ERR_PTR(-EIO);
There is a convention in get_new_data_page, anyway we should release ipage if there is any error occurs, but I think it will be ok to return directly since it seems impossible the new dentry page has its real block address.
Quoted 1 line
Makes sense, but definitely ipage should be put. :)
Alright. :)
Quoted 72 lines
Quoted 35 lines
To avoid any bug here or wrong usage, how about add bug_on as following patch?
Quoted 1 line
From d92f0f34493b27ef28da67c446d552ce721b5d6f Mon Sep 17 00:00:00 2001
From: Chao Yu <[email protected]>
Date: Tue, 5 Jan 2016 15:28:56 +0800
Subject: [PATCH] f2fs: add f2fs_bug_on in get_new_data_page
In get_new_data_page, locked inode page should not be hold before get_read_data_page, this patch adds f2fs_bug_on to detect this condition.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/data.c | 2 ++
 1 file changed, 2 insertions(+)
fs/f2fs/data.c +2 -1
index 48f0bd3..2c5e3f6 100644
@@ -440,6 +440,8 @@ repeat:
 		zero_user_segment(page, 0, PAGE_CACHE_SIZE);
 		SetPageUptodate(page);
 	} else {
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);
+
 		f2fs_put_page(page, 1);
 
 		page = get_read_data_page(inode, index, READ_SYNC, true);
--
2.6.3
Quoted 7 lines
fs/f2fs/data.c
Quoted 5 lines
fs/f2fs/data.c +5 -0
+		}
+		if (unlikely(page->mapping != mapping)) {
+			f2fs_put_page(page, 1);
+			goto repeat;
+		}
How about use get_lock_data_page to avoid duplicated code?
Agreed.
How about this?
From fef77fb244a706491e8e4c46cb245e99e22003c3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 1 Jan 2016 22:03:47 -0800
Subject: [PATCH] f2fs: check the page status filled from disk
After reading a page, we need to check whether there is any error.
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
fs/f2fs/data.c +1 -3
index 89a978c..89d633a 100644
@@ -442,12 +442,16 @@ repeat:
 	} else {
 		f2fs_put_page(page, 1);
 
-		page = get_read_data_page(inode, index, READ_SYNC, true);
-		if (IS_ERR(page))
-			goto repeat;
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);
 
Quoted 15 lines
fs/f2fs/data.c repeat: · line 453
Quoted 9 lines
fs/f2fs/data.c +7 -2
-		/* wait for read completion */
-		lock_page(page);
+		page = get_lock_data_page(inode, index, true);
+		if (IS_ERR(page)) {
+			if (PTR_ERR(page) == -EIO) {
+				f2fs_put_page(ipage, 1);
+				return page;
+			}
+			goto repeat;
Seems if get_lock_data_page always return -EFAULT, we may run into an infinite loop. IMO, it's not a bad thing to tolerate other error more than EIO returned from get_lock_data_page. How about return directly when error is returned? And add a bug_on for ENOENT which seems not impossible here?

Quoted 2 lines

Hmm. I can only expect EIO, ENOMEM, and ENOENT.
What condition can we get EFAULT?

It's possible in following call path:
- get_new_data_page
- get_read_data_page
- f2fs_submit_page_bio
- bio_add_page failed and return -EFAULT

Right?

Thanks,

Quoted 13 lines

Thanks,
Quoted 10 lines
Thanks,
Quoted 6 lines
fs/f2fs/data.c +1 -1
+		}
 	}
 got_it:
 	if (new_i_size && i_size_read(inode) <
--
2.6.3

Jaegeuk Kim

2016-01-06 04:02 UTC #1 lore raw

Quoted 154 lines · Chao Yu

Hi Jaegeuk,
Quoted 149 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 10:30 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
Hi,
Quoted 122 lines · Chao Yu
Hi Jaegeuk,
Quoted 44 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 1:49 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
Hi Chao,
Quoted 23 lines · Chao Yu
Hi Jaegeuk,
Quoted 21 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
After reading a page, we need to check whether there is any error.
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 8 ++++++++
 1 file changed, 8 insertions(+)
fs/f2fs/data.c +0 -0
index 89a978c..11b2111 100644
@@ -448,6 +448,14 @@ repeat:
 
Quoted 9 lines
fs/f2fs/data.c repeat: · line 453
Quoted 5 lines
fs/f2fs/data.c +3 -0
 		/* wait for read completion */
 		lock_page(page);
+		if (unlikely(!PageUptodate(page))) {
+			f2fs_put_page(page, 1);
+			return ERR_PTR(-EIO);
There is a convention in get_new_data_page, anyway we should release ipage if there is any error occurs, but I think it will be ok to return directly since it seems impossible the new dentry page has its real block address.
Quoted 1 line
Makes sense, but definitely ipage should be put. :)
Alright. :)
Quoted 72 lines
Quoted 35 lines
To avoid any bug here or wrong usage, how about add bug_on as following patch?
Quoted 1 line
From d92f0f34493b27ef28da67c446d552ce721b5d6f Mon Sep 17 00:00:00 2001
From: Chao Yu <[email protected]>
Date: Tue, 5 Jan 2016 15:28:56 +0800
Subject: [PATCH] f2fs: add f2fs_bug_on in get_new_data_page
In get_new_data_page, locked inode page should not be hold before get_read_data_page, this patch adds f2fs_bug_on to detect this condition.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/data.c | 2 ++
 1 file changed, 2 insertions(+)
fs/f2fs/data.c +2 -1
index 48f0bd3..2c5e3f6 100644
@@ -440,6 +440,8 @@ repeat:
 		zero_user_segment(page, 0, PAGE_CACHE_SIZE);
 		SetPageUptodate(page);
 	} else {
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);
+
 		f2fs_put_page(page, 1);
 
 		page = get_read_data_page(inode, index, READ_SYNC, true);
--
2.6.3
Quoted 7 lines
fs/f2fs/data.c
Quoted 5 lines
fs/f2fs/data.c +5 -0
+		}
+		if (unlikely(page->mapping != mapping)) {
+			f2fs_put_page(page, 1);
+			goto repeat;
+		}
How about use get_lock_data_page to avoid duplicated code?
Agreed.
How about this?
From fef77fb244a706491e8e4c46cb245e99e22003c3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 1 Jan 2016 22:03:47 -0800
Subject: [PATCH] f2fs: check the page status filled from disk
After reading a page, we need to check whether there is any error.
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
fs/f2fs/data.c +1 -3
index 89a978c..89d633a 100644
@@ -442,12 +442,16 @@ repeat:
 	} else {
 		f2fs_put_page(page, 1);
 
-		page = get_read_data_page(inode, index, READ_SYNC, true);
-		if (IS_ERR(page))
-			goto repeat;
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);
 
Quoted 15 lines
fs/f2fs/data.c repeat: · line 453
Quoted 9 lines
fs/f2fs/data.c +7 -2
-		/* wait for read completion */
-		lock_page(page);
+		page = get_lock_data_page(inode, index, true);
+		if (IS_ERR(page)) {
+			if (PTR_ERR(page) == -EIO) {
+				f2fs_put_page(ipage, 1);
+				return page;
+			}
+			goto repeat;
Seems if get_lock_data_page always return -EFAULT, we may run into an infinite loop. IMO, it's not a bad thing to tolerate other error more than EIO returned from get_lock_data_page. How about return directly when error is returned? And add a bug_on for ENOENT which seems not impossible here?
Quoted 2 lines
Hmm. I can only expect EIO, ENOMEM, and ENOENT.
What condition can we get EFAULT?

Quoted 7 lines

It's possible in following call path:
- get_new_data_page
- get_read_data_page
- f2fs_submit_page_bio
- bio_add_page failed and return -EFAULT
Right?

Indeed. But seems that it's impossible to get that error in this path. Anyway, yes, it is not a big deal to return any error directly. I'll modify this again. :)

Thanks,

Quoted 16 lines

Thanks,
Quoted 13 lines
Thanks,
Quoted 10 lines
Thanks,
Quoted 6 lines
fs/f2fs/data.c +1 -1
+		}
 	}
 got_it:
 	if (new_i_size && i_size_read(inode) <
--
2.6.3