[PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Josef Bacik]

Previously, we would set BTRFS_INODE_NOCOMPRESS if compression resulted
in larger buffers, meaning that the data probably wasn't great to be
compressed.  This would sometimes make the wrong decision, and thus end
up disabling compression in cases where we still wanted it in general.
Thus the -o compress-force option.

However some time later we got chattr -c, which is a user way of
indicating that the file shouldn't be compressed.  This is at odds with
-o compress-force.  We should be honoring what the user wants, which is
to disable compression.  The rub here is our setting of NOCOMPRESS when
we can't compress the file.

But, the way the code works, if we have -o compress-force we'll never
set NOCOMPRESS ourselves, it'll only be set by the user.  I think this
is a reasonable set of behaviors, if NOCOMPRESS is set on the inode then
we assume it was done at the users behest, and honor it no matter what.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0ce42d52d53e..6609b5679d27 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -479,15 +479,15 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
 			btrfs_ino(inode));
 		return 0;
 	}
+	/* bad compression ratios, or we were set with chattr -c. */
+	if (inode->flags & BTRFS_INODE_NOCOMPRESS)
+		return 0;
 	/* force compress */
 	if (btrfs_test_opt(fs_info, FORCE_COMPRESS))
 		return 1;
 	/* defrag ioctl */
 	if (inode->defrag_compress)
 		return 1;
-	/* bad compression ratios */
-	if (inode->flags & BTRFS_INODE_NOCOMPRESS)
-		return 0;
 	if (btrfs_test_opt(fs_info, COMPRESS) ||
 	    inode->flags & BTRFS_INODE_COMPRESS ||
 	    inode->prop_compress)
@@ -1304,8 +1304,7 @@ static int cow_file_range_async(struct btrfs_inode *inode,
 
 	unlock_extent(&inode->io_tree, start, end);
 
-	if (inode->flags & BTRFS_INODE_NOCOMPRESS &&
-	    !btrfs_test_opt(fs_info, FORCE_COMPRESS)) {
+	if (inode->flags & BTRFS_INODE_NOCOMPRESS) {
 		num_chunks = 1;
 		should_compress = false;
 	} else {
-- 
2.26.2

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Roman Mamedov]

On Mon, 30 Nov 2020 08:46:21 -0500
Josef Bacik <josef@toxicpanda.com> wrote:

> However some time later we got chattr -c, which is a user way of
> indicating that the file shouldn't be compressed.  This is at odds with
> -o compress-force.  We should be honoring what the user wants, which is
> to disable compression.

But chattr -c only removes the previously set chattr +c. There's no
"negative-c" to be forced by user in attributes. And +c is already unset on all
files by default. Unless I'm missing something? Thanks

-- 
With respect,
Roman

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Amy Parker]

On Mon, Nov 30, 2020 at 6:12 AM Roman Mamedov <rm@romanrm.net> wrote:
>
> On Mon, 30 Nov 2020 08:46:21 -0500
> Josef Bacik <josef@toxicpanda.com> wrote:
>
> > However some time later we got chattr -c, which is a user way of
> > indicating that the file shouldn't be compressed.  This is at odds with
> > -o compress-force.  We should be honoring what the user wants, which is
> > to disable compression.
>
> But chattr -c only removes the previously set chattr +c. There's no
> "negative-c" to be forced by user in attributes. And +c is already unset on all
> files by default. Unless I'm missing something? Thanks

Yeah, I must be missing something too. It isn't normally set + you can't go
negative...

Best regards,
Amy Parker
(she/her)

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Josef Bacik]

On 11/30/20 9:08 AM, Roman Mamedov wrote:
> On Mon, 30 Nov 2020 08:46:21 -0500
> Josef Bacik <josef@toxicpanda.com> wrote:
> 
>> However some time later we got chattr -c, which is a user way of
>> indicating that the file shouldn't be compressed.  This is at odds with
>> -o compress-force.  We should be honoring what the user wants, which is
>> to disable compression.
> 
> But chattr -c only removes the previously set chattr +c. There's no
> "negative-c" to be forced by user in attributes. And +c is already unset on all
> files by default. Unless I'm missing something? Thanks
> 

The thing you're missing is that when we do chattr -c we're setting NOCOMPRESS 
on the file.  The thing that I'm missing is what exactly we're trying to allow. 
  If chattr -c is supposed to just be the removal of +c, then btrfs is doing the 
wrong thing by setting NOCOMPRESS.  We also do the same thing when we clear a 
btrfs.compression property.

I guess the question is what do we want?  Do we want to only allow the user to 
indicate we want compression, or do we want to allow them to also indicate that 
they don't want compression?  If we don't want to enable them to disable 
compression for a file, then this patch needs to be thrown away, but then we 
also need to fix up all the places we set NOCOMPRESS when we clear these flags. 
  Thanks,

Josef

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Roman Mamedov]

On Mon, 30 Nov 2020 09:50:13 -0500
Josef Bacik <josef@toxicpanda.com> wrote:

