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

在 2026/7/26 05:42, Boris Burkov 写道:
> On Sat, Jul 25, 2026 at 07:34:46PM +0930, Qu Wenruo wrote:
[...]
>>
>> 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.
>>
> 
> the delalloc lookup is essential for bs == folio_size, to tell apart
> the case where you are submitting a folio that got redirtied behind you
> (don't skip) and the case where it is a true fixup.
> 
> Thinking about it more, though, that re-dirty shouldn't really be
> possible at least in typical cases, so maybe I can figure out how to
> remove this.

In that case, I'd prefer to use folio->private to have one extra bit, to 
indicate the folio is properly dirtied.

E.g. this commit in my unfinished branch:
https://github.com/adam900710/linux/commit/9a02921d348574573f1a6dbc50e54f993011e311

Which acts like the subpage dirty bit for bs == folio size case.
> 
> I think for subpage it probably doesn't matter, because those should not
> have fixup in their bitmaps.
> 
>>> +			continue;
>>> +		if (btrfs_is_subpage(fs_info, folio) &&
>>> +		    !btrfs_subpage_test_fixup(fs_info, folio, start, sectorsize)) {
[...]
>>
>> 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.
> 
> I think this approach here seems correct to me, except for maybe a
> really mean case which calls aops->dirty_folio() without locking the folio
> (not the case for dio read at least).
> 
> Won't any real reservation will cancel the fixup bits under the lock, so
> the way this implementation tries to handle it is to reserve the whole
> range then free the parts that don't end up getting in the "funded"
> bitmap (also a bad LLM name...). This is checking the fixup bitmap under
> the lock, so should see correct cancellations. Do you see an obvious
> problem with that strategy, besides that it is kind of ugly and annoying
> to handle the 'out:' path. Which feels similarly painful to a lot of our
> other delalloc/qgroup_rsv unwinding logic with data_reserveds on OE error
> or whatnot.

No, I have no better solution at all. So it's not an objection.

> 
> I think the clearer option of "take the lock, observe the real set,
> release, then reserve, then take the lock and do all the work" sounds
> appealing but almost certainly fails to some simple TOCTOU race.
> 
>>
>>
[..]
>>> +	 */
>>> +	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.
>>
> 
> You mean to avoid the ugly bfs->reserving_dirty thing?
> That does sound appealing...

Yep. However the ugly part is, we will need a local 
"btrfs_folio_mark_dirty_prepared()", which copies "folio_mark_dirty()", 
except call filemap_dirty_folio() instead of using aops->dirty_folio.

Not sure if MM will be happy about that.

Thanks,
Qu