Re: [PATCH 1/3 v2] btrfs: btrfs_decompress_bio() could accept compressed_bio instead

Re: [PATCH 1/3 v2] btrfs: btrfs_decompress_bio() could accept compressed_bio instead

[Nikolay Borisov]

On 26.05.2017 10:44, Anand Jain wrote:
> Instead of sending each argument of struct compressed_bio, send
> the compressed_bio itself.
> 
> Also by having struct compressed_bio in btrfs_decompress_bio()
> it would help tracing.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: no changes
>  fs/btrfs/compression.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 10e6b282d09d..ee934e612f15 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -81,9 +81,7 @@ struct compressed_bio {
>  	u32 sums;
>  };
>  
> -static int btrfs_decompress_bio(int type, struct page **pages_in,
> -				   u64 disk_start, struct bio *orig_bio,
> -				   size_t srclen);
> +static int btrfs_decompress_bio(struct compressed_bio *cb);
>  
>  static inline int compressed_bio_size(struct btrfs_fs_info *fs_info,
>  				      unsigned long disk_size)
> @@ -173,11 +171,8 @@ static void end_compressed_bio_read(struct bio *bio)
>  	/* ok, we're the last bio for this extent, lets start
>  	 * the decompression.
>  	 */
> -	ret = btrfs_decompress_bio(cb->compress_type,
> -				      cb->compressed_pages,
> -				      cb->start,
> -				      cb->orig_bio,
> -				      cb->compressed_len);
> +	ret = btrfs_decompress_bio(cb);
> +
>  csum_failed:
>  	if (ret)
>  		cb->errors = 1;
> @@ -961,18 +956,18 @@ int btrfs_compress_pages(int type, struct address_space *mapping,
>   * be contiguous.  They all correspond to the range of bytes covered by
>   * the compressed extent.
>   */
> -static int btrfs_decompress_bio(int type, struct page **pages_in,
> -				   u64 disk_start, struct bio *orig_bio,
> -				   size_t srclen)
> +static int btrfs_decompress_bio(struct compressed_bio *cb)
>  {
>  	struct list_head *workspace;
>  	int ret;
> +	int type = cb->compress_type;
>  
>  	workspace = find_workspace(type);
>  
> -	ret = btrfs_compress_op[type-1]->decompress_bio(workspace, pages_in,
> -							 disk_start, orig_bio,
> -							 srclen);
> +	ret = btrfs_compress_op[type-1]->decompress_bio(workspace,
> +			cb->compressed_pages, cb->start, cb->orig_bio,
> +			cb->compressed_len);
> +
>  	free_workspace(type, workspace);
>  	return ret;
>  }
> 

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 2/3 v2] btrfs: reduce arguments for decompress_bio ops

[Nikolay Borisov]

