[PATCH 1/4] f2fs: flush journal nat entries for nat_bits during unmount

[PATCH 1/4] f2fs: flush journal nat entries for nat_bits during unmount

[Jaegeuk Kim]

Let's flush journal nat entries for speed up in the next run.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/node.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 29237aeca041..0f076fb0d828 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -2613,6 +2613,13 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	nid_t set_idx = 0;
 	LIST_HEAD(sets);
 
+	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
+	if (enabled_nat_bits(sbi, cpc)) {
+		down_write(&nm_i->nat_tree_lock);
+		remove_nats_in_journal(sbi);
+		up_write(&nm_i->nat_tree_lock);
+	}
+
 	if (!nm_i->dirty_nat_cnt)
 		return;
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 2/4] f2fs: allow wrong configure dio to buffered write

[Jaegeuk Kim]

This fixes to support unaligned dio as buffered writes.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index e66379961804..6e8e78bb64a7 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2425,7 +2425,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 	err = check_direct_IO(inode, iter, offset);
 	if (err)
-		return err;
+		return 0;
 
 	if (f2fs_force_buffered_io(inode, rw))
 		return 0;
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev] [PATCH 2/4] f2fs: allow wrong configure dio to buffered write

[Chao Yu]

On 2018/7/7 5:09, Jaegeuk Kim wrote:
> This fixes to support unaligned dio as buffered writes.

Should we return -EINVAL as manual of write said:

EINVAL fd is attached to an object which is unsuitable for writing; or the file
was opened with the O_DIRECT flag,  and  either  the  address
specified in buf, the value specified in count, or the current file offset is
not suitably aligned.

Thanks,

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index e66379961804..6e8e78bb64a7 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2425,7 +2425,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>  
>  	err = check_direct_IO(inode, iter, offset);
>  	if (err)
> -		return err;
> +		return 0;
>  
>  	if (f2fs_force_buffered_io(inode, rw))
>  		return 0;
> 

Re: [f2fs-dev] [PATCH 2/4] f2fs: allow wrong configure dio to buffered write

[Jaegeuk Kim]

On 07/09, Chao Yu wrote:
> On 2018/7/7 5:09, Jaegeuk Kim wrote:
> > This fixes to support unaligned dio as buffered writes.
> 
> Should we return -EINVAL as manual of write said:
> 
> EINVAL fd is attached to an object which is unsuitable for writing; or the file
> was opened with the O_DIRECT flag,  and  either  the  address
> specified in buf, the value specified in count, or the current file offset is
> not suitably aligned.

I came from many xfstests failures regarding to direct IO error. It seems other
filesystems allow buffered writes as well.

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/data.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index e66379961804..6e8e78bb64a7 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -2425,7 +2425,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> >  
> >  	err = check_direct_IO(inode, iter, offset);
> >  	if (err)
> > -		return err;
> > +		return 0;
> >  
> >  	if (f2fs_force_buffered_io(inode, rw))
> >  		return 0;
> > 

Re: [f2fs-dev] [PATCH 2/4] f2fs: allow wrong configure dio to buffered write

[Chao Yu]

On 2018/7/10 4:41, Jaegeuk Kim wrote:
> On 07/09, Chao Yu wrote:
>> On 2018/7/7 5:09, Jaegeuk Kim wrote:
>>> This fixes to support unaligned dio as buffered writes.
>>
>> Should we return -EINVAL as manual of write said:
>>
>> EINVAL fd is attached to an object which is unsuitable for writing; or the file
>> was opened with the O_DIRECT flag,  and  either  the  address
>> specified in buf, the value specified in count, or the current file offset is
>> not suitably aligned.
> 
> I came from many xfstests failures regarding to direct IO error. It seems other
> filesystems allow buffered writes as well.

But as I see, it allows unaligned DIO write like xfs_io -d -c "pwrite 1 512",
where ext4 returns failure.

I guess, we only need to expand to support DIO write aligns to sector (512
bytes) for these cases?

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/data.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index e66379961804..6e8e78bb64a7 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -2425,7 +2425,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>>>  
>>>  	err = check_direct_IO(inode, iter, offset);
>>>  	if (err)
>>> -		return err;
>>> +		return 0;
>>>  
>>>  	if (f2fs_force_buffered_io(inode, rw))
>>>  		return 0;
>>>
> 
> 

