On 31.01.25 14:11, Kanchan Joshi wrote: > On 1/30/2025 7:58 PM, Theodore Ts'o wrote: >> On Thu, Jan 30, 2025 at 02:45:45PM +0530, Kanchan Joshi wrote: >>> I would like to propose a discussion on employing checksum offload in >>> filesystems. >>> It would be good to co-locate this with the storage track, as the >>> finer details lie in the block layer and NVMe driver. >> >> 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'). > 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. Thanks, Johannes
