2023-12-10 11:36:32

by Chao Yu

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

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 <[email protected]>
---
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

2023-12-10 11:36:33

by Chao Yu

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

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 <[email protected]>
---
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

2023-12-10 11:36:45

by Chao Yu

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

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 <[email protected]>
---
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

2023-12-10 11:36:45

by Chao Yu

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

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 <[email protected]>
Signed-off-by: Chao Yu <[email protected]>
---
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

2023-12-10 11:36:47

by Chao Yu

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

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 <[email protected]>
---
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

2023-12-11 22:08:57

by Jaegeuk Kim

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

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 <[email protected]>
> Signed-off-by: Chao Yu <[email protected]>
> ---
> 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

2023-12-11 22:12:07

by Jaegeuk Kim

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

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 <[email protected]>
> ---
> 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

2023-12-12 01:05:28

by Chao Yu

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

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 <[email protected]>
>> ---
>> 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

2023-12-12 22:22:08

by Jaegeuk Kim

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

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 <[email protected]>
> > > ---
> > > 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

2023-12-14 20:50:48

by patchwork-bot+f2fs

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

Hello:

This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <[email protected]>:

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

2023-12-28 15:34:12

by Chao Yu

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

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 <[email protected]>
>>>> ---
>>>> 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

2024-01-02 22:54:40

by Jaegeuk Kim

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

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 <[email protected]>
> > > > > ---
> > > > > 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

2024-01-11 03:07:38

by Chao Yu

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

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 <[email protected]>
>>>>>> ---
>>>>>> 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