| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6fb1d0be-b9d5-ce0a-7984-56b2e1a1a242@vivo.com>
Date: Mon, 19 Jun 2023 17:31:19 +0800
From: Yangtao Li <frank.li@...o.com>
To: Chao Yu <chao@...nel.org>, Jaegeuk Kim <jaegeuk@...nel.org>
Cc: Qi Han <hanqi@...o.com>, linux-f2fs-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] f2fs: compress: don't force buffered io when in
COMPR_MODE_USER mode
On 2023/6/19 12:04, Chao Yu wrote:
> On 2023/6/19 11:11, Yangtao Li wrote:
>> On 2023/6/19 8:54, Chao Yu wrote:
>>
>>> On 2023/6/13 12:14, Yangtao Li wrote:
>>>>
>>>> On 2023/6/12 22:38, Chao Yu wrote:
>>>>> On 2023/6/9 21:15, Yangtao Li wrote:
>>>>>> It is observed that when in user compression mode
>>>>>> (compress_extension=*),
>>>>>> even though the file is not compressed, the file is still forced
>>>>>> to use
>>>>>> buffer io, which makes the AndroBench sequential read and write drop
>>>>>> significantly. In fact, when the file is not compressed, we don't
>>>>>> need
>>>>>> to force it to buffer io.
>>>>>>
>>>>>> | w/o patch | w/ patch |
>>>>>> seq read (MB/s) | 1320.068 | 3696.154 |
>>>>>> seq write (MB/s) | 617.996 | 2978.478 |
>>>>>>
>>>>>> Fixes: 4c8ff7095bef ("f2fs: support data compression")
>>>>>> Signed-off-by: Qi Han <hanqi@...o.com>
>>>>>> Signed-off-by: Yangtao Li <frank.li@...o.com>
>>>>>> ---
>>>>>> fs/f2fs/f2fs.h | 14 ++++++++++++++
>>>>>> fs/f2fs/file.c | 2 +-
>>>>>> 2 files changed, 15 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>> index 1efcfd9e5a99..7f5472525310 100644
>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>> @@ -3168,6 +3168,20 @@ static inline int f2fs_compressed_file(struct
>>>>>> inode *inode)
>>>>>> is_inode_flag_set(inode, FI_COMPRESSED_FILE);
>>>>>> }
>>>>>> +static inline bool f2fs_is_compressed_file(struct inode *inode)
>>>>>> +{
>>>>>> + int compress_mode =
>>>>>> F2FS_OPTION(F2FS_I_SB(inode)).compress_mode;
>>>>>> +
>>>>>> + if (compress_mode == COMPR_MODE_FS)
>>>>>> + return f2fs_compressed_file(inode);
>>>>>> + else if (atomic_read(&F2FS_I(inode)->i_compr_blocks) ||
>>>>>
>>>>> Should check dirty page as well? i_compr_blocks may increase after
>>>>> data writeback.
>>>>>
>>>> IIUC, in COMPR_MODE_USER mode, i_compr_blocks will only be updated
>>>> when
>>>> FI_ENABLE_COMPRESS is enabled.
>>>>
>>>> If FI_ENABLE_COMPRESS is not enabled, i_compr_blocks will never be
>>>> updated after data writeback.
>>>>
>>>> So there is no need to additionally judge whether there is a dirty
>>>> page?
>>>
>>> Oh, user mode, that's correct.
>>>
>>> If we allow dio/aio on compress file, it needs to consider race case in
>>> between aio and ioc_compress_file.
>>
>>
>> The inode_lock is already held in f2fs_file_write_iter and
>> f2fs_ioc_compress_file, I guess this is enough?
>>
>> What else?
>
> aio may complete outside inode lock, so it needs to call inode_dio_wait()
> in f2fs_ioc_compress_file() to avoid the race case?
How about adding this below？
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f45d05c13ae5..5021d13e788b 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -4146,6 +4146,10 @@ static int f2fs_ioc_compress_file(struct file *filp)
goto out;
}
+ /* avoid race case between aio and ioc_compress_file */
+ if (F2FS_OPTION(sbi).compress_mode == COMPR_MODE_USER)
+ inode_dio_wait(inode);
+
ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
if (ret)
goto out;
Thanks,
>
> Thanks,
>
>>
>>
>> 4691 static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct
>> iov_iter *from)
>> 4692 {
>> 4693 struct inode *inode = file_inode(iocb->ki_filp);
>> 4694 const loff_t orig_pos = iocb->ki_pos;
>> 4695 const size_t orig_count = iov_iter_count(from);
>> 4696 loff_t target_size;
>> 4697 bool dio;
>> 4698 bool may_need_sync = true;
>> 4699 int preallocated;
>> 4700 ssize_t ret;
>> 4701
>> 4702 if (unlikely(f2fs_cp_error(F2FS_I_SB(inode)))) {
>> 4703 ret = -EIO;
>> 4704 goto out;
>> 4705 }
>> 4706
>> 4707 if (!f2fs_is_compress_backend_ready(inode)) {
>> 4708 ret = -EOPNOTSUPP;
>> 4709 goto out;
>> 4710 }
>> 4711
>> 4712 if (iocb->ki_flags & IOCB_NOWAIT) {
>> 4713 if (!inode_trylock(inode)) {
>> 4714 ret = -EAGAIN;
>> 4715 goto out;
>> 4716 }
>> 4717 } else {
>> 4718 inode_lock(inode);
>> 4719 }
>>
>>
>> 4115 static int f2fs_ioc_compress_file(struct file *filp)
>> 4116 {
>> 4117 struct inode *inode = file_inode(filp);
>> 4118 struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> 4119 pgoff_t page_idx = 0, last_idx;
>> 4120 unsigned int blk_per_seg = sbi->blocks_per_seg;
>> 4121 int cluster_size = F2FS_I(inode)->i_cluster_size;
>> 4122 int count, ret;
>> 4123
>> 4124 if (!f2fs_sb_has_compression(sbi) ||
>> 4125 F2FS_OPTION(sbi).compress_mode !=
>> COMPR_MODE_USER)
>> 4126 return -EOPNOTSUPP;
>> 4127
>> 4128 if (!(filp->f_mode & FMODE_WRITE))
>> 4129 return -EBADF;
>> 4130
>> 4131 if (!f2fs_compressed_file(inode))
>> 4132 return -EINVAL;
>> 4133
>> 4134 f2fs_balance_fs(sbi, true);
>> 4135
>> 4136 file_start_write(filp);
>> 4137 inode_lock(inode);
>>
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>>> Thanks,
>>>>>
>>>>>> + is_inode_flag_set(inode, FI_COMPRESS_RELEASED) ||
>>>>>> + is_inode_flag_set(inode, FI_ENABLE_COMPRESS))
>>>>>> + return true;
>>>>>> +
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>> static inline bool f2fs_need_compress_data(struct inode *inode)
>>>>>> {
>>>>>> int compress_mode =
>>>>>> F2FS_OPTION(F2FS_I_SB(inode)).compress_mode;
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index 74ecc9e20619..0698129b2165 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -821,7 +821,7 @@ static bool f2fs_force_buffered_io(struct inode
>>>>>> *inode, int rw)
>>>>>> return true;
>>>>>> if (fsverity_active(inode))
>>>>>> return true;
>>>>>> - if (f2fs_compressed_file(inode))
>>>>>> + if (f2fs_is_compressed_file(inode))
>>>>>> return true;
>>>>>> /* disallow direct IO if any of devices has unaligned
>>>>>> blksize */
Powered by blists - more mailing lists