btrfs: reject single block sized compression early

Currently for an inode that needs compression, even if there is a delalloc range that is single fs block sized and can not be inlined, we will still go through the compression path.

Then inside compress_file_range(), we have one extra check to reject single block sized range, and fall back to regular uncompressed write.

This rejection is in fact a little too late, we have already allocated memory to async_chunk, delayed the submission, just to fallback to the same uncompressed write.

Change the behavior to reject such cases earlier at inode_need_compress(), so for such single block sized range we won't even bother trying to go through compress path.

And since the inline small block check is inside inode_need_compress() and compress_file_range() also calls that function, we no longer need a dedicate check inside compress_file_range().

Signed-off-by Qu Wenruo <[email protected]>

Currently for an inode that needs compression, even if there is a delalloc range that is single fs block sized and can not be inlined, we will still go through the compression path.

Then inside compress_file_range(), we have one extra check to reject single block sized range, and fall back to regular uncompressed write.

This rejection is in fact a little too late, we have already allocated memory to async_chunk, delayed the submission, just to fallback to the same uncompressed write.

Change the behavior to reject such cases earlier at inode_need_compress(), so for such single block sized range we won't even bother trying to go through compress path.

And since the inline small block check is inside inode_need_compress() and compress_file_range() also calls that function, we no longer need a dedicate check inside compress_file_range().

Signed-off-by Qu Wenruo <[email protected]>

---
Changelog:
v2:
- Remove the duplicated check inside compress_file_range() now As the same check is done inside inode_need_compress() and compress_file_range() also call that function to do the check.

---
 fs/btrfs/inode.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

fs/btrfs/inode.c +9 -13

index cf452eaf0672..fbb8ad55b589 100644
@@ -816,6 +816,13 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
                return 0;
        }
 
+       /*
+        * If the delalloc range is only one fs block and can not be inlined,
+        * do not even bother try compression, as there will be no space saving
+        * and will always fallback to regular write later.
+        */
+       if (start != 0 && end + 1 - start <= fs_info->sectorsize)
+               return 0;
        /* Defrag ioctl takes precedence over mount options and properties. */
        if (inode->defrag_compress == BTRFS_DEFRAG_DONT_COMPRESS)
                return 0;

@@ -953,18 +960,8 @@ static void compress_file_range(struct btrfs_work *work)
        if (actual_end <= start)
                goto cleanup_and_bail_uncompressed;
 
-       total_compressed = actual_end - start;
-
-       /*
-        * Skip compression for a small file range(<=blocksize) that
-        * isn't an inline extent, since it doesn't save disk space at all.
-        */
-       if (total_compressed <= blocksize &&
-          (start > 0 || end + 1 < inode->disk_i_size))
-               goto cleanup_and_bail_uncompressed;
-
-       total_compressed = min_t(unsigned long, total_compressed,
-                       BTRFS_MAX_UNCOMPRESSED);
+       total_compressed = min_t(unsigned long, actual_end - start,
+                                BTRFS_MAX_UNCOMPRESSED);
        total_in = 0;
        ret = 0;
 
--

2.52.0

Currently for an inode that needs compression, even if there is a delalloc range that is single fs block sized and can not be inlined, we will still go through the compression path.

Then inside compress_file_range(), we have one extra check to reject single block sized range, and fall back to regular uncompressed write.

This rejection is in fact a little too late, we have already allocated memory to async_chunk, delayed the submission, just to fallback to the same uncompressed write.

Change the behavior to reject such cases earlier at inode_need_compress(), so for such single block sized range we won't even bother trying to go through compress path.

And since the inline small block check is inside inode_need_compress() and compress_file_range() also calls that function, we no longer need a dedicate check inside compress_file_range().

btrfs/inode.c +0 -0

index 247b373bf5cf0..be47aa58e944e 100644

btrfs/inode.c +0 -0

 	/* Defrag ioctl takes precedence over mount options and properties. */
 	if (inode->defrag_compress == BTRFS_DEFRAG_DONT_COMPRESS)
 		return 0;

btrfs/inode.c +1 -12

@@ -953,18 +960,7 @@ static void compress_file_range(struct btrfs_work *work)
 	if (actual_end <= start)
 		goto cleanup_and_bail_uncompressed;
 
-	total_compressed = actual_end - start;
-
-	/*
-	 * Skip compression for a small file range(<=blocksize) that
-	 * isn't an inline extent, since it doesn't save disk space at all.
-	 */
-	if (total_compressed <= blocksize &&
-	   (start > 0 || end + 1 < inode->disk_i_size))
-		goto cleanup_and_bail_uncompressed;
-
-	total_compressed = min_t(unsigned long, total_compressed,
-			BTRFS_MAX_UNCOMPRESSED);
+	total_compressed = min_t(unsigned long, actual_end - start, BTRFS_MAX_UNCOMPRESSED);
 	total_in = 0;
 	ret = 0;