Re: [f2fs-dev v2] [PATCH 2/4] f2fs: allow wrong configure dio to buffered write

[Jaegeuk Kim]

This fixes to support dio having unaligned buffers as buffered writes.

xfs_io -f -d -c "pwrite 0 512" $testfile
 -> okay

xfs_io -f -d -c "pwrite 1 512" $testfile
 -> EINVAL

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
v2 from v1:
 - return error depending on sector size

 fs/f2fs/data.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 8f931d699287..5e53d210e222 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2371,14 +2371,20 @@ static int f2fs_write_end(struct file *file,
 static int check_direct_IO(struct inode *inode, struct iov_iter *iter,
 			   loff_t offset)
 {
-	unsigned blocksize_mask = inode->i_sb->s_blocksize - 1;
-
-	if (offset & blocksize_mask)
-		return -EINVAL;
-
-	if (iov_iter_alignment(iter) & blocksize_mask)
-		return -EINVAL;
-
+	unsigned i_blkbits = READ_ONCE(inode->i_blkbits);
+	unsigned blkbits = i_blkbits;
+	unsigned blocksize_mask = (1 << blkbits) - 1;
+	unsigned long align = offset | iov_iter_alignment(iter);
+	struct block_device *bdev = inode->i_sb->s_bdev;
+
+	if (align & blocksize_mask) {
+		if (bdev)
+			blkbits = blksize_bits(bdev_logical_block_size(bdev));
+		blocksize_mask = (1 << blkbits) - 1;
+		if (align & blocksize_mask)
+			return -EINVAL;
+		return 1;
+	}
 	return 0;
 }
 
@@ -2396,7 +2402,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 	err = check_direct_IO(inode, iter, offset);
 	if (err)
-		return err;
+		return err < 0 ? err : 0;
 
 	if (f2fs_force_buffered_io(inode, rw))
 		return 0;
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev v2] [PATCH 2/4] f2fs: allow wrong configure dio to buffered write

[Chao Yu]

On 2018/7/13 1:59, Jaegeuk Kim wrote:
> This fixes to support dio having unaligned buffers as buffered writes.
> 
> xfs_io -f -d -c "pwrite 0 512" $testfile
>  -> okay
> 
> xfs_io -f -d -c "pwrite 1 512" $testfile
>  -> EINVAL
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

[PATCH 3/4] f2fs: indicate shutdown f2fs to allow unmount successfully

[Jaegeuk Kim]

Once we shutdown f2fs, we have to flush stale pages in order to unmount
the system. In order to make stable, we need to stop fault injection as well.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c |  1 +
 fs/f2fs/data.c       |  4 ++++
 fs/f2fs/f2fs.h       |  7 +++++++
 fs/f2fs/file.c       |  4 ++++
 fs/f2fs/inode.c      |  3 +++
 fs/f2fs/node.c       | 11 +++++++++--
 fs/f2fs/super.c      |  5 +----
 7 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index ee10b40309a1..8b698bd54490 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -28,6 +28,7 @@ struct kmem_cache *f2fs_inode_entry_slab;
 
 void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi, bool end_io)
 {
+	f2fs_build_fault_attr(sbi, 0);
 	set_ckpt_flags(sbi, CP_ERROR_FLAG);
 	if (!end_io)
 		f2fs_flush_merged_writes(sbi);
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 6e8e78bb64a7..c3f9b9baf140 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1829,6 +1829,10 @@ static int __write_data_page(struct page *page, bool *submitted,
 	/* we should bypass data pages to proceed the kworkder jobs */
 	if (unlikely(f2fs_cp_error(sbi))) {
 		mapping_set_error(page->mapping, -EIO);
+
+		if (is_sbi_flag_set(sbi, SBI_IS_SHUTDOWN))
+			goto out;
+
 		/*
 		 * don't drop any dirty dentry pages for keeping lastest
 		 * directory structure.
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 3cde41fb40a1..cc1ad4c8103c 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1069,6 +1069,7 @@ enum {
 	SBI_POR_DOING,				/* recovery is doing or not */
 	SBI_NEED_SB_WRITE,			/* need to recover superblock */
 	SBI_NEED_CP,				/* need to checkpoint */
+	SBI_IS_SHUTDOWN,			/* shutdown by ioctl */
 };
 
 enum {
@@ -3398,4 +3399,10 @@ static inline bool f2fs_force_buffered_io(struct inode *inode, int rw)
 			F2FS_I_SB(inode)->s_ndevs);
 }
 
+#ifdef CONFIG_F2FS_FAULT_INJECTION
+extern void f2fs_build_fault_attr(struct f2fs_sb_info *sbi, unsigned int rate);
+#else
+#define f2fs_build_fault_attr(sbi, rate)		do { } while (0)
+#endif
+
 #endif
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e8dfda4b4fcb..5e29d4053748 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1902,6 +1902,7 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 		}
 		if (sb) {
 			f2fs_stop_checkpoint(sbi, false);
+			set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
 			thaw_bdev(sb->s_bdev, sb);
 		}
 		break;
