[PATCH 0/4] optimize f2fs IPU v3

[PATCH 0/4] optimize f2fs IPU v3

[Hou Pengyang]

patch 1): Reconstruct code in do_write_data_page, code logic stays unchanged,
patch 2): f2fs: lookup extent cache first under IPU scenario
patch 3): Check IPU before cp to avoid IPU being blocked by cp
patch 3): Unlock dnode before IPU

Change log for v3: 
    - check IPU without f2fs_lock_op, to avoid IPU(fsync/fdatasync IPU) being blocked by long time cp

Hou Pengyang (4):
  f2fs: reconstruct code to write a data page
  f2fs: lookup extent cache first under IPU scenario
  f2fs: check IPU before cp to avoid IPU being blocked by cp
  f2fs: unlock dnode before IPU

 fs/f2fs/data.c    | 98 +++++++++++++++++++++++++++++++++++--------------------
 fs/f2fs/file.c    |  2 +-
 fs/f2fs/gc.c      |  2 ++
 fs/f2fs/segment.c |  2 ++
 fs/f2fs/segment.h |  2 +-
 5 files changed, 69 insertions(+), 37 deletions(-)

-- 
2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH 1/4] f2fs: reconstruct code to write a data page

[Hou Pengyang]

This patch introduces encrypt_one_page which encrypts one data page before
submit_bio, and change the use of need_inplace_update.

Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c    | 77 +++++++++++++++++++++++++++++++++----------------------
 fs/f2fs/file.c    |  2 +-
 fs/f2fs/segment.h |  2 +-
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index b8dcd1e..02400b0 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1302,6 +1302,45 @@ static int f2fs_read_data_pages(struct file *file,
 	return f2fs_mpage_readpages(mapping, pages, NULL, nr_pages);
 }
 
+static int encrypt_one_page(struct f2fs_io_info *fio)
+{
+	struct inode *inode = fio->page->mapping->host;
+	gfp_t gfp_flags = GFP_NOFS;
+
+	if (!f2fs_encrypted_inode(inode) || !S_ISREG(inode->i_mode))
+		return 0;
+
+	/* wait for GCed encrypted page writeback */
+	f2fs_wait_on_encrypted_page_writeback(fio->sbi, fio->old_blkaddr);
+
+retry_encrypt:
+	fio->encrypted_page = fscrypt_encrypt_page(inode, fio->page,
+			PAGE_SIZE, 0, fio->page->index, gfp_flags);
+	if (!IS_ERR(fio->encrypted_page))
+		return 0;
+
+	/* flush pending IOs and wait for a while in the ENOMEM case */
+	if (PTR_ERR(fio->encrypted_page) == -ENOMEM) {
+		f2fs_flush_merged_bios(fio->sbi);
+		congestion_wait(BLK_RW_ASYNC, HZ/50);
+		gfp_flags |= __GFP_NOFAIL;
+		goto retry_encrypt;
+	}
+	return PTR_ERR(fio->encrypted_page);
+}
+
+static inline bool need_inplace_update(struct f2fs_io_info *fio)
+{
+	if (fio->old_blkaddr == NEW_ADDR)
+		return false;
+	if (is_cold_data(fio->page))
+		return false;
+	if (IS_ATOMIC_WRITTEN_PAGE(fio->page))
+		return false;
+
+	return need_inplace_update_policy(fio->page->mapping->host, fio);
+}
+
 int do_write_data_page(struct f2fs_io_info *fio)
 {
 	struct page *page = fio->page;
@@ -1322,30 +1361,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		goto out_writepage;
 	}
 
-	if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode)) {
-		gfp_t gfp_flags = GFP_NOFS;
-
-		/* wait for GCed encrypted page writeback */
-		f2fs_wait_on_encrypted_page_writeback(F2FS_I_SB(inode),
-							fio->old_blkaddr);
-retry_encrypt:
-		fio->encrypted_page = fscrypt_encrypt_page(inode, fio->page,
-							PAGE_SIZE, 0,
-							fio->page->index,
-							gfp_flags);
-		if (IS_ERR(fio->encrypted_page)) {
-			err = PTR_ERR(fio->encrypted_page);
-			if (err == -ENOMEM) {
-				/* flush pending ios and wait for a while */
-				f2fs_flush_merged_bios(F2FS_I_SB(inode));
-				congestion_wait(BLK_RW_ASYNC, HZ/50);
-				gfp_flags |= __GFP_NOFAIL;
-				err = 0;
-				goto retry_encrypt;
-			}
-			goto out_writepage;
-		}
-	}
+	err = encrypt_one_page(fio);
+	if (err)
+		goto out_writepage;
 
 	set_page_writeback(page);
 
