diff mbox series

[2/5] btrfs: push the NODATASUM check into btrfs_lookup_bio_sums

Message ID bdf1bf5c65679fdf39021e16a242094acd71b270.1600961206.git.josef@toxicpanda.com
State New, archived
Headers show
Series New rescue mount options | expand

Commit Message

Josef Bacik Sept. 24, 2020, 3:32 p.m. UTC
When we move to being able to handle NULL csum_roots it'll be cleaner to
just check in btrfs_lookup_bio_sums instead of at all of the caller
locations, so push the NODATASUM check into it as well so it's unified.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/compression.c | 14 +++++---------
 fs/btrfs/file-item.c   |  3 +++
 fs/btrfs/inode.c       | 12 +++++++++---
 3 files changed, 17 insertions(+), 12 deletions(-)

Comments

Qu Wenruo Sept. 25, 2020, 12:39 a.m. UTC | #1
On 2020/9/24 下午11:32, Josef Bacik wrote:
> When we move to being able to handle NULL csum_roots it'll be cleaner to
> just check in btrfs_lookup_bio_sums instead of at all of the caller
> locations, so push the NODATASUM check into it as well so it's unified.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

But an off-topic question inlined below:

> ---
>  fs/btrfs/compression.c | 14 +++++---------
>  fs/btrfs/file-item.c   |  3 +++
>  fs/btrfs/inode.c       | 12 +++++++++---
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index eeface30facd..7e1eb57b923c 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -722,11 +722,9 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  			 */
>  			refcount_inc(&cb->pending_bios);
>  
> -			if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
> -				ret = btrfs_lookup_bio_sums(inode, comp_bio,
> -							    (u64)-1, sums);
> -				BUG_ON(ret); /* -ENOMEM */

Is it really possible to have compressed extent without data csum?
Won't nodatacsum disable compression?

Or are we just here to handle some old compressed but not csumed data?

Thanks,
Qu

> -			}
> +			ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1,
> +						    sums);
> +			BUG_ON(ret); /* -ENOMEM */
>  
>  			nr_sectors = DIV_ROUND_UP(comp_bio->bi_iter.bi_size,
>  						  fs_info->sectorsize);
> @@ -751,10 +749,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);
>  	BUG_ON(ret); /* -ENOMEM */
>  
> -	if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
> -		ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, sums);
> -		BUG_ON(ret); /* -ENOMEM */
> -	}
> +	ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, sums);
> +	BUG_ON(ret); /* -ENOMEM */
>  
>  	ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);
>  	if (ret) {
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 8f4f2bd6d9b9..8083d71d6af6 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -272,6 +272,9 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio,
>  	int count = 0;
>  	u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
>  
> +	if (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)
> +		return BLK_STS_OK;
> +
>  	path = btrfs_alloc_path();
>  	if (!path)
>  		return BLK_STS_RESOURCE;
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d526833b5ec0..d262944c4297 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2202,7 +2202,12 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>  							   mirror_num,
>  							   bio_flags);
>  			goto out;
> -		} else if (!skip_sum) {
> +		} else {
> +			/*
> +			 * Lookup bio sums does extra checks around whether we
> +			 * need to csum or not, which is why we ignore skip_sum
> +			 * here.
> +			 */
>  			ret = btrfs_lookup_bio_sums(inode, bio, (u64)-1, NULL);
>  			if (ret)
>  				goto out;
> @@ -7781,7 +7786,6 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
>  		struct bio *dio_bio, loff_t file_offset)
>  {
>  	const bool write = (bio_op(dio_bio) == REQ_OP_WRITE);
> -	const bool csum = !(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM);
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	const bool raid56 = (btrfs_data_alloc_profile(fs_info) &
>  			     BTRFS_BLOCK_GROUP_RAID56_MASK);
> @@ -7808,10 +7812,12 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
>  		return BLK_QC_T_NONE;
>  	}
>  
> -	if (!write && csum) {
> +	if (!write) {
>  		/*
>  		 * Load the csums up front to reduce csum tree searches and
>  		 * contention when submitting bios.
> +		 *
> +		 * If we have csums disabled this will do nothing.
>  		 */
>  		status = btrfs_lookup_bio_sums(inode, dio_bio, file_offset,
>  					       dip->csums);
>
Johannes Thumshirn Sept. 28, 2020, 12:39 p.m. UTC | #2
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Josef Bacik Sept. 28, 2020, 6:28 p.m. UTC | #3
On 9/24/20 8:39 PM, Qu Wenruo wrote:
> 
> 
> On 2020/9/24 下午11:32, Josef Bacik wrote:
>> When we move to being able to handle NULL csum_roots it'll be cleaner to
>> just check in btrfs_lookup_bio_sums instead of at all of the caller
>> locations, so push the NODATASUM check into it as well so it's unified.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> But an off-topic question inlined below:
> 
>> ---
>>   fs/btrfs/compression.c | 14 +++++---------
>>   fs/btrfs/file-item.c   |  3 +++
>>   fs/btrfs/inode.c       | 12 +++++++++---
>>   3 files changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
>> index eeface30facd..7e1eb57b923c 100644
>> --- a/fs/btrfs/compression.c
>> +++ b/fs/btrfs/compression.c
>> @@ -722,11 +722,9 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>>   			 */
>>   			refcount_inc(&cb->pending_bios);
>>   
>> -			if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
>> -				ret = btrfs_lookup_bio_sums(inode, comp_bio,
>> -							    (u64)-1, sums);
>> -				BUG_ON(ret); /* -ENOMEM */
> 
> Is it really possible to have compressed extent without data csum?
> Won't nodatacsum disable compression?
> 
> Or are we just here to handle some old compressed but not csumed data?
> 

We used to allow it, so I'm content to leave this here.  Maybe at some point 
we'll allow it in the future, but IDK it doesn't hurt anything to handle it 
here.  Thanks,

Josef
diff mbox series

Patch

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index eeface30facd..7e1eb57b923c 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -722,11 +722,9 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 			 */
 			refcount_inc(&cb->pending_bios);
 
-			if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
-				ret = btrfs_lookup_bio_sums(inode, comp_bio,
-							    (u64)-1, sums);
-				BUG_ON(ret); /* -ENOMEM */
-			}
+			ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1,
+						    sums);
+			BUG_ON(ret); /* -ENOMEM */
 
 			nr_sectors = DIV_ROUND_UP(comp_bio->bi_iter.bi_size,
 						  fs_info->sectorsize);
