| Message ID | 20231210113547.3412782-5-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,1/6] f2fs: fix to tag gcing flag on page during block migration | expand |
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
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
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
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
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
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
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; }
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(-)