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

[Anand Jain <anand.jain@oracle.com>]

On 05/27/2017 02:08 AM, David Sterba wrote:
> On Fri, May 26, 2017 at 03:45:00PM +0800, 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: no change
>>  fs/btrfs/compression.c       | 10 ++++++++++
>>  include/trace/events/btrfs.h | 36 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+)
>>
>> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
>> index e426a8f427b5..490590e62a2f 100644
>> --- a/fs/btrfs/compression.c
>> +++ b/fs/btrfs/compression.c
>> @@ -899,6 +899,10 @@ int btrfs_compress_pages(int type, struct address_space *mapping,
>>  						      start, pages,
>>  						      out_pages,
>>  						      total_in, total_out);
>> +
>> +	trace_btrfs_encoder(1, 0, mapping->host, type, *total_in,
>> +						*total_out, start, ret);
>> +
>>  	free_workspace(type, workspace);
>>  	return ret;
>>  }
>> @@ -927,6 +931,9 @@ static int btrfs_decompress_bio(struct compressed_bio *cb)
>>
>>  	ret = btrfs_compress_op[type-1]->decompress_bio(workspace, cb);
>>
>> +	trace_btrfs_encoder(0, 1, cb->inode, type,
>> +				cb->compressed_len, cb->len, cb->start, ret);
>> +
>>  	free_workspace(type, workspace);
>>  	return ret;
>>  }
>> @@ -948,6 +955,9 @@ int btrfs_decompress(int type, unsigned char *data_in, struct page *dest_page,
>>  						  dest_page, start_byte,
>>  						  srclen, destlen);
>>
>> +	trace_btrfs_encoder(0, 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 e37973526153..1ebffcd005a1 100644
>> --- a/include/trace/events/btrfs.h
>> +++ b/include/trace/events/btrfs.h
>> @@ -1658,6 +1658,42 @@ TRACE_EVENT(qgroup_meta_reserve,
>>  		show_root_type(__entry->refroot), __entry->diff)
>>  );
>>
>> +TRACE_EVENT(btrfs_encoder,
>
> So far the encoder is just compression/decompression. Having one
> tracepoint for both operations makes more sense, but I don't like the
> name.

  I am ok with any suggestion.
  In the long this will trace encryption as well so I was avoiding
  compress specific term here. The other choice I had was
     btrfs_tfm (transformer)

  Also planning to rename compression.c to encoder.c (or anything
  suggested), struct compressed_bio to encoder_bio (or anything
  suggested). etc..

>> +
>> +	TP_PROTO(int encode, int bio, struct inode *inode, int type,
>
> encode is confusing here, it seems to be an operation type
>
> bio as an int is confusing, seems to describe the data source type, or
> what 'pge' is supposed to mean

  I had a version with the string passed, feel that I was better.
  Will change it.

>> +			unsigned long bfr, unsigned long aft,
>
> please do not abbreviate that much, this is not necessary and makes it
> unreadable

  ok will change it.

>> +			unsigned long start, int ret),
>> +
>> +	TP_ARGS(encode, bio, inode, type, bfr, aft, start, ret),
>> +
>> +	TP_STRUCT__entry_btrfs(
>> +		__field(	int,		encode)
>> +		__field(	int,		bio)
>> +		__field(	unsigned long,	i_ino)
>> +		__field(	int,		type)
>> +		__field(	unsigned long,	bfr)
>> +		__field(	unsigned long,	aft)
>> +		__field(	unsigned long,	start)
>> +		__field(	int,		ret)
>> +	),
>> +
>> +	TP_fast_assign_btrfs(btrfs_sb(inode->i_sb),
>> +		__entry->encode		= encode;
>> +		__entry->bio		= bio;
>> +		__entry->i_ino		= inode->i_ino;
>> +		__entry->type		= type;
>> +		__entry->bfr		= bfr;
>> +		__entry->aft		= aft;
>> +		__entry->start		= start;
>> +		__entry->ret		= ret;
>> +	),
>> +
>> +	TP_printk_btrfs("%s %s ino:%lu tfm:%d bfr:%lu aft:%lu start:%lu ret:%d",
>
> please use "=" instead of ":"

  I'll do that.

Thanks, Anand


>> +		__entry->encode ? "encode":"decode",
>> +		__entry->bio ? "bio":"pge", __entry->i_ino, __entry->type,
>> +		__entry->bfr, __entry->aft, __entry->start, __entry->ret)
>> +
>> +);
>>  #endif /* _TRACE_BTRFS_H */
>>
>>  /* This part must be outside protection */