@@ -1911,13 +1912,16 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 		if (ret)
 			goto out;
 		f2fs_stop_checkpoint(sbi, false);
+		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
 		break;
 	case F2FS_GOING_DOWN_NOSYNC:
 		f2fs_stop_checkpoint(sbi, false);
+		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
 		break;
 	case F2FS_GOING_DOWN_METAFLUSH:
 		f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_META_IO);
 		f2fs_stop_checkpoint(sbi, false);
+		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 4c5e354a572c..8211f5c288a1 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -160,6 +160,9 @@ bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page)
 	struct f2fs_inode *ri;
 	__u32 provided, calculated;
 
+	if (unlikely(is_sbi_flag_set(sbi, SBI_IS_SHUTDOWN)))
+		return true;
+
 #ifdef CONFIG_F2FS_CHECK_FS
 	if (!f2fs_enable_inode_chksum(sbi, page))
 #else
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 0f076fb0d828..7293ea65f343 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1150,7 +1150,8 @@ static int read_node_page(struct page *page, int op_flags)
 
 	f2fs_get_node_info(sbi, page->index, &ni);
 
-	if (unlikely(ni.blk_addr == NULL_ADDR)) {
+	if (unlikely(ni.blk_addr == NULL_ADDR) ||
+			is_sbi_flag_set(sbi, SBI_IS_SHUTDOWN)) {
 		ClearPageUptodate(page);
 		return -ENOENT;
 	}
@@ -1372,8 +1373,14 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
 
 	trace_f2fs_writepage(page, NODE);
 
-	if (unlikely(f2fs_cp_error(sbi)))
+	if (unlikely(f2fs_cp_error(sbi))) {
+		if (is_sbi_flag_set(sbi, SBI_IS_SHUTDOWN)) {
+			dec_page_count(sbi, F2FS_DIRTY_NODES);
+			unlock_page(page);
+			return 0;
+		}
 		goto redirty_out;
+	}
 
 	if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
 		goto redirty_out;
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 383e3bff7c22..5562ff05c14e 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -57,8 +57,7 @@ char *fault_name[FAULT_MAX] = {
 	[FAULT_CHECKPOINT]	= "checkpoint error",
 };
 
-static void f2fs_build_fault_attr(struct f2fs_sb_info *sbi,
-						unsigned int rate)
+void f2fs_build_fault_attr(struct f2fs_sb_info *sbi, unsigned int rate)
 {
 	struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;
 
@@ -1380,9 +1379,7 @@ static void default_options(struct f2fs_sb_info *sbi)
 	set_opt(sbi, POSIX_ACL);
 #endif
 
-#ifdef CONFIG_F2FS_FAULT_INJECTION
 	f2fs_build_fault_attr(sbi, 0);
-#endif
 }
 
 #ifdef CONFIG_QUOTA
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev] [PATCH 3/4] f2fs: indicate shutdown f2fs to allow unmount successfully

[Chao Yu]

On 2018/7/7 5:09, Jaegeuk Kim wrote:
> Once we shutdown f2fs, we have to flush stale pages in order to unmount
> the system. In order to make stable, we need to stop fault injection as well.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

[PATCH 4/4] f2fs: should check the type of end_io after pullback by fscrypto

[Jaegeuk Kim]