> The thing you're missing is that when we do chattr -c we're setting NOCOMPRESS 
> on the file.

Wow, and does this need a previously set +c to work? Or just -c on an already
-c file will change the Btrfs flag under the hood? Seems to be very weird in
any case, as from the user perspective there's no way to view the current
status of that flag, with the only way to change it being via a side-effect of
another operation.

>   If chattr -c is supposed to just be the removal of +c, then btrfs is doing the 
> wrong thing by setting NOCOMPRESS.

I would agree with that.

> I guess the question is what do we want?  Do we want to only allow the user to 
> indicate we want compression, or do we want to allow them to also indicate that 
> they don't want compression?  If we don't want to enable them to disable 
> compression for a file, then this patch needs to be thrown away, but then we 
> also need to fix up all the places we set NOCOMPRESS when we clear these flags. 

The patch also seems to prioritize "no compress if compression ratio is bad"
over compress-force, whereas the whole point of compress-force feels to be to
compress no matter what, especially no matter what are the possibly imperfect
compression ratio estimates.

-- 
With respect,
Roman

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Josef Bacik]

On 11/30/20 10:01 AM, Roman Mamedov wrote:
> On Mon, 30 Nov 2020 09:50:13 -0500
> Josef Bacik <josef@toxicpanda.com> wrote:
> 
>> The thing you're missing is that when we do chattr -c we're setting NOCOMPRESS
>> on the file.
> 
> Wow, and does this need a previously set +c to work? Or just -c on an already
> -c file will change the Btrfs flag under the hood? Seems to be very weird in
> any case, as from the user perspective there's no way to view the current
> status of that flag, with the only way to change it being via a side-effect of
> another operation.
> 
>>    If chattr -c is supposed to just be the removal of +c, then btrfs is doing the
>> wrong thing by setting NOCOMPRESS.
> 
> I would agree with that.
> 
>> I guess the question is what do we want?  Do we want to only allow the user to
>> indicate we want compression, or do we want to allow them to also indicate that
>> they don't want compression?  If we don't want to enable them to disable
>> compression for a file, then this patch needs to be thrown away, but then we
>> also need to fix up all the places we set NOCOMPRESS when we clear these flags.
> 
> The patch also seems to prioritize "no compress if compression ratio is bad"
> over compress-force, whereas the whole point of compress-force feels to be to
> compress no matter what, especially no matter what are the possibly imperfect
> compression ratio estimates.
> 

Right, but if we have compress-force we don't set NOCOMPRESS if the compression 
is bad, so theoretically we shouldn't ever really have that problem?  But I 
agree, this is a weird source of ambiguity.  I'm thinking the best solution is 
to stop setting NOCOMPRESS except in the bad compression case, and then figure 
out a different mechanism to force no compression and deal with that separately. 
  Thanks,

Josef

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Roman Mamedov]

On Mon, 30 Nov 2020 10:10:42 -0500
Josef Bacik <josef@toxicpanda.com> wrote:

> Right, but if we have compress-force we don't set NOCOMPRESS if the compression 
> is bad, so theoretically we shouldn't ever really have that problem?

It is persistent on disk, so remounting or mounting next time with
compress-force would now skip compression for those files which were deemed
having a bad compression ratio at some point in the past.

-- 
With respect,
Roman

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Hugo Mills]

On Mon, Nov 30, 2020 at 09:50:13AM -0500, Josef Bacik wrote:
> On 11/30/20 9:08 AM, Roman Mamedov wrote:
> > On Mon, 30 Nov 2020 08:46:21 -0500
> > Josef Bacik <josef@toxicpanda.com> wrote:
> > 
> > > However some time later we got chattr -c, which is a user way of
> > > indicating that the file shouldn't be compressed.  This is at odds with
> > > -o compress-force.  We should be honoring what the user wants, which is
> > > to disable compression.
> > 
> > But chattr -c only removes the previously set chattr +c. There's no
> > "negative-c" to be forced by user in attributes. And +c is already unset on all
> > files by default. Unless I'm missing something? Thanks
> > 
> 
> The thing you're missing is that when we do chattr -c we're setting
> NOCOMPRESS on the file.  The thing that I'm missing is what exactly we're
> trying to allow.  If chattr -c is supposed to just be the removal of +c,
> then btrfs is doing the wrong thing by setting NOCOMPRESS.  We also do the
> same thing when we clear a btrfs.compression property.

   If I'm understanding this right, there's more than two states
here. There's a "default", and there's two "force" options -- forcing
nocompress, and forcing/allowing compression. If there's no c flag
set, the file could be in one of two states: default behaviour
(presumably defined by the value of the mount options), and
NOCOMPRESS. How do I tell which it is?

> I guess the question is what do we want?  Do we want to only allow the user
> to indicate we want compression, or do we want to allow them to also
> indicate that they don't want compression?  If we don't want to enable them
> to disable compression for a file, then this patch needs to be thrown away,
> but then we also need to fix up all the places we set NOCOMPRESS when we
> clear these flags.  Thanks,

   Hugo.