Quoted 11 lines · Qu Wenruo

Currently for an inode that needs compression, even if there is a delalloc range that is single fs block sized and can not be inlined, we will still go through the compression path.

Then inside compress_file_range(), we have one extra check to reject single block sized range, and fall back to regular uncompressed write.

This rejection is in fact a little too late, we have already allocated memory to async_chunk, delayed the submission, just to fallback to the same uncompressed write.

Quoted 8 lines

Change the behavior to reject such cases earlier at inode_need_compress(), so for such single block sized range we won't even bother trying to go through compress path.

And since the inline small block check is inside inode_need_compress() and compress_file_range() also calls that function, we no longer need a dedicate check inside compress_file_range().

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews are already useful. I've kept the headers to make it easier to double check that I'm actually replying to the right email, but please let me know if any parts of this are incorrect:

commit 59615e2c1f63dfd65f74c166e803873d3362e51a
Author: Qu Wenruo <[email protected]>

btrfs: reject single block sized compression early

This commit moves single block size rejection earlier from compress_file_range() to inode_need_compress() to avoid allocating async_chunk memory for ranges that will not be compressed anyway.

Link https://git.kernel.org/linus/59615e2c1f63dfd65f74c166e803873d3362e51a

Quoted 5 lines

btrfs/inode.c +0 -0

index 247b373bf5cf0..be47aa58e944e 100644

btrfs/inode.c static inline int inode_need_compress(struct btrfs_inode *inode, u64 start, · line 825

Quoted 10 lines

btrfs/inode.c +7 -0

@@ -816,6 +816,13 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
  		return 0;
  	}
 
+	/*
+	 * If the delalloc range is only one fs block and can not be inlined,
+	 * do not even bother try compression, as there will be no space saving
+	 * and will always fallback to regular write later.
+	 */
+	if (start != 0 && end + 1 - start <= fs_info->sectorsize)
+		return 0;

Is the early rejection check in inode_need_compress() missing the 'end + 1 < inode->disk_i_size' condition from the original check?

The old code in compress_file_range() rejected single-block ranges with:

     if (total_compressed <= blocksize &&
        (start > 0 || end + 1 < inode->disk_i_size))
         goto cleanup_and_bail_uncompressed;

The new check only tests 'start != 0' but doesn't check whether the write reaches EOF. This means a single-block write at offset 0 that doesn't reach EOF will now go through compression, inline attempt, and ratio checking in compress_file_range() before being rejected, when the old code rejected it immediately.

Doesn't this cause MORE work for the case where start == 0 AND single block AND end + 1 < disk_i_size?

Quoted 3 lines

btrfs/inode.c +0 -0

  	/* Defrag ioctl takes precedence over mount options and properties. */
  	if (inode->defrag_compress == BTRFS_DEFRAG_DONT_COMPRESS)
  		return 0;

[ ... ]

Quoted 19 lines

btrfs/inode.c +1 -12

@@ -953,18 +960,7 @@ static void compress_file_range(struct btrfs_work *work)
  	if (actual_end <= start)
  		goto cleanup_and_bail_uncompressed;
 
-	total_compressed = actual_end - start;
-
-	/*
-	 * Skip compression for a small file range(<=blocksize) that
-	 * isn't an inline extent, since it doesn't save disk space at all.
-	 */
-	if (total_compressed <= blocksize &&
-	   (start > 0 || end + 1 < inode->disk_i_size))
-		goto cleanup_and_bail_uncompressed;
-
-	total_compressed = min_t(unsigned long, total_compressed,
-			BTRFS_MAX_UNCOMPRESSED);
+	total_compressed = min_t(unsigned long, actual_end - start, BTRFS_MAX_UNCOMPRESSED);
  	total_in = 0;
  	ret = 0;

Quoted 69 lines · Chris Mason

Quoted 11 lines · Qu Wenruo

Currently for an inode that needs compression, even if there is a delalloc range that is single fs block sized and can not be inlined, we will still go through the compression path.

Then inside compress_file_range(), we have one extra check to reject single block sized range, and fall back to regular uncompressed write.

This rejection is in fact a little too late, we have already allocated memory to async_chunk, delayed the submission, just to fallback to the same uncompressed write.

Quoted 8 lines

Change the behavior to reject such cases earlier at inode_need_compress(), so for such single block sized range we won't even bother trying to go through compress path.

And since the inline small block check is inside inode_need_compress() and compress_file_range() also calls that function, we no longer need a dedicate check inside compress_file_range().

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews are already useful. I've kept the headers to make it easier to double check that I'm actually replying to the right email, but please let me know if any parts of this are incorrect:

commit 59615e2c1f63dfd65f74c166e803873d3362e51a
Author: Qu Wenruo <[email protected]>

btrfs: reject single block sized compression early

This commit moves single block size rejection earlier from compress_file_range() to inode_need_compress() to avoid allocating async_chunk memory for ranges that will not be compressed anyway.

