On 2/3/2025 1:17 PM, Johannes Thumshirn wrote: >>> I wouldn't call this "file system offload". Enabling the data >>> integrity feature or whatever you want to call it is really a block >>> layer issue. The file system doesn't need to get involved at all. >>> Indeed, looking the patch, the only reason why the file system is >>> getting involved is because (a) you've added a mount option, and (b) >>> the mount option flips a bit in the bio that gets sent to the block >>> layer. >> Mount option was only for the RFC. If everything else gets sorted, it >> would be about choosing whatever is liked by the Btrfs. >> > But this could also be done by adding a queue specific flag, at which >>> point the file system doesn't need to be involved at all. Why would >>> you want to enable the data ingregity feature on a per block I/O >>> basis, if the device supports it? >> Because I thought users (filesystems) would prefer flexibility. Per-IO >> control helps to choose different policy for say data and meta. Let me >> outline the differences. > But data and metadata checksums are handled differently in btrfs. For > data we checksum each block and write it into the checksum tree. For > metadata the checksum is part of the metadata (see 'struct btrfs_header'). Yes, I am aware that btrfs meta checksum is put into an exiting field in the B-tree header. It only affects the compute part, and not the write-amplification part unlike the data checksums. That is exactly why I did not find it worth to optimize in the existing RFC. >> Block-layer auto integrity >> - always attaches integrity-payload for each I/O. >> - it does compute checksum/reftag for each I/O. And this part does not >> do justice to the label 'offload'. >> >> The patches make auto-integrity >> - attach the integrity-buffer only if the device configuration demands. >> - never compute checksum/reftag at the block-layer. >> - keeps the offload choice at per I/O level. >> >> Btrfs checksum tree is created only for data blocks, so the patches >> apply the flag (REQ_INTEGRITY_OFFLOAD) on that. While metadata blocks, >> which maybe more important, continue to get checksummed at two levels >> (block and device). > The thing I don't like with the current RFC patchset is, it breaks > scrub, repair and device error statistics. It nothing that can't be > solved though. But as of now it just doesn't make any sense at all to > me. We at least need the FS to look at the BLK_STS_PROTECTION return and > handle accordingly in scrub, read repair and statistics. > > And that's only for feature parity. I'd also like to see some > performance numbers and numbers of reduced WAF, if this is really worth > the hassle. Have you seen the numbers mentioned in the cover letter of the RFC? For direct IO: IOPS increase from 144K to 172K, and extra-writes reduction from 66GB to 9GB. Something similar for buffered IO: https://lore.kernel.org/linux-block/20250129140207.22718-1-joshi.k@xxxxxxxxxxx/
