Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 15 Apr 2020 13:56:11 -0700
From:   Jaegeuk Kim <jaegeuk@kernel.org>
To:     Chao Yu <yuchao0@huawei.com>
Cc:     linux-kernel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com
Subject: Re: [f2fs-dev] [PATCH] f2fs: add tracepoint for f2fs iostat
Message-ID: <20200415205611.GA29167@google.com>
References: <20200413161649.38177-1-jaegeuk@kernel.org>
 <838f483d-377f-e60e-5ffe-cf52b913f294@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <838f483d-377f-e60e-5ffe-cf52b913f294@huawei.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 04/15, Chao Yu wrote:
> On 2020/4/14 0:16, Jaegeuk Kim wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> > 
> > Added a tracepoint to see iostat of f2fs. Default period of that
> > is 3 second. This tracepoint can be used to be monitoring
> > I/O statistics periodically.
> > 
> > Bug: 152162885
> > Change-Id: I6fbe010b9cf1a90caa0f4793a6dab77c4cba7da6
> 
> It needs to be removed?

Yup.

> 
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> >  fs/f2fs/f2fs.h              | 10 ++++++-
> >  fs/f2fs/sysfs.c             | 34 ++++++++++++++++++++++++
> >  include/trace/events/f2fs.h | 52 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 95 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index c2788738aa0d4..87baa09f76fb2 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2999,16 +2999,22 @@ static inline int get_inline_xattr_addrs(struct inode *inode)
> >  		sizeof((f2fs_inode)->field))			\
> >  		<= (F2FS_OLD_ATTRIBUTE_SIZE + (extra_isize)))	\
> >  
> > +extern unsigned long long f2fs_prev_iostat[NR_IO_TYPE];
> > +
> >  static inline void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> >  {
> >  	int i;
> >  
> >  	spin_lock(&sbi->iostat_lock);
> > -	for (i = 0; i < NR_IO_TYPE; i++)
> > +	for (i = 0; i < NR_IO_TYPE; i++) {
> >  		sbi->write_iostat[i] = 0;
> > +		f2fs_prev_iostat[i] = 0;
> > +	}
> >  	spin_unlock(&sbi->iostat_lock);
> >  }
> >  
> > +extern void f2fs_record_iostat(struct f2fs_sb_info *sbi);
> > +
> >  static inline void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> >  			enum iostat_type type, unsigned long long io_bytes)
> >  {
> > @@ -3022,6 +3028,8 @@ static inline void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> >  			sbi->write_iostat[APP_WRITE_IO] -
> >  			sbi->write_iostat[APP_DIRECT_IO];
> >  	spin_unlock(&sbi->iostat_lock);
> > +
> > +	f2fs_record_iostat(sbi);
> >  }
> >  
> >  #define __is_large_section(sbi)		((sbi)->segs_per_sec > 1)
> > diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> > index aeebfb5024a22..f34cb75cd039c 100644
> > --- a/fs/f2fs/sysfs.c
> > +++ b/fs/f2fs/sysfs.c
> > @@ -15,6 +15,7 @@
> >  #include "f2fs.h"
> >  #include "segment.h"
> >  #include "gc.h"
> > +#include <trace/events/f2fs.h>
> >  
> >  static struct proc_dir_entry *f2fs_proc_root;
> >  
> > @@ -751,6 +752,39 @@ static int __maybe_unused segment_bits_seq_show(struct seq_file *seq,
> >  	return 0;
> >  }
> >  
> > +static const unsigned long period_ms = 3000;
> > +static unsigned long next_period;
> > +unsigned long long f2fs_prev_iostat[NR_IO_TYPE] = {0};
> 
> These various should be per sbi, otherwise stats from different image could
> be interrupt others'.

Agreed. Let me pass it to work on it.

Thanks,

