[PATCH] btrfs: Remove unnecessary inode_need_compress() check

[PATCH] btrfs: Remove unnecessary inode_need_compress() check

[Goldwyn Rodrigues]

async extent writes are called only for compressed range. So, remove the
check for inode_need_compress() in compress_file_range() since it is
already checked in btrfs_run_delalloc_range().

Conditions for inode_can_compress() can be incorporated in
inode_need_compress()

To make the code more elegant, change the condition order in
btrfs_run_delalloc_range().

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/inode.c | 126 ++++++++++++++++++++---------------------------
 1 file changed, 54 insertions(+), 72 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4dfc02fbbad5..a89768963d9f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -480,17 +480,6 @@ static noinline int add_async_extent(struct async_chunk *cow,
 	return 0;
 }
 
-/*
- * Check if the inode has flags compatible with compression
- */
-static inline bool inode_can_compress(struct btrfs_inode *inode)
-{
-	if (inode->flags & BTRFS_INODE_NODATACOW ||
-	    inode->flags & BTRFS_INODE_NODATASUM)
-		return false;
-	return true;
-}
-
 /*
  * Check if the inode needs to be submitted to compression, based on mount
  * options, defragmentation, properties or heuristics.
@@ -500,12 +489,13 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 
-	if (!inode_can_compress(inode)) {
-		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
-			KERN_ERR "BTRFS: unexpected compression for ino %llu\n",
-			btrfs_ino(inode));
+	/*
+	 * Check if the inode has flags compatible with compression
+	 */
+	if (inode->flags & BTRFS_INODE_NODATACOW ||
+	    inode->flags & BTRFS_INODE_NODATASUM)
 		return 0;
