Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 10 May 2022 20:30:31 -0700
From:   Jaegeuk Kim <jaegeuk@kernel.org>
To:     Chao Yu <chao@kernel.org>
Cc:     linux-kernel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH 2/5 v2] f2fs: introduce f2fs_gc_control to
 consolidate f2fs_gc parameters
Message-ID: <Ynst13cs86AHMLQ8@google.com>
References: <20220506232032.1264078-1-jaegeuk@kernel.org>
 <20220506232032.1264078-2-jaegeuk@kernel.org>
 <YnlFiz/IgsjtGhqu@google.com>
 <0a58b401-6fa4-4314-d7b4-029993cb7a75@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0a58b401-6fa4-4314-d7b4-029993cb7a75@kernel.org>
Precedence: bulk

On 05/11, Chao Yu wrote:
> Jaegeuk,
> 
> Seems it includes a wrong android tracepoint patch?

Oops. :)

> 
> Thanks,
> 
> On 2022/5/10 0:47, Jaegeuk Kim wrote:
> > This was used in Android for a long time. Let's upstream it.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >   Change log from v1:
> >    - fix tracepoint for the "don't care" entry
> > 
> >   fs/f2fs/file.c              | 58 ++++++++++++++++++++---
> >   include/trace/events/f2fs.h | 94 +++++++++++++++++++++++++++++++++++++
> >   2 files changed, 145 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 68ddf4c7ca64..51df34f95984 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -4329,17 +4329,39 @@ static ssize_t f2fs_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
> >   static ssize_t f2fs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> >   {
> >   	struct inode *inode = file_inode(iocb->ki_filp);
> > +	const loff_t pos = iocb->ki_pos;
> >   	ssize_t ret;
> >   	if (!f2fs_is_compress_backend_ready(inode))
> >   		return -EOPNOTSUPP;
> > -	if (f2fs_should_use_dio(inode, iocb, to))
> > -		return f2fs_dio_read_iter(iocb, to);
> > +	if (trace_f2fs_dataread_start_enabled()) {
> > +		char *p = f2fs_kmalloc(F2FS_I_SB(inode), PATH_MAX, GFP_KERNEL);
> > +		char *path;
> > +
> > +		if (!p)
> > +			goto skip_read_trace;
> > +
> > +		path = dentry_path_raw(file_dentry(iocb->ki_filp), p, PATH_MAX);
> > +		if (IS_ERR(path)) {
> > +			kfree(p);
> > +			goto skip_read_trace;
> > +		}
> > -	ret = filemap_read(iocb, to, 0);
> > -	if (ret > 0)
> > -		f2fs_update_iostat(F2FS_I_SB(inode), APP_BUFFERED_READ_IO, ret);
> > +		trace_f2fs_dataread_start(inode, pos, iov_iter_count(to),
> > +					current->pid, path, current->comm);
> > +		kfree(p);
> > +	}
> > +skip_read_trace:
> > +	if (f2fs_should_use_dio(inode, iocb, to)) {
> > +		ret = f2fs_dio_read_iter(iocb, to);
> > +	} else {
> > +		ret = filemap_read(iocb, to, 0);
> > +		if (ret > 0)
> > +			f2fs_update_iostat(F2FS_I_SB(inode), APP_BUFFERED_READ_IO, ret);
> > +	}
> > +	if (trace_f2fs_dataread_end_enabled())
> > +		trace_f2fs_dataread_end(inode, pos, ret);
> >   	return ret;
> >   }
> > @@ -4637,14 +4659,36 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> >   	/* Possibly preallocate the blocks for the write. */
> >   	target_size = iocb->ki_pos + iov_iter_count(from);
> >   	preallocated = f2fs_preallocate_blocks(iocb, from, dio);
> > -	if (preallocated < 0)
> > +	if (preallocated < 0) {
> >   		ret = preallocated;
> > -	else
> > +	} else {
> > +		if (trace_f2fs_datawrite_start_enabled()) {
> > +			char *p = f2fs_kmalloc(F2FS_I_SB(inode),
> > +						PATH_MAX, GFP_KERNEL);
> > +			char *path;
> > +
> > +			if (!p)
> > +				goto skip_write_trace;
> > +			path = dentry_path_raw(file_dentry(iocb->ki_filp),
> > +								p, PATH_MAX);
> > +			if (IS_ERR(path)) {
> > +				kfree(p);
> > +				goto skip_write_trace;
> > +			}
> > +			trace_f2fs_datawrite_start(inode, orig_pos, orig_count,
> > +					current->pid, path, current->comm);
> > +			kfree(p);
> > +		}
> > +skip_write_trace:
> >   		/* Do the actual write. */
> >   		ret = dio ?
> >   			f2fs_dio_write_iter(iocb, from, &may_need_sync):
> >   			f2fs_buffered_write_iter(iocb, from);
> > +		if (trace_f2fs_datawrite_end_enabled())
> > +			trace_f2fs_datawrite_end(inode, orig_pos, ret);
> > +	}
> > +
> >   	/* Don't leave any preallocated blocks around past i_size. */
> >   	if (preallocated && i_size_read(inode) < target_size) {
> >   		f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> > diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> > index f701bb23f83c..11f6b7147be2 100644
> > --- a/include/trace/events/f2fs.h
> > +++ b/include/trace/events/f2fs.h
> > @@ -2068,6 +2068,100 @@ TRACE_EVENT(f2fs_fiemap,
> >   		__entry->ret)
> >   );
> > +DECLARE_EVENT_CLASS(f2fs__rw_start,
> > +
> > +	TP_PROTO(struct inode *inode, loff_t offset, int bytes,
> > +			pid_t pid, char *pathname, char *command),
> > +
> > +	TP_ARGS(inode, offset, bytes, pid, pathname, command),
> > +
> > +	TP_STRUCT__entry(
> > +		__string(pathbuf, pathname)
> > +		__field(loff_t, offset)
> > +		__field(int, bytes)
> > +		__field(loff_t, i_size)
> > +		__string(cmdline, command)
> > +		__field(pid_t, pid)
> > +		__field(ino_t, ino)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		/*
> > +		 * Replace the spaces in filenames and cmdlines
> > +		 * because this screws up the tooling that parses
> > +		 * the traces.
> > +		 */
> > +		__assign_str(pathbuf, pathname);
> > +		(void)strreplace(__get_str(pathbuf), ' ', '_');
> > +		__entry->offset = offset;
> > +		__entry->bytes = bytes;
> > +		__entry->i_size = i_size_read(inode);
> > +		__assign_str(cmdline, command);
> > +		(void)strreplace(__get_str(cmdline), ' ', '_');
> > +		__entry->pid = pid;
> > +		__entry->ino = inode->i_ino;
> > +	),
> > +
> > +	TP_printk("entry_name %s, offset %llu, bytes %d, cmdline %s,"
> > +		" pid %d, i_size %llu, ino %lu",
> > +		__get_str(pathbuf), __entry->offset, __entry->bytes,
> > +		__get_str(cmdline), __entry->pid, __entry->i_size,
> > +		(unsigned long) __entry->ino)
> > +);
> > +
> > +DECLARE_EVENT_CLASS(f2fs__rw_end,
> > +
> > +	TP_PROTO(struct inode *inode, loff_t offset, int bytes),
> > +
> > +	TP_ARGS(inode, offset, bytes),
> > +
> > +	TP_STRUCT__entry(
> > +		__field(ino_t,	ino)
> > +		__field(loff_t,	offset)
> > +		__field(int,	bytes)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__entry->ino		= inode->i_ino;
> > +		__entry->offset		= offset;
> > +		__entry->bytes		= bytes;
> > +	),
> > +
> > +	TP_printk("ino %lu, offset %llu, bytes %d",
> > +		(unsigned long) __entry->ino,
> > +		__entry->offset, __entry->bytes)
> > +);
> > +
> > +DEFINE_EVENT(f2fs__rw_start, f2fs_dataread_start,
> > +
> > +	TP_PROTO(struct inode *inode, loff_t offset, int bytes,
> > +		pid_t pid, char *pathname, char *command),
> > +
> > +	TP_ARGS(inode, offset, bytes, pid, pathname, command)
> > +);
> > +
> > +DEFINE_EVENT(f2fs__rw_end, f2fs_dataread_end,
> > +
> > +	TP_PROTO(struct inode *inode, loff_t offset, int bytes),
> > +
> > +	TP_ARGS(inode, offset, bytes)
> > +);
> > +
> > +DEFINE_EVENT(f2fs__rw_start, f2fs_datawrite_start,
> > +
> > +	TP_PROTO(struct inode *inode, loff_t offset, int bytes,
> > +		pid_t pid, char *pathname, char *command),
> > +
> > +	TP_ARGS(inode, offset, bytes, pid, pathname, command)
> > +);
> > +
> > +DEFINE_EVENT(f2fs__rw_end, f2fs_datawrite_end,
> > +
> > +	TP_PROTO(struct inode *inode, loff_t offset, int bytes),
> > +
> > +	TP_ARGS(inode, offset, bytes)
> > +);
> > +
> >   #endif /* _TRACE_F2FS_H */
> >    /* This part must be outside protection */