diff mbox series

[v2,05/13] btrfs: dont assume compressed_bio sums to be 4 bytes

Message ID 20190516084803.9774-6-jthumshirn@suse.de (mailing list archive)
State New, archived
Headers show
Series Add support for SHA-256 checksums | expand

Commit Message

Johannes Thumshirn May 16, 2019, 8:47 a.m. UTC 
BTRFS has the implicit assumption that a checksum in compressed_bio is 4
bytes. While this is true for CRC32C, it is not for any other checksum.

Change the data type to be a byte array and adjust loop index calculation
accordingly.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/compression.c  | 27 +++++++++++++++++----------
 fs/btrfs/compression.h  |  2 +-
 fs/btrfs/file-item.c    |  2 +-
 fs/btrfs/ordered-data.c |  3 ++-
 4 files changed, 21 insertions(+), 13 deletions(-)

Comments

Nikolay Borisov May 21, 2019, 12:56 p.m. UTC | #1 
On 16.05.19 г. 11:47 ч., Johannes Thumshirn wrote:
> BTRFS has the implicit assumption that a checksum in compressed_bio is 4
> bytes. While this is true for CRC32C, it is not for any other checksum.
> 
> Change the data type to be a byte array and adjust loop index calculation
> accordingly.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/compression.c  | 27 +++++++++++++++++----------
>  fs/btrfs/compression.h  |  2 +-
>  fs/btrfs/file-item.c    |  2 +-
>  fs/btrfs/ordered-data.c |  3 ++-
>  4 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 98d8c2ed367f..d5642f3b5c44 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -57,12 +57,14 @@ static int check_compressed_csum(struct btrfs_inode *inode,
>  				 struct compressed_bio *cb,
>  				 u64 disk_start)
>  {
> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +	u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
>  	int ret;
>  	struct page *page;
>  	unsigned long i;
>  	char *kaddr;
>  	u32 csum;
> -	u32 *cb_sum = &cb->sums;
> +	u8 *cb_sum = cb->sums;
>  
>  	if (inode->flags & BTRFS_INODE_NODATASUM)
>  		return 0;
> @@ -76,13 +78,13 @@ static int check_compressed_csum(struct btrfs_inode *inode,
>  		btrfs_csum_final(csum, (u8 *)&csum);
>  		kunmap_atomic(kaddr);
>  
> -		if (csum != *cb_sum) {
> +		if (memcmp(&csum, cb_sum, csum_size)) {
>  			btrfs_print_data_csum_error(inode, disk_start, csum,
> -					*cb_sum, cb->mirror_num);
> +					*(u32 *)cb_sum, cb->mirror_num);
>  			ret = -EIO;
>  			goto fail;
>  		}
> -		cb_sum++;
> +		cb_sum += csum_size;
>  
>  	}
>  	ret = 0;
> @@ -537,7 +539,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	struct extent_map *em;
>  	blk_status_t ret = BLK_STS_RESOURCE;
>  	int faili = 0;
> -	u32 *sums;
> +	u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
> +	u8 *sums;
>  
>  	em_tree = &BTRFS_I(inode)->extent_tree;
>  
> @@ -559,7 +562,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	cb->errors = 0;
>  	cb->inode = inode;
>  	cb->mirror_num = mirror_num;
> -	sums = &cb->sums;
> +	sums = cb->sums;
>  
>  	cb->start = em->orig_start;
>  	em_len = em->len;
> @@ -618,6 +621,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  		page->mapping = NULL;
>  		if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) <
>  		    PAGE_SIZE) {
> +			unsigned int nr_sectors;
> +
>  			ret = btrfs_bio_wq_end_io(fs_info, comp_bio,
>  						  BTRFS_WQ_ENDIO_DATA);
>  			BUG_ON(ret); /* -ENOMEM */
> @@ -632,11 +637,13 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  
>  			if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
>  				ret = btrfs_lookup_bio_sums(inode, comp_bio,
> -							    (u8 *)sums);
> +							    sums);
>  				BUG_ON(ret); /* -ENOMEM */
>  			}
> -			sums += DIV_ROUND_UP(comp_bio->bi_iter.bi_size,
> -					     fs_info->sectorsize);
> +
> +			nr_sectors = DIV_ROUND_UP(comp_bio->bi_iter.bi_size,
> +						  fs_info->sectorsize);
> +			sums += csum_size * nr_sectors;

