[PATCH 1/3] f2fs: fix to wait page writeback during revoking atomic write

[PATCH 1/3] f2fs: fix to wait page writeback during revoking atomic write

[Chao Yu]

After revoking atomic write, related LBA can be reused by others, so we
need to wait page writeback before reusing the LBA, in order to avoid
interference between old atomic written in-flight IO and new IO.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index cba59fc58eb6..24299f81f80d 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -231,6 +231,8 @@ static int __revoke_inmem_pages(struct inode *inode,
 
 		lock_page(page);
 
+		f2fs_wait_on_page_writeback(page, DATA, true);
+
 		if (recover) {
 			struct dnode_of_data dn;
 			struct node_info ni;
-- 
2.15.0.55.gc2ece9dc4de6

[PATCH 2/3] f2fs: clean up commit_inmem_pages()

[Chao Yu]

This patch moves error handling from commit_inmem_pages() into
__commit_inmem_page() for cleanup, no logic change.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/segment.c | 46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 24299f81f80d..dcb4fcb8b1b0 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -347,8 +347,7 @@ void drop_inmem_page(struct inode *inode, struct page *page)
 	trace_f2fs_commit_inmem_page(page, INMEM_INVALIDATE);
 }
 
-static int __commit_inmem_pages(struct inode *inode,
-					struct list_head *revoke_list)
+static int __commit_inmem_pages(struct inode *inode)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct f2fs_inode_info *fi = F2FS_I(inode);
@@ -361,9 +360,12 @@ static int __commit_inmem_pages(struct inode *inode,
 		.op_flags = REQ_SYNC | REQ_PRIO,
 		.io_type = FS_DATA_IO,
 	};
+	struct list_head revoke_list;
 	pgoff_t last_idx = ULONG_MAX;
 	int err = 0;
 
+	INIT_LIST_HEAD(&revoke_list);
+
 	list_for_each_entry_safe(cur, tmp, &fi->inmem_pages, list) {
 		struct page *page = cur->page;
 
@@ -397,14 +399,28 @@ static int __commit_inmem_pages(struct inode *inode,
 			last_idx = page->index;
 		}
 		unlock_page(page);
-		list_move_tail(&cur->list, revoke_list);
+		list_move_tail(&cur->list, &revoke_list);
 	}
 
 	if (last_idx != ULONG_MAX)
 		f2fs_submit_merged_write_cond(sbi, inode, 0, last_idx, DATA);
 
-	if (!err)
-		__revoke_inmem_pages(inode, revoke_list, false, false);
+	if (err) {
+		/*
+		 * try to revoke all committed pages, but still we could fail
+		 * due to no memory or other reason, if that happened, EAGAIN
+		 * will be returned, which means in such case, transaction is
+		 * already not integrity, caller should use journal to do the
+		 * recovery or rewrite & commit last transaction. For other
+		 * error number, revoking was done by filesystem itself.
+		 */
+		err = __revoke_inmem_pages(inode, &revoke_list, false, true);
+
+		/* drop all uncommitted pages */
+		__revoke_inmem_pages(inode, &fi->inmem_pages, true, false);
+	} else {
+		__revoke_inmem_pages(inode, &revoke_list, false, false);
+	}
 
 	return err;
 }
@@ -413,34 +429,16 @@ int commit_inmem_pages(struct inode *inode)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct f2fs_inode_info *fi = F2FS_I(inode);
-	struct list_head revoke_list;
 	int err;
 
-	INIT_LIST_HEAD(&revoke_list);
 	f2fs_balance_fs(sbi, true);
 	f2fs_lock_op(sbi);
 
 	set_inode_flag(inode, FI_ATOMIC_COMMIT);
 
 	mutex_lock(&fi->inmem_lock);
-	err = __commit_inmem_pages(inode, &revoke_list);
-	if (err) {
-		int ret;
-		/*
-		 * try to revoke all committed pages, but still we could fail
-		 * due to no memory or other reason, if that happened, EAGAIN
-		 * will be returned, which means in such case, transaction is
-		 * already not integrity, caller should use journal to do the
-		 * recovery or rewrite & commit last transaction. For other
-		 * error number, revoking was done by filesystem itself.
-		 */
-		ret = __revoke_inmem_pages(inode, &revoke_list, false, true);
-		if (ret)
-			err = ret;
+	err = __commit_inmem_pages(inode);
 
-		/* drop all uncommitted pages */
-		__revoke_inmem_pages(inode, &fi->inmem_pages, true, false);
-	}
 	spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
 	if (!list_empty(&fi->inmem_ilist))
 		list_del_init(&fi->inmem_ilist);
-- 
2.15.0.55.gc2ece9dc4de6

[PATCH 3/3] f2fs: wait IO writeback in commit_inmem_pages()

[Chao Yu]

Thread A				Thread B
- f2fs_ioc_commit_atomic_write
 - commit_inmem_pages
  - f2fs_submit_merged_write_cond
  : write data
					- write_checkpoint
					 - do_checkpoint
					 : commit all node within CP
					 -> SPO
  - f2fs_do_sync_file
   - file_write_and_wait_range
   : wait data writeback

In above race condition, data/node can be flushed in reversed order when
coming a checkpoint before f2fs_do_sync_file, after SPOR, it results in
atomic written data being corrupted.

