[PATCH v2 0/2] btrfs: remove the tailing zeros from compressed inline extents

[PATCH v2 0/2] btrfs: remove the tailing zeros from compressed inline extents

[Qu Wenruo]

[CHANGELOG]
v2:
- Remove the block based space saving checks completely from lzo
  Zlib and ZSTD do not have block based space saving in the first place.
  It's the caller's responsibility to check, and inlined and regular
  extents have different requirements.

  This simplify the first patch a lot.

- Update the cover letter
  Filipe's v3 fix is already merged, so we can not force push a fix but
  to co-operate the v3 fix.

Commit 3eaf5f082c4c ("btrfs: extract inlined creation into a dedicated
delalloc helper") changed the compression input from [0, i_size) to [0,
blocksize), which caused two problems:

- Other tools unable to decompress the inlined extent
  U-boot and btrfs-restore are affected, as they only allocated a buffer
  which is @ram_bytes sized.
  That buffer is too small to contain the decompressed data, which is
  @sectorsize.

  Those projects are fixed to have a more robust decompression routine
  which can handle both cases now.

- Worse ratio for those compressed inline extent.

  For the same 3K 0xcd filled range, the results are small but
  observable, 48 vs 61 bytes.

Filipe's v1 fix is very close to a proper fix, but btrfs will unable to
create inlined extents for lzo.
It turns out to be another bug in the copy_compressed_data_to_bio()
function.

Which is doing a premature block size based space saving checks,
meanwhile all other algorithms do not have block sized based checks, but
only a simple "@compressed >= @input" check.
Zlib and Zstd all rely on the caller to do proper space saving checks,
as regular and inlined extents have different requirements.

So this series is to properly fix the bug, firstly fix the lzo
regression which prevents inlined extent creation, then restore the old
i_size based inline extent creation.

Qu Wenruo (2):
  btrfs: lzo: fix space saving checks
  btrfs: remove the trailing zeros from compressed inline extent

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

-- 
2.55.0

[PATCH v2 1/2] btrfs: lzo: fix space saving checks

[Qu Wenruo]

