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

From: james harvey <jamespharvey20@gmail.com>
To: Nikolay Borisov <nborisov@suse.com>
Cc: Roman Mamedov <rm@romanrm.net>, Qu Wenruo <wqu@suse.com>,
	Btrfs BTRFS <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH] btrfs: inode: Don't compress if NODATASUM or NODATACOW set
Date: Mon, 14 May 2018 05:31:32 -0400	[thread overview]
Message-ID: <CA+X5Wn6r+jGAEhZFeunRf4cqhuL6qjtkwPUnxTf_DWOdru-JRA@mail.gmail.com> (raw)
In-Reply-To: <ea90a7da-a055-6879-e64f-f0f3bca81a23@suse.com>

On Mon, May 14, 2018 at 4:36 AM, Nikolay Borisov <nborisov@suse.com> wrote:
> On 14.05.2018 11:20, Roman Mamedov wrote:
>> On Mon, 14 May 2018 11:10:34 +0300
>> Nikolay Borisov <nborisov@suse.com> wrote:
>>
>>> But if we have mounted the fs with FORCE_COMPRESS shouldn't we disregard
>>> the inode flags, presumably the admin knows what he is doing?
>>
>> Please don't. Personally I always assumed chattr +C would prevent both CoW and
>> compression, and used that as a way to override volume-wide compress-force for
>> a particular folder. Now that it turns out this wasn't working, the patch
>> would fix it to behave in line with prior expectations.
>
>
> So what made you have these expectation, is it codified somewhere
> (docs/man pages etc)? I'm fine with that semantics IF this is what
> people expect.

https://btrfs.wiki.kernel.org/index.php/Compression

"Compression... does not work for NOCOW files."

IMO, without a checksum, allowing compression is too much of a data
risk.  Uncompressed, user may be able to salvage most of the data with
partial corruption.  Compressed, they can't get anything to even look
at, unless they have a mirrored copy that is fine, figure out which
disk is corrupted, and mount degraded without it.

> Now the question is why people grew up to have this expectation and not
> the other way round? IMO force_compress should really disregard
> everything else, if not then it should be documented. The man pages [0]
> say:
>
> "
> If compress-force is specified, the compression will allways be
> attempted, but the data may end up uncompressed if the compression would
> make them larger.
>
> As this is too simple, the compress-force is a workaround that will
> compress most of the files at the cost of some wasted CPU cycles on
> failed attempts. The heuristics of compress will improve in the future
> so this will not be necessary.
> "
>
> In this case I think the man page for this option should be documented
> to say that inode-specific flags preclude mount flags.
>
> https://btrfs.wiki.kernel.org/index.php/Manpage/btrfs(5)#MOUNT_OPTIONS

Agreed the man page should be clarified.