Re: [PATCH v2 05/13] generic/1226: Add atomic write test using fio crc check verifier

Ojaswin Mujoo
2025-07-11T10:39:45Z

On Tue, Jul 08, 2025 at 01:34:39PM +0100, John Garry wrote:
> On 08/07/2025 13:01, Ojaswin Mujoo wrote:
> > > You could do the statx call on the mounted bdev, and see if atomics are
> > > supported there. If not, skip the test.
> > >
> > > But even for xfs when HW support is available, we can still use software
> > > fallback when HW support is not possible, i.e. misaligned. So, in that
> > > regard, we really should only test this on ext4.
> > Hmm, okay so I'm a bit confused now. What is different in ext4 vs xfs.
> > Why does the software fallback for xfs matter in this case?
>
> For xfs, software fallback-based atomic writes are not even serialized
> against other software fallback-based atomic writes. They can trample on one
> another. That is a reason I say that this test should only be for HW-based
> atomic writes.
>
> But even if the bs <= HW limit, on xfs may rarely still use atomic write
> software fallback if HW-based atomic write is not possible, e.g. misaligned,
> multiple extents, etc. I will emphasis that this would be rare, but it is
> still possible.
>
> For ext4, since alignment is always guaranteed, and HW-based atomics will
> only ever be used.
>
> > If we limit
> > io to 16KB hoping that reads don't break, then either ways the fio
> > verify should work. And if reads break then both ext4 and xfs suffer
> > irrespective of hardware or software fallback. (We also saw that
> > hardware atomic writes also suffer with same issue when max_sectors is
> > low)
> >
> > > > Im not sure if returning non-zero for ext4 is also correct as per the
> > > > documented behavior from 5d894321c49e613.
> > > >
> > > > When zero, it means that there is no such performance boundary.
> > > >
> > > > Anyways, I think there might be a simpler solution...
> > > > > > The simplest way seems to be to just limit the $blocksize to
> > > > > > something like $(_min 16KB awu_max) and hope that 16KB is small enough
> > > > > > to not be split during reads. We anyways have other tests like
> > > > > > generic/1230 that can be used to test larger atomic write sizes
> > > > > >
> > > > > > Thoughts?
> > > > > As I said at the start, we never had guarantees of serialization of reads
> > > > > and atomic writes.
> > > > >
> > > > > However I still think that this is a useful test. It's just it is
> > > > > theoretically possible to give false positives.
> > > > >
> > > > > You could get the test to read max_sectors_kb, and check whether it is
> > > > > greater than the bs. Again, more complexity.
> > > > I think this might work. So i can set the iosize as 16kb which is
> > > > already low enought. Incase the max_sectors is < than 16kb then we bail
> > > > out. (but then maybe there is a very small chance of read split still)
> > > Sure, that seems fine.
> > >
> > > > OR another approach is to do the verify at the very end when all threads
> > > > are done writing so the reads don't race. The tradeoff is that this
> > > > will reduce the effictiveness of the test though to some extent.
> > > Yeah, right. We would want this test to prove that atomic writes are not
> > > getting split by the block stack, so racing reads can help prove that.
> > Yeahh, but now i'm leaning towards this approach of keeping verify at
> > the very end instead of parallel.
>
> I don't think that fio supports such a mode. Or does it?

Yes we can achieve it via combination of write job with verify=crc,do_verify=0 followed by
verify job with verify=crc,verify_only=1. I'm testing this right now to
see if it can survive max_sectors_kb = 4 (which i think it should since
there are no races now). If everything is fine I'll use this approach
for the fio tests in the next revision.
>
> > Yes, it is a bit less effective but
> > then having a data integrity test that can theoretically return false
> > positives is not good, it'll always keep me guessing about any failure.
> > Having verify at end will not have false failures for sure. Even though
> > it comes at a cost of less coverage.
> >
> > Maybe we can also get some other prespectives on this.
> > @Zorro, @Darrick, can you please give your thoughts on this?
> >
> > As a sidenote, with this issue of fio's verify read splitting, it seems
> > like the parallel verify feature in fio itself is flawed for non-atomic
> > writes as well since the read can always split and race with the writes.
>
> fio does warn that multiple read and writers in verify mode is unsafe.
Right.

>
> >
> > Not sure what the solution is here, maybe time for non splittable atomic
> > writes (:p)
>
> I just think that fio in verify mode for ext4 is ok, as long as
> max_sectors_kb is large.

Yes but i'd rather have the write followed by verify version to avoid
false positives in case the read bio does end up splitting for whatever
reason.

Thanks for catching this issue in the test!

Regards,
ojaswin