* [PATCH 1/2] f2fs: fix wrong i_atime recovery @ 2016-11-03 16:26 Chao Yu 2016-11-03 16:26 ` [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2016-11-03 16:26 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, yuchao0, liushuoran From: Chao Yu <yuchao0@huawei.com> Shouldn't update in-memory i_atime with on-disk i_mtime of inode when recovering inode. Shuoran found this bug which is hidden for a long time, honour is belong to him. Signed-off-by: Shuoran Liu <liushuoran@huawei.com> Signed-off-by: Chao Yu <yuchao0@huawei.com> --- fs/f2fs/recovery.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c index 2fc84a9..d2ba4da 100644 --- a/fs/f2fs/recovery.c +++ b/fs/f2fs/recovery.c @@ -180,10 +180,10 @@ static void recover_inode(struct inode *inode, struct page *page) inode->i_mode = le16_to_cpu(raw->i_mode); f2fs_i_size_write(inode, le64_to_cpu(raw->i_size)); - inode->i_atime.tv_sec = le64_to_cpu(raw->i_mtime); + inode->i_atime.tv_sec = le64_to_cpu(raw->i_atime); inode->i_ctime.tv_sec = le64_to_cpu(raw->i_ctime); inode->i_mtime.tv_sec = le64_to_cpu(raw->i_mtime); - inode->i_atime.tv_nsec = le32_to_cpu(raw->i_mtime_nsec); + inode->i_atime.tv_nsec = le32_to_cpu(raw->i_atime_nsec); inode->i_ctime.tv_nsec = le32_to_cpu(raw->i_ctime_nsec); inode->i_mtime.tv_nsec = le32_to_cpu(raw->i_mtime_nsec); -- 2.10.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times 2016-11-03 16:26 [PATCH 1/2] f2fs: fix wrong i_atime recovery Chao Yu @ 2016-11-03 16:26 ` Chao Yu 2016-11-03 18:02 ` Jaegeuk Kim 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2016-11-03 16:26 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, yuchao0, liushuoran From: Chao Yu <yuchao0@huawei.com> i_times of inode will be set with current system time which can be configured through 'date', so it's not safe to judge dnode block as garbage data depend on i_times. Now, we have used enhanced 'cp_ver + cp' crc method to verify valid dnode block, so I expect recoverying invalid dnode is almost not possible. Signed-off-by: Chao Yu <yuchao0@huawei.com> --- fs/f2fs/recovery.c | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c index d2ba4da..62523b2 100644 --- a/fs/f2fs/recovery.c +++ b/fs/f2fs/recovery.c @@ -196,32 +196,6 @@ static void recover_inode(struct inode *inode, struct page *page) ino_of_node(page), name); } -static bool is_same_inode(struct inode *inode, struct page *ipage) -{ - struct f2fs_inode *ri = F2FS_INODE(ipage); - struct timespec disk; - - if (!IS_INODE(ipage)) - return true; - - disk.tv_sec = le64_to_cpu(ri->i_ctime); - disk.tv_nsec = le32_to_cpu(ri->i_ctime_nsec); - if (timespec_compare(&inode->i_ctime, &disk) > 0) - return false; - - disk.tv_sec = le64_to_cpu(ri->i_atime); - disk.tv_nsec = le32_to_cpu(ri->i_atime_nsec); - if (timespec_compare(&inode->i_atime, &disk) > 0) - return false; - - disk.tv_sec = le64_to_cpu(ri->i_mtime); - disk.tv_nsec = le32_to_cpu(ri->i_mtime_nsec); - if (timespec_compare(&inode->i_mtime, &disk) > 0) - return false; - - return true; -} - static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head) { struct curseg_info *curseg; @@ -248,10 +222,7 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head) goto next; entry = get_fsync_inode(head, ino_of_node(page)); - if (entry) { - if (!is_same_inode(entry->inode, page)) - goto next; - } else { + if (!entry) { if (IS_INODE(page) && is_dent_dnode(page)) { err = recover_inode_page(sbi, page); if (err) -- 2.10.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times 2016-11-03 16:26 ` [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times Chao Yu @ 2016-11-03 18:02 ` Jaegeuk Kim 2016-11-04 8:30 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Jaegeuk Kim @ 2016-11-03 18:02 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, yuchao0, liushuoran On Fri, Nov 04, 2016 at 12:26:56AM +0800, Chao Yu wrote: > From: Chao Yu <yuchao0@huawei.com> > > i_times of inode will be set with current system time which can be > configured through 'date', so it's not safe to judge dnode block as > garbage data depend on i_times. This is not to detect garbage data, but to skip redundant unchanged inode. Anyway, in the code part, looks good to me. Thanks, > > Now, we have used enhanced 'cp_ver + cp' crc method to verify valid > dnode block, so I expect recoverying invalid dnode is almost not > possible. > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > --- > fs/f2fs/recovery.c | 31 +------------------------------ > 1 file changed, 1 insertion(+), 30 deletions(-) > > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c > index d2ba4da..62523b2 100644 > --- a/fs/f2fs/recovery.c > +++ b/fs/f2fs/recovery.c > @@ -196,32 +196,6 @@ static void recover_inode(struct inode *inode, struct page *page) > ino_of_node(page), name); > } > > -static bool is_same_inode(struct inode *inode, struct page *ipage) > -{ > - struct f2fs_inode *ri = F2FS_INODE(ipage); > - struct timespec disk; > - > - if (!IS_INODE(ipage)) > - return true; > - > - disk.tv_sec = le64_to_cpu(ri->i_ctime); > - disk.tv_nsec = le32_to_cpu(ri->i_ctime_nsec); > - if (timespec_compare(&inode->i_ctime, &disk) > 0) > - return false; > - > - disk.tv_sec = le64_to_cpu(ri->i_atime); > - disk.tv_nsec = le32_to_cpu(ri->i_atime_nsec); > - if (timespec_compare(&inode->i_atime, &disk) > 0) > - return false; > - > - disk.tv_sec = le64_to_cpu(ri->i_mtime); > - disk.tv_nsec = le32_to_cpu(ri->i_mtime_nsec); > - if (timespec_compare(&inode->i_mtime, &disk) > 0) > - return false; > - > - return true; > -} > - > static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head) > { > struct curseg_info *curseg; > @@ -248,10 +222,7 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head) > goto next; > > entry = get_fsync_inode(head, ino_of_node(page)); > - if (entry) { > - if (!is_same_inode(entry->inode, page)) > - goto next; > - } else { > + if (!entry) { > if (IS_INODE(page) && is_dent_dnode(page)) { > err = recover_inode_page(sbi, page); > if (err) > -- > 2.10.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times 2016-11-03 18:02 ` Jaegeuk Kim @ 2016-11-04 8:30 ` Chao Yu 2016-11-04 22:53 ` Jaegeuk Kim 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2016-11-04 8:30 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, liushuoran On 2016/11/4 2:02, Jaegeuk Kim wrote: > On Fri, Nov 04, 2016 at 12:26:56AM +0800, Chao Yu wrote: >> From: Chao Yu <yuchao0@huawei.com> >> >> i_times of inode will be set with current system time which can be >> configured through 'date', so it's not safe to judge dnode block as >> garbage data depend on i_times. > > This is not to detect garbage data, but to skip redundant unchanged inode. Oops, seems 807b1e1c8e08 ("f2fs: do not recover from previous remained wrong dnodes") did't describe like that. But after reading the codes, it looks like the purpose of this change is to skip unchanged inode. So, commit log in original is incorrect, right? Thanks, ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times 2016-11-04 8:30 ` Chao Yu @ 2016-11-04 22:53 ` Jaegeuk Kim 2016-11-05 3:12 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Jaegeuk Kim @ 2016-11-04 22:53 UTC (permalink / raw) To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel, liushuoran On Fri, Nov 04, 2016 at 04:30:09PM +0800, Chao Yu wrote: > On 2016/11/4 2:02, Jaegeuk Kim wrote: > > On Fri, Nov 04, 2016 at 12:26:56AM +0800, Chao Yu wrote: > >> From: Chao Yu <yuchao0@huawei.com> > >> > >> i_times of inode will be set with current system time which can be > >> configured through 'date', so it's not safe to judge dnode block as > >> garbage data depend on i_times. > > > > This is not to detect garbage data, but to skip redundant unchanged inode. > > Oops, seems 807b1e1c8e08 ("f2fs: do not recover from previous remained wrong > dnodes") did't describe like that. But after reading the codes, it looks like > the purpose of this change is to skip unchanged inode. So, commit log in > original is incorrect, right? Oh, right. This indicats both of purposes: stale data and detecting same inodes. Let me just revert the original patch. Thanks, > > Thanks, ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times 2016-11-04 22:53 ` Jaegeuk Kim @ 2016-11-05 3:12 ` Chao Yu 2016-11-05 7:02 ` Jaegeuk Kim 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2016-11-05 3:12 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, liushuoran On 2016/11/5 6:53, Jaegeuk Kim wrote: > On Fri, Nov 04, 2016 at 04:30:09PM +0800, Chao Yu wrote: >> On 2016/11/4 2:02, Jaegeuk Kim wrote: >>> On Fri, Nov 04, 2016 at 12:26:56AM +0800, Chao Yu wrote: >>>> From: Chao Yu <yuchao0@huawei.com> >>>> >>>> i_times of inode will be set with current system time which can be >>>> configured through 'date', so it's not safe to judge dnode block as >>>> garbage data depend on i_times. >>> >>> This is not to detect garbage data, but to skip redundant unchanged inode. >> >> Oops, seems 807b1e1c8e08 ("f2fs: do not recover from previous remained wrong >> dnodes") did't describe like that. But after reading the codes, it looks like >> the purpose of this change is to skip unchanged inode. So, commit log in >> original is incorrect, right? > > Oh, right. This indicats both of purposes: stale data and detecting same inodes. Alright. > Let me just revert the original patch. I can see that you have did reverting it in your git tree, but seems commit number is not right. Could you please merge my updated v2 patch instead? Thanks, > > Thanks, > >> >> Thanks, ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times 2016-11-05 3:12 ` Chao Yu @ 2016-11-05 7:02 ` Jaegeuk Kim 0 siblings, 0 replies; 7+ messages in thread From: Jaegeuk Kim @ 2016-11-05 7:02 UTC (permalink / raw) To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel, liushuoran On Sat, Nov 05, 2016 at 11:12:00AM +0800, Chao Yu wrote: > On 2016/11/5 6:53, Jaegeuk Kim wrote: > > On Fri, Nov 04, 2016 at 04:30:09PM +0800, Chao Yu wrote: > >> On 2016/11/4 2:02, Jaegeuk Kim wrote: > >>> On Fri, Nov 04, 2016 at 12:26:56AM +0800, Chao Yu wrote: > >>>> From: Chao Yu <yuchao0@huawei.com> > >>>> > >>>> i_times of inode will be set with current system time which can be > >>>> configured through 'date', so it's not safe to judge dnode block as > >>>> garbage data depend on i_times. > >>> > >>> This is not to detect garbage data, but to skip redundant unchanged inode. > >> > >> Oops, seems 807b1e1c8e08 ("f2fs: do not recover from previous remained wrong > >> dnodes") did't describe like that. But after reading the codes, it looks like > >> the purpose of this change is to skip unchanged inode. So, commit log in > >> original is incorrect, right? > > > > Oh, right. This indicats both of purposes: stale data and detecting same inodes. > > Alright. > > > Let me just revert the original patch. > > I can see that you have did reverting it in your git tree, but seems commit > number is not right. > > Could you please merge my updated v2 patch instead? Oops, yeah. :) > > Thanks, > > > > > Thanks, > > > >> > >> Thanks, ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-11-05 7:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-03 16:26 [PATCH 1/2] f2fs: fix wrong i_atime recovery Chao Yu 2016-11-03 16:26 ` [PATCH 2/2] f2fs: don't skip recovering inode depend on i_times Chao Yu 2016-11-03 18:02 ` Jaegeuk Kim 2016-11-04 8:30 ` Chao Yu 2016-11-04 22:53 ` Jaegeuk Kim 2016-11-05 3:12 ` Chao Yu 2016-11-05 7:02 ` Jaegeuk Kim
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).