On 01/25/2018 05:01 PM, Chao Yu wrote:
> Hi Hyunchul,
>
> On 2018/1/25 10:47, Hyunchul Lee wrote:
>> Hi Chao,
>>
>> On 01/25/2018 12:32 AM, Chao Yu wrote:
>>> On 2018/1/22 18:49, Hyunchul Lee wrote:
>>>> From: Hyunchul Lee <cheol.lee@xxxxxxx>
>>>>
>>>> Add the 'whint_mode' mount option that controls which write
>>>> hints are passed down to block layer. There are "off" and
>>>> "user-based" mode. The default mode is "off".
>>>>
>>>> 1) whint_mode=user-based. F2FS tries to pass down hints given
>>>> by users.
>>>
>>> Minor,
>>>
>>> 1) whint_mode=off. F2FS only passes down WRITE_LIFE_NOT_SET
>>>
>>> 2) whint_mode=user-based. F2FS tries to pass down hints given by users.
>>> ...
>>>
>>
>> Okay, I will reflect this.
>>
>>> How about changing all comments and codes with above order?
>>>
>>>>
>>>> User F2FS Block
>>>> ---- ---- -----
>>>> META WRITE_LIFE_NOT_SET
>>>> HOT_NODE "
>>>> WARM_NODE "
>>>> COLD_NODE "
>>>> ioctl(COLD) COLD_DATA WRITE_LIFE_EXTREME
>>>> extension list " "
>>>>
>>>> -- buffered io
>>>> WRITE_LIFE_EXTREME COLD_DATA WRITE_LIFE_EXTREME
>>>> WRITE_LIFE_SHORT HOT_DATA WRITE_LIFE_SHORT
>>>> WRITE_LIFE_NOT_SET WARM_DATA WRITE_LIFE_NOT_SET
>>>> WRITE_LIFE_NONE " "
>>>> WRITE_LIFE_MEDIUM " "
>>>> WRITE_LIFE_LONG " "
>>>>
>>>> -- direct io
>>>> WRITE_LIFE_EXTREME COLD_DATA WRITE_LIFE_EXTREME
>>>> WRITE_LIFE_SHORT HOT_DATA WRITE_LIFE_SHORT
>>>> WRITE_LIFE_NOT_SET WARM_DATA WRITE_LIFE_NOT_SET
>>>> WRITE_LIFE_NONE " WRITE_LIFE_NONE
>>>> WRITE_LIFE_MEDIUM " WRITE_LIFE_MEDIUM
>>>> WRITE_LIFE_LONG " WRITE_LIFE_LONG
>>>>
>>>> 2) whint_mode=off. F2FS only passes down WRITE_LIFE_NOT_SET.
>>>>
>>>> Many thanks to Chao Yu and Jaegeuk Kim for comments to
>>>> implement this patch.
>>>>
>>>> Signed-off-by: Hyunchul Lee <cheol.lee@xxxxxxx>
>>>> ---
>>>> fs/f2fs/data.c | 27 ++++++++++++++++++++-----
>>>> fs/f2fs/f2fs.h | 9 +++++++++
>>>> fs/f2fs/segment.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> fs/f2fs/super.c | 24 +++++++++++++++++++++-
>>>> 4 files changed, 113 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 6cba74e..c76ddc2 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -175,15 +175,22 @@ static bool __same_bdev(struct f2fs_sb_info *sbi,
>>>> */
>>>> static struct bio *__bio_alloc(struct f2fs_sb_info *sbi, block_t blk_addr,
>>>> struct writeback_control *wbc,
>>>> - int npages, bool is_read)
>>>> + int npages, bool is_read,
>>>> + enum page_type type, enum temp_type temp)
>>>> {
>>>> struct bio *bio;
>>>>
>>>> bio = f2fs_bio_alloc(sbi, npages, true);
>>>>
>>>> f2fs_target_device(sbi, blk_addr, bio);
>>>> - bio->bi_end_io = is_read ? f2fs_read_end_io : f2fs_write_end_io;
>>>> - bio->bi_private = is_read ? NULL : sbi;
>>>> + if (is_read) {
>>>> + bio->bi_end_io = f2fs_read_end_io;
>>>> + bio->bi_private = NULL;
>>>> + } else {
>>>> + bio->bi_end_io = f2fs_write_end_io;
>>>> + bio->bi_private = sbi;
>>>> + bio->bi_write_hint = io_type_to_rw_hint(sbi, type, temp);
>>>> + }
>>>> if (wbc)
>>>> wbc_init_bio(wbc, bio);
>>>>
>>>> @@ -382,7 +389,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>>>>
>>>> /* Allocate a new bio */
>>>> bio = __bio_alloc(fio->sbi, fio->new_blkaddr, fio->io_wbc,
>>>> - 1, is_read_io(fio->op));
>>>> + 1, is_read_io(fio->op), fio->type, fio->temp);
>>>>
>>>> if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
>>>> bio_put(bio);
>>>> @@ -445,7 +452,8 @@ int f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>> goto out_fail;
>>>> }
>>>> io->bio = __bio_alloc(sbi, fio->new_blkaddr, fio->io_wbc,
>>>> - BIO_MAX_PAGES, false);
>>>> + BIO_MAX_PAGES, false,
>>>> + fio->type, fio->temp);
>>>> io->fio = *fio;
>>>> }
>>>>
>>>> @@ -2287,10 +2295,12 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>>>> {
>>>> struct address_space *mapping = iocb->ki_filp->f_mapping;
>>>> struct inode *inode = mapping->host;
>>>> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>> size_t count = iov_iter_count(iter);
>>>> loff_t offset = iocb->ki_pos;
>>>> int rw = iov_iter_rw(iter);
>>>> int err;
>>>> + enum rw_hint hint;
>>>>
>>>> err = check_direct_IO(inode, iter, offset);
>>>> if (err)
>>>> @@ -2301,11 +2311,18 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>>>>
>>>> trace_f2fs_direct_IO_enter(inode, offset, count, rw);
>>>>
>>>> + if (rw == WRITE && sbi->whint_mode == WHINT_MODE_OFF) {
>>>> + hint = iocb->ki_hint;
>>>> + iocb->ki_hint = WRITE_LIFE_NOT_SET;
>>>> + }
>>>
>>> In f2fs_preallocate_blocks, we should wrap original iocb->ki_hint with
>>> WRITE_LIFE_NOT_SET under WHINT_MODE_OFF mode?
>>>
>> In f2fs_preallocate_blocks, we should keep the original iocb->ki_hint to
>> select proper segments. So I think f2fs_direct_IO is the best place
>> before submiting io.
>
> Oh, right.
>
> How about using temporary variable to store sbi->whint_mode? so that we
> won't be affected by sbi->whint_mode changing.
>
Yes, We need this.
> And could you please check to make sure all .temp assignment being covered?
>
I have checked all .temp assignments. But for read operations, this variable
is not used to determine write hints. so it is not initialized in this case.
Thanks,
> Thanks,
>
>>
>> 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]);
>>>>
>>>> if (rw == WRITE) {
>>>> + if (sbi->whint_mode == WHINT_MODE_OFF)
>>>> + iocb->ki_hint = hint;
>>>> if (err > 0) {
>>>> f2fs_update_iostat(F2FS_I_SB(inode), APP_DIRECT_IO,
>>>> err);
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index b7ba496..d7c2797 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -1035,6 +1035,11 @@ enum {
>>>> MAX_TIME,
>>>> };
>>>>
>>>> +enum {
>>>> + WHINT_MODE_OFF, /* not pass down write hints */
>>>> + WHINT_MODE_USER, /* try to pass down hints given by users */
>>>> +};
>>>> +
>>>> struct f2fs_sb_info {
>>>> struct super_block *sb; /* pointer to VFS super block */
>>>> struct proc_dir_entry *s_proc; /* proc entry */
>>>> @@ -1218,6 +1223,8 @@ struct f2fs_sb_info {
>>>> char *s_qf_names[MAXQUOTAS];
>>>> int s_jquota_fmt; /* Format of quota to use */
>>>> #endif
>>>> + /* For which write hints are passed down to block layer */
>>>> + int whint_mode;
>>>> };
>>>>
>>>> #ifdef CONFIG_F2FS_FAULT_INJECTION
>>>> @@ -2766,6 +2773,8 @@ int lookup_journal_in_cursum(struct f2fs_journal *journal, int type,
>>>> int __init create_segment_manager_caches(void);
>>>> void destroy_segment_manager_caches(void);
>>>> int rw_hint_to_seg_type(enum rw_hint hint);
>>>> +enum rw_hint io_type_to_rw_hint(struct f2fs_sb_info *sbi, enum page_type type,
>>>> + enum temp_type temp);
>>>>
>>>> /*
>>>> * checkpoint.c
>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>> index e5739ce..8bc1fc1 100644
>>>> --- a/fs/f2fs/segment.c
>>>> +++ b/fs/f2fs/segment.c
>>>> @@ -2458,6 +2458,62 @@ int rw_hint_to_seg_type(enum rw_hint hint)
>>>> }
>>>> }
>>>>
>>>> +/* This returns write hints for each segment type. This hints will be
>>>> + * passed down to block layer. There are 2 mapping tables which depend on
>>>> + * the mount option 'whint'.
>>>> + *
>>>> + * 1) whint_mode=user-based. F2FS tries to pass down hints given by users.
>>>> + *
>>>> + * User F2FS Block
>>>> + * ---- ---- -----
>>>> + * META WRITE_LIFE_NOT_SET
>>>> + * HOT_NODE "
>>>> + * WARM_NODE "
>>>> + * COLD_NODE "
>>>> + * ioctl(COLD) COLD_DATA WRITE_LIFE_EXTREME
>>>> + * extension list " "
>>>> + *
>>>> + * -- buffered io
>>>> + * WRITE_LIFE_EXTREME COLD_DATA WRITE_LIFE_EXTREME
>>>> + * WRITE_LIFE_SHORT HOT_DATA WRITE_LIFE_SHORT
>>>> + * WRITE_LIFE_NOT_SET WARM_DATA WRITE_LIFE_NOT_SET
>>>> + * WRITE_LIFE_NONE " "
>>>> + * WRITE_LIFE_MEDIUM " "
>>>> + * WRITE_LIFE_LONG " "
>>>> + *
>>>> + * -- direct io
>>>> + * WRITE_LIFE_EXTREME COLD_DATA WRITE_LIFE_EXTREME
>>>> + * WRITE_LIFE_SHORT HOT_DATA WRITE_LIFE_SHORT
>>>> + * WRITE_LIFE_NOT_SET WARM_DATA WRITE_LIFE_NOT_SET
>>>> + * WRITE_LIFE_NONE " WRITE_LIFE_NONE
>>>> + * WRITE_LIFE_MEDIUM " WRITE_LIFE_MEDIUM
>>>> + * WRITE_LIFE_LONG " WRITE_LIFE_LONG
>>>> + *
>>>> + * 2) whint_mode=off. F2FS only passes down WRITE_LIFE_NOT_SET.
>>>> + *
>>>> + */
>>>> +
>>>> +enum rw_hint io_type_to_rw_hint(struct f2fs_sb_info *sbi,
>>>> + enum page_type type, enum temp_type temp)
>>>> +{
>>>> + if (sbi->whint_mode == WHINT_MODE_USER) {
>>>> + if (type == DATA) {
>>>> + switch (temp) {
>>>> + case COLD:
>>>> + return WRITE_LIFE_EXTREME;
>>>> + case HOT:
>>>> + return WRITE_LIFE_SHORT;
>>>> + default:
>>>> + return WRITE_LIFE_NOT_SET;
>>>> + }
>>>> + } else {
>>>> + return WRITE_LIFE_NOT_SET;
>>>> + }
>>>> + } else {
>>>> + return WRITE_LIFE_NOT_SET;
>>>> + }
>>>> +}
>>>> +
>>>> static int __get_segment_type_2(struct f2fs_io_info *fio)
>>>> {
>>>> if (fio->type == DATA)
>>>> @@ -2645,6 +2701,7 @@ void write_meta_page(struct f2fs_sb_info *sbi, struct page *page,
>>>> struct f2fs_io_info fio = {
>>>> .sbi = sbi,
>>>> .type = META,
>>>> + .temp = HOT,
>>>
>>> Could you check to make sure all .temp being covered?
>>>
>>>> .op = REQ_OP_WRITE,
>>>> .op_flags = REQ_SYNC | REQ_META | REQ_PRIO,
>>>> .old_blkaddr = page->index,
>>>> @@ -2693,6 +2750,8 @@ int rewrite_data_page(struct f2fs_io_info *fio)
>>>> int err;
>>>>
>>>> fio->new_blkaddr = fio->old_blkaddr;
>>>> + /* i/o temperature is needed for passing down write hints */
>>>> + __get_segment_type(fio);
>>>> stat_inc_inplace_blocks(fio->sbi);
>>>>
>>>> err = f2fs_submit_page_bio(fio);
>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>> index 8173ae6..9e22926 100644
>>>> --- a/fs/f2fs/super.c
>>>> +++ b/fs/f2fs/super.c
>>>> @@ -129,6 +129,7 @@ enum {
>>>> Opt_jqfmt_vfsold,
>>>> Opt_jqfmt_vfsv0,
>>>> Opt_jqfmt_vfsv1,
>>>> + Opt_whint,
>>>> Opt_err,
>>>> };
>>>>
>>>> @@ -182,6 +183,7 @@ enum {
>>>> {Opt_jqfmt_vfsold, "jqfmt=vfsold"},
>>>> {Opt_jqfmt_vfsv0, "jqfmt=vfsv0"},
>>>> {Opt_jqfmt_vfsv1, "jqfmt=vfsv1"},
>>>> + {Opt_whint, "whint_mode=%s"},
>>>> {Opt_err, NULL},
>>>> };
>>>>
>>>> @@ -679,6 +681,22 @@ static int parse_options(struct super_block *sb, char *options)
>>>> "quota operations not supported");
>>>> break;
>>>> #endif
>>>> + case Opt_whint:
>>>> + name = match_strdup(&args[0]);
>>>> + if (!name)
>>>> + return -ENOMEM;
>>>> + if (strlen(name) == 10 &&
>>>> + !strncmp(name, "user-based", 10)) {
>>>> + sbi->whint_mode = WHINT_MODE_USER;
>>>> + } else if (strlen(name) == 3 &&
>>>> + !strncmp(name, "off", 3)) {
>>>> + sbi->whint_mode = WHINT_MODE_OFF;
>>>> + } else {
>>>> + kfree(name);
>>>> + return -EINVAL;
>>>> + }
>>>> + kfree(name);
>>>> + break;
>>>> default:
>>>> f2fs_msg(sb, KERN_ERR,
>>>> "Unrecognized mount option \"%s\" or missing value",
>>>> @@ -1225,6 +1243,8 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root)
>>>> seq_puts(seq, ",prjquota");
>>>> #endif
>>>> f2fs_show_quota_options(seq, sbi->sb);
>>>> + if (sbi->whint_mode == WHINT_MODE_USER)
>>>> + seq_printf(seq, ",whint_mode=%s", "user-based");
>>>>
>>>> return 0;
>>>> }
>>>> @@ -1234,6 +1254,7 @@ static void default_options(struct f2fs_sb_info *sbi)
>>>> /* init some FS parameters */
>>>> sbi->active_logs = NR_CURSEG_TYPE;
>>>> sbi->inline_xattr_size = DEFAULT_INLINE_XATTR_ADDRS;
>>>> + sbi->whint_mode = WHINT_MODE_OFF;
>>>>
>>>> set_opt(sbi, BG_GC);
>>>> set_opt(sbi, INLINE_XATTR);
>>>> @@ -1274,6 +1295,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
>>>> bool need_restart_gc = false;
>>>> bool need_stop_gc = false;
>>>> bool no_extent_cache = !test_opt(sbi, EXTENT_CACHE);
>>>> + int old_whint_mode = sbi->whint_mode;
>>>> #ifdef CONFIG_F2FS_FAULT_INJECTION
>>>> struct f2fs_fault_info ffi = sbi->fault_info;
>>>> #endif
>>>> @@ -1373,7 +1395,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
>>>> need_stop_gc = true;
>>>> }
>>>>
>>>> - if (*flags & SB_RDONLY) {
>>>> + if (*flags & SB_RDONLY || sbi->whint_mode != old_whint_mode) {
>>>> writeback_inodes_sb(sb, WB_REASON_SYNC);
>>>> sync_inodes_sb(sb);
>>>>
>>>>
>>>
>>
>> .
>>
>
>
