Re: [RFC PATCH] btrfs: trigger cow fixup via dirty_folio()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/7/25 15:54, Boris Burkov 写道:
[...]>
> Link: https://lore.kernel.org/linux-btrfs/20260721191152.101118-1-borntraeger@linux.ibm.com/
> Signed-off-by: Boris Burkov <boris@bur.io>
> Assisted-by: LLM

Without LLM it's very hard to come up with such huge change in such a 
short time, especially considering this fix is a little urgent due to 
both large folio and removal of cow fixup are in the same release.

So I'm totally fine with the involvement of LLM, just we will spend 
quite some time reworking/removing a lot of comments/commit message.

Not because they are not not grammarly sound (unlike my replies), but a 
lot of time they are too lengthy but brings not too much value compared 
to the number of lines.

But still, very impressed to see a working RFC this soon.

> ---
>   fs/btrfs/btrfs_inode.h       |   1 +
>   fs/btrfs/defrag.c            |   1 +
>   fs/btrfs/disk-io.c           |   6 +-
>   fs/btrfs/extent_io.c         | 127 +++++++++++++++++
>   fs/btrfs/file.c              |   2 +
>   fs/btrfs/fs.h                |  20 +++
>   fs/btrfs/inode.c             | 262 ++++++++++++++++++++++++++++++++++-
>   fs/btrfs/reflink.c           |   1 +
>   fs/btrfs/relocation.c        |   1 +
>   fs/btrfs/subpage.c           |  93 +++++++++++++
>   fs/btrfs/subpage.h           |  43 +++++-
>   include/trace/events/btrfs.h |  35 +++++
>   12 files changed, 585 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 7fdc6c3fd066..037445586b31 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -600,6 +600,7 @@ int btrfs_prealloc_file_range_trans(struct inode *inode,
>   				    loff_t actual_len, u64 *alloc_hint);
>   int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct folio *locked_folio,
>   			     u64 start, u64 end, struct writeback_control *wbc);
> +int btrfs_queue_writepage_fixup(struct btrfs_inode *inode, struct folio *folio);
>   int btrfs_encoded_io_compression_from_extent(struct btrfs_fs_info *fs_info,
>   					     int compress_type);
>   int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
> index 6ec5dd760d42..7de5febee9d1 100644
> --- a/fs/btrfs/defrag.c
> +++ b/fs/btrfs/defrag.c
> @@ -1158,6 +1158,7 @@ static void defrag_one_locked_target(struct btrfs_inode *inode,
>   		    start + len <= folio_pos(folio))
>   			continue;
>   		btrfs_folio_clamp_set_dirty(fs_info, folio, start, len);
> +		btrfs_folio_cancel_fixup(fs_info, folio, start, len);

This call is very frequent, and to be honest, it's redundant.

We can merge the cancel into btrfs_folio*_set_dirty(), as the 
btrfs_folio_*set_dirty() is the "proper dirty" entrance, so we know if 
we're going through that interface, we are ensured to cancel the fixup.

> +
> +	if (!folio_test_fixup_pending(folio))

if (likely())