This patch adds filemap_fdatawait() in commit_inmem_pages() to keep data
and node of atomic file being flushed orderly.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/file.c    | 21 ++++++++++++---------
 fs/f2fs/segment.c |  2 ++
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5d710f7f2684..4ea418cbd189 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -217,15 +217,18 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 
 	trace_f2fs_sync_file_enter(inode);
 
-	/* if fdatasync is triggered, let's do in-place-update */
-	if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)
-		set_inode_flag(inode, FI_NEED_IPU);
-	ret = file_write_and_wait_range(file, start, end);
-	clear_inode_flag(inode, FI_NEED_IPU);
-
-	if (ret) {
-		trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
-		return ret;
+	if (!atomic) {
+		/* if fdatasync is triggered, let's do in-place-update */
+		if (datasync || get_dirty_pages(inode) <=
+					SM_I(sbi)->min_fsync_blocks)
+			set_inode_flag(inode, FI_NEED_IPU);
+		ret = file_write_and_wait_range(file, start, end);
+		clear_inode_flag(inode, FI_NEED_IPU);
+
+		if (ret) {
+			trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
+			return ret;
+		}
 	}
 
 	/* if the inode is dirty, let's recover all the time */
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index dcb4fcb8b1b0..3b66c4aa04a6 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -420,6 +420,8 @@ static int __commit_inmem_pages(struct inode *inode)
 		__revoke_inmem_pages(inode, &fi->inmem_pages, true, false);
 	} else {
 		__revoke_inmem_pages(inode, &revoke_list, false, false);
+
+		err = filemap_fdatawait(inode->i_mapping);
 	}
 
 	return err;
-- 
2.15.0.55.gc2ece9dc4de6

Re: [PATCH 3/3] f2fs: wait IO writeback in commit_inmem_pages()

[Jaegeuk Kim]

On 04/23, Chao Yu wrote:
> Thread A				Thread B
> - f2fs_ioc_commit_atomic_write
>  - commit_inmem_pages
>   - f2fs_submit_merged_write_cond
>   : write data
> 					- write_checkpoint
> 					 - do_checkpoint
> 					 : commit all node within CP
> 					 -> SPO
>   - f2fs_do_sync_file
>    - file_write_and_wait_range
>    : wait data writeback
> 
> In above race condition, data/node can be flushed in reversed order when
> coming a checkpoint before f2fs_do_sync_file, after SPOR, it results in
> atomic written data being corrupted.
> 
> This patch adds filemap_fdatawait() in commit_inmem_pages() to keep data
> and node of atomic file being flushed orderly.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/file.c    | 21 ++++++++++++---------
>  fs/f2fs/segment.c |  2 ++
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 5d710f7f2684..4ea418cbd189 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -217,15 +217,18 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>  
>  	trace_f2fs_sync_file_enter(inode);

if (atomic)
	goto write_done;

>  
> -	/* if fdatasync is triggered, let's do in-place-update */
> -	if (datasync || get_dirty_pages(inode) <= SM_I(sbi)->min_fsync_blocks)
> -		set_inode_flag(inode, FI_NEED_IPU);
> -	ret = file_write_and_wait_range(file, start, end);
> -	clear_inode_flag(inode, FI_NEED_IPU);
> -
> -	if (ret) {
> -		trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
> -		return ret;
> +	if (!atomic) {
> +		/* if fdatasync is triggered, let's do in-place-update */
> +		if (datasync || get_dirty_pages(inode) <=
> +					SM_I(sbi)->min_fsync_blocks)
> +			set_inode_flag(inode, FI_NEED_IPU);
> +		ret = file_write_and_wait_range(file, start, end);
> +		clear_inode_flag(inode, FI_NEED_IPU);
> +
> +		if (ret) {
> +			trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
> +			return ret;
> +		}
>  	}

write_done:

>  
>  	/* if the inode is dirty, let's recover all the time */
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index dcb4fcb8b1b0..3b66c4aa04a6 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -420,6 +420,8 @@ static int __commit_inmem_pages(struct inode *inode)
>  		__revoke_inmem_pages(inode, &fi->inmem_pages, true, false);
>  	} else {
>  		__revoke_inmem_pages(inode, &revoke_list, false, false);
> +
> +		err = filemap_fdatawait(inode->i_mapping);
>  	}
>  
>  	return err;
> -- 
> 2.15.0.55.gc2ece9dc4de6

Re: [PATCH 1/3] f2fs: fix to wait page writeback during revoking atomic write

[Chao Yu]

Hi Jaegeuk,

Could you recheck this patch?

On 2018/4/23 10:36, Chao Yu wrote:
> After revoking atomic write, related LBA can be reused by others, so we
> need to wait page writeback before reusing the LBA, in order to avoid
> interference between old atomic written in-flight IO and new IO.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/segment.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index cba59fc58eb6..24299f81f80d 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -231,6 +231,8 @@ static int __revoke_inmem_pages(struct inode *inode,
>  
>  		lock_page(page);
>  
> +		f2fs_wait_on_page_writeback(page, DATA, true);
> +
>  		if (recover) {
>  			struct dnode_of_data dn;
>  			struct node_info ni;
>