[PATCH v5 0/2] Btrfs: compression fixes

[PATCH v5 0/2] Btrfs: compression fixes

[Timofey Titovets]

First patch:
Sync comparison logic of compressed/uncompressed sizes from zlib to lzo

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
  Thanks to Chandan

Changes since v4:
- Old first patch - merged (fix typo)
- New first patch - synced some logic (see patch) from zlib to lzo
- Check compressed/uncompressed size in right place compress_file_range()
  Thanks to David

Timofey Titovets (2):
  Btrfs: lzo.c compressed data size must be less then input size
  Btrfs: compression must free at least one sector size

 fs/btrfs/inode.c | 3 ++-
 fs/btrfs/lzo.c   | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

--
2.13.0

[PATCH v5 1/2] Btrfs: lzo.c - compressed data size must be less then input size

[Timofey Titovets]

Logic already return error if compression
make data bigger, let's sync logic with zlib
and also return error if compressed size
are equal to input size

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

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index f48c8c14..f66691e0 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -229,8 +229,10 @@ static int lzo_compress_pages(struct list_head *ws,
 		in_len = min(bytes_left, PAGE_SIZE);
 	}

-	if (tot_out > tot_in)
+	if (tot_out >= tot_in) {
+		ret = -E2BIG;
 		goto out;
+	}

 	/* store the size of all chunks of compressed data */
 	cpage_out = kmap(pages[0]);
--
2.13.0

[PATCH v5 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/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 17cbe930..2793007b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -609,9 +609,10 @@ static noinline void compress_file_range(struct inode *inode,
 		/*
 		 * one last check to make sure the compression is really a
 		 * win, compare the page count read with the blocks on disk
+		 * compression must free at least one sector size
 		 */
 		total_in = ALIGN(total_in, PAGE_SIZE);
-		if (total_compressed >= total_in) {
+		if (total_compressed + blocksize > total_in) {
 			will_compress = 0;
 		} else {
 			num_bytes = total_in;
--
2.13.0

Re: [PATCH v5 1/2] Btrfs: lzo.c - compressed data size must be less then input size

[David Sterba]

On Tue, May 30, 2017 at 02:18:04AM +0300, Timofey Titovets wrote:
> Logic already return error if compression
> make data bigger, let's sync logic with zlib
> and also return error if compressed size
> are equal to input size
> 
> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>

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

I've updated the changelog a bit.

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

[David Sterba]

On Tue, May 30, 2017 at 02:18:05AM +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
> 
> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
> ---
>  fs/btrfs/inode.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 17cbe930..2793007b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -609,9 +609,10 @@ static noinline void compress_file_range(struct inode *inode,
>  		/*
>  		 * one last check to make sure the compression is really a
>  		 * win, compare the page count read with the blocks on disk
> +		 * compression must free at least one sector size
>  		 */
>  		total_in = ALIGN(total_in, PAGE_SIZE);
> -		if (total_compressed >= total_in) {
> +		if (total_compressed + blocksize > total_in) {

We're doing aligned calculation here, shouldn't this be >= ?

If total_compressed + blocksize == total_in, we have saved exactly one
blocksize.

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

[Timofey Titovets]

2017-06-05 19:10 GMT+03:00 David Sterba <dsterba@suse.cz>:
> On Tue, May 30, 2017 at 02:18:05AM +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
>>
>> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
>> ---
>>  fs/btrfs/inode.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 17cbe930..2793007b 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -609,9 +609,10 @@ static noinline void compress_file_range(struct inode *inode,
>>               /*
>>                * one last check to make sure the compression is really a
>>                * win, compare the page count read with the blocks on disk
>> +              * compression must free at least one sector size
>>                */
>>               total_in = ALIGN(total_in, PAGE_SIZE);
>> -             if (total_compressed >= total_in) {
>> +             if (total_compressed + blocksize > total_in) {
>
> We're doing aligned calculation here, shouldn't this be >= ?
>
> If total_compressed + blocksize == total_in, we have saved exactly one
> blocksize.

We discussed that already in:
https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg64350.html

IIRC: long in short:
- input data size 8192
- output data size 4096

This invertion logic, i.e. check if compression can be skipped, if
comparasion are true -> skip compression.

In case of above check,
old logic:
total_compressed >= total_in
4096 >= 8192 -> will_compress=1
With if blocksize added:
4096+4096 >= 8192 -> will_compress=0
So this must be changed to:
4096+4096 > 8192 -> will_compress=1
Because compression save one blocksize

Also will_compress not used after this code, so in theory this code
can be refactored to be more obvious, like that:
  total_in = ALIGN(total_in, PAGE_SIZE);
-             if (total_compressed + blocksize > total_in) {
-                          will_compress = 0;
-             } else {
+            if (total_compressed + blocksize <= total_in) {
                          num_bytes = total_in;
                           *num_added += 1;

Thanks

-- 
Have a nice day,
Timofey.

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

[David Sterba]

On Mon, Jun 05, 2017 at 09:56:14PM +0300, Timofey Titovets wrote:
> 2017-06-05 19:10 GMT+03:00 David Sterba <dsterba@suse.cz>:
> > On Tue, May 30, 2017 at 02:18:05AM +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
> >>
> >> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
> >> ---
> >>  fs/btrfs/inode.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >> index 17cbe930..2793007b 100644
> >> --- a/fs/btrfs/inode.c
> >> +++ b/fs/btrfs/inode.c
> >> @@ -609,9 +609,10 @@ static noinline void compress_file_range(struct inode *inode,
> >>               /*
> >>                * one last check to make sure the compression is really a
> >>                * win, compare the page count read with the blocks on disk
> >> +              * compression must free at least one sector size
> >>                */
> >>               total_in = ALIGN(total_in, PAGE_SIZE);
> >> -             if (total_compressed >= total_in) {
> >> +             if (total_compressed + blocksize > total_in) {
> >
> > We're doing aligned calculation here, shouldn't this be >= ?
> >
> > If total_compressed + blocksize == total_in, we have saved exactly one
> > blocksize.
> 
> We discussed that already in:
> https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg64350.html
> 
> IIRC: long in short:
> - input data size 8192
> - output data size 4096
> 
> This invertion logic, i.e. check if compression can be skipped, if
> comparasion are true -> skip compression.
> 
> In case of above check,
> old logic:
> total_compressed >= total_in
> 4096 >= 8192 -> will_compress=1
> With if blocksize added:
> 4096+4096 >= 8192 -> will_compress=0
> So this must be changed to:
> 4096+4096 > 8192 -> will_compress=1
> Because compression save one blocksize
> 
> Also will_compress not used after this code, so in theory this code
> can be refactored to be more obvious, like that:
>   total_in = ALIGN(total_in, PAGE_SIZE);
> -             if (total_compressed + blocksize > total_in) {
> -                          will_compress = 0;
> -             } else {
> +            if (total_compressed + blocksize <= total_in) {

Thanks. That way it's much more obvious to read, please update the patch
in that regard (ie. also removing will_compress).

>                           num_bytes = total_in;
>                            *num_added += 1;