nit: I think nr_sectors is not a good name in this particular case
because you really care about nr_csums this bio spans. To me at least,
it feels more intuitive to see :

nr_csums = DIV_ROUND_UP
sums += csum_size * nr_csums.


>  
>  			ret = btrfs_map_bio(fs_info, comp_bio, mirror_num, 0);
>  			if (ret) {
> @@ -658,7 +665,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	BUG_ON(ret); /* -ENOMEM */
>  
>  	if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
> -		ret = btrfs_lookup_bio_sums(inode, comp_bio, (u8 *) sums);
> +		ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
>  		BUG_ON(ret); /* -ENOMEM */
>  	}
>  
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index 9976fe0f7526..191e5f4e3523 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -61,7 +61,7 @@ struct compressed_bio {
>  	 * the start of a variable length array of checksums only
>  	 * used by reads
>  	 */
> -	u32 sums;
> +	u8 sums[];
>  };
>  
>  static inline unsigned int btrfs_compress_type(unsigned int type_level)
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 210ff69917a0..c551479afa63 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -182,7 +182,7 @@ static blk_status_t __btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio
>  		}
>  		csum = btrfs_bio->csum;
>  	} else {
> -		csum = (u8 *)dst;
> +		csum = dst;
>  	}
>  
>  	if (bio->bi_iter.bi_size > PAGE_SIZE * 8)
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index 6f7a18148dcb..a65e5f32160b 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -927,9 +927,10 @@ int btrfs_find_ordered_sum(struct inode *inode, u64 offset, u64 disk_bytenr,
>  			   u8 *sum, int len)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> +	struct btrfs_inode *btrfs_inode = BTRFS_I(inode);
>  	struct btrfs_ordered_sum *ordered_sum;
>  	struct btrfs_ordered_extent *ordered;
> -	struct btrfs_ordered_inode_tree *tree = &BTRFS_I(inode)->ordered_tree;
> +	struct btrfs_ordered_inode_tree *tree = &btrfs_inode->ordered_tree;
>  	unsigned long num_sectors;
>  	unsigned long i;
>  	u32 sectorsize = btrfs_inode_sectorsize(inode);
> 

Irrelevant change, this hunk could be dropped. Furthermore, I don't see
how having an explicit variable brings any value apart from increased
stack usage.
Johannes Thumshirn May 21, 2019, 1 p.m. UTC | #2 
On Tue, May 21, 2019 at 03:56:09PM +0300, Nikolay Borisov wrote:
[...]
> >  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> > +	struct btrfs_inode *btrfs_inode = BTRFS_I(inode);
> >  	struct btrfs_ordered_sum *ordered_sum;
> >  	struct btrfs_ordered_extent *ordered;
> > -	struct btrfs_ordered_inode_tree *tree = &BTRFS_I(inode)->ordered_tree;
> > +	struct btrfs_ordered_inode_tree *tree = &btrfs_inode->ordered_tree;
> >  	unsigned long num_sectors;
> >  	unsigned long i;
> >  	u32 sectorsize = btrfs_inode_sectorsize(inode);
> > 
> 
> Irrelevant change, this hunk could be dropped. Furthermore, I don't see
> how having an explicit variable brings any value apart from increased
> stack usage.
> 

I have no idea how and why this crept in, probably due to a rebase.
diff mbox series

