On 2023/3/7 22:41, Christoph Hellwig wrote:
On Tue, Mar 07, 2023 at 09:44:32AM +0800, Qu Wenruo wrote:
With my recent restart on scrub rework, this patch makes me wonder, what if
scrub wants to use btrfs_bio, but don't want to pass a valid btrfs_inode
pointer?
The full inode is only really needed for the data repair code. But a lot
of code uses the fs_info, which would have to be added as a separate
counter. The other usage is the sync_writers counter, which is a bit
odd and should probably be keyed off the REQ_SYNC flag instead.
E.g. scrub code just wants to read certain mirror of a logical bytenr.
This can simplify the handling of RAID56, as for data stripes the repair
path is the same, just try the next mirror(s).
Furthermore most of the new btrfs_bio code is handling data reads by
triggering read-repair automatically.
This can be unnecessary for scrub.
This sounds like you don't want to use the btrfs_bio at all as you
don't rely on any of the functionality from it.
Well, to me the proper mirror_num based read is the core of btrfs_bio,
not the read-repair thing.
Thus I'm not that convinced fully automatic read-repair integrated into
btrfs_bio is a good idea.
Thanks,
Qu
And since we're here, can we also have btrfs equivalent of on-stack bio?
As scrub can benefit a lot from that, as for sector-by-sector read, we want
to avoid repeating allocating/freeing a btrfs_bio just for reading one
sector.
(The existing behavior is using on-stack bio with bio_init/bio_uninit
inside scrub_recheck_block())
You can do that right now by declaring a btrfs_bio on-stack and then
calling bio_init on the embedded bio followed by a btrfs_bio_init on
the btrfs_bio. But I don't think doing this will actually be a win
for the scrub code in terms of speed or code size.
