* [PATCH 1/4] f2fs: add WARN_ON in f2fs_bug_on @ 2014-08-15 22:03 ` Jaegeuk Kim 0 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-15 22:03 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim This patch adds WARN_ON when f2fs_bug_on is disable to see kernel messages. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/f2fs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 2d009ae..2723b2d 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -24,7 +24,7 @@ #define f2fs_bug_on(condition) BUG_ON(condition) #define f2fs_down_write(x, y) down_write_nest_lock(x, y) #else -#define f2fs_bug_on(condition) +#define f2fs_bug_on(condition) WARN_ON(condition) #define f2fs_down_write(x, y) down_write(x) #endif -- 1.8.5.2 (Apple Git-48) ------------------------------------------------------------------------------ ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 1/4] f2fs: add WARN_ON in f2fs_bug_on @ 2014-08-15 22:03 ` Jaegeuk Kim 0 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-15 22:03 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim This patch adds WARN_ON when f2fs_bug_on is disable to see kernel messages. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/f2fs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 2d009ae..2723b2d 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -24,7 +24,7 @@ #define f2fs_bug_on(condition) BUG_ON(condition) #define f2fs_down_write(x, y) down_write_nest_lock(x, y) #else -#define f2fs_bug_on(condition) +#define f2fs_bug_on(condition) WARN_ON(condition) #define f2fs_down_write(x, y) down_write(x) #endif -- 1.8.5.2 (Apple Git-48) ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] f2fs: prevent checkpoint during roll-forward 2014-08-15 22:03 ` Jaegeuk Kim @ 2014-08-15 22:03 ` Jaegeuk Kim -1 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-15 22:03 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim Any checkpoint should not be done during the core roll-forward procedure. Especially, it includes error cases too. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/recovery.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c index 7ca7aad..d36ef35 100644 --- a/fs/f2fs/recovery.c +++ b/fs/f2fs/recovery.c @@ -459,6 +459,9 @@ int recover_fsync_data(struct f2fs_sb_info *sbi) /* step #1: find fsynced inode numbers */ sbi->por_doing = true; + /* prevent checkpoint */ + mutex_lock(&sbi->cp_mutex); + blkaddr = NEXT_FREE_BLKADDR(sbi, curseg); err = find_fsync_dnodes(sbi, &inode_list); @@ -490,8 +493,13 @@ out: /* Flush all the NAT/SIT pages */ while (get_pages(sbi, F2FS_DIRTY_META)) sync_meta_pages(sbi, META, LONG_MAX); + set_ckpt_flags(sbi->ckpt, CP_ERROR_FLAG); + mutex_unlock(&sbi->cp_mutex); } else if (need_writecp) { + mutex_unlock(&sbi->cp_mutex); write_checkpoint(sbi, false); + } else { + mutex_unlock(&sbi->cp_mutex); } return err; } -- 1.8.5.2 (Apple Git-48) ------------------------------------------------------------------------------ ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] f2fs: prevent checkpoint during roll-forward @ 2014-08-15 22:03 ` Jaegeuk Kim 0 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-15 22:03 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim Any checkpoint should not be done during the core roll-forward procedure. Especially, it includes error cases too. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/recovery.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c index 7ca7aad..d36ef35 100644 --- a/fs/f2fs/recovery.c +++ b/fs/f2fs/recovery.c @@ -459,6 +459,9 @@ int recover_fsync_data(struct f2fs_sb_info *sbi) /* step #1: find fsynced inode numbers */ sbi->por_doing = true; + /* prevent checkpoint */ + mutex_lock(&sbi->cp_mutex); + blkaddr = NEXT_FREE_BLKADDR(sbi, curseg); err = find_fsync_dnodes(sbi, &inode_list); @@ -490,8 +493,13 @@ out: /* Flush all the NAT/SIT pages */ while (get_pages(sbi, F2FS_DIRTY_META)) sync_meta_pages(sbi, META, LONG_MAX); + set_ckpt_flags(sbi->ckpt, CP_ERROR_FLAG); + mutex_unlock(&sbi->cp_mutex); } else if (need_writecp) { + mutex_unlock(&sbi->cp_mutex); write_checkpoint(sbi, false); + } else { + mutex_unlock(&sbi->cp_mutex); } return err; } -- 1.8.5.2 (Apple Git-48) ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] f2fs: avoid double lock in truncate_blocks 2014-08-15 22:03 ` Jaegeuk Kim @ 2014-08-15 22:03 ` Jaegeuk Kim -1 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-15 22:03 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim The init_inode_metadata calls truncate_blocks when error is occurred. The callers holds f2fs_lock_op, so we should not call it again in truncate_blocks. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/data.c | 2 +- fs/f2fs/dir.c | 2 +- fs/f2fs/f2fs.h | 2 +- fs/f2fs/file.c | 13 ++++++++----- fs/f2fs/inline.c | 2 +- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 68834e2..14cc3e8 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to) if (to > inode->i_size) { truncate_pagecache(inode, inode->i_size); - truncate_blocks(inode, inode->i_size); + truncate_blocks(inode, inode->i_size, true); } } diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index a69bbfa..155fb05 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -391,7 +391,7 @@ put_error: error: /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ truncate_inode_pages(&inode->i_data, 0); - truncate_blocks(inode, 0); + truncate_blocks(inode, 0, false); remove_dirty_dir_inode(inode); remove_inode_page(inode); return ERR_PTR(err); diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 2723b2d..7f976c1 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) */ int f2fs_sync_file(struct file *, loff_t, loff_t, int); void truncate_data_blocks(struct dnode_of_data *); -int truncate_blocks(struct inode *, u64); +int truncate_blocks(struct inode *, u64, bool); void f2fs_truncate(struct inode *); int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); int f2fs_setattr(struct dentry *, struct iattr *); diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index ecbdf6a..a8e97f8 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -422,7 +422,7 @@ out: f2fs_put_page(page, 1); } -int truncate_blocks(struct inode *inode, u64 from) +int truncate_blocks(struct inode *inode, u64 from, bool lock) { struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); unsigned int blocksize = inode->i_sb->s_blocksize; @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) free_from = (pgoff_t) ((from + blocksize - 1) >> (sbi->log_blocksize)); - f2fs_lock_op(sbi); + if (lock) + f2fs_lock_op(sbi); set_new_dnode(&dn, inode, NULL, NULL, 0); err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); if (err) { if (err == -ENOENT) goto free_next; - f2fs_unlock_op(sbi); + if (lock) + f2fs_unlock_op(sbi); trace_f2fs_truncate_blocks_exit(inode, err); return err; } @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) f2fs_put_dnode(&dn); free_next: err = truncate_inode_blocks(inode, free_from); - f2fs_unlock_op(sbi); + if (lock) + f2fs_unlock_op(sbi); done: /* lastly zero out the first data page */ truncate_partial_data_page(inode, from); @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) trace_f2fs_truncate(inode); - if (!truncate_blocks(inode, i_size_read(inode))) { + if (!truncate_blocks(inode, i_size_read(inode), true)) { inode->i_mtime = inode->i_ctime = CURRENT_TIME; mark_inode_dirty(inode); } diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index 520758b..4d1f39f 100644 --- a/fs/f2fs/inline.c +++ b/fs/f2fs/inline.c @@ -247,7 +247,7 @@ process_inline: update_inode(inode, ipage); f2fs_put_page(ipage, 1); } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { - truncate_blocks(inode, 0); + truncate_blocks(inode, 0, false); set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); goto process_inline; } -- 1.8.5.2 (Apple Git-48) ------------------------------------------------------------------------------ ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] f2fs: avoid double lock in truncate_blocks @ 2014-08-15 22:03 ` Jaegeuk Kim 0 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-15 22:03 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim The init_inode_metadata calls truncate_blocks when error is occurred. The callers holds f2fs_lock_op, so we should not call it again in truncate_blocks. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/data.c | 2 +- fs/f2fs/dir.c | 2 +- fs/f2fs/f2fs.h | 2 +- fs/f2fs/file.c | 13 ++++++++----- fs/f2fs/inline.c | 2 +- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 68834e2..14cc3e8 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to) if (to > inode->i_size) { truncate_pagecache(inode, inode->i_size); - truncate_blocks(inode, inode->i_size); + truncate_blocks(inode, inode->i_size, true); } } diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index a69bbfa..155fb05 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -391,7 +391,7 @@ put_error: error: /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ truncate_inode_pages(&inode->i_data, 0); - truncate_blocks(inode, 0); + truncate_blocks(inode, 0, false); remove_dirty_dir_inode(inode); remove_inode_page(inode); return ERR_PTR(err); diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 2723b2d..7f976c1 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) */ int f2fs_sync_file(struct file *, loff_t, loff_t, int); void truncate_data_blocks(struct dnode_of_data *); -int truncate_blocks(struct inode *, u64); +int truncate_blocks(struct inode *, u64, bool); void f2fs_truncate(struct inode *); int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); int f2fs_setattr(struct dentry *, struct iattr *); diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index ecbdf6a..a8e97f8 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -422,7 +422,7 @@ out: f2fs_put_page(page, 1); } -int truncate_blocks(struct inode *inode, u64 from) +int truncate_blocks(struct inode *inode, u64 from, bool lock) { struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); unsigned int blocksize = inode->i_sb->s_blocksize; @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) free_from = (pgoff_t) ((from + blocksize - 1) >> (sbi->log_blocksize)); - f2fs_lock_op(sbi); + if (lock) + f2fs_lock_op(sbi); set_new_dnode(&dn, inode, NULL, NULL, 0); err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); if (err) { if (err == -ENOENT) goto free_next; - f2fs_unlock_op(sbi); + if (lock) + f2fs_unlock_op(sbi); trace_f2fs_truncate_blocks_exit(inode, err); return err; } @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) f2fs_put_dnode(&dn); free_next: err = truncate_inode_blocks(inode, free_from); - f2fs_unlock_op(sbi); + if (lock) + f2fs_unlock_op(sbi); done: /* lastly zero out the first data page */ truncate_partial_data_page(inode, from); @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) trace_f2fs_truncate(inode); - if (!truncate_blocks(inode, i_size_read(inode))) { + if (!truncate_blocks(inode, i_size_read(inode), true)) { inode->i_mtime = inode->i_ctime = CURRENT_TIME; mark_inode_dirty(inode); } diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index 520758b..4d1f39f 100644 --- a/fs/f2fs/inline.c +++ b/fs/f2fs/inline.c @@ -247,7 +247,7 @@ process_inline: update_inode(inode, ipage); f2fs_put_page(ipage, 1); } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { - truncate_blocks(inode, 0); + truncate_blocks(inode, 0, false); set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); goto process_inline; } -- 1.8.5.2 (Apple Git-48) ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks 2014-08-15 22:03 ` Jaegeuk Kim @ 2014-08-19 8:04 ` Chao Yu -1 siblings, 0 replies; 20+ messages in thread From: Chao Yu @ 2014-08-19 8:04 UTC (permalink / raw) To: 'Jaegeuk Kim'; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel Hi Jaegeuk, > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Saturday, August 16, 2014 6:04 AM > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > linux-f2fs-devel@lists.sourceforge.net > Cc: Jaegeuk Kim > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > The init_inode_metadata calls truncate_blocks when error is occurred. > The callers holds f2fs_lock_op, so we should not call it again in > truncate_blocks. Nice catch! Your solution is a good way to fix this issue. Previously, in create inode path, I found there are some redundant codes between init_inode_metadata and evict_inode, including: truncate_inode_pages(&inode->i_data, 0); truncate_blocks(inode, 0); remove_dirty_dir_inode(inode); remove_inode_page(inode); So I think there is another way to fix this issue by removing error path handling codes in init_inode_metadata, not making the inode bad to left garbage clean work in evict_inode. In this way we can avoid adding additional argument for all different callers. How do you think? Thanks, Yu > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/data.c | 2 +- > fs/f2fs/dir.c | 2 +- > fs/f2fs/f2fs.h | 2 +- > fs/f2fs/file.c | 13 ++++++++----- > fs/f2fs/inline.c | 2 +- > 5 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 68834e2..14cc3e8 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to) > > if (to > inode->i_size) { > truncate_pagecache(inode, inode->i_size); > - truncate_blocks(inode, inode->i_size); > + truncate_blocks(inode, inode->i_size, true); > } > } > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index a69bbfa..155fb05 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -391,7 +391,7 @@ put_error: > error: > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > truncate_inode_pages(&inode->i_data, 0); > - truncate_blocks(inode, 0); > + truncate_blocks(inode, 0, false); > remove_dirty_dir_inode(inode); > remove_inode_page(inode); > return ERR_PTR(err); > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 2723b2d..7f976c1 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > */ > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > void truncate_data_blocks(struct dnode_of_data *); > -int truncate_blocks(struct inode *, u64); > +int truncate_blocks(struct inode *, u64, bool); > void f2fs_truncate(struct inode *); > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > int f2fs_setattr(struct dentry *, struct iattr *); > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index ecbdf6a..a8e97f8 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -422,7 +422,7 @@ out: > f2fs_put_page(page, 1); > } > > -int truncate_blocks(struct inode *inode, u64 from) > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > { > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > unsigned int blocksize = inode->i_sb->s_blocksize; > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > free_from = (pgoff_t) > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > - f2fs_lock_op(sbi); > + if (lock) > + f2fs_lock_op(sbi); > > set_new_dnode(&dn, inode, NULL, NULL, 0); > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > if (err) { > if (err == -ENOENT) > goto free_next; > - f2fs_unlock_op(sbi); > + if (lock) > + f2fs_unlock_op(sbi); > trace_f2fs_truncate_blocks_exit(inode, err); > return err; > } > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > f2fs_put_dnode(&dn); > free_next: > err = truncate_inode_blocks(inode, free_from); > - f2fs_unlock_op(sbi); > + if (lock) > + f2fs_unlock_op(sbi); > done: > /* lastly zero out the first data page */ > truncate_partial_data_page(inode, from); > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > trace_f2fs_truncate(inode); > > - if (!truncate_blocks(inode, i_size_read(inode))) { > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > mark_inode_dirty(inode); > } > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > index 520758b..4d1f39f 100644 > --- a/fs/f2fs/inline.c > +++ b/fs/f2fs/inline.c > @@ -247,7 +247,7 @@ process_inline: > update_inode(inode, ipage); > f2fs_put_page(ipage, 1); > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > - truncate_blocks(inode, 0); > + truncate_blocks(inode, 0, false); > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > goto process_inline; > } > -- > 1.8.5.2 (Apple Git-48) > > > ------------------------------------------------------------------------------ > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks @ 2014-08-19 8:04 ` Chao Yu 0 siblings, 0 replies; 20+ messages in thread From: Chao Yu @ 2014-08-19 8:04 UTC (permalink / raw) To: 'Jaegeuk Kim'; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel Hi Jaegeuk, > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Saturday, August 16, 2014 6:04 AM > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > linux-f2fs-devel@lists.sourceforge.net > Cc: Jaegeuk Kim > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > The init_inode_metadata calls truncate_blocks when error is occurred. > The callers holds f2fs_lock_op, so we should not call it again in > truncate_blocks. Nice catch! Your solution is a good way to fix this issue. Previously, in create inode path, I found there are some redundant codes between init_inode_metadata and evict_inode, including: truncate_inode_pages(&inode->i_data, 0); truncate_blocks(inode, 0); remove_dirty_dir_inode(inode); remove_inode_page(inode); So I think there is another way to fix this issue by removing error path handling codes in init_inode_metadata, not making the inode bad to left garbage clean work in evict_inode. In this way we can avoid adding additional argument for all different callers. How do you think? Thanks, Yu > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/data.c | 2 +- > fs/f2fs/dir.c | 2 +- > fs/f2fs/f2fs.h | 2 +- > fs/f2fs/file.c | 13 ++++++++----- > fs/f2fs/inline.c | 2 +- > 5 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 68834e2..14cc3e8 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to) > > if (to > inode->i_size) { > truncate_pagecache(inode, inode->i_size); > - truncate_blocks(inode, inode->i_size); > + truncate_blocks(inode, inode->i_size, true); > } > } > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index a69bbfa..155fb05 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -391,7 +391,7 @@ put_error: > error: > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > truncate_inode_pages(&inode->i_data, 0); > - truncate_blocks(inode, 0); > + truncate_blocks(inode, 0, false); > remove_dirty_dir_inode(inode); > remove_inode_page(inode); > return ERR_PTR(err); > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 2723b2d..7f976c1 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > */ > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > void truncate_data_blocks(struct dnode_of_data *); > -int truncate_blocks(struct inode *, u64); > +int truncate_blocks(struct inode *, u64, bool); > void f2fs_truncate(struct inode *); > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > int f2fs_setattr(struct dentry *, struct iattr *); > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index ecbdf6a..a8e97f8 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -422,7 +422,7 @@ out: > f2fs_put_page(page, 1); > } > > -int truncate_blocks(struct inode *inode, u64 from) > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > { > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > unsigned int blocksize = inode->i_sb->s_blocksize; > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > free_from = (pgoff_t) > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > - f2fs_lock_op(sbi); > + if (lock) > + f2fs_lock_op(sbi); > > set_new_dnode(&dn, inode, NULL, NULL, 0); > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > if (err) { > if (err == -ENOENT) > goto free_next; > - f2fs_unlock_op(sbi); > + if (lock) > + f2fs_unlock_op(sbi); > trace_f2fs_truncate_blocks_exit(inode, err); > return err; > } > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > f2fs_put_dnode(&dn); > free_next: > err = truncate_inode_blocks(inode, free_from); > - f2fs_unlock_op(sbi); > + if (lock) > + f2fs_unlock_op(sbi); > done: > /* lastly zero out the first data page */ > truncate_partial_data_page(inode, from); > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > trace_f2fs_truncate(inode); > > - if (!truncate_blocks(inode, i_size_read(inode))) { > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > mark_inode_dirty(inode); > } > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > index 520758b..4d1f39f 100644 > --- a/fs/f2fs/inline.c > +++ b/fs/f2fs/inline.c > @@ -247,7 +247,7 @@ process_inline: > update_inode(inode, ipage); > f2fs_put_page(ipage, 1); > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > - truncate_blocks(inode, 0); > + truncate_blocks(inode, 0, false); > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > goto process_inline; > } > -- > 1.8.5.2 (Apple Git-48) > > > ------------------------------------------------------------------------------ > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks 2014-08-19 8:04 ` [f2fs-dev] " Chao Yu @ 2014-08-19 16:58 ` Jaegeuk Kim -1 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-19 16:58 UTC (permalink / raw) To: Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > Hi Jaegeuk, > > > -----Original Message----- > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > Sent: Saturday, August 16, 2014 6:04 AM > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > linux-f2fs-devel@lists.sourceforge.net > > Cc: Jaegeuk Kim > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > The callers holds f2fs_lock_op, so we should not call it again in > > truncate_blocks. > > Nice catch! Your solution is a good way to fix this issue. > > Previously, in create inode path, I found there are some redundant codes between > init_inode_metadata and evict_inode, including: > truncate_inode_pages(&inode->i_data, 0); > truncate_blocks(inode, 0); > remove_dirty_dir_inode(inode); > remove_inode_page(inode); > > So I think there is another way to fix this issue by removing error path handling > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > evict_inode. In this way we can avoid adding additional argument for all different > callers. Well, possible. But we need to take a closer look at the race condition on the inode cache. What can happen if this bad inode is reassigned to the other thread? > > How do you think? > > Thanks, > Yu > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/data.c | 2 +- > > fs/f2fs/dir.c | 2 +- > > fs/f2fs/f2fs.h | 2 +- > > fs/f2fs/file.c | 13 ++++++++----- > > fs/f2fs/inline.c | 2 +- > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index 68834e2..14cc3e8 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to) > > > > if (to > inode->i_size) { > > truncate_pagecache(inode, inode->i_size); > > - truncate_blocks(inode, inode->i_size); > > + truncate_blocks(inode, inode->i_size, true); > > } > > } > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > index a69bbfa..155fb05 100644 > > --- a/fs/f2fs/dir.c > > +++ b/fs/f2fs/dir.c > > @@ -391,7 +391,7 @@ put_error: > > error: > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > truncate_inode_pages(&inode->i_data, 0); > > - truncate_blocks(inode, 0); > > + truncate_blocks(inode, 0, false); > > remove_dirty_dir_inode(inode); > > remove_inode_page(inode); > > return ERR_PTR(err); > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 2723b2d..7f976c1 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > */ > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > void truncate_data_blocks(struct dnode_of_data *); > > -int truncate_blocks(struct inode *, u64); > > +int truncate_blocks(struct inode *, u64, bool); > > void f2fs_truncate(struct inode *); > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > int f2fs_setattr(struct dentry *, struct iattr *); > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > index ecbdf6a..a8e97f8 100644 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@ -422,7 +422,7 @@ out: > > f2fs_put_page(page, 1); > > } > > > > -int truncate_blocks(struct inode *inode, u64 from) > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > { > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > unsigned int blocksize = inode->i_sb->s_blocksize; > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > free_from = (pgoff_t) > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > - f2fs_lock_op(sbi); > > + if (lock) > > + f2fs_lock_op(sbi); > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > if (err) { > > if (err == -ENOENT) > > goto free_next; > > - f2fs_unlock_op(sbi); > > + if (lock) > > + f2fs_unlock_op(sbi); > > trace_f2fs_truncate_blocks_exit(inode, err); > > return err; > > } > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > f2fs_put_dnode(&dn); > > free_next: > > err = truncate_inode_blocks(inode, free_from); > > - f2fs_unlock_op(sbi); > > + if (lock) > > + f2fs_unlock_op(sbi); > > done: > > /* lastly zero out the first data page */ > > truncate_partial_data_page(inode, from); > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > trace_f2fs_truncate(inode); > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > mark_inode_dirty(inode); > > } > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > index 520758b..4d1f39f 100644 > > --- a/fs/f2fs/inline.c > > +++ b/fs/f2fs/inline.c > > @@ -247,7 +247,7 @@ process_inline: > > update_inode(inode, ipage); > > f2fs_put_page(ipage, 1); > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > - truncate_blocks(inode, 0); > > + truncate_blocks(inode, 0, false); > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > goto process_inline; > > } > > -- > > 1.8.5.2 (Apple Git-48) > > > > > > ------------------------------------------------------------------------------ > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > ------------------------------------------------------------------------------ > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks @ 2014-08-19 16:58 ` Jaegeuk Kim 0 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-19 16:58 UTC (permalink / raw) To: Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > Hi Jaegeuk, > > > -----Original Message----- > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > Sent: Saturday, August 16, 2014 6:04 AM > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > linux-f2fs-devel@lists.sourceforge.net > > Cc: Jaegeuk Kim > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > The callers holds f2fs_lock_op, so we should not call it again in > > truncate_blocks. > > Nice catch! Your solution is a good way to fix this issue. > > Previously, in create inode path, I found there are some redundant codes between > init_inode_metadata and evict_inode, including: > truncate_inode_pages(&inode->i_data, 0); > truncate_blocks(inode, 0); > remove_dirty_dir_inode(inode); > remove_inode_page(inode); > > So I think there is another way to fix this issue by removing error path handling > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > evict_inode. In this way we can avoid adding additional argument for all different > callers. Well, possible. But we need to take a closer look at the race condition on the inode cache. What can happen if this bad inode is reassigned to the other thread? > > How do you think? > > Thanks, > Yu > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/data.c | 2 +- > > fs/f2fs/dir.c | 2 +- > > fs/f2fs/f2fs.h | 2 +- > > fs/f2fs/file.c | 13 ++++++++----- > > fs/f2fs/inline.c | 2 +- > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index 68834e2..14cc3e8 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to) > > > > if (to > inode->i_size) { > > truncate_pagecache(inode, inode->i_size); > > - truncate_blocks(inode, inode->i_size); > > + truncate_blocks(inode, inode->i_size, true); > > } > > } > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > index a69bbfa..155fb05 100644 > > --- a/fs/f2fs/dir.c > > +++ b/fs/f2fs/dir.c > > @@ -391,7 +391,7 @@ put_error: > > error: > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > truncate_inode_pages(&inode->i_data, 0); > > - truncate_blocks(inode, 0); > > + truncate_blocks(inode, 0, false); > > remove_dirty_dir_inode(inode); > > remove_inode_page(inode); > > return ERR_PTR(err); > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 2723b2d..7f976c1 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > */ > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > void truncate_data_blocks(struct dnode_of_data *); > > -int truncate_blocks(struct inode *, u64); > > +int truncate_blocks(struct inode *, u64, bool); > > void f2fs_truncate(struct inode *); > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > int f2fs_setattr(struct dentry *, struct iattr *); > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > index ecbdf6a..a8e97f8 100644 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@ -422,7 +422,7 @@ out: > > f2fs_put_page(page, 1); > > } > > > > -int truncate_blocks(struct inode *inode, u64 from) > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > { > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > unsigned int blocksize = inode->i_sb->s_blocksize; > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > free_from = (pgoff_t) > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > - f2fs_lock_op(sbi); > > + if (lock) > > + f2fs_lock_op(sbi); > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > if (err) { > > if (err == -ENOENT) > > goto free_next; > > - f2fs_unlock_op(sbi); > > + if (lock) > > + f2fs_unlock_op(sbi); > > trace_f2fs_truncate_blocks_exit(inode, err); > > return err; > > } > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > f2fs_put_dnode(&dn); > > free_next: > > err = truncate_inode_blocks(inode, free_from); > > - f2fs_unlock_op(sbi); > > + if (lock) > > + f2fs_unlock_op(sbi); > > done: > > /* lastly zero out the first data page */ > > truncate_partial_data_page(inode, from); > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > trace_f2fs_truncate(inode); > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > mark_inode_dirty(inode); > > } > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > index 520758b..4d1f39f 100644 > > --- a/fs/f2fs/inline.c > > +++ b/fs/f2fs/inline.c > > @@ -247,7 +247,7 @@ process_inline: > > update_inode(inode, ipage); > > f2fs_put_page(ipage, 1); > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > - truncate_blocks(inode, 0); > > + truncate_blocks(inode, 0, false); > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > goto process_inline; > > } > > -- > > 1.8.5.2 (Apple Git-48) > > > > > > ------------------------------------------------------------------------------ > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > ------------------------------------------------------------------------------ > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks 2014-08-19 16:58 ` [f2fs-dev] " Jaegeuk Kim @ 2014-08-20 2:07 ` Chao Yu -1 siblings, 0 replies; 20+ messages in thread From: Chao Yu @ 2014-08-20 2:07 UTC (permalink / raw) To: 'Jaegeuk Kim'; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Wednesday, August 20, 2014 12:58 AM > To: Chao Yu > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > > Hi Jaegeuk, > > > > > -----Original Message----- > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > Sent: Saturday, August 16, 2014 6:04 AM > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > linux-f2fs-devel@lists.sourceforge.net > > > Cc: Jaegeuk Kim > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > > The callers holds f2fs_lock_op, so we should not call it again in > > > truncate_blocks. > > > > Nice catch! Your solution is a good way to fix this issue. > > > > Previously, in create inode path, I found there are some redundant codes between > > init_inode_metadata and evict_inode, including: > > truncate_inode_pages(&inode->i_data, 0); > > truncate_blocks(inode, 0); > > remove_dirty_dir_inode(inode); > > remove_inode_page(inode); > > > > So I think there is another way to fix this issue by removing error path handling > > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > > evict_inode. In this way we can avoid adding additional argument for all different > > callers. > > Well, possible. > But we need to take a closer look at the race condition on the inode cache. > What can happen if this bad inode is reassigned to the other thread? I don't get it. As I know, in evict(), we call ->evict_inode before remove_inode_hash(), so before all clean work was done we will not reassign this hashed uncleaned inode to other thread. Am I missing anything? Regards, Yu > > > > > How do you think? > > > > Thanks, > > Yu > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > --- > > > fs/f2fs/data.c | 2 +- > > > fs/f2fs/dir.c | 2 +- > > > fs/f2fs/f2fs.h | 2 +- > > > fs/f2fs/file.c | 13 ++++++++----- > > > fs/f2fs/inline.c | 2 +- > > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > index 68834e2..14cc3e8 100644 > > > --- a/fs/f2fs/data.c > > > +++ b/fs/f2fs/data.c > > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t > to) > > > > > > if (to > inode->i_size) { > > > truncate_pagecache(inode, inode->i_size); > > > - truncate_blocks(inode, inode->i_size); > > > + truncate_blocks(inode, inode->i_size, true); > > > } > > > } > > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > index a69bbfa..155fb05 100644 > > > --- a/fs/f2fs/dir.c > > > +++ b/fs/f2fs/dir.c > > > @@ -391,7 +391,7 @@ put_error: > > > error: > > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > > truncate_inode_pages(&inode->i_data, 0); > > > - truncate_blocks(inode, 0); > > > + truncate_blocks(inode, 0, false); > > > remove_dirty_dir_inode(inode); > > > remove_inode_page(inode); > > > return ERR_PTR(err); > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > index 2723b2d..7f976c1 100644 > > > --- a/fs/f2fs/f2fs.h > > > +++ b/fs/f2fs/f2fs.h > > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > > */ > > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > > void truncate_data_blocks(struct dnode_of_data *); > > > -int truncate_blocks(struct inode *, u64); > > > +int truncate_blocks(struct inode *, u64, bool); > > > void f2fs_truncate(struct inode *); > > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > > int f2fs_setattr(struct dentry *, struct iattr *); > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > index ecbdf6a..a8e97f8 100644 > > > --- a/fs/f2fs/file.c > > > +++ b/fs/f2fs/file.c > > > @@ -422,7 +422,7 @@ out: > > > f2fs_put_page(page, 1); > > > } > > > > > > -int truncate_blocks(struct inode *inode, u64 from) > > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > > { > > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > > unsigned int blocksize = inode->i_sb->s_blocksize; > > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > > free_from = (pgoff_t) > > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > > > - f2fs_lock_op(sbi); > > > + if (lock) > > > + f2fs_lock_op(sbi); > > > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > > if (err) { > > > if (err == -ENOENT) > > > goto free_next; > > > - f2fs_unlock_op(sbi); > > > + if (lock) > > > + f2fs_unlock_op(sbi); > > > trace_f2fs_truncate_blocks_exit(inode, err); > > > return err; > > > } > > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > > f2fs_put_dnode(&dn); > > > free_next: > > > err = truncate_inode_blocks(inode, free_from); > > > - f2fs_unlock_op(sbi); > > > + if (lock) > > > + f2fs_unlock_op(sbi); > > > done: > > > /* lastly zero out the first data page */ > > > truncate_partial_data_page(inode, from); > > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > > > trace_f2fs_truncate(inode); > > > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > > mark_inode_dirty(inode); > > > } > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > index 520758b..4d1f39f 100644 > > > --- a/fs/f2fs/inline.c > > > +++ b/fs/f2fs/inline.c > > > @@ -247,7 +247,7 @@ process_inline: > > > update_inode(inode, ipage); > > > f2fs_put_page(ipage, 1); > > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > > - truncate_blocks(inode, 0); > > > + truncate_blocks(inode, 0, false); > > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > > goto process_inline; > > > } > > > -- > > > 1.8.5.2 (Apple Git-48) > > > > > > > > > ------------------------------------------------------------------------------ > > > _______________________________________________ > > > Linux-f2fs-devel mailing list > > > Linux-f2fs-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > ------------------------------------------------------------------------------ > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks @ 2014-08-20 2:07 ` Chao Yu 0 siblings, 0 replies; 20+ messages in thread From: Chao Yu @ 2014-08-20 2:07 UTC (permalink / raw) To: 'Jaegeuk Kim'; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Wednesday, August 20, 2014 12:58 AM > To: Chao Yu > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > > Hi Jaegeuk, > > > > > -----Original Message----- > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > Sent: Saturday, August 16, 2014 6:04 AM > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > linux-f2fs-devel@lists.sourceforge.net > > > Cc: Jaegeuk Kim > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > > The callers holds f2fs_lock_op, so we should not call it again in > > > truncate_blocks. > > > > Nice catch! Your solution is a good way to fix this issue. > > > > Previously, in create inode path, I found there are some redundant codes between > > init_inode_metadata and evict_inode, including: > > truncate_inode_pages(&inode->i_data, 0); > > truncate_blocks(inode, 0); > > remove_dirty_dir_inode(inode); > > remove_inode_page(inode); > > > > So I think there is another way to fix this issue by removing error path handling > > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > > evict_inode. In this way we can avoid adding additional argument for all different > > callers. > > Well, possible. > But we need to take a closer look at the race condition on the inode cache. > What can happen if this bad inode is reassigned to the other thread? I don't get it. As I know, in evict(), we call ->evict_inode before remove_inode_hash(), so before all clean work was done we will not reassign this hashed uncleaned inode to other thread. Am I missing anything? Regards, Yu > > > > > How do you think? > > > > Thanks, > > Yu > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > --- > > > fs/f2fs/data.c | 2 +- > > > fs/f2fs/dir.c | 2 +- > > > fs/f2fs/f2fs.h | 2 +- > > > fs/f2fs/file.c | 13 ++++++++----- > > > fs/f2fs/inline.c | 2 +- > > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > index 68834e2..14cc3e8 100644 > > > --- a/fs/f2fs/data.c > > > +++ b/fs/f2fs/data.c > > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t > to) > > > > > > if (to > inode->i_size) { > > > truncate_pagecache(inode, inode->i_size); > > > - truncate_blocks(inode, inode->i_size); > > > + truncate_blocks(inode, inode->i_size, true); > > > } > > > } > > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > index a69bbfa..155fb05 100644 > > > --- a/fs/f2fs/dir.c > > > +++ b/fs/f2fs/dir.c > > > @@ -391,7 +391,7 @@ put_error: > > > error: > > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > > truncate_inode_pages(&inode->i_data, 0); > > > - truncate_blocks(inode, 0); > > > + truncate_blocks(inode, 0, false); > > > remove_dirty_dir_inode(inode); > > > remove_inode_page(inode); > > > return ERR_PTR(err); > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > index 2723b2d..7f976c1 100644 > > > --- a/fs/f2fs/f2fs.h > > > +++ b/fs/f2fs/f2fs.h > > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > > */ > > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > > void truncate_data_blocks(struct dnode_of_data *); > > > -int truncate_blocks(struct inode *, u64); > > > +int truncate_blocks(struct inode *, u64, bool); > > > void f2fs_truncate(struct inode *); > > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > > int f2fs_setattr(struct dentry *, struct iattr *); > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > index ecbdf6a..a8e97f8 100644 > > > --- a/fs/f2fs/file.c > > > +++ b/fs/f2fs/file.c > > > @@ -422,7 +422,7 @@ out: > > > f2fs_put_page(page, 1); > > > } > > > > > > -int truncate_blocks(struct inode *inode, u64 from) > > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > > { > > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > > unsigned int blocksize = inode->i_sb->s_blocksize; > > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > > free_from = (pgoff_t) > > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > > > - f2fs_lock_op(sbi); > > > + if (lock) > > > + f2fs_lock_op(sbi); > > > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > > if (err) { > > > if (err == -ENOENT) > > > goto free_next; > > > - f2fs_unlock_op(sbi); > > > + if (lock) > > > + f2fs_unlock_op(sbi); > > > trace_f2fs_truncate_blocks_exit(inode, err); > > > return err; > > > } > > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > > f2fs_put_dnode(&dn); > > > free_next: > > > err = truncate_inode_blocks(inode, free_from); > > > - f2fs_unlock_op(sbi); > > > + if (lock) > > > + f2fs_unlock_op(sbi); > > > done: > > > /* lastly zero out the first data page */ > > > truncate_partial_data_page(inode, from); > > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > > > trace_f2fs_truncate(inode); > > > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > > mark_inode_dirty(inode); > > > } > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > index 520758b..4d1f39f 100644 > > > --- a/fs/f2fs/inline.c > > > +++ b/fs/f2fs/inline.c > > > @@ -247,7 +247,7 @@ process_inline: > > > update_inode(inode, ipage); > > > f2fs_put_page(ipage, 1); > > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > > - truncate_blocks(inode, 0); > > > + truncate_blocks(inode, 0, false); > > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > > goto process_inline; > > > } > > > -- > > > 1.8.5.2 (Apple Git-48) > > > > > > > > > ------------------------------------------------------------------------------ > > > _______________________________________________ > > > Linux-f2fs-devel mailing list > > > Linux-f2fs-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > ------------------------------------------------------------------------------ > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks 2014-08-20 2:07 ` [f2fs-dev] " Chao Yu @ 2014-08-21 16:45 ` Jaegeuk Kim -1 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-21 16:45 UTC (permalink / raw) To: Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel On Wed, Aug 20, 2014 at 10:07:04AM +0800, Chao Yu wrote: > > -----Original Message----- > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > Sent: Wednesday, August 20, 2014 12:58 AM > > To: Chao Yu > > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-f2fs-devel@lists.sourceforge.net > > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > > > Hi Jaegeuk, > > > > > > > -----Original Message----- > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > > Sent: Saturday, August 16, 2014 6:04 AM > > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > > linux-f2fs-devel@lists.sourceforge.net > > > > Cc: Jaegeuk Kim > > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > > > The callers holds f2fs_lock_op, so we should not call it again in > > > > truncate_blocks. > > > > > > Nice catch! Your solution is a good way to fix this issue. > > > > > > Previously, in create inode path, I found there are some redundant codes between > > > init_inode_metadata and evict_inode, including: > > > truncate_inode_pages(&inode->i_data, 0); > > > truncate_blocks(inode, 0); > > > remove_dirty_dir_inode(inode); > > > remove_inode_page(inode); > > > > > > So I think there is another way to fix this issue by removing error path handling > > > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > > > evict_inode. In this way we can avoid adding additional argument for all different > > > callers. > > > > Well, possible. > > But we need to take a closer look at the race condition on the inode cache. > > What can happen if this bad inode is reassigned to the other thread? > > I don't get it. As I know, in evict(), we call ->evict_inode before > remove_inode_hash(), so before all clean work was done we will not reassign > this hashed uncleaned inode to other thread. > > Am I missing anything? What I meant was it may happen between init_inode_metadata and iput. Thanks, > > Regards, > Yu > > > > > > > > > How do you think? > > > > > > Thanks, > > > Yu > > > > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > --- > > > > fs/f2fs/data.c | 2 +- > > > > fs/f2fs/dir.c | 2 +- > > > > fs/f2fs/f2fs.h | 2 +- > > > > fs/f2fs/file.c | 13 ++++++++----- > > > > fs/f2fs/inline.c | 2 +- > > > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > > index 68834e2..14cc3e8 100644 > > > > --- a/fs/f2fs/data.c > > > > +++ b/fs/f2fs/data.c > > > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t > > to) > > > > > > > > if (to > inode->i_size) { > > > > truncate_pagecache(inode, inode->i_size); > > > > - truncate_blocks(inode, inode->i_size); > > > > + truncate_blocks(inode, inode->i_size, true); > > > > } > > > > } > > > > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > > index a69bbfa..155fb05 100644 > > > > --- a/fs/f2fs/dir.c > > > > +++ b/fs/f2fs/dir.c > > > > @@ -391,7 +391,7 @@ put_error: > > > > error: > > > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > > > truncate_inode_pages(&inode->i_data, 0); > > > > - truncate_blocks(inode, 0); > > > > + truncate_blocks(inode, 0, false); > > > > remove_dirty_dir_inode(inode); > > > > remove_inode_page(inode); > > > > return ERR_PTR(err); > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > > index 2723b2d..7f976c1 100644 > > > > --- a/fs/f2fs/f2fs.h > > > > +++ b/fs/f2fs/f2fs.h > > > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > > > */ > > > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > > > void truncate_data_blocks(struct dnode_of_data *); > > > > -int truncate_blocks(struct inode *, u64); > > > > +int truncate_blocks(struct inode *, u64, bool); > > > > void f2fs_truncate(struct inode *); > > > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > > > int f2fs_setattr(struct dentry *, struct iattr *); > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > > index ecbdf6a..a8e97f8 100644 > > > > --- a/fs/f2fs/file.c > > > > +++ b/fs/f2fs/file.c > > > > @@ -422,7 +422,7 @@ out: > > > > f2fs_put_page(page, 1); > > > > } > > > > > > > > -int truncate_blocks(struct inode *inode, u64 from) > > > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > > > { > > > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > > > unsigned int blocksize = inode->i_sb->s_blocksize; > > > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > free_from = (pgoff_t) > > > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > > > > > - f2fs_lock_op(sbi); > > > > + if (lock) > > > > + f2fs_lock_op(sbi); > > > > > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > > > if (err) { > > > > if (err == -ENOENT) > > > > goto free_next; > > > > - f2fs_unlock_op(sbi); > > > > + if (lock) > > > > + f2fs_unlock_op(sbi); > > > > trace_f2fs_truncate_blocks_exit(inode, err); > > > > return err; > > > > } > > > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > f2fs_put_dnode(&dn); > > > > free_next: > > > > err = truncate_inode_blocks(inode, free_from); > > > > - f2fs_unlock_op(sbi); > > > > + if (lock) > > > > + f2fs_unlock_op(sbi); > > > > done: > > > > /* lastly zero out the first data page */ > > > > truncate_partial_data_page(inode, from); > > > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > > > > > trace_f2fs_truncate(inode); > > > > > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > > > mark_inode_dirty(inode); > > > > } > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > > index 520758b..4d1f39f 100644 > > > > --- a/fs/f2fs/inline.c > > > > +++ b/fs/f2fs/inline.c > > > > @@ -247,7 +247,7 @@ process_inline: > > > > update_inode(inode, ipage); > > > > f2fs_put_page(ipage, 1); > > > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > > > - truncate_blocks(inode, 0); > > > > + truncate_blocks(inode, 0, false); > > > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > > > goto process_inline; > > > > } > > > > -- > > > > 1.8.5.2 (Apple Git-48) > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > > > > Linux-f2fs-devel mailing list > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > > > > ------------------------------------------------------------------------------ > > > _______________________________________________ > > > Linux-f2fs-devel mailing list > > > Linux-f2fs-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks @ 2014-08-21 16:45 ` Jaegeuk Kim 0 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-21 16:45 UTC (permalink / raw) To: Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel On Wed, Aug 20, 2014 at 10:07:04AM +0800, Chao Yu wrote: > > -----Original Message----- > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > Sent: Wednesday, August 20, 2014 12:58 AM > > To: Chao Yu > > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-f2fs-devel@lists.sourceforge.net > > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > > > Hi Jaegeuk, > > > > > > > -----Original Message----- > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > > Sent: Saturday, August 16, 2014 6:04 AM > > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > > linux-f2fs-devel@lists.sourceforge.net > > > > Cc: Jaegeuk Kim > > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > > > The callers holds f2fs_lock_op, so we should not call it again in > > > > truncate_blocks. > > > > > > Nice catch! Your solution is a good way to fix this issue. > > > > > > Previously, in create inode path, I found there are some redundant codes between > > > init_inode_metadata and evict_inode, including: > > > truncate_inode_pages(&inode->i_data, 0); > > > truncate_blocks(inode, 0); > > > remove_dirty_dir_inode(inode); > > > remove_inode_page(inode); > > > > > > So I think there is another way to fix this issue by removing error path handling > > > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > > > evict_inode. In this way we can avoid adding additional argument for all different > > > callers. > > > > Well, possible. > > But we need to take a closer look at the race condition on the inode cache. > > What can happen if this bad inode is reassigned to the other thread? > > I don't get it. As I know, in evict(), we call ->evict_inode before > remove_inode_hash(), so before all clean work was done we will not reassign > this hashed uncleaned inode to other thread. > > Am I missing anything? What I meant was it may happen between init_inode_metadata and iput. Thanks, > > Regards, > Yu > > > > > > > > > How do you think? > > > > > > Thanks, > > > Yu > > > > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > --- > > > > fs/f2fs/data.c | 2 +- > > > > fs/f2fs/dir.c | 2 +- > > > > fs/f2fs/f2fs.h | 2 +- > > > > fs/f2fs/file.c | 13 ++++++++----- > > > > fs/f2fs/inline.c | 2 +- > > > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > > index 68834e2..14cc3e8 100644 > > > > --- a/fs/f2fs/data.c > > > > +++ b/fs/f2fs/data.c > > > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t > > to) > > > > > > > > if (to > inode->i_size) { > > > > truncate_pagecache(inode, inode->i_size); > > > > - truncate_blocks(inode, inode->i_size); > > > > + truncate_blocks(inode, inode->i_size, true); > > > > } > > > > } > > > > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > > index a69bbfa..155fb05 100644 > > > > --- a/fs/f2fs/dir.c > > > > +++ b/fs/f2fs/dir.c > > > > @@ -391,7 +391,7 @@ put_error: > > > > error: > > > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > > > truncate_inode_pages(&inode->i_data, 0); > > > > - truncate_blocks(inode, 0); > > > > + truncate_blocks(inode, 0, false); > > > > remove_dirty_dir_inode(inode); > > > > remove_inode_page(inode); > > > > return ERR_PTR(err); > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > > index 2723b2d..7f976c1 100644 > > > > --- a/fs/f2fs/f2fs.h > > > > +++ b/fs/f2fs/f2fs.h > > > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > > > */ > > > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > > > void truncate_data_blocks(struct dnode_of_data *); > > > > -int truncate_blocks(struct inode *, u64); > > > > +int truncate_blocks(struct inode *, u64, bool); > > > > void f2fs_truncate(struct inode *); > > > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > > > int f2fs_setattr(struct dentry *, struct iattr *); > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > > index ecbdf6a..a8e97f8 100644 > > > > --- a/fs/f2fs/file.c > > > > +++ b/fs/f2fs/file.c > > > > @@ -422,7 +422,7 @@ out: > > > > f2fs_put_page(page, 1); > > > > } > > > > > > > > -int truncate_blocks(struct inode *inode, u64 from) > > > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > > > { > > > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > > > unsigned int blocksize = inode->i_sb->s_blocksize; > > > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > free_from = (pgoff_t) > > > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > > > > > - f2fs_lock_op(sbi); > > > > + if (lock) > > > > + f2fs_lock_op(sbi); > > > > > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > > > if (err) { > > > > if (err == -ENOENT) > > > > goto free_next; > > > > - f2fs_unlock_op(sbi); > > > > + if (lock) > > > > + f2fs_unlock_op(sbi); > > > > trace_f2fs_truncate_blocks_exit(inode, err); > > > > return err; > > > > } > > > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > f2fs_put_dnode(&dn); > > > > free_next: > > > > err = truncate_inode_blocks(inode, free_from); > > > > - f2fs_unlock_op(sbi); > > > > + if (lock) > > > > + f2fs_unlock_op(sbi); > > > > done: > > > > /* lastly zero out the first data page */ > > > > truncate_partial_data_page(inode, from); > > > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > > > > > trace_f2fs_truncate(inode); > > > > > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > > > mark_inode_dirty(inode); > > > > } > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > > index 520758b..4d1f39f 100644 > > > > --- a/fs/f2fs/inline.c > > > > +++ b/fs/f2fs/inline.c > > > > @@ -247,7 +247,7 @@ process_inline: > > > > update_inode(inode, ipage); > > > > f2fs_put_page(ipage, 1); > > > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > > > - truncate_blocks(inode, 0); > > > > + truncate_blocks(inode, 0, false); > > > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > > > goto process_inline; > > > > } > > > > -- > > > > 1.8.5.2 (Apple Git-48) > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > > > > Linux-f2fs-devel mailing list > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > > > > ------------------------------------------------------------------------------ > > > _______________________________________________ > > > Linux-f2fs-devel mailing list > > > Linux-f2fs-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks 2014-08-21 16:45 ` [f2fs-dev] " Jaegeuk Kim @ 2014-08-22 6:56 ` Chao Yu -1 siblings, 0 replies; 20+ messages in thread From: Chao Yu @ 2014-08-22 6:56 UTC (permalink / raw) To: 'Jaegeuk Kim'; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel Hi Jaegeuk, > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Friday, August 22, 2014 12:45 AM > To: Chao Yu > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > On Wed, Aug 20, 2014 at 10:07:04AM +0800, Chao Yu wrote: > > > -----Original Message----- > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > Sent: Wednesday, August 20, 2014 12:58 AM > > > To: Chao Yu > > > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > > > linux-f2fs-devel@lists.sourceforge.net > > > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > > > > Hi Jaegeuk, > > > > > > > > > -----Original Message----- > > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > > > Sent: Saturday, August 16, 2014 6:04 AM > > > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > > > linux-f2fs-devel@lists.sourceforge.net > > > > > Cc: Jaegeuk Kim > > > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > > > > The callers holds f2fs_lock_op, so we should not call it again in > > > > > truncate_blocks. > > > > > > > > Nice catch! Your solution is a good way to fix this issue. > > > > > > > > Previously, in create inode path, I found there are some redundant codes between > > > > init_inode_metadata and evict_inode, including: > > > > truncate_inode_pages(&inode->i_data, 0); > > > > truncate_blocks(inode, 0); > > > > remove_dirty_dir_inode(inode); > > > > remove_inode_page(inode); > > > > > > > > So I think there is another way to fix this issue by removing error path handling > > > > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > > > > evict_inode. In this way we can avoid adding additional argument for all different > > > > callers. > > > > > > Well, possible. > > > But we need to take a closer look at the race condition on the inode cache. > > > What can happen if this bad inode is reassigned to the other thread? > > > > I don't get it. As I know, in evict(), we call ->evict_inode before > > remove_inode_hash(), so before all clean work was done we will not reassign > > this hashed uncleaned inode to other thread. > > > > Am I missing anything? > > What I meant was it may happen between init_inode_metadata and iput. Actually, can happen between unlock_new_inode and iput in error path of create/symlink/mkdir/mknod/tmpfile. Scenario is like this: ->f2fs_mkdir ->f2fs_add_link ->__f2fs_add_link ->init_inode_metadata failed here ->gc_thread_func ->f2fs_gc ->do_garbage_collect ->gc_data_segment ->f2fs_iget ->iget_locked ->wait_on_inode ->unlock_new_inode ->move_data_page ->make_bad_inode ->iput No problem now, but I'd like to remove unlock_new_inode from error path of inode creating procedure as we'd better wakeup waiter in end of ->iput instead of wakeup waiter in unlock_new_inode before invoking make_bad_inod. How do you think? Anyway, drop this proposal if you do not like it, although I think it will be ok theoretically after we fix above issue. Thanks, Yu > > Thanks, > > > > > Regards, > > Yu > > > > > > > > > > > > > How do you think? > > > > > > > > Thanks, > > > > Yu > > > > > > > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > --- > > > > > fs/f2fs/data.c | 2 +- > > > > > fs/f2fs/dir.c | 2 +- > > > > > fs/f2fs/f2fs.h | 2 +- > > > > > fs/f2fs/file.c | 13 ++++++++----- > > > > > fs/f2fs/inline.c | 2 +- > > > > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > > > index 68834e2..14cc3e8 100644 > > > > > --- a/fs/f2fs/data.c > > > > > +++ b/fs/f2fs/data.c > > > > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t > > > to) > > > > > > > > > > if (to > inode->i_size) { > > > > > truncate_pagecache(inode, inode->i_size); > > > > > - truncate_blocks(inode, inode->i_size); > > > > > + truncate_blocks(inode, inode->i_size, true); > > > > > } > > > > > } > > > > > > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > > > index a69bbfa..155fb05 100644 > > > > > --- a/fs/f2fs/dir.c > > > > > +++ b/fs/f2fs/dir.c > > > > > @@ -391,7 +391,7 @@ put_error: > > > > > error: > > > > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > > > > truncate_inode_pages(&inode->i_data, 0); > > > > > - truncate_blocks(inode, 0); > > > > > + truncate_blocks(inode, 0, false); > > > > > remove_dirty_dir_inode(inode); > > > > > remove_inode_page(inode); > > > > > return ERR_PTR(err); > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > > > index 2723b2d..7f976c1 100644 > > > > > --- a/fs/f2fs/f2fs.h > > > > > +++ b/fs/f2fs/f2fs.h > > > > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > > > > */ > > > > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > > > > void truncate_data_blocks(struct dnode_of_data *); > > > > > -int truncate_blocks(struct inode *, u64); > > > > > +int truncate_blocks(struct inode *, u64, bool); > > > > > void f2fs_truncate(struct inode *); > > > > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > > > > int f2fs_setattr(struct dentry *, struct iattr *); > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > > > index ecbdf6a..a8e97f8 100644 > > > > > --- a/fs/f2fs/file.c > > > > > +++ b/fs/f2fs/file.c > > > > > @@ -422,7 +422,7 @@ out: > > > > > f2fs_put_page(page, 1); > > > > > } > > > > > > > > > > -int truncate_blocks(struct inode *inode, u64 from) > > > > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > > > > { > > > > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > > > > unsigned int blocksize = inode->i_sb->s_blocksize; > > > > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > > free_from = (pgoff_t) > > > > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > > > > > > > - f2fs_lock_op(sbi); > > > > > + if (lock) > > > > > + f2fs_lock_op(sbi); > > > > > > > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > > > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > > > > if (err) { > > > > > if (err == -ENOENT) > > > > > goto free_next; > > > > > - f2fs_unlock_op(sbi); > > > > > + if (lock) > > > > > + f2fs_unlock_op(sbi); > > > > > trace_f2fs_truncate_blocks_exit(inode, err); > > > > > return err; > > > > > } > > > > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > > f2fs_put_dnode(&dn); > > > > > free_next: > > > > > err = truncate_inode_blocks(inode, free_from); > > > > > - f2fs_unlock_op(sbi); > > > > > + if (lock) > > > > > + f2fs_unlock_op(sbi); > > > > > done: > > > > > /* lastly zero out the first data page */ > > > > > truncate_partial_data_page(inode, from); > > > > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > > > > > > > trace_f2fs_truncate(inode); > > > > > > > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > > > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > > > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > > > > mark_inode_dirty(inode); > > > > > } > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > > > index 520758b..4d1f39f 100644 > > > > > --- a/fs/f2fs/inline.c > > > > > +++ b/fs/f2fs/inline.c > > > > > @@ -247,7 +247,7 @@ process_inline: > > > > > update_inode(inode, ipage); > > > > > f2fs_put_page(ipage, 1); > > > > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > > > > - truncate_blocks(inode, 0); > > > > > + truncate_blocks(inode, 0, false); > > > > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > > > > goto process_inline; > > > > > } > > > > > -- > > > > > 1.8.5.2 (Apple Git-48) > > > > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > > _______________________________________________ > > > > > Linux-f2fs-devel mailing list > > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > > > > Linux-f2fs-devel mailing list > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks @ 2014-08-22 6:56 ` Chao Yu 0 siblings, 0 replies; 20+ messages in thread From: Chao Yu @ 2014-08-22 6:56 UTC (permalink / raw) To: 'Jaegeuk Kim'; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel Hi Jaegeuk, > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Friday, August 22, 2014 12:45 AM > To: Chao Yu > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > On Wed, Aug 20, 2014 at 10:07:04AM +0800, Chao Yu wrote: > > > -----Original Message----- > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > Sent: Wednesday, August 20, 2014 12:58 AM > > > To: Chao Yu > > > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > > > linux-f2fs-devel@lists.sourceforge.net > > > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > > > > Hi Jaegeuk, > > > > > > > > > -----Original Message----- > > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > > > Sent: Saturday, August 16, 2014 6:04 AM > > > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > > > linux-f2fs-devel@lists.sourceforge.net > > > > > Cc: Jaegeuk Kim > > > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > > > > The callers holds f2fs_lock_op, so we should not call it again in > > > > > truncate_blocks. > > > > > > > > Nice catch! Your solution is a good way to fix this issue. > > > > > > > > Previously, in create inode path, I found there are some redundant codes between > > > > init_inode_metadata and evict_inode, including: > > > > truncate_inode_pages(&inode->i_data, 0); > > > > truncate_blocks(inode, 0); > > > > remove_dirty_dir_inode(inode); > > > > remove_inode_page(inode); > > > > > > > > So I think there is another way to fix this issue by removing error path handling > > > > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > > > > evict_inode. In this way we can avoid adding additional argument for all different > > > > callers. > > > > > > Well, possible. > > > But we need to take a closer look at the race condition on the inode cache. > > > What can happen if this bad inode is reassigned to the other thread? > > > > I don't get it. As I know, in evict(), we call ->evict_inode before > > remove_inode_hash(), so before all clean work was done we will not reassign > > this hashed uncleaned inode to other thread. > > > > Am I missing anything? > > What I meant was it may happen between init_inode_metadata and iput. Actually, can happen between unlock_new_inode and iput in error path of create/symlink/mkdir/mknod/tmpfile. Scenario is like this: ->f2fs_mkdir ->f2fs_add_link ->__f2fs_add_link ->init_inode_metadata failed here ->gc_thread_func ->f2fs_gc ->do_garbage_collect ->gc_data_segment ->f2fs_iget ->iget_locked ->wait_on_inode ->unlock_new_inode ->move_data_page ->make_bad_inode ->iput No problem now, but I'd like to remove unlock_new_inode from error path of inode creating procedure as we'd better wakeup waiter in end of ->iput instead of wakeup waiter in unlock_new_inode before invoking make_bad_inod. How do you think? Anyway, drop this proposal if you do not like it, although I think it will be ok theoretically after we fix above issue. Thanks, Yu > > Thanks, > > > > > Regards, > > Yu > > > > > > > > > > > > > How do you think? > > > > > > > > Thanks, > > > > Yu > > > > > > > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > --- > > > > > fs/f2fs/data.c | 2 +- > > > > > fs/f2fs/dir.c | 2 +- > > > > > fs/f2fs/f2fs.h | 2 +- > > > > > fs/f2fs/file.c | 13 ++++++++----- > > > > > fs/f2fs/inline.c | 2 +- > > > > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > > > index 68834e2..14cc3e8 100644 > > > > > --- a/fs/f2fs/data.c > > > > > +++ b/fs/f2fs/data.c > > > > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t > > > to) > > > > > > > > > > if (to > inode->i_size) { > > > > > truncate_pagecache(inode, inode->i_size); > > > > > - truncate_blocks(inode, inode->i_size); > > > > > + truncate_blocks(inode, inode->i_size, true); > > > > > } > > > > > } > > > > > > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > > > index a69bbfa..155fb05 100644 > > > > > --- a/fs/f2fs/dir.c > > > > > +++ b/fs/f2fs/dir.c > > > > > @@ -391,7 +391,7 @@ put_error: > > > > > error: > > > > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > > > > truncate_inode_pages(&inode->i_data, 0); > > > > > - truncate_blocks(inode, 0); > > > > > + truncate_blocks(inode, 0, false); > > > > > remove_dirty_dir_inode(inode); > > > > > remove_inode_page(inode); > > > > > return ERR_PTR(err); > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > > > index 2723b2d..7f976c1 100644 > > > > > --- a/fs/f2fs/f2fs.h > > > > > +++ b/fs/f2fs/f2fs.h > > > > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > > > > */ > > > > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > > > > void truncate_data_blocks(struct dnode_of_data *); > > > > > -int truncate_blocks(struct inode *, u64); > > > > > +int truncate_blocks(struct inode *, u64, bool); > > > > > void f2fs_truncate(struct inode *); > > > > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > > > > int f2fs_setattr(struct dentry *, struct iattr *); > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > > > index ecbdf6a..a8e97f8 100644 > > > > > --- a/fs/f2fs/file.c > > > > > +++ b/fs/f2fs/file.c > > > > > @@ -422,7 +422,7 @@ out: > > > > > f2fs_put_page(page, 1); > > > > > } > > > > > > > > > > -int truncate_blocks(struct inode *inode, u64 from) > > > > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > > > > { > > > > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > > > > unsigned int blocksize = inode->i_sb->s_blocksize; > > > > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > > free_from = (pgoff_t) > > > > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > > > > > > > - f2fs_lock_op(sbi); > > > > > + if (lock) > > > > > + f2fs_lock_op(sbi); > > > > > > > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > > > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > > > > if (err) { > > > > > if (err == -ENOENT) > > > > > goto free_next; > > > > > - f2fs_unlock_op(sbi); > > > > > + if (lock) > > > > > + f2fs_unlock_op(sbi); > > > > > trace_f2fs_truncate_blocks_exit(inode, err); > > > > > return err; > > > > > } > > > > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > > f2fs_put_dnode(&dn); > > > > > free_next: > > > > > err = truncate_inode_blocks(inode, free_from); > > > > > - f2fs_unlock_op(sbi); > > > > > + if (lock) > > > > > + f2fs_unlock_op(sbi); > > > > > done: > > > > > /* lastly zero out the first data page */ > > > > > truncate_partial_data_page(inode, from); > > > > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > > > > > > > trace_f2fs_truncate(inode); > > > > > > > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > > > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > > > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > > > > mark_inode_dirty(inode); > > > > > } > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > > > index 520758b..4d1f39f 100644 > > > > > --- a/fs/f2fs/inline.c > > > > > +++ b/fs/f2fs/inline.c > > > > > @@ -247,7 +247,7 @@ process_inline: > > > > > update_inode(inode, ipage); > > > > > f2fs_put_page(ipage, 1); > > > > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > > > > - truncate_blocks(inode, 0); > > > > > + truncate_blocks(inode, 0, false); > > > > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > > > > goto process_inline; > > > > > } > > > > > -- > > > > > 1.8.5.2 (Apple Git-48) > > > > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > > _______________________________________________ > > > > > Linux-f2fs-devel mailing list > > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > > > > Linux-f2fs-devel mailing list > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks 2014-08-22 6:56 ` [f2fs-dev] " Chao Yu @ 2014-08-22 15:49 ` Jaegeuk Kim -1 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-22 15:49 UTC (permalink / raw) To: Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel On Fri, Aug 22, 2014 at 02:56:37PM +0800, Chao Yu wrote: > Hi Jaegeuk, > > > -----Original Message----- > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > Sent: Friday, August 22, 2014 12:45 AM > > To: Chao Yu > > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-f2fs-devel@lists.sourceforge.net > > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > On Wed, Aug 20, 2014 at 10:07:04AM +0800, Chao Yu wrote: > > > > -----Original Message----- > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > > Sent: Wednesday, August 20, 2014 12:58 AM > > > > To: Chao Yu > > > > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > > > > linux-f2fs-devel@lists.sourceforge.net > > > > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > > > On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > > > > > Hi Jaegeuk, > > > > > > > > > > > -----Original Message----- > > > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > > > > Sent: Saturday, August 16, 2014 6:04 AM > > > > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > > > > linux-f2fs-devel@lists.sourceforge.net > > > > > > Cc: Jaegeuk Kim > > > > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > > > > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > > > > > The callers holds f2fs_lock_op, so we should not call it again in > > > > > > truncate_blocks. > > > > > > > > > > Nice catch! Your solution is a good way to fix this issue. > > > > > > > > > > Previously, in create inode path, I found there are some redundant codes between > > > > > init_inode_metadata and evict_inode, including: > > > > > truncate_inode_pages(&inode->i_data, 0); > > > > > truncate_blocks(inode, 0); > > > > > remove_dirty_dir_inode(inode); > > > > > remove_inode_page(inode); > > > > > > > > > > So I think there is another way to fix this issue by removing error path handling > > > > > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > > > > > evict_inode. In this way we can avoid adding additional argument for all different > > > > > callers. > > > > > > > > Well, possible. > > > > But we need to take a closer look at the race condition on the inode cache. > > > > What can happen if this bad inode is reassigned to the other thread? > > > > > > I don't get it. As I know, in evict(), we call ->evict_inode before > > > remove_inode_hash(), so before all clean work was done we will not reassign > > > this hashed uncleaned inode to other thread. > > > > > > Am I missing anything? > > > > What I meant was it may happen between init_inode_metadata and iput. > > Actually, can happen between unlock_new_inode and iput in error path of > create/symlink/mkdir/mknod/tmpfile. Scenario is like this: > > ->f2fs_mkdir > ->f2fs_add_link > ->__f2fs_add_link > ->init_inode_metadata failed here > ->gc_thread_func > ->f2fs_gc > ->do_garbage_collect > ->gc_data_segment > ->f2fs_iget > ->iget_locked > ->wait_on_inode > ->unlock_new_inode > ->move_data_page > ->make_bad_inode > ->iput > > No problem now, but I'd like to remove unlock_new_inode from error path of inode > creating procedure as we'd better wakeup waiter in end of ->iput instead of > wakeup waiter in unlock_new_inode before invoking make_bad_inod. > > How do you think? Hmm. It seems that this is a lot different issue wrt this patch. Anyway, I agreed that it needs to relocate unlock_new_inode. Thanks, > > Anyway, drop this proposal if you do not like it, although I think it will be > ok theoretically after we fix above issue. > > Thanks, > Yu > > > > > Thanks, > > > > > > > > Regards, > > > Yu > > > > > > > > > > > > > > > > > How do you think? > > > > > > > > > > Thanks, > > > > > Yu > > > > > > > > > > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > --- > > > > > > fs/f2fs/data.c | 2 +- > > > > > > fs/f2fs/dir.c | 2 +- > > > > > > fs/f2fs/f2fs.h | 2 +- > > > > > > fs/f2fs/file.c | 13 ++++++++----- > > > > > > fs/f2fs/inline.c | 2 +- > > > > > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > > > > index 68834e2..14cc3e8 100644 > > > > > > --- a/fs/f2fs/data.c > > > > > > +++ b/fs/f2fs/data.c > > > > > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t > > > > to) > > > > > > > > > > > > if (to > inode->i_size) { > > > > > > truncate_pagecache(inode, inode->i_size); > > > > > > - truncate_blocks(inode, inode->i_size); > > > > > > + truncate_blocks(inode, inode->i_size, true); > > > > > > } > > > > > > } > > > > > > > > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > > > > index a69bbfa..155fb05 100644 > > > > > > --- a/fs/f2fs/dir.c > > > > > > +++ b/fs/f2fs/dir.c > > > > > > @@ -391,7 +391,7 @@ put_error: > > > > > > error: > > > > > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > > > > > truncate_inode_pages(&inode->i_data, 0); > > > > > > - truncate_blocks(inode, 0); > > > > > > + truncate_blocks(inode, 0, false); > > > > > > remove_dirty_dir_inode(inode); > > > > > > remove_inode_page(inode); > > > > > > return ERR_PTR(err); > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > > > > index 2723b2d..7f976c1 100644 > > > > > > --- a/fs/f2fs/f2fs.h > > > > > > +++ b/fs/f2fs/f2fs.h > > > > > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > > > > > */ > > > > > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > > > > > void truncate_data_blocks(struct dnode_of_data *); > > > > > > -int truncate_blocks(struct inode *, u64); > > > > > > +int truncate_blocks(struct inode *, u64, bool); > > > > > > void f2fs_truncate(struct inode *); > > > > > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > > > > > int f2fs_setattr(struct dentry *, struct iattr *); > > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > > > > index ecbdf6a..a8e97f8 100644 > > > > > > --- a/fs/f2fs/file.c > > > > > > +++ b/fs/f2fs/file.c > > > > > > @@ -422,7 +422,7 @@ out: > > > > > > f2fs_put_page(page, 1); > > > > > > } > > > > > > > > > > > > -int truncate_blocks(struct inode *inode, u64 from) > > > > > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > > > > > { > > > > > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > > > > > unsigned int blocksize = inode->i_sb->s_blocksize; > > > > > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > > > free_from = (pgoff_t) > > > > > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > > > > > > > > > - f2fs_lock_op(sbi); > > > > > > + if (lock) > > > > > > + f2fs_lock_op(sbi); > > > > > > > > > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > > > > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > > > > > if (err) { > > > > > > if (err == -ENOENT) > > > > > > goto free_next; > > > > > > - f2fs_unlock_op(sbi); > > > > > > + if (lock) > > > > > > + f2fs_unlock_op(sbi); > > > > > > trace_f2fs_truncate_blocks_exit(inode, err); > > > > > > return err; > > > > > > } > > > > > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > > > f2fs_put_dnode(&dn); > > > > > > free_next: > > > > > > err = truncate_inode_blocks(inode, free_from); > > > > > > - f2fs_unlock_op(sbi); > > > > > > + if (lock) > > > > > > + f2fs_unlock_op(sbi); > > > > > > done: > > > > > > /* lastly zero out the first data page */ > > > > > > truncate_partial_data_page(inode, from); > > > > > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > > > > > > > > > trace_f2fs_truncate(inode); > > > > > > > > > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > > > > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > > > > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > > > > > mark_inode_dirty(inode); > > > > > > } > > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > > > > index 520758b..4d1f39f 100644 > > > > > > --- a/fs/f2fs/inline.c > > > > > > +++ b/fs/f2fs/inline.c > > > > > > @@ -247,7 +247,7 @@ process_inline: > > > > > > update_inode(inode, ipage); > > > > > > f2fs_put_page(ipage, 1); > > > > > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > > > > > - truncate_blocks(inode, 0); > > > > > > + truncate_blocks(inode, 0, false); > > > > > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > > > > > goto process_inline; > > > > > > } > > > > > > -- > > > > > > 1.8.5.2 (Apple Git-48) > > > > > > > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > > > _______________________________________________ > > > > > > Linux-f2fs-devel mailing list > > > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > > _______________________________________________ > > > > > Linux-f2fs-devel mailing list > > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks @ 2014-08-22 15:49 ` Jaegeuk Kim 0 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-22 15:49 UTC (permalink / raw) To: Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel On Fri, Aug 22, 2014 at 02:56:37PM +0800, Chao Yu wrote: > Hi Jaegeuk, > > > -----Original Message----- > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > Sent: Friday, August 22, 2014 12:45 AM > > To: Chao Yu > > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-f2fs-devel@lists.sourceforge.net > > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > On Wed, Aug 20, 2014 at 10:07:04AM +0800, Chao Yu wrote: > > > > -----Original Message----- > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > > Sent: Wednesday, August 20, 2014 12:58 AM > > > > To: Chao Yu > > > > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; > > > > linux-f2fs-devel@lists.sourceforge.net > > > > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > > > On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote: > > > > > Hi Jaegeuk, > > > > > > > > > > > -----Original Message----- > > > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > > > > > Sent: Saturday, August 16, 2014 6:04 AM > > > > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > > > > linux-f2fs-devel@lists.sourceforge.net > > > > > > Cc: Jaegeuk Kim > > > > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks > > > > > > > > > > > > The init_inode_metadata calls truncate_blocks when error is occurred. > > > > > > The callers holds f2fs_lock_op, so we should not call it again in > > > > > > truncate_blocks. > > > > > > > > > > Nice catch! Your solution is a good way to fix this issue. > > > > > > > > > > Previously, in create inode path, I found there are some redundant codes between > > > > > init_inode_metadata and evict_inode, including: > > > > > truncate_inode_pages(&inode->i_data, 0); > > > > > truncate_blocks(inode, 0); > > > > > remove_dirty_dir_inode(inode); > > > > > remove_inode_page(inode); > > > > > > > > > > So I think there is another way to fix this issue by removing error path handling > > > > > codes in init_inode_metadata, not making the inode bad to left garbage clean work in > > > > > evict_inode. In this way we can avoid adding additional argument for all different > > > > > callers. > > > > > > > > Well, possible. > > > > But we need to take a closer look at the race condition on the inode cache. > > > > What can happen if this bad inode is reassigned to the other thread? > > > > > > I don't get it. As I know, in evict(), we call ->evict_inode before > > > remove_inode_hash(), so before all clean work was done we will not reassign > > > this hashed uncleaned inode to other thread. > > > > > > Am I missing anything? > > > > What I meant was it may happen between init_inode_metadata and iput. > > Actually, can happen between unlock_new_inode and iput in error path of > create/symlink/mkdir/mknod/tmpfile. Scenario is like this: > > ->f2fs_mkdir > ->f2fs_add_link > ->__f2fs_add_link > ->init_inode_metadata failed here > ->gc_thread_func > ->f2fs_gc > ->do_garbage_collect > ->gc_data_segment > ->f2fs_iget > ->iget_locked > ->wait_on_inode > ->unlock_new_inode > ->move_data_page > ->make_bad_inode > ->iput > > No problem now, but I'd like to remove unlock_new_inode from error path of inode > creating procedure as we'd better wakeup waiter in end of ->iput instead of > wakeup waiter in unlock_new_inode before invoking make_bad_inod. > > How do you think? Hmm. It seems that this is a lot different issue wrt this patch. Anyway, I agreed that it needs to relocate unlock_new_inode. Thanks, > > Anyway, drop this proposal if you do not like it, although I think it will be > ok theoretically after we fix above issue. > > Thanks, > Yu > > > > > Thanks, > > > > > > > > Regards, > > > Yu > > > > > > > > > > > > > > > > > How do you think? > > > > > > > > > > Thanks, > > > > > Yu > > > > > > > > > > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > --- > > > > > > fs/f2fs/data.c | 2 +- > > > > > > fs/f2fs/dir.c | 2 +- > > > > > > fs/f2fs/f2fs.h | 2 +- > > > > > > fs/f2fs/file.c | 13 ++++++++----- > > > > > > fs/f2fs/inline.c | 2 +- > > > > > > 5 files changed, 12 insertions(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > > > > index 68834e2..14cc3e8 100644 > > > > > > --- a/fs/f2fs/data.c > > > > > > +++ b/fs/f2fs/data.c > > > > > > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t > > > > to) > > > > > > > > > > > > if (to > inode->i_size) { > > > > > > truncate_pagecache(inode, inode->i_size); > > > > > > - truncate_blocks(inode, inode->i_size); > > > > > > + truncate_blocks(inode, inode->i_size, true); > > > > > > } > > > > > > } > > > > > > > > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > > > > index a69bbfa..155fb05 100644 > > > > > > --- a/fs/f2fs/dir.c > > > > > > +++ b/fs/f2fs/dir.c > > > > > > @@ -391,7 +391,7 @@ put_error: > > > > > > error: > > > > > > /* once the failed inode becomes a bad inode, i_mode is S_IFREG */ > > > > > > truncate_inode_pages(&inode->i_data, 0); > > > > > > - truncate_blocks(inode, 0); > > > > > > + truncate_blocks(inode, 0, false); > > > > > > remove_dirty_dir_inode(inode); > > > > > > remove_inode_page(inode); > > > > > > return ERR_PTR(err); > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > > > > index 2723b2d..7f976c1 100644 > > > > > > --- a/fs/f2fs/f2fs.h > > > > > > +++ b/fs/f2fs/f2fs.h > > > > > > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi) > > > > > > */ > > > > > > int f2fs_sync_file(struct file *, loff_t, loff_t, int); > > > > > > void truncate_data_blocks(struct dnode_of_data *); > > > > > > -int truncate_blocks(struct inode *, u64); > > > > > > +int truncate_blocks(struct inode *, u64, bool); > > > > > > void f2fs_truncate(struct inode *); > > > > > > int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *); > > > > > > int f2fs_setattr(struct dentry *, struct iattr *); > > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > > > > index ecbdf6a..a8e97f8 100644 > > > > > > --- a/fs/f2fs/file.c > > > > > > +++ b/fs/f2fs/file.c > > > > > > @@ -422,7 +422,7 @@ out: > > > > > > f2fs_put_page(page, 1); > > > > > > } > > > > > > > > > > > > -int truncate_blocks(struct inode *inode, u64 from) > > > > > > +int truncate_blocks(struct inode *inode, u64 from, bool lock) > > > > > > { > > > > > > struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); > > > > > > unsigned int blocksize = inode->i_sb->s_blocksize; > > > > > > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > > > free_from = (pgoff_t) > > > > > > ((from + blocksize - 1) >> (sbi->log_blocksize)); > > > > > > > > > > > > - f2fs_lock_op(sbi); > > > > > > + if (lock) > > > > > > + f2fs_lock_op(sbi); > > > > > > > > > > > > set_new_dnode(&dn, inode, NULL, NULL, 0); > > > > > > err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE); > > > > > > if (err) { > > > > > > if (err == -ENOENT) > > > > > > goto free_next; > > > > > > - f2fs_unlock_op(sbi); > > > > > > + if (lock) > > > > > > + f2fs_unlock_op(sbi); > > > > > > trace_f2fs_truncate_blocks_exit(inode, err); > > > > > > return err; > > > > > > } > > > > > > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from) > > > > > > f2fs_put_dnode(&dn); > > > > > > free_next: > > > > > > err = truncate_inode_blocks(inode, free_from); > > > > > > - f2fs_unlock_op(sbi); > > > > > > + if (lock) > > > > > > + f2fs_unlock_op(sbi); > > > > > > done: > > > > > > /* lastly zero out the first data page */ > > > > > > truncate_partial_data_page(inode, from); > > > > > > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode) > > > > > > > > > > > > trace_f2fs_truncate(inode); > > > > > > > > > > > > - if (!truncate_blocks(inode, i_size_read(inode))) { > > > > > > + if (!truncate_blocks(inode, i_size_read(inode), true)) { > > > > > > inode->i_mtime = inode->i_ctime = CURRENT_TIME; > > > > > > mark_inode_dirty(inode); > > > > > > } > > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > > > > index 520758b..4d1f39f 100644 > > > > > > --- a/fs/f2fs/inline.c > > > > > > +++ b/fs/f2fs/inline.c > > > > > > @@ -247,7 +247,7 @@ process_inline: > > > > > > update_inode(inode, ipage); > > > > > > f2fs_put_page(ipage, 1); > > > > > > } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { > > > > > > - truncate_blocks(inode, 0); > > > > > > + truncate_blocks(inode, 0, false); > > > > > > set_inode_flag(F2FS_I(inode), FI_INLINE_DATA); > > > > > > goto process_inline; > > > > > > } > > > > > > -- > > > > > > 1.8.5.2 (Apple Git-48) > > > > > > > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > > > _______________________________________________ > > > > > > Linux-f2fs-devel mailing list > > > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > > _______________________________________________ > > > > > Linux-f2fs-devel mailing list > > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] f2fs: remove rewrite_node_page 2014-08-15 22:03 ` Jaegeuk Kim @ 2014-08-15 22:03 ` Jaegeuk Kim -1 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-15 22:03 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim I think we need to let the dirty node pages remain in the page cache instead of rewriting them in their places. So, after done with successful recovery, write_checkpoint will flush all of them through the normal write path. Through this, we can avoid potential error cases in terms of block allocation. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/f2fs.h | 4 ---- fs/f2fs/node.c | 9 --------- fs/f2fs/recovery.c | 2 -- fs/f2fs/segment.c | 49 ------------------------------------------------- 4 files changed, 64 deletions(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 7f976c1..e921242 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1207,8 +1207,6 @@ int sync_node_pages(struct f2fs_sb_info *, nid_t, struct writeback_control *); bool alloc_nid(struct f2fs_sb_info *, nid_t *); void alloc_nid_done(struct f2fs_sb_info *, nid_t); void alloc_nid_failed(struct f2fs_sb_info *, nid_t); -void recover_node_page(struct f2fs_sb_info *, struct page *, - struct f2fs_summary *, struct node_info *, block_t); void recover_inline_xattr(struct inode *, struct page *); void recover_xattr_data(struct inode *, struct page *, block_t); int recover_inode_page(struct f2fs_sb_info *, struct page *); @@ -1243,8 +1241,6 @@ void write_data_page(struct page *, struct dnode_of_data *, block_t *, void rewrite_data_page(struct page *, block_t, struct f2fs_io_info *); void recover_data_page(struct f2fs_sb_info *, struct page *, struct f2fs_summary *, block_t, block_t); -void rewrite_node_page(struct f2fs_sb_info *, struct page *, - struct f2fs_summary *, block_t, block_t); void allocate_data_block(struct f2fs_sb_info *, struct page *, block_t, block_t *, struct f2fs_summary *, int); void f2fs_wait_on_page_writeback(struct page *, enum page_type); diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index d2f7842..b4d9640 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1545,15 +1545,6 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid) kmem_cache_free(free_nid_slab, i); } -void recover_node_page(struct f2fs_sb_info *sbi, struct page *page, - struct f2fs_summary *sum, struct node_info *ni, - block_t new_blkaddr) -{ - rewrite_node_page(sbi, page, sum, ni->blk_addr, new_blkaddr); - set_node_addr(sbi, ni, new_blkaddr, false); - clear_node_page_dirty(page); -} - void recover_inline_xattr(struct inode *inode, struct page *page) { struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c index d36ef35..756c41c 100644 --- a/fs/f2fs/recovery.c +++ b/fs/f2fs/recovery.c @@ -371,8 +371,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode, fill_node_footer(dn.node_page, dn.nid, ni.ino, ofs_of_node(page), false); set_page_dirty(dn.node_page); - - recover_node_page(sbi, dn.node_page, &sum, &ni, blkaddr); err: f2fs_put_dnode(&dn); f2fs_unlock_op(sbi); diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 31b630e..0aa337c 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -1103,55 +1103,6 @@ void recover_data_page(struct f2fs_sb_info *sbi, mutex_unlock(&curseg->curseg_mutex); } -void rewrite_node_page(struct f2fs_sb_info *sbi, - struct page *page, struct f2fs_summary *sum, - block_t old_blkaddr, block_t new_blkaddr) -{ - struct sit_info *sit_i = SIT_I(sbi); - int type = CURSEG_WARM_NODE; - struct curseg_info *curseg; - unsigned int segno, old_cursegno; - block_t next_blkaddr = next_blkaddr_of_node(page); - unsigned int next_segno = GET_SEGNO(sbi, next_blkaddr); - struct f2fs_io_info fio = { - .type = NODE, - .rw = WRITE_SYNC, - }; - - curseg = CURSEG_I(sbi, type); - - mutex_lock(&curseg->curseg_mutex); - mutex_lock(&sit_i->sentry_lock); - - segno = GET_SEGNO(sbi, new_blkaddr); - old_cursegno = curseg->segno; - - /* change the current segment */ - if (segno != curseg->segno) { - curseg->next_segno = segno; - change_curseg(sbi, type, true); - } - curseg->next_blkoff = GET_BLKOFF_FROM_SEG0(sbi, new_blkaddr); - __add_sum_entry(sbi, type, sum); - - /* change the current log to the next block addr in advance */ - if (next_segno != segno) { - curseg->next_segno = next_segno; - change_curseg(sbi, type, true); - } - curseg->next_blkoff = GET_BLKOFF_FROM_SEG0(sbi, next_blkaddr); - - /* rewrite node page */ - set_page_writeback(page); - f2fs_submit_page_mbio(sbi, page, new_blkaddr, &fio); - f2fs_submit_merged_bio(sbi, NODE, WRITE); - refresh_sit_entry(sbi, old_blkaddr, new_blkaddr); - locate_dirty_segment(sbi, old_cursegno); - - mutex_unlock(&sit_i->sentry_lock); - mutex_unlock(&curseg->curseg_mutex); -} - static inline bool is_merged_page(struct f2fs_sb_info *sbi, struct page *page, enum page_type type) { -- 1.8.5.2 (Apple Git-48) ------------------------------------------------------------------------------ ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/4] f2fs: remove rewrite_node_page @ 2014-08-15 22:03 ` Jaegeuk Kim 0 siblings, 0 replies; 20+ messages in thread From: Jaegeuk Kim @ 2014-08-15 22:03 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim I think we need to let the dirty node pages remain in the page cache instead of rewriting them in their places. So, after done with successful recovery, write_checkpoint will flush all of them through the normal write path. Through this, we can avoid potential error cases in terms of block allocation. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/f2fs.h | 4 ---- fs/f2fs/node.c | 9 --------- fs/f2fs/recovery.c | 2 -- fs/f2fs/segment.c | 49 ------------------------------------------------- 4 files changed, 64 deletions(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 7f976c1..e921242 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1207,8 +1207,6 @@ int sync_node_pages(struct f2fs_sb_info *, nid_t, struct writeback_control *); bool alloc_nid(struct f2fs_sb_info *, nid_t *); void alloc_nid_done(struct f2fs_sb_info *, nid_t); void alloc_nid_failed(struct f2fs_sb_info *, nid_t); -void recover_node_page(struct f2fs_sb_info *, struct page *, - struct f2fs_summary *, struct node_info *, block_t); void recover_inline_xattr(struct inode *, struct page *); void recover_xattr_data(struct inode *, struct page *, block_t); int recover_inode_page(struct f2fs_sb_info *, struct page *); @@ -1243,8 +1241,6 @@ void write_data_page(struct page *, struct dnode_of_data *, block_t *, void rewrite_data_page(struct page *, block_t, struct f2fs_io_info *); void recover_data_page(struct f2fs_sb_info *, struct page *, struct f2fs_summary *, block_t, block_t); -void rewrite_node_page(struct f2fs_sb_info *, struct page *, - struct f2fs_summary *, block_t, block_t); void allocate_data_block(struct f2fs_sb_info *, struct page *, block_t, block_t *, struct f2fs_summary *, int); void f2fs_wait_on_page_writeback(struct page *, enum page_type); diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index d2f7842..b4d9640 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1545,15 +1545,6 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid) kmem_cache_free(free_nid_slab, i); } -void recover_node_page(struct f2fs_sb_info *sbi, struct page *page, - struct f2fs_summary *sum, struct node_info *ni, - block_t new_blkaddr) -{ - rewrite_node_page(sbi, page, sum, ni->blk_addr, new_blkaddr); - set_node_addr(sbi, ni, new_blkaddr, false); - clear_node_page_dirty(page); -} - void recover_inline_xattr(struct inode *inode, struct page *page) { struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb); diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c index d36ef35..756c41c 100644 --- a/fs/f2fs/recovery.c +++ b/fs/f2fs/recovery.c @@ -371,8 +371,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode, fill_node_footer(dn.node_page, dn.nid, ni.ino, ofs_of_node(page), false); set_page_dirty(dn.node_page); - - recover_node_page(sbi, dn.node_page, &sum, &ni, blkaddr); err: f2fs_put_dnode(&dn); f2fs_unlock_op(sbi); diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 31b630e..0aa337c 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -1103,55 +1103,6 @@ void recover_data_page(struct f2fs_sb_info *sbi, mutex_unlock(&curseg->curseg_mutex); } -void rewrite_node_page(struct f2fs_sb_info *sbi, - struct page *page, struct f2fs_summary *sum, - block_t old_blkaddr, block_t new_blkaddr) -{ - struct sit_info *sit_i = SIT_I(sbi); - int type = CURSEG_WARM_NODE; - struct curseg_info *curseg; - unsigned int segno, old_cursegno; - block_t next_blkaddr = next_blkaddr_of_node(page); - unsigned int next_segno = GET_SEGNO(sbi, next_blkaddr); - struct f2fs_io_info fio = { - .type = NODE, - .rw = WRITE_SYNC, - }; - - curseg = CURSEG_I(sbi, type); - - mutex_lock(&curseg->curseg_mutex); - mutex_lock(&sit_i->sentry_lock); - - segno = GET_SEGNO(sbi, new_blkaddr); - old_cursegno = curseg->segno; - - /* change the current segment */ - if (segno != curseg->segno) { - curseg->next_segno = segno; - change_curseg(sbi, type, true); - } - curseg->next_blkoff = GET_BLKOFF_FROM_SEG0(sbi, new_blkaddr); - __add_sum_entry(sbi, type, sum); - - /* change the current log to the next block addr in advance */ - if (next_segno != segno) { - curseg->next_segno = next_segno; - change_curseg(sbi, type, true); - } - curseg->next_blkoff = GET_BLKOFF_FROM_SEG0(sbi, next_blkaddr); - - /* rewrite node page */ - set_page_writeback(page); - f2fs_submit_page_mbio(sbi, page, new_blkaddr, &fio); - f2fs_submit_merged_bio(sbi, NODE, WRITE); - refresh_sit_entry(sbi, old_blkaddr, new_blkaddr); - locate_dirty_segment(sbi, old_cursegno); - - mutex_unlock(&sit_i->sentry_lock); - mutex_unlock(&curseg->curseg_mutex); -} - static inline bool is_merged_page(struct f2fs_sb_info *sbi, struct page *page, enum page_type type) { -- 1.8.5.2 (Apple Git-48) ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2014-08-22 15:49 UTC | newest] Thread overview: 20+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-15 22:03 [PATCH 1/4] f2fs: add WARN_ON in f2fs_bug_on Jaegeuk Kim 2014-08-15 22:03 ` Jaegeuk Kim 2014-08-15 22:03 ` [PATCH 2/4] f2fs: prevent checkpoint during roll-forward Jaegeuk Kim 2014-08-15 22:03 ` Jaegeuk Kim 2014-08-15 22:03 ` [PATCH 3/4] f2fs: avoid double lock in truncate_blocks Jaegeuk Kim 2014-08-15 22:03 ` Jaegeuk Kim 2014-08-19 8:04 ` Chao Yu 2014-08-19 8:04 ` [f2fs-dev] " Chao Yu 2014-08-19 16:58 ` Jaegeuk Kim 2014-08-19 16:58 ` [f2fs-dev] " Jaegeuk Kim 2014-08-20 2:07 ` Chao Yu 2014-08-20 2:07 ` [f2fs-dev] " Chao Yu 2014-08-21 16:45 ` Jaegeuk Kim 2014-08-21 16:45 ` [f2fs-dev] " Jaegeuk Kim 2014-08-22 6:56 ` Chao Yu 2014-08-22 6:56 ` [f2fs-dev] " Chao Yu 2014-08-22 15:49 ` Jaegeuk Kim 2014-08-22 15:49 ` [f2fs-dev] " Jaegeuk Kim 2014-08-15 22:03 ` [PATCH 4/4] f2fs: remove rewrite_node_page Jaegeuk Kim 2014-08-15 22:03 ` Jaegeuk Kim
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.