| 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: <5A08F9ED.6070309@gmail.com>
Date: Mon, 13 Nov 2017 10:48:29 +0900
From: Hyunchul Lee <hyc.lee@...il.com>
To: Chao Yu <yuchao0@...wei.com>, Chao Yu <chao@...nel.org>,
Jaegeuk Kim <jaegeuk@...nel.org>
CC: kernel-team@....com, Hyunchul Lee <cheol.lee@....com>,
linux-kernel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [RFC PATHC 2/2] f2fs: apply write hints to select
the type of segment for direct write
On 11/13/2017 10:24 AM, Chao Yu wrote:
> On 2017/11/13 8:07, Hyunchul Lee wrote:
>> On 11/11/2017 09:38 AM, Chao Yu wrote:
>>> On 2017/11/9 13:51, Hyunchul Lee wrote:
>>>> From: Hyunchul Lee <cheol.lee@....com>
>>>>
>>>> Select the type of the segment using write hints, when blocks are
>>>> allocated for direct write.
>>>>
>>>> There are unhandled corner cases. Hints are not applied in
>>>> in-place update. And if the blocks of a file is not pre-allocated
>>>> because of the invalid user buffer, CURSEG_WARM_DATA segment will
>>>> be selected.
>>>>
>>>> Signed-off-by: Hyunchul Lee <cheol.lee@....com>
>>>> ---
>>>> fs/f2fs/data.c | 101 ++++++++++++++++++++++++++++++++++-----------------------
>>>> fs/f2fs/f2fs.h | 1 +
>>>> 2 files changed, 61 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 36b5352..d06048a 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -783,7 +783,7 @@ struct page *get_new_data_page(struct inode *inode,
>>>> return page;
>>>> }
>>>>
>>>> -static int __allocate_data_block(struct dnode_of_data *dn)
>>>> +static int __allocate_data_block(struct dnode_of_data *dn, int seg_type)
>>>> {
>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(dn->inode);
>>>> struct f2fs_summary sum;
>>>> @@ -808,7 +808,7 @@ static int __allocate_data_block(struct dnode_of_data *dn)
>>>> set_summary(&sum, dn->nid, dn->ofs_in_node, ni.version);
>>>>
>>>> allocate_data_block(sbi, NULL, dn->data_blkaddr, &dn->data_blkaddr,
>>>> - &sum, CURSEG_WARM_DATA, NULL, false);
>>>> + &sum, seg_type, NULL, false);
>>>> set_data_blkaddr(dn);
>>>>
>>>> /* update i_size */
>>>> @@ -827,42 +827,6 @@ static inline bool __force_buffered_io(struct inode *inode, int rw)
>>>> F2FS_I_SB(inode)->s_ndevs);
>>>> }
>>>>
>>>> -int f2fs_preallocate_blocks(struct kiocb *iocb, struct iov_iter *from)
>>>> -{
>>>> - struct inode *inode = file_inode(iocb->ki_filp);
>>>> - struct f2fs_map_blocks map;
>>>> - int err = 0;
>>>> -
>>>> - if (is_inode_flag_set(inode, FI_NO_PREALLOC))
>>>> - return 0;
>>>> -
>>>> - map.m_lblk = F2FS_BLK_ALIGN(iocb->ki_pos);
>>>> - map.m_len = F2FS_BYTES_TO_BLK(iocb->ki_pos + iov_iter_count(from));
>>>> - if (map.m_len > map.m_lblk)
>>>> - map.m_len -= map.m_lblk;
>>>> - else
>>>> - map.m_len = 0;
>>>> -
>>>> - map.m_next_pgofs = NULL;
>>>> -
>>>> - if (iocb->ki_flags & IOCB_DIRECT) {
>>>> - err = f2fs_convert_inline_inode(inode);
>>>> - if (err)
>>>> - return err;
>>>> - return f2fs_map_blocks(inode, &map, 1,
>>>> - __force_buffered_io(inode, WRITE) ?
>>>> - F2FS_GET_BLOCK_PRE_AIO :
>>>> - F2FS_GET_BLOCK_PRE_DIO);
>>>> - }
>>>> - if (iocb->ki_pos + iov_iter_count(from) > MAX_INLINE_DATA(inode)) {
>>>> - err = f2fs_convert_inline_inode(inode);
>>>> - if (err)
>>>> - return err;
>>>> - }
>>>> - if (!f2fs_has_inline_data(inode))
>>>> - return f2fs_map_blocks(inode, &map, 1, F2FS_GET_BLOCK_PRE_AIO);
>>>> - return err;
>>>> -}
>>>>
>>>> static inline void __do_map_lock(struct f2fs_sb_info *sbi, int flag, bool lock)
>>>> {
>>>> @@ -888,8 +852,8 @@ static inline void __do_map_lock(struct f2fs_sb_info *sbi, int flag, bool lock)
>>>> * b. do not use extent cache for better performance
>>>> * c. give the block addresses to blockdev
>>>> */
>>>> -int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>>>> - int create, int flag)
>>>> +static int __f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>>>> + int create, int flag, int seg_type)
>>>> {
>>>> unsigned int maxblocks = map->m_len;
>>>> struct dnode_of_data dn;
>>>> @@ -957,7 +921,12 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>>>> last_ofs_in_node = dn.ofs_in_node;
>>>> }
>>>> } else {
>>>> - err = __allocate_data_block(&dn);
>>>> + /* if this inode is marked with FI_NO_PREALLOC,
>>>> + * @seg_type is NO_CHECK_TYPE
>>>> + */
>>>> + if (seg_type == NO_CHECK_TYPE)
>>>> + seg_type = CURSEG_WARM_DATA;
>>>> + err = __allocate_data_block(&dn, seg_type);
>>>
>>> We need to use inode.i_write_hint instead of ki_hint passed from file.f_write_hint?
>>>
>>
>> The following commit says to use file.f_write_hint if it is available.
>> "c75b1d9 fs: add fcntl() interface for setting/getting write life time hints"
>
> Oh, yes, f_write_hint is recommended. So, I'm OK with this.
>
> One left question as below.
>
>>
>> And ki_hint is assiged to file.f_write_hint or inode.i_write_hint
>> by file_write_hint() in init_sync_kiocb().
>> So, I think we need to use ki_hint instead of inode.i_write_hint.
>>
>> Thanks
>>
>>> Thanks,
>>>
>>>> if (!err)
>>>> set_inode_flag(inode, FI_APPEND_WRITE);
>>>> }
>>>> @@ -1048,6 +1017,51 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>>>> return err;
>>>> }
>>>>
>>>> +int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>>>> + int create, int flag)
>>>> +{
>>>> + return __f2fs_map_blocks(inode, map, create, flag, NO_CHECK_TYPE);
>>>> +}
>>>> +
>>>> +int f2fs_preallocate_blocks(struct kiocb *iocb, struct iov_iter *from)
>>>> +{
>>>> + struct inode *inode = file_inode(iocb->ki_filp);
>>>> + struct f2fs_map_blocks map;
>>>> + int err = 0;
>>>> +
>>>> + if (is_inode_flag_set(inode, FI_NO_PREALLOC))
>>>> + return 0;
>>>> +
>>>> + map.m_lblk = F2FS_BLK_ALIGN(iocb->ki_pos);
>>>> + map.m_len = F2FS_BYTES_TO_BLK(iocb->ki_pos + iov_iter_count(from));
>>>> + if (map.m_len > map.m_lblk)
>>>> + map.m_len -= map.m_lblk;
>>>> + else
>>>> + map.m_len = 0;
>>>> +
>>>> + map.m_next_pgofs = NULL;
>>>> +
>>>> + if (iocb->ki_flags & IOCB_DIRECT) {
>>>> + err = f2fs_convert_inline_inode(inode);
>>>> + if (err)
>>>> + return err;
>>>> + return __f2fs_map_blocks(inode, &map, 1,
>>>> + __force_buffered_io(inode, WRITE) ?
>>>> + F2FS_GET_BLOCK_PRE_AIO :
>>>> + F2FS_GET_BLOCK_PRE_DIO,
>>>> + rw_hint_to_seg_type(iocb->ki_hint));
>>>> + }
>>>> + if (iocb->ki_pos + iov_iter_count(from) > MAX_INLINE_DATA(inode)) {
>>>> + err = f2fs_convert_inline_inode(inode);
>>>> + if (err)
>>>> + return err;
>>>> + }
>>>> + if (!f2fs_has_inline_data(inode))
>>>> + return f2fs_map_blocks(inode, &map, 1, F2FS_GET_BLOCK_PRE_AIO);
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>>> static int __get_data_block(struct inode *inode, sector_t iblock,
>>>> struct buffer_head *bh, int create, int flag,
>>>> pgoff_t *next_pgofs)
>>>> @@ -2082,6 +2096,11 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>>>>
>>>> trace_f2fs_direct_IO_enter(inode, offset, count, rw);
>>>>
>>>> + /* This is for avoiding the situation that the data of a segment is
>>>> + * passed down to devices with different hints
>>>> + */
>>>> + iocb->ki_hint = WRITE_LIFE_NOT_SET;
>
> Why we need to change this? I'm not sure this will be used later in somewhere,
> if it's not necessary, how about keeping it as it is?
>
blkdev_direct_IO will pass down this hint to block layer. But we need to control
the hint before this.
So I think that after we implement this control, passing down the hint is better.
iocb->ki_hint needs to be re-assigned to the orignal hint after the write.
I will handle this.
Thanks
> Thanks,
>
>>>> +
>>>> down_read(&F2FS_I(inode)->dio_rwsem[rw]);
>>>> err = blockdev_direct_IO(iocb, inode, iter, get_data_block_dio);
>>>> up_read(&F2FS_I(inode)->dio_rwsem[rw]);
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index 4b4a72f..9be5658 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -2562,6 +2562,7 @@ int lookup_journal_in_cursum(struct f2fs_journal *journal, int type,
>>>> void destroy_segment_manager(struct f2fs_sb_info *sbi);
>>>> int __init create_segment_manager_caches(void);
>>>> void destroy_segment_manager_caches(void);
>>>> +int rw_hint_to_seg_type(enum rw_hint hint);
>>>>
>>>> /*
>>>> * checkpoint.c
>>>>
>>>
>>
>> .
>>
>
>
Powered by blists - more mailing lists