@@ -1353,15 +1371,14 @@ int do_write_data_page(struct f2fs_io_info *fio)
 	 * If current allocation needs SSR,
 	 * it had better in-place writes for updated data.
 	 */
-	if (unlikely(fio->old_blkaddr != NEW_ADDR &&
-			!is_cold_data(page) &&
-			!IS_ATOMIC_WRITTEN_PAGE(page) &&
-			need_inplace_update(inode, fio))) {
-		f2fs_unlock_op(F2FS_I_SB(inode));
+	if (need_inplace_update(fio)) {
+		f2fs_bug_on(fio->sbi, !fio->cp_rwsem_locked);
+		f2fs_unlock_op(fio->sbi);
 		fio->cp_rwsem_locked = false;
+
 		err = rewrite_data_page(fio);
+		trace_f2fs_do_write_data_page(fio->page, IPU);
 		set_inode_flag(inode, FI_UPDATE_WRITE);
-		trace_f2fs_do_write_data_page(page, IPU);
 	} else {
 		write_data_page(&dn, fio);
 		trace_f2fs_do_write_data_page(page, OPU);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e7a2b54..a0d9588 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1898,7 +1898,7 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
 	int err;
 
 	/* if in-place-update policy is enabled, don't waste time here */
-	if (need_inplace_update(inode, NULL))
+	if (need_inplace_update_policy(inode, NULL))
 		return -EINVAL;
 
 	pg_start = range->start >> PAGE_SHIFT;
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 4a85035..2a6b8c1 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -561,7 +561,7 @@ enum {
 	F2FS_IPU_ASYNC,
 };
 
-static inline bool need_inplace_update(struct inode *inode,
+static inline bool need_inplace_update_policy(struct inode *inode,
 				struct f2fs_io_info *fio)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
-- 
2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH 2/4] f2fs: lookup extent cache first under IPU scenario

[Hou Pengyang]

If a page is cold, NOT atomit written and need_ipu now, there is
a high probability that IPU should be adapted. For IPU, we try to
check extent tree to get the block index first, instead of reading
the dnode page, where may lead to an useless dnode IO, since no need to
update the dnode index for IPU.

Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c    | 11 ++++++++++-
 fs/f2fs/gc.c      |  1 +
 fs/f2fs/segment.c |  1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 02400b0..183a426 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1346,21 +1346,29 @@ int do_write_data_page(struct f2fs_io_info *fio)
 	struct page *page = fio->page;
 	struct inode *inode = page->mapping->host;
 	struct dnode_of_data dn;
+	struct extent_info ei = {0,0,0};
 	int err = 0;
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
+	if (need_inplace_update(fio) &&
+			f2fs_lookup_extent_cache(inode, page->index, &ei)) {
+		fio->old_blkaddr = ei.blk + page->index - ei.fofs;
+		if (fio->old_blkaddr != NULL_ADDR &&
+				fio->old_blkaddr != NEW_ADDR)
+			goto got_it;
+	}
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
 		return err;
 
 	fio->old_blkaddr = dn.data_blkaddr;
-
 	/* This page is already truncated */
 	if (fio->old_blkaddr == NULL_ADDR) {
 		ClearPageUptodate(page);
 		goto out_writepage;
 	}
 
+got_it:
 	err = encrypt_one_page(fio);
 	if (err)
 		goto out_writepage;
@@ -1408,6 +1416,7 @@ static int __write_data_page(struct page *page, bool *submitted,
 		.type = DATA,
 		.op = REQ_OP_WRITE,
 		.op_flags = wbc_to_write_flags(wbc),
+		.old_blkaddr = NULL_ADDR,
 		.page = page,
 		.encrypted_page = NULL,
 		.submitted = false,
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 499a43f..e034857 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -712,6 +712,7 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
 			.type = DATA,
 			.op = REQ_OP_WRITE,
 			.op_flags = REQ_SYNC,
+			.old_blkaddr = NULL_ADDR,
 			.page = page,
 			.encrypted_page = NULL,
 		};
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 2a95535..9f86b98 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -291,6 +291,7 @@ static int __commit_inmem_pages(struct inode *inode,
 		.type = DATA,
 		.op = REQ_OP_WRITE,
 		.op_flags = REQ_SYNC | REQ_PRIO,
+		.old_blkaddr = NULL_ADDR,
 		.encrypted_page = NULL,
 	};
 	pgoff_t last_idx = ULONG_MAX;
-- 
2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 2/4] f2fs: lookup extent cache first under IPU scenario

[Jaegeuk Kim]

Hi Pengyang,

On 04/25, Hou Pengyang wrote:
> If a page is cold, NOT atomit written and need_ipu now, there is
> a high probability that IPU should be adapted. For IPU, we try to
> check extent tree to get the block index first, instead of reading
> the dnode page, where may lead to an useless dnode IO, since no need to
> update the dnode index for IPU.
> 
> Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/data.c    | 11 ++++++++++-
>  fs/f2fs/gc.c      |  1 +
>  fs/f2fs/segment.c |  1 +
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 02400b0..183a426 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1346,21 +1346,29 @@ int do_write_data_page(struct f2fs_io_info *fio)
>  	struct page *page = fio->page;
>  	struct inode *inode = page->mapping->host;
>  	struct dnode_of_data dn;
> +	struct extent_info ei = {0,0,0};
>  	int err = 0;
>  
>  	set_new_dnode(&dn, inode, NULL, NULL, 0);
> +	if (need_inplace_update(fio) &&
> +			f2fs_lookup_extent_cache(inode, page->index, &ei)) {
> +		fio->old_blkaddr = ei.blk + page->index - ei.fofs;
> +		if (fio->old_blkaddr != NULL_ADDR &&
> +				fio->old_blkaddr != NEW_ADDR)

			ipu_force = true;

I added one variable to do below ipu explicitly.

Thanks,

> +			goto got_it;
> +	}
>  	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
>  	if (err)
>  		return err;
>  
>  	fio->old_blkaddr = dn.data_blkaddr;
> -
>  	/* This page is already truncated */
>  	if (fio->old_blkaddr == NULL_ADDR) {
>  		ClearPageUptodate(page);
>  		goto out_writepage;
>  	}
>  
> +got_it:
>  	err = encrypt_one_page(fio);
>  	if (err)
>  		goto out_writepage;
> @@ -1408,6 +1416,7 @@ static int __write_data_page(struct page *page, bool *submitted,
>  		.type = DATA,
>  		.op = REQ_OP_WRITE,
>  		.op_flags = wbc_to_write_flags(wbc),
> +		.old_blkaddr = NULL_ADDR,
>  		.page = page,
>  		.encrypted_page = NULL,
>  		.submitted = false,
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 499a43f..e034857 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -712,6 +712,7 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  			.type = DATA,
>  			.op = REQ_OP_WRITE,
>  			.op_flags = REQ_SYNC,
> +			.old_blkaddr = NULL_ADDR,
>  			.page = page,
>  			.encrypted_page = NULL,
>  		};
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 2a95535..9f86b98 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -291,6 +291,7 @@ static int __commit_inmem_pages(struct inode *inode,
>  		.type = DATA,
>  		.op = REQ_OP_WRITE,
>  		.op_flags = REQ_SYNC | REQ_PRIO,
> +		.old_blkaddr = NULL_ADDR,
>  		.encrypted_page = NULL,
>  	};
>  	pgoff_t last_idx = ULONG_MAX;
> -- 
> 2.10.1
> 
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH 3/4] f2fs: check IPU before cp to avoid IPU being blocked by cp