> +		return 0;
> +
> +	for_each_set_bit(bit, bio_ctrl->submit_bitmap, blocks_per_folio) {
> +		const u64 start = page_start + (bit << fs_info->sectorsize_bits);
> +
> +		if (btrfs_test_range_bit_exists(&inode->io_tree, start,
> +						start + sectorsize - 1,
> +						EXTENT_DELALLOC))

Can we simplify the check to just check if the fixup bit is not set?

To me, without fixup bit it means it went through the regular write 
preparation, thus should have EXTENT_DELALLOC.
And checking a bit is way faster than check an extent range.

> +			continue;
> +		if (btrfs_is_subpage(fs_info, folio) &&
> +		    !btrfs_subpage_test_fixup(fs_info, folio, start, sectorsize)) {

Again, we prefer not to use btrfs_is_subpage() and btrfs_subpage_*() out 
of subpage.c when possible.

So just some helper, like btrfs_folio_test_fixup() would make it more 
readable.

> +			struct btrfs_ordered_extent *ordered;
> +
> +			/*
> +			 * Not a suspect: the only legal explanation for a
> +			 * dirty, unfunded block is that it is pending its own
> +			 * submission into a covering ordered extent - punting
> +			 * it would starve that ordered extent forever, and
> +			 * the worker could not see it anyway (its work list
> +			 * is the fixup bits). Verify the ordered extent
> +			 * exists; if not, the invariant is broken - leave the
> +			 * block in the bitmap so submission fails loudly
> +			 * (EUCLEAN) instead of silently repairing.
> +			 */

The block can be dirtied properly before, then ->dirty_folio() callback 
is triggered.

In that case, we may have not run delalloc for the folio, thus there may 
be no OE created yet.

It looks like the assumption is not correct to me, and may cause false 
alerts.

But on the other hand, it's already very hard to hit a case where we 
have some proper dirty blocks, and ->dirty_folio() is called.
E.g. for the direct IO case, ->dirty_folio() is only called when the 
folio is clean.

Thus this false alerts case is not easy to test.

> +			ordered = btrfs_lookup_ordered_range(inode, start,
> +							     sectorsize);

Doing this is a little overkilled.

As the recent patch "btrfs: use ordered extent to grab the logical 
address for submission" will do it at submission time, with better 
cached OE usage.

I'd prefer skip the OE lookup completely, and let the final submission 
path to grab the OE (which can reuse bbio->ordered when possible).
> +			if (unlikely(!ordered))
> +				DEBUG_WARN();
> +			else
> +				btrfs_put_ordered_extent(ordered);
> +			continue;
> +		}
> +		/*
> +		 * Block re-dirtied via dirty_folio(), requires async fixup.
> +		 * All other blocks in the folio should proceed as normal so
> +		 * that their OEs can make progress and complete.
> +		 */
> +		bitmap_clear(bio_ctrl->submit_bitmap, bit, 1);
> +		punt = true;

The variable naming choice is definitely very LLM.

Non-native speaker like me may need to google the word first...

> +	}
> +	if (punt) {
> +		btrfs_queue_writepage_fixup(inode, folio);
> +		/*
> +		 * Keep the folio dirty for the punted blocks.  If nothing
> +		 * is left to submit this pass, the folio is done and we own
> +		 * the unlock, matching the async-submission contract.
> +		 */
> +		folio_redirty_for_writepage(bio_ctrl->wbc, folio);

We haven't yet cleared any bit nor folio dirty in the first place.
So not sure if this is really needed, nor the comment.

> +		if (bitmap_empty(bio_ctrl->submit_bitmap, blocks_per_folio)) {
> +			folio_unlock(folio);
> +			return 1;
> +		}
> +		return 0;
> +	}
> +	/* Every dirty block is now covered; the folio is ordinary again. */
> +	if (btrfs_is_subpage(fs_info, folio))
> +		btrfs_subpage_clear_fixup(fs_info, folio, page_start,
> +					  folio_size(folio));

Again, a btrfs_folio_clear_fixup() is more helpful.

