* [PATCH v2] btrfs: add compression trace points
@ 2017-06-12 8:32 Anand Jain
2017-06-12 8:44 ` Nikolay Borisov
2017-06-12 12:55 ` kbuild test robot
0 siblings, 2 replies; 5+ messages in thread
From: Anand Jain @ 2017-06-12 8:32 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba
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(+)
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 */
--
2.13.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: add compression trace points
2017-06-12 8:32 [PATCH v2] btrfs: add compression trace points Anand Jain
@ 2017-06-12 8:44 ` Nikolay Borisov
2017-06-12 9:44 ` Anand Jain
2017-06-12 12:55 ` kbuild test robot
1 sibling, 1 reply; 5+ messages in thread
From: Nikolay Borisov @ 2017-06-12 8:44 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: dsterba
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 */
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: add compression trace points
2017-06-12 8:44 ` Nikolay Borisov
@ 2017-06-12 9:44 ` Anand Jain
2017-06-12 10:36 ` Nikolay Borisov
0 siblings, 1 reply; 5+ messages in thread
From: Anand Jain @ 2017-06-12 9:44 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs; +Cc: dsterba
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: add compression trace points
2017-06-12 9:44 ` Anand Jain
@ 2017-06-12 10:36 ` Nikolay Borisov
0 siblings, 0 replies; 5+ messages in thread
From: Nikolay Borisov @ 2017-06-12 10:36 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: dsterba
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>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: add compression trace points
2017-06-12 8:32 [PATCH v2] btrfs: add compression trace points Anand Jain
2017-06-12 8:44 ` Nikolay Borisov
@ 2017-06-12 12:55 ` kbuild test robot
1 sibling, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2017-06-12 12:55 UTC (permalink / raw)
To: Anand Jain; +Cc: kbuild-all, linux-btrfs, dsterba
[-- Attachment #1: Type: text/plain, Size: 4088 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46562 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-12 12:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-12 8:32 [PATCH v2] btrfs: add compression trace points Anand Jain
2017-06-12 8:44 ` Nikolay Borisov
2017-06-12 9:44 ` Anand Jain
2017-06-12 10:36 ` Nikolay Borisov
2017-06-12 12:55 ` kbuild test robot
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.