Re: [PATCH] btrfs: inode: Don't compress if NODATASUM or NODATACOW set

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

On 2018年05月14日 16:10, Nikolay Borisov wrote:
> 
> 
> On 14.05.2018 10:02, Qu Wenruo wrote:
>> As btrfs(5) specified:
>>
>> 	Note
>> 	If nodatacow or nodatasum are enabled, compression is disabled.
>>
>> If NODATASUM or NODATACOW set, we should not compress the extent.
>>
>> And in fact, we have bug report about corrupted compressed extent
>> leading to memory corruption in mail list.
>> Although it's mostly buggy lzo implementation causing the problem, btrfs
> 
> What does it mean "it's mostly buggy lzo implementation"? If we have bug
> in the LZO implementation that btrfs uses shouldn't it be fixed as well ?

It looks like buggy lzo decompress code which can't handle corrupted
data and results some random kernel memory corruption.
It's also possible that some incorrect function call in btrfs that
caused the problem.

It needs to be fixed of course, but right now, no binary dump of the
offending data, so we can't verify or further investigate the root cause.

Thanks,
Qu

> 
>> still needs to be fixed to meet the specification.
>>
>> Reported-by: James Harvey <jamespharvey20@gmail.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/inode.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index d241285a0d2a..dbef3f404559 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -396,6 +396,14 @@ static inline int inode_need_compress(struct inode *inode, u64 start, u64 end)
>>  {
>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>>  
>> +	/*
>> +	 * Btrfs doesn't support compression without csum or CoW.
>> +	 * This should have the highest priority.
>> +	 */
>> +	if (BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW ||
>> +	    BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)
>> +		return 0;
>> +
>>  	/* force compress */
>>  	if (btrfs_test_opt(fs_info, FORCE_COMPRESS))
>>  		return 1;
> 
> But if we have mounted the fs with FORCE_COMPRESS shouldn't we disregard
> the inode flags,

I'm afraid not.
AFAIK the purpose of force compress is to bypass the compression ratio
detection, other than generating compressed extent for NODATASUM use case.

Thanks,
Qu

> presumably the admin knows what he is doing? Won't it
> be better if somewhere further in the call chain we check if
> FORCE_COMPRESS is set and if so override any inode flags. This can be
> implemented by _always_ calling inode_need_compress to decide if we
> should compress or not?
> 
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>