[PATCH v3 0/2] btrfs: enhance function extent_range_clear_dirty_for_io()

[PATCH v3 0/2] btrfs: enhance function extent_range_clear_dirty_for_io()

[Qu Wenruo]

[Changelog]
v3:
- Drop the patch to use subpage helper
  For subpage cases, fsstress with compression can lead to hang where
  OE seems hanging and never to be finished.
  So far it looks like some race with i_size change but still not sure
  why the code change is involved.
  Drop the subpage helper change for now.

v2:
- Split the original patch into 3

- Return the error from filemap_get_folio() to be future-proof

- Enhance the comments for the new ASSERT() on
  extent_range_clear_dirty_for_io() error
  In fact, even if some pages are missing, we do not need to handle the
  error at compress_file_range(), as btrfs_compress_folios() and each
  compression routine would handle the missing folio correctly.

  Thus the new ASSERT() is only an early warning for developers.

Qu Wenruo (2):
  btrfs: move extent_range_clear_dirty_for_io() into inode.c
  btrfs: remove the BUG_ON() inside extent_range_clear_dirty_for_io()

 fs/btrfs/extent_io.c | 15 ---------------
 fs/btrfs/extent_io.h |  1 -
 fs/btrfs/inode.c     | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 30 insertions(+), 17 deletions(-)

-- 
2.45.1

[PATCH v3 1/2] btrfs: move extent_range_clear_dirty_for_io() into inode.c

[Qu Wenruo]

The function is only utilized inside inode.c by compress_file_range(),
so move it to inode.c and unexport it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 15 ---------------
 fs/btrfs/extent_io.h |  1 -
 fs/btrfs/inode.c     | 15 +++++++++++++++
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7275bd919a3e..4af16c09dd88 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -164,21 +164,6 @@ void __cold extent_buffer_free_cachep(void)
 	kmem_cache_destroy(extent_buffer_cache);
 }
 
-void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
-{
-	unsigned long index = start >> PAGE_SHIFT;
-	unsigned long end_index = end >> PAGE_SHIFT;
-	struct page *page;
-
-	while (index <= end_index) {
-		page = find_get_page(inode->i_mapping, index);
-		BUG_ON(!page); /* Pages should be in the extent_io_tree */
-		clear_page_dirty_for_io(page);
-		put_page(page);
-		index++;
-	}
-}
-
 static void process_one_page(struct btrfs_fs_info *fs_info,
 			     struct page *page, struct page *locked_page,
 			     unsigned long page_ops, u64 start, u64 end)
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index dca6b12769ec..7c2f1bbc6b67 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -350,7 +350,6 @@ void extent_buffer_bitmap_clear(const struct extent_buffer *eb,
 void set_extent_buffer_dirty(struct extent_buffer *eb);
 void set_extent_buffer_uptodate(struct extent_buffer *eb);
 void clear_extent_buffer_uptodate(struct extent_buffer *eb);
-void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end);
 void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
 				  struct page *locked_page,
 				  struct extent_state **cached,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 000809e16aba..99be256f4f0e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -890,6 +890,21 @@ static inline void inode_should_defrag(struct btrfs_inode *inode,
 		btrfs_add_inode_defrag(NULL, inode, small_write);
 }
 
+static void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
+{
+	unsigned long index = start >> PAGE_SHIFT;
+	unsigned long end_index = end >> PAGE_SHIFT;
+	struct page *page;
+
+	while (index <= end_index) {
+		page = find_get_page(inode->i_mapping, index);
+		BUG_ON(!page); /* Pages should be in the extent_io_tree */
+		clear_page_dirty_for_io(page);
+		put_page(page);
+		index++;
+	}
+}
+
 /*
  * Work queue call back to started compression on a file and pages.
  *
-- 
2.45.1

[PATCH v3 2/2] btrfs: remove the BUG_ON() inside extent_range_clear_dirty_for_io()

[Qu Wenruo]

Previously we have BUG_ON() inside extent_range_clear_dirty_for_io(), as
we expect all involved folios are still locked, thus no folio should be
missing.

However for extent_range_clear_dirty_for_io() itself, we can skip the
missing folio and handling the remaining ones, and return an error if
there is anything wrong.

So this patch would remove the BUG_ON() and let the caller to handle the
error.
In the caller we do not have a quick way to cleanup the error, but all
the compression routines would handle the missing folio as an error and
properly error out, so we only need to do an ASSERT() for developers,
meanwhile for non-debug build the compression routine would handle the
error correctly.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 99be256f4f0e..126457236427 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -890,19 +890,24 @@ static inline void inode_should_defrag(struct btrfs_inode *inode,
 		btrfs_add_inode_defrag(NULL, inode, small_write);
 }
 
-static void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
+static int extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
 {
-	unsigned long index = start >> PAGE_SHIFT;
 	unsigned long end_index = end >> PAGE_SHIFT;
 	struct page *page;
+	int ret = 0;
 
-	while (index <= end_index) {
+	for (unsigned long index = start >> PAGE_SHIFT;
+	     index <= end_index; index++) {
 		page = find_get_page(inode->i_mapping, index);
-		BUG_ON(!page); /* Pages should be in the extent_io_tree */
+		if (unlikely(!page)) {
+			if (!ret)
+				ret = -ENOENT;
+			continue;
+		}
 		clear_page_dirty_for_io(page);
 		put_page(page);
-		index++;
 	}
+	return ret;
 }
 
 /*
@@ -946,7 +951,16 @@ static void compress_file_range(struct btrfs_work *work)
 	 * Otherwise applications with the file mmap'd can wander in and change
 	 * the page contents while we are compressing them.
 	 */
-	extent_range_clear_dirty_for_io(&inode->vfs_inode, start, end);
+	ret = extent_range_clear_dirty_for_io(&inode->vfs_inode, start, end);
+
+	/*
+	 * All the folios should have been locked thus no failure.
+	 *
+	 * And even some folios are missing, btrfs_compress_folios()
+	 * would handle them correctly, so here just do an ASSERT() check for
+	 * early logic errors.
+	 */
+	ASSERT(ret == 0);
 
 	/*
 	 * We need to save i_size before now because it could change in between
-- 
2.45.1

Re: [PATCH v3 0/2] btrfs: enhance function extent_range_clear_dirty_for_io()

[David Sterba]

On Thu, May 23, 2024 at 10:49:36AM +0930, Qu Wenruo wrote:
> [Changelog]
> v3:
> - Drop the patch to use subpage helper
>   For subpage cases, fsstress with compression can lead to hang where
>   OE seems hanging and never to be finished.
>   So far it looks like some race with i_size change but still not sure
>   why the code change is involved.
>   Drop the subpage helper change for now.
> 
> v2:
> - Split the original patch into 3
> 
> - Return the error from filemap_get_folio() to be future-proof
> 
> - Enhance the comments for the new ASSERT() on
>   extent_range_clear_dirty_for_io() error
>   In fact, even if some pages are missing, we do not need to handle the
>   error at compress_file_range(), as btrfs_compress_folios() and each
>   compression routine would handle the missing folio correctly.
> 
>   Thus the new ASSERT() is only an early warning for developers.
> 
> Qu Wenruo (2):
>   btrfs: move extent_range_clear_dirty_for_io() into inode.c
>   btrfs: remove the BUG_ON() inside extent_range_clear_dirty_for_io()

Reviewed-by: David Sterba <dsterba@suse.com>