[Hou Pengyang]

IPU checking is under f2fs_lock_op, as a result, some IPU page(such as fsync/fdatasync IPU)
may be blocked by a long time cp.

This patch fix this by doing IPU checking before f2fs_lock_op, so fsync IPU could go along with cp.

Suggested-by: He Yunlei <heyunlei@huawei.com>
Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
---
 fs/f2fs/data.c    | 14 +++++++-------
 fs/f2fs/gc.c      |  1 +
 fs/f2fs/segment.c |  1 +
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 183a426..9bf9c7d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1380,19 +1380,20 @@ int do_write_data_page(struct f2fs_io_info *fio)
 	 * it had better in-place writes for updated data.
 	 */
 	if (need_inplace_update(fio)) {
-		f2fs_bug_on(fio->sbi, !fio->cp_rwsem_locked);
-		f2fs_unlock_op(fio->sbi);
-		fio->cp_rwsem_locked = false;
-
+		f2fs_bug_on(fio->sbi, fio->cp_rwsem_locked);
 		err = rewrite_data_page(fio);
 		trace_f2fs_do_write_data_page(fio->page, IPU);
 		set_inode_flag(inode, FI_UPDATE_WRITE);
 	} else {
+		if (!fio->cp_rwsem_locked)
+			f2fs_lock_op(fio->sbi);
 		write_data_page(&dn, fio);
 		trace_f2fs_do_write_data_page(page, OPU);
 		set_inode_flag(inode, FI_APPEND_WRITE);
 		if (page->index == 0)
 			set_inode_flag(inode, FI_FIRST_BLOCK_WRITTEN);
+		if (!fio->cp_rwsem_locked)
+			f2fs_unlock_op(fio->sbi);
 	}
 out_writepage:
 	f2fs_put_dnode(&dn);
