diff mbox series

[16/23] btrfs: further simplify the compress or not logic in compress_file_range

Message ID 20230628153144.22834-17-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/23] btrfs: pass a flags argument to cow_file_range | expand

Commit Message

Christoph Hellwig June 28, 2023, 3:31 p.m. UTC
Currently the logic whether to compress or not in compress_file_range is
a bit convoluted because it tries to share code for creating inline
extents for the compressible [1] path and the bail to uncompressed path.

But the latter isn't needed at all, because cow_file_range as called by
submit_uncompressed_range will already create inline extents as needed,
so there is no need to have special handling for it if we can live with
the fact that it will be called a bit later in the ->ordered_func of the
workqueue instead of right now.

[1] there is undocumented logic that creates an uncompressed inline
extent outside of the shall not compress logic if total_in is too small.
This logic isn't explained in comments or any commit log I could find,
so I've preserved it.  Documentation explaining it would be appreciated
if anyone understands this code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/inode.c | 51 ++++++++++++++++--------------------------------
 1 file changed, 17 insertions(+), 34 deletions(-)

Comments

Josef Bacik July 14, 2023, 1:47 p.m. UTC | #1
On Wed, Jun 28, 2023 at 05:31:37PM +0200, Christoph Hellwig wrote:
> Currently the logic whether to compress or not in compress_file_range is
> a bit convoluted because it tries to share code for creating inline
> extents for the compressible [1] path and the bail to uncompressed path.
> 
> But the latter isn't needed at all, because cow_file_range as called by
> submit_uncompressed_range will already create inline extents as needed,
> so there is no need to have special handling for it if we can live with
> the fact that it will be called a bit later in the ->ordered_func of the
> workqueue instead of right now.
> 
> [1] there is undocumented logic that creates an uncompressed inline
> extent outside of the shall not compress logic if total_in is too small.
> This logic isn't explained in comments or any commit log I could find,
> so I've preserved it.  Documentation explaining it would be appreciated
> if anyone understands this code.
> 

Looks like it's just a optimization, no reason to allocate a whole async extent
if we can just inline a page and be done.  It's not necessarily required, but
could use a comment.  Thanks,

Josef
diff mbox series

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e7c05d07ff50f8..560682a5d9d7aa 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -847,7 +847,6 @@  static void compress_file_range(struct btrfs_work *work)
 	unsigned long total_in = 0;
 	unsigned int poff;
 	int i;
-	int will_compress;
 	int compress_type = fs_info->compress_type;
 	int redirty = 0;
 
@@ -867,7 +866,6 @@  static void compress_file_range(struct btrfs_work *work)
 	barrier();
 	actual_end = min_t(u64, i_size, end + 1);
 again:
-	will_compress = 0;
 	pages = NULL;
 	nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
 	nr_pages = min_t(unsigned long, nr_pages, BTRFS_MAX_COMPRESSED_PAGES);
@@ -917,14 +915,12 @@  static void compress_file_range(struct btrfs_work *work)
 	 * discover bad compression ratios.
 	 */
 	if (!inode_need_compress(inode, start, end))
-		goto cont;
+		goto cleanup_and_bail_uncompressed;
 
 	pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
-	if (!pages) {
+	if (!pages)
 		/* just bail out to the uncompressed code */
-		nr_pages = 0;
-		goto cont;
-	}
+		goto cleanup_and_bail_uncompressed;
 
 	if (inode->defrag_compress)
 		compress_type = inode->defrag_compress;
@@ -953,7 +949,7 @@  static void compress_file_range(struct btrfs_work *work)
 				   mapping, start, pages, &nr_pages, &total_in,
 				   &total_compressed);
 	if (ret)
-		goto cont;
+		goto cleanup_and_bail_uncompressed;
 
 	/*
 	 * Zero the tail end of the last page, as we might be sending it down
@@ -962,24 +958,22 @@  static void compress_file_range(struct btrfs_work *work)
 	poff = offset_in_page(total_compressed);
 	if (poff)
 		memzero_page(pages[nr_pages - 1], poff, PAGE_SIZE - poff);
-	will_compress = 1;
 
-cont:
 	/*
+	 * Try to create an inline extent.
+	 *
+	 * If we didn't compress the entire range, try to create an uncompressed
+	 * inline extent, else a compressed one.
+	 *
 	 * Check cow_file_range() for why we don't even try to create inline
 	 * extent for the subpage case.
 	 */
 	if (start == 0 && fs_info->sectorsize == PAGE_SIZE) {
-		/* lets try to make an inline extent */
-		if (ret || total_in < actual_end) {
-			/* we didn't compress the entire range, try
-			 * to make an uncompressed inline extent.
-			 */
-			ret = cow_file_range_inline(inode, actual_end,
-						    0, BTRFS_COMPRESS_NONE,
-						    NULL, false);
+		if (total_in < actual_end) {
+			ret = cow_file_range_inline(inode, actual_end, 0,
+						    BTRFS_COMPRESS_NONE, NULL,
+						    false);
 		} else {
-			/* try making a compressed inline extent */
 			ret = cow_file_range_inline(inode, actual_end,
 						    total_compressed,
 						    compress_type, pages,
@@ -1009,26 +1003,15 @@  static void compress_file_range(struct btrfs_work *work)
 						     PAGE_UNLOCK |
 						     PAGE_START_WRITEBACK |
 						     PAGE_END_WRITEBACK);
-
-			/*
-			 * Ensure we only free the compressed pages if we have
-			 * them allocated, as we can still reach here with
-			 * inode_need_compress() == false.
-			 */
-			if (pages) {
-				for (i = 0; i < nr_pages; i++) {
-					WARN_ON(pages[i]->mapping);
-					put_page(pages[i]);
-				}
-				kfree(pages);
+			for (i = 0; i < nr_pages; i++) {
+				WARN_ON(pages[i]->mapping);
+				put_page(pages[i]);
 			}
+			kfree(pages);
 			return;
 		}
 	}
 
-	if (!will_compress)
-		goto cleanup_and_bail_uncompressed;
-
 	/*
 	 * We aren't doing an inline extent. Round the compressed size up to a
 	 * block size boundary so the allocator does sane things.