From: Jaegeuk Kim <jaegeuk@kernel.org>
To: Chao Yu <chao@kernel.org>
Cc: Yangtao Li <frank.li@vivo.com>,
linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
Date: Mon, 30 Jan 2023 14:23:27 -0800 [thread overview]
Message-ID: <Y9hDXzXnyFA9ejc3@google.com> (raw)
In-Reply-To: <Y9g4PcQvsCOj1d0r@google.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Jaegeuk Kim <jaegeuk@kernel.org>
To: Chao Yu <chao@kernel.org>
Cc: linux-f2fs-devel@lists.sourceforge.net,
linux-kernel@vger.kernel.org, Yangtao Li <frank.li@vivo.com>
Subject: Re: [f2fs-dev] [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()
Date: Mon, 30 Jan 2023 14:23:27 -0800 [thread overview]
Message-ID: <Y9hDXzXnyFA9ejc3@google.com> (raw)
In-Reply-To: <Y9g4PcQvsCOj1d0r@google.com>
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
next prev parent reply other threads:[~2023-01-30 22:23 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
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-05 4:22 ` Yangtao Li
2023-01-28 3:25 ` [f2fs-dev] " Chao Yu
2023-01-28 3:25 ` Chao Yu
2023-01-30 21:35 ` [f2fs-dev] " Jaegeuk Kim
2023-01-30 21:35 ` Jaegeuk Kim
2023-01-30 22:23 ` Jaegeuk Kim [this message]
2023-01-30 22:23 ` [f2fs-dev] " Jaegeuk Kim
2023-01-31 1:58 ` Chao Yu
2023-01-31 1:58 ` Chao Yu
2023-01-31 1:56 ` Chao Yu
2023-01-31 1:56 ` Chao Yu
-- strict thread matches above, loose matches on Subject: below --
2023-01-13 9:07 kernel test robot
2023-01-13 9:26 ` Dan Carpenter
2023-01-13 9:26 ` [f2fs-dev] " Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y9hDXzXnyFA9ejc3@google.com \
--to=jaegeuk@kernel.org \
--cc=chao@kernel.org \
--cc=frank.li@vivo.com \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.