diff mbox series

f2fs: compress: don't force buffered io when in COMPR_MODE_USER mode

Message ID 20230609131555.56651-1-frank.li@vivo.com
State New
Headers
Series f2fs: compress: don't force buffered io when in COMPR_MODE_USER mode |

Commit Message

李扬韬 June 9, 2023, 1:15 p.m. UTC 
  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@vivo.com>
Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/f2fs.h | 14 ++++++++++++++
 fs/f2fs/file.c |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Chao Yu June 12, 2023, 2:38 p.m. UTC | #1 
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@vivo.com>
> Signed-off-by: Yangtao Li <frank.li@vivo.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.

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 */
李扬韬 June 13, 2023, 4:14 a.m. UTC | #2 
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@vivo.com>
>> Signed-off-by: Yangtao Li <frank.li@vivo.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?


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 */
Chao Yu June 19, 2023, 12:54 a.m. UTC | #3 
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@vivo.com>
>>> Signed-off-by: Yangtao Li <frank.li@vivo.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.

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 */
李扬韬 June 19, 2023, 3:11 a.m. UTC | #4 
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@vivo.com>
>>>> Signed-off-by: Yangtao Li <frank.li@vivo.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?


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 */
Chao Yu June 19, 2023, 4:04 a.m. UTC | #5 
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@vivo.com>
>>>>> Signed-off-by: Yangtao Li <frank.li@vivo.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?

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 */
李扬韬 June 19, 2023, 9:31 a.m. UTC | #6 
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@vivo.com>
>>>>>> Signed-off-by: Yangtao Li <frank.li@vivo.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 */
Chao Yu June 19, 2023, 2:13 p.m. UTC | #7 
On 2023/6/19 17:31, Yangtao Li wrote:
> 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@vivo.com>
>>>>>>> Signed-off-by: Yangtao Li <frank.li@vivo.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)

f2fs_ioc_compress_file() has already checked the mode?

Thanks,

> +               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 */
diff mbox series

Patch

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) ||
+		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 */