* [PATCH RFC] f2fs: fix an error case of missing update inode page @ 2017-12-05 4:07 Yunlei He 2017-12-08 5:26 ` Jaegeuk Kim 2017-12-14 19:46 ` Jaegeuk Kim 0 siblings, 2 replies; 9+ messages in thread From: Yunlei He @ 2017-12-05 4:07 UTC (permalink / raw) To: jaegeuk, yuchao0, linux-f2fs-devel; +Cc: ning.jia, heyunlei -Thread A Thread B -write_checkpoint -block_operations -f2fs_unlock_all -f2fs_sync_file -f2fs_write_inode -f2fs_inode_synced -f2fs_sync_inode_meta -sync_node_pages -set_page_drity In this case, if sudden power off without next new checkpoint, the last inode page update will lost. wb_writeback is same with fsync. Signed-off-by: Yunlei He <heyunlei@huawei.com> --- fs/f2fs/f2fs.h | 4 ++-- fs/f2fs/inode.c | 15 +++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 82f1dc3..38f9324 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat, struct inode *f2fs_iget(struct super_block *sb, unsigned long ino); struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino); int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink); -int update_inode(struct inode *inode, struct page *node_page); -int update_inode_page(struct inode *inode); +void update_inode(struct inode *inode, struct page *node_page); +void update_inode_page(struct inode *inode); int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc); void f2fs_evict_inode(struct inode *inode); void handle_failed_inode(struct inode *inode); diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c index b4c4f2b..10d3c7c 100644 --- a/fs/f2fs/inode.c +++ b/fs/f2fs/inode.c @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino) return inode; } -int update_inode(struct inode *inode, struct page *node_page) +void update_inode(struct inode *inode, struct page *node_page) { struct f2fs_inode *ri; struct extent_tree *et = F2FS_I(inode)->extent_tree; - f2fs_inode_synced(inode); - f2fs_wait_on_page_writeback(node_page, NODE, true); + set_page_dirty(node_page); + + f2fs_inode_synced(inode); ri = F2FS_INODE(node_page); @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page) if (inode->i_nlink == 0) clear_inline_node(node_page); - return set_page_dirty(node_page); } -int update_inode_page(struct inode *inode) +void update_inode_page(struct inode *inode) { struct f2fs_sb_info *sbi = F2FS_I_SB(inode); struct page *node_page; @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode) } else if (err != -ENOENT) { f2fs_stop_checkpoint(sbi, false); } - return 0; + return; } - ret = update_inode(inode, node_page); + update_inode(inode, node_page); f2fs_put_page(node_page, 1); - return ret; } int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) -- 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page 2017-12-05 4:07 [PATCH RFC] f2fs: fix an error case of missing update inode page Yunlei He @ 2017-12-08 5:26 ` Jaegeuk Kim 2017-12-08 5:44 ` heyunlei 2017-12-14 19:46 ` Jaegeuk Kim 1 sibling, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2017-12-08 5:26 UTC (permalink / raw) To: Yunlei He; +Cc: ning.jia, linux-f2fs-devel On 12/05, Yunlei He wrote: > -Thread A Thread B > > -write_checkpoint > -block_operations > -f2fs_unlock_all -f2fs_sync_file > -f2fs_write_inode > -f2fs_inode_synced > > -f2fs_sync_inode_meta > -sync_node_pages > -set_page_drity > > In this case, if sudden power off without next new checkpoint, > the last inode page update will lost. wb_writeback is same with > fsync. BTW, what do you mean wb_writeback is same with fsync? Thanks, > > Signed-off-by: Yunlei He <heyunlei@huawei.com> > --- > fs/f2fs/f2fs.h | 4 ++-- > fs/f2fs/inode.c | 15 +++++++-------- > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 82f1dc3..38f9324 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat, > struct inode *f2fs_iget(struct super_block *sb, unsigned long ino); > struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino); > int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink); > -int update_inode(struct inode *inode, struct page *node_page); > -int update_inode_page(struct inode *inode); > +void update_inode(struct inode *inode, struct page *node_page); > +void update_inode_page(struct inode *inode); > int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc); > void f2fs_evict_inode(struct inode *inode); > void handle_failed_inode(struct inode *inode); > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > index b4c4f2b..10d3c7c 100644 > --- a/fs/f2fs/inode.c > +++ b/fs/f2fs/inode.c > @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino) > return inode; > } > > -int update_inode(struct inode *inode, struct page *node_page) > +void update_inode(struct inode *inode, struct page *node_page) > { > struct f2fs_inode *ri; > struct extent_tree *et = F2FS_I(inode)->extent_tree; > > - f2fs_inode_synced(inode); > - > f2fs_wait_on_page_writeback(node_page, NODE, true); > + set_page_dirty(node_page); > + > + f2fs_inode_synced(inode); > > ri = F2FS_INODE(node_page); > > @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page) > if (inode->i_nlink == 0) > clear_inline_node(node_page); > > - return set_page_dirty(node_page); > } > > -int update_inode_page(struct inode *inode) > +void update_inode_page(struct inode *inode) > { > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > struct page *node_page; > @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode) > } else if (err != -ENOENT) { > f2fs_stop_checkpoint(sbi, false); > } > - return 0; > + return; > } > - ret = update_inode(inode, node_page); > + update_inode(inode, node_page); > f2fs_put_page(node_page, 1); > - return ret; > } > > int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > -- > 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page 2017-12-08 5:26 ` Jaegeuk Kim @ 2017-12-08 5:44 ` heyunlei 2017-12-11 13:31 ` Chao Yu 0 siblings, 1 reply; 9+ messages in thread From: heyunlei @ 2017-12-08 5:44 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net >-----Original Message----- >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] >Sent: Friday, December 08, 2017 1:27 PM >To: heyunlei >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler) >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page > >On 12/05, Yunlei He wrote: >> -Thread A Thread B >> >> -write_checkpoint >> -block_operations >> -f2fs_unlock_all -f2fs_sync_file >> -f2fs_write_inode >> -f2fs_inode_synced >> >> -f2fs_sync_inode_meta >> -sync_node_pages >> -set_page_drity >> >> In this case, if sudden power off without next new checkpoint, >> the last inode page update will lost. wb_writeback is same with >> fsync. > >BTW, what do you mean wb_writeback is same with fsync? Background write back will call f2fs_write_inode, which is same with f2fs_sync_file. Thanks. > >Thanks, > >> >> Signed-off-by: Yunlei He <heyunlei@huawei.com> >> --- >> fs/f2fs/f2fs.h | 4 ++-- >> fs/f2fs/inode.c | 15 +++++++-------- >> 2 files changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 82f1dc3..38f9324 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat, >> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino); >> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino); >> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink); >> -int update_inode(struct inode *inode, struct page *node_page); >> -int update_inode_page(struct inode *inode); >> +void update_inode(struct inode *inode, struct page *node_page); >> +void update_inode_page(struct inode *inode); >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc); >> void f2fs_evict_inode(struct inode *inode); >> void handle_failed_inode(struct inode *inode); >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >> index b4c4f2b..10d3c7c 100644 >> --- a/fs/f2fs/inode.c >> +++ b/fs/f2fs/inode.c >> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino) >> return inode; >> } >> >> -int update_inode(struct inode *inode, struct page *node_page) >> +void update_inode(struct inode *inode, struct page *node_page) >> { >> struct f2fs_inode *ri; >> struct extent_tree *et = F2FS_I(inode)->extent_tree; >> >> - f2fs_inode_synced(inode); >> - >> f2fs_wait_on_page_writeback(node_page, NODE, true); >> + set_page_dirty(node_page); >> + >> + f2fs_inode_synced(inode); >> >> ri = F2FS_INODE(node_page); >> >> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page) >> if (inode->i_nlink == 0) >> clear_inline_node(node_page); >> >> - return set_page_dirty(node_page); >> } >> >> -int update_inode_page(struct inode *inode) >> +void update_inode_page(struct inode *inode) >> { >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >> struct page *node_page; >> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode) >> } else if (err != -ENOENT) { >> f2fs_stop_checkpoint(sbi, false); >> } >> - return 0; >> + return; >> } >> - ret = update_inode(inode, node_page); >> + update_inode(inode, node_page); >> f2fs_put_page(node_page, 1); >> - return ret; >> } >> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) >> -- >> 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page 2017-12-08 5:44 ` heyunlei @ 2017-12-11 13:31 ` Chao Yu 0 siblings, 0 replies; 9+ messages in thread From: Chao Yu @ 2017-12-11 13:31 UTC (permalink / raw) To: heyunlei, Jaegeuk Kim Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net On 2017/12/8 13:44, heyunlei wrote: > > >> -----Original Message----- >> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] >> Sent: Friday, December 08, 2017 1:27 PM >> To: heyunlei >> Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler) >> Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page >> >> On 12/05, Yunlei He wrote: >>> -Thread A Thread B >>> >>> -write_checkpoint >>> -block_operations >>> -f2fs_unlock_all -f2fs_sync_file >>> -f2fs_write_inode >>> -f2fs_inode_synced >>> >>> -f2fs_sync_inode_meta >>> -sync_node_pages >>> -set_page_drity >>> >>> In this case, if sudden power off without next new checkpoint, >>> the last inode page update will lost. wb_writeback is same with >>> fsync. >> >> BTW, what do you mean wb_writeback is same with fsync? > > Background write back will call f2fs_write_inode, which is same with f2fs_sync_file. Can you add related call stack in commit log? Other part looks good to me. Reviewed-by: Chao Yu <yuchao0@huawei.com> Thanks, ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page 2017-12-05 4:07 [PATCH RFC] f2fs: fix an error case of missing update inode page Yunlei He 2017-12-08 5:26 ` Jaegeuk Kim @ 2017-12-14 19:46 ` Jaegeuk Kim 2017-12-15 2:24 ` heyunlei 1 sibling, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2017-12-14 19:46 UTC (permalink / raw) To: Yunlei He; +Cc: ning.jia, linux-f2fs-devel On 12/05, Yunlei He wrote: > -Thread A Thread B > > -write_checkpoint > -block_operations > -f2fs_unlock_all -f2fs_sync_file > -f2fs_write_inode > -f2fs_inode_synced > > -f2fs_sync_inode_meta > -sync_node_pages > -set_page_drity > > In this case, if sudden power off without next new checkpoint, > the last inode page update will lost. wb_writeback is same with > fsync. Does this patch fix the problem? -write_checkpoint -block_operations -f2fs_unlock_all -f2fs_sync_file -f2fs_write_inode -set_page_drity -f2fs_inode_synced -f2fs_sync_inode_meta -sync_node_pages I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again. Thanks, > > Signed-off-by: Yunlei He <heyunlei@huawei.com> > --- > fs/f2fs/f2fs.h | 4 ++-- > fs/f2fs/inode.c | 15 +++++++-------- > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 82f1dc3..38f9324 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat, > struct inode *f2fs_iget(struct super_block *sb, unsigned long ino); > struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino); > int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink); > -int update_inode(struct inode *inode, struct page *node_page); > -int update_inode_page(struct inode *inode); > +void update_inode(struct inode *inode, struct page *node_page); > +void update_inode_page(struct inode *inode); > int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc); > void f2fs_evict_inode(struct inode *inode); > void handle_failed_inode(struct inode *inode); > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > index b4c4f2b..10d3c7c 100644 > --- a/fs/f2fs/inode.c > +++ b/fs/f2fs/inode.c > @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino) > return inode; > } > > -int update_inode(struct inode *inode, struct page *node_page) > +void update_inode(struct inode *inode, struct page *node_page) > { > struct f2fs_inode *ri; > struct extent_tree *et = F2FS_I(inode)->extent_tree; > > - f2fs_inode_synced(inode); > - > f2fs_wait_on_page_writeback(node_page, NODE, true); > + set_page_dirty(node_page); > + > + f2fs_inode_synced(inode); > > ri = F2FS_INODE(node_page); > > @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page) > if (inode->i_nlink == 0) > clear_inline_node(node_page); > > - return set_page_dirty(node_page); > } > > -int update_inode_page(struct inode *inode) > +void update_inode_page(struct inode *inode) > { > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > struct page *node_page; > @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode) > } else if (err != -ENOENT) { > f2fs_stop_checkpoint(sbi, false); > } > - return 0; > + return; > } > - ret = update_inode(inode, node_page); > + update_inode(inode, node_page); > f2fs_put_page(node_page, 1); > - return ret; > } > > int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > -- > 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page 2017-12-14 19:46 ` Jaegeuk Kim @ 2017-12-15 2:24 ` heyunlei 2017-12-15 4:20 ` Jaegeuk Kim 0 siblings, 1 reply; 9+ messages in thread From: heyunlei @ 2017-12-15 2:24 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net >-----Original Message----- >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] >Sent: Friday, December 15, 2017 3:46 AM >To: heyunlei >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler) >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page > >On 12/05, Yunlei He wrote: >> -Thread A Thread B >> >> -write_checkpoint >> -block_operations >> -f2fs_unlock_all -f2fs_sync_file >> -f2fs_write_inode >> -f2fs_inode_synced >> >> -f2fs_sync_inode_meta >> -sync_node_pages >> -set_page_drity >> >> In this case, if sudden power off without next new checkpoint, >> the last inode page update will lost. wb_writeback is same with >> fsync. > >Does this patch fix the problem? > I modify code as below: @@ -366,7 +366,7 @@ int update_inode(struct inode *inode, struct page *node_page) struct extent_tree *et = F2FS_I(inode)->extent_tree; f2fs_inode_synced(inode); - + msleep(10000); f2fs_wait_on_page_writeback(node_page, NODE, true); shell 1: shell2: dd if=/dev/zero of=./test bs=1M count=10 sync echo "hello" >> ./test fsync test // sleep 10s sync //return quickly echo c > /proc/sysrq-trigger //before fsync return fsck will find iblocks mismatch, and with this patch it 's ok. Besides,I came across another error like this: f2fs_inode-> i_xattr_nid is non-zero, but node blkaddr in raw nat entry is NULL. This case makes sense to this problem also. > -write_checkpoint > -block_operations > -f2fs_unlock_all -f2fs_sync_file > -f2fs_write_inode > -set_page_drity > -f2fs_inode_synced > -f2fs_sync_inode_meta > -sync_node_pages > >I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again. It can also work well with f2fs_lock_op(), I am afraid other cases will update inode like f2fs_write_inode without f2fs_lock_op Thanks > >Thanks, > >> >> Signed-off-by: Yunlei He <heyunlei@huawei.com> >> --- >> fs/f2fs/f2fs.h | 4 ++-- >> fs/f2fs/inode.c | 15 +++++++-------- >> 2 files changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 82f1dc3..38f9324 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat, >> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino); >> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino); >> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink); >> -int update_inode(struct inode *inode, struct page *node_page); >> -int update_inode_page(struct inode *inode); >> +void update_inode(struct inode *inode, struct page *node_page); >> +void update_inode_page(struct inode *inode); >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc); >> void f2fs_evict_inode(struct inode *inode); >> void handle_failed_inode(struct inode *inode); >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >> index b4c4f2b..10d3c7c 100644 >> --- a/fs/f2fs/inode.c >> +++ b/fs/f2fs/inode.c >> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino) >> return inode; >> } >> >> -int update_inode(struct inode *inode, struct page *node_page) >> +void update_inode(struct inode *inode, struct page *node_page) >> { >> struct f2fs_inode *ri; >> struct extent_tree *et = F2FS_I(inode)->extent_tree; >> >> - f2fs_inode_synced(inode); >> - >> f2fs_wait_on_page_writeback(node_page, NODE, true); >> + set_page_dirty(node_page); >> + >> + f2fs_inode_synced(inode); >> >> ri = F2FS_INODE(node_page); >> >> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page) >> if (inode->i_nlink == 0) >> clear_inline_node(node_page); >> >> - return set_page_dirty(node_page); >> } >> >> -int update_inode_page(struct inode *inode) >> +void update_inode_page(struct inode *inode) >> { >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >> struct page *node_page; >> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode) >> } else if (err != -ENOENT) { >> f2fs_stop_checkpoint(sbi, false); >> } >> - return 0; >> + return; >> } >> - ret = update_inode(inode, node_page); >> + update_inode(inode, node_page); >> f2fs_put_page(node_page, 1); >> - return ret; >> } >> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) >> -- >> 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page 2017-12-15 2:24 ` heyunlei @ 2017-12-15 4:20 ` Jaegeuk Kim 2017-12-15 6:47 ` heyunlei 0 siblings, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2017-12-15 4:20 UTC (permalink / raw) To: heyunlei; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net On 12/15, heyunlei wrote: > > > >-----Original Message----- > >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > >Sent: Friday, December 15, 2017 3:46 AM > >To: heyunlei > >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler) > >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page > > > >On 12/05, Yunlei He wrote: > >> -Thread A Thread B > >> > >> -write_checkpoint > >> -block_operations > >> -f2fs_unlock_all -f2fs_sync_file > >> -f2fs_write_inode > >> -f2fs_inode_synced > >> > >> -f2fs_sync_inode_meta > >> -sync_node_pages > >> -set_page_drity > >> > >> In this case, if sudden power off without next new checkpoint, > >> the last inode page update will lost. wb_writeback is same with > >> fsync. > > > >Does this patch fix the problem? > > > I modify code as below: > @@ -366,7 +366,7 @@ int update_inode(struct inode *inode, struct page *node_page) > struct extent_tree *et = F2FS_I(inode)->extent_tree; > > f2fs_inode_synced(inode); > - > + msleep(10000); > f2fs_wait_on_page_writeback(node_page, NODE, true); > > shell 1: shell2: > dd if=/dev/zero of=./test bs=1M count=10 > sync > echo "hello" >> ./test > fsync test // sleep 10s > sync //return quickly > echo c > /proc/sysrq-trigger //before fsync return > fsck will find iblocks mismatch, and with this patch it 's ok. So, do you mean it was fixed by adding msleep() on top of this patch as well? Like: - f2fs_write_inode() - set_page_drity - f2fs_inode_synced - msleep(10000); > > Besides,I came across another error like this: > > f2fs_inode-> i_xattr_nid is non-zero, but node blkaddr in raw nat entry is NULL. > > This case makes sense to this problem also. > > > -write_checkpoint > > -block_operations > > -f2fs_unlock_all -f2fs_sync_file > > -f2fs_write_inode > > -set_page_drity > > -f2fs_inode_synced > > -f2fs_sync_inode_meta > > -sync_node_pages > > > >I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again. > > It can also work well with f2fs_lock_op(), I am afraid other cases will update inode > like f2fs_write_inode without f2fs_lock_op > > Thanks > > > >Thanks, > > > >> > >> Signed-off-by: Yunlei He <heyunlei@huawei.com> > >> --- > >> fs/f2fs/f2fs.h | 4 ++-- > >> fs/f2fs/inode.c | 15 +++++++-------- > >> 2 files changed, 9 insertions(+), 10 deletions(-) > >> > >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >> index 82f1dc3..38f9324 100644 > >> --- a/fs/f2fs/f2fs.h > >> +++ b/fs/f2fs/f2fs.h > >> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat, > >> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino); > >> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino); > >> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink); > >> -int update_inode(struct inode *inode, struct page *node_page); > >> -int update_inode_page(struct inode *inode); > >> +void update_inode(struct inode *inode, struct page *node_page); > >> +void update_inode_page(struct inode *inode); > >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc); > >> void f2fs_evict_inode(struct inode *inode); > >> void handle_failed_inode(struct inode *inode); > >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > >> index b4c4f2b..10d3c7c 100644 > >> --- a/fs/f2fs/inode.c > >> +++ b/fs/f2fs/inode.c > >> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino) > >> return inode; > >> } > >> > >> -int update_inode(struct inode *inode, struct page *node_page) > >> +void update_inode(struct inode *inode, struct page *node_page) > >> { > >> struct f2fs_inode *ri; > >> struct extent_tree *et = F2FS_I(inode)->extent_tree; > >> > >> - f2fs_inode_synced(inode); > >> - > >> f2fs_wait_on_page_writeback(node_page, NODE, true); > >> + set_page_dirty(node_page); > >> + > >> + f2fs_inode_synced(inode); > >> > >> ri = F2FS_INODE(node_page); > >> > >> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page) > >> if (inode->i_nlink == 0) > >> clear_inline_node(node_page); > >> > >> - return set_page_dirty(node_page); > >> } > >> > >> -int update_inode_page(struct inode *inode) > >> +void update_inode_page(struct inode *inode) > >> { > >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > >> struct page *node_page; > >> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode) > >> } else if (err != -ENOENT) { > >> f2fs_stop_checkpoint(sbi, false); > >> } > >> - return 0; > >> + return; > >> } > >> - ret = update_inode(inode, node_page); > >> + update_inode(inode, node_page); > >> f2fs_put_page(node_page, 1); > >> - return ret; > >> } > >> > >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > >> -- > >> 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page 2017-12-15 4:20 ` Jaegeuk Kim @ 2017-12-15 6:47 ` heyunlei 2017-12-19 23:37 ` Jaegeuk Kim 0 siblings, 1 reply; 9+ messages in thread From: heyunlei @ 2017-12-15 6:47 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net >-----Original Message----- >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] >Sent: Friday, December 15, 2017 12:21 PM >To: heyunlei >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler) >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page > >On 12/15, heyunlei wrote: >> >> >> >-----Original Message----- >> >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] >> >Sent: Friday, December 15, 2017 3:46 AM >> >To: heyunlei >> >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler) >> >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page >> > >> >On 12/05, Yunlei He wrote: >> >> -Thread A Thread B >> >> >> >> -write_checkpoint >> >> -block_operations >> >> -f2fs_unlock_all -f2fs_sync_file >> >> -f2fs_write_inode >> >> -f2fs_inode_synced >> >> >> >> -f2fs_sync_inode_meta >> >> -sync_node_pages >> >> -set_page_drity >> >> >> >> In this case, if sudden power off without next new checkpoint, >> >> the last inode page update will lost. wb_writeback is same with >> >> fsync. >> > >> >Does this patch fix the problem? >> > >> I modify code as below: >> @@ -366,7 +366,7 @@ int update_inode(struct inode *inode, struct page *node_page) >> struct extent_tree *et = F2FS_I(inode)->extent_tree; >> >> f2fs_inode_synced(inode); >> - >> + msleep(10000); >> f2fs_wait_on_page_writeback(node_page, NODE, true); >> >> shell 1: shell2: >> dd if=/dev/zero of=./test bs=1M count=10 >> sync >> echo "hello" >> ./test >> fsync test // sleep 10s >> sync //return quickly >> echo c > /proc/sysrq-trigger //before fsync return >> fsck will find iblocks mismatch, and with this patch it 's ok. > >So, do you mean it was fixed by adding msleep() on top of this patch as well? >Like: > - f2fs_write_inode() > - set_page_drity > - f2fs_inode_synced > - msleep(10000); > Yes, write_checkpoint will be blocked in sync dirty node pages, until fsync update inode page and unlock node page. Thanks. >> >> Besides,I came across another error like this: >> >> f2fs_inode-> i_xattr_nid is non-zero, but node blkaddr in raw nat entry is NULL. >> >> This case makes sense to this problem also. >> >> > -write_checkpoint >> > -block_operations >> > -f2fs_unlock_all -f2fs_sync_file >> > -f2fs_write_inode >> > -set_page_drity >> > -f2fs_inode_synced >> > -f2fs_sync_inode_meta >> > -sync_node_pages >> > >> >I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again. >> >> It can also work well with f2fs_lock_op(), I am afraid other cases will update inode >> like f2fs_write_inode without f2fs_lock_op >> >> Thanks >> > >> >Thanks, >> > >> >> >> >> Signed-off-by: Yunlei He <heyunlei@huawei.com> >> >> --- >> >> fs/f2fs/f2fs.h | 4 ++-- >> >> fs/f2fs/inode.c | 15 +++++++-------- >> >> 2 files changed, 9 insertions(+), 10 deletions(-) >> >> >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> >> index 82f1dc3..38f9324 100644 >> >> --- a/fs/f2fs/f2fs.h >> >> +++ b/fs/f2fs/f2fs.h >> >> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat, >> >> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino); >> >> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino); >> >> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink); >> >> -int update_inode(struct inode *inode, struct page *node_page); >> >> -int update_inode_page(struct inode *inode); >> >> +void update_inode(struct inode *inode, struct page *node_page); >> >> +void update_inode_page(struct inode *inode); >> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc); >> >> void f2fs_evict_inode(struct inode *inode); >> >> void handle_failed_inode(struct inode *inode); >> >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >> >> index b4c4f2b..10d3c7c 100644 >> >> --- a/fs/f2fs/inode.c >> >> +++ b/fs/f2fs/inode.c >> >> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino) >> >> return inode; >> >> } >> >> >> >> -int update_inode(struct inode *inode, struct page *node_page) >> >> +void update_inode(struct inode *inode, struct page *node_page) >> >> { >> >> struct f2fs_inode *ri; >> >> struct extent_tree *et = F2FS_I(inode)->extent_tree; >> >> >> >> - f2fs_inode_synced(inode); >> >> - >> >> f2fs_wait_on_page_writeback(node_page, NODE, true); >> >> + set_page_dirty(node_page); >> >> + >> >> + f2fs_inode_synced(inode); >> >> >> >> ri = F2FS_INODE(node_page); >> >> >> >> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page) >> >> if (inode->i_nlink == 0) >> >> clear_inline_node(node_page); >> >> >> >> - return set_page_dirty(node_page); >> >> } >> >> >> >> -int update_inode_page(struct inode *inode) >> >> +void update_inode_page(struct inode *inode) >> >> { >> >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >> >> struct page *node_page; >> >> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode) >> >> } else if (err != -ENOENT) { >> >> f2fs_stop_checkpoint(sbi, false); >> >> } >> >> - return 0; >> >> + return; >> >> } >> >> - ret = update_inode(inode, node_page); >> >> + update_inode(inode, node_page); >> >> f2fs_put_page(node_page, 1); >> >> - return ret; >> >> } >> >> >> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) >> >> -- >> >> 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page 2017-12-15 6:47 ` heyunlei @ 2017-12-19 23:37 ` Jaegeuk Kim 0 siblings, 0 replies; 9+ messages in thread From: Jaegeuk Kim @ 2017-12-19 23:37 UTC (permalink / raw) To: heyunlei; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net On 12/15, heyunlei wrote: > > > >-----Original Message----- > >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > >Sent: Friday, December 15, 2017 12:21 PM > >To: heyunlei > >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler) > >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page > > > >On 12/15, heyunlei wrote: > >> > >> > >> >-----Original Message----- > >> >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > >> >Sent: Friday, December 15, 2017 3:46 AM > >> >To: heyunlei > >> >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler) > >> >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page > >> > > >> >On 12/05, Yunlei He wrote: > >> >> -Thread A Thread B > >> >> > >> >> -write_checkpoint > >> >> -block_operations > >> >> -f2fs_unlock_all -f2fs_sync_file > >> >> -f2fs_write_inode > >> >> -f2fs_inode_synced > >> >> > >> >> -f2fs_sync_inode_meta > >> >> -sync_node_pages > >> >> -set_page_drity > >> >> > >> >> In this case, if sudden power off without next new checkpoint, > >> >> the last inode page update will lost. wb_writeback is same with > >> >> fsync. > >> > > >> >Does this patch fix the problem? > >> > > >> I modify code as below: > >> @@ -366,7 +366,7 @@ int update_inode(struct inode *inode, struct page *node_page) > >> struct extent_tree *et = F2FS_I(inode)->extent_tree; > >> > >> f2fs_inode_synced(inode); > >> - > >> + msleep(10000); > >> f2fs_wait_on_page_writeback(node_page, NODE, true); > >> > >> shell 1: shell2: > >> dd if=/dev/zero of=./test bs=1M count=10 > >> sync > >> echo "hello" >> ./test > >> fsync test // sleep 10s > >> sync //return quickly > >> echo c > /proc/sysrq-trigger //before fsync return > >> fsck will find iblocks mismatch, and with this patch it 's ok. > > > >So, do you mean it was fixed by adding msleep() on top of this patch as well? > >Like: > > - f2fs_write_inode() > > - set_page_drity > > - f2fs_inode_synced > > - msleep(10000); > > > Yes, write_checkpoint will be blocked in sync dirty node pages, until fsync update inode > page and unlock node page. Got it. I've concerned about inversion of PageDirty() and f2fs_inode_synced(), but for now, I can't find any corner case simply. Let me do stress test with this. Thanks, > > Thanks. > >> > >> Besides,I came across another error like this: > >> > >> f2fs_inode-> i_xattr_nid is non-zero, but node blkaddr in raw nat entry is NULL. > >> > >> This case makes sense to this problem also. > >> > >> > -write_checkpoint > >> > -block_operations > >> > -f2fs_unlock_all -f2fs_sync_file > >> > -f2fs_write_inode > >> > -set_page_drity > >> > -f2fs_inode_synced > >> > -f2fs_sync_inode_meta > >> > -sync_node_pages > >> > > >> >I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again. > >> > >> It can also work well with f2fs_lock_op(), I am afraid other cases will update inode > >> like f2fs_write_inode without f2fs_lock_op > >> > >> Thanks > >> > > >> >Thanks, > >> > > >> >> > >> >> Signed-off-by: Yunlei He <heyunlei@huawei.com> > >> >> --- > >> >> fs/f2fs/f2fs.h | 4 ++-- > >> >> fs/f2fs/inode.c | 15 +++++++-------- > >> >> 2 files changed, 9 insertions(+), 10 deletions(-) > >> >> > >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >> >> index 82f1dc3..38f9324 100644 > >> >> --- a/fs/f2fs/f2fs.h > >> >> +++ b/fs/f2fs/f2fs.h > >> >> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat, > >> >> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino); > >> >> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino); > >> >> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink); > >> >> -int update_inode(struct inode *inode, struct page *node_page); > >> >> -int update_inode_page(struct inode *inode); > >> >> +void update_inode(struct inode *inode, struct page *node_page); > >> >> +void update_inode_page(struct inode *inode); > >> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc); > >> >> void f2fs_evict_inode(struct inode *inode); > >> >> void handle_failed_inode(struct inode *inode); > >> >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > >> >> index b4c4f2b..10d3c7c 100644 > >> >> --- a/fs/f2fs/inode.c > >> >> +++ b/fs/f2fs/inode.c > >> >> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino) > >> >> return inode; > >> >> } > >> >> > >> >> -int update_inode(struct inode *inode, struct page *node_page) > >> >> +void update_inode(struct inode *inode, struct page *node_page) > >> >> { > >> >> struct f2fs_inode *ri; > >> >> struct extent_tree *et = F2FS_I(inode)->extent_tree; > >> >> > >> >> - f2fs_inode_synced(inode); > >> >> - > >> >> f2fs_wait_on_page_writeback(node_page, NODE, true); > >> >> + set_page_dirty(node_page); > >> >> + > >> >> + f2fs_inode_synced(inode); > >> >> > >> >> ri = F2FS_INODE(node_page); > >> >> > >> >> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page) > >> >> if (inode->i_nlink == 0) > >> >> clear_inline_node(node_page); > >> >> > >> >> - return set_page_dirty(node_page); > >> >> } > >> >> > >> >> -int update_inode_page(struct inode *inode) > >> >> +void update_inode_page(struct inode *inode) > >> >> { > >> >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > >> >> struct page *node_page; > >> >> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode) > >> >> } else if (err != -ENOENT) { > >> >> f2fs_stop_checkpoint(sbi, false); > >> >> } > >> >> - return 0; > >> >> + return; > >> >> } > >> >> - ret = update_inode(inode, node_page); > >> >> + update_inode(inode, node_page); > >> >> f2fs_put_page(node_page, 1); > >> >> - return ret; > >> >> } > >> >> > >> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > >> >> -- > >> >> 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-19 23:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-05 4:07 [PATCH RFC] f2fs: fix an error case of missing update inode page Yunlei He 2017-12-08 5:26 ` Jaegeuk Kim 2017-12-08 5:44 ` heyunlei 2017-12-11 13:31 ` Chao Yu 2017-12-14 19:46 ` Jaegeuk Kim 2017-12-15 2:24 ` heyunlei 2017-12-15 4:20 ` Jaegeuk Kim 2017-12-15 6:47 ` heyunlei 2017-12-19 23:37 ` 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).