diff mbox series

[14/16] btrfs: don't redirty the locked page for extent_write_locked_range

Message ID 20230531060505.468704-15-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/16] btrfs: fix range_end calculation in extent_write_locked_range | expand

Commit Message

Christoph Hellwig May 31, 2023, 6:05 a.m. UTC 
Instead of redirtying the locked page before calling
extent_write_locked_range, just pass a locked_page argument similar to
many other functions in the btrfs writeback code, and then exclude the
locked page from clearing the dirty bit in extent_write_locked_range.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/extent_io.c | 17 ++++++++++-------
 fs/btrfs/extent_io.h |  3 ++-
 fs/btrfs/inode.c     | 25 ++++++-------------------
 3 files changed, 18 insertions(+), 27 deletions(-)

Comments

David Sterba June 5, 2023, 9 p.m. UTC | #1 
On Wed, May 31, 2023 at 08:05:03AM +0200, Christoph Hellwig wrote:
> Instead of redirtying the locked page before calling
> extent_write_locked_range, just pass a locked_page argument similar to
> many other functions in the btrfs writeback code, and then exclude the
> locked page from clearing the dirty bit in extent_write_locked_range.

I'm still not convinced this is safe, the changelog should be much
longer for this kind of change. I remember some bugs with redirtying
(e.g. 1d53c9e6723022b12e4a, 4adaa611020fa6ac65b0a). The code has evolved
since so I'd like to see more than "instead of this just do that" style
of changelog.
diff mbox series

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f4d3c56b29009b..bc061b2ac55a12 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2210,8 +2210,8 @@  static int extent_write_cache_pages(struct address_space *mapping,
  * already been ran (aka, ordered extent inserted) and all pages are still
  * locked.
  */
-int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
-			      struct writeback_control *wbc)
+int extent_write_locked_range(struct inode *inode, struct page *locked_page,
+			      u64 start, u64 end, struct writeback_control *wbc)
 {
 	bool found_error = false;
 	int first_error = 0;
@@ -2237,14 +2237,17 @@  int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
 		int nr = 0;
 
 		page = find_get_page(mapping, cur >> PAGE_SHIFT);
+
 		/*
-		 * All pages in the range are locked since
-		 * btrfs_run_delalloc_range(), thus there is no way to clear
-		 * the page dirty flag.
+		 * All pages have been locked by btrfs_run_delalloc_range(),
+		 * thus the dirty bit can't have been cleared.
 		 */
 		ASSERT(PageLocked(page));
-		ASSERT(PageDirty(page));
-		clear_page_dirty_for_io(page);
+		if (page != locked_page) {
+			/* already cleared by extent_write_cache_pages */
+			ASSERT(PageDirty(page));
+			clear_page_dirty_for_io(page);
+		}
 
 		ret = __extent_writepage_io(BTRFS_I(inode), page, &bio_ctrl,
 					    i_size, &nr);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index c5fae3a7d911bf..00c468aea010a1 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -178,7 +178,8 @@  int try_release_extent_mapping(struct page *page, gfp_t mask);
 int try_release_extent_buffer(struct page *page);
 
 int btrfs_read_folio(struct file *file, struct folio *folio);
-int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
+int extent_write_locked_range(struct inode *inode, struct page *locked_page,
+			      u64 start, u64 end,
 			      struct writeback_control *wbc);
 int extent_writepages(struct address_space *mapping,
 		      struct writeback_control *wbc);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 54b4b241b354fc..68ae20a3f785e3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1088,17 +1088,9 @@  static noinline int compress_file_range(struct async_chunk *async_chunk)
 cleanup_and_bail_uncompressed:
 	/*
 	 * No compression, but we still need to write the pages in the file
-	 * we've been given so far.  redirty the locked page if it corresponds
-	 * to our extent and set things up for the async work queue to run
-	 * cow_file_range to do the normal delalloc dance.
+	 * we've been given so far.  Set things up for the async work queue to
+	 * run cow_file_range to do the normal delalloc dance.
 	 */
-	if (async_chunk->locked_page &&
-	    (page_offset(async_chunk->locked_page) >= start &&
-	     page_offset(async_chunk->locked_page)) <= end) {
-		__set_page_dirty_nobuffers(async_chunk->locked_page);
-		/* unlocked later on in the async handlers */
-	}
-
 	if (redirty)
 		extent_range_redirty_for_io(&inode->vfs_inode, start, end);
 	add_async_extent(async_chunk, start, end - start + 1, 0, NULL, 0,
@@ -1169,7 +1161,8 @@  static int submit_uncompressed_range(struct btrfs_inode *inode,
 
 	/* All pages will be unlocked, including @locked_page */
 	wbc_attach_fdatawrite_inode(&wbc, &inode->vfs_inode);
-	ret = extent_write_locked_range(&inode->vfs_inode, start, end, &wbc);
+	ret = extent_write_locked_range(&inode->vfs_inode, locked_page, start,
+					end, &wbc);
 	wbc_detach_inode(&wbc);
 	return ret;
 }
@@ -1829,7 +1822,6 @@  static noinline int run_delalloc_zoned(struct btrfs_inode *inode,
 {
 	u64 done_offset = end;
 	int ret;
-	bool locked_page_done = false;
 
 	while (start <= end) {
 		ret = cow_file_range(inode, locked_page, start, end, page_started,
@@ -1852,13 +1844,8 @@  static noinline int run_delalloc_zoned(struct btrfs_inode *inode,
 			continue;
 		}
 
-		if (!locked_page_done) {
-			__set_page_dirty_nobuffers(locked_page);
-			account_page_redirty(locked_page);
-		}
-		locked_page_done = true;
-		extent_write_locked_range(&inode->vfs_inode, start, done_offset,
-					  wbc);
+		extent_write_locked_range(&inode->vfs_inode, locked_page, start,
+					  done_offset, wbc);
 		start = done_offset + 1;
 	}