Link https://git.kernel.org/linus/59615e2c1f63dfd65f74c166e803873d3362e51a

Quoted 5 lines

btrfs/inode.c +0 -0

index 247b373bf5cf0..be47aa58e944e 100644

btrfs/inode.c static inline int inode_need_compress(struct btrfs_inode *inode, u64 start, · line 825

Quoted 10 lines

btrfs/inode.c +7 -0

@@ -816,6 +816,13 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
  		return 0;
  	}
 
+	/*
+	 * If the delalloc range is only one fs block and can not be inlined,
+	 * do not even bother try compression, as there will be no space saving
+	 * and will always fallback to regular write later.
+	 */
+	if (start != 0 && end + 1 - start <= fs_info->sectorsize)
+		return 0;

Is the early rejection check in inode_need_compress() missing the 'end + 1 < inode->disk_i_size' condition from the original check?

The old code in compress_file_range() rejected single-block ranges with:

     if (total_compressed <= blocksize &&
        (start > 0 || end + 1 < inode->disk_i_size))
         goto cleanup_and_bail_uncompressed;

Quoted 5 lines

The new check only tests 'start != 0' but doesn't check whether the write reaches EOF. This means a single-block write at offset 0 that doesn't reach EOF will now go through compression, inline attempt, and ratio checking in compress_file_range() before being rejected, when the old code rejected it immediately.

I think the change is just fine.

The point is, the rejection for that specific case happens at the same timing, inside compress_file_range(), no matter if it's before or after the patch.

The patch improves the checks for most single block writes by rejecting them much earlier.

But for this particular fileoff 0 single block writes, we just go back to the old timing inside compress_file_range(), aka falling back to the old behavior.

So there is no extra lose, just a corner case where it still goes the old path.

Quoted 91 lines · Qu Wenruo

Quoted 69 lines · Chris Mason

Quoted 11 lines · Qu Wenruo

Currently for an inode that needs compression, even if there is a delalloc range that is single fs block sized and can not be inlined, we will still go through the compression path.

Then inside compress_file_range(), we have one extra check to reject single block sized range, and fall back to regular uncompressed write.

This rejection is in fact a little too late, we have already allocated memory to async_chunk, delayed the submission, just to fallback to the same uncompressed write.

Quoted 8 lines

Change the behavior to reject such cases earlier at inode_need_compress(), so for such single block sized range we won't even bother trying to go through compress path.

And since the inline small block check is inside inode_need_compress() and compress_file_range() also calls that function, we no longer need a dedicate check inside compress_file_range().

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews are already useful. I've kept the headers to make it easier to double check that I'm actually replying to the right email, but please let me know if any parts of this are incorrect:

commit 59615e2c1f63dfd65f74c166e803873d3362e51a
Author: Qu Wenruo <[email protected]>

btrfs: reject single block sized compression early

This commit moves single block size rejection earlier from compress_file_range() to inode_need_compress() to avoid allocating async_chunk memory for ranges that will not be compressed anyway.

Link https://git.kernel.org/linus/59615e2c1f63dfd65f74c166e803873d3362e51a

Quoted 5 lines

btrfs/inode.c +0 -0

index 247b373bf5cf0..be47aa58e944e 100644

btrfs/inode.c static inline int inode_need_compress(struct btrfs_inode *inode, u64 start, · line 825

Quoted 10 lines

btrfs/inode.c +7 -0

@@ -816,6 +816,13 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
   		return 0;
   	}
 
+	/*
+	 * If the delalloc range is only one fs block and can not be inlined,
+	 * do not even bother try compression, as there will be no space saving
+	 * and will always fallback to regular write later.
+	 */
+	if (start != 0 && end + 1 - start <= fs_info->sectorsize)
+		return 0;

Is the early rejection check in inode_need_compress() missing the 'end + 1 < inode->disk_i_size' condition from the original check?

The old code in compress_file_range() rejected single-block ranges with:

      if (total_compressed <= blocksize &&
         (start > 0 || end + 1 < inode->disk_i_size))
          goto cleanup_and_bail_uncompressed;

Quoted 5 lines

The new check only tests 'start != 0' but doesn't check whether the write reaches EOF. This means a single-block write at offset 0 that doesn't reach EOF will now go through compression, inline attempt, and ratio checking in compress_file_range() before being rejected, when the old code rejected it immediately.

I think the change is just fine.

The point is, the rejection for that specific case happens at the same timing, inside compress_file_range(), no matter if it's before or after the patch.

The patch improves the checks for most single block writes by rejecting them much earlier.

But for this particular fileoff 0 single block writes, we just go back to the old timing inside compress_file_range(), aka falling back to the old behavior.

Quoted 2 lines

So there is no extra lose, just a corner case where it still goes the old path.

I almost didn't send this one because I do see your point. But what do we lose by moving the original test into the short circuit? I don't see why we'd want to do extra work when start == 0?

-chris