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

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

[Qu Wenruo]

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.

So this series is to properly fix the bug, firstly fix the lzo
regression which prevents inlined extent creation, then we can apply
Filipe's v1 fix.

However since Filipe's v3 is already in the for-next, the second patch
is reverting part of the v3 fix while keeping the extra size check, then
apply v1 fix.

If the series got reviewed, I'll change the 2nd patch to use Filipe's v1
fix with extra checks from v3 kept.

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

 fs/btrfs/inode.c |  7 +------
 fs/btrfs/lzo.c   | 32 +++++++++++++++++++++++++-------
 2 files changed, 26 insertions(+), 13 deletions(-)

-- 
2.55.0

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

[Qu Wenruo]

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

- No compression can be done if the length is smaller than @sectorsize

- If no space is saved, lzo can still create a compressed bio

[CAUSE]
The function copy_compressed_data_to_bio() is doing the space saving
checks 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 checks always
  return true and copy_compressed_data_to_bio() always return -E2BIG and
  failed to create an lzo compressed inlined extent.

- If the input length is multiple sectors (regular cases)
  E.g. max_out is 8K, sectorsize is 4K.
  Then we got (*total_out + copy_len) reaching 4K + 1, at this stage we
  should not continue compressing already.

  As the on-disk extent will already be round_up(4K + 1, 4K), reaching
  the original length (8K), thus saving no space.

[FIX]
Pass the full @cb structure into copy_compressed_data_to_bio(), so that
we know exactly the compression range, and know if this is for an
inlined extent.

And split the space saving checks into two parts:

- For inlined cases
  Only return -E2BIG if the compressed size reaches or exceeds the
  original size.

  This is the new check.

- For regular cases
  Only return -E2BIG if the rounded up compressed size reaches or
  exceeds the rounded up original size.

  The original check is only doing rounding down, which can be too late
  already.

  E.g. The current compressed size reaches 4K + 1bytes, and the max_out
  is 8K, at this stage we saves no space and should reject the
  compression.

Finally also skip the padding for inlined extent, as there should only
be one lzo payload, thus no padding is needed.

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

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 2f0996692da0..d0f3498bf6dd 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -154,7 +154,7 @@ static int write_and_queue_folio(struct bio *out_bio, struct folio **out_folio,
 /*
  * Copy compressed data to bio.
  *
- * @out_bio:		The bio that will contain all the compressed data.
+ * @cb:			The compressed bio that will contain all the compressed data.
  * @compressed_data:	The compressed data of this segment.
  * @compressed_size:	The size of the compressed data.
  * @out_folio:		The current output folio, will be updated if a new
@@ -174,16 +174,18 @@ static int write_and_queue_folio(struct bio *out_bio, struct folio **out_folio,
  * Will allocate new pages when needed.
  */
 static int copy_compressed_data_to_bio(struct btrfs_fs_info *fs_info,
-				       struct bio *out_bio,
+				       struct compressed_bio *cb,
 				       const char *compressed_data,
 				       size_t compressed_size,
 				       struct folio **out_folio,
 				       u32 *total_out, u32 max_out)
 {
+	struct bio *out_bio = &cb->bbio.bio;
 	const u32 sectorsize = fs_info->sectorsize;
 	const u32 sectorsize_bits = fs_info->sectorsize_bits;
 	const u32 fsize = btrfs_min_folio_size(fs_info);
 	const u32 old_size = out_bio->bi_iter.bi_size;
+	const bool is_inline = (cb->start == 0 && cb->len <= sectorsize);
 	u32 copy_start;
 	u32 sector_bytes_left;
 	char *kaddr;
@@ -223,9 +225,23 @@ static int copy_compressed_data_to_bio(struct btrfs_fs_info *fs_info,
 				     copy_start + compressed_size - *total_out);
 		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)
+		/*
+		 * To check if the compressed result really saves space,
+		 * the conditions are different for inline and regular cases.
+		 *
+		 * For regular cases, the rounded up size should not
+		 * reach the rounded up original size. Or we save no space.
+		 *
+		 * For inline cases, the compressed data should not reach the
+		 * original size, no need to consider the extent size since it
+		 * will be inlined.
+		 * If following the regular case condition, no inlined extent
+		 * can be created.
+		 */
+		if (*total_out + copy_len >= max_out && is_inline)
+			return -E2BIG;
+		if (round_up(*total_out + copy_len, sectorsize) >=
+		    round_up(max_out, sectorsize) && !is_inline)
 			return -E2BIG;
 
 		if (!*out_folio) {
@@ -245,9 +261,11 @@ static int copy_compressed_data_to_bio(struct btrfs_fs_info *fs_info,
 	/*
 	 * Check if we can fit the next segment header into the remaining space
 	 * of the sector.
+	 *
+	 * For inlined case no padding needed as there is no next payload.
 	 */
 	sector_bytes_left = round_up(*total_out, sectorsize) - *total_out;
-	if (sector_bytes_left >= LZO_LEN || sector_bytes_left == 0)
+	if (sector_bytes_left >= LZO_LEN || sector_bytes_left == 0 || is_inline)
 		return 0;
 
 	ASSERT(*out_folio);
@@ -316,7 +334,7 @@ int lzo_compress_bio(struct list_head *ws, struct compressed_bio *cb)
 			goto out;
 		}
 
-		ret = copy_compressed_data_to_bio(fs_info, bio, workspace->cbuf, out_len,
+		ret = copy_compressed_data_to_bio(fs_info, cb, workspace->cbuf, out_len,
 						  &folio_out, &total_out, len);
 		if (ret < 0)
 			goto out;
-- 
2.55.0

[PATCH 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]
Previous patch "btrfs: fix the incorrect lzo space saving checks"
fixed a regression in lzo_compress_bio() where it doesn't properly handle
inlined extents.

With that regression fixed, we can finally just pass range [0, i_size)
into btrfs_compress_bio(), and this gets rid of the trailing zeros,
getting back the old behavior, along with the older 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>
---
This is mostly the v1 fix from Filipe, the missing part is the fix in
the lzo path, which is now a dedicated patch.

If this solution is fine, I'll use Filipe's v1 fix instead, although I'd
prefer to keep the extra compressed size check as one extra final safenet to
catch unexpected LZO payload paddings.
---
 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