Messages in this thread | | | Date | Tue, 31 Jan 2023 09:58:12 +0800 | Subject | Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx() | From | Chao Yu <> |
| |
On 2023/1/31 6:23, Jaegeuk Kim wrote: > On 01/30, Jaegeuk Kim wrote: >> On 01/28, Chao Yu wrote: >>> On 2023/1/5 12:22, Yangtao Li wrote: >>>> Convert to use iostat_lat_type as parameter instead of raw number. >>>> BTW, move NUM_PREALLOC_IOSTAT_CTXS to the header file, and rename >>>> iotype to page_type to match the definition. >>>> >>>> Signed-off-by: Yangtao Li <frank.li@vivo.com> >>>> --- >>>> fs/f2fs/data.c | 5 +++-- >>>> fs/f2fs/iostat.c | 34 +++++++++++----------------------- >>>> fs/f2fs/iostat.h | 19 ++++++++++--------- >>>> 3 files changed, 24 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>>> index c940da1c540f..4e8fd5697c42 100644 >>>> --- a/fs/f2fs/data.c >>>> +++ b/fs/f2fs/data.c >>>> @@ -292,7 +292,7 @@ static void f2fs_read_end_io(struct bio *bio) >>>> struct bio_post_read_ctx *ctx; >>>> bool intask = in_task(); >>>> - iostat_update_and_unbind_ctx(bio, 0); >>>> + iostat_update_and_unbind_ctx(bio, READ_IO); >>>> ctx = bio->bi_private; >>>> if (time_to_inject(sbi, FAULT_READ_IO)) >>>> @@ -330,7 +330,8 @@ static void f2fs_write_end_io(struct bio *bio) >>>> struct bio_vec *bvec; >>>> struct bvec_iter_all iter_all; >>>> - iostat_update_and_unbind_ctx(bio, 1); >>>> + iostat_update_and_unbind_ctx(bio, bio->bi_opf & REQ_SYNC ? WRITE_SYNC_IO : >>>> + WRITE_ASYNC_IO); >>> >>> We can use op_is_write(bio_op(bio)) to check IO's rw type, why not just >>> passing bio arguement, and parse rw/sync types from bio inside >>> iostat_update_and_unbind_ctx(), it can avoid passing unneeded arguements. >> >> Chao, let's write another patch to clean up, if you're interested in. > > Ok, it seems you need to add this comment in v3 that Yangtao sent.
Oh, I missed last patch, I've added comments on v3.
Thanks,
> >> >>> >>> Thanks, >>> >>>> sbi = bio->bi_private; >>>> if (time_to_inject(sbi, FAULT_WRITE_IO)) >>>> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c >>>> index 59c72f92191a..20944c4a683a 100644 >>>> --- a/fs/f2fs/iostat.c >>>> +++ b/fs/f2fs/iostat.c >>>> @@ -14,7 +14,6 @@ >>>> #include "iostat.h" >>>> #include <trace/events/f2fs.h> >>>> -#define NUM_PREALLOC_IOSTAT_CTXS 128 >>>> static struct kmem_cache *bio_iostat_ctx_cache; >>>> static mempool_t *bio_iostat_ctx_pool; >>>> @@ -210,49 +209,38 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi, struct inode *inode, >>>> } >>>> static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx, >>>> - int rw, bool is_sync) >>>> + enum iostat_lat_type type) >>>> { >>>> unsigned long ts_diff; >>>> - unsigned int iotype = iostat_ctx->type; >>>> + unsigned int page_type = iostat_ctx->type; >>>> struct f2fs_sb_info *sbi = iostat_ctx->sbi; >>>> struct iostat_lat_info *io_lat = sbi->iostat_io_lat; >>>> - int idx; >>>> unsigned long flags; >>>> if (!sbi->iostat_enable) >>>> return; >>>> ts_diff = jiffies - iostat_ctx->submit_ts; >>>> - if (iotype >= META_FLUSH) >>>> - iotype = META; >>>> - >>>> - if (rw == 0) { >>>> - idx = READ_IO; >>>> - } else { >>>> - if (is_sync) >>>> - idx = WRITE_SYNC_IO; >>>> - else >>>> - idx = WRITE_ASYNC_IO; >>>> - } >>>> + if (page_type >= META_FLUSH) >>>> + page_type = META; >>>> spin_lock_irqsave(&sbi->iostat_lat_lock, flags); >>>> - io_lat->sum_lat[idx][iotype] += ts_diff; >>>> - io_lat->bio_cnt[idx][iotype]++; >>>> - if (ts_diff > io_lat->peak_lat[idx][iotype]) >>>> - io_lat->peak_lat[idx][iotype] = ts_diff; >>>> + io_lat->sum_lat[type][page_type] += ts_diff; >>>> + io_lat->bio_cnt[type][page_type]++; >>>> + if (ts_diff > io_lat->peak_lat[type][page_type]) >>>> + io_lat->peak_lat[type][page_type] = ts_diff; >>>> spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags); >>>> } >>>> -void iostat_update_and_unbind_ctx(struct bio *bio, int rw) >>>> +void iostat_update_and_unbind_ctx(struct bio *bio, enum iostat_lat_type type) >>>> { >>>> struct bio_iostat_ctx *iostat_ctx = bio->bi_private; >>>> - bool is_sync = bio->bi_opf & REQ_SYNC; >>>> - if (rw == 0) >>>> + if (type == READ_IO) >>>> bio->bi_private = iostat_ctx->post_read_ctx; >>>> else >>>> bio->bi_private = iostat_ctx->sbi; >>>> - __update_iostat_latency(iostat_ctx, rw, is_sync); >>>> + __update_iostat_latency(iostat_ctx, type); >>>> mempool_free(iostat_ctx, bio_iostat_ctx_pool); >>>> } >>>> diff --git a/fs/f2fs/iostat.h b/fs/f2fs/iostat.h >>>> index 2c048307b6e0..1f827a2fe6b2 100644 >>>> --- a/fs/f2fs/iostat.h >>>> +++ b/fs/f2fs/iostat.h >>>> @@ -8,20 +8,21 @@ >>>> struct bio_post_read_ctx; >>>> +enum iostat_lat_type { >>>> + READ_IO = 0, >>>> + WRITE_SYNC_IO, >>>> + WRITE_ASYNC_IO, >>>> + MAX_IO_TYPE, >>>> +}; >>>> + >>>> #ifdef CONFIG_F2FS_IOSTAT >>>> +#define NUM_PREALLOC_IOSTAT_CTXS 128 >>>> #define DEFAULT_IOSTAT_PERIOD_MS 3000 >>>> #define MIN_IOSTAT_PERIOD_MS 100 >>>> /* maximum period of iostat tracing is 1 day */ >>>> #define MAX_IOSTAT_PERIOD_MS 8640000 >>>> -enum { >>>> - READ_IO, >>>> - WRITE_SYNC_IO, >>>> - WRITE_ASYNC_IO, >>>> - MAX_IO_TYPE, >>>> -}; >>>> - >>>> struct iostat_lat_info { >>>> unsigned long sum_lat[MAX_IO_TYPE][NR_PAGE_TYPE]; /* sum of io latencies */ >>>> unsigned long peak_lat[MAX_IO_TYPE][NR_PAGE_TYPE]; /* peak io latency */ >>>> @@ -57,7 +58,7 @@ static inline struct bio_post_read_ctx *get_post_read_ctx(struct bio *bio) >>>> return iostat_ctx->post_read_ctx; >>>> } >>>> -extern void iostat_update_and_unbind_ctx(struct bio *bio, int rw); >>>> +extern void iostat_update_and_unbind_ctx(struct bio *bio, enum iostat_lat_type type); >>>> extern void iostat_alloc_and_bind_ctx(struct f2fs_sb_info *sbi, >>>> struct bio *bio, struct bio_post_read_ctx *ctx); >>>> extern int f2fs_init_iostat_processing(void); >>>> @@ -67,7 +68,7 @@ extern void f2fs_destroy_iostat(struct f2fs_sb_info *sbi); >>>> #else >>>> static inline void f2fs_update_iostat(struct f2fs_sb_info *sbi, struct inode *inode, >>>> enum iostat_type type, unsigned long long io_bytes) {} >>>> -static inline void iostat_update_and_unbind_ctx(struct bio *bio, int rw) {} >>>> +static inline void iostat_update_and_unbind_ctx(struct bio *bio, enum iostat_lat_type type) {} >>>> static inline void iostat_alloc_and_bind_ctx(struct f2fs_sb_info *sbi, >>>> struct bio *bio, struct bio_post_read_ctx *ctx) {} >>>> static inline void iostat_update_submit_ctx(struct bio *bio, >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
| |