@@ -1473,13 +1474,12 @@ static int __write_data_page(struct page *page, bool *submitted,
 		if (!err)
 			goto out;
 	}
-	f2fs_lock_op(sbi);
+
+	fio.cp_rwsem_locked = false;
 	if (err == -EAGAIN)
 		err = do_write_data_page(&fio);
 	if (F2FS_I(inode)->last_disk_size < psize)
 		F2FS_I(inode)->last_disk_size = psize;
-	if (fio.cp_rwsem_locked)
-		f2fs_unlock_op(sbi);
 done:
 	if (err && err != -ENOENT)
 		goto redirty_out;
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index e034857..b32cc30 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -715,6 +715,7 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
 			.old_blkaddr = NULL_ADDR,
 			.page = page,
 			.encrypted_page = NULL,
+			.cp_rwsem_locked = true,
 		};
 		bool is_dirty = PageDirty(page);
 		int err;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 9f86b98..463a77b 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -293,6 +293,7 @@ static int __commit_inmem_pages(struct inode *inode,
 		.op_flags = REQ_SYNC | REQ_PRIO,
 		.old_blkaddr = NULL_ADDR,
 		.encrypted_page = NULL,
+		.cp_rwsem_locked = true,
 	};
 	pgoff_t last_idx = ULONG_MAX;
 	int err = 0;
-- 
2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 3/4] f2fs: check IPU before cp to avoid IPU being blocked by cp

[Jaegeuk Kim]

Hi Pengyang,

This makes xfstests/generic/013 stuck on fsstress.

Thanks,

