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

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

[Qu Wenruo]

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
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;
-- 
2.17.0

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

[Nikolay Borisov]

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 ?

> 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, 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?

> 

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

[Roman Mamedov]

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.

-- 
With respect,
Roman

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

[Qu Wenruo]

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
> 

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

[Nikolay Borisov]

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.


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

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

[james harvey]

On Mon, May 14, 2018 at 4:20 AM, Roman Mamedov <rm@romanrm.net> 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.

"the patch would fix it to behave" might be missing one part.  Qu, am
I right (best guess, not familiar with btrfs internals enough) this
patch will prevent new files and files of size 0 from getting
compression with NODATASUM or NODATACOW, but would leave existing
compressed files (that shouldn't have been compressed) as they are?
After getting an error from btrfs check, presumably, user would need
to copy the file(s) to a temporary and copy it back over itself, to
get rid of the unwanted compression?

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

[james harvey]

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.

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

[Roman Mamedov]

On Mon, 14 May 2018 11:36:26 +0300
Nikolay Borisov <nborisov@suse.com> wrote:

> 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.

"Compression ...does not work for NOCOW files":
https://btrfs.wiki.kernel.org/index.php/Compression

The mount options man page does not say that the NOCOW attribute of files will
be disregarded with compress-force.  It only mentions interaction with the
nodatacow and nodatasum mount options. So I'd expect the attribute to still
work and prevent compression of NOCOW files.

> 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

Both are knobs that the user needs to explicitly set, the difference is that
the +C attribute is fine-grained and the mount option is global. If they are
set by the user to conflicting values, it seems more useful to have the
fine-grained control override the global one, not the other way round.

-- 
With respect,
Roman

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

[Nikolay Borisov]

[Adding David to CC]

On 14.05.2018 12:39, Roman Mamedov wrote:
> On Mon, 14 May 2018 11:36:26 +0300
> Nikolay Borisov <nborisov@suse.com> wrote:
> 
>> 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.
> 
> "Compression ...does not work for NOCOW files":
> https://btrfs.wiki.kernel.org/index.php/Compression
> 
> The mount options man page does not say that the NOCOW attribute of files will
> be disregarded with compress-force.  It only mentions interaction with the
> nodatacow and nodatasum mount options. So I'd expect the attribute to still
> work and prevent compression of NOCOW files.

I wouldn't say this is very clear, it needs to be stated explicitly.

> 
>> 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
> 
> Both are knobs that the user needs to explicitly set, the difference is that
> the +C attribute is fine-grained and the mount option is global. If they are
> set by the user to conflicting values, it seems more useful to have the
> fine-grained control override the global one, not the other way round.

This is valid reasoning but so is mine. So I'd like to have some rules
on that matter such that in the future things will have consistent
semantics. Obviously in this case the "local options trump global ones"
seems to be prevalent. I don't have problem with that but this should
codified somewhere.


David, what's your take on that. Where do you think will be the best
place to say that local, per-inode options take precedence over global ones?

> 

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

[Qu Wenruo]

On 2018年05月14日 17:30, james harvey wrote:
> On Mon, May 14, 2018 at 4:20 AM, Roman Mamedov <rm@romanrm.net> 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.
> 
> "the patch would fix it to behave" might be missing one part.  Qu, am
> I right (best guess, not familiar with btrfs internals enough) this
> patch will prevent new files and files of size 0 from getting
> compression with NODATASUM or NODATACOW,

Yep.

> but would leave existing
> compressed files (that shouldn't have been compressed) as they are?

Yep again, unfortunately.

> After getting an error from btrfs check,

BTW for this particular case, --mode=lowmem would provide more detailed
info.

> presumably, user would need
> to copy the file(s) to a temporary and copy it back over itself, to
> get rid of the unwanted compression?

Unfortunately, reading (copying) data from the original fs is no longer
that safe (just like your case).

But if reading works fine, then copying things back will trigger normal
write routine, thus it would be plain text extent.

And if possible, please don't just remove those offending files (yet).
Your binary dump would help a lot locating the root case.

Thanks,
Qu

> --
> 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
> 

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

[David Sterba]

On Mon, May 14, 2018 at 12:46:31PM +0300, Nikolay Borisov wrote:
> [Adding David to CC]
> 
> On 14.05.2018 12:39, Roman Mamedov wrote:
> > On Mon, 14 May 2018 11:36:26 +0300
> > Nikolay Borisov <nborisov@suse.com> wrote:
> > 
> >> 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.
> > 
> > "Compression ...does not work for NOCOW files":
> > https://btrfs.wiki.kernel.org/index.php/Compression
> > 
> > The mount options man page does not say that the NOCOW attribute of files will
> > be disregarded with compress-force.  It only mentions interaction with the
> > nodatacow and nodatasum mount options. So I'd expect the attribute to still
> > work and prevent compression of NOCOW files.
> 
> I wouldn't say this is very clear, it needs to be stated explicitly.
> 
> > 
> >> 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
> > 
> > Both are knobs that the user needs to explicitly set, the difference is that
> > the +C attribute is fine-grained and the mount option is global. If they are
> > set by the user to conflicting values, it seems more useful to have the
> > fine-grained control override the global one, not the other way round.
> 
> This is valid reasoning but so is mine. So I'd like to have some rules
> on that matter such that in the future things will have consistent
> semantics. Obviously in this case the "local options trump global ones"
> seems to be prevalent. I don't have problem with that but this should
> codified somewhere.
> 
> 
> David, what's your take on that. Where do you think will be the best
> place to say that local, per-inode options take precedence over global ones?

The order of precedence is roughly like this, whenever the respective
containing object exits:

- inode
- directory
- subvolume
- mount options
- filesystem defaults

There's some special cases for the compression, that are documented
separately.

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

[David Sterba]

On Mon, May 14, 2018 at 03:02:10PM +0800, 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.

Link please.

> Although it's mostly buggy lzo implementation causing the problem, btrfs
> still needs to be fixed to meet the specification.

That's very vague, what's the LZO bug? If the input is garbage and lzo
decompression cannot decompress it, it's not a lzo bug.

> 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;

This is also the wrong place to fix that, NODATASUM or NODATACOW inode
should never make it to compress_file_range (that calls
inode_need_compress).

> +
>  	/* force compress */
>  	if (btrfs_test_opt(fs_info, FORCE_COMPRESS))
>  		return 1;
> -- 
> 2.17.0
> 
> --
> 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

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

[Timofey Titovets]

пн, 14 мая 2018 г. в 20:32, David Sterba <dsterba@suse.cz>:

> On Mon, May 14, 2018 at 03:02:10PM +0800, 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.

> Link please.

> > Although it's mostly buggy lzo implementation causing the problem, btrfs
> > still needs to be fixed to meet the specification.

> That's very vague, what's the LZO bug? If the input is garbage and lzo
> decompression cannot decompress it, it's not a lzo bug.

> > 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;

> This is also the wrong place to fix that, NODATASUM or NODATACOW inode
> should never make it to compress_file_range (that calls
> inode_need_compress).


David, i've talk about that some time ago:
https://www.spinics.net/lists/linux-btrfs/msg73137.html

NoCow files can be *easy* compressed.
```
➜  ~ touch test
➜  ~ chattr +C test
➜  ~ lsattr test
---------------C-- test
➜  ~ dd if=/dev/zero of=./test bs=1M count=1
1+0 records in
1+0 records out
1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.00099878 s, 1.0 GB/s
➜  ~ sync
➜  ~ filefrag -v test
Filesystem type is: 9123683e
File size of test is 1048576 (256 blocks of 4096 bytes)
ext:     logical_offset:        physical_offset: length:   expected: flags:
   0:        0..     255:   88592741..  88592996:    256:
last,eof
test: 1 extent found
➜  ~ btrfs fi def -vrczstd test
test
➜  ~ filefrag -v test
Filesystem type is: 9123683e
File size of test is 1048576 (256 blocks of 4096 bytes)
ext:     logical_offset:        physical_offset: length:   expected: flags:
   0:        0..      31:       3125..      3156:     32:             encoded
   1:       32..      63:       3180..      3211:     32:       3157: encoded
   2:       64..      95:       3185..      3216:     32:       3212: encoded
   3:       96..     127:       3188..      3219:     32:       3217: encoded
   4:      128..     159:       3263..      3294:     32:       3220: encoded
   5:      160..     191:       3355..      3386:     32:       3295: encoded
   6:      192..     223:       3376..      3407:     32:       3387: encoded
   7:      224..     255:       3411..      3442:     32:       3408:
last,encoded,eof
test: 8 extents found
```
-- 
Have a nice day,
Timofey.

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

[james harvey]

On Mon, May 14, 2018 at 6:35 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> And if possible, please don't just remove those offending files (yet).
> Your binary dump would help a lot locating the root case.

Absolutely.  This is on a 50G LVM root volume, so I've been able to
leave the original one unmodified and always mounted ro.  Copied it
and changed the UUID.  So, I can leave it around for a while without a
problem.

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

[james harvey]

On Mon, May 14, 2018 at 12:52 PM, David Sterba <dsterba@suse.cz> wrote:
> On Mon, May 14, 2018 at 03:02:10PM +0800, 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.
>
> Link please.

https://bugzilla.kernel.org/show_bug.cgi?id=199707
&
https://www.spinics.net/lists/linux-btrfs/msg77971.html

>> Although it's mostly buggy lzo implementation causing the problem, btrfs
>> still needs to be fixed to meet the specification.
>
> That's very vague, what's the LZO bug? If the input is garbage and lzo
> decompression cannot decompress it, it's not a lzo bug.

The bug is the kernel doesn't just give an I/O error.  It totally
crashes the system.  Even a regular user cat'ing an invalid
btrfs-lzo-compressed file will take the system down.  Qu's current
theory which I agree with is it causes some kind of random kernel
memory corruption.  The crashes sometimes include "BTRFS: decompress
failed" at the start, but give backtraces for other things than btrfs
lzo code.

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

[james harvey]

Don't know if this will help.  I just learned about pstore, and see in
there a dmesg that's interesting.

The serial port kernel errors started this time with "BUG: unable to
handle kernel paging request".  The pstore dmesg has everything from
there until the end of the first trace.

But, the interesting part is the pstore dmesg has 310 "BTRFS:
decompress failed" messages the serial port (the versions I've shared)
version doesn't. (Sometimes the serial crashes have 1 of these btrfs
errors, but never repeated like this.)

These 310 btrfs errors are all uptime-stamped from 13110.016096 -
13110.253752, when the BUG is later at 13110.370494 (when the serial
errors start.)

With the kernel trying to decompress 310 times within 0.237656
seconds, maybe that's an indication with invalid data, it retries
forever in a bad way crashing the kernel, rather than failing?

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2651 bytes --]



