| Message ID | 20170529231805.4797-3-nefelim4ag@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- 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-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
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; -- 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
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;
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(-) -- 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