On 03.02.25 14:25, Kanchan Joshi wrote: > On 1/31/2025 3:59 PM, Johannes Thumshirn wrote: >>> I tested the series for read, but only the success cases. In this case >>> checksum generation/verification happens only within the device. It was >>> slightly tricky to inject an error and I skipped that. >>> >>> Since separate checksum I/Os are omitted, this is about handling the >>> error condition in data read I/O path itself. I have not yet checked if >>> repair code triggers when Btrfs is working with existing 'nodatasum' >>> mount option. But I get your point that this needs to be handled. >>> >> So this as of now disables one of the most useful features of the FS, >> repairing bad data. The whole "story" for the RAID code in the FS is >> build around this assumption, that we can repair bad data, unlike say MD >> RAID. > > Does repairing-bad-data work when Btrfs is mounted with NODATASUM? > If not, should not the proposed option be seen as an upgrade over that? > > You might be knowing, but I do not know how does Btrfs currently decide > to apply NODATSUM! With these patches it becomes possible to know if > checksum-offload is supported by the underlying hardware. And that makes > it possible to apply NODATASUM in an informed manner. NODATASUM is something I personally would only ever tun on on VM images, so we don't have the stable page vs checksums f*up (see Qu's answers). > I have not reduced anything, but added an opt-in for deployments that > may have a different definition of what is useful. Not all planets are > Mars. The cost of checksum tree will be different (say on QLC vs SLC). > But NODATASUM isn't something that is actively recommended unless you know what you're doing. I thought of your patches as an offload of the checksum tree to the T10-PI extended sector format, which I personally like. And it's not that hard to do that. If it's about getting people to use NODATASUM I'm really starting to dislike the patchset. Also NODATASUM implies deactivated compression. So this to me then sounds like a good use case for 1 or 2 specific scenarios but not really all too helpful for the broader picture, which it could've been.
