Re: [PATCH] btrfs: write-protect folios during data writeback

在 2026/7/9 14:01, Boris Burkov 写道:
> Commit 095be159f3eb ("btrfs: unify folio dirty flag clearing") replaced
> the folio_clear_dirty_for_io() call in extent_write_cache_pages() with a
> plain folio_test_dirty() check. Besides clearing the dirty flag,
> folio_clear_dirty_for_io() also calls folio_mkclean(), which write-protects
> the shared mmap PTEs mapping the folio.
> 
> Without the write-protection, a process with the file mmap'd can modify
> a sector while it is being used by writeback in a way that expects a
> stable folio (checksumming, compressing, copying, etc...) which
> manifests as a handful of concrete bugs.
> 
> For large folios or subpage sectorsize, this will result in an invalid
> checksum and later corruption reports on read (if sectorsize ==
> folio_size() then we will always be in the case where we call
> folio_mkclean() when we clear the dirty on the sector, which is the
> whole folio).

Just want to make the cause more clear, and also explain under what 
exact situation the problem can happen.

Firstly, we still call folio_clear_dirty_for_io(), but at a much later 
timing, now it's only called when the last dirty block got its dirty 
flag cleared, instead of just before extent_writepage().

This means, if there is only a single contig dirty range inside a folio, 
and we can submit that contig range in one go, then the problem will not 
happen.

E.g. a 32K folio with 4K block size, and inside the folio the dirty 
range looks like this:

     0     8K     16K     24K    32K
     |     |//////////////|      |

Then during writeback, we clear the block dirty and mark writeback, 
queue that block into a bbio (but not yet submitted).

Finally at the last block [20K, 24K), we cleared the last dirty block, 
thus will call folio_clear_dirty_for_io() for the whole folio, then 
submit the bio for range [8K, 24K), and everything is still safe.

But another commit, "btrfs: limit size of bios submitted from 
writeback", is making the situation worse, as if we get a very large 
folio, e.g. 128KiB one, even if the whole range is dirty, it will still 
be submitted as two 64K bios, in that case, the first half will be 
submitted without folio_mkclean() called, thus causing the problems 
you're describing.


With all these said, this may help you to craft a more reliable 
reproducer, by:

1. Allocate a large folio (e.g, 64KiB on 4K ps systems)
    By doing a 64KiB sized buffered write at file offset 0.

2. Mmap range [4K, 8K) and [16K, 20K)

3. Trigger writeback with mmaped range being modified



> 
> For zoned submissions which are done in batch separate from the main
> extent_writepage() loop, we also risk csum violations for those
> submissions.
> 
> For inline extents this will subtly risk losing writes that happen
> after/while we copy the inline extent but before we clear dirty on
> the folio.

Inline is indeed affected, as we only rely on the final 
extent_clear_unlock_delalloc() -> process_one_folio() -> 
btrfs_folio_clamp_clear_dirty() to clear the block dirty, which is too late.

As the data is already copied into metadata without folio_mkclean().


> 
> For folios spanning EOF, mmap could tamper with the zeroed bytes past
> EOF and cause them to be persisted where future faults would impoperly
> see them instead of zeros.
> 
> Finally, for compressed extents, we risk modifying the folios while we
> work on compressing them which will result in corrupted compressed data.
> This particular gap was introduced by a second patch in the same series:
> Commit a4ef54dbb576 ("btrfs: make extent_range_clear_dirty_for_io() to handle sector size < page size cases")

Yes, compression corruption is still true, but only for the last/first 
folio.

Firstly compression range is always contig, only the first and last 
folio can have other dirty ranges out of the compression range.

So the middle folios will always have their folio dirty flags cleared by 
extent_range_clear_dirty_for_io() -> btrfs_folio_clamp_clear_dirty().

