Re: [PATCH 0/7] fio: atomic write support

[Jens Axboe <axboe@kernel.dk>]

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