btrfs: inode: Don't compress if NODATASUM or NODATACOW set
PATCH 1/1 btrfs: inode: Don't compress if NODATASUM or NODATACOW set
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.
--- fs/btrfs/inode.c | 8 ++++++++ 1 file changed, 8 insertions(+)
fs/btrfs/inode.c +8 -0
@@ -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 ;
Signature · 1 line
2.17.0
Quoted 10 lines · 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
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 ?
Quoted 17 lines
still needs to be fixed to meet the specification.
Reported-by James Harvey <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/inode.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/btrfs/inode.c +1
-0@@ -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); +/*
fs/btrfs/inode.c static inline int inode_need_compress(struct inode *inode, u64 start, u64 end) · line 409
Quoted 10 lines
fs/btrfs/inode.c +7
-0+* 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?
Quoted 1 line
Quoted 8 lines · Nikolay Borisov
Quoted 5 lines · Qu Wenruo
As btrfs(5) specified:
Note
If nodatacow or nodatasum are enabled, compression is disabled.
Quoted 8 lines
Quoted 5 lines
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, btrfsWhat 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
Quoted 17 lines
Quoted 17 lines
still needs to be fixed to meet the specification.
Reported-by James Harvey <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/inode.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/btrfs/inode.c +0
-0@@ -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);
Quoted 14 lines
fs/btrfs/inode.c static inline int inode_need_compress(struct inode *inode, u64 start, u64 end) · line 409
Quoted 11 lines
fs/btrfs/inode.c +8
-0+/* +* 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_COMPRESSshouldn'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
Quoted 12 lines
presumably the admin knows what he is doing? Won't it
be better if somewhere further in the call chain we check ifFORCE_COMPRESSis set and if so override any inode flags. This can be implemented by _always_ callinginode_need_compressto decide if we should compress or not?Quoted 1 line
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines · On Mon, 14 May 2018 11:10:34 +0300 Nikolay Borisov <[email protected]> wrote:
But if we have mounted the fs with
FORCE_COMPRESSshouldn'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.
Signature · 2 lines
With respect, Roman
Quoted 10 lines · Roman Mamedov
Quoted 2 lines · On Mon, 14 May 2018 11:10:34 +0300 Nikolay Borisov <[email protected]> wrote:
But if we have mounted the fs with
FORCE_COMPRESSshouldn'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
Quoted 13 lines · Nikolay Borisov
Quoted 10 lines · Roman Mamedov
Quoted 2 lines · On Mon, 14 May 2018 11:10:34 +0300 Nikolay Borisov <[email protected]> wrote:
But if we have mounted the fs with
FORCE_COMPRESSshouldn'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.
Quoted 3 lines
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.
Quoted 16 lines
Now the question is why people grew up to have this expectation and not the other way round? IMO
force_compressshould 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.
"
Quoted 4 lines
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.
Quoted 3 lines · On Mon, 14 May 2018 11:36:26 +0300 Nikolay Borisov <[email protected]> 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.
Quoted 2 lines
Now the question is why people grew up to have this expectation and not the other way round? IMO
force_compressshould 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.
Signature · 2 lines
With respect, Roman
[Adding David to CC]
Quoted 7 lines · Roman Mamedov
Quoted 3 lines · On Mon, 14 May 2018 11:36:26 +0300 Nikolay Borisov <[email protected]> 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.
Quoted 7 lines
"Compression ...does not work for NOCOW files":
https://btrfs.wiki.kernel.org/index.php/CompressionThe 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.
Quoted 8 lines
Quoted 3 lines
Now the question is why people grew up to have this expectation and not the other way round? IMO
force_compressshould really disregard everything elseBoth 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?
Quoted 1 line
Quoted 36 lines · Nikolay Borisov
[Adding David to CC]
Quoted 7 lines · Roman Mamedov
Quoted 3 lines · On Mon, 14 May 2018 11:36:26 +0300 Nikolay Borisov <[email protected]> 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.
Quoted 7 lines
"Compression ...does not work for NOCOW files":
https://btrfs.wiki.kernel.org/index.php/CompressionThe 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.
Quoted 8 lines
Quoted 3 lines
Now the question is why people grew up to have this expectation and not the other way round? IMO
force_compressshould really disregard everything elseBoth 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.
Quoted 2 lines
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.
Quoted 10 lines · Roman Mamedov
Quoted 2 lines · On Mon, 14 May 2018 11:10:34 +0300 Nikolay Borisov <[email protected]> wrote:
But if we have mounted the fs with
FORCE_COMPRESSshouldn'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?
Quoted 12 lines · james harvey
Quoted 10 lines · Roman Mamedov
Quoted 2 lines · On Mon, 14 May 2018 11:10:34 +0300 Nikolay Borisov <[email protected]> wrote:
But if we have mounted the fs with
FORCE_COMPRESSshouldn'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.
Quoted 4 lines
"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.
Quoted 2 lines
but would leave existing
compressed files (that shouldn't have been compressed) as they are?
Yep again, unfortunately.
Quoted 1 line
After getting an error from btrfs check,
BTW for this particular case, --mode=lowmem would provide more detailed info.
Quoted 3 lines
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
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines · Qu Wenruo
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.
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?
Quoted 9 lines · 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.
Link please.
Quoted 2 lines
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.
Quoted 11 lines
Reported-by James Harvey <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/inode.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/btrfs/inode.c +0
-0
fs/btrfs/inode.c static inline int inode_need_compress(struct inode *inode, u64 start, u64 end) · line 405
Quoted 10 lines
fs/btrfs/inode.c +7
-0@@ -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).
Quoted 11 lines
fs/btrfs/inode.c +1
-0+/* force compress */ if (btrfs_test_opt (fs_info,FORCE_COMPRESS ))return 1 ;Signature · 6 lines
2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
пн, 14 мая 2018 г. в 20:32, David Sterba <[email protected]>:
Quoted 25 lines · Qu Wenruo
Quoted 9 lines
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.
Quoted 2 lines
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.
Quoted 6 lines
Reported-by James Harvey <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/inode.c | 8 ++++++++ 1 file changed, 8 insertions(+)
Quoted 5 lines
fs/btrfs/inode.c static inline int inode_need_compress(struct inode
Quoted 5 lines
fs/btrfs/inode.c +0
-0@@ -396,6 +396,14 @@ static inline int inode_need_compress(struct inode
*inode, u64 start, u64 end)
Quoted 11 lines
fs/btrfs/inode.c static inline int inode_need_compress(struct inode *inode, u64 start, u64 end) · line 405
Quoted 10 lines
fs/btrfs/inode.c +7
-0{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 ;
Quoted 3 lines
This is also the wrong place to fix that, NODATASUM or NODATACOW inode should never make it to
compress_file_range(that callsinode_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 ```
Signature · 2 lines
Have a nice day, Timofey.
Quoted 18 lines · Timofey Titovets
Quoted 1 line
Review static inline int inode_need_compress(struct inode
Quoted 1 line
(quoted diff) +0
-0@@ -396,6 +396,14 @@ static inline int inode_need_compress(struct inode*inode, u64 start, u64 end)
Quoted 11 lines
fs/btrfs/inode.c static inline int inode_need_compress(struct inode *inode, u64 start, u64 end) · line 405
Quoted 10 lines
fs/btrfs/inode.c +7
-0{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 ;Quoted 3 lines
This is also the wrong place to fix that, NODATASUM or NODATACOW inode should never make it to
compress_file_range(that callsinode_need_compress).
Quoted 4 lines
David, i've talk about that some time ago:
https://www.spinics.net/lists/linux-btrfs/msg73137.htmlNoCow 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.
Quoted 6 lines · David Sterba
Quoted 5 lines · Qu Wenruo
As btrfs(5) specified:
Note
If nodatacow or nodatasum are enabled, compression is disabled.
Quoted 6 lines
Quoted 4 lines
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
Quoted 5 lines
Quoted 2 lines
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.
Quoted 11 lines · David Sterba
Quoted 9 lines · 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.
Quoted 7 lines
Link please.
Quoted 2 lines
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.
Quoted 15 lines
Quoted 15 lines
Reported-by James Harvey <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/inode.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/btrfs/inode.c +0
-0@@ -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);
Quoted 11 lines
fs/btrfs/inode.c static inline int inode_need_compress(struct inode *inode, u64 start, u64 end) · line 405
Quoted 7 lines
fs/btrfs/inode.c +7
-0+/* +* 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 callsinode_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
Quoted 17 lines
Quoted 12 lines
fs/btrfs/inode.c +1
-0+/* force compress */ if (btrfs_test_opt (fs_info,FORCE_COMPRESS ))return 1 ;Signature · 6 lines
2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html