[...]
> +/*
> + * The folio has (or, for single-block folios, is) dirty blocks that were
> + * dirtied without notifying the filesystem (e.g. set_page_dirty_lock() on a
> + * GUP pin) and carry no reservation.  Set by btrfs_data_dirty_folio(), which
> + * records the affected blocks in the subpage fixup bitmap; cleared when the
> + * writepage fixup has covered every dirty block, or by a reserving write
> + * that covers the whole folio.  Reuses the PG_owner_2 bit that the folio
> + * ordered flag occupied before it was removed.

Again, very LLM comments. That's why always delete all comments from LLM 
by default.

The older, removed comment is very simple for folio_ordered:

  * We use folio flag owner_2 to indicate there is an ordered extent with
  * unfinished IO.


[...]
> +static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
> +{
> +	struct btrfs_writepage_fixup *fixup =
> +		container_of(work, struct btrfs_writepage_fixup, work);
> +	struct extent_state *cached_state = NULL;
> +	struct extent_changeset *data_reserved = NULL;
> +	unsigned long funded[BITS_TO_LONGS(BTRFS_MAX_BLOCKS_PER_FOLIO)] = { 0 };
> +	struct folio *folio = fixup->folio;
> +	struct btrfs_inode *inode = fixup->inode;
> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +	const unsigned int blocks_per_folio = btrfs_blocks_per_folio(fs_info, folio);
> +	const u32 sectorsize = fs_info->sectorsize;
> +	const u64 page_start = folio_pos(folio);
> +	const u64 page_end = folio_next_pos(folio) - 1;
> +	unsigned int start_bit;
> +	unsigned int end_bit;
> +	unsigned int bit;
> +	bool reserved;
> +	int ret;
> +
> +	/*
> +	 * Reserve for the whole folio before taking the folio lock, like
> +	 * page_mkwrite: the reservation may flush, and flushing writeback
> +	 * takes folio locks.  Which blocks actually need funding is only
> +	 * knowable under the lock, so reserve the worst case and release
> +	 * the unused ranges once the funded set is known.
> +	 */
> +	ret = btrfs_delalloc_reserve_space(inode, &data_reserved, page_start,
> +					   folio_size(folio));

You fall into the same pitfall I'm worried about, but unfortunately or 
fortunately, it's very hard to hit.

The problem here is, we can over-reserve, e.g. some range is already 
properly dirtied, thus they have already space reserved.

And you can not release the reserved space for them, because release 
will also clear the qgroup reserved extent flag.

This is the biggest problem that I have no way to fix.
As there is no way to ensure the space we reserved is really needed 
before holding the folio lock.


But since it's really hard to get a dirty folio to be called with 
->dirty_folio(), thus it may not be exposed through tests.