On 04/25, Hou Pengyang wrote:
> IPU checking is under f2fs_lock_op, as a result, some IPU page(such as fsync/fdatasync IPU)
> may be blocked by a long time cp.
> 
> This patch fix this by doing IPU checking before f2fs_lock_op, so fsync IPU could go along with cp.
> 
> Suggested-by: He Yunlei <heyunlei@huawei.com>
> Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
> ---
>  fs/f2fs/data.c    | 14 +++++++-------
>  fs/f2fs/gc.c      |  1 +
>  fs/f2fs/segment.c |  1 +
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 183a426..9bf9c7d 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1380,19 +1380,20 @@ int do_write_data_page(struct f2fs_io_info *fio)
>  	 * it had better in-place writes for updated data.
>  	 */
>  	if (need_inplace_update(fio)) {
> -		f2fs_bug_on(fio->sbi, !fio->cp_rwsem_locked);
> -		f2fs_unlock_op(fio->sbi);
> -		fio->cp_rwsem_locked = false;
> -
> +		f2fs_bug_on(fio->sbi, fio->cp_rwsem_locked);
>  		err = rewrite_data_page(fio);
>  		trace_f2fs_do_write_data_page(fio->page, IPU);
>  		set_inode_flag(inode, FI_UPDATE_WRITE);
>  	} else {
> +		if (!fio->cp_rwsem_locked)
> +			f2fs_lock_op(fio->sbi);
>  		write_data_page(&dn, fio);
>  		trace_f2fs_do_write_data_page(page, OPU);
>  		set_inode_flag(inode, FI_APPEND_WRITE);
>  		if (page->index == 0)
>  			set_inode_flag(inode, FI_FIRST_BLOCK_WRITTEN);
> +		if (!fio->cp_rwsem_locked)
> +			f2fs_unlock_op(fio->sbi);
>  	}
>  out_writepage:
>  	f2fs_put_dnode(&dn);
> @@ -1473,13 +1474,12 @@ static int __write_data_page(struct page *page, bool *submitted,
>  		if (!err)
>  			goto out;
>  	}
> -	f2fs_lock_op(sbi);
> +
> +	fio.cp_rwsem_locked = false;
>  	if (err == -EAGAIN)
>  		err = do_write_data_page(&fio);
>  	if (F2FS_I(inode)->last_disk_size < psize)
>  		F2FS_I(inode)->last_disk_size = psize;
> -	if (fio.cp_rwsem_locked)
> -		f2fs_unlock_op(sbi);
>  done:
>  	if (err && err != -ENOENT)
>  		goto redirty_out;
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index e034857..b32cc30 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -715,6 +715,7 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  			.old_blkaddr = NULL_ADDR,
>  			.page = page,
>  			.encrypted_page = NULL,
> +			.cp_rwsem_locked = true,
>  		};
>  		bool is_dirty = PageDirty(page);
>  		int err;
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 9f86b98..463a77b 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -293,6 +293,7 @@ static int __commit_inmem_pages(struct inode *inode,
>  		.op_flags = REQ_SYNC | REQ_PRIO,
>  		.old_blkaddr = NULL_ADDR,
>  		.encrypted_page = NULL,
> +		.cp_rwsem_locked = true,
>  	};
>  	pgoff_t last_idx = ULONG_MAX;
>  	int err = 0;
> -- 
> 2.10.1
> 
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 3/4] f2fs: check IPU before cp to avoid IPU being blocked by cp

[Jaegeuk Kim]

On 04/25, Jaegeuk Kim wrote:
> Hi Pengyang,
> 
> This makes xfstests/generic/013 stuck on fsstress.

Oh, it seems lock inversion problem between f2fs_lock_op and get_dnode_of_data.
The basic rule is get_dnode_of_data is covered by f2fs_lock_op.
I'll drop this patch at this moment.

Thanks,