This fixes to get the page type after pullback by fscrypto, since the original
page is bounce page but we should need to get control page which is what we
originally submitted.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index c3f9b9baf140..9a0fcec53d0e 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -152,7 +152,6 @@ static void f2fs_write_end_io(struct bio *bio)
 
 	bio_for_each_segment_all(bvec, bio, i) {
 		struct page *page = bvec->bv_page;
-		enum count_type type = WB_DATA_TYPE(page);
 
 		if (IS_DUMMY_WRITTEN_PAGE(page)) {
 			set_page_private(page, (unsigned long)NULL);
@@ -169,14 +168,14 @@ static void f2fs_write_end_io(struct bio *bio)
 
 		if (unlikely(bio->bi_status)) {
 			mapping_set_error(page->mapping, -EIO);
-			if (type == F2FS_WB_CP_DATA)
+			if (WB_DATA_TYPE(page) == F2FS_WB_CP_DATA)
 				f2fs_stop_checkpoint(sbi, true);
 		}
 
 		f2fs_bug_on(sbi, page->mapping == NODE_MAPPING(sbi) &&
 					page->index != nid_of_node(page));
 
-		dec_page_count(sbi, type);
+		dec_page_count(sbi, WB_DATA_TYPE(page));
 		clear_cold_data(page);
 		end_page_writeback(page);
 	}
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [f2fs-dev] [PATCH 4/4] f2fs: should check the type of end_io after pullback by fscrypto

[Chao Yu]

On 2018/7/7 5:09, Jaegeuk Kim wrote:
> This fixes to get the page type after pullback by fscrypto, since the original
> page is bounce page but we should need to get control page which is what we
> originally submitted.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index c3f9b9baf140..9a0fcec53d0e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -152,7 +152,6 @@ static void f2fs_write_end_io(struct bio *bio)
>  
>  	bio_for_each_segment_all(bvec, bio, i) {
>  		struct page *page = bvec->bv_page;
> -		enum count_type type = WB_DATA_TYPE(page);
>  
>  		if (IS_DUMMY_WRITTEN_PAGE(page)) {
>  			set_page_private(page, (unsigned long)NULL);
> @@ -169,14 +168,14 @@ static void f2fs_write_end_io(struct bio *bio)
>  
>  		if (unlikely(bio->bi_status)) {
>  			mapping_set_error(page->mapping, -EIO);
> -			if (type == F2FS_WB_CP_DATA)
> +			if (WB_DATA_TYPE(page) == F2FS_WB_CP_DATA)
>  				f2fs_stop_checkpoint(sbi, true);
>  		}
>  
>  		f2fs_bug_on(sbi, page->mapping == NODE_MAPPING(sbi) &&
>  					page->index != nid_of_node(page));
>  
> -		dec_page_count(sbi, type);
> +		dec_page_count(sbi, WB_DATA_TYPE(page));

But we account F2FS_WB_CP_DATA/F2FS_WB_DATA type page based bounce page instead
of original page.

	bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;

	/* set submitted = true as a return value */
	fio->submitted = true;

	inc_page_count(sbi, WB_DATA_TYPE(bio_page));

Because for encrypted page of regular inode, we should account it with
F2FS_WB_DATA type:

static bool __is_cp_guaranteed(struct page *page)
{
....
	if (!mapping)   <---bounce page's mapping is NULL
		return false;
}

So we will get F2FS_WB_DATA type from __is_cp_guaranteed below:

enum count_type type = WB_DATA_TYPE(page);

Is there any problem?

Thanks

>  		clear_cold_data(page);
>  		end_page_writeback(page);
>  	}
> 

Re: [f2fs-dev] [PATCH 4/4] f2fs: should check the type of end_io after pullback by fscrypto

[Jaegeuk Kim]

On 07/09, Chao Yu wrote:
> On 2018/7/7 5:09, Jaegeuk Kim wrote:
> > This fixes to get the page type after pullback by fscrypto, since the original
> > page is bounce page but we should need to get control page which is what we
> > originally submitted.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/data.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index c3f9b9baf140..9a0fcec53d0e 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -152,7 +152,6 @@ static void f2fs_write_end_io(struct bio *bio)
> >  
> >  	bio_for_each_segment_all(bvec, bio, i) {
> >  		struct page *page = bvec->bv_page;
> > -		enum count_type type = WB_DATA_TYPE(page);
> >  
> >  		if (IS_DUMMY_WRITTEN_PAGE(page)) {
> >  			set_page_private(page, (unsigned long)NULL);
> > @@ -169,14 +168,14 @@ static void f2fs_write_end_io(struct bio *bio)
> >  
> >  		if (unlikely(bio->bi_status)) {
> >  			mapping_set_error(page->mapping, -EIO);
> > -			if (type == F2FS_WB_CP_DATA)
> > +			if (WB_DATA_TYPE(page) == F2FS_WB_CP_DATA)
> >  				f2fs_stop_checkpoint(sbi, true);
> >  		}
> >  
> >  		f2fs_bug_on(sbi, page->mapping == NODE_MAPPING(sbi) &&
> >  					page->index != nid_of_node(page));
> >  
> > -		dec_page_count(sbi, type);
> > +		dec_page_count(sbi, WB_DATA_TYPE(page));
> 
> But we account F2FS_WB_CP_DATA/F2FS_WB_DATA type page based bounce page instead
> of original page.
> 
> 	bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;
> 
> 	/* set submitted = true as a return value */
> 	fio->submitted = true;
> 
> 	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> 
> Because for encrypted page of regular inode, we should account it with
> F2FS_WB_DATA type:
> 
> static bool __is_cp_guaranteed(struct page *page)
> {
> ....
> 	if (!mapping)   <---bounce page's mapping is NULL
> 		return false;
> }
> 
> So we will get F2FS_WB_DATA type from __is_cp_guaranteed below:
> 
> enum count_type type = WB_DATA_TYPE(page);
> 
> Is there any problem?

Yeah, right. Let me drop this and test again.

Thanks,

> 
> Thanks
> 
> >  		clear_cold_data(page);
> >  		end_page_writeback(page);
> >  	}
> > 

Re: [f2fs-dev] [PATCH 4/4] f2fs: should check the type of end_io after pullback by fscrypto

[Jaegeuk Kim]

On 07/09, Chao Yu wrote:
> On 2018/7/7 5:09, Jaegeuk Kim wrote:
> > This fixes to get the page type after pullback by fscrypto, since the original
> > page is bounce page but we should need to get control page which is what we
> > originally submitted.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/data.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index c3f9b9baf140..9a0fcec53d0e 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -152,7 +152,6 @@ static void f2fs_write_end_io(struct bio *bio)
> >  
> >  	bio_for_each_segment_all(bvec, bio, i) {
> >  		struct page *page = bvec->bv_page;
> > -		enum count_type type = WB_DATA_TYPE(page);
> >  
> >  		if (IS_DUMMY_WRITTEN_PAGE(page)) {
> >  			set_page_private(page, (unsigned long)NULL);
> > @@ -169,14 +168,14 @@ static void f2fs_write_end_io(struct bio *bio)
> >  
> >  		if (unlikely(bio->bi_status)) {
> >  			mapping_set_error(page->mapping, -EIO);
> > -			if (type == F2FS_WB_CP_DATA)
> > +			if (WB_DATA_TYPE(page) == F2FS_WB_CP_DATA)
> >  				f2fs_stop_checkpoint(sbi, true);
> >  		}
> >  
> >  		f2fs_bug_on(sbi, page->mapping == NODE_MAPPING(sbi) &&
> >  					page->index != nid_of_node(page));
> >  
> > -		dec_page_count(sbi, type);
> > +		dec_page_count(sbi, WB_DATA_TYPE(page));
> 
> But we account F2FS_WB_CP_DATA/F2FS_WB_DATA type page based bounce page instead
> of original page.
> 
> 	bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;
> 
> 	/* set submitted = true as a return value */
> 	fio->submitted = true;
> 
> 	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> 
> Because for encrypted page of regular inode, we should account it with
> F2FS_WB_DATA type:
> 
> static bool __is_cp_guaranteed(struct page *page)
> {
> ....
> 	if (!mapping)   <---bounce page's mapping is NULL
> 		return false;
> }
> 
> So we will get F2FS_WB_DATA type from __is_cp_guaranteed below:
> 
> enum count_type type = WB_DATA_TYPE(page);
> 
> Is there any problem?

Wait a minute.

	bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;

1. fio->encrypted_page

	inc_page_count(sbi, F2FS_WB_CP_DATA); // page->mapping == META_MAPPING

  In write_end_io(),
	type = F2FS_WB_DATA; // page->mapping == NULL
	fscrypt_pullback_bio_page(&page);
	  page = fio->encrypted_page;
	
	dec_page_count(F2FS_WB_DATA);

2. fio->page

	inc_page_count(sbi, F2FS_WB_DATA);

  In write_end_io(),
	type = F2FS_WB_DATA; // page->mapping == NULL
	fscrypt_pullback_bio_page(&page);
	  page = fio->encrypted_page;
	
	dec_page_count(F2FS_WB_DATA);

Thoughts?

Re: [f2fs-dev] [PATCH 4/4] f2fs: should check the type of end_io after pullback by fscrypto

[Jaegeuk Kim]

On 07/10, Jaegeuk Kim wrote:
> On 07/09, Chao Yu wrote:
> > On 2018/7/7 5:09, Jaegeuk Kim wrote:
> > > This fixes to get the page type after pullback by fscrypto, since the original
> > > page is bounce page but we should need to get control page which is what we
> > > originally submitted.
> > > 
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/data.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index c3f9b9baf140..9a0fcec53d0e 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -152,7 +152,6 @@ static void f2fs_write_end_io(struct bio *bio)
> > >  
> > >  	bio_for_each_segment_all(bvec, bio, i) {
> > >  		struct page *page = bvec->bv_page;
> > > -		enum count_type type = WB_DATA_TYPE(page);
> > >  
> > >  		if (IS_DUMMY_WRITTEN_PAGE(page)) {
> > >  			set_page_private(page, (unsigned long)NULL);
> > > @@ -169,14 +168,14 @@ static void f2fs_write_end_io(struct bio *bio)
> > >  
> > >  		if (unlikely(bio->bi_status)) {
> > >  			mapping_set_error(page->mapping, -EIO);
> > > -			if (type == F2FS_WB_CP_DATA)
> > > +			if (WB_DATA_TYPE(page) == F2FS_WB_CP_DATA)
> > >  				f2fs_stop_checkpoint(sbi, true);
> > >  		}
> > >  
> > >  		f2fs_bug_on(sbi, page->mapping == NODE_MAPPING(sbi) &&
> > >  					page->index != nid_of_node(page));
> > >  
> > > -		dec_page_count(sbi, type);
> > > +		dec_page_count(sbi, WB_DATA_TYPE(page));
> > 
> > But we account F2FS_WB_CP_DATA/F2FS_WB_DATA type page based bounce page instead
> > of original page.
> > 
> > 	bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;
> > 
> > 	/* set submitted = true as a return value */
> > 	fio->submitted = true;
> > 
> > 	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> > 
> > Because for encrypted page of regular inode, we should account it with
> > F2FS_WB_DATA type:
> > 
> > static bool __is_cp_guaranteed(struct page *page)
> > {
> > ....
> > 	if (!mapping)   <---bounce page's mapping is NULL
> > 		return false;
> > }
> > 
> > So we will get F2FS_WB_DATA type from __is_cp_guaranteed below:
> > 
> > enum count_type type = WB_DATA_TYPE(page);
> > 
> > Is there any problem?
> 
> Wait a minute.
> 
> 	bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;
> 
> 1. fio->encrypted_page
> 
> 	inc_page_count(sbi, F2FS_WB_CP_DATA); // page->mapping == META_MAPPING
> 
>   In write_end_io(),
> 	type = F2FS_WB_DATA; // page->mapping == NULL
> 	fscrypt_pullback_bio_page(&page);
> 	  page = fio->encrypted_page;
> 	
> 	dec_page_count(F2FS_WB_DATA);
> 
> 2. fio->page
> 
> 	inc_page_count(sbi, F2FS_WB_DATA);
> 
>   In write_end_io(),
> 	type = F2FS_WB_DATA; // page->mapping == NULL
> 	fscrypt_pullback_bio_page(&page);
> 	  page = fio->encrypted_page;
> 	
> 	dec_page_count(F2FS_WB_DATA);
> 
> Thoughts?

Oops, nvm. I was confused. :P

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

Re: [f2fs-dev] [PATCH 1/4] f2fs: flush journal nat entries for nat_bits during unmount

[Chao Yu]

On 2018/7/7 5:09, Jaegeuk Kim wrote:
> Let's flush journal nat entries for speed up in the next run.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/node.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 29237aeca041..0f076fb0d828 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -2613,6 +2613,13 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	nid_t set_idx = 0;
>  	LIST_HEAD(sets);
>  
> +	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
> +	if (enabled_nat_bits(sbi, cpc)) {
> +		down_write(&nm_i->nat_tree_lock);
> +		remove_nats_in_journal(sbi);
> +		up_write(&nm_i->nat_tree_lock);
> +	}

The case will cover that nm_i->dirty_nat_cnt is zero and there is cached nats in
journal?

So enabled_nat_bits() below should be removed?

	if (enabled_nat_bits(sbi, cpc) ||
		!__has_cursum_space(journal, nm_i->dirty_nat_cnt, NAT_JOURNAL))
		remove_nats_in_journal(sbi);

Thanks,

> +
>  	if (!nm_i->dirty_nat_cnt)
>  		return;
>  
> 

Re: [f2fs-dev] [PATCH 1/4] f2fs: flush journal nat entries for nat_bits during unmount

[Jaegeuk Kim]

On 07/09, Chao Yu wrote:
> On 2018/7/7 5:09, Jaegeuk Kim wrote:
> > Let's flush journal nat entries for speed up in the next run.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/node.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 29237aeca041..0f076fb0d828 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -2613,6 +2613,13 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	nid_t set_idx = 0;
> >  	LIST_HEAD(sets);
> >  
> > +	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
> > +	if (enabled_nat_bits(sbi, cpc)) {
> > +		down_write(&nm_i->nat_tree_lock);
> > +		remove_nats_in_journal(sbi);
> > +		up_write(&nm_i->nat_tree_lock);
> > +	}
> 
> The case will cover that nm_i->dirty_nat_cnt is zero and there is cached nats in
> journal?

Yes.

> 
> So enabled_nat_bits() below should be removed?

It's out of lock, nm_i->nat_tree_lock, logically..

> 
> 	if (enabled_nat_bits(sbi, cpc) ||
> 		!__has_cursum_space(journal, nm_i->dirty_nat_cnt, NAT_JOURNAL))
> 		remove_nats_in_journal(sbi);
> 
> Thanks,
> 
> > +
> >  	if (!nm_i->dirty_nat_cnt)
> >  		return;
> >  
> > 

Re: [f2fs-dev] [PATCH 1/4] f2fs: flush journal nat entries for nat_bits during unmount

[Chao Yu]

On 2018/7/10 4:43, Jaegeuk Kim wrote:
> On 07/09, Chao Yu wrote:
>> On 2018/7/7 5:09, Jaegeuk Kim wrote:
>>> Let's flush journal nat entries for speed up in the next run.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/node.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index 29237aeca041..0f076fb0d828 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -2613,6 +2613,13 @@ void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	nid_t set_idx = 0;
>>>  	LIST_HEAD(sets);
>>>  
>>> +	/* during unmount, let's flush nat_bits before checking dirty_nat_cnt */
>>> +	if (enabled_nat_bits(sbi, cpc)) {
>>> +		down_write(&nm_i->nat_tree_lock);
>>> +		remove_nats_in_journal(sbi);
>>> +		up_write(&nm_i->nat_tree_lock);
>>> +	}
>>
>> The case will cover that nm_i->dirty_nat_cnt is zero and there is cached nats in
>> journal?
> 
> Yes.
> 
>>
>> So enabled_nat_bits() below should be removed?
> 
> It's out of lock, nm_i->nat_tree_lock, logically..

CP_NAT_BITS_FLAG modification should be happened in mount or checkpoint, so
there should be no contention.

Anyway, keep it here is okay to me.

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> 
>>
>> 	if (enabled_nat_bits(sbi, cpc) ||
>> 		!__has_cursum_space(journal, nm_i->dirty_nat_cnt, NAT_JOURNAL))
>> 		remove_nats_in_journal(sbi);
>>
>> Thanks,
>>
>>> +
>>>  	if (!nm_i->dirty_nat_cnt)
>>>  		return;
>>>  
>>>
> 
> .
>