Patch

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 98d8c2ed367f..d5642f3b5c44 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -57,12 +57,14 @@  static int check_compressed_csum(struct btrfs_inode *inode,
 				 struct compressed_bio *cb,
 				 u64 disk_start)
 {
+	struct btrfs_fs_info *fs_info = inode->root->fs_info;
+	u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
 	int ret;
 	struct page *page;
 	unsigned long i;
 	char *kaddr;
 	u32 csum;
-	u32 *cb_sum = &cb->sums;
+	u8 *cb_sum = cb->sums;
 
 	if (inode->flags & BTRFS_INODE_NODATASUM)
 		return 0;
@@ -76,13 +78,13 @@  static int check_compressed_csum(struct btrfs_inode *inode,
 		btrfs_csum_final(csum, (u8 *)&csum);
 		kunmap_atomic(kaddr);
 
-		if (csum != *cb_sum) {
+		if (memcmp(&csum, cb_sum, csum_size)) {
 			btrfs_print_data_csum_error(inode, disk_start, csum,
-					*cb_sum, cb->mirror_num);
+					*(u32 *)cb_sum, cb->mirror_num);
 			ret = -EIO;
 			goto fail;
 		}
-		cb_sum++;
+		cb_sum += csum_size;
 
 	}
 	ret = 0;
@@ -537,7 +539,8 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	struct extent_map *em;
 	blk_status_t ret = BLK_STS_RESOURCE;
 	int faili = 0;
-	u32 *sums;
+	u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
+	u8 *sums;
 
 	em_tree = &BTRFS_I(inode)->extent_tree;
 
@@ -559,7 +562,7 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	cb->errors = 0;
 	cb->inode = inode;
 	cb->mirror_num = mirror_num;
-	sums = &cb->sums;
+	sums = cb->sums;
 
 	cb->start = em->orig_start;
 	em_len = em->len;
@@ -618,6 +621,8 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 		page->mapping = NULL;
 		if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) <
 		    PAGE_SIZE) {
+			unsigned int nr_sectors;
+
 			ret = btrfs_bio_wq_end_io(fs_info, comp_bio,
 						  BTRFS_WQ_ENDIO_DATA);
 			BUG_ON(ret); /* -ENOMEM */
@@ -632,11 +637,13 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 
 			if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
 				ret = btrfs_lookup_bio_sums(inode, comp_bio,
-							    (u8 *)sums);
+							    sums);
 				BUG_ON(ret); /* -ENOMEM */
 			}
-			sums += DIV_ROUND_UP(comp_bio->bi_iter.bi_size,
-					     fs_info->sectorsize);
+
+			nr_sectors = DIV_ROUND_UP(comp_bio->bi_iter.bi_size,
+						  fs_info->sectorsize);
+			sums += csum_size * nr_sectors;
 
 			ret = btrfs_map_bio(fs_info, comp_bio, mirror_num, 0);
 			if (ret) {
@@ -658,7 +665,7 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	BUG_ON(ret); /* -ENOMEM */
 
 	if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
-		ret = btrfs_lookup_bio_sums(inode, comp_bio, (u8 *) sums);
+		ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
 		BUG_ON(ret); /* -ENOMEM */
 	}
 
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 9976fe0f7526..191e5f4e3523 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -61,7 +61,7 @@  struct compressed_bio {
 	 * the start of a variable length array of checksums only
 	 * used by reads
 	 */
-	u32 sums;
+	u8 sums[];
 };
 
 static inline unsigned int btrfs_compress_type(unsigned int type_level)
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 210ff69917a0..c551479afa63 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -182,7 +182,7 @@  static blk_status_t __btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio
 		}
 		csum = btrfs_bio->csum;
 	} else {
-		csum = (u8 *)dst;
+		csum = dst;
 	}
 
 	if (bio->bi_iter.bi_size > PAGE_SIZE * 8)
diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index 6f7a18148dcb..a65e5f32160b 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -927,9 +927,10 @@  int btrfs_find_ordered_sum(struct inode *inode, u64 offset, u64 disk_bytenr,
 			   u8 *sum, int len)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+	struct btrfs_inode *btrfs_inode = BTRFS_I(inode);
 	struct btrfs_ordered_sum *ordered_sum;
 	struct btrfs_ordered_extent *ordered;
-	struct btrfs_ordered_inode_tree *tree = &BTRFS_I(inode)->ordered_tree;
+	struct btrfs_ordered_inode_tree *tree = &btrfs_inode->ordered_tree;
 	unsigned long num_sectors;
 	unsigned long i;
 	u32 sectorsize = btrfs_inode_sectorsize(inode);