> 
> Thanks,
> 
> On 04/25, Hou Pengyang wrote:
> > IPU checking is under f2fs_lock_op, as a result, some IPU page(such as fsync/fdatasync IPU)
> > may be blocked by a long time cp.
> > 
> > This patch fix this by doing IPU checking before f2fs_lock_op, so fsync IPU could go along with cp.
> > 
> > Suggested-by: He Yunlei <heyunlei@huawei.com>
> > Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
> > ---
> >  fs/f2fs/data.c    | 14 +++++++-------
> >  fs/f2fs/gc.c      |  1 +
> >  fs/f2fs/segment.c |  1 +
> >  3 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 183a426..9bf9c7d 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1380,19 +1380,20 @@ int do_write_data_page(struct f2fs_io_info *fio)
> >  	 * it had better in-place writes for updated data.
> >  	 */
> >  	if (need_inplace_update(fio)) {
> > -		f2fs_bug_on(fio->sbi, !fio->cp_rwsem_locked);
> > -		f2fs_unlock_op(fio->sbi);
> > -		fio->cp_rwsem_locked = false;
> > -
> > +		f2fs_bug_on(fio->sbi, fio->cp_rwsem_locked);
> >  		err = rewrite_data_page(fio);
> >  		trace_f2fs_do_write_data_page(fio->page, IPU);
> >  		set_inode_flag(inode, FI_UPDATE_WRITE);
> >  	} else {
> > +		if (!fio->cp_rwsem_locked)
> > +			f2fs_lock_op(fio->sbi);
> >  		write_data_page(&dn, fio);
> >  		trace_f2fs_do_write_data_page(page, OPU);
> >  		set_inode_flag(inode, FI_APPEND_WRITE);
> >  		if (page->index == 0)
> >  			set_inode_flag(inode, FI_FIRST_BLOCK_WRITTEN);
> > +		if (!fio->cp_rwsem_locked)
> > +			f2fs_unlock_op(fio->sbi);
> >  	}
> >  out_writepage:
> >  	f2fs_put_dnode(&dn);
> > @@ -1473,13 +1474,12 @@ static int __write_data_page(struct page *page, bool *submitted,
> >  		if (!err)
> >  			goto out;
> >  	}
> > -	f2fs_lock_op(sbi);
> > +
> > +	fio.cp_rwsem_locked = false;
> >  	if (err == -EAGAIN)
> >  		err = do_write_data_page(&fio);
> >  	if (F2FS_I(inode)->last_disk_size < psize)
> >  		F2FS_I(inode)->last_disk_size = psize;
> > -	if (fio.cp_rwsem_locked)
> > -		f2fs_unlock_op(sbi);
> >  done:
> >  	if (err && err != -ENOENT)
> >  		goto redirty_out;
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index e034857..b32cc30 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -715,6 +715,7 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
> >  			.old_blkaddr = NULL_ADDR,
> >  			.page = page,
> >  			.encrypted_page = NULL,
> > +			.cp_rwsem_locked = true,
> >  		};
> >  		bool is_dirty = PageDirty(page);
> >  		int err;
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 9f86b98..463a77b 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -293,6 +293,7 @@ static int __commit_inmem_pages(struct inode *inode,
> >  		.op_flags = REQ_SYNC | REQ_PRIO,
> >  		.old_blkaddr = NULL_ADDR,
> >  		.encrypted_page = NULL,
> > +		.cp_rwsem_locked = true,
> >  	};
> >  	pgoff_t last_idx = ULONG_MAX;
> >  	int err = 0;
> > -- 
> > 2.10.1
> > 
> > 
> > ------------------------------------------------------------------------------
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 3/4] f2fs: check IPU before cp to avoid IPU being blocked by cp

[Chao Yu]

Hi Jaegeuk, Pengyang,

On 2017/4/26 9:27, Jaegeuk Kim wrote:
> On 04/25, Jaegeuk Kim wrote:
>> Hi Pengyang,
>>
>> This makes xfstests/generic/013 stuck on fsstress.
> 
> Oh, it seems lock inversion problem between f2fs_lock_op and get_dnode_of_data.
> The basic rule is get_dnode_of_data is covered by f2fs_lock_op.
> I'll drop this patch at this moment.

Seems we need to change as below?

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9ec9eace05df..f3564a948293 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1364,12 +1364,17 @@ int do_write_data_page(struct f2fs_io_info *fio)

 		if (valid_ipu_blkaddr(fio)) {
 			ipu_force = true;
+			fio->need_lock = false;
 			goto got_it;
 		}
 	}
+
+	if (fio->need_lock)
+		f2fs_lock_op(fio->sbi);
+
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
-		return err;
+		goto out;

 	fio->old_blkaddr = dn.data_blkaddr;

@@ -1400,17 +1405,16 @@ int do_write_data_page(struct f2fs_io_info *fio)
 	}

 	/* LFS mode write path */
-	if (!fio->cp_rwsem_locked)
-		f2fs_lock_op(fio->sbi);
 	write_data_page(&dn, fio);
 	trace_f2fs_do_write_data_page(page, OPU);
 	set_inode_flag(inode, FI_APPEND_WRITE);
 	if (page->index == 0)
 		set_inode_flag(inode, FI_FIRST_BLOCK_WRITTEN);
-	if (!fio->cp_rwsem_locked)
-		f2fs_unlock_op(fio->sbi);
 out_writepage:
 	f2fs_put_dnode(&dn);
