don't poke into bio internals

don't poke into bio internals

[Christoph Hellwig]

Hi all,

this series has a few patches that switch btrfs to use the proper helpers for
accessing bio internals.  This helps to prepare for supporting multi-page
bio_vecs, which are currently under development.

[PATCH 1/9] btrfs: use bio iterators for the decompression handlers

[Christoph Hellwig]

Pass the full bio to the decompression routines and use bio iterators
to iterate over the data in the bio.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/compression.c | 122 +++++++++++++++++--------------------------------
 fs/btrfs/compression.h |  12 ++---
 fs/btrfs/lzo.c         |  17 ++-----
 fs/btrfs/zlib.c        |  15 ++----
 4 files changed, 54 insertions(+), 112 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index d4d8b7e..12a631d 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -81,9 +81,9 @@ struct compressed_bio {
 	u32 sums;
 };
 
-static int btrfs_decompress_biovec(int type, struct page **pages_in,
-				   u64 disk_start, struct bio_vec *bvec,
-				   int vcnt, size_t srclen);
+static int btrfs_decompress_bio(int type, struct page **pages_in,
+				   u64 disk_start, struct bio *orig_bio,
+				   size_t srclen);
 
 static inline int compressed_bio_size(struct btrfs_root *root,
 				      unsigned long disk_size)
@@ -175,11 +175,10 @@ 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_biovec(cb->compress_type,
+	ret = btrfs_decompress_bio(cb->compress_type,
 				      cb->compressed_pages,
 				      cb->start,
-				      cb->orig_bio->bi_io_vec,
-				      cb->orig_bio->bi_vcnt,
+				      cb->orig_bio,
 				      cb->compressed_len);
 csum_failed:
 	if (ret)
@@ -959,9 +958,7 @@ int btrfs_compress_pages(int type, struct address_space *mapping,
  *
  * disk_start is the starting logical offset of this array in the file
  *
- * bvec is a bio_vec of pages from the file that we want to decompress into
- *
- * vcnt is the count of pages in the biovec
+ * orig_bio contains the pages from the file that we want to decompress into
  *
  * srclen is the number of bytes in pages_in
  *
@@ -970,18 +967,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_biovec(int type, struct page **pages_in,
-				   u64 disk_start, struct bio_vec *bvec,
-				   int vcnt, size_t srclen)
+static int btrfs_decompress_bio(int type, struct page **pages_in,
+				   u64 disk_start, struct bio *orig_bio,
+				   size_t srclen)
 {
 	struct list_head *workspace;
 	int ret;
 
 	workspace = find_workspace(type);
 
-	ret = btrfs_compress_op[type-1]->decompress_biovec(workspace, pages_in,
-							 disk_start,
-							 bvec, vcnt, srclen);
+	ret = btrfs_compress_op[type-1]->decompress_bio(workspace, pages_in,
+							 disk_start, orig_bio,
+							 srclen);
 	free_workspace(type, workspace);
 	return ret;
 }
@@ -1021,9 +1018,7 @@ void btrfs_exit_compress(void)
  */
 int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
 			      unsigned long total_out, u64 disk_start,
-			      struct bio_vec *bvec, int vcnt,
-			      unsigned long *pg_index,
-			      unsigned long *pg_offset)
+			      struct bio *bio)
 {
 	unsigned long buf_offset;
 	unsigned long current_buf_start;
@@ -1031,13 +1026,13 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
 	unsigned long working_bytes = total_out - buf_start;
 	unsigned long bytes;
 	char *kaddr;
-	struct page *page_out = bvec[*pg_index].bv_page;
+	struct bio_vec bvec = bio_iter_iovec(bio, bio->bi_iter);
 
 	/*
 	 * start byte is the first byte of the page we're currently
 	 * copying into relative to the start of the compressed data.
 	 */
-	start_byte = page_offset(page_out) - disk_start;
+	start_byte = page_offset(bvec.bv_page) - disk_start;
 
 	/* we haven't yet hit data corresponding to this page */
 	if (total_out <= start_byte)
@@ -1057,80 +1052,45 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
 
 	/* copy bytes from the working buffer into the pages */
 	while (working_bytes > 0) {
-		bytes = min(PAGE_SIZE - *pg_offset,
-			    PAGE_SIZE - buf_offset);
+		bytes = min_t(unsigned long, bvec.bv_len,
+				PAGE_SIZE - buf_offset);
 		bytes = min(bytes, working_bytes);
-		kaddr = kmap_atomic(page_out);
-		memcpy(kaddr + *pg_offset, buf + buf_offset, bytes);
+
+		kaddr = kmap_atomic(bvec.bv_page);
+		memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);
 		kunmap_atomic(kaddr);
-		flush_dcache_page(page_out);
+		flush_dcache_page(bvec.bv_page);
 
-		*pg_offset += bytes;
 		buf_offset += bytes;
 		working_bytes -= bytes;
 		current_buf_start += bytes;
 
 		/* check if we need to pick another page */
-		if (*pg_offset == PAGE_SIZE) {
-			(*pg_index)++;
-			if (*pg_index >= vcnt)
-				return 0;
+		bio_advance(bio, bytes);
+		if (!bio->bi_iter.bi_size)
+			return 0;
 
-			page_out = bvec[*pg_index].bv_page;
-			*pg_offset = 0;
-			start_byte = page_offset(page_out) - disk_start;
+		start_byte = page_offset(bvec.bv_page) - disk_start;
 
-			/*
-			 * make sure our new page is covered by this
-			 * working buffer
-			 */
-			if (total_out <= start_byte)
-				return 1;
+		/*
+		 * make sure our new page is covered by this
+		 * working buffer
+		 */
+		if (total_out <= start_byte)
+			return 1;
 
-			/*
-			 * the next page in the biovec might not be adjacent
-			 * to the last page, but it might still be found
-			 * inside this working buffer. bump our offset pointer
-			 */
-			if (total_out > start_byte &&
-			    current_buf_start < start_byte) {
-				buf_offset = start_byte - buf_start;
-				working_bytes = total_out - start_byte;
-				current_buf_start = buf_start + buf_offset;
-			}
+		/*
+		 * the next page in the biovec might not be adjacent
+		 * to the last page, but it might still be found
+		 * inside this working buffer. bump our offset pointer
+		 */
+		if (total_out > start_byte &&
+		    current_buf_start < start_byte) {
+			buf_offset = start_byte - buf_start;
+			working_bytes = total_out - start_byte;
+			current_buf_start = buf_start + buf_offset;
 		}
 	}
 
 	return 1;
 }
-
-/*
- * When uncompressing data, we need to make sure and zero any parts of
- * the biovec that were not filled in by the decompression code.  pg_index
- * and pg_offset indicate the last page and the last offset of that page
- * that have been filled in.  This will zero everything remaining in the
- * biovec.
- */
-void btrfs_clear_biovec_end(struct bio_vec *bvec, int vcnt,
-				   unsigned long pg_index,
-				   unsigned long pg_offset)
-{
-	while (pg_index < vcnt) {
-		struct page *page = bvec[pg_index].bv_page;
-		unsigned long off = bvec[pg_index].bv_offset;
-		unsigned long len = bvec[pg_index].bv_len;
-
-		if (pg_offset < off)
-			pg_offset = off;
-		if (pg_offset < off + len) {
-			unsigned long bytes = off + len - pg_offset;
-			char *kaddr;
-
-			kaddr = kmap_atomic(page);
-			memset(kaddr + pg_offset, 0, bytes);
-			kunmap_atomic(kaddr);
-		}
-		pg_index++;
-		pg_offset = 0;
-	}
-}
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index f49d8b8..0987957 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -34,9 +34,7 @@ int btrfs_decompress(int type, unsigned char *data_in, struct page *dest_page,
 		     unsigned long start_byte, size_t srclen, size_t destlen);
 int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
 			      unsigned long total_out, u64 disk_start,
-			      struct bio_vec *bvec, int vcnt,
-			      unsigned long *pg_index,
-			      unsigned long *pg_offset);
+			      struct bio *bio);
 
 int btrfs_submit_compressed_write(struct inode *inode, u64 start,
 				  unsigned long len, u64 disk_start,
@@ -45,9 +43,6 @@ int btrfs_submit_compressed_write(struct inode *inode, u64 start,
 				  unsigned long nr_pages);
 int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 				 int mirror_num, unsigned long bio_flags);
