Re: [PATCH] btrfs: Merge inode_can_compress in inode_need_compress

[Nikolay Borisov <nborisov@suse.com>]

On 31.08.20 г. 23:40 ч., David Sterba wrote:
> On Tue, Aug 18, 2020 at 09:30:56AM +0300, Nikolay Borisov wrote:
>> The latter is the only caller of the former. Just open code can_compress
>> into need_compress and also remove the warning since it's made redundant
>> by this change.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/inode.c | 21 +++------------------
>>  1 file changed, 3 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 15dc8b6871ac..19e1918bad5c 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -410,17 +410,6 @@ static noinline int add_async_extent(struct async_chunk *cow,
>>  	return 0;
>>  }
>>  
>> -/*
>> - * Check if the inode has flags compatible with compression
>> - */
>> -static inline bool inode_can_compress(struct btrfs_inode *inode)
>> -{
>> -	if (inode->flags & BTRFS_INODE_NODATACOW ||
>> -	    inode->flags & BTRFS_INODE_NODATASUM)
>> -		return false;
>> -	return true;
>> -}
>> -
>>  /*
>>   * Check if the inode needs to be submitted to compression, based on mount
>>   * options, defragmentation, properties or heuristics.
>> @@ -430,12 +419,9 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
>>  {
>>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>>  
>> -	if (!inode_can_compress(inode)) {
>> -		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
>> -			KERN_ERR "BTRFS: unexpected compression for ino %llu\n",
>> -			btrfs_ino(inode));
>> +	if (inode->flags & BTRFS_INODE_NODATACOW ||
>> +	    inode->flags & BTRFS_INODE_NODATASUM)
>>  		return 0;
> 
> This serves as a runtime friendly assert, when inode_need_compress is
> called from compress_file_range.
> 
> First call in btrfs_run_delalloc_range duplicates the
> nodatacow/nodatasum which would be cleaned up by this patch but we'd
> lose the warning in case compress_file_range detects the unexpected
> flags.
> 


I think we should simply come to an agreement at which stage of a write
shall we check for the presence of flags. IMO the less places we have
the  better. This will obviate the need for a run-time assert... Frankly
I think we should be checking only at the time of btrfs_run_delalloc_range