+out:
+	if (fio->need_lock)
+		f2fs_unlock_op(fio->sbi);
 	return err;
 }

@@ -1435,7 +1439,7 @@ static int __write_data_page(struct page *page, bool *submitted,
 		.page = page,
 		.encrypted_page = NULL,
 		.submitted = false,
-		.cp_rwsem_locked = true,
+		.need_lock = true,
 	};

 	trace_f2fs_writepage(page, DATA);
@@ -1471,6 +1475,7 @@ static int __write_data_page(struct page *page, bool *submitted,

 	/* Dentry blocks are controlled by checkpoint */
 	if (S_ISDIR(inode->i_mode)) {
+		fio.need_lock = false,
 		err = do_write_data_page(&fio);
 		goto done;
 	}
@@ -1488,13 +1493,12 @@ static int __write_data_page(struct page *page, bool *submitted,
 		if (!err)
 			goto out;
 	}
-	f2fs_lock_op(sbi);
+
 	if (err == -EAGAIN)
 		err = do_write_data_page(&fio);
 	if (F2FS_I(inode)->last_disk_size < psize)
 		F2FS_I(inode)->last_disk_size = psize;
-	if (fio.cp_rwsem_locked)
-		f2fs_unlock_op(sbi);
+
 done:
 	if (err && err != -ENOENT)
 		goto redirty_out;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index bf48213642ab..95fb347e7abe 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -802,7 +802,7 @@ struct f2fs_io_info {
 	struct page *page;	/* page to be written */
 	struct page *encrypted_page;	/* encrypted page */
 	bool submitted;		/* indicate IO submission */
-	bool cp_rwsem_locked;	/* indicate cp_rwsem is held */
+	bool need_lock;		/* indicate we need to lock cp_rwsem */
 };

 #define is_read_io(rw) ((rw) == READ)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index c2a9ae8397d3..ffabdcf52b85 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -717,6 +717,7 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
 			.old_blkaddr = NULL_ADDR,
 			.page = page,
 			.encrypted_page = NULL,
+			.need_lock = false,
 		};
 		bool is_dirty = PageDirty(page);
 		int err;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index ee9b56da0b7e..c155adbac8fc 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -293,6 +293,7 @@ static int __commit_inmem_pages(struct inode *inode,
 		.op_flags = REQ_SYNC | REQ_PRIO,
 		.old_blkaddr = NULL_ADDR,
 		.encrypted_page = NULL,
+		.need_lock = false,
 	};
 	pgoff_t last_idx = ULONG_MAX;
 	int err = 0;


