* [PATCH v5 0/2] Btrfs: compression fixes @ 2017-05-29 23:18 Timofey Titovets 2017-05-29 23:18 ` [PATCH v5 1/2] Btrfs: lzo.c - compressed data size must be less then input size Timofey Titovets 2017-05-29 23:18 ` [PATCH v5 2/2] Btrfs: compression must free at least one sector size Timofey Titovets 0 siblings, 2 replies; 7+ messages in thread From: Timofey Titovets @ 2017-05-29 23:18 UTC (permalink / raw) To: linux-btrfs; +Cc: 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v5 1/2] Btrfs: lzo.c - compressed data size must be less then input size 2017-05-29 23:18 [PATCH v5 0/2] Btrfs: compression fixes Timofey Titovets @ 2017-05-29 23:18 ` Timofey Titovets 2017-06-05 15:59 ` David Sterba 2017-05-29 23:18 ` [PATCH v5 2/2] Btrfs: compression must free at least one sector size Timofey Titovets 1 sibling, 1 reply; 7+ messages in thread From: Timofey Titovets @ 2017-05-29 23:18 UTC (permalink / raw) To: linux-btrfs; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/2] Btrfs: lzo.c - compressed data size must be less then input size 2017-05-29 23:18 ` [PATCH v5 1/2] Btrfs: lzo.c - compressed data size must be less then input size Timofey Titovets @ 2017-06-05 15:59 ` David Sterba 0 siblings, 0 replies; 7+ messages in thread From: David Sterba @ 2017-06-05 15:59 UTC (permalink / raw) To: Timofey Titovets; +Cc: linux-btrfs 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v5 2/2] Btrfs: compression must free at least one sector size 2017-05-29 23:18 [PATCH v5 0/2] Btrfs: compression fixes Timofey Titovets 2017-05-29 23:18 ` [PATCH v5 1/2] Btrfs: lzo.c - compressed data size must be less then input size Timofey Titovets @ 2017-05-29 23:18 ` Timofey Titovets 2017-06-05 16:10 ` David Sterba 1 sibling, 1 reply; 7+ messages in thread From: Timofey Titovets @ 2017-05-29 23:18 UTC (permalink / raw) To: linux-btrfs; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v5 2/2] Btrfs: compression must free at least one sector size 2017-05-29 23:18 ` [PATCH v5 2/2] Btrfs: compression must free at least one sector size Timofey Titovets @ 2017-06-05 16:10 ` David Sterba 2017-06-05 18:56 ` Timofey Titovets 0 siblings, 1 reply; 7+ messages in thread From: David Sterba @ 2017-06-05 16:10 UTC (permalink / raw) To: Timofey Titovets; +Cc: linux-btrfs 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 2/2] Btrfs: compression must free at least one sector size 2017-06-05 16:10 ` David Sterba @ 2017-06-05 18:56 ` Timofey Titovets 2017-06-06 11:22 ` David Sterba 0 siblings, 1 reply; 7+ messages in thread From: Timofey Titovets @ 2017-06-05 18:56 UTC (permalink / raw) To: dsterba, Timofey Titovets, linux-btrfs 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 2/2] Btrfs: compression must free at least one sector size 2017-06-05 18:56 ` Timofey Titovets @ 2017-06-06 11:22 ` David Sterba 0 siblings, 0 replies; 7+ messages in thread From: David Sterba @ 2017-06-06 11:22 UTC (permalink / raw) To: Timofey Titovets; +Cc: linux-btrfs 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-06-06 11:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-29 23:18 [PATCH v5 0/2] Btrfs: compression fixes Timofey Titovets 2017-05-29 23:18 ` [PATCH v5 1/2] Btrfs: lzo.c - compressed data size must be less then input size Timofey Titovets 2017-06-05 15:59 ` David Sterba 2017-05-29 23:18 ` [PATCH v5 2/2] Btrfs: compression must free at least one sector size Timofey Titovets 2017-06-05 16:10 ` David Sterba 2017-06-05 18:56 ` Timofey Titovets 2017-06-06 11:22 ` David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.