btrfs: Check the compression level before getting a workspace
diff mbox series

Message ID 1556247665-22859-1-git-send-email-johnnyc@synology.com
State New
Headers show
Series
  • btrfs: Check the compression level before getting a workspace
Related show

Commit Message

Johnny Chang April 26, 2019, 3:01 a.m. UTC
When a file's compression property is set as zlib or zstd but leave
the compression mount option not be set, that means btrfs will try
to compress the file with default compression level. But in
btrfs_compress_pages(), it calls get_workspace() with level = 0.
This will return a workspace with a wrong compression level.
For zlib, the compression level in the workspace will be 0
(that means "store only"). And for zstd, the compression in the
workspace will be 1, not the default level 3.

How to reproduce:
mkfs -t btrfs /dev/sdb
mount /dev/sdb /mnt/
mkdir /mnt/zlib
btrfs property set /mnt/zlib/ compression zlib
dd if=/dev/zero of=/mnt/zlib/compression-friendly-file-10M bs=1M count=10
sync
btrfs-debugfs -f /mnt/zlib/compression-friendly-file-10M

btrfs-debugfs output:
* before:
...
(258 9961472): ram 524288 disk 1106247680 disk_size 524288
file: ... extents 20 disk size 10485760 logical size 10485760 ratio 1.00

* after:
...
(258 10354688): ram 131072 disk 14217216 disk_size 4096
file: ... extents 80 disk size 327680 logical size 10485760 ratio 32.00

The steps for zstd are similar, but need to put a degging message to
show the level of the return workspace in zstd_get_workspace().

This commit adds a check of the compression level before getting a
workspace by set_level().

Signed-off-by: Johnny Chang <johnnyc@synology.com>
---
 fs/btrfs/compression.c | 1 +
 1 file changed, 1 insertion(+)

Comments

David Sterba April 26, 2019, 2:13 p.m. UTC | #1
On Fri, Apr 26, 2019 at 11:01:05AM +0800, Johnny Chang wrote:
> When a file's compression property is set as zlib or zstd but leave
> the compression mount option not be set, that means btrfs will try
> to compress the file with default compression level. But in
> btrfs_compress_pages(), it calls get_workspace() with level = 0.
> This will return a workspace with a wrong compression level.
> For zlib, the compression level in the workspace will be 0
> (that means "store only"). And for zstd, the compression in the
> workspace will be 1, not the default level 3.
> 
> How to reproduce:
> mkfs -t btrfs /dev/sdb
> mount /dev/sdb /mnt/
> mkdir /mnt/zlib
> btrfs property set /mnt/zlib/ compression zlib
> dd if=/dev/zero of=/mnt/zlib/compression-friendly-file-10M bs=1M count=10
> sync
> btrfs-debugfs -f /mnt/zlib/compression-friendly-file-10M
> 
> btrfs-debugfs output:
> * before:
> ...
> (258 9961472): ram 524288 disk 1106247680 disk_size 524288
> file: ... extents 20 disk size 10485760 logical size 10485760 ratio 1.00
> 
> * after:
> ...
> (258 10354688): ram 131072 disk 14217216 disk_size 4096
> file: ... extents 80 disk size 327680 logical size 10485760 ratio 32.00
> 
> The steps for zstd are similar, but need to put a degging message to
> show the level of the return workspace in zstd_get_workspace().
> 
> This commit adds a check of the compression level before getting a
> workspace by set_level().

Good catch. I think it's safest set the level at btrfs_compress_pages
though it means one extra call, compared to setting it when the property
is set. This will change once the properties store the level too as it
has to be combined with fs_info::compress_level.

Reviewed-by: David Sterba <dsterba@suse.com>

Patch
diff mbox series

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 4f2a8ae..716656d 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1009,6 +1009,7 @@  int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
 	struct list_head *workspace;
 	int ret;
 
+	level = btrfs_compress_op[type]->set_level(level);
 	workspace = get_workspace(type, level);
 	ret = btrfs_compress_op[type]->compress_pages(workspace, mapping,
 						      start, pages,