[PATCH 1/6] f2fs: fix to tag gcing flag on page during block migration

[PATCH 1/6] f2fs: fix to tag gcing flag on page during block migration

[Chao Yu]

It needs to add missing gcing flag on page during block migration,
in order to garantee migrated data be persisted during checkpoint,
otherwise out-of-order persistency between data and node may cause
data corruption after SPOR.

Similar issue was fixed by commit 2d1fe8a86bf5 ("f2fs: fix to tag
gcing flag on page during file defragment").

Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/compress.c | 4 +++-
 fs/f2fs/file.c     | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index b35be5799726..c5a4364c4482 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1036,8 +1036,10 @@ static void set_cluster_dirty(struct compress_ctx *cc)
 	int i;
 
 	for (i = 0; i < cc->cluster_size; i++)
-		if (cc->rpages[i])
+		if (cc->rpages[i]) {
 			set_page_dirty(cc->rpages[i]);
+			set_page_private_gcing(cc->rpages[i]);
+		}
 }
 
 static int prepare_compress_overwrite(struct compress_ctx *cc,
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 60290940018d..156b0ff05a3b 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1312,6 +1312,7 @@ static int __clone_blkaddrs(struct inode *src_inode, struct inode *dst_inode,
 			}
 			memcpy_page(pdst, 0, psrc, 0, PAGE_SIZE);
 			set_page_dirty(pdst);
+			set_page_private_gcing(pdst);
 			f2fs_put_page(pdst, 1);
 			f2fs_put_page(psrc, 1);
 
@@ -4046,6 +4047,7 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
 		f2fs_bug_on(F2FS_I_SB(inode), !page);
 
 		set_page_dirty(page);
+		set_page_private_gcing(page);
 		f2fs_put_page(page, 1);
 		f2fs_put_page(page, 0);
 	}
-- 
2.40.1

[PATCH 2/6] f2fs: fix to wait on block writeback for post_read case

[Chao Yu]

If inode is compressed, but not encrypted, it missed to call
f2fs_wait_on_block_writeback() to wait for GCed page writeback
in IPU write path.

Thread A				GC-Thread
					- f2fs_gc
					 - do_garbage_collect
					  - gc_data_segment
					   - move_data_block
					    - f2fs_submit_page_write
					     migrate normal cluster's block via
					     meta_inode's page cache
- f2fs_write_single_data_page
 - f2fs_do_write_data_page
  - f2fs_inplace_write_data
   - f2fs_submit_page_bio

IRQ
- f2fs_read_end_io
					IRQ
					old data overrides new data due to
					out-of-order GC and common IO.
					- f2fs_read_end_io

Fixes: 4c8ff7095bef ("f2fs: support data compression")
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/data.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 27015b7875ae..dce8defdf4c7 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2556,9 +2556,6 @@ int f2fs_encrypt_one_page(struct f2fs_io_info *fio)
 
 	page = fio->compressed_page ? fio->compressed_page : fio->page;
 
-	/* wait for GCed page writeback via META_MAPPING */
-	f2fs_wait_on_block_writeback(inode, fio->old_blkaddr);
-
 	if (fscrypt_inode_uses_inline_crypto(inode))
 		return 0;
 
@@ -2745,6 +2742,10 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
 		goto out_writepage;
 	}
 
