diff mbox series

f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()

Message ID 20230105042240.24738-1-frank.li@vivo.com
State New
Headers
Series f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx() |

Commit Message

李扬韬 Jan. 5, 2023, 4:22 a.m. UTC 
  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(-)

Comments

Dan Carpenter Jan. 13, 2023, 9:26 a.m. UTC | #1 
Hi Yangtao,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yangtao-Li/f2fs-use-iostat_lat_type-directly-as-a-parameter-in-the-iostat_update_and_unbind_ctx/20230105-122414
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git dev-test
patch link:    https://lore.kernel.org/r/20230105042240.24738-1-frank.li%40vivo.com
patch subject: [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
config: alpha-randconfig-m041-20230110
compiler: alpha-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>

New smatch warnings:
fs/f2fs/iostat.c:228 __update_iostat_latency() error: buffer overflow 'io_lat->sum_lat[type]' 3 <= 3
fs/f2fs/iostat.c:229 __update_iostat_latency() error: buffer overflow 'io_lat->bio_cnt[type]' 3 <= 3
fs/f2fs/iostat.c:230 __update_iostat_latency() error: buffer overflow 'io_lat->peak_lat[type]' 3 <= 3

Old smatch warnings:
fs/f2fs/iostat.c:231 __update_iostat_latency() error: buffer overflow 'io_lat->peak_lat[type]' 3 <= 3

vim +228 fs/f2fs/iostat.c

a4b6817625e71d Daeho Jeong 2021-08-20  211  static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
2804a18e00dbd6 Yangtao Li  2023-01-05  212  				enum iostat_lat_type type)
a4b6817625e71d Daeho Jeong 2021-08-20  213  {
a4b6817625e71d Daeho Jeong 2021-08-20  214  	unsigned long ts_diff;
2804a18e00dbd6 Yangtao Li  2023-01-05  215  	unsigned int page_type = iostat_ctx->type;
a4b6817625e71d Daeho Jeong 2021-08-20  216  	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
a4b6817625e71d Daeho Jeong 2021-08-20  217  	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
61803e984307c7 Daeho Jeong 2022-06-10  218  	unsigned long flags;
a4b6817625e71d Daeho Jeong 2021-08-20  219  
a4b6817625e71d Daeho Jeong 2021-08-20  220  	if (!sbi->iostat_enable)
a4b6817625e71d Daeho Jeong 2021-08-20  221  		return;
a4b6817625e71d Daeho Jeong 2021-08-20  222  
a4b6817625e71d Daeho Jeong 2021-08-20  223  	ts_diff = jiffies - iostat_ctx->submit_ts;
2804a18e00dbd6 Yangtao Li  2023-01-05  224  	if (page_type >= META_FLUSH)
                                                                 ^^^^^^^^^^

2804a18e00dbd6 Yangtao Li  2023-01-05  225  		page_type = META;
a4b6817625e71d Daeho Jeong 2021-08-20  226  
61803e984307c7 Daeho Jeong 2022-06-10  227  	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
2804a18e00dbd6 Yangtao Li  2023-01-05 @228  	io_lat->sum_lat[type][page_type] += ts_diff;
                                                                      ^^^^^^^^^
Mixup between META_FLUSH and NR_PAGE_TYPE leads to memory corruption.

2804a18e00dbd6 Yangtao Li  2023-01-05 @229  	io_lat->bio_cnt[type][page_type]++;
2804a18e00dbd6 Yangtao Li  2023-01-05 @230  	if (ts_diff > io_lat->peak_lat[type][page_type])
2804a18e00dbd6 Yangtao Li  2023-01-05  231  		io_lat->peak_lat[type][page_type] = ts_diff;
61803e984307c7 Daeho Jeong 2022-06-10  232  	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
a4b6817625e71d Daeho Jeong 2021-08-20  233  }
Chao Yu Jan. 28, 2023, 3:25 a.m. UTC | #2 
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.

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,
Jaegeuk Kim Jan. 30, 2023, 9:35 p.m. UTC | #3 
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.

> 
> 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,
Jaegeuk Kim Jan. 30, 2023, 10:23 p.m. UTC | #4 
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.

> 
> > 
> > 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
Chao Yu Jan. 31, 2023, 1:56 a.m. UTC | #5 
On 2023/1/31 5:35, 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.

Jaegeuk, it's fine, will send a separated patch to clean up parameters.

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,
Chao Yu Jan. 31, 2023, 1:58 a.m. UTC | #6 
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
diff mbox series

Patch

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);
 	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,