| Message ID | 20230711122208.65020-1-zhangzehao@vivo.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] exfat: Add ftrace support for exfat and add some tracepoints | expand |
Hi, On Tue, 11 Jul 2023 20:22:08 +0800 Zehao Zhang <zhangzehao@vivo.com> wrote: > Add ftrace support for exFAT file system, > and add some tracepoints: > exfat_read_folio(), exfat_writepages(), exfat_write_begin(), > exfat_write_end(), exfat_lookup_start(), exfat_lookup_end() > > exfat_read_folio(): > shows the dev number, inode and the folio index. > > exfat_writepages(): > shows the inode and fields in struct writeback_control. > > exfat_write_begin(): > shows the inode, file position offset and length. > > exfat_write_end(): > shows the inode, file position offset, bytes write to page > and bytes copied from user. > > exfat_lookup_start(): > shows the target inode, dentry and flags. > > exfat_lookup_end(): > shows the target inode, dentry and err code. > I still doubt including these. If VFS layer maintainer rejects the generic events, that means they expect to change the VFS layer in the future. If so, is that good to expose the trace event which is visible to users from exFAT? If VFS layer changes the interface in the future, these events can be out-of-dated. I think some of events looks good because those are enough , like exfat_write_begin/end, exfat_lookup_start/end (BTW, why one is 'begin' and another is 'start'?) But I think exfat_read_folio and exfat_writepages expose some implement depending parameters. Zehao, could you tell me what you really want to do with these? Maybe you can expose tracepoint instead of traceevent for tracing such implement depending parameters. Thank you, > Signed-off-by: Zehao Zhang <zhangzehao@vivo.com> > --- > v2: > - Keep the dereferencing logic out of the code path, > only happens in the trace event functions. > > Thanks for Steve's advice. > > MAINTAINERS | 1 + > fs/exfat/inode.c | 9 ++ > fs/exfat/namei.c | 10 +- > include/trace/events/exfat.h | 190 +++++++++++++++++++++++++++++++++++ > 4 files changed, 209 insertions(+), 1 deletion(-) > create mode 100644 include/trace/events/exfat.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4f115c355a41..fbe1caa61a38 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7752,6 +7752,7 @@ L: linux-fsdevel@vger.kernel.org > S: Maintained > T: git git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git > F: fs/exfat/ > +F: include/trace/events/exfat.h > > EXT2 FILE SYSTEM > M: Jan Kara <jack@suse.com> > diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c > index 481dd338f2b8..81fe715baab0 100644 > --- a/fs/exfat/inode.c > +++ b/fs/exfat/inode.c > @@ -17,6 +17,9 @@ > #include "exfat_raw.h" > #include "exfat_fs.h" > > +#define CREATE_TRACE_POINTS > +#include <trace/events/exfat.h> > + > int __exfat_write_inode(struct inode *inode, int sync) > { > unsigned long long on_disk_size; > @@ -335,6 +338,7 @@ static int exfat_get_block(struct inode *inode, sector_t iblock, > > static int exfat_read_folio(struct file *file, struct folio *folio) > { > + trace_exfat_read_folio(folio); > return mpage_read_folio(folio, exfat_get_block); > } > > @@ -346,6 +350,7 @@ static void exfat_readahead(struct readahead_control *rac) > static int exfat_writepages(struct address_space *mapping, > struct writeback_control *wbc) > { > + trace_exfat_writepages(mapping, wbc); > return mpage_writepages(mapping, wbc, exfat_get_block); > } > > @@ -371,6 +376,8 @@ static int exfat_write_begin(struct file *file, struct address_space *mapping, > exfat_get_block, > &EXFAT_I(mapping->host)->i_size_ondisk); > > + trace_exfat_write_begin(mapping, pos, len); > + > if (ret < 0) > exfat_write_failed(mapping, pos+len); > > @@ -394,6 +401,8 @@ static int exfat_write_end(struct file *file, struct address_space *mapping, > return -EIO; > } > > + trace_exfat_write_end(inode, pos, len, copied); > + > if (err < len) > exfat_write_failed(mapping, pos+len); > > diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c > index e0ff9d156f6f..f4f36de4ca0d 100644 > --- a/fs/exfat/namei.c > +++ b/fs/exfat/namei.c > @@ -12,6 +12,8 @@ > #include "exfat_raw.h" > #include "exfat_fs.h" > > +#include <trace/events/exfat.h> > + > static inline unsigned long exfat_d_version(struct dentry *dentry) > { > return (unsigned long) dentry->d_fsdata; > @@ -707,6 +709,8 @@ static struct dentry *exfat_lookup(struct inode *dir, struct dentry *dentry, > loff_t i_pos; > mode_t i_mode; > > + trace_exfat_lookup_start(dir, dentry, flags); > + > mutex_lock(&EXFAT_SB(sb)->s_lock); > err = exfat_find(dir, &dentry->d_name, &info); > if (err) { > @@ -766,7 +770,11 @@ static struct dentry *exfat_lookup(struct inode *dir, struct dentry *dentry, > if (!inode) > exfat_d_version_set(dentry, inode_query_iversion(dir)); > > - return d_splice_alias(inode, dentry); > + alias = d_splice_alias(inode, dentry); > + trace_exfat_lookup_end(dir, !IS_ERR_OR_NULL(alias) ? alias : dentry, > + IS_ERR(alias) ? PTR_ERR(alias) : err); > + > + return alias; > unlock: > mutex_unlock(&EXFAT_SB(sb)->s_lock); > return ERR_PTR(err); > diff --git a/include/trace/events/exfat.h b/include/trace/events/exfat.h > new file mode 100644 > index 000000000000..782dae6e2e42 > --- /dev/null > +++ b/include/trace/events/exfat.h > @@ -0,0 +1,190 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#undef TRACE_SYSTEM > +#define TRACE_SYSTEM exfat > + > +#if !defined(_TRACE_EXFAT_H) || defined(TRACE_HEADER_MULTI_READ) > +#define _TRACE_EXFAT_H > + > +#include <linux/tracepoint.h> > + > +#define EXFAT_I(inode) (container_of(inode, struct exfat_inode_info, vfs_inode)) > + > + > +TRACE_EVENT(exfat_write_begin, > + TP_PROTO(struct address_space *mapping, loff_t pos, unsigned int len), > + > + TP_ARGS(mapping, pos, len), > + > + TP_STRUCT__entry( > + __field(dev_t, dev) > + __field(ino_t, ino) > + __field(loff_t, pos) > + __field(unsigned int, len) > + ), > + > + TP_fast_assign( > + __entry->dev = mapping->host->i_sb->s_dev; > + __entry->ino = mapping->host->i_ino; > + __entry->pos = pos; > + __entry->len = len; > + ), > + > + TP_printk("dev (%d,%d) ino %lu pos %lld len %u", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + (unsigned long) __entry->ino, > + __entry->pos, __entry->len) > + > +); > + > +TRACE_EVENT(exfat_write_end, > + TP_PROTO(struct inode *inode, loff_t pos, unsigned int len, > + unsigned int copied), > + > + TP_ARGS(inode, pos, len, copied), > + > + TP_STRUCT__entry( > + __field(dev_t, dev) > + __field(ino_t, ino) > + __field(loff_t, pos) > + __field(unsigned int, len) > + __field(unsigned int, copied) > + ), > + > + TP_fast_assign( > + __entry->dev = inode->i_sb->s_dev; > + __entry->ino = inode->i_ino; > + __entry->pos = pos; > + __entry->len = len; > + __entry->copied = copied; > + ), > + > + TP_printk("dev (%d,%d) ino %lu pos %lld len %u copied %u", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + (unsigned long) __entry->ino, > + __entry->pos, __entry->len, __entry->copied) > +); > + > +TRACE_EVENT(exfat_read_folio, > + TP_PROTO(struct folio *folio), > + > + TP_ARGS(folio), > + > + TP_STRUCT__entry( > + __field(dev_t, dev) > + __field(ino_t, ino) > + __field(pgoff_t, index) > + > + ), > + > + TP_fast_assign( > + __entry->dev = folio->mapping->host->i_sb->s_dev; > + __entry->ino = folio->mapping->host->i_ino; > + __entry->index = folio->index; > + ), > + > + TP_printk("dev (%d,%d) ino %lu folio_index %lu", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + (unsigned long) __entry->ino, > + (unsigned long) __entry->index) > +); > + > +TRACE_EVENT(exfat_writepages, > + TP_PROTO(struct address_space *mapping, struct writeback_control *wbc), > + > + TP_ARGS(mapping, wbc), > + > + TP_STRUCT__entry( > + __field(dev_t, dev) > + __field(ino_t, ino) > + __field(long, nr_to_write) > + __field(long, pages_skipped) > + __field(loff_t, range_start) > + __field(loff_t, range_end) > + __field(pgoff_t, writeback_index) > + __field(int, sync_mode) > + __field(char, for_kupdate) > + __field(char, range_cyclic) > + ), > + > + TP_fast_assign( > + __entry->dev = mapping->host->i_sb->s_dev; > + __entry->ino = mapping->host->i_ino; > + __entry->nr_to_write = wbc->nr_to_write; > + __entry->pages_skipped = wbc->pages_skipped; > + __entry->range_start = wbc->range_start; > + __entry->range_end = wbc->range_end; > + __entry->writeback_index = mapping->host->i_mapping->writeback_index; > + __entry->sync_mode = wbc->sync_mode; > + __entry->for_kupdate = wbc->for_kupdate; > + __entry->range_cyclic = wbc->range_cyclic; > + ), > + > + TP_printk("dev (%d,%d) ino %lu nr_to_write %ld pages_skipped %ld " > + "range_start %lld range_end %lld sync_mode %d " > + "for_kupdate %d range_cyclic %d writeback_index %lu", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + (unsigned long) __entry->ino, __entry->nr_to_write, > + __entry->pages_skipped, __entry->range_start, > + __entry->range_end, __entry->sync_mode, > + __entry->for_kupdate, __entry->range_cyclic, > + (unsigned long) __entry->writeback_index) > +); > + > +TRACE_EVENT(exfat_lookup_start, > + > + TP_PROTO(struct inode *dir, struct dentry *dentry, unsigned int flags), > + > + TP_ARGS(dir, dentry, flags), > + > + TP_STRUCT__entry( > + __field(dev_t, dev) > + __field(ino_t, ino) > + __string(name, dentry->d_name.name) > + __field(unsigned int, flags) > + ), > + > + TP_fast_assign( > + __entry->dev = dir->i_sb->s_dev; > + __entry->ino = dir->i_ino; > + __assign_str(name, dentry->d_name.name); > + __entry->flags = flags; > + ), > + > + TP_printk("dev = (%d,%d), pino = %lu, name:%s, flags:%u", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + (unsigned long)__entry->ino, > + __get_str(name), > + __entry->flags) > +); > + > +TRACE_EVENT(exfat_lookup_end, > + > + TP_PROTO(struct inode *dir, struct dentry *dentry, int err), > + > + TP_ARGS(dir, dentry, err), > + > + TP_STRUCT__entry( > + __field(dev_t, dev) > + __field(ino_t, ino) > + __string(name, dentry->d_name.name) > + __field(int, err) > + ), > + > + TP_fast_assign( > + __entry->dev = dir->i_sb->s_dev; > + __entry->ino = dir->i_ino; > + __assign_str(name, dentry->d_name.name); > + __entry->err = err; > + ), > + > + TP_printk("dev = (%d,%d), pino = %lu, name:%s, err:%d", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + (unsigned long)__entry->ino, > + __get_str(name), > + __entry->err) > +); > + > +#endif /* _TRACE_EXFAT_H */ > + > +/* This part must be outside protection */ > +#include <trace/define_trace.h> > -- > 2.35.3 >
On Tue, Jul 11, 2023 at 08:22:08PM +0800, Zehao Zhang wrote: > Add ftrace support for exFAT file system, > and add some tracepoints: Why? I don't believe these are useful. Because if you actually had use for them, you would have noticed that you're missing 99% of the reads issued to the filesystem from the VFS -- through exfat_readahead(), which you didn't instrument. So what do you actually want to know, and why can't you get that information already?
diff --git a/MAINTAINERS b/MAINTAINERS index 4f115c355a41..fbe1caa61a38 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7752,6 +7752,7 @@ L: linux-fsdevel@vger.kernel.org S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git F: fs/exfat/ +F: include/trace/events/exfat.h EXT2 FILE SYSTEM M: Jan Kara <jack@suse.com> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c index 481dd338f2b8..81fe715baab0 100644 --- a/fs/exfat/inode.c +++ b/fs/exfat/inode.c @@ -17,6 +17,9 @@ #include "exfat_raw.h" #include "exfat_fs.h" +#define CREATE_TRACE_POINTS +#include <trace/events/exfat.h> + int __exfat_write_inode(struct inode *inode, int sync) { unsigned long long on_disk_size; @@ -335,6 +338,7 @@ static int exfat_get_block(struct inode *inode, sector_t iblock, static int exfat_read_folio(struct file *file, struct folio *folio) { + trace_exfat_read_folio(folio); return mpage_read_folio(folio, exfat_get_block); } @@ -346,6 +350,7 @@ static void exfat_readahead(struct readahead_control *rac) static int exfat_writepages(struct address_space *mapping, struct writeback_control *wbc) { + trace_exfat_writepages(mapping, wbc); return mpage_writepages(mapping, wbc, exfat_get_block); } @@ -371,6 +376,8 @@ static int exfat_write_begin(struct file *file, struct address_space *mapping, exfat_get_block, &EXFAT_I(mapping->host)->i_size_ondisk); + trace_exfat_write_begin(mapping, pos, len); + if (ret < 0) exfat_write_failed(mapping, pos+len); @@ -394,6 +401,8 @@ static int exfat_write_end(struct file *file, struct address_space *mapping, return -EIO; } + trace_exfat_write_end(inode, pos, len, copied); + if (err < len) exfat_write_failed(mapping, pos+len); diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index e0ff9d156f6f..f4f36de4ca0d 100644 --- a/fs/exfat/namei.c +++ b/fs/exfat/namei.c @@ -12,6 +12,8 @@ #include "exfat_raw.h" #include "exfat_fs.h" +#include <trace/events/exfat.h> + static inline unsigned long exfat_d_version(struct dentry *dentry) { return (unsigned long) dentry->d_fsdata; @@ -707,6 +709,8 @@ static struct dentry *exfat_lookup(struct inode *dir, struct dentry *dentry, loff_t i_pos; mode_t i_mode; + trace_exfat_lookup_start(dir, dentry, flags); + mutex_lock(&EXFAT_SB(sb)->s_lock); err = exfat_find(dir, &dentry->d_name, &info); if (err) { @@ -766,7 +770,11 @@ static struct dentry *exfat_lookup(struct inode *dir, struct dentry *dentry, if (!inode) exfat_d_version_set(dentry, inode_query_iversion(dir)); - return d_splice_alias(inode, dentry); + alias = d_splice_alias(inode, dentry); + trace_exfat_lookup_end(dir, !IS_ERR_OR_NULL(alias) ? alias : dentry, + IS_ERR(alias) ? PTR_ERR(alias) : err); + + return alias; unlock: mutex_unlock(&EXFAT_SB(sb)->s_lock); return ERR_PTR(err); diff --git a/include/trace/events/exfat.h b/include/trace/events/exfat.h new file mode 100644 index 000000000000..782dae6e2e42 --- /dev/null +++ b/include/trace/events/exfat.h @@ -0,0 +1,190 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM exfat + +#if !defined(_TRACE_EXFAT_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_EXFAT_H + +#include <linux/tracepoint.h> + +#define EXFAT_I(inode) (container_of(inode, struct exfat_inode_info, vfs_inode)) + + +TRACE_EVENT(exfat_write_begin, + TP_PROTO(struct address_space *mapping, loff_t pos, unsigned int len), + + TP_ARGS(mapping, pos, len), + + TP_STRUCT__entry( + __field(dev_t, dev) + __field(ino_t, ino) + __field(loff_t, pos) + __field(unsigned int, len) + ), + + TP_fast_assign( + __entry->dev = mapping->host->i_sb->s_dev; + __entry->ino = mapping->host->i_ino; + __entry->pos = pos; + __entry->len = len; + ), + + TP_printk("dev (%d,%d) ino %lu pos %lld len %u", + MAJOR(__entry->dev), MINOR(__entry->dev), + (unsigned long) __entry->ino, + __entry->pos, __entry->len) + +); + +TRACE_EVENT(exfat_write_end, + TP_PROTO(struct inode *inode, loff_t pos, unsigned int len, + unsigned int copied), + + TP_ARGS(inode, pos, len, copied), + + TP_STRUCT__entry( + __field(dev_t, dev) + __field(ino_t, ino) + __field(loff_t, pos) + __field(unsigned int, len) + __field(unsigned int, copied) + ), + + TP_fast_assign( + __entry->dev = inode->i_sb->s_dev; + __entry->ino = inode->i_ino; + __entry->pos = pos; + __entry->len = len; + __entry->copied = copied; + ), + + TP_printk("dev (%d,%d) ino %lu pos %lld len %u copied %u", + MAJOR(__entry->dev), MINOR(__entry->dev), + (unsigned long) __entry->ino, + __entry->pos, __entry->len, __entry->copied) +); + +TRACE_EVENT(exfat_read_folio, + TP_PROTO(struct folio *folio), + + TP_ARGS(folio), + + TP_STRUCT__entry( + __field(dev_t, dev) + __field(ino_t, ino) + __field(pgoff_t, index) + + ), + + TP_fast_assign( + __entry->dev = folio->mapping->host->i_sb->s_dev; + __entry->ino = folio->mapping->host->i_ino; + __entry->index = folio->index; + ), + + TP_printk("dev (%d,%d) ino %lu folio_index %lu", + MAJOR(__entry->dev), MINOR(__entry->dev), + (unsigned long) __entry->ino, + (unsigned long) __entry->index) +); + +TRACE_EVENT(exfat_writepages, + TP_PROTO(struct address_space *mapping, struct writeback_control *wbc), + + TP_ARGS(mapping, wbc), + + TP_STRUCT__entry( + __field(dev_t, dev) + __field(ino_t, ino) + __field(long, nr_to_write) + __field(long, pages_skipped) + __field(loff_t, range_start) + __field(loff_t, range_end) + __field(pgoff_t, writeback_index) + __field(int, sync_mode) + __field(char, for_kupdate) + __field(char, range_cyclic) + ), + + TP_fast_assign( + __entry->dev = mapping->host->i_sb->s_dev; + __entry->ino = mapping->host->i_ino; + __entry->nr_to_write = wbc->nr_to_write; + __entry->pages_skipped = wbc->pages_skipped; + __entry->range_start = wbc->range_start; + __entry->range_end = wbc->range_end; + __entry->writeback_index = mapping->host->i_mapping->writeback_index; + __entry->sync_mode = wbc->sync_mode; + __entry->for_kupdate = wbc->for_kupdate; + __entry->range_cyclic = wbc->range_cyclic; + ), + + TP_printk("dev (%d,%d) ino %lu nr_to_write %ld pages_skipped %ld " + "range_start %lld range_end %lld sync_mode %d " + "for_kupdate %d range_cyclic %d writeback_index %lu", + MAJOR(__entry->dev), MINOR(__entry->dev), + (unsigned long) __entry->ino, __entry->nr_to_write, + __entry->pages_skipped, __entry->range_start, + __entry->range_end, __entry->sync_mode, + __entry->for_kupdate, __entry->range_cyclic, + (unsigned long) __entry->writeback_index) +); + +TRACE_EVENT(exfat_lookup_start, + + TP_PROTO(struct inode *dir, struct dentry *dentry, unsigned int flags), + + TP_ARGS(dir, dentry, flags), + + TP_STRUCT__entry( + __field(dev_t, dev) + __field(ino_t, ino) + __string(name, dentry->d_name.name) + __field(unsigned int, flags) + ), + + TP_fast_assign( + __entry->dev = dir->i_sb->s_dev; + __entry->ino = dir->i_ino; + __assign_str(name, dentry->d_name.name); + __entry->flags = flags; + ), + + TP_printk("dev = (%d,%d), pino = %lu, name:%s, flags:%u", + MAJOR(__entry->dev), MINOR(__entry->dev), + (unsigned long)__entry->ino, + __get_str(name), + __entry->flags) +); + +TRACE_EVENT(exfat_lookup_end, + + TP_PROTO(struct inode *dir, struct dentry *dentry, int err), + + TP_ARGS(dir, dentry, err), + + TP_STRUCT__entry( + __field(dev_t, dev) + __field(ino_t, ino) + __string(name, dentry->d_name.name) + __field(int, err) + ), + + TP_fast_assign( + __entry->dev = dir->i_sb->s_dev; + __entry->ino = dir->i_ino; + __assign_str(name, dentry->d_name.name); + __entry->err = err; + ), + + TP_printk("dev = (%d,%d), pino = %lu, name:%s, err:%d", + MAJOR(__entry->dev), MINOR(__entry->dev), + (unsigned long)__entry->ino, + __get_str(name), + __entry->err) +); + +#endif /* _TRACE_EXFAT_H */ + +/* This part must be outside protection */ +#include <trace/define_trace.h>
Add ftrace support for exFAT file system, and add some tracepoints: exfat_read_folio(), exfat_writepages(), exfat_write_begin(), exfat_write_end(), exfat_lookup_start(), exfat_lookup_end() exfat_read_folio(): shows the dev number, inode and the folio index. exfat_writepages(): shows the inode and fields in struct writeback_control. exfat_write_begin(): shows the inode, file position offset and length. exfat_write_end(): shows the inode, file position offset, bytes write to page and bytes copied from user. exfat_lookup_start(): shows the target inode, dentry and flags. exfat_lookup_end(): shows the target inode, dentry and err code. Signed-off-by: Zehao Zhang <zhangzehao@vivo.com> --- v2: - Keep the dereferencing logic out of the code path, only happens in the trace event functions. Thanks for Steve's advice. MAINTAINERS | 1 + fs/exfat/inode.c | 9 ++ fs/exfat/namei.c | 10 +- include/trace/events/exfat.h | 190 +++++++++++++++++++++++++++++++++++ 4 files changed, 209 insertions(+), 1 deletion(-) create mode 100644 include/trace/events/exfat.h