Re: [PATCH v2] btrfs: add compression trace points

[kbuild test robot]

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  80memcpy(__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  83TP_STRUCT__entry(   
\
bc074524 Jeff Mahoney 2016-06-09  84TP_STRUCT__entry_fsid   
\
bc074524 Jeff Mahoney 2016-06-09  85args)
bc074524 Jeff Mahoney 2016-06-09  86  #define TP_fast_assign_btrfs(fs_info, 
args...)\
bc074524 Jeff Mahoney 2016-06-09  87TP_fast_assign( 
\
bc074524 Jeff Mahoney 2016-06-09  88TP_fast_assign_fsid(fs_info);   
\
bc074524 Jeff Mahoney 2016-06-09  89args)
bc074524 Jeff Mahoney 2016-06-09  90  #define TP_printk_btrfs(fmt, args...) \
bc074524 Jeff Mahoney 2016-06-09 @91TP_printk("%pU: " fmt, __entry->fsid, 
args)
8c2a3ca2 Josef Bacik  2012-01-10  92  
1abe9b8a liubo2011-03-24  93  TRACE_EVENT(btrfs_transaction_commit,
1abe9b8a liubo2011-03-24  94  
1abe9b8a liubo2011-03-24  95TP_PROTO(struct btrfs_root *root),
1abe9b8a liubo2011-03-24  96  
1abe9b8a liubo2011-03-24  97TP_ARGS(root),
1abe9b8a liubo2011-03-24  98  
bc074524 Jeff Mahoney 2016-06-09  99TP_STRUCT__entry_btrfs(

:: The code at line 91 was first introduced by commit
:: bc074524e123ded281cde25ebc5661910f9679e3 btrfs: prefix fsid to all trace 
events

:: TO: Jeff Mahoney 
:: CC: David Sterba 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v2] btrfs: add compression trace points

[Nikolay Borisov]

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 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] btrfs: add compression trace points

[Anand Jain]

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 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] btrfs: add compression trace points

[Nikolay Borisov]

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 
> ---
> 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 

[PATCH v2] btrfs: add compression trace points

[Anand Jain]

This patch adds compression and decompression trace points for the
purpose of debugging.

Signed-off-by: Anand Jain 
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html