On 9/10/24 10:38 AM, John Garry wrote: > On 10/09/2024 16:56, Jens Axboe wrote: >>>> t drop that patch. >> (side note - please wrap your email lines, I always re-wrap when >> replying) > > ok This one too :) >>> 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 don't currently. If an atomic write size is out-of-range, the kernel > will reject it with -EINVAL. However -EINVAL can be returned for a > multitude of issues, so not much help. So I could add a statx call to > get the limits and then error the fio config (if bs is out-of-range). Good point, I guess that's good enough then. If it's an invalid configuration, then the writes will get errored anyway. >> 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. > > I already saw verify modes "no header" and "header only", so I was > reluctant to add a potentially conflicting new separate option to > ignore the header sequence. However, I can look to add a new verify > option to ignore the header sequence and ensure it respects those > mentioned verify modes. I think you'd be fine adding a verify_write_sequence bool and just have it default to true. >> 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. > > ok, I'm happy to do that. Great, thanks. -- Jens Axboe
