* [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
@ 2023-01-05 4:22 Yangtao Li via Linux-f2fs-devel
2023-01-13 9:26 ` Dan Carpenter
2023-01-28 3:25 ` Chao Yu
0 siblings, 2 replies; 7+ messages in thread
From: Yangtao Li via Linux-f2fs-devel @ 2023-01-05 4:22 UTC (permalink / raw)
To: jaegeuk, chao; +Cc: Yangtao Li, linux-kernel, linux-f2fs-devel
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);
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,
--
2.25.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
2023-01-05 4:22 [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx() Yangtao Li via Linux-f2fs-devel
@ 2023-01-13 9:26 ` Dan Carpenter
2023-01-28 3:25 ` Chao Yu
1 sibling, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2023-01-13 9:26 UTC (permalink / raw)
To: oe-kbuild, Yangtao Li, jaegeuk, chao
Cc: linux-kernel, linux-f2fs-devel, lkp, Yangtao Li, oe-kbuild-all
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 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
2023-01-05 4:22 [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx() Yangtao Li via Linux-f2fs-devel
2023-01-13 9:26 ` Dan Carpenter
@ 2023-01-28 3:25 ` Chao Yu
2023-01-30 21:35 ` Jaegeuk Kim
1 sibling, 1 reply; 7+ messages in thread
From: Chao Yu @ 2023-01-28 3:25 UTC (permalink / raw)
To: Yangtao Li, jaegeuk; +Cc: linux-kernel, linux-f2fs-devel
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,
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
2023-01-28 3:25 ` Chao Yu
@ 2023-01-30 21:35 ` Jaegeuk Kim
2023-01-30 22:23 ` Jaegeuk Kim
2023-01-31 1:56 ` Chao Yu
0 siblings, 2 replies; 7+ messages in thread
From: Jaegeuk Kim @ 2023-01-30 21:35 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, Yangtao Li
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,
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
2023-01-30 21:35 ` Jaegeuk Kim
@ 2023-01-30 22:23 ` Jaegeuk Kim
2023-01-31 1:58 ` Chao Yu
2023-01-31 1:56 ` Chao Yu
1 sibling, 1 reply; 7+ messages in thread
From: Jaegeuk Kim @ 2023-01-30 22:23 UTC (permalink / raw)
To: Chao Yu; +Cc: Yangtao Li, linux-kernel, linux-f2fs-devel
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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
2023-01-30 21:35 ` Jaegeuk Kim
2023-01-30 22:23 ` Jaegeuk Kim
@ 2023-01-31 1:56 ` Chao Yu
1 sibling, 0 replies; 7+ messages in thread
From: Chao Yu @ 2023-01-31 1:56 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, Yangtao Li
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,
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
2023-01-30 22:23 ` Jaegeuk Kim
@ 2023-01-31 1:58 ` Chao Yu
0 siblings, 0 replies; 7+ messages in thread
From: Chao Yu @ 2023-01-31 1:58 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Yangtao Li, linux-kernel, linux-f2fs-devel
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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-01-31 1:58 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-05 4:22 [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx() Yangtao Li via Linux-f2fs-devel
2023-01-13 9:26 ` Dan Carpenter
2023-01-28 3:25 ` Chao Yu
2023-01-30 21:35 ` Jaegeuk Kim
2023-01-30 22:23 ` Jaegeuk Kim
2023-01-31 1:58 ` Chao Yu
2023-01-31 1:56 ` Chao Yu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).