-	}
+
 	/*
 	 * Special check for subpage.
 	 *
@@ -661,62 +651,55 @@ static noinline int compress_file_range(struct async_chunk *async_chunk)
 	total_in = 0;
 	ret = 0;
 
-	/*
-	 * we do compression for mount -o compress and when the
-	 * inode has not been flagged as nocompress.  This flag can
-	 * change at any time if we discover bad compression ratios.
-	 */
-	if (inode_need_compress(BTRFS_I(inode), start, end)) {
-		WARN_ON(pages);
-		pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
-		if (!pages) {
-			/* just bail out to the uncompressed code */
-			nr_pages = 0;
-			goto cont;
-		}
+	WARN_ON(pages);
+	pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
+	if (!pages) {
+		/* just bail out to the uncompressed code */
+		nr_pages = 0;
+		goto cont;
+	}
 
-		if (BTRFS_I(inode)->defrag_compress)
-			compress_type = BTRFS_I(inode)->defrag_compress;
-		else if (BTRFS_I(inode)->prop_compress)
-			compress_type = BTRFS_I(inode)->prop_compress;
+	if (BTRFS_I(inode)->defrag_compress)
+		compress_type = BTRFS_I(inode)->defrag_compress;
+	else if (BTRFS_I(inode)->prop_compress)
+		compress_type = BTRFS_I(inode)->prop_compress;
 
-		/*
-		 * we need to call clear_page_dirty_for_io on each
-		 * page in the range.  Otherwise applications with the file
-		 * mmap'd can wander in and change the page contents while
-		 * we are compressing them.
-		 *
-		 * If the compression fails for any reason, we set the pages
-		 * dirty again later on.
-		 *
-		 * Note that the remaining part is redirtied, the start pointer
-		 * has moved, the end is the original one.
-		 */
-		if (!redirty) {
-			extent_range_clear_dirty_for_io(inode, start, end);
-			redirty = 1;
-		}
+	/*
+	 * we need to call clear_page_dirty_for_io on each
+	 * page in the range.  Otherwise applications with the file
+	 * mmap'd can wander in and change the page contents while
+	 * we are compressing them.
+	 *
+	 * If the compression fails for any reason, we set the pages
+	 * dirty again later on.
+	 *
+	 * Note that the remaining part is redirtied, the start pointer
+	 * has moved, the end is the original one.
+	 */
+	if (!redirty) {
+		extent_range_clear_dirty_for_io(inode, start, end);
+		redirty = 1;
+	}
 
-		/* Compression level is applied here and only here */
-		ret = btrfs_compress_pages(
+	/* Compression level is applied here and only here */
+	ret = btrfs_compress_pages(
 			compress_type | (fs_info->compress_level << 4),
-					   inode->i_mapping, start,
-					   pages,
-					   &nr_pages,
-					   &total_in,
-					   &total_compressed);
+			inode->i_mapping, start,
+			pages,
+			&nr_pages,
+			&total_in,
+			&total_compressed);
 
-		if (!ret) {
-			unsigned long offset = offset_in_page(total_compressed);
-			struct page *page = pages[nr_pages - 1];
+	if (!ret) {
+		unsigned long offset = offset_in_page(total_compressed);
+		struct page *page = pages[nr_pages - 1];
 
-			/* zero the tail end of the last page, we might be
-			 * sending it down to disk
-			 */
-			if (offset)
-				memzero_page(page, offset, PAGE_SIZE - offset);
-			will_compress = 1;
-		}
+		/* zero the tail end of the last page, we might be
+		 * sending it down to disk
+		 */
+		if (offset)
+			memzero_page(page, offset, PAGE_SIZE - offset);
+		will_compress = 1;
 	}
 cont:
 	/*
@@ -2019,18 +2002,17 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
 		ASSERT(!zoned || btrfs_is_data_reloc_root(inode->root));
 		ret = run_delalloc_nocow(inode, locked_page, start, end,
 					 page_started, nr_written);
-	} else if (!inode_can_compress(inode) ||
-		   !inode_need_compress(inode, start, end)) {
+	} else if (inode_need_compress(inode, start, end)) {
+		set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags);
+		ret = cow_file_range_async(inode, wbc, locked_page, start, end,
+					   page_started, nr_written);
+	} else {
 		if (zoned)
 			ret = run_delalloc_zoned(inode, locked_page, start, end,
 						 page_started, nr_written);
 		else
 			ret = cow_file_range(inode, locked_page, start, end,
 					     page_started, nr_written, 1);
-	} else {
-		set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags);
-		ret = cow_file_range_async(inode, wbc, locked_page, start, end,
-					   page_started, nr_written);
 	}
 	ASSERT(ret <= 0);
 	if (ret)
-- 
2.35.3


-- 
Goldwyn

Re: [PATCH] btrfs: Remove unnecessary inode_need_compress() check

[Goldwyn Rodrigues]

Apparently, BTRFS_INODE_NOCOMPRESS can change runtime, so this patch is
incorrect. Sorry.

On 15:07 28/04, Goldwyn Rodrigues wrote:
> async extent writes are called only for compressed range. So, remove the
> check for inode_need_compress() in compress_file_range() since it is
> already checked in btrfs_run_delalloc_range().
> 
> Conditions for inode_can_compress() can be incorporated in
> inode_need_compress()
> 
> To make the code more elegant, change the condition order in
> btrfs_run_delalloc_range().
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/inode.c | 126 ++++++++++++++++++++---------------------------
>  1 file changed, 54 insertions(+), 72 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 4dfc02fbbad5..a89768963d9f 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -480,17 +480,6 @@ static noinline int add_async_extent(struct async_chunk *cow,
>  	return 0;
>  }
>  
> -/*
> - * Check if the inode has flags compatible with compression
> - */
> -static inline bool inode_can_compress(struct btrfs_inode *inode)
> -{
> -	if (inode->flags & BTRFS_INODE_NODATACOW ||
> -	    inode->flags & BTRFS_INODE_NODATASUM)
> -		return false;
> -	return true;
> -}
> -
>  /*
>   * Check if the inode needs to be submitted to compression, based on mount
>   * options, defragmentation, properties or heuristics.
> @@ -500,12 +489,13 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
>  {
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>  
> -	if (!inode_can_compress(inode)) {
> -		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
> -			KERN_ERR "BTRFS: unexpected compression for ino %llu\n",
> -			btrfs_ino(inode));
> +	/*
> +	 * Check if the inode has flags compatible with compression
> +	 */
> +	if (inode->flags & BTRFS_INODE_NODATACOW ||
> +	    inode->flags & BTRFS_INODE_NODATASUM)
>  		return 0;
> -	}
> +
>  	/*
>  	 * Special check for subpage.
>  	 *
> @@ -661,62 +651,55 @@ static noinline int compress_file_range(struct async_chunk *async_chunk)
>  	total_in = 0;
>  	ret = 0;
>  
> -	/*
> -	 * we do compression for mount -o compress and when the
> -	 * inode has not been flagged as nocompress.  This flag can
> -	 * change at any time if we discover bad compression ratios.
> -	 */
> -	if (inode_need_compress(BTRFS_I(inode), start, end)) {
> -		WARN_ON(pages);
> -		pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
> -		if (!pages) {
> -			/* just bail out to the uncompressed code */
> -			nr_pages = 0;
> -			goto cont;
> -		}
> +	WARN_ON(pages);
> +	pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
> +	if (!pages) {
> +		/* just bail out to the uncompressed code */
> +		nr_pages = 0;
> +		goto cont;
> +	}
>  
> -		if (BTRFS_I(inode)->defrag_compress)
> -			compress_type = BTRFS_I(inode)->defrag_compress;
> -		else if (BTRFS_I(inode)->prop_compress)
> -			compress_type = BTRFS_I(inode)->prop_compress;
> +	if (BTRFS_I(inode)->defrag_compress)
> +		compress_type = BTRFS_I(inode)->defrag_compress;
> +	else if (BTRFS_I(inode)->prop_compress)
> +		compress_type = BTRFS_I(inode)->prop_compress;
>  
> -		/*
> -		 * we need to call clear_page_dirty_for_io on each
> -		 * page in the range.  Otherwise applications with the file
> -		 * mmap'd can wander in and change the page contents while
> -		 * we are compressing them.
> -		 *
> -		 * If the compression fails for any reason, we set the pages
> -		 * dirty again later on.
> -		 *
> -		 * Note that the remaining part is redirtied, the start pointer
> -		 * has moved, the end is the original one.
> -		 */
> -		if (!redirty) {
> -			extent_range_clear_dirty_for_io(inode, start, end);
> -			redirty = 1;
> -		}
> +	/*
> +	 * we need to call clear_page_dirty_for_io on each
> +	 * page in the range.  Otherwise applications with the file
> +	 * mmap'd can wander in and change the page contents while
> +	 * we are compressing them.
> +	 *
> +	 * If the compression fails for any reason, we set the pages
> +	 * dirty again later on.
> +	 *
> +	 * Note that the remaining part is redirtied, the start pointer
> +	 * has moved, the end is the original one.
> +	 */
> +	if (!redirty) {
> +		extent_range_clear_dirty_for_io(inode, start, end);
> +		redirty = 1;
> +	}
>  
> -		/* Compression level is applied here and only here */
> -		ret = btrfs_compress_pages(
> +	/* Compression level is applied here and only here */
> +	ret = btrfs_compress_pages(
>  			compress_type | (fs_info->compress_level << 4),
> -					   inode->i_mapping, start,
> -					   pages,
> -					   &nr_pages,
> -					   &total_in,
> -					   &total_compressed);
> +			inode->i_mapping, start,
> +			pages,
> +			&nr_pages,
> +			&total_in,
> +			&total_compressed);
>  
> -		if (!ret) {
> -			unsigned long offset = offset_in_page(total_compressed);
> -			struct page *page = pages[nr_pages - 1];
> +	if (!ret) {
> +		unsigned long offset = offset_in_page(total_compressed);
> +		struct page *page = pages[nr_pages - 1];
>  
> -			/* zero the tail end of the last page, we might be
> -			 * sending it down to disk
> -			 */
> -			if (offset)
> -				memzero_page(page, offset, PAGE_SIZE - offset);
> -			will_compress = 1;
> -		}
> +		/* zero the tail end of the last page, we might be
> +		 * sending it down to disk
> +		 */
> +		if (offset)
> +			memzero_page(page, offset, PAGE_SIZE - offset);
> +		will_compress = 1;
>  	}
>  cont:
>  	/*
> @@ -2019,18 +2002,17 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
>  		ASSERT(!zoned || btrfs_is_data_reloc_root(inode->root));
>  		ret = run_delalloc_nocow(inode, locked_page, start, end,
>  					 page_started, nr_written);
> -	} else if (!inode_can_compress(inode) ||
> -		   !inode_need_compress(inode, start, end)) {
> +	} else if (inode_need_compress(inode, start, end)) {
> +		set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags);
> +		ret = cow_file_range_async(inode, wbc, locked_page, start, end,
> +					   page_started, nr_written);
> +	} else {
>  		if (zoned)
>  			ret = run_delalloc_zoned(inode, locked_page, start, end,
>  						 page_started, nr_written);
>  		else
>  			ret = cow_file_range(inode, locked_page, start, end,
>  					     page_started, nr_written, 1);
> -	} else {
> -		set_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, &inode->runtime_flags);
> -		ret = cow_file_range_async(inode, wbc, locked_page, start, end,
> -					   page_started, nr_written);
>  	}
>  	ASSERT(ret <= 0);
>  	if (ret)
> -- 
> 2.35.3
> 
> 
> -- 
> Goldwyn

-- 
Goldwyn

Re: [PATCH] btrfs: Remove unnecessary inode_need_compress() check

[Nikolay Borisov]

On 29.04.22 г. 17:41 ч., Goldwyn Rodrigues wrote:
> Apparently, BTRFS_INODE_NOCOMPRESS can change runtime, so this patch is
> incorrect. Sorry.
> 

Yes, when doing writes on compressed-enabled filesystem if a particular 
file gets bad compression ratio that flag is getting set from 
compress_file_range:


   /* flag the file so we don't compress in the future */
      32                 if (!btrfs_test_opt(fs_info, FORCE_COMPRESS) && 

      31                     !(BTRFS_I(inode)->prop_compress)) { 

      30                         BTRFS_I(inode)->flags |= 
BTRFS_INODE_NOCOMPRESS;
      29                 }