Re: [PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
On 2016/5/26 0:54, Jaegeuk Kim wrote: > Hi Yunlong, > > Do we have a bug report in terms of this? > Hi Kim, I found the old following patch, you have mentioned one reason why "nid != nid_of_node(page)" in that commit message. http://git.kernel.org/cgit/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev=3bb5e2c8fe2296ddd9d864dcfb5ee1b77135f3ec commit 3bb5e2c8fe2296ddd9d864dcfb5ee1b77135f3ec Author: Jaegeuk KimDate: Tue Apr 1 17:38:26 2014 +0900 f2fs: return -EIO when node id is not matched During the cleaing of node segments, F2FS can get errored node blocks due to data race between node page lock and its valid bitmap operations. In that case, it needs to return an error to skip such the obsolete block copy. Signed-off-by: Jaegeuk Kim diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index eced8d7..065cd99 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -958,7 +958,7 @@ repeat: goto got_it; lock_page(page); - if (unlikely(!PageUptodate(page))) { + if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) { f2fs_put_page(page, 1); return ERR_PTR(-EIO); } @@ -967,7 +967,6 @@ repeat: goto repeat; } got_it: - f2fs_bug_on(nid != nid_of_node(page)); mark_page_accessed(page); return page; } -- Thanks, Yunlong Song
Re: [PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
On 2016/5/26 0:54, Jaegeuk Kim wrote: > Hi Yunlong, > > Do we have a bug report in terms of this? > Hi Kim, I found the old following patch, you have mentioned one reason why "nid != nid_of_node(page)" in that commit message. http://git.kernel.org/cgit/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev=3bb5e2c8fe2296ddd9d864dcfb5ee1b77135f3ec commit 3bb5e2c8fe2296ddd9d864dcfb5ee1b77135f3ec Author: Jaegeuk Kim Date: Tue Apr 1 17:38:26 2014 +0900 f2fs: return -EIO when node id is not matched During the cleaing of node segments, F2FS can get errored node blocks due to data race between node page lock and its valid bitmap operations. In that case, it needs to return an error to skip such the obsolete block copy. Signed-off-by: Jaegeuk Kim diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index eced8d7..065cd99 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -958,7 +958,7 @@ repeat: goto got_it; lock_page(page); - if (unlikely(!PageUptodate(page))) { + if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) { f2fs_put_page(page, 1); return ERR_PTR(-EIO); } @@ -967,7 +967,6 @@ repeat: goto repeat; } got_it: - f2fs_bug_on(nid != nid_of_node(page)); mark_page_accessed(page); return page; } -- Thanks, Yunlong Song
Re: [PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
Forget this wrong version, I have re-sent a patch v2 version. -- Thanks, Yunlong Song
Re: [PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
Forget this wrong version, I have re-sent a patch v2 version. -- Thanks, Yunlong Song
[PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page contents all the time") pointed out that "sometimes it was reported that its contents was missing", so it checks the page's mapping and contents. When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs: clean up node page updating flow") moves "nid != nid_of_node(page)" test to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it was reported that its contents was missing" happens. This patch restores to check node page contents all the time, and returns the errno to make the caller known something is wrong and avoid to use the page. This patch also moves f2fs_bug_on to its proper location. Signed-off-by: Yunlong Song--- fs/f2fs/node.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 1f21aae..eee56aa 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1149,16 +1149,21 @@ repeat: lock_page(page); - if (unlikely(!PageUptodate(page))) { - f2fs_put_page(page, 1); - return ERR_PTR(-EIO); - } + if (unlikely(!PageUptodate(page))) + goto out_err; + if (unlikely(page->mapping != NODE_MAPPING(sbi))) { f2fs_put_page(page, 1); goto repeat; } page_hit: - f2fs_bug_on(sbi, nid != nid_of_node(page)); + if(nid != nid_of_node(page)) { + f2fs_bug_on(sbi, 1); + ClearPageUptodate(page); +out_err: + f2fs_put_page(page, 1); + return ERR_PTR(-EIO); + } return page; } -- 1.8.5.2
[PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page contents all the time") pointed out that "sometimes it was reported that its contents was missing", so it checks the page's mapping and contents. When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs: clean up node page updating flow") moves "nid != nid_of_node(page)" test to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it was reported that its contents was missing" happens. This patch restores to check node page contents all the time, and returns the errno to make the caller known something is wrong and avoid to use the page. This patch also moves f2fs_bug_on to its proper location. Signed-off-by: Yunlong Song --- fs/f2fs/node.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 1f21aae..eee56aa 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1149,16 +1149,21 @@ repeat: lock_page(page); - if (unlikely(!PageUptodate(page))) { - f2fs_put_page(page, 1); - return ERR_PTR(-EIO); - } + if (unlikely(!PageUptodate(page))) + goto out_err; + if (unlikely(page->mapping != NODE_MAPPING(sbi))) { f2fs_put_page(page, 1); goto repeat; } page_hit: - f2fs_bug_on(sbi, nid != nid_of_node(page)); + if(nid != nid_of_node(page)) { + f2fs_bug_on(sbi, 1); + ClearPageUptodate(page); +out_err: + f2fs_put_page(page, 1); + return ERR_PTR(-EIO); + } return page; } -- 1.8.5.2
Re: [PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
Hi Yunlong, Do we have a bug report in terms of this? Thanks, On Wed, May 25, 2016 at 09:01:01PM +0800, Yunlong Song wrote: > Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page > contents all the time") pointed out that "sometimes it was reported that > its contents was missing", so it checks the page's mapping and contents. > When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the > caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs: > clean up node page updating flow") moves "nid != nid_of_node(page)" test > to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a > wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it > was reported that its contents was missing" happens. > > This patch restores to check node page contents all the time, and > returns the errno to make the caller known something is wrong and avoid > to use the page. This patch also moves f2fs_bug_on to its proper location. > > Signed-off-by: Yunlong Song> --- > fs/f2fs/node.c | 12 +--- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index 1f21aae..4b04af4 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1140,16 +1140,16 @@ repeat: > if (err < 0) { > f2fs_put_page(page, 1); > return ERR_PTR(err); > - } else if (err == LOCKED_PAGE) { > - goto page_hit; > + } else if (err != LOCKED_PAGE) { > + lock_page(page); > } > > if (parent) > ra_node_pages(parent, start + 1, MAX_RA_NODE); > > - lock_page(page); > - > - if (unlikely(!PageUptodate(page))) { > + if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) { > + f2fs_bug_on(sbi, 1); > + ClearPageUptodate(page); > f2fs_put_page(page, 1); > return ERR_PTR(-EIO); > } > @@ -1157,8 +1157,6 @@ repeat: > f2fs_put_page(page, 1); > goto repeat; > } > -page_hit: > - f2fs_bug_on(sbi, nid != nid_of_node(page)); > return page; > } > > -- > 1.8.5.2
Re: [PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
Hi Yunlong, Do we have a bug report in terms of this? Thanks, On Wed, May 25, 2016 at 09:01:01PM +0800, Yunlong Song wrote: > Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page > contents all the time") pointed out that "sometimes it was reported that > its contents was missing", so it checks the page's mapping and contents. > When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the > caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs: > clean up node page updating flow") moves "nid != nid_of_node(page)" test > to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a > wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it > was reported that its contents was missing" happens. > > This patch restores to check node page contents all the time, and > returns the errno to make the caller known something is wrong and avoid > to use the page. This patch also moves f2fs_bug_on to its proper location. > > Signed-off-by: Yunlong Song > --- > fs/f2fs/node.c | 12 +--- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index 1f21aae..4b04af4 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1140,16 +1140,16 @@ repeat: > if (err < 0) { > f2fs_put_page(page, 1); > return ERR_PTR(err); > - } else if (err == LOCKED_PAGE) { > - goto page_hit; > + } else if (err != LOCKED_PAGE) { > + lock_page(page); > } > > if (parent) > ra_node_pages(parent, start + 1, MAX_RA_NODE); > > - lock_page(page); > - > - if (unlikely(!PageUptodate(page))) { > + if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) { > + f2fs_bug_on(sbi, 1); > + ClearPageUptodate(page); > f2fs_put_page(page, 1); > return ERR_PTR(-EIO); > } > @@ -1157,8 +1157,6 @@ repeat: > f2fs_put_page(page, 1); > goto repeat; > } > -page_hit: > - f2fs_bug_on(sbi, nid != nid_of_node(page)); > return page; > } > > -- > 1.8.5.2
[PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page contents all the time") pointed out that "sometimes it was reported that its contents was missing", so it checks the page's mapping and contents. When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs: clean up node page updating flow") moves "nid != nid_of_node(page)" test to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it was reported that its contents was missing" happens. This patch restores to check node page contents all the time, and returns the errno to make the caller known something is wrong and avoid to use the page. This patch also moves f2fs_bug_on to its proper location. Signed-off-by: Yunlong Song--- fs/f2fs/node.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 1f21aae..4b04af4 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1140,16 +1140,16 @@ repeat: if (err < 0) { f2fs_put_page(page, 1); return ERR_PTR(err); - } else if (err == LOCKED_PAGE) { - goto page_hit; + } else if (err != LOCKED_PAGE) { + lock_page(page); } if (parent) ra_node_pages(parent, start + 1, MAX_RA_NODE); - lock_page(page); - - if (unlikely(!PageUptodate(page))) { + if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) { + f2fs_bug_on(sbi, 1); + ClearPageUptodate(page); f2fs_put_page(page, 1); return ERR_PTR(-EIO); } @@ -1157,8 +1157,6 @@ repeat: f2fs_put_page(page, 1); goto repeat; } -page_hit: - f2fs_bug_on(sbi, nid != nid_of_node(page)); return page; } -- 1.8.5.2
[PATCH] f2fs: Return the errno to the caller to avoid using a wrong page
Commit aaf9607516ed38825268515ef4d773289a44f429 ("f2fs: check node page contents all the time") pointed out that "sometimes it was reported that its contents was missing", so it checks the page's mapping and contents. When "nid != nid_of_node(page)", ERR_PTR(-EIO) will be returned to the caller. However, commit e1c51b9f1df2f9efc2ec11488717e40cd12015f9 ("f2fs: clean up node page updating flow") moves "nid != nid_of_node(page)" test to "f2fs_bug_on(sbi, nid != nid_of_node(page))", this will return a wrong page to the caller when F2FS_CHECK_FS is off when "sometimes it was reported that its contents was missing" happens. This patch restores to check node page contents all the time, and returns the errno to make the caller known something is wrong and avoid to use the page. This patch also moves f2fs_bug_on to its proper location. Signed-off-by: Yunlong Song --- fs/f2fs/node.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 1f21aae..4b04af4 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1140,16 +1140,16 @@ repeat: if (err < 0) { f2fs_put_page(page, 1); return ERR_PTR(err); - } else if (err == LOCKED_PAGE) { - goto page_hit; + } else if (err != LOCKED_PAGE) { + lock_page(page); } if (parent) ra_node_pages(parent, start + 1, MAX_RA_NODE); - lock_page(page); - - if (unlikely(!PageUptodate(page))) { + if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) { + f2fs_bug_on(sbi, 1); + ClearPageUptodate(page); f2fs_put_page(page, 1); return ERR_PTR(-EIO); } @@ -1157,8 +1157,6 @@ repeat: f2fs_put_page(page, 1); goto repeat; } -page_hit: - f2fs_bug_on(sbi, nid != nid_of_node(page)); return page; } -- 1.8.5.2