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).
> 
> 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.
> 
> 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")
> 
> 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.

Just to mention, we can workaround the problem, by extracting the real 
work of btrfs_invalidate_folio() into a helper, with a bool parameter to 
indicate if we're called from the writeback path.

Then we would have a better chance to rever to 
folio_clear_dirty_for_io(), but there could be other things involved.
So I'm totally fine with the current hot fix, but eventually we will 
still revert back to folio_clear_dirty_for_io(), to align the code to iomap.

[...]
>   
> +#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);

My question is, why we need the call here?

Isn't folio_mkclean() called during extent_write_cache_pages() when 
we're writing back the folio?
Calling it here looks a little too early, as the folio may not even go 
through writeback later (e.g. being truncated).

Otherwise looks good to me.

Thanks,
Qu