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

On Thu, Jul 09, 2026 at 03:08:49PM +0930, Qu Wenruo wrote:
> 
> 
> 在 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).

The idea is:
1. call it early when you lock to not call it too late
2. it covers compression and zoned batch write in one unified spot with
   a describable invariant (locked for delalloc <-> cleaned) so its more
   future proof against future refactors or writing models.

This patch says v1 but on my git tree it's more like v6 :). Including
variants where I tried to do it last second at every relevant callsite
(basically where I put the asserts) if you want that instead, it's fine
with me, but I personally liked this the best.

> 
> Otherwise looks good to me.
> 
> Thanks,
> Qu