> 
> We cannot simply restore the call to folio_clear_dirty_for_io() because
> that also drops the dirty flag off the folio which violates invariants
> introduced for large folios by
> commit 334509ce9d07 ("btrfs: use dirty flag to check if an ordered extent needs to be truncated")
> and results in failing to invalidate clean folios past i_size, resulting
> in deadlocks.
> 
> Therefore, to fix it, leave the existing semantics w.r.t. the folio's
> dirty flag (to preserve the correct invalidate behavior) but ensure that
> the other aspect of folio_clear_dirty_for_io(), folio_mkclean(), is run
> on the folio when we lock it for writeback.
> 
> Finally, to help prevent similar regressions in the future, add a debug
> warning that triggers at the known corruption sites if we have failed to
> write protect the folio.
> 
> Assisted-by: LLM (debug, reproduce, research fix, review patch)
> Fixes: 095be159f3eb ("btrfs: unify folio dirty flag clearing")
> Fixes: a4ef54dbb576 ("btrfs: make extent_range_clear_dirty_for_io() to handle sector size < page size cases")
> Signed-off-by: Boris Burkov <[email protected]>
> ---
>   fs/btrfs/extent_io.c | 31 +++++++++++++++++++++++++++++++
>   fs/btrfs/extent_io.h |  5 +++++
>   fs/btrfs/inode.c     | 23 +++++++++++++++++------
>   3 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 8fbb798767ca..647b109ef61b 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -6,6 +6,7 @@
>   #include <linux/mm.h>
>   #include <linux/pagemap.h>
>   #include <linux/page-flags.h>
> +#include <linux/rmap.h>
>   #include <linux/sched/mm.h>
>   #include <linux/spinlock.h>
>   #include <linux/blkdev.h>
> @@ -299,6 +300,25 @@ static noinline void unlock_delalloc_folio(const struct inode *inode,
>   				PAGE_UNLOCK);
>   }
>   
> +#ifdef CONFIG_BTRFS_DEBUG
> +/*
> + * Writeback must write-protect a folio when locking it for IO, before
> + * anything consumes its data (zeroing, inline copy, compression,
> + * checksumming). If this fails, then an mmap writer would be able to
> + * modify the data concurrently while we need it to be stable.
> + */
> +void btrfs_check_folio_write_protected(struct folio *folio)
> +{
> +	if (folio_mkclean(folio)) {
> +		const struct btrfs_inode *inode = BTRFS_I(folio->mapping->host);
> +
> +		DEBUG_WARN("writable mmap PTEs, root %llu ino %llu pos %llu order %u",
> +			   btrfs_root_id(inode->root), btrfs_ino(inode), folio_pos(folio),
> +			   folio_order(folio));
> +	}
> +}
> +#endif
> +
>   static noinline int lock_delalloc_folios(struct inode *inode,
>   					 struct folio *locked_folio,
>   					 u64 start, u64 end)
> @@ -332,6 +352,8 @@ static noinline int lock_delalloc_folios(struct inode *inode,
>   				folio_unlock(folio);
>   				goto out;
>   			}
> +			/* Locked for writeback; revoke writable mmap PTEs before using the data. */
> +			folio_mkclean(folio);

I believe this is mostly for compression.

As inline folio is already cleaned by folio_mkclean() before 
extent_writepage().
The same for regular COW/NOCOW writes.

In that case, as I mentioned in another thread, I'd prefer not to call 
folio_mkclean() too early.

