Messages in this thread Patch in this message | | | Date | Mon, 29 Jan 2018 10:49:07 +0900 | From | Hyunchul Lee <> | Subject | Re: [f2fs-dev] [PATCH 1/3] f2fs: support passing down write hints given by users to block layer |
| |
On 01/26/2018 11:10 AM, Chao Yu wrote: > On 2018/1/26 7:46, Hyunchul Lee wrote: >> 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@lge.com> >>>>>> >>>>>> 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@lge.com> >>>>>> --- >>>>>> 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 > > I've checked that too, there is nothing was missing. > > One more thing, our whint_mode is completely based on active six type logs, > once some of them is close due to user configuration, write IO hint will be > messed up, so what about just enabling WHINT_MODE_OFF mode if active log > number is two or four? >
Yes, I tought this is reasonable. write hints are not useful in two or four.
> And we need to adjust rw_hint_to_seg_type to be aware of active log number? >
We does not have any choices in the two, But we can adjust the function like the following in the four.
* in 4 active logs F2FS --------------------------------------- directory HOT_DATA WRITE_LIFE_SHORT HOT_DATA others COLD_DATA
>> is not used to determine write hints. so it is not initialized in this case. > > I think that's OK. > > Thanks, > >> >> 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); >>>>>> >>>>>> >>>>> >>>> >>>> . >>>> >>> >>> >> >> . >> > >
| |