> +	reserved = (ret == 0);
> +again:
> +	folio_lock(folio);
> +
> +	/*
> +	 * The queueing writeback pass took references on the folio and the
> +	 * inode, but nothing else is stable: an in-band write may have funded
> +	 * the blocks and cancelled the fixup state, or the folio may have
> +	 * been invalidated.  Then there is nothing left to do - and a failed
> +	 * reservation against a folio that no longer needs one is no error.
> +	 */
> +	if (!folio->mapping || !folio_test_dirty(folio) ||
> +	    !folio_test_fixup_pending(folio)) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	/* The folio still needs funding we could not get. */
> +	if (ret)
> +		goto out_error;
> +
> +	btrfs_lock_extent(&inode->io_tree, page_start, page_end, &cached_state);
> +
> +	for (bit = 0; bit < blocks_per_folio; bit++) {
> +		struct btrfs_ordered_extent *ordered;
> +		const u64 start = page_start + (bit << fs_info->sectorsize_bits);
> +
> +		if (test_bit(bit, funded))
> +			continue;
> +		if (btrfs_is_subpage(fs_info, folio) &&
> +		    !btrfs_subpage_test_fixup(fs_info, folio, start, sectorsize))
> +			continue;
> +		/* An in-band write may have funded the block while we waited. */
> +		if (btrfs_test_range_bit_exists(&inode->io_tree, start,
> +						start + sectorsize - 1,
> +						EXTENT_DELALLOC))
> +			continue;
> +		ordered = btrfs_lookup_ordered_range(inode, start, sectorsize);
> +		if (ordered) {
> +			trace_btrfs_writepage_fixup_defer(inode, ordered);
> +			btrfs_unlock_extent(&inode->io_tree, page_start,
> +					    page_end, &cached_state);
> +			folio_unlock(folio);
> +			btrfs_start_ordered_extent(ordered);
> +			btrfs_put_ordered_extent(ordered);
> +			goto again;
> +		}
> +		ret = btrfs_set_extent_delalloc(inode, start,
> +						start + sectorsize - 1, 0,
> +						&cached_state);
> +		if (ret) {
> +			btrfs_unlock_extent(&inode->io_tree, page_start,
> +					    page_end, &cached_state);
> +			goto out_error;
> +		}
> +		trace_btrfs_writepage_fixup_reserve(inode, start, sectorsize);
> +		if (btrfs_is_subpage(fs_info, folio))
> +			btrfs_subpage_clear_fixup(fs_info, folio, start,
> +						  sectorsize);
> +		__set_bit(bit, funded);

Why __set_bit() not set_bit()?

> +	}
> +
> +	/* Every fixup block is funded; the folio is ordinary again. */
> +	folio_clear_fixup_pending(folio);
> +	btrfs_unlock_extent(&inode->io_tree, page_start, page_end, &cached_state);
> +	goto out;
> +
> +out_error:
> +	/*
> +	 * The remaining unnotified blocks cannot be funded.  There is no
> +	 * syscall to return this through, so surface it the way a failed
> +	 * writeback would - record the error on the mapping - and drop the
> +	 * blocks, or writeback would find and requeue them forever.
> +	 */
> +	mapping_set_error(folio->mapping, ret);
> +	if (btrfs_is_subpage(fs_info, folio)) {
> +		bool last = false;
> +
> +		for (bit = 0; bit < blocks_per_folio; bit++) {

Again, a proper btrfs_folio_clear_fixup() and pass the folio range into 
it should handle the loop.

> +			const u64 start = page_start +
> +					  (bit << fs_info->sectorsize_bits);
> +
> +			if (!btrfs_subpage_test_fixup(fs_info, folio, start,
> +						      sectorsize))
> +				continue;
> +			last = btrfs_subpage_clear_and_test_dirty(fs_info,
> +							folio, start,
> +							sectorsize);
> +		}
> +		btrfs_folio_cancel_fixup(fs_info, folio, page_start,
> +					 folio_size(folio));
> +		if (last)
> +			folio_clear_dirty_for_io(folio);

Again, the regular btrfs_folio_clear_dirty() is going to handle all 
these, and no need for a special subpage branch.

> +	} else {
> +		folio_clear_dirty_for_io(folio);
> +	}
> +	folio_clear_fixup_pending(folio);
> +out:
> +	if (reserved) {
> +		btrfs_delalloc_release_extents(inode, folio_size(folio));
> +		for_each_clear_bitrange(start_bit, end_bit, funded,
> +					blocks_per_folio)
> +			btrfs_delalloc_release_space(inode, data_reserved,
> +				page_start + (start_bit << fs_info->sectorsize_bits),
> +				(end_bit - start_bit) << fs_info->sectorsize_bits,
> +				true);
> +	}
> +	folio_unlock(folio);
> +	folio_put(folio);
> +	kfree(fixup);
> +	extent_changeset_free(data_reserved);
> +	/*
> +	 * As a precaution, do a delayed iput in case it would be the last
> +	 * iput that could need flushing space. Recursing back to the fixup
> +	 * worker would deadlock.
> +	 */
> +	btrfs_add_delayed_iput(inode);
> +}
[...]
> +static bool btrfs_data_dirty_folio(struct address_space *mapping,
> +				   struct folio *folio)
> +{
> +	struct btrfs_inode *inode = BTRFS_I(mapping->host);
> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +	bool marked = false;
> +
> +	if (btrfs_is_subpage(fs_info, folio)) {
> +		const struct btrfs_folio_state *bfs = folio_get_private(folio);
> +		const u64 page_start = folio_pos(folio);
> +		const u64 range_end = min_t(u64, page_start + folio_size(folio),
> +					    round_up(i_size_read(&inode->vfs_inode),
> +						     fs_info->sectorsize));
> +
> +		if (!READ_ONCE(bfs->reserving_dirty) &&
> +		    folio_test_uptodate(folio) && range_end > page_start)
> +			marked = btrfs_subpage_set_fixup_dirty(fs_info, folio,
> +							       page_start,
> +							       range_end - page_start);
> +	} else if (!folio_test_dirty(folio) &&
> +		   !btrfs_test_range_bit_exists(&inode->io_tree,
> +						folio_pos(folio),
> +						folio_next_pos(folio) - 1,
> +						EXTENT_DELALLOC)) {

Again, the same question why the folio_test_dirty() check is not enough.

The subpage part has no such check.

> +		marked = true;
> +	}
> +	if (marked)
> +		folio_set_fixup_pending(folio);
> +	return filemap_dirty_folio(mapping, folio);
> +}
> +
[..]
> --- a/fs/btrfs/subpage.c
> +++ b/fs/btrfs/subpage.c
> @@ -356,7 +356,17 @@ void btrfs_subpage_set_dirty(const struct btrfs_fs_info *fs_info,
>   	spin_lock_irqsave(&bfs->lock, flags);
>   	bitmap_set(bfs->bitmaps, start_bit, len >> fs_info->sectorsize_bits);
>   	spin_unlock_irqrestore(&bfs->lock, flags);
> +	/*
> +	 * Every caller of this helper is a reserving write path dirtying
> +	 * exactly the range it reserved; the dirty_folio callback must not
> +	 * treat it as potentially unnotified and drag the folio's clean
> +	 * sibling blocks into COW (that breaks reflink sharing of the
> +	 * untouched blocks, among other things).  Unnotified dirtiers call
> +	 * folio_mark_dirty() directly and never pass through here.
> +	 */
> +	WRITE_ONCE(bfs->reserving_dirty, true);
>   	folio_mark_dirty(folio);

My solution in my initial patchset is to not call folio_mark_dirty() as 
it will call back to the ->dirty_folio() call back.

So I implemented an local version which call back to 
filemap_mark_dirty(), other than ->dirty_folio() which sets the fixup bits.

> +	WRITE_ONCE(bfs->reserving_dirty, false);
>   }
>   
>   static void folio_clear_tags(struct folio *folio)
> @@ -457,6 +467,88 @@ void btrfs_subpage_clear_writeback(const struct btrfs_fs_info *fs_info,
>   	spin_unlock_irqrestore(&bfs->lock, flags);
>   }
>   
> +void btrfs_subpage_clear_fixup(const struct btrfs_fs_info *fs_info,
> +			       struct folio *folio, u64 start, u32 len)
> +{
> +	struct btrfs_folio_state *bfs = folio_get_private(folio);
> +	unsigned int start_bit = subpage_calc_start_bit(fs_info, folio,
> +							fixup, start, len);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bfs->lock, flags);
> +	bitmap_clear(bfs->bitmaps, start_bit, len >> fs_info->sectorsize_bits);
> +	spin_unlock_irqrestore(&bfs->lock, flags);

Shouldn't we also check if we have cleared the last fixup bit and clear 
the folio fixup flag?
[...]
> +void btrfs_folio_cancel_fixup(const struct btrfs_fs_info *fs_info,
> +			      struct folio *folio, u64 start, u32 len)

Why not let IMPLEMNT_BTRFS_PAGE_OPS() to implement all the fixup helpers?

[...]
>   
> @@ -50,6 +65,14 @@ enum {
>   struct btrfs_folio_state {
>   	/* Common members for both data and metadata pages */
>   	spinlock_t lock;
> +	/*
> +	 * Set across the folio_mark_dirty() that a reserving write path
> +	 * issues from btrfs_subpage_set_dirty(), so that the dirty_folio
> +	 * callback does not treat the dirtying as potentially unnotified
> +	 * and mark the folio's still-clean sibling blocks.  Written under
> +	 * the folio lock, which every reserving path holds.
> +	 */
> +	bool reserving_dirty;

As I mentioned, this is only for dirty_folio() to skip setting fixup bit.

We can get rid of it, if the btrfs_folio_set_dirty() helper doesn't go 
through folio_mark_dirty().

Thanks,
Qu