+	/* wait for GCed page writeback via META_MAPPING */
+	if (fio->post_read)
+		f2fs_wait_on_block_writeback(inode, fio->old_blkaddr);
+
 	/*
 	 * If current allocation needs SSR,
 	 * it had better in-place writes for updated data.
-- 
2.40.1

[PATCH 3/6] f2fs: fix to check compress file in f2fs_move_file_range()

[Chao Yu]

f2fs_move_file_range() doesn't support migrating compressed cluster
data, let's add the missing check condition and return -EOPNOTSUPP
for the case until we support it.

Fixes: 4c8ff7095bef ("f2fs: support data compression")
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/file.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 156b0ff05a3b..5c2f99ada6be 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2813,6 +2813,11 @@ static int f2fs_move_file_range(struct file *file_in, loff_t pos_in,
 			goto out;
 	}
 
+	if (f2fs_compressed_file(src) || f2fs_compressed_file(dst)) {
+		ret = -EOPNOTSUPP;
+		goto out_unlock;
+	}
+
 	ret = -EINVAL;
 	if (pos_in + len > src->i_size || pos_in + len < pos_in)
 		goto out_unlock;
-- 
2.40.1

[PATCH 4/6] f2fs: don't set FI_PREALLOCATED_ALL for partial write

[Chao Yu]

In f2fs_preallocate_blocks(), if it is partial write in 4KB, it's not
necessary to call f2fs_map_blocks() and set FI_PREALLOCATED_ALL flag.

Cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/file.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5c2f99ada6be..1a3c29a9a6a0 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -4561,13 +4561,14 @@ static int f2fs_preallocate_blocks(struct kiocb *iocb, struct iov_iter *iter,
 			return ret;
 	}
 
-	/* Do not preallocate blocks that will be written partially in 4KB. */
 	map.m_lblk = F2FS_BLK_ALIGN(pos);
 	map.m_len = F2FS_BYTES_TO_BLK(pos + count);
-	if (map.m_len > map.m_lblk)
-		map.m_len -= map.m_lblk;
-	else
-		map.m_len = 0;
+
+	/* Do not preallocate blocks that will be written partially in 4KB. */
+	if (map.m_len <= map.m_lblk)
+		return 0;
+
+	map.m_len -= map.m_lblk;
 	map.m_may_create = true;
 	if (dio) {
 		map.m_seg_type = f2fs_rw_hint_to_seg_type(inode->i_write_hint);
-- 
2.40.1

[PATCH 5/6] f2fs: fix to restrict condition of compress inode conversion

[Chao Yu]

This patch adds i_size check during compress inode conversion in order
to avoid .page_mkwrite races w/ conversion.

Fixes: 4c8ff7095bef ("f2fs: support data compression")
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/f2fs.h | 8 +++++++-
 fs/f2fs/file.c | 5 ++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 65294e3b0bef..c9b8a1953913 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode)
 #endif
 }
 