@@ -751,10 +749,8 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);
 	BUG_ON(ret); /* -ENOMEM */
 
-	if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
-		ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, sums);
-		BUG_ON(ret); /* -ENOMEM */
-	}
+	ret = btrfs_lookup_bio_sums(inode, comp_bio, (u64)-1, sums);
+	BUG_ON(ret); /* -ENOMEM */
 
 	ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);
 	if (ret) {
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 8f4f2bd6d9b9..8083d71d6af6 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -272,6 +272,9 @@  blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio,
 	int count = 0;
 	u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
 
+	if (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)
+		return BLK_STS_OK;
+
 	path = btrfs_alloc_path();
 	if (!path)
 		return BLK_STS_RESOURCE;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d526833b5ec0..d262944c4297 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2202,7 +2202,12 @@  blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
 							   mirror_num,
 							   bio_flags);
 			goto out;
-		} else if (!skip_sum) {
+		} else {
+			/*
+			 * Lookup bio sums does extra checks around whether we
+			 * need to csum or not, which is why we ignore skip_sum
+			 * here.
+			 */
 			ret = btrfs_lookup_bio_sums(inode, bio, (u64)-1, NULL);
 			if (ret)
 				goto out;
@@ -7781,7 +7786,6 @@  static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
 		struct bio *dio_bio, loff_t file_offset)
 {
 	const bool write = (bio_op(dio_bio) == REQ_OP_WRITE);
-	const bool csum = !(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	const bool raid56 = (btrfs_data_alloc_profile(fs_info) &
 			     BTRFS_BLOCK_GROUP_RAID56_MASK);
@@ -7808,10 +7812,12 @@  static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
 		return BLK_QC_T_NONE;
 	}
 
-	if (!write && csum) {
+	if (!write) {
 		/*
 		 * Load the csums up front to reduce csum tree searches and
 		 * contention when submitting bios.
+		 *
+		 * If we have csums disabled this will do nothing.
 		 */
 		status = btrfs_lookup_bio_sums(inode, dio_bio, file_offset,
 					       dip->csums);