Re: [PATCH] btrfs: tag all metadata I/O with REQ_META

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

在 2026/11/6 01:17, Mark Korenberg 写道:
> On 2026/11/5 19:37, Qu Wenruo wrote:
[...]
>>
>> E.g. a btrfs over a LVM/mdraid, and btrfs passes REQ_META for a tree
>> block read, then should the LVM/mdraid pass REQ_META to the underlying
>> devices?
> 
> dm clones keep bi_opf, so LVM and dm-crypt pass REQ_META down.  md
> raid1/raid10 pass it too since e879a0d9cb08 ("md/raid1,raid10: don't
> ignore IO flags"), which names the priority inversion when REQ_META is
> cleared.  md raid5 drops it: ops_run_io() builds the per-device bios
> with FUA and SYNC only.  I see that as a gap in raid5 that the
> raid1/raid10 fix did not cover, not as something to follow.

I'd say, for all reads (RAID56 reads are no different from RAID0 with 
rotation), keep the REQ_META as long as it's an extent buffer read.

For simple writes (everything but RAID56), keep the REQ_META too.

For raid56 writes/recovery, scrub/replace/reloaction, just don't bother.

LVM's pv move won't know if the data it's copying is metadata or data.

Although btrfs has some clue, it's not reliable clue, and furthermore, 
it's not the usual metadata IO pattern.

Regular btrfs metadata IO has its size limited to nodesize, and never 
merge the bios from btrfs.

Scrub/replace/relocation and RAID56 writes/recovery doesn't follow that, 
thus I can always argue they are no different than data IO patterns.

> 
>> If not, I see no particular reason to strictly follow that for
>> scrub/dev-replace/raid56 recovery.
> 
> md resync cannot know what it copies, btrfs scrub and dev-replace do.

Scrub don't care. It's just reading out the full 64KiB in one go, 
doesn't care what's in it unless there is some corruption and need to be 
repaired.

And again, the pattern doesn't match metadata IO patterns.

> 
>> I'm more interested in what you really want to archieve?
>>
>> Just to make the REQ_META more consistent?
> 
> Not only.  The ublk zoned device I use sees REQ_META as
> UBLK_IO_F_META, and its server caches and places metadata differently
> from data.

That's to benefit regular metadata IOs.

Scrub/replace/balance doesn't follow that pattern, and I'd say doing 
such metadata caching may not cause any benefit.

Furthermore, I'd say the optimization should be transparent, and should 
only be an optimization.

Their optimization may even not be suitable for btrfs' dynamic chunk 
allocation.

E.g. previously one chunk is metadata, but later the chunk is freed and 
re-allocated with a data chunk.

I don't think it should be something to follow dogmatically.

>  A tree block read issued by scrub or by RAID56 recovery is
> still a metadata read for it.

In fact, the scrub case is exactly an anti-example.

Scrub should have lower priority, no matter if it's handling a metadata.

Scrub is always working on committed metadata/data, and it should not 
compete with existing metadata priority.

>  The same holds for the in-kernel users:
> BFQ prefers REQ_META requests, blk-cgroup issues them as root to avoid
> priority inversion, and wbt does not throttle them.
> 
>> At least I do not think it's a good idea to touch RAID56, unless you can
>> prove that mdraid is also passing those flags.
>>
>> I want to align btrfs to existing RAID56 implementation, not the opposite.
> 
> I'll move the RAID56 part to a separate patch in v2, so the rest does
> not wait on it.

No, I think it should not be done at all for RAID56/scrub/replace/balance.

> 
> Still, I don't see what it can break: it changes no algorithm, only
> the hint.

The hint is already against what we expect for scrub.

> And within btrfs, metadata writes on DUP, RAID1, RAID1C3/4
> and RAID10 already carry REQ_META, because the per-mirror bios are
> clones of the original.

And that's the only part I agree.

>  RAID56 is the only profile that builds new
> per-device bios and loses it.  md raid1/raid10 pass it as well, see
> above.
> 
>>> +static inline blk_opf_t btrfs_chunk_req_meta(u64 type)
>>> +{
>>> +	return (type & BTRFS_BLOCK_GROUP_DATA) ? 0 : REQ_META;
>>
>> Missing BLOCK_GROUP_METADATA_REMAP and also handling BLOCK_GROUP_SYSTEM.
> 
> Both get REQ_META: the check is for the DATA bit, and tree-checker
> allows only five chunk types (DATA, METADATA, SYSTEM, METADATA_REMAP
> and DATA|METADATA), so METADATA, SYSTEM and METADATA_REMAP are all
> tagged.  The SYSTEM rows in my test show it, METADATA_REMAP I did not
> test on its own.
> 
> That is clearly too implicit, and it leaves mixed chunks out.  In v2
> the helper returns 0 only for a pure data chunk:
> 
> 	return (type & BTRFS_BLOCK_GROUP_TYPE_MASK) == BTRFS_BLOCK_GROUP_DATA ?
> 	       0 : REQ_META;
> 
> and the comment lists what gets tagged: METADATA, SYSTEM,
> METADATA_REMAP and mixed chunks.