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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4176 bytes --]



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);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]