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

From: Josef Bacik <josef@toxicpanda.com>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>,
	linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 2/5] btrfs: push the NODATASUM check into btrfs_lookup_bio_sums
Date: Mon, 28 Sep 2020 14:28:32 -0400	[thread overview]
Message-ID: <d208801b-01cd-9b81-a666-b4fa910c6a8e@toxicpanda.com> (raw)
In-Reply-To: <a3d17402-7e3d-7fb4-9831-2db5be18d5b2@gmx.com>

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