On 26.05.2017 10:44, Anand Jain wrote:
> struct compressed_bio pointer can be used instead.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: added this patch
>  fs/btrfs/compression.c | 43 +------------------------------------------
>  fs/btrfs/compression.h | 44 ++++++++++++++++++++++++++++++++++++++++----
>  fs/btrfs/lzo.c         | 10 +++++-----
>  fs/btrfs/zlib.c        |  9 +++++----
>  4 files changed, 51 insertions(+), 55 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index ee934e612f15..e426a8f427b5 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -42,45 +42,6 @@
>  #include "extent_io.h"
>  #include "extent_map.h"
>  
> -struct compressed_bio {
> -	/* number of bios pending for this compressed extent */
> -	refcount_t pending_bios;
> -
> -	/* the pages with the compressed data on them */
> -	struct page **compressed_pages;
> -
> -	/* inode that owns this data */
> -	struct inode *inode;
> -
> -	/* starting offset in the inode for our pages */
> -	u64 start;
> -
> -	/* number of bytes in the inode we're working on */
> -	unsigned long len;
> -
> -	/* number of bytes on disk */
> -	unsigned long compressed_len;
> -
> -	/* the compression algorithm for this bio */
> -	int compress_type;
> -
> -	/* number of compressed pages in the array */
> -	unsigned long nr_pages;
> -
> -	/* IO errors */
> -	int errors;
> -	int mirror_num;
> -
> -	/* for reads, this is the bio we are copying the data into */
> -	struct bio *orig_bio;
> -
> -	/*
> -	 * the start of a variable length array of checksums only
> -	 * used by reads
> -	 */
> -	u32 sums;
> -};
> -
>  static int btrfs_decompress_bio(struct compressed_bio *cb);
>  
>  static inline int compressed_bio_size(struct btrfs_fs_info *fs_info,
> @@ -964,9 +925,7 @@ static int btrfs_decompress_bio(struct compressed_bio *cb)
>  
>  	workspace = find_workspace(type);
>  
> -	ret = btrfs_compress_op[type-1]->decompress_bio(workspace,
> -			cb->compressed_pages, cb->start, cb->orig_bio,
> -			cb->compressed_len);
> +	ret = btrfs_compress_op[type-1]->decompress_bio(workspace, cb);
>  
>  	free_workspace(type, workspace);
>  	return ret;
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index 39ec43ab8df1..89bcf975efb8 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -34,6 +34,45 @@
>  /* Maximum size of data before compression */
>  #define BTRFS_MAX_UNCOMPRESSED		(SZ_128K)
>  
> +struct compressed_bio {
> +	/* number of bios pending for this compressed extent */
> +	refcount_t pending_bios;
> +
> +	/* the pages with the compressed data on them */
> +	struct page **compressed_pages;
> +
> +	/* inode that owns this data */
> +	struct inode *inode;
> +
> +	/* starting offset in the inode for our pages */
> +	u64 start;
> +
> +	/* number of bytes in the inode we're working on */
> +	unsigned long len;
> +
> +	/* number of bytes on disk */
> +	unsigned long compressed_len;
> +
> +	/* the compression algorithm for this bio */
> +	int compress_type;
> +
> +	/* number of compressed pages in the array */
> +	unsigned long nr_pages;
> +
> +	/* IO errors */
> +	int errors;
> +	int mirror_num;
> +
> +	/* for reads, this is the bio we are copying the data into */
> +	struct bio *orig_bio;
> +
> +	/*
> +	 * the start of a variable length array of checksums only
> +	 * used by reads
> +	 */
> +	u32 sums;
> +};
> +
>  void btrfs_init_compress(void);
>  void btrfs_exit_compress(void);
>  
> @@ -78,10 +117,7 @@ struct btrfs_compress_op {
>  			      unsigned long *total_out);
>  
>  	int (*decompress_bio)(struct list_head *workspace,
> -				 struct page **pages_in,
> -				 u64 disk_start,
> -				 struct bio *orig_bio,
> -				 size_t srclen);
> +				struct compressed_bio *cb);
>  
>  	int (*decompress)(struct list_head *workspace,
>  			  unsigned char *data_in,
> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
> index f48c8c14dc14..29b2e1319e6e 100644
> --- a/fs/btrfs/lzo.c
> +++ b/fs/btrfs/lzo.c
> @@ -254,16 +254,13 @@ static int lzo_compress_pages(struct list_head *ws,
>  	return ret;
>  }
>  
> -static int lzo_decompress_bio(struct list_head *ws,
> -				 struct page **pages_in,
> -				 u64 disk_start,
> -				 struct bio *orig_bio,
> -				 size_t srclen)
> +static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
>  {
>  	struct workspace *workspace = list_entry(ws, struct workspace, list);
>  	int ret = 0, ret2;
>  	char *data_in;
>  	unsigned long page_in_index = 0;
> +	size_t srclen = cb->compressed_len;
>  	unsigned long total_pages_in = DIV_ROUND_UP(srclen, PAGE_SIZE);
>  	unsigned long buf_start;
>  	unsigned long buf_offset = 0;
> @@ -278,6 +275,9 @@ static int lzo_decompress_bio(struct list_head *ws,
>  	unsigned long tot_len;
>  	char *buf;
>  	bool may_late_unmap, need_unmap;
> +	struct page **pages_in = cb->compressed_pages;
> +	u64 disk_start = cb->start;
> +	struct bio *orig_bio = cb->orig_bio;
>  
>  	data_in = kmap(pages_in[0]);
>  	tot_len = read_compress_length(data_in);
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index 135b10823c6d..c0f592406bb1 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -211,10 +211,7 @@ static int zlib_compress_pages(struct list_head *ws,
>  	return ret;
>  }
>  
> -static int zlib_decompress_bio(struct list_head *ws, struct page **pages_in,
> -				  u64 disk_start,
> -				  struct bio *orig_bio,
> -				  size_t srclen)
> +static int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
>  {
>  	struct workspace *workspace = list_entry(ws, struct workspace, list);
>  	int ret = 0, ret2;
> @@ -222,8 +219,12 @@ static int zlib_decompress_bio(struct list_head *ws, struct page **pages_in,
>  	char *data_in;
>  	size_t total_out = 0;
>  	unsigned long page_in_index = 0;
> +	size_t srclen = cb->compressed_len;
>  	unsigned long total_pages_in = DIV_ROUND_UP(srclen, PAGE_SIZE);
>  	unsigned long buf_start;
> +	struct page **pages_in = cb->compressed_pages;
> +	u64 disk_start = cb->start;
> +	struct bio *orig_bio = cb->orig_bio;
>  
>  	data_in = kmap(pages_in[page_in_index]);
>  	workspace->strm.next_in = data_in;
> 


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

[PATCH 0/3 v2] compression ops args clean up and trace point

[Anand Jain]

Patch 1 and 2 are preparatory and clean up patches, and patch 3 adds
the trace point for the compression.

I have used same trace function for both compress and decompress
as I wanted to maintain compress and decompress debug data aligned

v2: added patch #2
Anand Jain (3):
  btrfs: btrfs_decompress_bio() could accept compressed_bio instead
  btrfs: reduce arguments for decompress_bio ops
  btrfs: add compression trace points

 fs/btrfs/compression.c       | 70 +++++++++++---------------------------------
 fs/btrfs/compression.h       | 44 +++++++++++++++++++++++++---
 fs/btrfs/lzo.c               | 10 +++----
 fs/btrfs/zlib.c              |  9 +++---
 include/trace/events/btrfs.h | 36 +++++++++++++++++++++++
 5 files changed, 103 insertions(+), 66 deletions(-)

-- 
2.10.0

[PATCH 1/3 v2] btrfs: btrfs_decompress_bio() could accept compressed_bio instead

[Anand Jain]

Instead of sending each argument of struct compressed_bio, send
the compressed_bio itself.

Also by having struct compressed_bio in btrfs_decompress_bio()
it would help tracing.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: no changes
 fs/btrfs/compression.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 10e6b282d09d..ee934e612f15 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -81,9 +81,7 @@ struct compressed_bio {
 	u32 sums;
 };
 
-static int btrfs_decompress_bio(int type, struct page **pages_in,
-				   u64 disk_start, struct bio *orig_bio,
-				   size_t srclen);
+static int btrfs_decompress_bio(struct compressed_bio *cb);
 
 static inline int compressed_bio_size(struct btrfs_fs_info *fs_info,
 				      unsigned long disk_size)
@@ -173,11 +171,8 @@ static void end_compressed_bio_read(struct bio *bio)
 	/* ok, we're the last bio for this extent, lets start
 	 * the decompression.
 	 */
-	ret = btrfs_decompress_bio(cb->compress_type,
-				      cb->compressed_pages,
-				      cb->start,
-				      cb->orig_bio,
-				      cb->compressed_len);
+	ret = btrfs_decompress_bio(cb);
+
 csum_failed:
 	if (ret)
 		cb->errors = 1;
@@ -961,18 +956,18 @@ int btrfs_compress_pages(int type, struct address_space *mapping,
  * be contiguous.  They all correspond to the range of bytes covered by
  * the compressed extent.
  */
-static int btrfs_decompress_bio(int type, struct page **pages_in,
-				   u64 disk_start, struct bio *orig_bio,
-				   size_t srclen)
+static int btrfs_decompress_bio(struct compressed_bio *cb)
 {
 	struct list_head *workspace;
 	int ret;
+	int type = cb->compress_type;
 
 	workspace = find_workspace(type);
 
-	ret = btrfs_compress_op[type-1]->decompress_bio(workspace, pages_in,
-							 disk_start, orig_bio,
-							 srclen);
+	ret = btrfs_compress_op[type-1]->decompress_bio(workspace,
+			cb->compressed_pages, cb->start, cb->orig_bio,
+			cb->compressed_len);
+
 	free_workspace(type, workspace);
 	return ret;
 }
-- 
2.10.0

[PATCH 2/3 v2] btrfs: reduce arguments for decompress_bio ops

[Anand Jain]

struct compressed_bio pointer can be used instead.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: added this patch
 fs/btrfs/compression.c | 43 +------------------------------------------
 fs/btrfs/compression.h | 44 ++++++++++++++++++++++++++++++++++++++++----
 fs/btrfs/lzo.c         | 10 +++++-----
 fs/btrfs/zlib.c        |  9 +++++----
 4 files changed, 51 insertions(+), 55 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index ee934e612f15..e426a8f427b5 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -42,45 +42,6 @@
 #include "extent_io.h"
 #include "extent_map.h"
 
-struct compressed_bio {
-	/* number of bios pending for this compressed extent */
-	refcount_t pending_bios;
-
-	/* the pages with the compressed data on them */
-	struct page **compressed_pages;
-
-	/* inode that owns this data */
-	struct inode *inode;
-
-	/* starting offset in the inode for our pages */
-	u64 start;
-
-	/* number of bytes in the inode we're working on */
-	unsigned long len;
-
-	/* number of bytes on disk */
-	unsigned long compressed_len;
-
-	/* the compression algorithm for this bio */
-	int compress_type;
-
-	/* number of compressed pages in the array */
-	unsigned long nr_pages;
-
-	/* IO errors */
-	int errors;
-	int mirror_num;
-
-	/* for reads, this is the bio we are copying the data into */
-	struct bio *orig_bio;
-
-	/*
-	 * the start of a variable length array of checksums only
-	 * used by reads
-	 */
-	u32 sums;
-};
-
 static int btrfs_decompress_bio(struct compressed_bio *cb);
 
 static inline int compressed_bio_size(struct btrfs_fs_info *fs_info,
@@ -964,9 +925,7 @@ static int btrfs_decompress_bio(struct compressed_bio *cb)
 
 	workspace = find_workspace(type);
 
-	ret = btrfs_compress_op[type-1]->decompress_bio(workspace,
-			cb->compressed_pages, cb->start, cb->orig_bio,
-			cb->compressed_len);
+	ret = btrfs_compress_op[type-1]->decompress_bio(workspace, cb);
 
 	free_workspace(type, workspace);
 	return ret;
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 39ec43ab8df1..89bcf975efb8 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -34,6 +34,45 @@
 /* Maximum size of data before compression */
 #define BTRFS_MAX_UNCOMPRESSED		(SZ_128K)
 
+struct compressed_bio {
+	/* number of bios pending for this compressed extent */
+	refcount_t pending_bios;
+
+	/* the pages with the compressed data on them */
+	struct page **compressed_pages;
+
+	/* inode that owns this data */
+	struct inode *inode;
+
+	/* starting offset in the inode for our pages */
+	u64 start;
+
+	/* number of bytes in the inode we're working on */
+	unsigned long len;
+
+	/* number of bytes on disk */
+	unsigned long compressed_len;
+
+	/* the compression algorithm for this bio */
+	int compress_type;
+
+	/* number of compressed pages in the array */
+	unsigned long nr_pages;
+
+	/* IO errors */
+	int errors;
+	int mirror_num;
+
+	/* for reads, this is the bio we are copying the data into */
+	struct bio *orig_bio;
+
+	/*
+	 * the start of a variable length array of checksums only
+	 * used by reads
+	 */
+	u32 sums;
+};
+
 void btrfs_init_compress(void);
 void btrfs_exit_compress(void);
 
@@ -78,10 +117,7 @@ struct btrfs_compress_op {
 			      unsigned long *total_out);
 
 	int (*decompress_bio)(struct list_head *workspace,
-				 struct page **pages_in,
-				 u64 disk_start,
-				 struct bio *orig_bio,
-				 size_t srclen);
+				struct compressed_bio *cb);
 
 	int (*decompress)(struct list_head *workspace,
 			  unsigned char *data_in,
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index f48c8c14dc14..29b2e1319e6e 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -254,16 +254,13 @@ static int lzo_compress_pages(struct list_head *ws,
 	return ret;
 }
 
-static int lzo_decompress_bio(struct list_head *ws,
-				 struct page **pages_in,
-				 u64 disk_start,
-				 struct bio *orig_bio,
-				 size_t srclen)
+static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 {
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
 	int ret = 0, ret2;
 	char *data_in;
 	unsigned long page_in_index = 0;
+	size_t srclen = cb->compressed_len;
 	unsigned long total_pages_in = DIV_ROUND_UP(srclen, PAGE_SIZE);
 	unsigned long buf_start;
 	unsigned long buf_offset = 0;
@@ -278,6 +275,9 @@ static int lzo_decompress_bio(struct list_head *ws,
 	unsigned long tot_len;
 	char *buf;
 	bool may_late_unmap, need_unmap;
+	struct page **pages_in = cb->compressed_pages;
+	u64 disk_start = cb->start;
+	struct bio *orig_bio = cb->orig_bio;
 
 	data_in = kmap(pages_in[0]);
 	tot_len = read_compress_length(data_in);
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 135b10823c6d..c0f592406bb1 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -211,10 +211,7 @@ static int zlib_compress_pages(struct list_head *ws,
 	return ret;
 }
 
-static int zlib_decompress_bio(struct list_head *ws, struct page **pages_in,
-				  u64 disk_start,
-				  struct bio *orig_bio,
-				  size_t srclen)
+static int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 {
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
 	int ret = 0, ret2;
@@ -222,8 +219,12 @@ static int zlib_decompress_bio(struct list_head *ws, struct page **pages_in,
 	char *data_in;
 	size_t total_out = 0;
 	unsigned long page_in_index = 0;
+	size_t srclen = cb->compressed_len;
 	unsigned long total_pages_in = DIV_ROUND_UP(srclen, PAGE_SIZE);
 	unsigned long buf_start;
+	struct page **pages_in = cb->compressed_pages;
+	u64 disk_start = cb->start;
+	struct bio *orig_bio = cb->orig_bio;
 
 	data_in = kmap(pages_in[page_in_index]);
 	workspace->strm.next_in = data_in;
-- 
2.10.0

[PATCH 3/3 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 <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,
+
+	TP_PROTO(int encode, int bio, struct inode *inode, int type,
+			unsigned long bfr, unsigned long aft,
+			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",
+		__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 */
-- 
2.10.0

Re: [PATCH 2/3 v2] btrfs: reduce arguments for decompress_bio ops

[Anand Jain]

>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>


Thanks.

Anand

Re: [PATCH 2/3 v2] btrfs: reduce arguments for decompress_bio ops

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

Hi Anand,

[auto build test ERROR on tip/perf/core]
[also build test ERROR on v4.12-rc2 next-20170526]
[cannot apply to btrfs/next]
[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/compression-ops-args-clean-up-and-trace-point/20170526-184159
config: s390-default_defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
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 errors (new ones prefixed by >>):

   In file included from fs/btrfs/zlib.c:33:0:
>> fs/btrfs/compression.h:39:2: error: unknown type name 'refcount_t'
     refcount_t pending_bios;
     ^~~~~~~~~~

vim +/refcount_t +39 fs/btrfs/compression.h

    33	#define BTRFS_MAX_COMPRESSED		(SZ_128K)
    34	/* Maximum size of data before compression */
    35	#define BTRFS_MAX_UNCOMPRESSED		(SZ_128K)
    36	
    37	struct compressed_bio {
    38		/* number of bios pending for this compressed extent */
  > 39		refcount_t pending_bios;
    40	
    41		/* the pages with the compressed data on them */
    42		struct page **compressed_pages;

---
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: 17191 bytes --]

Re: [PATCH 1/3 v2] btrfs: btrfs_decompress_bio() could accept compressed_bio instead

[David Sterba]

On Fri, May 26, 2017 at 03:44:58PM +0800, Anand Jain wrote:
> Instead of sending each argument of struct compressed_bio, send
> the compressed_bio itself.
> 
> Also by having struct compressed_bio in btrfs_decompress_bio()
> it would help tracing.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: no changes
>  fs/btrfs/compression.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 10e6b282d09d..ee934e612f15 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -81,9 +81,7 @@ struct compressed_bio {
>  	u32 sums;
>  };
>  
> -static int btrfs_decompress_bio(int type, struct page **pages_in,
> -				   u64 disk_start, struct bio *orig_bio,
> -				   size_t srclen);
> +static int btrfs_decompress_bio(struct compressed_bio *cb);
>  
>  static inline int compressed_bio_size(struct btrfs_fs_info *fs_info,
>  				      unsigned long disk_size)
> @@ -173,11 +171,8 @@ static void end_compressed_bio_read(struct bio *bio)
>  	/* ok, we're the last bio for this extent, lets start
>  	 * the decompression.
>  	 */
> -	ret = btrfs_decompress_bio(cb->compress_type,
> -				      cb->compressed_pages,
> -				      cb->start,
> -				      cb->orig_bio,
> -				      cb->compressed_len);
> +	ret = btrfs_decompress_bio(cb);

Nice.

> +
>  csum_failed:
>  	if (ret)
>  		cb->errors = 1;
> @@ -961,18 +956,18 @@ int btrfs_compress_pages(int type, struct address_space *mapping,
>   * be contiguous.  They all correspond to the range of bytes covered by
>   * the compressed extent.
>   */
> -static int btrfs_decompress_bio(int type, struct page **pages_in,
> -				   u64 disk_start, struct bio *orig_bio,
> -				   size_t srclen)
> +static int btrfs_decompress_bio(struct compressed_bio *cb)
>  {
>  	struct list_head *workspace;
>  	int ret;
> +	int type = cb->compress_type;
>  
>  	workspace = find_workspace(type);
>  
> -	ret = btrfs_compress_op[type-1]->decompress_bio(workspace, pages_in,
> -							 disk_start, orig_bio,
> -							 srclen);
> +	ret = btrfs_compress_op[type-1]->decompress_bio(workspace,

A future note: if you spot minor codingstyle issues, feel free to fix
them on the way, ie. "type-1" to "type - 1".

> +			cb->compressed_pages, cb->start, cb->orig_bio,
> +			cb->compressed_len);
> +
>  	free_workspace(type, workspace);
>  	return ret;
>  }
> -- 
> 2.10.0
> 
> --
> 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

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

[David Sterba]

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.

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

> +			unsigned long bfr, unsigned long aft,

please do not abbreviate that much, this is not necessary and makes it
unreadable

> +			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 ":"

> +		__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 */

Re: [PATCH 0/3 v2] compression ops args clean up and trace point

[David Sterba]

On Fri, May 26, 2017 at 03:44:57PM +0800, Anand Jain wrote:
> Patch 1 and 2 are preparatory and clean up patches, and patch 3 adds
> the trace point for the compression.

1 and 2 added to queue, thanks.

[PATCH] fixup: btrfs: reduce arguments for decompress_bio ops

[Anand Jain]

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Hi David,
 I note the original patch [1] is already queued so here I am sending
 a fix-up patch can you pls squash this to [1]
  [1]
   [PATCH 2/3 v2] btrfs: reduce arguments for decompress_bio ops

 This fixup patch, fixes issues reported by kbuild test robot which
 reported the build failure with ARCH=s390, looking at what was missed
 i.e to include refcount.h it puzzles me why there wasn't such an error
 on my default x86 build though. Thanks.

 fs/btrfs/lzo.c  | 1 +
 fs/btrfs/zlib.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 29b2e1319e6e..5995563ca56f 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -25,6 +25,7 @@
 #include <linux/pagemap.h>
 #include <linux/bio.h>
 #include <linux/lzo.h>
+#include <linux/refcount.h>
 #include "compression.h"
 
 #define LZO_LEN	4
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index c0f592406bb1..d5446e18bb59 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -30,6 +30,7 @@
 #include <linux/sched.h>
 #include <linux/pagemap.h>
 #include <linux/bio.h>
+#include <linux/refcount.h>
 #include "compression.h"
 
 struct workspace {
-- 
2.10.0

Re: [PATCH 2/3 v2] btrfs: reduce arguments for decompress_bio ops

[Anand Jain]

On 05/26/2017 10:07 PM, kbuild test robot wrote:
> Hi Anand,
>
> [auto build test ERROR on tip/perf/core]
> [also build test ERROR on v4.12-rc2 next-20170526]
> [cannot apply to btrfs/next]
> [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/compression-ops-args-clean-up-and-trace-point/20170526-184159
> config: s390-default_defconfig (attached as .config)
> compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> 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 errors (new ones prefixed by >>):
>
>    In file included from fs/btrfs/zlib.c:33:0:
>>> fs/btrfs/compression.h:39:2: error: unknown type name 'refcount_t'
>      refcount_t pending_bios;
>      ^~~~~~~~~~

  Ok . Thanks. Fixed this with.
     fixup: btrfs: reduce arguments for decompress_bio ops

-Anand


> vim +/refcount_t +39 fs/btrfs/compression.h
>
>     33	#define BTRFS_MAX_COMPRESSED		(SZ_128K)
>     34	/* Maximum size of data before compression */
>     35	#define BTRFS_MAX_UNCOMPRESSED		(SZ_128K)
>     36	
>     37	struct compressed_bio {
>     38		/* number of bios pending for this compressed extent */
>   > 39		refcount_t pending_bios;
>     40	
>     41		/* the pages with the compressed data on them */
>     42		struct page **compressed_pages;
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>

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

[Anand Jain]

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

Re: [PATCH] fixup: btrfs: reduce arguments for decompress_bio ops

[David Sterba]

On Sat, May 27, 2017 at 06:36:33AM +0800, Anand Jain wrote:
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> Hi David,
>  I note the original patch [1] is already queued so here I am sending
>  a fix-up patch can you pls squash this to [1]
>   [1]
>    [PATCH 2/3 v2] btrfs: reduce arguments for decompress_bio ops
> 
>  This fixup patch, fixes issues reported by kbuild test robot which
>  reported the build failure with ARCH=s390, looking at what was missed
>  i.e to include refcount.h it puzzles me why there wasn't such an error
>  on my default x86 build though. Thanks.

Thanks, patch updated.