From: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
This adds atomic write test using fio based on it's crc check verifier.
fio adds a crc for each data block. If the underlying device supports atomic
write then it is guaranteed that we will never have a mix data from two
threads writing on the same physical block.
Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
tests/generic/1226 | 101 +++++++++++++++++++++++++++++++++++++++++
tests/generic/1226.out | 2 +
2 files changed, 103 insertions(+)
create mode 100755 tests/generic/1226
create mode 100644 tests/generic/1226.out
diff --git a/tests/generic/1226 b/tests/generic/1226
new file mode 100755
index 00000000..455fc55f
--- /dev/null
+++ b/tests/generic/1226
@@ -0,0 +1,101 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025 IBM Corporation. All Rights Reserved.
+#
+# FS QA Test 1226
+#
+# Validate FS atomic write using fio crc check verifier.
+#
+. ./common/preamble
+. ./common/atomicwrites
+
+_begin_fstest auto aio rw atomicwrites
+
+_require_scratch_write_atomic
+_require_odirect
+_require_aio
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+touch "$SCRATCH_MNT/f1"
+awu_min_write=$(_get_atomic_write_unit_min "$SCRATCH_MNT/f1")
+awu_max_write=$(_get_atomic_write_unit_max "$SCRATCH_MNT/f1")
+blocksize=$(_max "$awu_min_write" "$((awu_max_write/2))")
+
+fio_config=$tmp.fio
+fio_out=$tmp.fio.out
+
+FIO_LOAD=$(($(nproc) * 2 * LOAD_FACTOR))
+SIZE=$((100 * 1024 * 1024))
+
+function create_fio_configs()
+{
+ create_fio_aw_config
+ create_fio_verify_config
+}
+
+function create_fio_verify_config()
+{
+cat >$fio_verify_config <<EOF
+ [verify-job]
+ direct=1
+ ioengine=libaio
+ rw=randwrite
+ bs=$blocksize
+ fallocate=native
+ filename=$SCRATCH_MNT/test-file
+ size=$SIZE
+ iodepth=$FIO_LOAD
+ group_reporting=1
+
+ verify_only=1
+ verify=crc32c
+ verify_fatal=1
+ verify_state_save=0
+ verify_write_sequence=0
+EOF
+}
+
+function create_fio_aw_config()
+{
+cat >$fio_aw_config <<EOF
+ [atomicwrite-job]
+ direct=1
+ ioengine=libaio
+ rw=randwrite
+ bs=$blocksize
+ fallocate=native
+ filename=$SCRATCH_MNT/test-file
+ size=$SIZE
+ iodepth=$FIO_LOAD
+ numjobs=$FIO_LOAD
+ group_reporting=1
+ atomic=1
+
+ verify_state_save=0
+ verify=crc32c
+ do_verify=0
+EOF
+}
+
+fio_aw_config=$tmp.aw.fio
+fio_verify_config=$tmp.verify.fio
+
+create_fio_configs
+_require_fio $fio_aw_config
+
+cat $fio_aw_config >> $seqres.full
+cat $fio_verify_config >> $seqres.full
+
+$FIO_PROG $fio_aw_config >> $seqres.full
+ret1=$?
+$FIO_PROG $fio_verify_config >> $seqres.full
+ret2=$?
+
+[[ $ret1 -eq 0 && $ret2 -eq 0 ]] || _fail "fio with atomic write failed"
+
+# success, all done
+echo Silence is golden
+status=0
+exit
diff --git a/tests/generic/1226.out b/tests/generic/1226.out
new file mode 100644
index 00000000..6dce0ea5
--- /dev/null
+++ b/tests/generic/1226.out
@@ -0,0 +1,2 @@
+QA output created by 1226
+Silence is golden
--
2.49.0On 12/07/2025 15:12, Ojaswin Mujoo wrote:
> From: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
>
> This adds atomic write test using fio based on it's crc check verifier.
> fio adds a crc for each data block. If the underlying device supports atomic
> write then it is guaranteed that we will never have a mix data from two
> threads writing on the same physical block.
I think that you should mention that 2-phase approach.
Is there something which ensures that we have fio which supports
RWF_ATOMIC? fio for some time supported the "atomic" cmdline param, but
did not do anything until recently
>
> Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
> tests/generic/1226 | 101 +++++++++++++++++++++++++++++++++++++++++
> tests/generic/1226.out | 2 +
Was this tested with xfs?
> 2 files changed, 103 insertions(+)
> create mode 100755 tests/generic/1226
> create mode 100644 tests/generic/1226.out
>
> diff --git a/tests/generic/1226 b/tests/generic/1226
> new file mode 100755
> index 00000000..455fc55f
> --- /dev/null
> +++ b/tests/generic/1226
> @@ -0,0 +1,101 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2025 IBM Corporation. All Rights Reserved.
> +#
> +# FS QA Test 1226
> +#
> +# Validate FS atomic write using fio crc check verifier.
> +#
> +. ./common/preamble
> +. ./common/atomicwrites
> +
> +_begin_fstest auto aio rw atomicwrites
> +
> +_require_scratch_write_atomic
> +_require_odirect
> +_require_aio
> +
> +_scratch_mkfs >> $seqres.full 2>&1
> +_scratch_mount
> +
> +touch "$SCRATCH_MNT/f1"
> +awu_min_write=$(_get_atomic_write_unit_min "$SCRATCH_MNT/f1")
> +awu_max_write=$(_get_atomic_write_unit_max "$SCRATCH_MNT/f1")
> +blocksize=$(_max "$awu_min_write" "$((awu_max_write/2))")
> +
> +fio_config=$tmp.fio
> +fio_out=$tmp.fio.out
> +
> +FIO_LOAD=$(($(nproc) * 2 * LOAD_FACTOR))
> +SIZE=$((100 * 1024 * 1024))
> +
> +function create_fio_configs()
> +{
> + create_fio_aw_config
it's strange ordering in this file, since create_fio_aw_config is
declared below here
> + create_fio_verify_config
same
> +}
> +
> +function create_fio_verify_config()
> +{
> +cat >$fio_verify_config <<EOF
> + [verify-job]
> + direct=1
> + ioengine=libaio
> + rw=randwrite
is this really required? Maybe it is. I would use read if something was
required for this param
> + bs=$blocksize
> + fallocate=native
> + filename=$SCRATCH_MNT/test-file
> + size=$SIZE
> + iodepth=$FIO_LOAD
> + group_reporting=1
> +
> + verify_only=1
> + verify=crc32c
> + verify_fatal=1
> + verify_state_save=0
> + verify_write_sequence=0
> +EOF
> +}
> +
> +function create_fio_aw_config()
> +{
> +cat >$fio_aw_config <<EOF
> + [atomicwrite-job]
> + direct=1
> + ioengine=libaio
> + rw=randwrite
> + bs=$blocksize
> + fallocate=native
> + filename=$SCRATCH_MNT/test-file
> + size=$SIZE
> + iodepth=$FIO_LOAD
> + numjobs=$FIO_LOAD
> + group_reporting=1
> + atomic=1
> +
> + verify_state_save=0
> + verify=crc32c
> + do_verify=0
> +EOF
> +}
> +
> +fio_aw_config=$tmp.aw.fio
> +fio_verify_config=$tmp.verify.fio
> +
> +create_fio_configs
> +_require_fio $fio_aw_config
> +
> +cat $fio_aw_config >> $seqres.full
> +cat $fio_verify_config >> $seqres.full
> +
> +$FIO_PROG $fio_aw_config >> $seqres.full
> +ret1=$?
> +$FIO_PROG $fio_verify_config >> $seqres.full
> +ret2=$?
> +
> +[[ $ret1 -eq 0 && $ret2 -eq 0 ]] || _fail "fio with atomic write failed"
> +
> +# success, all done
> +echo Silence is golden
> +status=0
> +exit
> diff --git a/tests/generic/1226.out b/tests/generic/1226.out
> new file mode 100644
> index 00000000..6dce0ea5
> --- /dev/null
> +++ b/tests/generic/1226.out
> @@ -0,0 +1,2 @@
> +QA output created by 1226
> +Silence is golden
On Thu, Jul 17, 2025 at 02:00:18PM +0100, John Garry wrote:
> On 12/07/2025 15:12, Ojaswin Mujoo wrote:
> > From: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
> >
> > This adds atomic write test using fio based on it's crc check verifier.
> > fio adds a crc for each data block. If the underlying device supports atomic
> > write then it is guaranteed that we will never have a mix data from two
> > threads writing on the same physical block.
>
> I think that you should mention that 2-phase approach.
Sure I can add a comment and update the commit message with this.
>
> Is there something which ensures that we have fio which supports RWF_ATOMIC?
> fio for some time supported the "atomic" cmdline param, but did not do
> anything until recently
We do have _require_fio which ensures the options passed are supported
by the current fio. If you are saying some versions of fio have --atomic
valid but dont do an RWF_ATOMIC then I'm not really sure if that can be
caught though.
>
> >
> > Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> > tests/generic/1226 | 101 +++++++++++++++++++++++++++++++++++++++++
> > tests/generic/1226.out | 2 +
>
> Was this tested with xfs?
Yes, I've tested with XFS with software fallback as well. Also, tested
xfs while keeping io size as 16kb so we stress the hw paths too. Both
seem to be passing as expected.
>
> > 2 files changed, 103 insertions(+)
> > create mode 100755 tests/generic/1226
> > create mode 100644 tests/generic/1226.out
> >
> > diff --git a/tests/generic/1226 b/tests/generic/1226
> > new file mode 100755
> > index 00000000..455fc55f
> > --- /dev/null
> > +++ b/tests/generic/1226
> > @@ -0,0 +1,101 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2025 IBM Corporation. All Rights Reserved.
> > +#
> > +# FS QA Test 1226
> > +#
> > +# Validate FS atomic write using fio crc check verifier.
> > +#
> > +. ./common/preamble
> > +. ./common/atomicwrites
> > +
> > +_begin_fstest auto aio rw atomicwrites
> > +
> > +_require_scratch_write_atomic
> > +_require_odirect
> > +_require_aio
> > +
> > +_scratch_mkfs >> $seqres.full 2>&1
> > +_scratch_mount
> > +
> > +touch "$SCRATCH_MNT/f1"
> > +awu_min_write=$(_get_atomic_write_unit_min "$SCRATCH_MNT/f1")
> > +awu_max_write=$(_get_atomic_write_unit_max "$SCRATCH_MNT/f1")
> > +blocksize=$(_max "$awu_min_write" "$((awu_max_write/2))")
> > +
> > +fio_config=$tmp.fio
> > +fio_out=$tmp.fio.out
> > +
> > +FIO_LOAD=$(($(nproc) * 2 * LOAD_FACTOR))
> > +SIZE=$((100 * 1024 * 1024))
> > +
> > +function create_fio_configs()
> > +{
> > + create_fio_aw_config
>
> it's strange ordering in this file, since create_fio_aw_config is declared
> below here
>
> > + create_fio_verify_config
>
> same
That works in bash.
>
> > +}
> > +
> > +function create_fio_verify_config()
> > +{
> > +cat >$fio_verify_config <<EOF
> > + [verify-job]
> > + direct=1
> > + ioengine=libaio
> > + rw=randwrite
>
> is this really required? Maybe it is. I would use read if something was
> required for this param
Usually the fio verfiy phase internally converts writes to reads so
rw=write and rw=read doesnt matter much. I can make the change tho,
should be fine.
Thanks,
ojaswin
>
> > + bs=$blocksize
> > + fallocate=native
> > + filename=$SCRATCH_MNT/test-file
> > + size=$SIZE
> > + iodepth=$FIO_LOAD
> > + group_reporting=1
> > +
> > + verify_only=1
> > + verify=crc32c
> > + verify_fatal=1
> > + verify_state_save=0
> > + verify_write_sequence=0
> > +EOF
> > +}
> > +
> > +function create_fio_aw_config()
> > +{
> > +cat >$fio_aw_config <<EOF
> > + [atomicwrite-job]
> > + direct=1
> > + ioengine=libaio
> > + rw=randwrite
> > + bs=$blocksize
> > + fallocate=native
> > + filename=$SCRATCH_MNT/test-file
> > + size=$SIZE
> > + iodepth=$FIO_LOAD
> > + numjobs=$FIO_LOAD
> > + group_reporting=1
> > + atomic=1
> > +
> > + verify_state_save=0
> > + verify=crc32c
> > + do_verify=0
> > +EOF
> > +}
> > +
> > +fio_aw_config=$tmp.aw.fio
> > +fio_verify_config=$tmp.verify.fio
> > +
> > +create_fio_configs
> > +_require_fio $fio_aw_config
> > +
> > +cat $fio_aw_config >> $seqres.full
> > +cat $fio_verify_config >> $seqres.full
> > +
> > +$FIO_PROG $fio_aw_config >> $seqres.full
> > +ret1=$?
> > +$FIO_PROG $fio_verify_config >> $seqres.full
> > +ret2=$?
> > +
> > +[[ $ret1 -eq 0 && $ret2 -eq 0 ]] || _fail "fio with atomic write failed"
> > +
> > +# success, all done
> > +echo Silence is golden
> > +status=0
> > +exit
> > diff --git a/tests/generic/1226.out b/tests/generic/1226.out
> > new file mode 100644
> > index 00000000..6dce0ea5
> > --- /dev/null
> > +++ b/tests/generic/1226.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 1226
> > +Silence is golden
>
On 17/07/2025 14:52, Ojaswin Mujoo wrote: > On Thu, Jul 17, 2025 at 02:00:18PM +0100, John Garry wrote: >> On 12/07/2025 15:12, Ojaswin Mujoo wrote: >>> From: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com> >>> >>> This adds atomic write test using fio based on it's crc check verifier. >>> fio adds a crc for each data block. If the underlying device supports atomic >>> write then it is guaranteed that we will never have a mix data from two >>> threads writing on the same physical block. >> >> I think that you should mention that 2-phase approach. > > Sure I can add a comment and update the commit message with this. > >> >> Is there something which ensures that we have fio which supports RWF_ATOMIC? >> fio for some time supported the "atomic" cmdline param, but did not do >> anything until recently > > We do have _require_fio which ensures the options passed are supported > by the current fio. If you are saying some versions of fio have --atomic > valid but dont do an RWF_ATOMIC then I'm not really sure if that can be > caught though. Can you check the fio version? > >> >>> >>> Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >>> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> >>> --- >>> tests/generic/1226 | 101 +++++++++++++++++++++++++++++++++++++++++ >>> tests/generic/1226.out | 2 + >> >> Was this tested with xfs? > > Yes, I've tested with XFS with software fallback as well. Also, tested > xfs while keeping io size as 16kb so we stress the hw paths too. so is that requirement implemented with the _require_scratch_write_atomic check? > Both > seem to be passing as expected. >>
On Thu, Jul 17, 2025 at 03:06:01PM +0100, John Garry wrote: > On 17/07/2025 14:52, Ojaswin Mujoo wrote: > > On Thu, Jul 17, 2025 at 02:00:18PM +0100, John Garry wrote: > > > On 12/07/2025 15:12, Ojaswin Mujoo wrote: > > > > From: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com> > > > > > > > > This adds atomic write test using fio based on it's crc check verifier. > > > > fio adds a crc for each data block. If the underlying device supports atomic > > > > write then it is guaranteed that we will never have a mix data from two > > > > threads writing on the same physical block. > > > > > > I think that you should mention that 2-phase approach. > > > > Sure I can add a comment and update the commit message with this. > > > > > > > > Is there something which ensures that we have fio which supports RWF_ATOMIC? > > > fio for some time supported the "atomic" cmdline param, but did not do > > > anything until recently > > > > We do have _require_fio which ensures the options passed are supported > > by the current fio. If you are saying some versions of fio have --atomic > > valid but dont do an RWF_ATOMIC then I'm not really sure if that can be > > caught though. > > Can you check the fio version? We don't have a helper but yes I think that should be possible > > > > > > > > > > > > > > Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > > > > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > > > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> > > > > --- > > > > tests/generic/1226 | 101 +++++++++++++++++++++++++++++++++++++++++ > > > > tests/generic/1226.out | 2 + > > > > > > Was this tested with xfs? > > > > Yes, I've tested with XFS with software fallback as well. Also, tested > > xfs while keeping io size as 16kb so we stress the hw paths too. > > so is that requirement implemented with the _require_scratch_write_atomic > check? No, its just something i hardcoded for that particular run. This patch doesn't enforce hardware only atomic writes Regards, ojaswin > > > Both > > seem to be passing as expected. > > > > >
On 22/07/2025 09:47, Ojaswin Mujoo wrote: >>> Yes, I've tested with XFS with software fallback as well. Also, tested >>> xfs while keeping io size as 16kb so we stress the hw paths too. >> so is that requirement implemented with the _require_scratch_write_atomic >> check? > No, its just something i hardcoded for that particular run. This patch > doesn't enforce hardware only atomic writes If we are to test this for XFS then we need to ensure that HW atomics are available. Thanks, John
On Wed, Jul 23, 2025 at 12:33:27PM +0100, John Garry wrote: > On 22/07/2025 09:47, Ojaswin Mujoo wrote: > > > > Yes, I've tested with XFS with software fallback as well. Also, tested > > > > xfs while keeping io size as 16kb so we stress the hw paths too. > > > so is that requirement implemented with the _require_scratch_write_atomic > > > check? > > No, its just something i hardcoded for that particular run. This patch > > doesn't enforce hardware only atomic writes > > If we are to test this for XFS then we need to ensure that HW atomics are > available. Why is that? Now with the verification step happening after writes, software atomic writes should also pass this test since there are no racing writes to the verify reads. Regards, ojaswin > > Thanks, > John
On 23/07/2025 14:51, Ojaswin Mujoo wrote: >>> No, its just something i hardcoded for that particular run. This patch >>> doesn't enforce hardware only atomic writes >> If we are to test this for XFS then we need to ensure that HW atomics are >> available. > Why is that? Now with the verification step happening after writes, > software atomic writes should also pass this test since there are no > racing writes to the verify reads. Sure, but racing software atomic writes against other software atomic writes is not safe. Thanks, John
On Wed, Jul 23, 2025 at 05:25:41PM +0100, John Garry wrote: > On 23/07/2025 14:51, Ojaswin Mujoo wrote: > > > > No, its just something i hardcoded for that particular run. This patch > > > > doesn't enforce hardware only atomic writes > > > If we are to test this for XFS then we need to ensure that HW atomics are > > > available. > > Why is that? Now with the verification step happening after writes, > > software atomic writes should also pass this test since there are no > > racing writes to the verify reads. > > Sure, but racing software atomic writes against other software atomic writes > is not safe. > > Thanks, > John What do you mean by not safe? Does it mean the test can fail?
On 25/07/2025 07:27, Ojaswin Mujoo wrote:
> On Wed, Jul 23, 2025 at 05:25:41PM +0100, John Garry wrote:
>> On 23/07/2025 14:51, Ojaswin Mujoo wrote:
>>>>> No, its just something i hardcoded for that particular run. This patch
>>>>> doesn't enforce hardware only atomic writes
>>>> If we are to test this for XFS then we need to ensure that HW atomics are
>>>> available.
>>> Why is that? Now with the verification step happening after writes,
>>> software atomic writes should also pass this test since there are no
>>> racing writes to the verify reads.
>> Sure, but racing software atomic writes against other software atomic writes
>> is not safe.
>>
>> Thanks,
>> John
> What do you mean by not safe?
Multiple threads issuing atomic writes may trample over one another.
It is due to the steps used to issue an atomic write in xfs by software
method. Here we do 3x steps:
a. allocate blocks for out-of-place write
b. do write in those blocks
c. atomically update extent mapping.
In this, threads wanting to atomic write to the same address will use
the new blocks and can trample over one another before we atomically
update the mapping.
So we do not guarantee serialization of atomic writes vs atomic writes.
And this is why I said that this test is never totally safe for xfs.
We could change this simply to have serialization of software-based
atomic writes against all other dio, like follows:
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -747,6 +747,7 @@ xfs_file_dio_write_atomic(
unsigned int iolock = XFS_IOLOCK_SHARED;
ssize_t ret, ocount = iov_iter_count(from);
const struct iomap_ops *dops;
+ unsigned int dio_flags = 0;
/*
* HW offload should be faster, so try that first if it is already
@@ -766,15 +767,12 @@ xfs_file_dio_write_atomic(
if (ret)
goto out_unlock;
- /* Demote similar to xfs_file_dio_write_aligned() */
- if (iolock == XFS_IOLOCK_EXCL) {
- xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
- iolock = XFS_IOLOCK_SHARED;
- }
+ if (dio_flags & IOMAP_DIO_FORCE_WAIT)
+ inode_dio_wait(VFS_I(ip));
trace_xfs_file_direct_write(iocb, from);
ret = iomap_dio_rw(iocb, from, dops, &xfs_dio_write_ops,
- 0, NULL, 0);
+ dio_flags, NULL, 0);
/*
* The retry mechanism is based on the ->iomap_begin method
returning
@@ -785,6 +783,8 @@ xfs_file_dio_write_atomic(
if (ret == -ENOPROTOOPT && dops == &xfs_direct_write_iomap_ops) {
xfs_iunlock(ip, iolock);
dops = &xfs_atomic_write_cow_iomap_ops;
+ iolock = XFS_IOLOCK_EXCL;
+ dio_flags = IOMAP_DIO_FORCE_WAIT;
goto retry;
}
But it may affect performance.
> Does it mean the test can fail?
Yes, but it is unlikely if we have HW atomics available. That is because
we will rarely be using software-based atomic method, as HW method
should often be possible.
On Fri, Jul 25, 2025 at 09:14:25AM +0100, John Garry wrote:
> On 25/07/2025 07:27, Ojaswin Mujoo wrote:
> > On Wed, Jul 23, 2025 at 05:25:41PM +0100, John Garry wrote:
> > > On 23/07/2025 14:51, Ojaswin Mujoo wrote:
> > > > > > No, its just something i hardcoded for that particular run. This patch
> > > > > > doesn't enforce hardware only atomic writes
> > > > > If we are to test this for XFS then we need to ensure that HW atomics are
> > > > > available.
> > > > Why is that? Now with the verification step happening after writes,
> > > > software atomic writes should also pass this test since there are no
> > > > racing writes to the verify reads.
> > > Sure, but racing software atomic writes against other software atomic writes
> > > is not safe.
> > >
> > > Thanks,
> > > John
> > What do you mean by not safe?
>
> Multiple threads issuing atomic writes may trample over one another.
>
> It is due to the steps used to issue an atomic write in xfs by software
> method. Here we do 3x steps:
> a. allocate blocks for out-of-place write
> b. do write in those blocks
> c. atomically update extent mapping.
>
> In this, threads wanting to atomic write to the same address will use the
> new blocks and can trample over one another before we atomically update the
> mapping.
So iiuc, w/ software fallback, a thread atomically writing to a range
will use a new block A. Another parallel thread trying to atomically
write to the same range will also use A, and there is no serialization
b/w the 2 so A could end up with a mix of data from both threads.
If this is true, aren't we violating the atomic guarantees. Nothing
prevents the userspace from doing overlapping parallel atomic writes and
it is kernels duty to error out if the write could get torn.
>
> So we do not guarantee serialization of atomic writes vs atomic writes. And
> this is why I said that this test is never totally safe for xfs.
>
> We could change this simply to have serialization of software-based atomic
> writes against all other dio, like follows:
>
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -747,6 +747,7 @@ xfs_file_dio_write_atomic(
> unsigned int iolock = XFS_IOLOCK_SHARED;
> ssize_t ret, ocount = iov_iter_count(from);
> const struct iomap_ops *dops;
> + unsigned int dio_flags = 0;
>
> /*
> * HW offload should be faster, so try that first if it is already
> @@ -766,15 +767,12 @@ xfs_file_dio_write_atomic(
> if (ret)
> goto out_unlock;
>
> - /* Demote similar to xfs_file_dio_write_aligned() */
> - if (iolock == XFS_IOLOCK_EXCL) {
> - xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
> - iolock = XFS_IOLOCK_SHARED;
> - }
> + if (dio_flags & IOMAP_DIO_FORCE_WAIT)
> + inode_dio_wait(VFS_I(ip));
>
> trace_xfs_file_direct_write(iocb, from);
> ret = iomap_dio_rw(iocb, from, dops, &xfs_dio_write_ops,
> - 0, NULL, 0);
> + dio_flags, NULL, 0);
>
> /*
> * The retry mechanism is based on the ->iomap_begin method returning
> @@ -785,6 +783,8 @@ xfs_file_dio_write_atomic(
> if (ret == -ENOPROTOOPT && dops == &xfs_direct_write_iomap_ops) {
> xfs_iunlock(ip, iolock);
> dops = &xfs_atomic_write_cow_iomap_ops;
> + iolock = XFS_IOLOCK_EXCL;
> + dio_flags = IOMAP_DIO_FORCE_WAIT;
> goto retry;
> }
>
>
> But it may affect performance.
>
> > Does it mean the test can fail?
>
> Yes, but it is unlikely if we have HW atomics available. That is because we
> will rarely be using software-based atomic method, as HW method should often
> be possible.
Yes, but having a chance to tear the write when it is not possible is
not the right behavior.
>
On 28/07/2025 07:43, Ojaswin Mujoo wrote: >>> What do you mean by not safe? >> Multiple threads issuing atomic writes may trample over one another. >> >> It is due to the steps used to issue an atomic write in xfs by software >> method. Here we do 3x steps: >> a. allocate blocks for out-of-place write >> b. do write in those blocks >> c. atomically update extent mapping. >> >> In this, threads wanting to atomic write to the same address will use the >> new blocks and can trample over one another before we atomically update the >> mapping. > So iiuc, w/ software fallback, a thread atomically writing to a range > will use a new block A. Another parallel thread trying to atomically > write to the same range will also use A, and there is no serialization > b/w the 2 so A could end up with a mix of data from both threads. right > > If this is true, aren't we violating the atomic guarantees. Nothing > prevents the userspace from doing overlapping parallel atomic writes and > it is kernels duty to error out if the write could get torn. Correct, but simply userspace should not do this. Direct I/O applications are responsible for ordering. We guarantee that the write is committed all-or-nothing, but do rely on userspace not issuing racing atomic writes or racing regular writes. I can easily change this, as I mentioned, but I am not convinced that it is a must. Thanks, John
On Mon, Jul 28, 2025 at 10:09:47AM +0100, John Garry wrote: > On 28/07/2025 07:43, Ojaswin Mujoo wrote: > > > > What do you mean by not safe? > > > Multiple threads issuing atomic writes may trample over one another. > > > > > > It is due to the steps used to issue an atomic write in xfs by software > > > method. Here we do 3x steps: > > > a. allocate blocks for out-of-place write > > > b. do write in those blocks > > > c. atomically update extent mapping. > > > > > > In this, threads wanting to atomic write to the same address will use the > > > new blocks and can trample over one another before we atomically update the > > > mapping. > > So iiuc, w/ software fallback, a thread atomically writing to a range > > will use a new block A. Another parallel thread trying to atomically > > write to the same range will also use A, and there is no serialization > > b/w the 2 so A could end up with a mix of data from both threads. > > right > > > > > If this is true, aren't we violating the atomic guarantees. Nothing > > prevents the userspace from doing overlapping parallel atomic writes and > > it is kernels duty to error out if the write could get torn. > > Correct, but simply userspace should not do this. Direct I/O applications > are responsible for ordering. > > We guarantee that the write is committed all-or-nothing, but do rely on > userspace not issuing racing atomic writes or racing regular writes. > > I can easily change this, as I mentioned, but I am not convinced that it is > a must. Purely from a design point of view, I feel we are breaking atomicity and hence we should serialize or just stop userspace from doing this (which is a bit extreme). I know userspace should ideally not do overwriting atomic writes but if it is something we are allowing (which we do) then it is kernel's responsibility to ensure atomicity. Sure we can penalize them by serializing the writes but not by tearing it. With that reasoning, I don't think the test should accomodate for this particular scenario. Regards, ojaswin
On 28/07/2025 14:35, Ojaswin Mujoo wrote: >> We guarantee that the write is committed all-or-nothing, but do rely on >> userspace not issuing racing atomic writes or racing regular writes. >> >> I can easily change this, as I mentioned, but I am not convinced that it is >> a must. > Purely from a design point of view, I feel we are breaking atomicity and > hence we should serialize or just stop userspace from doing this (which > is a bit extreme). If you check the man page description of RWF_ATOMIC, it does not mention serialization. The user should conclude that usual direct IO rules apply, i.e. userspace is responsible for serializing. > > I know userspace should ideally not do overwriting atomic writes but if > it is something we are allowing (which we do) then it is > kernel's responsibility to ensure atomicity. Sure we can penalize them > by serializing the writes but not by tearing it. > > With that reasoning, I don't think the test should accomodate for this > particular scenario. I can send a patch to the community for xfs (to provide serialization), like I showed earlier, to get opinion. Thanks, John
On Mon, Jul 28, 2025 at 03:00:40PM +0100, John Garry wrote: > On 28/07/2025 14:35, Ojaswin Mujoo wrote: > > > We guarantee that the write is committed all-or-nothing, but do rely on > > > userspace not issuing racing atomic writes or racing regular writes. > > > > > > I can easily change this, as I mentioned, but I am not convinced that it is > > > a must. > > Purely from a design point of view, I feel we are breaking atomicity and > > hence we should serialize or just stop userspace from doing this (which > > is a bit extreme). > > If you check the man page description of RWF_ATOMIC, it does not mention > serialization. The user should conclude that usual direct IO rules apply, > i.e. userspace is responsible for serializing. My mental model of serialization in context of atomic writes is that if user does 64k atomic write A followed by a parallel overlapping 64kb atomic write B then the user might see complete A or complete B (we don't guarantee) but not a mix of A and B. > > > > > I know userspace should ideally not do overwriting atomic writes but if > > it is something we are allowing (which we do) then it is > > kernel's responsibility to ensure atomicity. Sure we can penalize them > > by serializing the writes but not by tearing it. > > > > With that reasoning, I don't think the test should accomodate for this > > particular scenario. > > I can send a patch to the community for xfs (to provide serialization), like > I showed earlier, to get opinion. Thanks, that would be great. Regards, John > > Thanks, > John >
On Tue, Jul 29, 2025 at 11:41:39AM +0530, Ojaswin Mujoo wrote: > On Mon, Jul 28, 2025 at 03:00:40PM +0100, John Garry wrote: > > On 28/07/2025 14:35, Ojaswin Mujoo wrote: > > > > We guarantee that the write is committed all-or-nothing, but do rely on > > > > userspace not issuing racing atomic writes or racing regular writes. > > > > > > > > I can easily change this, as I mentioned, but I am not convinced that it is > > > > a must. > > > Purely from a design point of view, I feel we are breaking atomicity and > > > hence we should serialize or just stop userspace from doing this (which > > > is a bit extreme). > > > > If you check the man page description of RWF_ATOMIC, it does not mention > > serialization. The user should conclude that usual direct IO rules apply, > > i.e. userspace is responsible for serializing. > > My mental model of serialization in context of atomic writes is that if > user does 64k atomic write A followed by a parallel overlapping 64kb > atomic write B then the user might see complete A or complete B (we > don't guarantee) but not a mix of A and B. Heh, here comes that feature naming confusing again. This is my definition: RWF_ATOMIC means the system won't introduce new tearing when persisting file writes. The application is allowed to introduce tearing by writing to overlapping ranges at the same time. The system does not isolate overlapping reads from writes. --D > > > > > > > > I know userspace should ideally not do overwriting atomic writes but if > > > it is something we are allowing (which we do) then it is > > > kernel's responsibility to ensure atomicity. Sure we can penalize them > > > by serializing the writes but not by tearing it. > > > > > > With that reasoning, I don't think the test should accomodate for this > > > particular scenario. > > > > I can send a patch to the community for xfs (to provide serialization), like > > I showed earlier, to get opinion. > > Thanks, that would be great. > > Regards, > John > > > > Thanks, > > John > > >
On Tue, Jul 29, 2025 at 07:45:26AM -0700, Darrick J. Wong wrote: > On Tue, Jul 29, 2025 at 11:41:39AM +0530, Ojaswin Mujoo wrote: > > On Mon, Jul 28, 2025 at 03:00:40PM +0100, John Garry wrote: > > > On 28/07/2025 14:35, Ojaswin Mujoo wrote: > > > > > We guarantee that the write is committed all-or-nothing, but do rely on > > > > > userspace not issuing racing atomic writes or racing regular writes. > > > > > > > > > > I can easily change this, as I mentioned, but I am not convinced that it is > > > > > a must. > > > > Purely from a design point of view, I feel we are breaking atomicity and > > > > hence we should serialize or just stop userspace from doing this (which > > > > is a bit extreme). > > > > > > If you check the man page description of RWF_ATOMIC, it does not mention > > > serialization. The user should conclude that usual direct IO rules apply, > > > i.e. userspace is responsible for serializing. > > > > My mental model of serialization in context of atomic writes is that if > > user does 64k atomic write A followed by a parallel overlapping 64kb > > atomic write B then the user might see complete A or complete B (we > > don't guarantee) but not a mix of A and B. > > Heh, here comes that feature naming confusing again. This is my > definition: > > RWF_ATOMIC means the system won't introduce new tearing when persisting > file writes. The application is allowed to introduce tearing by writing > to overlapping ranges at the same time. The system does not isolate > overlapping reads from writes. > > --D Hey Darrick, John, So seems like my expectations of RWF_ATOMIC were a bit misplaced. I understand now that we don't really guarantee much when there are overlapping parallel writes going on. Even 2 overlapping RWF_ATOMIC writes can get torn. Seems like there are some edge cases where this might happen with hardware atomic writes as well. In that sense, if this fio test is doing overlapped atomic io and expecting them to be untorn, I don't think that's the correct way to test it since that is beyond what RWF_ATOMIC guarantees. I'll try to check if we can modify the tests to write on non-overlapping ranges in a file. Thanks and sorry for the confusion :) Ojaswin > > > > > > > > > > > > I know userspace should ideally not do overwriting atomic writes but if > > > > it is something we are allowing (which we do) then it is > > > > kernel's responsibility to ensure atomicity. Sure we can penalize them > > > > by serializing the writes but not by tearing it. > > > > > > > > With that reasoning, I don't think the test should accomodate for this > > > > particular scenario. > > > > > > I can send a patch to the community for xfs (to provide serialization), like > > > I showed earlier, to get opinion. > > > > Thanks, that would be great. > > > > Regards, > > John > > > > > > Thanks, > > > John > > > > >
On 31/07/2025 05:18, Ojaswin Mujoo wrote: >> Heh, here comes that feature naming confusing again. This is my >> definition: >> >> RWF_ATOMIC means the system won't introduce new tearing when persisting >> file writes. The application is allowed to introduce tearing by writing >> to overlapping ranges at the same time. The system does not isolate >> overlapping reads from writes. >> >> --D > Hey Darrick, John, > > So seems like my expectations of RWF_ATOMIC were a bit misplaced. I > understand now that we don't really guarantee much when there are > overlapping parallel writes going on. Even 2 overlapping RWF_ATOMIC > writes can get torn. Seems like there are some edge cases where this > might happen with hardware atomic writes as well. > > In that sense, if this fio test is doing overlapped atomic io and > expecting them to be untorn, I don't think that's the correct way to > test it since that is beyond what RWF_ATOMIC guarantees. I think that this test has value, but can only be used for ext4 or any FS which only relies on HW atomics only. The value is that we prove that we don't get any bios being split in the storage stack, which is essential for HW atomics support. Both NVMe and SCSI guarantee serialization of atomic writes. > > I'll try to check if we can modify the tests to write on non-overlapping > ranges in a file. JFYI, for testing SW-based atomic writes on XFS, I do something like this. I have multiple threads each writing to separate regions of a file or writing to separate files. I use this for power-fail testing with my RPI. Indeed, I have also being using this sort of test in qemu for shutting down the VM when fio is running - I would like to automate this, but I am not sure how yet. Please let me know if you want further info on the fio script. Thanks, John
On Thu, Jul 31, 2025 at 08:58:59AM +0100, John Garry wrote: > On 31/07/2025 05:18, Ojaswin Mujoo wrote: > > > Heh, here comes that feature naming confusing again. This is my > > > definition: > > > > > > RWF_ATOMIC means the system won't introduce new tearing when persisting > > > file writes. The application is allowed to introduce tearing by writing > > > to overlapping ranges at the same time. The system does not isolate > > > overlapping reads from writes. > > > > > > --D > > Hey Darrick, John, > > > > So seems like my expectations of RWF_ATOMIC were a bit misplaced. I > > understand now that we don't really guarantee much when there are > > overlapping parallel writes going on. Even 2 overlapping RWF_ATOMIC > > writes can get torn. Seems like there are some edge cases where this > > might happen with hardware atomic writes as well. > > > > In that sense, if this fio test is doing overlapped atomic io and > > expecting them to be untorn, I don't think that's the correct way to > > test it since that is beyond what RWF_ATOMIC guarantees. > > I think that this test has value, but can only be used for ext4 or any FS > which only relies on HW atomics only. > > The value is that we prove that we don't get any bios being split in the > storage stack, which is essential for HW atomics support. > > Both NVMe and SCSI guarantee serialization of atomic writes. Hi John, Got it, I think I can make this test work for ext4 only but then it might be more appropriate to run the fio tests directly on atomic blkdev and skip the FS, since we anyways want to focus on the storage stack. > > > > > I'll try to check if we can modify the tests to write on non-overlapping > > ranges in a file. > > JFYI, for testing SW-based atomic writes on XFS, I do something like this. I > have multiple threads each writing to separate regions of a file or writing > to separate files. I use this for power-fail testing with my RPI. Indeed, I > have also being using this sort of test in qemu for shutting down the VM > when fio is running - I would like to automate this, but I am not sure how > yet. > > Please let me know if you want further info on the fio script. Got it, thanks for the insights. I was thinking of something similar now where I can modify the fio files of this test to write on non overlapping ranges in the same file. The only doubt i have right now is that when I have eg, numjobs=10 filesize=1G, how do i ensure each job writes to its own separate range and not overlap with each other. I saw the offset_increment= fio options which might help, yet to try it out though. If you know any better way please do share. Thanks, Ojaswin > > Thanks, > John >
On 01/08/2025 07:41, Ojaswin Mujoo wrote: > Got it, I think I can make this test work for ext4 only but then it might > be more appropriate to run the fio tests directly on atomic blkdev and > skip the FS, since we anyways want to focus on the storage stack. > testing on ext4 will prove also that the FS and iomap behave correctly in that they generate a single bio per atomic write (as well as testing the block stack and below). >>> I'll try to check if we can modify the tests to write on non-overlapping >>> ranges in a file. >> JFYI, for testing SW-based atomic writes on XFS, I do something like this. I >> have multiple threads each writing to separate regions of a file or writing >> to separate files. I use this for power-fail testing with my RPI. Indeed, I >> have also being using this sort of test in qemu for shutting down the VM >> when fio is running - I would like to automate this, but I am not sure how >> yet. >> >> Please let me know if you want further info on the fio script. > Got it, thanks for the insights. I was thinking of something similar now > where I can modify the fio files of this test to write on non > overlapping ranges in the same file. The only doubt i have right now is > that when I have eg, numjobs=10 filesize=1G, how do i ensure each job > writes to its own separate range and not overlap with each other. > > I saw the offset_increment= fio options which might help, yet to try it > out though. If you know any better way please do share. Yeah, so I use something like: --numjobs=2 --offset_align=0 --offset_increment=1M --size=1M Thanks, John
On Fri, Aug 01, 2025 at 09:23:46AM +0100, John Garry wrote: > On 01/08/2025 07:41, Ojaswin Mujoo wrote: > > Got it, I think I can make this test work for ext4 only but then it might > > be more appropriate to run the fio tests directly on atomic blkdev and > > skip the FS, since we anyways want to focus on the storage stack. > > > > testing on ext4 will prove also that the FS and iomap behave correctly in > that they generate a single bio per atomic write (as well as testing the > block stack and below). Okay, I think we are already testing those in the ext4/061 ext4/062 tests of this patchset. Just thought blkdev test might be useful to keep in generic. Do you see a value in that or shall I just drop the generic overlapping write tests? Also, just for the records, ext4 passes the fio tests ONLY because we use the same io size for all threads. If we happen to start overlapping RWF_ATOMIC writes with different sizes that can get torn due to racing unwritten conversion. > > > > > I'll try to check if we can modify the tests to write on non-overlapping > > > > ranges in a file. > > > JFYI, for testing SW-based atomic writes on XFS, I do something like this. I > > > have multiple threads each writing to separate regions of a file or writing > > > to separate files. I use this for power-fail testing with my RPI. Indeed, I > > > have also being using this sort of test in qemu for shutting down the VM > > > when fio is running - I would like to automate this, but I am not sure how > > > yet. > > > > > > Please let me know if you want further info on the fio script. > > Got it, thanks for the insights. I was thinking of something similar now > > where I can modify the fio files of this test to write on non > > overlapping ranges in the same file. The only doubt i have right now is > > that when I have eg, numjobs=10 filesize=1G, how do i ensure each job > > writes to its own separate range and not overlap with each other. > > > > I saw the offset_increment= fio options which might help, yet to try it > > out though. If you know any better way please do share. > > Yeah, so I use something like: > --numjobs=2 --offset_align=0 --offset_increment=1M --size=1M Got it, thanks! ojaswin > > Thanks, > John >
On 02/08/2025 07:49, Ojaswin Mujoo wrote: > On Fri, Aug 01, 2025 at 09:23:46AM +0100, John Garry wrote: >> On 01/08/2025 07:41, Ojaswin Mujoo wrote: >>> Got it, I think I can make this test work for ext4 only but then it might >>> be more appropriate to run the fio tests directly on atomic blkdev and >>> skip the FS, since we anyways want to focus on the storage stack. >>> >> testing on ext4 will prove also that the FS and iomap behave correctly in >> that they generate a single bio per atomic write (as well as testing the >> block stack and below). > Okay, I think we are already testing those in the ext4/061 ext4/062 > tests of this patchset. Just thought blkdev test might be useful to keep > in generic. Do you see a value in that or shall I just drop the generic > overlapping write tests? If you want to just test fio on the blkdev, then I think that is fine. Indeed, maybe such tests are useful in blktests also. > > Also, just for the records, ext4 passes the fio tests ONLY because we use > the same io size for all threads. If we happen to start overlapping > RWF_ATOMIC writes with different sizes that can get torn due to racing > unwritten conversion. I'd keep the same io size for all threads in the tests. Thanks, John
On Mon, Aug 04, 2025 at 08:12:00AM +0100, John Garry wrote: > On 02/08/2025 07:49, Ojaswin Mujoo wrote: > > On Fri, Aug 01, 2025 at 09:23:46AM +0100, John Garry wrote: > > > On 01/08/2025 07:41, Ojaswin Mujoo wrote: > > > > Got it, I think I can make this test work for ext4 only but then it might > > > > be more appropriate to run the fio tests directly on atomic blkdev and > > > > skip the FS, since we anyways want to focus on the storage stack. > > > > > > > testing on ext4 will prove also that the FS and iomap behave correctly in > > > that they generate a single bio per atomic write (as well as testing the > > > block stack and below). > > Okay, I think we are already testing those in the ext4/061 ext4/062 > > tests of this patchset. Just thought blkdev test might be useful to keep > > in generic. Do you see a value in that or shall I just drop the generic > > overlapping write tests? > > If you want to just test fio on the blkdev, then I think that is fine. > Indeed, maybe such tests are useful in blktests also. Okay, I think it is better suited for blktests, so I'll add it there. > > > > > Also, just for the records, ext4 passes the fio tests ONLY because we use > > the same io size for all threads. If we happen to start overlapping > > RWF_ATOMIC writes with different sizes that can get torn due to racing > > unwritten conversion. > > I'd keep the same io size for all threads in the tests. Yep Thanks, Ojaswin > > Thanks, > John
© 2016 - 2025 Red Hat, Inc.