[BUG]
Since commit 3be8a788eed3 ("btrfs: lzo: introduce lzo_compress_bio()
helper"), the space saving check on lzo is broken:

- Space saving check is always done based on block size
  This prevents lzo_compress_bio() to do any compression if the @len is
  smaller than sectorsize.

  This prevents proper inlined extent creation.

- Space saving check is not including the padding zeros
  If the remaining space in the output folio is not enough, we add
  padding zeros (up to 3 bytes), which contributes to the total
  compressed size.

  But that is not checked.

[CAUSE]

The function copy_compressed_data_to_bio() is doing the space saving
check wrong:

		/* With the range copied, we're larger than the original range. */
		if (((*total_out + copy_len) >> sectorsize_bits) >=
		    max_out >> sectorsize_bits)

- If the input length is smaller than sectorsize (inlined case)
  Then @max_out >> sectorsize_bits returns 0, the above check always
  returns true and copy_compressed_data_to_bio() always return -E2BIG and
  failed to create an lzo compressed inlined extent.

This check is LZO specific, as other algorithms do not have any block
aligned check during compression.
The remaining two algorithms are all relying on regular input/output
size check, and the final common space saving check at
compress_file_extent().

And for the extra padding bytes, there is no space saving check at all.

[FIX]
- Do a simpler space saving check
  By just check if the compressed size meets or exceeds the input size.
  If so reject it with -E2BIG.

  With this fixed, lzo_compress_bio() can properly handle a range that is
  smaller than a sector, which is required to get better compression ratio
  for inlined extents.

- Do space saving check before padding
  This will ensure that the compressed size never meets or exceeds the
  input size.

Fixes: 3be8a788eed3 ("btrfs: lzo: introduce lzo_compress_bio() helper")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/lzo.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 2f0996692da0..4e7837dec332 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -224,8 +224,7 @@ static int copy_compressed_data_to_bio(struct btrfs_fs_info *fs_info,
 		u32 foffset = *total_out & (fsize - 1);
 
 		/* With the range copied, we're larger than the original range. */
-		if (((*total_out + copy_len) >> sectorsize_bits) >=
-		    max_out >> sectorsize_bits)
+		if (*total_out + copy_len >= max_out)
 			return -E2BIG;
 
 		if (!*out_folio) {
@@ -253,6 +252,8 @@ static int copy_compressed_data_to_bio(struct btrfs_fs_info *fs_info,
 	ASSERT(*out_folio);
 
 	/* The remaining size is not enough, pad it with zeros */
+	if (*total_out + sector_bytes_left >= max_out)
+		return -E2BIG;
 	folio_zero_range(*out_folio, offset_in_folio(*out_folio, *total_out), sector_bytes_left);
 	return write_and_queue_folio(out_bio, out_folio, total_out, sector_bytes_left);
 }
-- 
2.55.0

[PATCH v2 2/2] btrfs: remove the trailing zeros from compressed inline extent

[Qu Wenruo]

[BEHAVIOR CHANGE]
After commit 3eaf5f082c4c ("btrfs: extract inlined creation into a
dedicated delalloc helper"), btrfs changed its behavior when generating
compressed inlined extents.

Previously the compression input was the file range [0, i_size), but
after that commit the input is file range [0, sectorsize).

This means the decompression handling needs to have a buffer that is no
smaller than sectorsize, or the decompression will fail.

This has already caused problems for other projects, like u-boot and
btrfs-restore from btrfs-progs.
Although those projects are fixed with a more robust decompression path,
this kernel change also causes extra space usage for compressed inlined
extents:

 All doing a 3K writes with content filled with 0xcd

 Before:
        item 6 key (257 EXTENT_DATA 0) itemoff 15794 itemsize 69
                generation 9 type 0 (inline)
                inline extent data size 48 ram_bytes 3072 compression 2 (lzo) encryption 0

 After:
         item 6 key (257 EXTENT_DATA 0) itemoff 15781 itemsize 82
                generation 9 type 0 (inline)
                inline extent data size 61 ram_bytes 3072 compression 2 (lzo) encryption 0

This behavior change also increased the lzo compressed size from 48 bytes to 61 bytes.

[FIX]
The previous patch "btrfs: lzo: fix space saving checks which prevents
inline extents" fixed a regression in lzo_compress_bio() where it doesn't
properly handle ranges smaller than a sector.

With that regression fixed, we can finally pass the range [0, i_size) into
btrfs_compress_bio(), and this gets rid of the trailing zeros,
restoring the old behavior, along with the older and better
compression ratio.

Fixes: 3eaf5f082c4c ("btrfs: extract inlined creation into a dedicated delalloc helper")
Reported-by: Hanabishi <i.r.e.c.c.a.k.u.n+kernel.org@gmail.com>
Link: https://lore.kernel.org/linux-btrfs/c97652a5-ac6b-4de6-aa23-3cdebc01d00b@gmail.com/
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 53f4532593b3..5e083ea5c577 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2342,12 +2342,7 @@ static int run_delalloc_inline(struct btrfs_inode *inode, struct folio *locked_f
 		} else if (inode->prop_compress) {
 			compress_type = inode->prop_compress;
 		}
-		/*
-		 * We need to pass blocksize and not i_size, otherwise we can't
-		 * create compressed inline extents for data smaller than sector
-		 * size with lzo.
-		 */
-		cb = btrfs_compress_bio(inode, 0, blocksize, compress_type, compress_level, 0);
+		cb = btrfs_compress_bio(inode, 0, i_size, compress_type, compress_level, 0);
 		if (IS_ERR(cb)) {
 			cb = NULL;
 			/* Just fall back to non-compressed case. */
-- 
2.55.0