+static inline bool inode_has_data(struct inode *inode)
+{
+	return (S_ISREG(inode->i_mode) &&
+		(F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
+}
+
 static inline bool f2fs_disable_compressed_file(struct inode *inode)
 {
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 
 	if (!f2fs_compressed_file(inode))
 		return true;
-	if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
+	if (inode_has_data(inode))
 		return false;
 
 	fi->i_flags &= ~F2FS_COMPR_FL;
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 1a3c29a9a6a0..8af4b29c3e1a 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
 
 			f2fs_down_write(&F2FS_I(inode)->i_sem);
 			if (!f2fs_may_compress(inode) ||
-					(S_ISREG(inode->i_mode) &&
-					F2FS_HAS_BLOCKS(inode))) {
+					inode_has_data(inode)) {
 				f2fs_up_write(&F2FS_I(inode)->i_sem);
 				return -EINVAL;
 			}
@@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
 		goto out;
 	}
 
-	if (F2FS_HAS_BLOCKS(inode)) {
+	if (inode_has_data(inode)) {
 		ret = -EFBIG;
 		goto out;
 	}
-- 
2.40.1

[PATCH 6/6] f2fs: fix to update iostat correctly in f2fs_filemap_fault()

[Chao Yu]

In f2fs_filemap_fault(), it fixes to update iostat info only if
VM_FAULT_LOCKED is tagged in return value of filemap_fault().

Fixes: 8b83ac81f428 ("f2fs: support read iostat")
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 8af4b29c3e1a..0626da43fa84 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -42,7 +42,7 @@ static vm_fault_t f2fs_filemap_fault(struct vm_fault *vmf)
 	vm_fault_t ret;
 
 	ret = filemap_fault(vmf);
-	if (!ret)
+	if (ret & VM_FAULT_LOCKED)
 		f2fs_update_iostat(F2FS_I_SB(inode), inode,
 					APP_MAPPED_READ_IO, F2FS_BLKSIZE);
 
-- 
2.40.1

Re: [PATCH 4/6] f2fs: don't set FI_PREALLOCATED_ALL for partial write

[Jaegeuk Kim]

On 12/10, Chao Yu wrote:
> In f2fs_preallocate_blocks(), if it is partial write in 4KB, it's not
> necessary to call f2fs_map_blocks() and set FI_PREALLOCATED_ALL flag.
> 
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/file.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 5c2f99ada6be..1a3c29a9a6a0 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -4561,13 +4561,14 @@ static int f2fs_preallocate_blocks(struct kiocb *iocb, struct iov_iter *iter,
>  			return ret;
>  	}
>  
> -	/* Do not preallocate blocks that will be written partially in 4KB. */
>  	map.m_lblk = F2FS_BLK_ALIGN(pos);
>  	map.m_len = F2FS_BYTES_TO_BLK(pos + count);
> -	if (map.m_len > map.m_lblk)
> -		map.m_len -= map.m_lblk;
> -	else

		return 0;

We just need the above?

> -		map.m_len = 0;
> +
> +	/* Do not preallocate blocks that will be written partially in 4KB. */
> +	if (map.m_len <= map.m_lblk)
> +		return 0;
> +
> +	map.m_len -= map.m_lblk;
>  	map.m_may_create = true;
>  	if (dio) {
>  		map.m_seg_type = f2fs_rw_hint_to_seg_type(inode->i_write_hint);
> -- 
> 2.40.1

Re: [PATCH 5/6] f2fs: fix to restrict condition of compress inode conversion

[Jaegeuk Kim]

On 12/10, Chao Yu wrote:
> This patch adds i_size check during compress inode conversion in order
> to avoid .page_mkwrite races w/ conversion.

Which race condition do you see?

> 
> Fixes: 4c8ff7095bef ("f2fs: support data compression")
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/f2fs.h | 8 +++++++-
>  fs/f2fs/file.c | 5 ++---
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 65294e3b0bef..c9b8a1953913 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode)
>  #endif
>  }
>  
> +static inline bool inode_has_data(struct inode *inode)
> +{
> +	return (S_ISREG(inode->i_mode) &&
> +		(F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
> +}
> +
>  static inline bool f2fs_disable_compressed_file(struct inode *inode)
>  {
>  	struct f2fs_inode_info *fi = F2FS_I(inode);
>  
>  	if (!f2fs_compressed_file(inode))
>  		return true;
> -	if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
> +	if (inode_has_data(inode))
>  		return false;
>  
>  	fi->i_flags &= ~F2FS_COMPR_FL;
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 1a3c29a9a6a0..8af4b29c3e1a 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>  
>  			f2fs_down_write(&F2FS_I(inode)->i_sem);
>  			if (!f2fs_may_compress(inode) ||
> -					(S_ISREG(inode->i_mode) &&
> -					F2FS_HAS_BLOCKS(inode))) {
> +					inode_has_data(inode)) {
>  				f2fs_up_write(&F2FS_I(inode)->i_sem);
>  				return -EINVAL;
>  			}
> @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
>  		goto out;
>  	}
>  
> -	if (F2FS_HAS_BLOCKS(inode)) {
> +	if (inode_has_data(inode)) {
>  		ret = -EFBIG;
>  		goto out;
>  	}
> -- 
> 2.40.1

Re: [PATCH 5/6] f2fs: fix to restrict condition of compress inode conversion

[Chao Yu]

On 2023/12/12 6:11, Jaegeuk Kim wrote:
> On 12/10, Chao Yu wrote:
>> This patch adds i_size check during compress inode conversion in order
>> to avoid .page_mkwrite races w/ conversion.
> 
> Which race condition do you see?

Something like:

- f2fs_setflags_common
  - check S_ISREG && F2FS_HAS_BLOCKS
					- mkwrite
					 - f2fs_get_block_locked
					  : update metadata in old inode's disk layout
  - set_compress_context

Thanks,

> 
>>
>> Fixes: 4c8ff7095bef ("f2fs: support data compression")
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>>   fs/f2fs/f2fs.h | 8 +++++++-
>>   fs/f2fs/file.c | 5 ++---
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 65294e3b0bef..c9b8a1953913 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode)
>>   #endif
>>   }
>>   
>> +static inline bool inode_has_data(struct inode *inode)
>> +{
>> +	return (S_ISREG(inode->i_mode) &&
>> +		(F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
>> +}
>> +
>>   static inline bool f2fs_disable_compressed_file(struct inode *inode)
>>   {
>>   	struct f2fs_inode_info *fi = F2FS_I(inode);
>>   
>>   	if (!f2fs_compressed_file(inode))
>>   		return true;
>> -	if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
>> +	if (inode_has_data(inode))
>>   		return false;
>>   
>>   	fi->i_flags &= ~F2FS_COMPR_FL;
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 1a3c29a9a6a0..8af4b29c3e1a 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>>   
>>   			f2fs_down_write(&F2FS_I(inode)->i_sem);
>>   			if (!f2fs_may_compress(inode) ||
>> -					(S_ISREG(inode->i_mode) &&
>> -					F2FS_HAS_BLOCKS(inode))) {
>> +					inode_has_data(inode)) {
>>   				f2fs_up_write(&F2FS_I(inode)->i_sem);
>>   				return -EINVAL;
>>   			}
>> @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
>>   		goto out;
>>   	}
>>   
>> -	if (F2FS_HAS_BLOCKS(inode)) {
>> +	if (inode_has_data(inode)) {
>>   		ret = -EFBIG;
>>   		goto out;
>>   	}
>> -- 
>> 2.40.1

Re: [PATCH 5/6] f2fs: fix to restrict condition of compress inode conversion

[Jaegeuk Kim]

On 12/12, Chao Yu wrote:
> On 2023/12/12 6:11, Jaegeuk Kim wrote:
> > On 12/10, Chao Yu wrote:
> > > This patch adds i_size check during compress inode conversion in order
> > > to avoid .page_mkwrite races w/ conversion.
> > 
> > Which race condition do you see?
> 
> Something like:
> 
> - f2fs_setflags_common
>  - check S_ISREG && F2FS_HAS_BLOCKS
> 					- mkwrite
> 					 - f2fs_get_block_locked
> 					  : update metadata in old inode's disk layout

Don't we need to prevent setting this for mmapped file?

>  - set_compress_context
> 
> Thanks,
> 
> > 
> > > 
> > > Fixes: 4c8ff7095bef ("f2fs: support data compression")
> > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > ---
> > >   fs/f2fs/f2fs.h | 8 +++++++-
> > >   fs/f2fs/file.c | 5 ++---
> > >   2 files changed, 9 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 65294e3b0bef..c9b8a1953913 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode)
> > >   #endif
> > >   }
> > > +static inline bool inode_has_data(struct inode *inode)
> > > +{
> > > +	return (S_ISREG(inode->i_mode) &&
> > > +		(F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
> > > +}
> > > +
> > >   static inline bool f2fs_disable_compressed_file(struct inode *inode)
> > >   {
> > >   	struct f2fs_inode_info *fi = F2FS_I(inode);
> > >   	if (!f2fs_compressed_file(inode))
> > >   		return true;
> > > -	if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
> > > +	if (inode_has_data(inode))
> > >   		return false;
> > >   	fi->i_flags &= ~F2FS_COMPR_FL;
> > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > index 1a3c29a9a6a0..8af4b29c3e1a 100644
> > > --- a/fs/f2fs/file.c
> > > +++ b/fs/f2fs/file.c
> > > @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
> > >   			f2fs_down_write(&F2FS_I(inode)->i_sem);
> > >   			if (!f2fs_may_compress(inode) ||
> > > -					(S_ISREG(inode->i_mode) &&
> > > -					F2FS_HAS_BLOCKS(inode))) {
> > > +					inode_has_data(inode)) {
> > >   				f2fs_up_write(&F2FS_I(inode)->i_sem);
> > >   				return -EINVAL;
> > >   			}
> > > @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
> > >   		goto out;
> > >   	}
> > > -	if (F2FS_HAS_BLOCKS(inode)) {
> > > +	if (inode_has_data(inode)) {
> > >   		ret = -EFBIG;
> > >   		goto out;
> > >   	}
> > > -- 
> > > 2.40.1

Re: [f2fs-dev] [PATCH 1/6] f2fs: fix to tag gcing flag on page during block migration

[patchwork-bot+f2fs]

Hello:

This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Sun, 10 Dec 2023 19:35:42 +0800 you wrote:
> It needs to add missing gcing flag on page during block migration,
> in order to garantee migrated data be persisted during checkpoint,
> otherwise out-of-order persistency between data and node may cause
> data corruption after SPOR.
> 
> Similar issue was fixed by commit 2d1fe8a86bf5 ("f2fs: fix to tag
> gcing flag on page during file defragment").
> 
> [...]

Here is the summary with links:
  - [f2fs-dev,1/6] f2fs: fix to tag gcing flag on page during block migration
    https://git.kernel.org/jaegeuk/f2fs/c/4961acdd65c9
  - [f2fs-dev,2/6] f2fs: fix to wait on block writeback for post_read case
    https://git.kernel.org/jaegeuk/f2fs/c/55fdc1c24a1d
  - [f2fs-dev,3/6] f2fs: fix to check compress file in f2fs_move_file_range()
    https://git.kernel.org/jaegeuk/f2fs/c/fb9b65340c81
  - [f2fs-dev,4/6] f2fs: don't set FI_PREALLOCATED_ALL for partial write
    (no matching commit)
  - [f2fs-dev,5/6] f2fs: fix to restrict condition of compress inode conversion
    (no matching commit)
  - [f2fs-dev,6/6] f2fs: fix to update iostat correctly in f2fs_filemap_fault()
    https://git.kernel.org/jaegeuk/f2fs/c/bb34cc6ca87f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 5/6] f2fs: fix to restrict condition of compress inode conversion

[Chao Yu]

On 2023/12/13 6:21, Jaegeuk Kim wrote:
> On 12/12, Chao Yu wrote:
>> On 2023/12/12 6:11, Jaegeuk Kim wrote:
>>> On 12/10, Chao Yu wrote:
>>>> This patch adds i_size check during compress inode conversion in order
>>>> to avoid .page_mkwrite races w/ conversion.
>>>
>>> Which race condition do you see?
>>
>> Something like:
>>
>> - f2fs_setflags_common
>>   - check S_ISREG && F2FS_HAS_BLOCKS
>> 					- mkwrite
>> 					 - f2fs_get_block_locked
>> 					  : update metadata in old inode's disk layout
> 
> Don't we need to prevent setting this for mmapped file?

Oh, we have used i_sem lock to prevent such race case, however
we missed f2fs_disable_compressed_file():

- f2fs_disable_compressed_file
  - check inode_has_data
						- f2fs_file_mmap
						- mkwrite
						 - f2fs_get_block_locked
						 : update metadata in compressed
						   inode's disk layout
  - fi->i_flags &= ~F2FS_COMPR_FL
  - clear_inode_flag(inode, FI_COMPRESSED_FILE);

Thanks,

> 
>>   - set_compress_context
>>
>> Thanks,
>>
>>>
>>>>
>>>> Fixes: 4c8ff7095bef ("f2fs: support data compression")
>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>> ---
>>>>    fs/f2fs/f2fs.h | 8 +++++++-
>>>>    fs/f2fs/file.c | 5 ++---
>>>>    2 files changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index 65294e3b0bef..c9b8a1953913 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode)
>>>>    #endif
>>>>    }
>>>> +static inline bool inode_has_data(struct inode *inode)
>>>> +{
>>>> +	return (S_ISREG(inode->i_mode) &&
>>>> +		(F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
>>>> +}
>>>> +
>>>>    static inline bool f2fs_disable_compressed_file(struct inode *inode)
>>>>    {
>>>>    	struct f2fs_inode_info *fi = F2FS_I(inode);
>>>>    	if (!f2fs_compressed_file(inode))
>>>>    		return true;
>>>> -	if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
>>>> +	if (inode_has_data(inode))
>>>>    		return false;
>>>>    	fi->i_flags &= ~F2FS_COMPR_FL;
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index 1a3c29a9a6a0..8af4b29c3e1a 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>>>>    			f2fs_down_write(&F2FS_I(inode)->i_sem);
>>>>    			if (!f2fs_may_compress(inode) ||
>>>> -					(S_ISREG(inode->i_mode) &&
>>>> -					F2FS_HAS_BLOCKS(inode))) {
>>>> +					inode_has_data(inode)) {
>>>>    				f2fs_up_write(&F2FS_I(inode)->i_sem);
>>>>    				return -EINVAL;
>>>>    			}
>>>> @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
>>>>    		goto out;
>>>>    	}
>>>> -	if (F2FS_HAS_BLOCKS(inode)) {
>>>> +	if (inode_has_data(inode)) {
>>>>    		ret = -EFBIG;
>>>>    		goto out;
>>>>    	}
>>>> -- 
>>>> 2.40.1

Re: [PATCH 5/6] f2fs: fix to restrict condition of compress inode conversion

[Jaegeuk Kim]

On 12/28, Chao Yu wrote:
> On 2023/12/13 6:21, Jaegeuk Kim wrote:
> > On 12/12, Chao Yu wrote:
> > > On 2023/12/12 6:11, Jaegeuk Kim wrote:
> > > > On 12/10, Chao Yu wrote:
> > > > > This patch adds i_size check during compress inode conversion in order
> > > > > to avoid .page_mkwrite races w/ conversion.
> > > > 
> > > > Which race condition do you see?
> > > 
> > > Something like:
> > > 
> > > - f2fs_setflags_common
> > >   - check S_ISREG && F2FS_HAS_BLOCKS
> > > 					- mkwrite
> > > 					 - f2fs_get_block_locked
> > > 					  : update metadata in old inode's disk layout
> > 
> > Don't we need to prevent setting this for mmapped file?
> 
> Oh, we have used i_sem lock to prevent such race case, however
> we missed f2fs_disable_compressed_file():
> 
> - f2fs_disable_compressed_file
>  - check inode_has_data
> 						- f2fs_file_mmap
> 						- mkwrite
> 						 - f2fs_get_block_locked
> 						 : update metadata in compressed
> 						   inode's disk layout
>  - fi->i_flags &= ~F2FS_COMPR_FL
>  - clear_inode_flag(inode, FI_COMPRESSED_FILE);

So, needing i_sem for disabling it on mmapped file? It seems i_size would not
be enough?

> 
> Thanks,
> 
> > 
> > >   - set_compress_context
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > Fixes: 4c8ff7095bef ("f2fs: support data compression")
> > > > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > > > ---
> > > > >    fs/f2fs/f2fs.h | 8 +++++++-
> > > > >    fs/f2fs/file.c | 5 ++---
> > > > >    2 files changed, 9 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > index 65294e3b0bef..c9b8a1953913 100644
> > > > > --- a/fs/f2fs/f2fs.h
> > > > > +++ b/fs/f2fs/f2fs.h
> > > > > @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode)
> > > > >    #endif
> > > > >    }
> > > > > +static inline bool inode_has_data(struct inode *inode)
> > > > > +{
> > > > > +	return (S_ISREG(inode->i_mode) &&
> > > > > +		(F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
> > > > > +}
> > > > > +
> > > > >    static inline bool f2fs_disable_compressed_file(struct inode *inode)
> > > > >    {
> > > > >    	struct f2fs_inode_info *fi = F2FS_I(inode);
> > > > >    	if (!f2fs_compressed_file(inode))
> > > > >    		return true;
> > > > > -	if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
> > > > > +	if (inode_has_data(inode))
> > > > >    		return false;
> > > > >    	fi->i_flags &= ~F2FS_COMPR_FL;
> > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > > > index 1a3c29a9a6a0..8af4b29c3e1a 100644
> > > > > --- a/fs/f2fs/file.c
> > > > > +++ b/fs/f2fs/file.c
> > > > > @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
> > > > >    			f2fs_down_write(&F2FS_I(inode)->i_sem);
> > > > >    			if (!f2fs_may_compress(inode) ||
> > > > > -					(S_ISREG(inode->i_mode) &&
> > > > > -					F2FS_HAS_BLOCKS(inode))) {
> > > > > +					inode_has_data(inode)) {
> > > > >    				f2fs_up_write(&F2FS_I(inode)->i_sem);
> > > > >    				return -EINVAL;
> > > > >    			}
> > > > > @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
> > > > >    		goto out;
> > > > >    	}
> > > > > -	if (F2FS_HAS_BLOCKS(inode)) {
> > > > > +	if (inode_has_data(inode)) {
> > > > >    		ret = -EFBIG;
> > > > >    		goto out;
> > > > >    	}
> > > > > -- 
> > > > > 2.40.1

Re: [PATCH 5/6] f2fs: fix to restrict condition of compress inode conversion

[Chao Yu]

On 2024/1/3 6:54, Jaegeuk Kim wrote:
> On 12/28, Chao Yu wrote:
>> On 2023/12/13 6:21, Jaegeuk Kim wrote:
>>> On 12/12, Chao Yu wrote:
>>>> On 2023/12/12 6:11, Jaegeuk Kim wrote:
>>>>> On 12/10, Chao Yu wrote:
>>>>>> This patch adds i_size check during compress inode conversion in order
>>>>>> to avoid .page_mkwrite races w/ conversion.
>>>>>
>>>>> Which race condition do you see?
>>>>
>>>> Something like:
>>>>
>>>> - f2fs_setflags_common
>>>>    - check S_ISREG && F2FS_HAS_BLOCKS
>>>> 					- mkwrite
>>>> 					 - f2fs_get_block_locked
>>>> 					  : update metadata in old inode's disk layout
>>>
>>> Don't we need to prevent setting this for mmapped file?
>>
>> Oh, we have used i_sem lock to prevent such race case, however
>> we missed f2fs_disable_compressed_file():
>>
>> - f2fs_disable_compressed_file
>>   - check inode_has_data
>> 						- f2fs_file_mmap
>> 						- mkwrite
>> 						 - f2fs_get_block_locked
>> 						 : update metadata in compressed
>> 						   inode's disk layout
>>   - fi->i_flags &= ~F2FS_COMPR_FL
>>   - clear_inode_flag(inode, FI_COMPRESSED_FILE);
> 
> So, needing i_sem for disabling it on mmapped file? It seems i_size would not
> be enough?

Agreed, let me update the patch.

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>    - set_compress_context
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Fixes: 4c8ff7095bef ("f2fs: support data compression")
>>>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>>>> ---
>>>>>>     fs/f2fs/f2fs.h | 8 +++++++-
>>>>>>     fs/f2fs/file.c | 5 ++---
>>>>>>     2 files changed, 9 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>> index 65294e3b0bef..c9b8a1953913 100644
>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>> @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode)
>>>>>>     #endif
>>>>>>     }
>>>>>> +static inline bool inode_has_data(struct inode *inode)
>>>>>> +{
>>>>>> +	return (S_ISREG(inode->i_mode) &&
>>>>>> +		(F2FS_HAS_BLOCKS(inode) || i_size_read(inode)));
>>>>>> +}
>>>>>> +
>>>>>>     static inline bool f2fs_disable_compressed_file(struct inode *inode)
>>>>>>     {
>>>>>>     	struct f2fs_inode_info *fi = F2FS_I(inode);
>>>>>>     	if (!f2fs_compressed_file(inode))
>>>>>>     		return true;
>>>>>> -	if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode))
>>>>>> +	if (inode_has_data(inode))
>>>>>>     		return false;
>>>>>>     	fi->i_flags &= ~F2FS_COMPR_FL;
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index 1a3c29a9a6a0..8af4b29c3e1a 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>>>>>>     			f2fs_down_write(&F2FS_I(inode)->i_sem);
>>>>>>     			if (!f2fs_may_compress(inode) ||
>>>>>> -					(S_ISREG(inode->i_mode) &&
>>>>>> -					F2FS_HAS_BLOCKS(inode))) {
>>>>>> +					inode_has_data(inode)) {
>>>>>>     				f2fs_up_write(&F2FS_I(inode)->i_sem);
>>>>>>     				return -EINVAL;
>>>>>>     			}
>>>>>> @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg)
>>>>>>     		goto out;
>>>>>>     	}
>>>>>> -	if (F2FS_HAS_BLOCKS(inode)) {
>>>>>> +	if (inode_has_data(inode)) {
>>>>>>     		ret = -EFBIG;
>>>>>>     		goto out;
>>>>>>     	}
>>>>>> -- 
>>>>>> 2.40.1