On 9/10/24 4:03 AM, John Garry wrote: > On 10/09/2024 02:01, Jens Axboe wrote: > > + Dave, who might be interested in this > >>> I was preparing a v2 the same as this, but do you think that I should >>> just add a new verify option for atomic writes to ignore the header? >>> Or is just saying "you have selected atomic=1, so I'll just ignore the >>> header for you" ok? >> I'm still not following why atomic writes make this any different. They >> should not. If you have multiple writers writing the same blocks, yeah >> you will get verification errors. This is true for any kind of write, be >> it buffered, dio, or atomic. So unless I'm missing something here, I'd >> just drop that patch. (side note - please wrap your email lines, I always re-wrap when replying) > Some background is that main selling point of atomic writes is that we > guarantee writes to storage will not be torn for a power failure or > kernel crash. > > Another aspect of atomic writes is that they handle racing writes and > reads, such that a read racing with a write will see all the data from > the write or none. Well, SCSI and NVMe guarantee this if using > RWF_ATOMIC, but it is not formally stated as a feature of RWF_ATOMIC. > > It can be argued that having racing reads and writes is an application > bug. Furthermore, as I understand, even if posix guarantees that > regular writes are "atomic", it is not the case generally. > > So one part of the relevance of atomic writes to fio verify is that we > can verify that atomic writes "safely" handle racing read and writes. > For this, the CRC checks would be successful if we have many jobs; > however header sequence numbers are not. Hence patch 4/7. > > I had also been using the verify feature to test atomic writes for > power failures. In this case, I run a single verify job with > --rw=write, power fail, and use verify in read mode to prove no > invalid data in the file, like: > > fio --filename=mnt/file --direct=1 --rw=read --bs=8k --iodepth=100 --na > me=iops --numjobs=1 --loops=1 --verify=crc64 --ioengine=libaio > --verify_fatal=1 --group_reporting --exitall_on_error > > This power fail test is what I am mostly interested in. > > So my point is that the patch to ignore invalid headers could be > dropped, but let me know your thoughts. Gotcha, that makes sense. For atomic writes, it's totally fine to have overlapping writers if the write size is in the atomic units, but we can of course expect sequences to be out of order depending on which one makes it to stable storage. But you have no checks for whether or not the write size is within the atomic range? I think dropping patch 4 and adding a verify option to specifically ignore the sequence would make sense, leaving the control in the hands of the user. And then bonus points for adding an example job file (with comments) in your series that shows how to use atomic writes (and uses that option) would be useful. -- Jens Axboe