-- 
Hugo Mills             | The last man on Earth sat in a room. Suddenly, there
hugo@... carfax.org.uk | was a knock at the door.
http://carfax.org.uk/  |
PGP: E2AB1DE4          |                                        Frederic Brown

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[Josef Bacik]

On 11/30/20 10:04 AM, Hugo Mills wrote:
> On Mon, Nov 30, 2020 at 09:50:13AM -0500, Josef Bacik wrote:
>> On 11/30/20 9:08 AM, Roman Mamedov wrote:
>>> On Mon, 30 Nov 2020 08:46:21 -0500
>>> Josef Bacik <josef@toxicpanda.com> wrote:
>>>
>>>> However some time later we got chattr -c, which is a user way of
>>>> indicating that the file shouldn't be compressed.  This is at odds with
>>>> -o compress-force.  We should be honoring what the user wants, which is
>>>> to disable compression.
>>>
>>> But chattr -c only removes the previously set chattr +c. There's no
>>> "negative-c" to be forced by user in attributes. And +c is already unset on all
>>> files by default. Unless I'm missing something? Thanks
>>>
>>
>> The thing you're missing is that when we do chattr -c we're setting
>> NOCOMPRESS on the file.  The thing that I'm missing is what exactly we're
>> trying to allow.  If chattr -c is supposed to just be the removal of +c,
>> then btrfs is doing the wrong thing by setting NOCOMPRESS.  We also do the
>> same thing when we clear a btrfs.compression property.
> 
>     If I'm understanding this right, there's more than two states
> here. There's a "default", and there's two "force" options -- forcing
> nocompress, and forcing/allowing compression. If there's no c flag
> set, the file could be in one of two states: default behaviour
> (presumably defined by the value of the mount options), and
> NOCOMPRESS. How do I tell which it is?

Yeah you can't really.  If you're mounted with -o compress-force then you won't 
get NOCOMPRESS set automatically, so you can sort of (emphasis on sort of) 
assume that if you have NOCOMPRESS set then it was done by some user specific 
action.  But it seems like we don't even really clearly define that this is an 
option, so we're probably better off just killing that usage everywhere.  Thanks,

Josef

Re: [PATCH] btrfs: do not allow -o compress-force to override per-inode settings

[sys]

---- From: Hugo Mills <hugo@carfax.org.uk> -- Sent: 2020-11-30 - 16:04 ----

> On Mon, Nov 30, 2020 at 09:50:13AM -0500, Josef Bacik wrote:
>> On 11/30/20 9:08 AM, Roman Mamedov wrote:
>> > On Mon, 30 Nov 2020 08:46:21 -0500
>> > Josef Bacik <josef@toxicpanda.com> wrote:
>> > 
>> > > However some time later we got chattr -c, which is a user way of
>> > > indicating that the file shouldn't be compressed.  This is at odds with
>> > > -o compress-force.  We should be honoring what the user wants, which is
>> > > to disable compression.
>> > 
>> > But chattr -c only removes the previously set chattr +c. There's no
>> > "negative-c" to be forced by user in attributes. And +c is already unset on all
>> > files by default. Unless I'm missing something? Thanks
>> > 
>> 
>> The thing you're missing is that when we do chattr -c we're setting
>> NOCOMPRESS on the file.  The thing that I'm missing is what exactly we're
>> trying to allow.  If chattr -c is supposed to just be the removal of +c,
>> then btrfs is doing the wrong thing by setting NOCOMPRESS.  We also do the
>> same thing when we clear a btrfs.compression property.
> 
>    If I'm understanding this right, there's more than two states
> here. There's a "default", and there's two "force" options -- forcing
> nocompress, and forcing/allowing compression. If there's no c flag
> set, the file could be in one of two states: default behaviour
> (presumably defined by the value of the mount options), and
> NOCOMPRESS. How do I tell which it is?

 Fsattr (chattr/lsattr) seem to have been a simplified way of exposing the compression xattr to users, wasn't it? Could it not be better to promote the use of btrfs.compression xattrs instead? 

> 
>> I guess the question is what do we want?  Do we want to only allow the user
>> to indicate we want compression, or do we want to allow them to also
>> indicate that they don't want compression? 

I think it is a good thing to have the three options, but we may need to drop the use of chattr/lsattr and go with only xattrs. 

Thanks, 
Forza 

 If we don't want to enable them
>> to disable compression for a file, then this patch needs to be thrown away,
>> but then we also need to fix up all the places we set NOCOMPRESS when we
>> clear these flags.  Thanks,
> 
>    Hugo.
> 
> -- 
> Hugo Mills             | The last man on Earth sat in a room. Suddenly, there
> hugo@... carfax.org.uk | was a knock at the door.
> http://carfax.org.uk/  |
> PGP: E2AB1DE4          |                                        Frederic Brown