> 
> Thanks,
> 
>>
>> Thanks,
>>
>> On 04/25, Hou Pengyang wrote:
>>> IPU checking is under f2fs_lock_op, as a result, some IPU page(such as fsync/fdatasync IPU)
>>> may be blocked by a long time cp.
>>>
>>> This patch fix this by doing IPU checking before f2fs_lock_op, so fsync IPU could go along with cp.
>>>
>>> Suggested-by: He Yunlei <heyunlei@huawei.com>
>>> Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
>>> ---
>>>  fs/f2fs/data.c    | 14 +++++++-------
>>>  fs/f2fs/gc.c      |  1 +
>>>  fs/f2fs/segment.c |  1 +
>>>  3 files changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 183a426..9bf9c7d 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -1380,19 +1380,20 @@ int do_write_data_page(struct f2fs_io_info *fio)
>>>  	 * it had better in-place writes for updated data.
>>>  	 */
>>>  	if (need_inplace_update(fio)) {
>>> -		f2fs_bug_on(fio->sbi, !fio->cp_rwsem_locked);
>>> -		f2fs_unlock_op(fio->sbi);
>>> -		fio->cp_rwsem_locked = false;
>>> -
>>> +		f2fs_bug_on(fio->sbi, fio->cp_rwsem_locked);
>>>  		err = rewrite_data_page(fio);
>>>  		trace_f2fs_do_write_data_page(fio->page, IPU);
>>>  		set_inode_flag(inode, FI_UPDATE_WRITE);
>>>  	} else {
>>> +		if (!fio->cp_rwsem_locked)
>>> +			f2fs_lock_op(fio->sbi);
>>>  		write_data_page(&dn, fio);
>>>  		trace_f2fs_do_write_data_page(page, OPU);
>>>  		set_inode_flag(inode, FI_APPEND_WRITE);
>>>  		if (page->index == 0)
>>>  			set_inode_flag(inode, FI_FIRST_BLOCK_WRITTEN);
>>> +		if (!fio->cp_rwsem_locked)
>>> +			f2fs_unlock_op(fio->sbi);
>>>  	}
>>>  out_writepage:
>>>  	f2fs_put_dnode(&dn);
>>> @@ -1473,13 +1474,12 @@ static int __write_data_page(struct page *page, bool *submitted,
>>>  		if (!err)
>>>  			goto out;
>>>  	}
>>> -	f2fs_lock_op(sbi);
>>> +
>>> +	fio.cp_rwsem_locked = false;
>>>  	if (err == -EAGAIN)
>>>  		err = do_write_data_page(&fio);
>>>  	if (F2FS_I(inode)->last_disk_size < psize)
>>>  		F2FS_I(inode)->last_disk_size = psize;
>>> -	if (fio.cp_rwsem_locked)
>>> -		f2fs_unlock_op(sbi);
>>>  done:
>>>  	if (err && err != -ENOENT)
>>>  		goto redirty_out;
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index e034857..b32cc30 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -715,6 +715,7 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>  			.old_blkaddr = NULL_ADDR,
>>>  			.page = page,
>>>  			.encrypted_page = NULL,
>>> +			.cp_rwsem_locked = true,
>>>  		};
>>>  		bool is_dirty = PageDirty(page);
>>>  		int err;
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 9f86b98..463a77b 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -293,6 +293,7 @@ static int __commit_inmem_pages(struct inode *inode,
>>>  		.op_flags = REQ_SYNC | REQ_PRIO,
>>>  		.old_blkaddr = NULL_ADDR,
>>>  		.encrypted_page = NULL,
>>> +		.cp_rwsem_locked = true,
>>>  	};
>>>  	pgoff_t last_idx = ULONG_MAX;
>>>  	int err = 0;
>>> -- 
>>> 2.10.1
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Check out the vibrant tech community on one of the world's most
>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>> ------------------------------------------------------------------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH 4/4] f2fs: unlock dnode before IPU

[Hou Pengyang]

Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
---
 fs/f2fs/data.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9bf9c7d..1d7157b 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1381,9 +1381,11 @@ int do_write_data_page(struct f2fs_io_info *fio)
 	 */
 	if (need_inplace_update(fio)) {
 		f2fs_bug_on(fio->sbi, fio->cp_rwsem_locked);
+		f2fs_put_dnode(&dn);
 		err = rewrite_data_page(fio);
 		trace_f2fs_do_write_data_page(fio->page, IPU);
 		set_inode_flag(inode, FI_UPDATE_WRITE);
+		return err;
 	} else {
 		if (!fio->cp_rwsem_locked)
 			f2fs_lock_op(fio->sbi);
-- 
2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 4/4] f2fs: unlock dnode before IPU

[Jaegeuk Kim]

On 04/25, Hou Pengyang wrote:
> Signed-off-by: Hou Pengyang <houpengyang@huawei.com>
> ---
>  fs/f2fs/data.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 9bf9c7d..1d7157b 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1381,9 +1381,11 @@ int do_write_data_page(struct f2fs_io_info *fio)
>  	 */
>  	if (need_inplace_update(fio)) {
>  		f2fs_bug_on(fio->sbi, fio->cp_rwsem_locked);
> +		f2fs_put_dnode(&dn);
>  		err = rewrite_data_page(fio);
>  		trace_f2fs_do_write_data_page(fio->page, IPU);
>  		set_inode_flag(inode, FI_UPDATE_WRITE);
> +		return err;
>  	} else {

We don't need else phrase here.
I changed and merged it.

Thanks,

>  		if (!fio->cp_rwsem_locked)
>  			f2fs_lock_op(fio->sbi);
> -- 
> 2.10.1
> 
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot