| Message ID | 20170525181220.24692-3-nefelim4ag@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 25, 2017 at 09:12:20PM +0300, Timofey Titovets wrote: > Btrfs already skip store of data where compression didn't > free at least one byte. Let's make logic better and make check > that compression free at least one sector size > because in another case it useless to store this data compressed Yeah, there's a room for improvement. Saving at least one sectorsize sounds ok to me. I'm not sure if this should be implemented inside the compressors (lzo, zlib). There'res the quick shortcut (the check "if (tot_in > 8192 && tot_in < tot_out)"), but otherwise the overall decision whether to use the compressed data is done in compress_file_range. 601 if (will_compress) { 602 /* 603 * we aren't doing an inline extent round the compressed size 604 * up to a block size boundary so the allocator does sane 605 * things 606 */ 607 total_compressed = ALIGN(total_compressed, blocksize); 608 609 /* 610 * one last check to make sure the compression is really a 611 * win, compare the page count read with the blocks on disk 612 */ 613 total_in = ALIGN(total_in, PAGE_SIZE); 614 if (total_compressed >= total_in) { 615 will_compress = 0; 616 } else { ... so the check would go to line 614. There's one case that your patch misses and it's the compressed inline extent. As we'd never submit more than one sectorsize of data to compression, the savings would be bigger than one page and thus we'd skip the compression. This could be fixed easily though, but I'd like to use it as an example why the decision should be moved upwards in the callchain (ie. to compress_file_range). -- 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
2017-05-29 17:23 GMT+03:00 David Sterba <dsterba@suse.cz>: > On Thu, May 25, 2017 at 09:12:20PM +0300, Timofey Titovets wrote: >> Btrfs already skip store of data where compression didn't >> free at least one byte. Let's make logic better and make check >> that compression free at least one sector size >> because in another case it useless to store this data compressed > > Yeah, there's a room for improvement. > > Saving at least one sectorsize sounds ok to me. I'm not sure if this > should be implemented inside the compressors (lzo, zlib). There'res the > quick shortcut (the check "if (tot_in > 8192 && tot_in < tot_out)"), but > otherwise the overall decision whether to use the compressed data is > done in compress_file_range. > > 601 if (will_compress) { > 602 /* > 603 * we aren't doing an inline extent round the compressed size > 604 * up to a block size boundary so the allocator does sane > 605 * things > 606 */ > 607 total_compressed = ALIGN(total_compressed, blocksize); > 608 > 609 /* > 610 * one last check to make sure the compression is really a > 611 * win, compare the page count read with the blocks on disk > 612 */ > 613 total_in = ALIGN(total_in, PAGE_SIZE); > 614 if (total_compressed >= total_in) { > 615 will_compress = 0; > 616 } else { > ... > > so the check would go to line 614. > > There's one case that your patch misses and it's the compressed inline extent. > As we'd never submit more than one sectorsize of data to compression, the > savings would be bigger than one page and thus we'd skip the compression. > > This could be fixed easily though, but I'd like to use it as an example why the > decision should be moved upwards in the callchain (ie. to compress_file_range). Thanks for advice Devid, i will update the patch. Also, as i move the check logic to new place, i want send another patch what will fix the difference in behaviour of check logic in lzo/zlib, i.e.: lzo.c: 232 if (tot_out > tot_in) 233 goto out; zlib.c: 194 if (workspace->strm.total_out >= workspace->strm.total_in) { 195 ret = -E2BIG; 196 goto out; 197 } I think that the zlib logic more smart, because if compressed size == uncompressed it's also useless
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c index bd0b0938..4aafae6f 100644 --- a/fs/btrfs/lzo.c +++ b/fs/btrfs/lzo.c @@ -26,6 +26,7 @@ #include <linux/bio.h> #include <linux/lzo.h> #include "compression.h" +#include "ctree.h" #define LZO_LEN 4 @@ -99,6 +100,7 @@ static int lzo_compress_pages(struct list_head *ws, int nr_pages = 0; struct page *in_page = NULL; struct page *out_page = NULL; + u32 sectorsize; unsigned long bytes_left; unsigned long len = *total_out; unsigned long nr_dest_pages = *out_pages; @@ -229,8 +231,13 @@ static int lzo_compress_pages(struct list_head *ws, in_len = min(bytes_left, PAGE_SIZE); } - if (tot_out > tot_in) + /* Compression must save at least one sectorsize */ + sectorsize = btrfs_inode_sectorsize(mapping->host); + + if (tot_out + sectorsize > tot_in) { + ret = -E2BIG; goto out; + } /* store the size of all chunks of compressed data */ cpage_out = kmap(pages[0]); diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c index 135b1082..f9957248 100644 --- a/fs/btrfs/zlib.c +++ b/fs/btrfs/zlib.c @@ -31,6 +31,7 @@ #include <linux/pagemap.h> #include <linux/bio.h> #include "compression.h" +#include "ctree.h" struct workspace { z_stream strm; @@ -86,6 +87,7 @@ static int zlib_compress_pages(struct list_head *ws, int nr_pages = 0; struct page *in_page = NULL; struct page *out_page = NULL; + u32 sectorsize; unsigned long bytes_left; unsigned long len = *total_out; unsigned long nr_dest_pages = *out_pages; @@ -191,7 +193,10 @@ static int zlib_compress_pages(struct list_head *ws, goto out; } - if (workspace->strm.total_out >= workspace->strm.total_in) { + /* Compression must save at least one sectorsize */ + sectorsize = btrfs_inode_sectorsize(mapping->host); + + if (workspace->strm.total_out + sectorsize > workspace->strm.total_in) { ret = -E2BIG; goto out; }
Btrfs already skip store of data where compression didn't free at least one byte. Let's make logic better and make check that compression free at least one sector size because in another case it useless to store this data compressed Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com> --- fs/btrfs/lzo.c | 9 ++++++++- fs/btrfs/zlib.c | 7 ++++++- 2 files changed, 14 insertions(+), 2 deletions(-) -- 2.13.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