[PATCH 1/4] f2fs: add WARN_ON in f2fs_bug_on

[PATCH 1/4] f2fs: add WARN_ON in f2fs_bug_on

[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)


------------------------------------------------------------------------------

[PATCH 2/4] f2fs: prevent checkpoint during roll-forward

[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)


------------------------------------------------------------------------------

[PATCH 3/4] f2fs: avoid double lock in truncate_blocks

[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)


------------------------------------------------------------------------------

[PATCH 4/4] f2fs: remove rewrite_node_page

[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)


------------------------------------------------------------------------------

Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks

[Chao Yu]

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


------------------------------------------------------------------------------

Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks

[Jaegeuk Kim]

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

------------------------------------------------------------------------------

Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks

[Chao Yu]

> -----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/

Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks

[Jaegeuk Kim]

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/

Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks

[Chao Yu]

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/

Re: [PATCH 3/4] f2fs: avoid double lock in truncate_blocks

[Jaegeuk Kim]

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/