I'd prefer to call folio_mkclean() inside 
extent_range_clear_dirty_for_io() instead.
>   			range_start = max_t(u64, folio_pos(folio), start);
>   			range_len = min_t(u64, folio_next_pos(folio), end + 1) - range_start;
>   			btrfs_folio_set_lock(fs_info, folio, range_start, range_len);
> @@ -1780,6 +1802,13 @@ static noinline_for_stack int extent_writepage_io(struct btrfs_inode *inode,
>   	ASSERT(end <= folio_end, "start=%llu len=%u folio_start=%llu folio_size=%zu",
>   	       start, len, folio_start, folio_size(folio));
>   
> +	/*
> +	 * We are about to checksum and write out the data, so it must not be
> +	 * mmap writeable, or we could corrupt the data and end up with invalid
> +	 * checksums.
> +	 */
> +	btrfs_check_folio_write_protected(folio);
> +
>   	/* Truncate the submit bitmap to the current range. */
>   	if (start > folio_start)
>   		bitmap_clear(bio_ctrl->submit_bitmap, 0,
> @@ -2590,6 +2619,8 @@ static int extent_write_cache_pages(struct address_space *mapping,
>   				continue;
>   			}
>   
> +			/* Locked for writeback; revoke writable mmap PTEs before using the data. */
> +			folio_mkclean(folio);
>   			ret = extent_writepage(folio, bio_ctrl);
>   			if (ret < 0) {
>   				done = true;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 9896e15ddc40..869925337699 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -255,6 +255,11 @@ bool try_release_extent_mapping(struct folio *folio, gfp_t mask);
>   int try_release_extent_buffer(struct folio *folio);
>   
>   int btrfs_read_folio(struct file *file, struct folio *folio);
> +#ifdef CONFIG_BTRFS_DEBUG
> +void btrfs_check_folio_write_protected(struct folio *folio);
> +#else
> +static inline void btrfs_check_folio_write_protected(struct folio *folio) { }
> +#endif
>   void extent_write_locked_range(struct inode *inode, const struct folio *locked_folio,
>   			       u64 start, u64 end, struct writeback_control *wbc,
>   			       bool pages_dirty);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b47e2aa5071d..636196705fa3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -775,19 +775,28 @@ static inline void inode_should_defrag(struct btrfs_inode *inode,
>   
>   static int extent_range_clear_dirty_for_io(struct btrfs_inode *inode, u64 start, u64 end)
>   {
> +	pgoff_t index = start >> PAGE_SHIFT;
>   	const pgoff_t end_index = end >> PAGE_SHIFT;
>   	struct folio *folio;
>   	int ret = 0;
>   
> -	for (pgoff_t index = start >> PAGE_SHIFT; index <= end_index; index++) {
> +	while (index <= end_index) {
>   		folio = filemap_get_folio(inode->vfs_inode.i_mapping, index);
>   		if (IS_ERR(folio)) {
>   			if (!ret)
>   				ret = PTR_ERR(folio);
> +			index++;
>   			continue;
>   		}
> +		/*
> +		 * We are about to compress the folio, so it must not be mmap
> +		 * writeable or we could corrupt the data as we attempt to
> +		 * compress it.
> +		 */
> +		btrfs_check_folio_write_protected(folio);
>   		btrfs_folio_clamp_clear_dirty(inode->root->fs_info, folio, start,
>   					      end + 1 - start);
> +		index = folio_next_index(folio);
>   		folio_put(folio);
>   	}
>   	return ret;
> @@ -877,11 +886,6 @@ static void compress_file_range(struct btrfs_work *work)
>   
>   	inode_should_defrag(inode, start, end, end - start + 1, SZ_16K);
>   
> -	/*
> -	 * 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.
> -	 */

And I'm a total idiot not noticing this existing comment...

Otherwise it looks good to me.

Thanks a lot of pinning down a bug that I'm completely unfamiliar with.
Didn't notice folio_mkclean() has such an important role related to 
mmaped writes.

Thanks,
Qu

>   	ret = extent_range_clear_dirty_for_io(inode, start, end);
>   
>   	/*
> @@ -2317,6 +2321,13 @@ static int run_delalloc_inline(struct btrfs_inode *inode, struct folio *locked_f
>   	int ret;
>   
>   	ASSERT(folio_pos(locked_folio) == 0);
> +	/*
> +	 * If an mmap writer could modify the folio while we copy it into an
> +	 * inline extent we might see only part of their modification then
> +	 * wrongly mark it clean again after copying, losing that write. So the
> +	 * folio must be write protected here.
> +	 */
> +	btrfs_check_folio_write_protected(locked_folio);
>   
>   	if (btrfs_inode_can_compress(inode) &&
>   	    inode_need_compress(inode, 0, blocksize, true)) {