| Message ID | 1497256346-666-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12.06.2017 11:32, Anand Jain wrote: > This patch adds compression and decompression trace points for the > purpose of debugging. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v2: > . Use better naming. > (If transform is not good enough I have run out of ideas, pls suggest). > . To be applied on top of > git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next > (tested without namelen check patch set). I haven't read previous submissions and any discussions that might have occurred there but why not just stick to btrfs_data_compression/btrfs_data_compressor. I know there is certain semantic overload since we call it a compressor yet it also does decompression but let's focus on making the code/intention clear for the code reader and not bogging down too much on actual word semantics. To me "compressor" is a synonym to something which compresses AND decompresses. It's very well possible that this is just me in which case my argument is flawed and you can ignore it :) > > fs/btrfs/compression.c | 11 +++++++++++ > include/trace/events/btrfs.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 55 insertions(+) > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > index fd6508bcff77..53908722d80e 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -884,6 +884,10 @@ int btrfs_compress_pages(int type, struct address_space *mapping, > start, pages, > out_pages, > total_in, total_out); > + > + trace_btrfs_data_transformer(0, 0, mapping->host, type, *total_in, > + *total_out, start, ret); > + > free_workspace(type, workspace); > return ret; > } > @@ -910,6 +914,10 @@ static int btrfs_decompress_bio(struct compressed_bio *cb) > > workspace = find_workspace(type); > ret = btrfs_compress_op[type - 1]->decompress_bio(workspace, cb); > + > + trace_btrfs_data_transformer(1, 1, cb->inode, type, > + cb->compressed_len, cb->len, cb->start, ret); > + > free_workspace(type, workspace); > > return ret; > @@ -932,6 +940,9 @@ int btrfs_decompress(int type, unsigned char *data_in, struct page *dest_page, > dest_page, start_byte, > srclen, destlen); > > + trace_btrfs_data_transformer(1, 0, dest_page->mapping->host, > + type, srclen, destlen, start_byte, ret); > + > free_workspace(type, workspace); > return ret; > } > diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h > index cd99a3658156..7c2442dab6a0 100644 > --- a/include/trace/events/btrfs.h > +++ b/include/trace/events/btrfs.h > @@ -1622,6 +1622,50 @@ TRACE_EVENT(qgroup_meta_reserve, > show_root_type(__entry->refroot), __entry->diff) > ); > > +#define show_transformer_type(type) \ Why not show_compression_type ? > + __print_symbolic(type, \ > + { BTRFS_COMPRESS_ZLIB, "zlib" }, \ > + { BTRFS_COMPRESS_LZO, "lzo" }) > + > +TRACE_EVENT(btrfs_data_transformer, > + > + TP_PROTO(int uncompress, int its_bio, struct inode *inode, int type, > + unsigned long len_before, unsigned long len_after, > + unsigned long start, int ret)> + > + TP_ARGS(uncompress, its_bio, inode, type, len_before, > + len_after, start, ret), > + > + TP_STRUCT__entry_btrfs( > + __field( int, uncompress) > + __field( int, its_bio) > + __field( ino_t, i_ino) > + __field( int, type) > + __field( unsigned long, len_before) > + __field( unsigned long, len_after) > + __field( unsigned long, start) > + __field( int, ret) > + ), > + > + TP_fast_assign_btrfs(btrfs_sb(inode->i_sb), > + __entry->uncompress = uncompress; > + __entry->its_bio = its_bio; > + __entry->i_ino = inode->i_ino; > + __entry->type = type; > + __entry->len_before = len_before; > + __entry->len_after = len_after; > + __entry->start = start; > + __entry->ret = ret; > + ), > + > + TP_printk_btrfs("%s %s ino=%lu type=%s len_before=%lu len_after=%lu " > + "start=%lu ret=%d", > + __entry->uncompress ? "untransform":"transform", decompress/compress. Transform/untransform are as cryptic as they can be. It's a lot easier to put those terms in context when reading the len_before/len_after values. Otherwise one might ask themselves "What kind of transformation are we talking about?" Even if you don't do a v3 you can add: Reviewed-by: Nikolay Borisov <nborisov@suse.com> > + __entry->its_bio ? "bio":"page", __entry->i_ino, > + show_transformer_type(__entry->type), __entry->len_before, > + __entry->len_after, __entry->start, __entry->ret) > + > +); > #endif /* _TRACE_BTRFS_H */ > > /* This part must be outside protection */ > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review. > I haven't read previous submissions and any discussions that might have > occurred there but why not just stick to > btrfs_data_compression/btrfs_data_compressor. I know there is certain > semantic overload since we call it a compressor yet it also does > decompression but let's focus on making the code/intention clear for the > code reader and not bogging down too much on actual word semantics. To > me "compressor" is a synonym to something which compresses AND > decompresses. It's very well possible that this is just me in which case > my argument is flawed and you can ignore it :) The same tracer will also trace encryption at some point in future. >> +#define show_transformer_type(type) \ > > Why not show_compression_type ? as above. >> + TP_printk_btrfs("%s %s ino=%lu type=%s len_before=%lu len_after=%lu " >> + "start=%lu ret=%d", >> + __entry->uncompress ? "untransform":"transform", > > decompress/compress. Transform/untransform are as cryptic as they can > be. It's a lot easier to put those terms in context when reading the > len_before/len_after values. > Otherwise one might ask themselves "What > kind of transformation are we talking about?" > > Even if you don't do a v3 you can add: ha. Thanks. current trace an example: untransform bio ino=258 type=lzo len_before=4096 len_after=16384 start=393216 ret=0 before after size may be same in encryption, so we need extra specifier about the operation. How about: (for example) de-lzo bio ino=258 len_before=4096 len_after=16384 start=393216 ret=0 lzo ino=258 .. Thanks, Anand > Reviewed-by: Nikolay Borisov <nborisov@suse.com> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12.06.2017 12:44, Anand Jain wrote: > > Thanks for the review. > >> I haven't read previous submissions and any discussions that might have >> occurred there but why not just stick to >> btrfs_data_compression/btrfs_data_compressor. I know there is certain >> semantic overload since we call it a compressor yet it also does >> decompression but let's focus on making the code/intention clear for the >> code reader and not bogging down too much on actual word semantics. To >> me "compressor" is a synonym to something which compresses AND >> decompresses. It's very well possible that this is just me in which case >> my argument is flawed and you can ignore it :) > > The same tracer will also trace encryption at some point in future. Right, in that case why don't you introduce the concept of stages/pipeline. We can have a compression stage with either compress/decompress ops. We can have an encryption stage with encrypt/decrypt? How does that abstraction sound? > >>> +#define show_transformer_type(type) \ >> >> Why not show_compression_type ? > > as above. > >>> + TP_printk_btrfs("%s %s ino=%lu type=%s len_before=%lu >>> len_after=%lu " >>> + "start=%lu ret=%d", >>> + __entry->uncompress ? "untransform":"transform", >> >> decompress/compress. Transform/untransform are as cryptic as they can >> be. It's a lot easier to put those terms in context when reading the >> len_before/len_after values. > >> Otherwise one might ask themselves "What >> kind of transformation are we talking about?" >> >> Even if you don't do a v3 you can add: > > ha. Thanks. > > current trace an example: > untransform bio ino=258 type=lzo len_before=4096 len_after=16384 > start=393216 ret=0 Now that I"m seeing this, why not prepent something in front of bio/page .e.g granularity/unit: unit=bio granularity=page I'm more inclined towards the latter. > > before after size may be same in encryption, so we need extra specifier > about the operation. > > How about: (for example) > de-lzo bio ino=258 len_before=4096 len_after=16384 start=393216 ret=0 > lzo ino=258 .. de-lzo still seems a bit botched. what about stage: compress op=lzo-decompress/zlib-compress granularity=bio/page stage: encryption op=XTS-decrypt/encrypt ... ? > > Thanks, Anand > > >> Reviewed-by: Nikolay Borisov <nborisov@suse.com> > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Anand,
[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20170609]
[cannot apply to btrfs/next tip/perf/core v4.12-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-add-compression-trace-points/20170612-184615
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.3.0-18) 6.3.0 20170516
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All warnings (new ones prefixed by >>):
In file included from include/trace/define_trace.h:95:0,
from include/trace/events/btrfs.h:1672,
from fs/btrfs/super.c:65:
include/trace/events/btrfs.h: In function 'trace_raw_output_btrfs_data_transformer':
>> include/trace/events/btrfs.h:91:12: warning: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'ino_t {aka unsigned int}' [-Wformat=]
TP_printk("%pU: " fmt, __entry->fsid, args)
^
include/trace/trace_events.h:343:22: note: in definition of macro 'DECLARE_EVENT_CLASS'
trace_seq_printf(s, print); \
^~~~~
include/trace/trace_events.h:65:9: note: in expansion of macro 'PARAMS'
PARAMS(print)); \
^~~~~~
>> include/trace/events/btrfs.h:1630:1: note: in expansion of macro 'TRACE_EVENT'
TRACE_EVENT(btrfs_data_transformer,
^~~~~~~~~~~
>> include/trace/events/btrfs.h:91:2: note: in expansion of macro 'TP_printk'
TP_printk("%pU: " fmt, __entry->fsid, args)
^~~~~~~~~
>> include/trace/events/btrfs.h:1661:2: note: in expansion of macro 'TP_printk_btrfs'
TP_printk_btrfs("%s %s ino=%lu type=%s len_before=%lu len_after=%lu "
^~~~~~~~~~~~~~~
vim +91 include/trace/events/btrfs.h
3f7de037 Josef Bacik 2011-11-10 75
8c2a3ca2 Josef Bacik 2012-01-10 76 #define BTRFS_UUID_SIZE 16
bc074524 Jeff Mahoney 2016-06-09 77 #define TP_STRUCT__entry_fsid __array(u8, fsid, BTRFS_UUID_SIZE)
bc074524 Jeff Mahoney 2016-06-09 78
bc074524 Jeff Mahoney 2016-06-09 79 #define TP_fast_assign_fsid(fs_info) \
bc074524 Jeff Mahoney 2016-06-09 80 memcpy(__entry->fsid, fs_info->fsid, BTRFS_UUID_SIZE)
bc074524 Jeff Mahoney 2016-06-09 81
bc074524 Jeff Mahoney 2016-06-09 82 #define TP_STRUCT__entry_btrfs(args...) \
bc074524 Jeff Mahoney 2016-06-09 83 TP_STRUCT__entry( \
bc074524 Jeff Mahoney 2016-06-09 84 TP_STRUCT__entry_fsid \
bc074524 Jeff Mahoney 2016-06-09 85 args)
bc074524 Jeff Mahoney 2016-06-09 86 #define TP_fast_assign_btrfs(fs_info, args...) \
bc074524 Jeff Mahoney 2016-06-09 87 TP_fast_assign( \
bc074524 Jeff Mahoney 2016-06-09 88 TP_fast_assign_fsid(fs_info); \
bc074524 Jeff Mahoney 2016-06-09 89 args)
bc074524 Jeff Mahoney 2016-06-09 90 #define TP_printk_btrfs(fmt, args...) \
bc074524 Jeff Mahoney 2016-06-09 @91 TP_printk("%pU: " fmt, __entry->fsid, args)
8c2a3ca2 Josef Bacik 2012-01-10 92
1abe9b8a liubo 2011-03-24 93 TRACE_EVENT(btrfs_transaction_commit,
1abe9b8a liubo 2011-03-24 94
1abe9b8a liubo 2011-03-24 95 TP_PROTO(struct btrfs_root *root),
1abe9b8a liubo 2011-03-24 96
1abe9b8a liubo 2011-03-24 97 TP_ARGS(root),
1abe9b8a liubo 2011-03-24 98
bc074524 Jeff Mahoney 2016-06-09 99 TP_STRUCT__entry_btrfs(
:::::: The code at line 91 was first introduced by commit
:::::: bc074524e123ded281cde25ebc5661910f9679e3 btrfs: prefix fsid to all trace events
:::::: TO: Jeff Mahoney <jeffm@suse.com>
:::::: CC: David Sterba <dsterba@suse.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index fd6508bcff77..53908722d80e 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -884,6 +884,10 @@ int btrfs_compress_pages(int type, struct address_space *mapping, start, pages, out_pages, total_in, total_out); + + trace_btrfs_data_transformer(0, 0, mapping->host, type, *total_in, + *total_out, start, ret); + free_workspace(type, workspace); return ret; } @@ -910,6 +914,10 @@ static int btrfs_decompress_bio(struct compressed_bio *cb) workspace = find_workspace(type); ret = btrfs_compress_op[type - 1]->decompress_bio(workspace, cb); + + trace_btrfs_data_transformer(1, 1, cb->inode, type, + cb->compressed_len, cb->len, cb->start, ret); + free_workspace(type, workspace); return ret; @@ -932,6 +940,9 @@ int btrfs_decompress(int type, unsigned char *data_in, struct page *dest_page, dest_page, start_byte, srclen, destlen); + trace_btrfs_data_transformer(1, 0, dest_page->mapping->host, + type, srclen, destlen, start_byte, ret); + free_workspace(type, workspace); return ret; } diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h index cd99a3658156..7c2442dab6a0 100644 --- a/include/trace/events/btrfs.h +++ b/include/trace/events/btrfs.h @@ -1622,6 +1622,50 @@ TRACE_EVENT(qgroup_meta_reserve, show_root_type(__entry->refroot), __entry->diff) ); +#define show_transformer_type(type) \ + __print_symbolic(type, \ + { BTRFS_COMPRESS_ZLIB, "zlib" }, \ + { BTRFS_COMPRESS_LZO, "lzo" }) + +TRACE_EVENT(btrfs_data_transformer, + + TP_PROTO(int uncompress, int its_bio, struct inode *inode, int type, + unsigned long len_before, unsigned long len_after, + unsigned long start, int ret), + + TP_ARGS(uncompress, its_bio, inode, type, len_before, + len_after, start, ret), + + TP_STRUCT__entry_btrfs( + __field( int, uncompress) + __field( int, its_bio) + __field( ino_t, i_ino) + __field( int, type) + __field( unsigned long, len_before) + __field( unsigned long, len_after) + __field( unsigned long, start) + __field( int, ret) + ), + + TP_fast_assign_btrfs(btrfs_sb(inode->i_sb), + __entry->uncompress = uncompress; + __entry->its_bio = its_bio; + __entry->i_ino = inode->i_ino; + __entry->type = type; + __entry->len_before = len_before; + __entry->len_after = len_after; + __entry->start = start; + __entry->ret = ret; + ), + + TP_printk_btrfs("%s %s ino=%lu type=%s len_before=%lu len_after=%lu " + "start=%lu ret=%d", + __entry->uncompress ? "untransform":"transform", + __entry->its_bio ? "bio":"page", __entry->i_ino, + show_transformer_type(__entry->type), __entry->len_before, + __entry->len_after, __entry->start, __entry->ret) + +); #endif /* _TRACE_BTRFS_H */ /* This part must be outside protection */
This patch adds compression and decompression trace points for the purpose of debugging. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v2: . Use better naming. (If transform is not good enough I have run out of ideas, pls suggest). . To be applied on top of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next (tested without namelen check patch set). fs/btrfs/compression.c | 11 +++++++++++ include/trace/events/btrfs.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+)