> 
> Thanks,
> 
> > +
> > +static DEFINE_SPINLOCK(iostat_lock);
> > +
> > +void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > +{
> > +	unsigned long long iostat_diff[NR_IO_TYPE];
> > +	int i;
> > +
> > +	if (time_is_after_jiffies(next_period))
> > +		return;
> > +
> > +	/* Need double check under the lock */
> > +	spin_lock(&iostat_lock);
> > +	if (time_is_after_jiffies(next_period)) {
> > +		spin_unlock(&iostat_lock);
> > +		return;
> > +	}
> > +	next_period = jiffies + msecs_to_jiffies(period_ms);
> > +	spin_unlock(&iostat_lock);
> > +
> > +	spin_lock(&sbi->iostat_lock);
> > +	for (i = 0; i < NR_IO_TYPE; i++) {
> > +		iostat_diff[i] = sbi->write_iostat[i] - f2fs_prev_iostat[i];
> > +		f2fs_prev_iostat[i] = sbi->write_iostat[i];
> > +	}
> > +	spin_unlock(&sbi->iostat_lock);
> > +
> > +	trace_f2fs_iostat(sbi, iostat_diff);
> > +}
> > +
> >  static int __maybe_unused iostat_info_seq_show(struct seq_file *seq,
> >  					       void *offset)
> >  {
> > diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> > index d97adfc327f03..e78c8696e2adc 100644
> > --- a/include/trace/events/f2fs.h
> > +++ b/include/trace/events/f2fs.h
> > @@ -1812,6 +1812,58 @@ DEFINE_EVENT(f2fs_zip_end, f2fs_decompress_pages_end,
> >  	TP_ARGS(inode, cluster_idx, compressed_size, ret)
> >  );
> >  
> > +TRACE_EVENT(f2fs_iostat,
> > +
> > +	TP_PROTO(struct f2fs_sb_info *sbi, unsigned long long *iostat),
> > +
> > +	TP_ARGS(sbi, iostat),
> > +
> > +	TP_STRUCT__entry(
> > +		__field(dev_t,	dev)
> > +		__field(unsigned long long,	app_dio)
> > +		__field(unsigned long long,	app_bio)
> > +		__field(unsigned long long,	app_wio)
> > +		__field(unsigned long long,	app_mio)
> > +		__field(unsigned long long,	fs_dio)
> > +		__field(unsigned long long,	fs_nio)
> > +		__field(unsigned long long,	fs_mio)
> > +		__field(unsigned long long,	fs_gc_dio)
> > +		__field(unsigned long long,	fs_gc_nio)
> > +		__field(unsigned long long,	fs_cp_dio)
> > +		__field(unsigned long long,	fs_cp_nio)
> > +		__field(unsigned long long,	fs_cp_mio)
> > +		__field(unsigned long long,	fs_discard)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__entry->dev		= sbi->sb->s_dev;
> > +		__entry->app_dio	= iostat[APP_DIRECT_IO];
> > +		__entry->app_bio	= iostat[APP_BUFFERED_IO];
> > +		__entry->app_wio	= iostat[APP_WRITE_IO];
> > +		__entry->app_mio	= iostat[APP_MAPPED_IO];
> > +		__entry->fs_dio		= iostat[FS_DATA_IO];
> > +		__entry->fs_nio		= iostat[FS_NODE_IO];
> > +		__entry->fs_mio		= iostat[FS_META_IO];
> > +		__entry->fs_gc_dio	= iostat[FS_GC_DATA_IO];
> > +		__entry->fs_gc_nio	= iostat[FS_GC_NODE_IO];
> > +		__entry->fs_cp_dio	= iostat[FS_CP_DATA_IO];
> > +		__entry->fs_cp_nio	= iostat[FS_CP_NODE_IO];
> > +		__entry->fs_cp_mio	= iostat[FS_CP_META_IO];
> > +		__entry->fs_discard	= iostat[FS_DISCARD];
> > +	),
> > +
> > +	TP_printk("dev = (%d,%d), "
> > +		"app [write=%llu (direct=%llu, buffered=%llu), mapped=%llu], "
> > +		"fs [data=%llu, node=%llu, meta=%llu, discard=%llu], "
> > +		"gc [data=%llu, node=%llu], "
> > +		"cp [data=%llu, node=%llu, meta=%llu]",
> > +		show_dev(__entry->dev), __entry->app_wio, __entry->app_dio,
> > +		__entry->app_bio, __entry->app_mio, __entry->fs_dio,
> > +		__entry->fs_nio, __entry->fs_mio, __entry->fs_discard,
> > +		__entry->fs_gc_dio, __entry->fs_gc_nio, __entry->fs_cp_dio,
> > +		__entry->fs_cp_nio, __entry->fs_cp_mio)
> > +);
> > +
> >  #endif /* _TRACE_F2FS_H */
> >  
> >   /* This part must be outside protection */
> >