[PATCH v4 0/2] Btrfs: compression fixes

[PATCH v4 0/2] Btrfs: compression fixes

[Timofey Titovets]

First patch:
Fix copy paste typo in debug message for lzo.c, lzo is not deflate

Second patch:
Force btrfs to not store data as compressed,
if compression will not free at least one sector size,
because it's useless in term of saving storage space and
reading data from disk, as a result productivity suffers.

Changes since v1:
- Merge patches for zlib and lzo in one
- Sync check logic for zlib and lzo
- Check profit after all data are compressed (not while compressing)

Changes since v2:
- Fix comparassion logic, it's enough if:
  compressed size + PAGE_SIZE not bigger then input data size

Changes since v3:
- Use btrfs sector size directly instead of assume that PAGE_SIZE == sectorsize

Timofey Titovets (2):
  Btrfs: lzo.c pr_debug() deflate->lzo
  Btrfs: compression must free at least one sector size

 fs/btrfs/lzo.c  | 11 +++++++++--
 fs/btrfs/zlib.c |  7 ++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

--
2.13.0

[PATCH v4 1/2] Btrfs: lzo.c pr_debug() deflate->lzo

[Timofey Titovets]

Fix copy paste typo in debug message for lzo.c, lzo is not deflate

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/lzo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index f48c8c14..bd0b0938 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -141,7 +141,7 @@ static int lzo_compress_pages(struct list_head *ws,
 		ret = lzo1x_1_compress(data_in, in_len, workspace->cbuf,
 				       &out_len, workspace->mem);
 		if (ret != LZO_E_OK) {
-			pr_debug("BTRFS: deflate in loop returned %d\n",
+			pr_debug("BTRFS: lzo in loop returned %d\n",
 			       ret);
 			ret = -EIO;
 			goto out;
--
2.13.0

Re: [PATCH v4 1/2] Btrfs: lzo.c pr_debug() deflate->lzo

[David Sterba]

On Thu, May 25, 2017 at 09:12:19PM +0300, Timofey Titovets wrote:
> Fix copy paste typo in debug message for lzo.c, lzo is not deflate
> 
> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>

Patch added, thanks.

[PATCH v4 2/2] Btrfs: compression must free at least one sector size

[Timofey Titovets]

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

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

Re: [PATCH v4 2/2] Btrfs: compression must free at least one sector size

[David Sterba]

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

Re: [PATCH v4 2/2] Btrfs: compression must free at least one sector size

[Timofey Titovets]

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

-- 
Have a nice day,
Timofey.