On 2018年05月15日 00:52, David Sterba wrote:
> On Mon, May 14, 2018 at 03:02:10PM +0800, 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.
> 
> Link please.
> 
>> Although it's mostly buggy lzo implementation causing the problem, btrfs
>> still needs to be fixed to meet the specification.
> 
> That's very vague, what's the LZO bug? If the input is garbage and lzo
> decompression cannot decompress it, it's not a lzo bug.

Still digging.
Would update this content in next version.

> 
>> 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;
> 
> This is also the wrong place to fix that, NODATASUM or NODATACOW inode
> should never make it to compress_file_range (that calls
> inode_need_compress).

Nope, in run_delalloc_range(), we calls inode_need_compress() to
determine if we goes to normal cow routine or goes to async routine
(compression).

So inode_need_compress() looks like a pretty valid location.

Just mount with nodatasum and create an inode, then remount to
datasum,compress, write something to that inode, you could hit the behavior.

Thanks,
Qu

> 
>> +
>>  	/* force compress */
>>  	if (btrfs_test_opt(fs_info, FORCE_COMPRESS))
>>  		return 1;
>> -- 
>> 2.17.0
>>
>> --
>> 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
> --
> 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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[David Sterba]

On Mon, May 14, 2018 at 11:46:09PM +0300, Timofey Titovets wrote:
> > > @@ -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;
> 
> > This is also the wrong place to fix that, NODATASUM or NODATACOW inode
> > should never make it to compress_file_range (that calls
> > inode_need_compress).
> 
> 
> David, i've talk about that some time ago:
> https://www.spinics.net/lists/linux-btrfs/msg73137.html
> 
> NoCow files can be *easy* compressed.

I missed your previous mail, the issue with compression and nocow/nosum
is there. Seems like some of the combinations are not properly handled
when it's mount option vs defrag.