-void btrfs_clear_biovec_end(struct bio_vec *bvec, int vcnt,
-				   unsigned long pg_index,
-				   unsigned long pg_offset);
 
 enum btrfs_compression_type {
 	BTRFS_COMPRESS_NONE  = 0,
@@ -72,11 +67,10 @@ struct btrfs_compress_op {
 			      unsigned long *total_out,
 			      unsigned long max_out);
 
-	int (*decompress_biovec)(struct list_head *workspace,
+	int (*decompress_bio)(struct list_head *workspace,
 				 struct page **pages_in,
 				 u64 disk_start,
-				 struct bio_vec *bvec,
-				 int vcnt,
+				 struct bio *orig_bio,
 				 size_t srclen);
 
 	int (*decompress)(struct list_head *workspace,
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 48655da..45d2698 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -254,25 +254,21 @@ static int lzo_compress_pages(struct list_head *ws,
 	return ret;
 }
 
-static int lzo_decompress_biovec(struct list_head *ws,
+static int lzo_decompress_bio(struct list_head *ws,
 				 struct page **pages_in,
 				 u64 disk_start,
-				 struct bio_vec *bvec,
-				 int vcnt,
+				 struct bio *orig_bio,
 				 size_t srclen)
 {
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
 	int ret = 0, ret2;
 	char *data_in;
 	unsigned long page_in_index = 0;
-	unsigned long page_out_index = 0;
 	unsigned long total_pages_in = DIV_ROUND_UP(srclen, PAGE_SIZE);
 	unsigned long buf_start;
 	unsigned long buf_offset = 0;
 	unsigned long bytes;
 	unsigned long working_bytes;
-	unsigned long pg_offset;
-
 	size_t in_len;
 	size_t out_len;
 	unsigned long in_offset;
@@ -292,7 +288,6 @@ static int lzo_decompress_biovec(struct list_head *ws,
 	in_page_bytes_left = PAGE_SIZE - LZO_LEN;
 
 	tot_out = 0;
-	pg_offset = 0;
 
 	while (tot_in < tot_len) {
 		in_len = read_compress_length(data_in + in_offset);
@@ -365,16 +360,14 @@ static int lzo_decompress_biovec(struct list_head *ws,
 		tot_out += out_len;
 
 		ret2 = btrfs_decompress_buf2page(workspace->buf, buf_start,
-						 tot_out, disk_start,
-						 bvec, vcnt,
-						 &page_out_index, &pg_offset);
+						 tot_out, disk_start, orig_bio);
 		if (ret2 == 0)
 			break;
 	}
 done:
 	kunmap(pages_in[page_in_index]);
 	if (!ret)
-		btrfs_clear_biovec_end(bvec, vcnt, page_out_index, pg_offset);
+		zero_fill_bio(orig_bio);
 	return ret;
 }
 
@@ -438,6 +431,6 @@ const struct btrfs_compress_op btrfs_lzo_compress = {
 	.alloc_workspace	= lzo_alloc_workspace,
 	.free_workspace		= lzo_free_workspace,
 	.compress_pages		= lzo_compress_pages,
-	.decompress_biovec	= lzo_decompress_biovec,
+	.decompress_bio		= lzo_decompress_bio,
 	.decompress		= lzo_decompress,
 };
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 441b81a..0d5f28e 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -210,10 +210,9 @@ static int zlib_compress_pages(struct list_head *ws,
 	return ret;
 }
 
-static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
+static int zlib_decompress_bio(struct list_head *ws, struct page **pages_in,
 				  u64 disk_start,
-				  struct bio_vec *bvec,
-				  int vcnt,
+				  struct bio *orig_bio,
 				  size_t srclen)
 {
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
@@ -222,10 +221,8 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
 	char *data_in;
 	size_t total_out = 0;
 	unsigned long page_in_index = 0;
-	unsigned long page_out_index = 0;
 	unsigned long total_pages_in = DIV_ROUND_UP(srclen, PAGE_SIZE);
 	unsigned long buf_start;
-	unsigned long pg_offset;
 
 	data_in = kmap(pages_in[page_in_index]);
 	workspace->strm.next_in = data_in;
@@ -235,7 +232,6 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
 	workspace->strm.total_out = 0;
 	workspace->strm.next_out = workspace->buf;
 	workspace->strm.avail_out = PAGE_SIZE;
-	pg_offset = 0;
 
 	/* If it's deflate, and it's got no preset dictionary, then
 	   we can tell zlib to skip the adler32 check. */
@@ -266,8 +262,7 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
 
 		ret2 = btrfs_decompress_buf2page(workspace->buf, buf_start,
 						 total_out, disk_start,
-						 bvec, vcnt,
-						 &page_out_index, &pg_offset);
+						 orig_bio);
 		if (ret2 == 0) {
 			ret = 0;
 			goto done;
@@ -300,7 +295,7 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
 	if (data_in)
 		kunmap(pages_in[page_in_index]);
 	if (!ret)
-		btrfs_clear_biovec_end(bvec, vcnt, page_out_index, pg_offset);
+		zero_fill_bio(orig_bio);
 	return ret;
 }
 
@@ -407,6 +402,6 @@ const struct btrfs_compress_op btrfs_zlib_compress = {
 	.alloc_workspace	= zlib_alloc_workspace,
 	.free_workspace		= zlib_free_workspace,
 	.compress_pages		= zlib_compress_pages,
-	.decompress_biovec	= zlib_decompress_biovec,
+	.decompress_bio		= zlib_decompress_bio,
 	.decompress		= zlib_decompress,
 };
-- 
2.1.4

Re: [PATCH 1/9] btrfs: use bio iterators for the decompression handlers

[Liu Bo]

On Wed, Nov 16, 2016 at 01:52:08PM +0100, Christoph Hellwig wrote:
> Pass the full bio to the decompression routines and use bio iterators
> to iterate over the data in the bio.

One question below,

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/compression.c | 122 +++++++++++++++++--------------------------------
>  fs/btrfs/compression.h |  12 ++---
>  fs/btrfs/lzo.c         |  17 ++-----
>  fs/btrfs/zlib.c        |  15 ++----
>  4 files changed, 54 insertions(+), 112 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index d4d8b7e..12a631d 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -81,9 +81,9 @@ struct compressed_bio {
>  	u32 sums;
>  };
>  
> -static int btrfs_decompress_biovec(int type, struct page **pages_in,
> -				   u64 disk_start, struct bio_vec *bvec,
> -				   int vcnt, size_t srclen);
> +static int btrfs_decompress_bio(int type, struct page **pages_in,
> +				   u64 disk_start, struct bio *orig_bio,
> +				   size_t srclen);
>  
>  static inline int compressed_bio_size(struct btrfs_root *root,
>  				      unsigned long disk_size)
> @@ -175,11 +175,10 @@ 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_biovec(cb->compress_type,
> +	ret = btrfs_decompress_bio(cb->compress_type,
>  				      cb->compressed_pages,
>  				      cb->start,
> -				      cb->orig_bio->bi_io_vec,
> -				      cb->orig_bio->bi_vcnt,
> +				      cb->orig_bio,
>  				      cb->compressed_len);
>  csum_failed:
>  	if (ret)
> @@ -959,9 +958,7 @@ int btrfs_compress_pages(int type, struct address_space *mapping,
>   *
>   * disk_start is the starting logical offset of this array in the file
>   *
> - * bvec is a bio_vec of pages from the file that we want to decompress into
> - *
> - * vcnt is the count of pages in the biovec
> + * orig_bio contains the pages from the file that we want to decompress into
>   *
>   * srclen is the number of bytes in pages_in
>   *
> @@ -970,18 +967,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_biovec(int type, struct page **pages_in,
> -				   u64 disk_start, struct bio_vec *bvec,
> -				   int vcnt, size_t srclen)
> +static int btrfs_decompress_bio(int type, struct page **pages_in,
> +				   u64 disk_start, struct bio *orig_bio,
> +				   size_t srclen)
>  {
>  	struct list_head *workspace;
>  	int ret;
>  
>  	workspace = find_workspace(type);
>  
> -	ret = btrfs_compress_op[type-1]->decompress_biovec(workspace, pages_in,
> -							 disk_start,
> -							 bvec, vcnt, srclen);
> +	ret = btrfs_compress_op[type-1]->decompress_bio(workspace, pages_in,
> +							 disk_start, orig_bio,
> +							 srclen);
>  	free_workspace(type, workspace);
>  	return ret;
>  }
> @@ -1021,9 +1018,7 @@ void btrfs_exit_compress(void)
>   */
>  int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
>  			      unsigned long total_out, u64 disk_start,
> -			      struct bio_vec *bvec, int vcnt,
> -			      unsigned long *pg_index,
> -			      unsigned long *pg_offset)
> +			      struct bio *bio)
>  {
>  	unsigned long buf_offset;
>  	unsigned long current_buf_start;
> @@ -1031,13 +1026,13 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
>  	unsigned long working_bytes = total_out - buf_start;
>  	unsigned long bytes;
>  	char *kaddr;
> -	struct page *page_out = bvec[*pg_index].bv_page;
> +	struct bio_vec bvec = bio_iter_iovec(bio, bio->bi_iter);
>  
>  	/*
>  	 * start byte is the first byte of the page we're currently
>  	 * copying into relative to the start of the compressed data.
>  	 */
> -	start_byte = page_offset(page_out) - disk_start;
> +	start_byte = page_offset(bvec.bv_page) - disk_start;
>  
>  	/* we haven't yet hit data corresponding to this page */
>  	if (total_out <= start_byte)
> @@ -1057,80 +1052,45 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
>  
>  	/* copy bytes from the working buffer into the pages */
>  	while (working_bytes > 0) {
> -		bytes = min(PAGE_SIZE - *pg_offset,
> -			    PAGE_SIZE - buf_offset);
> +		bytes = min_t(unsigned long, bvec.bv_len,
> +				PAGE_SIZE - buf_offset);
>  		bytes = min(bytes, working_bytes);
> -		kaddr = kmap_atomic(page_out);
> -		memcpy(kaddr + *pg_offset, buf + buf_offset, bytes);
> +
> +		kaddr = kmap_atomic(bvec.bv_page);
> +		memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);

This doesn't seem to be right, 'bvec.bv_offset' is not updated in the
following bio_advance(bio, bytes), shouldn't it be 
'kaddr + bvec.bv_offset + bio->bi_iter.bi_bvec_done'?

Others look good.

Thanks,

-liubo
>  		kunmap_atomic(kaddr);
> -		flush_dcache_page(page_out);
> +		flush_dcache_page(bvec.bv_page);
>  
> -		*pg_offset += bytes;
>  		buf_offset += bytes;
>  		working_bytes -= bytes;
>  		current_buf_start += bytes;
>  
>  		/* check if we need to pick another page */
> -		if (*pg_offset == PAGE_SIZE) {
> -			(*pg_index)++;
> -			if (*pg_index >= vcnt)
> -				return 0;
> +		bio_advance(bio, bytes);
> +		if (!bio->bi_iter.bi_size)
> +			return 0;
>  
> -			page_out = bvec[*pg_index].bv_page;
> -			*pg_offset = 0;
> -			start_byte = page_offset(page_out) - disk_start;
> +		start_byte = page_offset(bvec.bv_page) - disk_start;
>  
> -			/*
> -			 * make sure our new page is covered by this
> -			 * working buffer
> -			 */
> -			if (total_out <= start_byte)
> -				return 1;
> +		/*
> +		 * make sure our new page is covered by this
> +		 * working buffer
> +		 */
> +		if (total_out <= start_byte)
> +			return 1;
>  
> -			/*
> -			 * the next page in the biovec might not be adjacent
> -			 * to the last page, but it might still be found
> -			 * inside this working buffer. bump our offset pointer
> -			 */
> -			if (total_out > start_byte &&
> -			    current_buf_start < start_byte) {
> -				buf_offset = start_byte - buf_start;
> -				working_bytes = total_out - start_byte;
> -				current_buf_start = buf_start + buf_offset;
> -			}
> +		/*
> +		 * the next page in the biovec might not be adjacent
> +		 * to the last page, but it might still be found
> +		 * inside this working buffer. bump our offset pointer
> +		 */
> +		if (total_out > start_byte &&
> +		    current_buf_start < start_byte) {
> +			buf_offset = start_byte - buf_start;
> +			working_bytes = total_out - start_byte;
> +			current_buf_start = buf_start + buf_offset;
>  		}
>  	}
>  
>  	return 1;
>  }
> -
> -/*
> - * When uncompressing data, we need to make sure and zero any parts of
> - * the biovec that were not filled in by the decompression code.  pg_index
> - * and pg_offset indicate the last page and the last offset of that page
> - * that have been filled in.  This will zero everything remaining in the
> - * biovec.
> - */
> -void btrfs_clear_biovec_end(struct bio_vec *bvec, int vcnt,
> -				   unsigned long pg_index,
> -				   unsigned long pg_offset)
> -{
> -	while (pg_index < vcnt) {
> -		struct page *page = bvec[pg_index].bv_page;
> -		unsigned long off = bvec[pg_index].bv_offset;
> -		unsigned long len = bvec[pg_index].bv_len;
> -
> -		if (pg_offset < off)
> -			pg_offset = off;
> -		if (pg_offset < off + len) {
> -			unsigned long bytes = off + len - pg_offset;
> -			char *kaddr;
> -
> -			kaddr = kmap_atomic(page);
> -			memset(kaddr + pg_offset, 0, bytes);
> -			kunmap_atomic(kaddr);
> -		}
> -		pg_index++;
> -		pg_offset = 0;
> -	}
> -}
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index f49d8b8..0987957 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -34,9 +34,7 @@ int btrfs_decompress(int type, unsigned char *data_in, struct page *dest_page,
>  		     unsigned long start_byte, size_t srclen, size_t destlen);
>  int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
>  			      unsigned long total_out, u64 disk_start,
> -			      struct bio_vec *bvec, int vcnt,
> -			      unsigned long *pg_index,
> -			      unsigned long *pg_offset);
> +			      struct bio *bio);
>  
>  int btrfs_submit_compressed_write(struct inode *inode, u64 start,
>  				  unsigned long len, u64 disk_start,
> @@ -45,9 +43,6 @@ int btrfs_submit_compressed_write(struct inode *inode, u64 start,
>  				  unsigned long nr_pages);
>  int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  				 int mirror_num, unsigned long bio_flags);
> -void btrfs_clear_biovec_end(struct bio_vec *bvec, int vcnt,
> -				   unsigned long pg_index,
> -				   unsigned long pg_offset);
>  
>  enum btrfs_compression_type {
>  	BTRFS_COMPRESS_NONE  = 0,
> @@ -72,11 +67,10 @@ struct btrfs_compress_op {
>  			      unsigned long *total_out,
>  			      unsigned long max_out);
>  
> -	int (*decompress_biovec)(struct list_head *workspace,
> +	int (*decompress_bio)(struct list_head *workspace,
>  				 struct page **pages_in,
>  				 u64 disk_start,
> -				 struct bio_vec *bvec,
> -				 int vcnt,
> +				 struct bio *orig_bio,
>  				 size_t srclen);
>  
>  	int (*decompress)(struct list_head *workspace,
> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
> index 48655da..45d2698 100644
> --- a/fs/btrfs/lzo.c
> +++ b/fs/btrfs/lzo.c
> @@ -254,25 +254,21 @@ static int lzo_compress_pages(struct list_head *ws,
>  	return ret;
>  }
>  
> -static int lzo_decompress_biovec(struct list_head *ws,
> +static int lzo_decompress_bio(struct list_head *ws,
>  				 struct page **pages_in,
>  				 u64 disk_start,
> -				 struct bio_vec *bvec,
> -				 int vcnt,
> +				 struct bio *orig_bio,
>  				 size_t srclen)
>  {
>  	struct workspace *workspace = list_entry(ws, struct workspace, list);
>  	int ret = 0, ret2;
>  	char *data_in;
>  	unsigned long page_in_index = 0;
> -	unsigned long page_out_index = 0;
>  	unsigned long total_pages_in = DIV_ROUND_UP(srclen, PAGE_SIZE);
>  	unsigned long buf_start;
>  	unsigned long buf_offset = 0;
>  	unsigned long bytes;
>  	unsigned long working_bytes;
> -	unsigned long pg_offset;
> -
>  	size_t in_len;
>  	size_t out_len;
>  	unsigned long in_offset;
> @@ -292,7 +288,6 @@ static int lzo_decompress_biovec(struct list_head *ws,
>  	in_page_bytes_left = PAGE_SIZE - LZO_LEN;
>  
>  	tot_out = 0;
> -	pg_offset = 0;
>  
>  	while (tot_in < tot_len) {
>  		in_len = read_compress_length(data_in + in_offset);
> @@ -365,16 +360,14 @@ static int lzo_decompress_biovec(struct list_head *ws,
>  		tot_out += out_len;
>  
>  		ret2 = btrfs_decompress_buf2page(workspace->buf, buf_start,
> -						 tot_out, disk_start,
> -						 bvec, vcnt,
> -						 &page_out_index, &pg_offset);
> +						 tot_out, disk_start, orig_bio);
>  		if (ret2 == 0)
>  			break;
>  	}
>  done:
>  	kunmap(pages_in[page_in_index]);
>  	if (!ret)
> -		btrfs_clear_biovec_end(bvec, vcnt, page_out_index, pg_offset);
> +		zero_fill_bio(orig_bio);
>  	return ret;
>  }
>  
> @@ -438,6 +431,6 @@ const struct btrfs_compress_op btrfs_lzo_compress = {
>  	.alloc_workspace	= lzo_alloc_workspace,
>  	.free_workspace		= lzo_free_workspace,
>  	.compress_pages		= lzo_compress_pages,
> -	.decompress_biovec	= lzo_decompress_biovec,
> +	.decompress_bio		= lzo_decompress_bio,
>  	.decompress		= lzo_decompress,
>  };
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index 441b81a..0d5f28e 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -210,10 +210,9 @@ static int zlib_compress_pages(struct list_head *ws,
>  	return ret;
>  }
>  
> -static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
> +static int zlib_decompress_bio(struct list_head *ws, struct page **pages_in,
>  				  u64 disk_start,
> -				  struct bio_vec *bvec,
> -				  int vcnt,
> +				  struct bio *orig_bio,
>  				  size_t srclen)
>  {
>  	struct workspace *workspace = list_entry(ws, struct workspace, list);
> @@ -222,10 +221,8 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
>  	char *data_in;
>  	size_t total_out = 0;
>  	unsigned long page_in_index = 0;
> -	unsigned long page_out_index = 0;
>  	unsigned long total_pages_in = DIV_ROUND_UP(srclen, PAGE_SIZE);
>  	unsigned long buf_start;
> -	unsigned long pg_offset;
>  
>  	data_in = kmap(pages_in[page_in_index]);
>  	workspace->strm.next_in = data_in;
> @@ -235,7 +232,6 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
>  	workspace->strm.total_out = 0;
>  	workspace->strm.next_out = workspace->buf;
>  	workspace->strm.avail_out = PAGE_SIZE;
> -	pg_offset = 0;
>  
>  	/* If it's deflate, and it's got no preset dictionary, then
>  	   we can tell zlib to skip the adler32 check. */
> @@ -266,8 +262,7 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
>  
>  		ret2 = btrfs_decompress_buf2page(workspace->buf, buf_start,
>  						 total_out, disk_start,
> -						 bvec, vcnt,
> -						 &page_out_index, &pg_offset);
> +						 orig_bio);
>  		if (ret2 == 0) {
>  			ret = 0;
>  			goto done;
> @@ -300,7 +295,7 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
>  	if (data_in)
>  		kunmap(pages_in[page_in_index]);
>  	if (!ret)
> -		btrfs_clear_biovec_end(bvec, vcnt, page_out_index, pg_offset);
> +		zero_fill_bio(orig_bio);
>  	return ret;
>  }
>  
> @@ -407,6 +402,6 @@ const struct btrfs_compress_op btrfs_zlib_compress = {
>  	.alloc_workspace	= zlib_alloc_workspace,
>  	.free_workspace		= zlib_free_workspace,
>  	.compress_pages		= zlib_compress_pages,
> -	.decompress_biovec	= zlib_decompress_biovec,
> +	.decompress_bio		= zlib_decompress_bio,
>  	.decompress		= zlib_decompress,
>  };
> -- 
> 2.1.4
> 
> --
> 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 1/9] btrfs: use bio iterators for the decompression handlers

[Christoph Hellwig]

On Fri, Nov 18, 2016 at 05:29:06PM -0800, Liu Bo wrote:
> On Wed, Nov 16, 2016 at 01:52:08PM +0100, Christoph Hellwig wrote:
> > Pass the full bio to the decompression routines and use bio iterators
> > to iterate over the data in the bio.
> 
> One question below,

It would be nice to cut down the email to actually find your question
without running through hundreds+ quoted lines.

> >  	/* copy bytes from the working buffer into the pages */
> >  	while (working_bytes > 0) {
> > -		bytes = min(PAGE_SIZE - *pg_offset,
> > -			    PAGE_SIZE - buf_offset);
> > +		bytes = min_t(unsigned long, bvec.bv_len,
> > +				PAGE_SIZE - buf_offset);
> >  		bytes = min(bytes, working_bytes);
> > -		kaddr = kmap_atomic(page_out);
> > -		memcpy(kaddr + *pg_offset, buf + buf_offset, bytes);
> > +
> > +		kaddr = kmap_atomic(bvec.bv_page);
> > +		memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);
> 
> This doesn't seem to be right, 'bvec.bv_offset' is not updated in the
> following bio_advance(bio, bytes),

Good spot - and this means xfstests doesn't cover this area very
well :(

> shouldn't it be 
> 'kaddr + bvec.bv_offset + bio->bi_iter.bi_bvec_done'?

No, we just need to get a new bvec using bio_iter_iovec after
the call to bio_advance.

Re: [PATCH 1/9] btrfs: use bio iterators for the decompression handlers

[Liu Bo]

On Tue, Nov 22, 2016 at 01:38:46AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 18, 2016 at 05:29:06PM -0800, Liu Bo wrote:
> > On Wed, Nov 16, 2016 at 01:52:08PM +0100, Christoph Hellwig wrote:
> > > Pass the full bio to the decompression routines and use bio iterators
> > > to iterate over the data in the bio.
> > 
> > One question below,
> 
> It would be nice to cut down the email to actually find your question
> without running through hundreds+ quoted lines.

Sure :)

> 
> > >  	/* copy bytes from the working buffer into the pages */
> > >  	while (working_bytes > 0) {
> > > -		bytes = min(PAGE_SIZE - *pg_offset,
> > > -			    PAGE_SIZE - buf_offset);
> > > +		bytes = min_t(unsigned long, bvec.bv_len,
> > > +				PAGE_SIZE - buf_offset);
> > >  		bytes = min(bytes, working_bytes);
> > > -		kaddr = kmap_atomic(page_out);
> > > -		memcpy(kaddr + *pg_offset, buf + buf_offset, bytes);
> > > +
> > > +		kaddr = kmap_atomic(bvec.bv_page);
> > > +		memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);
> > 
> > This doesn't seem to be right, 'bvec.bv_offset' is not updated in the
> > following bio_advance(bio, bytes),
> 
> Good spot - and this means xfstests doesn't cover this area very
> well :(

The decompress part starts after checking checksum stored in btrfs, I'm
wondering if fio --verify could detect it.

> 
> > shouldn't it be 
> > 'kaddr + bvec.bv_offset + bio->bi_iter.bi_bvec_done'?
> 
> No, we just need to get a new bvec using bio_iter_iovec after
> the call to bio_advance.

I see, that's great.

Thanks,

-liubo

[PATCH 2/9] btrfs: don't access the bio directly in the raid5/6 code

[Christoph Hellwig]

Just use bio_for_each_segment_all to iterate over all segments.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/raid56.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index d016d4a..da941fb 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1144,10 +1144,10 @@ static void validate_rbio_for_rmw(struct btrfs_raid_bio *rbio)
 static void index_rbio_pages(struct btrfs_raid_bio *rbio)
 {
 	struct bio *bio;
+	struct bio_vec *bvec;
 	u64 start;
 	unsigned long stripe_offset;
 	unsigned long page_index;
-	struct page *p;
 	int i;
 
 	spin_lock_irq(&rbio->bio_list_lock);
@@ -1156,10 +1156,8 @@ static void index_rbio_pages(struct btrfs_raid_bio *rbio)
 		stripe_offset = start - rbio->bbio->raid_map[0];
 		page_index = stripe_offset >> PAGE_SHIFT;
 
-		for (i = 0; i < bio->bi_vcnt; i++) {
-			p = bio->bi_io_vec[i].bv_page;
-			rbio->bio_pages[page_index + i] = p;
-		}
+		bio_for_each_segment_all(bvec, bio, i)
+			rbio->bio_pages[page_index+ i] = bvec->bv_page;
 	}
 	spin_unlock_irq(&rbio->bio_list_lock);
 }
@@ -1433,13 +1431,11 @@ static int fail_bio_stripe(struct btrfs_raid_bio *rbio,
  */
 static void set_bio_pages_uptodate(struct bio *bio)
 {
+	struct bio_vec *bvec;
 	int i;
-	struct page *p;
 
-	for (i = 0; i < bio->bi_vcnt; i++) {
-		p = bio->bi_io_vec[i].bv_page;
-		SetPageUptodate(p);
-	}
+	bio_for_each_segment_all(bvec, bio, i)
+		SetPageUptodate(bvec->bv_page);
 }
 
 /*
-- 
2.1.4

Re: [PATCH 2/9] btrfs: don't access the bio directly in the raid5/6 code

[Omar Sandoval]

On Wed, Nov 16, 2016 at 01:52:09PM +0100, Christoph Hellwig wrote:
> Just use bio_for_each_segment_all to iterate over all segments.
> 

Besides the minor whitespace issue below,

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/raid56.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index d016d4a..da941fb 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1144,10 +1144,10 @@ static void validate_rbio_for_rmw(struct btrfs_raid_bio *rbio)
>  static void index_rbio_pages(struct btrfs_raid_bio *rbio)
>  {
>  	struct bio *bio;
> +	struct bio_vec *bvec;
>  	u64 start;
>  	unsigned long stripe_offset;
>  	unsigned long page_index;
> -	struct page *p;
>  	int i;
>  
>  	spin_lock_irq(&rbio->bio_list_lock);
> @@ -1156,10 +1156,8 @@ static void index_rbio_pages(struct btrfs_raid_bio *rbio)
>  		stripe_offset = start - rbio->bbio->raid_map[0];
>  		page_index = stripe_offset >> PAGE_SHIFT;
>  
> -		for (i = 0; i < bio->bi_vcnt; i++) {
> -			p = bio->bi_io_vec[i].bv_page;
> -			rbio->bio_pages[page_index + i] = p;
> -		}
> +		bio_for_each_segment_all(bvec, bio, i)
> +			rbio->bio_pages[page_index+ i] = bvec->bv_page;

Missing a space right here ----------------------^

>  	}
>  	spin_unlock_irq(&rbio->bio_list_lock);
>  }
> @@ -1433,13 +1431,11 @@ static int fail_bio_stripe(struct btrfs_raid_bio *rbio,
>   */
>  static void set_bio_pages_uptodate(struct bio *bio)
>  {
> +	struct bio_vec *bvec;
>  	int i;
> -	struct page *p;
>  
> -	for (i = 0; i < bio->bi_vcnt; i++) {
> -		p = bio->bi_io_vec[i].bv_page;
> -		SetPageUptodate(p);
> -	}
> +	bio_for_each_segment_all(bvec, bio, i)
> +		SetPageUptodate(bvec->bv_page);
>  }
>  
>  /*
> -- 
> 2.1.4
> 
> --
> 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

-- 
Omar

[PATCH 3/9] btrfs: don't access the bio directly in the raid5/6 code

[Christoph Hellwig]

Just use bio_for_each_segment_all to iterate over all segments.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/inode.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 147df4c..3f09cb6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8394,7 +8394,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct bio *bio;
 	struct bio *orig_bio = dip->orig_bio;
-	struct bio_vec *bvec = orig_bio->bi_io_vec;
+	struct bio_vec *bvec;
 	u64 start_sector = orig_bio->bi_iter.bi_sector;
 	u64 file_offset = dip->logical_offset;
 	u64 submit_len = 0;
@@ -8403,7 +8403,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
 	int async_submit = 0;
 	int nr_sectors;
 	int ret;
-	int i;
+	int i, j;
 
 	map_length = orig_bio->bi_iter.bi_size;
 	ret = btrfs_map_block(root->fs_info, btrfs_op(orig_bio),
@@ -8433,7 +8433,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
 	btrfs_io_bio(bio)->logical = file_offset;
 	atomic_inc(&dip->pending_bios);
 
-	while (bvec <= (orig_bio->bi_io_vec + orig_bio->bi_vcnt - 1)) {
+	bio_for_each_segment_all(bvec, orig_bio, j) {
 		nr_sectors = BTRFS_BYTES_TO_BLKS(root->fs_info, bvec->bv_len);
 		i = 0;
 next_block:
@@ -8487,7 +8487,6 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
 				i++;
 				goto next_block;
 			}
-			bvec++;
 		}
 	}
 
-- 
2.1.4

Re: [PATCH 3/9] btrfs: don't access the bio directly in the raid5/6 code

[Omar Sandoval]

On Wed, Nov 16, 2016 at 01:52:10PM +0100, Christoph Hellwig wrote:
> Just use bio_for_each_segment_all to iterate over all segments.

The subject seems to be copied from patch 2 for this one. Otherwise,

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/inode.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 147df4c..3f09cb6 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8394,7 +8394,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>  	struct bio *bio;
>  	struct bio *orig_bio = dip->orig_bio;
> -	struct bio_vec *bvec = orig_bio->bi_io_vec;
> +	struct bio_vec *bvec;
>  	u64 start_sector = orig_bio->bi_iter.bi_sector;
>  	u64 file_offset = dip->logical_offset;
>  	u64 submit_len = 0;
> @@ -8403,7 +8403,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
>  	int async_submit = 0;
>  	int nr_sectors;
>  	int ret;
> -	int i;
> +	int i, j;
>  
>  	map_length = orig_bio->bi_iter.bi_size;
>  	ret = btrfs_map_block(root->fs_info, btrfs_op(orig_bio),
> @@ -8433,7 +8433,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
>  	btrfs_io_bio(bio)->logical = file_offset;
>  	atomic_inc(&dip->pending_bios);
>  
> -	while (bvec <= (orig_bio->bi_io_vec + orig_bio->bi_vcnt - 1)) {
> +	bio_for_each_segment_all(bvec, orig_bio, j) {
>  		nr_sectors = BTRFS_BYTES_TO_BLKS(root->fs_info, bvec->bv_len);
>  		i = 0;
>  next_block:
> @@ -8487,7 +8487,6 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
>  				i++;
>  				goto next_block;
>  			}
> -			bvec++;
>  		}
>  	}
>  
> -- 
> 2.1.4
> 
> --
> 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

-- 
Omar

[PATCH 4/9] btrfs: don't access the bio directly in btrfs_csum_one_bio

[Christoph Hellwig]

Use bio_for_each_segment_all to iterate over the segments instead.
This requires a bit of reshuffling so that we only lookup up the ordered
item once inside the bio_for_each_segment_all loop.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/file-item.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index d0d571c..fa8aa53 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -447,13 +447,12 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct inode *inode,
 		       struct bio *bio, u64 file_start, int contig)
 {
 	struct btrfs_ordered_sum *sums;
-	struct btrfs_ordered_extent *ordered;
+	struct btrfs_ordered_extent *ordered = NULL;
 	char *data;
-	struct bio_vec *bvec = bio->bi_io_vec;
-	int bio_index = 0;
+	struct bio_vec *bvec;
 	int index;
 	int nr_sectors;
-	int i;
+	int i, j;
 	unsigned long total_bytes = 0;
 	unsigned long this_sum_bytes = 0;
 	u64 offset;
@@ -470,17 +469,20 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct inode *inode,
 	if (contig)
 		offset = file_start;
 	else
-		offset = page_offset(bvec->bv_page) + bvec->bv_offset;
+		offset = 0; /* shut up gcc */
 
-	ordered = btrfs_lookup_ordered_extent(inode, offset);
-	BUG_ON(!ordered); /* Logic error */
 	sums->bytenr = (u64)bio->bi_iter.bi_sector << 9;
 	index = 0;
 
-	while (bio_index < bio->bi_vcnt) {
+	bio_for_each_segment_all(bvec, bio, j) {
 		if (!contig)
 			offset = page_offset(bvec->bv_page) + bvec->bv_offset;
 
+		if (!ordered) {
+			ordered = btrfs_lookup_ordered_extent(inode, offset);
+			BUG_ON(!ordered); /* Logic error */
+		}
+
 		data = kmap_atomic(bvec->bv_page);
 
 		nr_sectors = BTRFS_BYTES_TO_BLKS(root->fs_info,
@@ -529,9 +531,6 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct inode *inode,
 		}
 
 		kunmap_atomic(data);
-
-		bio_index++;
-		bvec++;
 	}
 	this_sum_bytes = 0;
 	btrfs_add_ordered_sum(inode, ordered, sums);
-- 
2.1.4

Re: [PATCH 4/9] btrfs: don't access the bio directly in btrfs_csum_one_bio

[Omar Sandoval]

On Wed, Nov 16, 2016 at 01:52:11PM +0100, Christoph Hellwig wrote:
> Use bio_for_each_segment_all to iterate over the segments instead.
> This requires a bit of reshuffling so that we only lookup up the ordered
> item once inside the bio_for_each_segment_all loop.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/file-item.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index d0d571c..fa8aa53 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -447,13 +447,12 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct inode *inode,
>  		       struct bio *bio, u64 file_start, int contig)
>  {
>  	struct btrfs_ordered_sum *sums;
> -	struct btrfs_ordered_extent *ordered;
> +	struct btrfs_ordered_extent *ordered = NULL;
>  	char *data;
> -	struct bio_vec *bvec = bio->bi_io_vec;
> -	int bio_index = 0;
> +	struct bio_vec *bvec;
>  	int index;
>  	int nr_sectors;
> -	int i;
> +	int i, j;
>  	unsigned long total_bytes = 0;
>  	unsigned long this_sum_bytes = 0;
>  	u64 offset;
> @@ -470,17 +469,20 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct inode *inode,
>  	if (contig)
>  		offset = file_start;
>  	else
> -		offset = page_offset(bvec->bv_page) + bvec->bv_offset;
> +		offset = 0; /* shut up gcc */
>  
> -	ordered = btrfs_lookup_ordered_extent(inode, offset);
> -	BUG_ON(!ordered); /* Logic error */
>  	sums->bytenr = (u64)bio->bi_iter.bi_sector << 9;
>  	index = 0;
>  
> -	while (bio_index < bio->bi_vcnt) {
> +	bio_for_each_segment_all(bvec, bio, j) {
>  		if (!contig)
>  			offset = page_offset(bvec->bv_page) + bvec->bv_offset;
>  
> +		if (!ordered) {
> +			ordered = btrfs_lookup_ordered_extent(inode, offset);
> +			BUG_ON(!ordered); /* Logic error */
> +		}
> +
>  		data = kmap_atomic(bvec->bv_page);
>  
>  		nr_sectors = BTRFS_BYTES_TO_BLKS(root->fs_info,
> @@ -529,9 +531,6 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct inode *inode,
>  		}
>  
>  		kunmap_atomic(data);
> -
> -		bio_index++;
> -		bvec++;
>  	}
>  	this_sum_bytes = 0;
>  	btrfs_add_ordered_sum(inode, ordered, sums);
> -- 
> 2.1.4
> 
> --
> 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

-- 
Omar

[PATCH 5/9] btrfs: use bi_size

[Christoph Hellwig]

Instead of using bi_vcnt to calculate it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/compression.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 12a631d..8618ac3 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -562,7 +562,6 @@ static noinline int add_ra_bio_pages(struct inode *inode,
  *
  * bio->bi_iter.bi_sector points to the compressed extent on disk
  * bio->bi_io_vec points to all of the inode pages
- * bio->bi_vcnt is a count of pages
  *
  * After the compressed pages are read, we copy the bytes into the
  * bio we were passed and then call the bio end_io calls
@@ -574,7 +573,6 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	struct extent_map_tree *em_tree;
 	struct compressed_bio *cb;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-	unsigned long uncompressed_len = bio->bi_vcnt * PAGE_SIZE;
 	unsigned long compressed_len;
 	unsigned long nr_pages;
 	unsigned long pg_index;
@@ -619,7 +617,7 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	free_extent_map(em);
 	em = NULL;
 
-	cb->len = uncompressed_len;
+	cb->len = bio->bi_iter.bi_size;
 	cb->compressed_len = compressed_len;
 	cb->compress_type = extent_compress_type(bio_flags);
 	cb->orig_bio = bio;
@@ -647,8 +645,7 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	add_ra_bio_pages(inode, em_start + em_len, cb);
 
 	/* include any pages we added in add_ra-bio_pages */
-	uncompressed_len = bio->bi_vcnt * PAGE_SIZE;
-	cb->len = uncompressed_len;
+	cb->len = bio->bi_iter.bi_size;
 
 	comp_bio = compressed_bio_alloc(bdev, cur_disk_byte, GFP_NOFS);
 	if (!comp_bio)
-- 
2.1.4

Re: [PATCH 5/9] btrfs: use bi_size

[Omar Sandoval]

On Wed, Nov 16, 2016 at 01:52:12PM +0100, Christoph Hellwig wrote:
> Instead of using bi_vcnt to calculate it.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/compression.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 12a631d..8618ac3 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -562,7 +562,6 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>   *
>   * bio->bi_iter.bi_sector points to the compressed extent on disk
>   * bio->bi_io_vec points to all of the inode pages
> - * bio->bi_vcnt is a count of pages
>   *
>   * After the compressed pages are read, we copy the bytes into the
>   * bio we were passed and then call the bio end_io calls
> @@ -574,7 +573,6 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	struct extent_map_tree *em_tree;
>  	struct compressed_bio *cb;
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> -	unsigned long uncompressed_len = bio->bi_vcnt * PAGE_SIZE;
>  	unsigned long compressed_len;
>  	unsigned long nr_pages;
>  	unsigned long pg_index;
> @@ -619,7 +617,7 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	free_extent_map(em);
>  	em = NULL;
>  
> -	cb->len = uncompressed_len;
> +	cb->len = bio->bi_iter.bi_size;
>  	cb->compressed_len = compressed_len;
>  	cb->compress_type = extent_compress_type(bio_flags);
>  	cb->orig_bio = bio;
> @@ -647,8 +645,7 @@ int btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	add_ra_bio_pages(inode, em_start + em_len, cb);
>  
>  	/* include any pages we added in add_ra-bio_pages */
> -	uncompressed_len = bio->bi_vcnt * PAGE_SIZE;
> -	cb->len = uncompressed_len;
> +	cb->len = bio->bi_iter.bi_size;
>  
>  	comp_bio = compressed_bio_alloc(bdev, cur_disk_byte, GFP_NOFS);
>  	if (!comp_bio)
> -- 
> 2.1.4
> 
> --
> 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

-- 
Omar

[PATCH 6/9] btrfs: calculate end of bio offset properly

[Christoph Hellwig]

Use the bvec offset and len members to prepare for multipage bvecs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/compression.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 8618ac3..27e9feb 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -445,6 +445,13 @@ int btrfs_submit_compressed_write(struct inode *inode, u64 start,
 	return 0;
 }
 
+static u64 bio_end_offset(struct bio *bio)
+{
+	struct bio_vec *last = &bio->bi_io_vec[bio->bi_vcnt - 1];
+
+	return page_offset(last->bv_page) + last->bv_len - last->bv_offset;
+}
+
 static noinline int add_ra_bio_pages(struct inode *inode,
 				     u64 compressed_end,
 				     struct compressed_bio *cb)
@@ -463,8 +470,7 @@ static noinline int add_ra_bio_pages(struct inode *inode,
 	u64 end;
 	int misses = 0;
 
-	page = cb->orig_bio->bi_io_vec[cb->orig_bio->bi_vcnt - 1].bv_page;
-	last_offset = (page_offset(page) + PAGE_SIZE);
+	last_offset = bio_end_offset(cb->orig_bio);
 	em_tree = &BTRFS_I(inode)->extent_tree;
 	tree = &BTRFS_I(inode)->io_tree;
 
-- 
2.1.4

Re: [PATCH 6/9] btrfs: calculate end of bio offset properly

[Omar Sandoval]

On Wed, Nov 16, 2016 at 01:52:13PM +0100, Christoph Hellwig wrote:
> Use the bvec offset and len members to prepare for multipage bvecs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/compression.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 8618ac3..27e9feb 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -445,6 +445,13 @@ int btrfs_submit_compressed_write(struct inode *inode, u64 start,
>  	return 0;
>  }
>  
> +static u64 bio_end_offset(struct bio *bio)
> +{
> +	struct bio_vec *last = &bio->bi_io_vec[bio->bi_vcnt - 1];
> +
> +	return page_offset(last->bv_page) + last->bv_len - last->bv_offset;

Why is this minus bv_offset and not plus? Am I misunderstanding
bv_offset?

> +}
> +
>  static noinline int add_ra_bio_pages(struct inode *inode,
>  				     u64 compressed_end,
>  				     struct compressed_bio *cb)
> @@ -463,8 +470,7 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>  	u64 end;
>  	int misses = 0;
>  
> -	page = cb->orig_bio->bi_io_vec[cb->orig_bio->bi_vcnt - 1].bv_page;
> -	last_offset = (page_offset(page) + PAGE_SIZE);
> +	last_offset = bio_end_offset(cb->orig_bio);
>  	em_tree = &BTRFS_I(inode)->extent_tree;
>  	tree = &BTRFS_I(inode)->io_tree;
>  
> -- 
> 2.1.4
> 
> --
> 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

-- 
Omar

Re: [PATCH 6/9] btrfs: calculate end of bio offset properly

[Christoph Hellwig]

On Fri, Nov 18, 2016 at 12:04:38PM -0800, Omar Sandoval wrote:
> > +static u64 bio_end_offset(struct bio *bio)
> > +{
> > +	struct bio_vec *last = &bio->bi_io_vec[bio->bi_vcnt - 1];
> > +
> > +	return page_offset(last->bv_page) + last->bv_len - last->bv_offset;
> 
> Why is this minus bv_offset and not plus? Am I misunderstanding
> bv_offset?

This should be a plus, thanks.

Can anyone help me on how to get test coverage for the compression
code?

Re: [PATCH 6/9] btrfs: calculate end of bio offset properly

[Omar Sandoval]

On Tue, Nov 22, 2016 at 10:42:40AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 18, 2016 at 12:04:38PM -0800, Omar Sandoval wrote:
> > > +static u64 bio_end_offset(struct bio *bio)
> > > +{
> > > +	struct bio_vec *last = &bio->bi_io_vec[bio->bi_vcnt - 1];
> > > +
> > > +	return page_offset(last->bv_page) + last->bv_len - last->bv_offset;
> > 
> > Why is this minus bv_offset and not plus? Am I misunderstanding
> > bv_offset?
> 
> This should be a plus, thanks.
> 
> Can anyone help me on how to get test coverage for the compression
> code?

I'm not surprised xfstests missed this one since it's just readahead.
You might be able to get better coverage with

export MOUNT_OPTS="-o compress-force"

-- 
Omar

Re: [PATCH 6/9] btrfs: calculate end of bio offset properly

[Anand Jain]

>> Can anyone help me on how to get test coverage for the compression
>> code?
>
> I'm not surprised xfstests missed this one since it's just readahead.
> You might be able to get better coverage with
>
> export MOUNT_OPTS="-o compress-force"

  And where the data is /dev/urandom the btrfs compression will
  bail out, so xfstest cases which uses /dev/urandom won't test
  the compression code.

Re: [PATCH 6/9] btrfs: calculate end of bio offset properly

[Omar Sandoval]

On Wed, Nov 23, 2016 at 12:21:41PM +0800, Anand Jain wrote:
> 
> 
> > > Can anyone help me on how to get test coverage for the compression
> > > code?
> > 
> > I'm not surprised xfstests missed this one since it's just readahead.
> > You might be able to get better coverage with
> > 
> > export MOUNT_OPTS="-o compress-force"
> 
>  And where the data is /dev/urandom the btrfs compression will
>  bail out, so xfstest cases which uses /dev/urandom won't test
>  the compression code.

Isn't that just with "-o compress"? That's why I recommended "-o
compress-force".

-- 
Omar

Re: [PATCH 6/9] btrfs: calculate end of bio offset properly

[Anand Jain]

On 11/23/16 12:21, Omar Sandoval wrote:
> On Wed, Nov 23, 2016 at 12:21:41PM +0800, Anand Jain wrote:
>>
>>
>>>> Can anyone help me on how to get test coverage for the compression
>>>> code?
>>>
>>> I'm not surprised xfstests missed this one since it's just readahead.
>>> You might be able to get better coverage with
>>>
>>> export MOUNT_OPTS="-o compress-force"
>>
>>  And where the data is /dev/urandom the btrfs compression will
>>  bail out, so xfstest cases which uses /dev/urandom won't test
>>  the compression code.
>
> Isn't that just with "-o compress"? That's why I recommended "-o
> compress-force".

  Nope. compress-force doesn't enforce compress even if the data
  isn't compressible. I am not sure if its a bug, but its been
  like that.

  The difference between compress and compress-force is that
  compress-force will never give up and compress will give up
  compress by setting nocompress flag if the first extent is
  not compressible.

Thanks, Anand

Re: [PATCH 6/9] btrfs: calculate end of bio offset properly

[Omar Sandoval]

On Wed, Nov 23, 2016 at 12:32:26PM +0800, Anand Jain wrote:
> 
> 
> On 11/23/16 12:21, Omar Sandoval wrote:
> > On Wed, Nov 23, 2016 at 12:21:41PM +0800, Anand Jain wrote:
> > > 
> > > 
> > > > > Can anyone help me on how to get test coverage for the compression
> > > > > code?
> > > > 
> > > > I'm not surprised xfstests missed this one since it's just readahead.
> > > > You might be able to get better coverage with
> > > > 
> > > > export MOUNT_OPTS="-o compress-force"
> > > 
> > >  And where the data is /dev/urandom the btrfs compression will
> > >  bail out, so xfstest cases which uses /dev/urandom won't test
> > >  the compression code.
> > 
> > Isn't that just with "-o compress"? That's why I recommended "-o
> > compress-force".
> 
>  Nope. compress-force doesn't enforce compress even if the data
>  isn't compressible. I am not sure if its a bug, but its been
>  like that.
> 
>  The difference between compress and compress-force is that
>  compress-force will never give up and compress will give up
>  compress by setting nocompress flag if the first extent is
>  not compressible.
> 
> Thanks, Anand

Huh, you're right. The wording in man 5 btrfs could use some
clarification.

-- 
Omar

[PATCH 7/9] btrfs: refactor __btrfs_lookup_bio_sums to use bio_for_each_segment_all

[Christoph Hellwig]

Rework the loop a little bit to use the generic bio_for_each_segment_all
helper for iterating over the bio.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/file-item.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index fa8aa53..54ccb91 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -163,7 +163,7 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root *root,
 				   struct inode *inode, struct bio *bio,
 				   u64 logical_offset, u32 *dst, int dio)
 {
-	struct bio_vec *bvec = bio->bi_io_vec;
+	struct bio_vec *bvec;
 	struct btrfs_io_bio *btrfs_bio = btrfs_io_bio(bio);
 	struct btrfs_csum_item *item = NULL;
 	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
@@ -177,7 +177,7 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root *root,
 	u32 diff;
 	int nblocks;
 	int bio_index = 0;
-	int count;
+	int count = 0;
 	u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy);
 
 	path = btrfs_alloc_path();
@@ -223,8 +223,11 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root *root,
 	if (dio)
 		offset = logical_offset;
 
-	page_bytes_left = bvec->bv_len;
-	while (bio_index < bio->bi_vcnt) {
+	bio_for_each_segment_all(bvec, bio, bio_index) {
+		page_bytes_left = bvec->bv_len;
+		if (count)
+			goto next;
+
 		if (!dio)
 			offset = page_offset(bvec->bv_page) + bvec->bv_offset;
 		count = btrfs_find_ordered_sum(inode, offset, disk_bytenr,
@@ -285,29 +288,17 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root *root,
 found:
 		csum += count * csum_size;
 		nblocks -= count;
-
+next:
 		while (count--) {
 			disk_bytenr += root->sectorsize;
 			offset += root->sectorsize;
 			page_bytes_left -= root->sectorsize;
-			if (!page_bytes_left) {
-				bio_index++;
-				/*
-				 * make sure we're still inside the
-				 * bio before we update page_bytes_left
-				 */
-				if (bio_index >= bio->bi_vcnt) {
-					WARN_ON_ONCE(count);
-					goto done;
-				}
-				bvec++;
-				page_bytes_left = bvec->bv_len;
-			}
-
+			if (!page_bytes_left)
+				break; /* move to next bio */
 		}
 	}
 
-done:
+	WARN_ON_ONCE(count);
 	btrfs_free_path(path);
 	return 0;
 }
-- 
2.1.4

Re: [PATCH 7/9] btrfs: refactor __btrfs_lookup_bio_sums to use bio_for_each_segment_all

[Omar Sandoval]

On Wed, Nov 16, 2016 at 01:52:14PM +0100, Christoph Hellwig wrote:
> Rework the loop a little bit to use the generic bio_for_each_segment_all
> helper for iterating over the bio.

One minor nit. Besides that,

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/file-item.c | 31 +++++++++++--------------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index fa8aa53..54ccb91 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -163,7 +163,7 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root *root,
>  				   struct inode *inode, struct bio *bio,
>  				   u64 logical_offset, u32 *dst, int dio)
>  {
> -	struct bio_vec *bvec = bio->bi_io_vec;
> +	struct bio_vec *bvec;
>  	struct btrfs_io_bio *btrfs_bio = btrfs_io_bio(bio);
>  	struct btrfs_csum_item *item = NULL;
>  	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> @@ -177,7 +177,7 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root *root,
>  	u32 diff;
>  	int nblocks;
>  	int bio_index = 0;
> -	int count;
> +	int count = 0;
>  	u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy);
>  
>  	path = btrfs_alloc_path();
> @@ -223,8 +223,11 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root *root,
>  	if (dio)
>  		offset = logical_offset;
>  
> -	page_bytes_left = bvec->bv_len;
> -	while (bio_index < bio->bi_vcnt) {
> +	bio_for_each_segment_all(bvec, bio, bio_index) {

Can we just rename bio_index to i now?

> +		page_bytes_left = bvec->bv_len;
> +		if (count)
> +			goto next;
> +
>  		if (!dio)
>  			offset = page_offset(bvec->bv_page) + bvec->bv_offset;
>  		count = btrfs_find_ordered_sum(inode, offset, disk_bytenr,
> @@ -285,29 +288,17 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root *root,
>  found:
>  		csum += count * csum_size;
>  		nblocks -= count;
> -
> +next:
>  		while (count--) {
>  			disk_bytenr += root->sectorsize;
>  			offset += root->sectorsize;
>  			page_bytes_left -= root->sectorsize;
> -			if (!page_bytes_left) {
> -				bio_index++;
> -				/*
> -				 * make sure we're still inside the
> -				 * bio before we update page_bytes_left
> -				 */
> -				if (bio_index >= bio->bi_vcnt) {
> -					WARN_ON_ONCE(count);
> -					goto done;
> -				}
> -				bvec++;
> -				page_bytes_left = bvec->bv_len;
> -			}
> -
> +			if (!page_bytes_left)
> +				break; /* move to next bio */
>  		}
>  	}
>  
> -done:
> +	WARN_ON_ONCE(count);
>  	btrfs_free_path(path);
>  	return 0;
>  }
> -- 
> 2.1.4
> 
> --
> 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

-- 
Omar

[PATCH 8/9] btrfs: use bio_for_each_segment_all in __btrfsic_submit_bio

[Christoph Hellwig]

And remove the bogus check for a NULL return value from kmap, which
can't happen.  While we're at it: I don't think that kmapping up to 256
will work without deadlocks on highmem machines, a better idea would
be to use vm_map_ram to map all of them into a single virtual address
range.  Incidentally that would also simplify the code a lot.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/check-integrity.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index a6f657f..86f681f 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -2819,10 +2819,11 @@ static void __btrfsic_submit_bio(struct bio *bio)
 	 * btrfsic_mount(), this might return NULL */
 	dev_state = btrfsic_dev_state_lookup(bio->bi_bdev);
 	if (NULL != dev_state &&
-	    (bio_op(bio) == REQ_OP_WRITE) && NULL != bio->bi_io_vec) {
+	    (bio_op(bio) == REQ_OP_WRITE) && bio_has_data(bio)) {
 		unsigned int i;
 		u64 dev_bytenr;
 		u64 cur_bytenr;
+		struct bio_vec *bvec;
 		int bio_is_patched;
 		char **mapped_datav;
 
@@ -2840,32 +2841,23 @@ static void __btrfsic_submit_bio(struct bio *bio)
 		if (!mapped_datav)
 			goto leave;
 		cur_bytenr = dev_bytenr;
-		for (i = 0; i < bio->bi_vcnt; i++) {
-			BUG_ON(bio->bi_io_vec[i].bv_len != PAGE_SIZE);
-			mapped_datav[i] = kmap(bio->bi_io_vec[i].bv_page);
-			if (!mapped_datav[i]) {
-				while (i > 0) {
-					i--;
-					kunmap(bio->bi_io_vec[i].bv_page);
-				}
-				kfree(mapped_datav);
-				goto leave;
-			}
+
+		bio_for_each_segment_all(bvec, bio, i) {
+			BUG_ON(bvec->bv_len != PAGE_SIZE);
+			mapped_datav[i] = kmap(bvec->bv_page);
+
 			if (dev_state->state->print_mask &
 			    BTRFSIC_PRINT_MASK_SUBMIT_BIO_BH_VERBOSE)
 				pr_info("#%u: bytenr=%llu, len=%u, offset=%u\n",
-				       i, cur_bytenr, bio->bi_io_vec[i].bv_len,
-				       bio->bi_io_vec[i].bv_offset);
-			cur_bytenr += bio->bi_io_vec[i].bv_len;
+				       i, cur_bytenr, bvec->bv_len, bvec->bv_offset);
+			cur_bytenr += bvec->bv_len;
 		}
 		btrfsic_process_written_block(dev_state, dev_bytenr,
 					      mapped_datav, bio->bi_vcnt,
 					      bio, &bio_is_patched,
 					      NULL, bio->bi_opf);
-		while (i > 0) {
-			i--;
-			kunmap(bio->bi_io_vec[i].bv_page);
-		}
+		bio_for_each_segment_all(bvec, bio, i)
+			kunmap(bvec->bv_page);
 		kfree(mapped_datav);
 	} else if (NULL != dev_state && (bio->bi_opf & REQ_PREFLUSH)) {
 		if (dev_state->state->print_mask &
-- 
2.1.4

Re: [PATCH 8/9] btrfs: use bio_for_each_segment_all in __btrfsic_submit_bio

[Omar Sandoval]

On Wed, Nov 16, 2016 at 01:52:15PM +0100, Christoph Hellwig wrote:
> And remove the bogus check for a NULL return value from kmap, which
> can't happen.  While we're at it: I don't think that kmapping up to 256
> will work without deadlocks on highmem machines, a better idea would
> be to use vm_map_ram to map all of them into a single virtual address
> range.  Incidentally that would also simplify the code a lot.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/check-integrity.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index a6f657f..86f681f 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -2819,10 +2819,11 @@ static void __btrfsic_submit_bio(struct bio *bio)
>  	 * btrfsic_mount(), this might return NULL */
>  	dev_state = btrfsic_dev_state_lookup(bio->bi_bdev);
>  	if (NULL != dev_state &&
> -	    (bio_op(bio) == REQ_OP_WRITE) && NULL != bio->bi_io_vec) {
> +	    (bio_op(bio) == REQ_OP_WRITE) && bio_has_data(bio)) {
>  		unsigned int i;
>  		u64 dev_bytenr;
>  		u64 cur_bytenr;
> +		struct bio_vec *bvec;
>  		int bio_is_patched;
>  		char **mapped_datav;
>  
> @@ -2840,32 +2841,23 @@ static void __btrfsic_submit_bio(struct bio *bio)
>  		if (!mapped_datav)
>  			goto leave;
>  		cur_bytenr = dev_bytenr;
> -		for (i = 0; i < bio->bi_vcnt; i++) {
> -			BUG_ON(bio->bi_io_vec[i].bv_len != PAGE_SIZE);
> -			mapped_datav[i] = kmap(bio->bi_io_vec[i].bv_page);
> -			if (!mapped_datav[i]) {
> -				while (i > 0) {
> -					i--;
> -					kunmap(bio->bi_io_vec[i].bv_page);
> -				}
> -				kfree(mapped_datav);
> -				goto leave;
> -			}
> +
> +		bio_for_each_segment_all(bvec, bio, i) {
> +			BUG_ON(bvec->bv_len != PAGE_SIZE);
> +			mapped_datav[i] = kmap(bvec->bv_page);
> +
>  			if (dev_state->state->print_mask &
>  			    BTRFSIC_PRINT_MASK_SUBMIT_BIO_BH_VERBOSE)
>  				pr_info("#%u: bytenr=%llu, len=%u, offset=%u\n",
> -				       i, cur_bytenr, bio->bi_io_vec[i].bv_len,
> -				       bio->bi_io_vec[i].bv_offset);
> -			cur_bytenr += bio->bi_io_vec[i].bv_len;
> +				       i, cur_bytenr, bvec->bv_len, bvec->bv_offset);
> +			cur_bytenr += bvec->bv_len;
>  		}
>  		btrfsic_process_written_block(dev_state, dev_bytenr,
>  					      mapped_datav, bio->bi_vcnt,
>  					      bio, &bio_is_patched,
>  					      NULL, bio->bi_opf);
> -		while (i > 0) {
> -			i--;
> -			kunmap(bio->bi_io_vec[i].bv_page);
> -		}
> +		bio_for_each_segment_all(bvec, bio, i)
> +			kunmap(bvec->bv_page);
>  		kfree(mapped_datav);
>  	} else if (NULL != dev_state && (bio->bi_opf & REQ_PREFLUSH)) {
>  		if (dev_state->state->print_mask &
> -- 
> 2.1.4
> 
> --
> 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

-- 
Omar

[PATCH 9/9] btrfs: only check bio size to see if a repair bio should have the failfast flag

[Christoph Hellwig]

The number of pages in a bio is a bad indicatator for the number of
splits lower levels could do, and with the multipage bio_vec work even
that measure goes away and will become a number of segments of physically
contiguous areas instead.  Check the total bio size vs the sector size
instead, which gives us an indication without any false negatives,
although the false positive rate might increase a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/extent_io.c | 4 ++--
 fs/btrfs/inode.c     | 4 +---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index ea9ade7..a05fc41 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2296,7 +2296,7 @@ int btrfs_check_repairable(struct inode *inode, struct bio *failed_bio,
 	 *	a) deliver good data to the caller
 	 *	b) correct the bad sectors on disk
 	 */
-	if (failed_bio->bi_vcnt > 1) {
+	if (failed_bio->bi_iter.bi_size > BTRFS_I(inode)->root->sectorsize) {
 		/*
 		 * to fulfill b), we need to know the exact failing sectors, as
 		 * we don't want to rewrite any more than the failed ones. thus,
@@ -2403,7 +2403,7 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
 		return -EIO;
 	}
 
-	if (failed_bio->bi_vcnt > 1)
+	if (failed_bio->bi_iter.bi_size > BTRFS_I(inode)->root->sectorsize)
 		read_mode = READ_SYNC | REQ_FAILFAST_DEV;
 	else
 		read_mode = READ_SYNC;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3f09cb6..54afe41 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7933,9 +7933,7 @@ static int dio_read_error(struct inode *inode, struct bio *failed_bio,
 		return -EIO;
 	}
 
-	if ((failed_bio->bi_vcnt > 1)
-		|| (failed_bio->bi_io_vec->bv_len
-			> BTRFS_I(inode)->root->sectorsize))
+	if (failed_bio->bi_iter.bi_size > BTRFS_I(inode)->root->sectorsize)
 		read_mode = READ_SYNC | REQ_FAILFAST_DEV;
 	else
 		read_mode = READ_SYNC;
-- 
2.1.4

Re: don't poke into bio internals

[David Sterba]

On Wed, Nov 16, 2016 at 01:52:07PM +0100, Christoph Hellwig wrote:
> this series has a few patches that switch btrfs to use the proper helpers for
> accessing bio internals.  This helps to prepare for supporting multi-page
> bio_vecs, which are currently under development.

Looks good to me, thanks. I'll let it pass through tests, expected
merge target is 4.10.