The main motivation of adding this function on top of _require_fio is
that there has been a case in fio where atomic= option was added but
later it was changed to noop since kernel didn't yet have support for
atomic writes. It was then again utilized to do atomic writes in a later
version, once kernel got the support. Due to this there is a point in
fio where _require_fio w/ atomic=1 will succeed even though it would
not be doing atomic writes.

Hence, add an explicit helper to ensure tests to require specific
versions of fio to work past such issues.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 common/rc | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/common/rc b/common/rc
index 35a1c835..f45b9a38 100644
--- a/common/rc
+++ b/common/rc
@@ -5997,6 +5997,38 @@ _max() {
 	echo $ret
 }
 
+# Check the required fio version. Examples:
+#   _require_fio_version 3.38 (matches 3.38 only)
+#   _require_fio_version 3.38+ (matches 3.38 and above)
+#   _require_fio_version 3.38- (matches 3.38 and below)
+_require_fio_version() {
+	local req_ver="$1"
+	local fio_ver
+
+	_require_fio
+	_require_math
+
+	fio_ver=$(fio -v | cut -d"-" -f2)
+
+	case "$req_ver" in
+	*+)
+		req_ver=${req_ver%+}
+		test $(_math "$fio_ver >= $req_ver") -eq 1 || \
+			_notrun "need fio >= $req_ver (found $fio_ver)"
+		;;
+	*-)
+		req_ver=${req_ver%-}
+		test $(_math "$fio_ver <= $req_ver") -eq 1 || \
+			_notrun "need fio <= $req_ver (found $fio_ver)"
+		;;
+	*)
+		req_ver=${req_ver%-}
+		test $(_math "$fio_ver == $req_ver") -eq 1 || \
+			_notrun "need fio = $req_ver (found $fio_ver)"
+		;;
+	esac
+}
+
 ################################################################################
 # make sure this script returns success
 /bin/true
-- 
2.49.0

On 22/08/2025 09:02, Ojaswin Mujoo wrote:
> The main motivation of adding this function on top of _require_fio is
> that there has been a case in fio where atomic= option was added but
> later it was changed to noop since kernel didn't yet have support for
> atomic writes. It was then again utilized to do atomic writes in a later
> version, once kernel got the support. Due to this there is a point in
> fio where _require_fio w/ atomic=1 will succeed even though it would
> not be doing atomic writes.
> 
> Hence, add an explicit helper to ensure tests to require specific
> versions of fio to work past such issues.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>   common/rc | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 35a1c835..f45b9a38 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5997,6 +5997,38 @@ _max() {
>   	echo $ret
>   }
>   
> +# Check the required fio version. Examples:
> +#   _require_fio_version 3.38 (matches 3.38 only)
> +#   _require_fio_version 3.38+ (matches 3.38 and above)
> +#   _require_fio_version 3.38- (matches 3.38 and below)

This requires the user to know the version which corresponds to the 
feature. Is that how things are done for other such utilities and their 
versions vs features?

I was going to suggest exporting something like 
_require_fio_atomic_writes(), and _require_fio_atomic_writes() calls 
_require_fio_version() to check the version.

Thanks,
John

On Tue, Sep 02, 2025 at 03:50:10PM +0100, John Garry wrote:
> On 22/08/2025 09:02, Ojaswin Mujoo wrote:
> > The main motivation of adding this function on top of _require_fio is
> > that there has been a case in fio where atomic= option was added but
> > later it was changed to noop since kernel didn't yet have support for
> > atomic writes. It was then again utilized to do atomic writes in a later
> > version, once kernel got the support. Due to this there is a point in
> > fio where _require_fio w/ atomic=1 will succeed even though it would
> > not be doing atomic writes.
> > 
> > Hence, add an explicit helper to ensure tests to require specific
> > versions of fio to work past such issues.
> > 
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >   common/rc | 32 ++++++++++++++++++++++++++++++++
> >   1 file changed, 32 insertions(+)
> > 
> > diff --git a/common/rc b/common/rc
> > index 35a1c835..f45b9a38 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -5997,6 +5997,38 @@ _max() {
> >   	echo $ret
> >   }
> > +# Check the required fio version. Examples:
> > +#   _require_fio_version 3.38 (matches 3.38 only)
> > +#   _require_fio_version 3.38+ (matches 3.38 and above)
> > +#   _require_fio_version 3.38- (matches 3.38 and below)
> 
> This requires the user to know the version which corresponds to the feature.
> Is that how things are done for other such utilities and their versions vs
> features?

Hi John,

So there are not many such helpers but the 2 I could see were used this
way:

tests/btrfs/284:
   _require_btrfs_send_version 2

tests/nfs/001:
   _require_test_nfs_version 4

So I though of keeping it this way.

Regards,
ojaswin

> 
> I was going to suggest exporting something like
> _require_fio_atomic_writes(), and _require_fio_atomic_writes() calls
> _require_fio_version() to check the version.

> 
> Thanks,
> John

> 
>

On 05/09/2025 16:51, Ojaswin Mujoo wrote:
>> This requires the user to know the version which corresponds to the feature.
>> Is that how things are done for other such utilities and their versions vs
>> features?
> Hi John,
> 
> So there are not many such helpers but the 2 I could see were used this
> way:
> 
> tests/btrfs/284:
>     _require_btrfs_send_version 2
> 
> tests/nfs/001:
>     _require_test_nfs_version 4
> 
> So I though of keeping it this way.

What about the example of _require_xfs_io_command param, which checks if 
$param is supported?

We could have _require_fio_option atomics, which checks if a specific 
version is available which supports atomic? Or a more straightforward 
would be _require_fio_with_atomics.

Cheers

On Fri, Sep 05, 2025 at 05:14:47PM +0100, John Garry wrote:
> On 05/09/2025 16:51, Ojaswin Mujoo wrote:
> > > This requires the user to know the version which corresponds to the feature.
> > > Is that how things are done for other such utilities and their versions vs
> > > features?
> > Hi John,
> > 
> > So there are not many such helpers but the 2 I could see were used this
> > way:
> > 
> > tests/btrfs/284:
> >     _require_btrfs_send_version 2
> > 
> > tests/nfs/001:
> >     _require_test_nfs_version 4
> > 
> > So I though of keeping it this way.
> 
> What about the example of _require_xfs_io_command param, which checks if
> $param is supported?
> 
> We could have _require_fio_option atomics, which checks if a specific
> version is available which supports atomic? Or a more straightforward would
> be _require_fio_with_atomics.

Hey John,

Sure Im okay with having a high level helper. I liked the name you
previously suggested:

  _require_fio_atomic_writes() {
    _require_fio_version 3.38+
  }

And the tests could use it as:

  _require_fio_atomic_writes()
  fio_config="abc.fio"
  _require_fio $fio_config

------------------------

OR would you prefer:

  _require_fio_atomic_writes() {
    _require_fio_version 3.38+
    _require_fio $fio_config
  }

And the tests could use it as:

  fio_config="abc.fio"
  _require_fio_atomic_writes $fio_config

------------------------

Let me know which one would you prefer.

Regards,
ojaswin

> 
> Cheers
>

On Fri, Aug 22, 2025 at 01:32:01PM +0530, Ojaswin Mujoo wrote:
> The main motivation of adding this function on top of _require_fio is
> that there has been a case in fio where atomic= option was added but
> later it was changed to noop since kernel didn't yet have support for
> atomic writes. It was then again utilized to do atomic writes in a later
> version, once kernel got the support. Due to this there is a point in
> fio where _require_fio w/ atomic=1 will succeed even though it would
> not be doing atomic writes.
> 
> Hence, add an explicit helper to ensure tests to require specific
> versions of fio to work past such issues.

Actually I'm wondering if fstests really needs to care about this. This's
just a temporary issue of fio, not kernel or any fs usespace program. Do
we need to add a seperated helper only for a temporary fio issue? If fio
doesn't break fstests running, let it run. Just the testers install proper
fio (maybe latest) they need. What do you and others think?

Thanks,
Zorro

> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  common/rc | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 35a1c835..f45b9a38 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5997,6 +5997,38 @@ _max() {
>  	echo $ret
>  }
>  
> +# Check the required fio version. Examples:
> +#   _require_fio_version 3.38 (matches 3.38 only)
> +#   _require_fio_version 3.38+ (matches 3.38 and above)
> +#   _require_fio_version 3.38- (matches 3.38 and below)
> +_require_fio_version() {
> +	local req_ver="$1"
> +	local fio_ver
> +
> +	_require_fio
> +	_require_math
> +
> +	fio_ver=$(fio -v | cut -d"-" -f2)
> +
> +	case "$req_ver" in
> +	*+)
> +		req_ver=${req_ver%+}
> +		test $(_math "$fio_ver >= $req_ver") -eq 1 || \
> +			_notrun "need fio >= $req_ver (found $fio_ver)"
> +		;;
> +	*-)
> +		req_ver=${req_ver%-}
> +		test $(_math "$fio_ver <= $req_ver") -eq 1 || \
> +			_notrun "need fio <= $req_ver (found $fio_ver)"
> +		;;
> +	*)
> +		req_ver=${req_ver%-}
> +		test $(_math "$fio_ver == $req_ver") -eq 1 || \
> +			_notrun "need fio = $req_ver (found $fio_ver)"
> +		;;
> +	esac
> +}
> +
>  ################################################################################
>  # make sure this script returns success
>  /bin/true
> -- 
> 2.49.0
>

On Tue, Aug 26, 2025 at 12:08:01AM +0800, Zorro Lang wrote:
> On Fri, Aug 22, 2025 at 01:32:01PM +0530, Ojaswin Mujoo wrote:
> > The main motivation of adding this function on top of _require_fio is
> > that there has been a case in fio where atomic= option was added but
> > later it was changed to noop since kernel didn't yet have support for
> > atomic writes. It was then again utilized to do atomic writes in a later
> > version, once kernel got the support. Due to this there is a point in
> > fio where _require_fio w/ atomic=1 will succeed even though it would
> > not be doing atomic writes.
> > 
> > Hence, add an explicit helper to ensure tests to require specific
> > versions of fio to work past such issues.
> 
> Actually I'm wondering if fstests really needs to care about this. This's
> just a temporary issue of fio, not kernel or any fs usespace program. Do
> we need to add a seperated helper only for a temporary fio issue? If fio
> doesn't break fstests running, let it run. Just the testers install proper
> fio (maybe latest) they need. What do you and others think?
> 
> Thanks,
> Zorro

Hey Zorro,

Sure I'm okay with not keeping the helper and letting the user make sure
the fio version is correct.

@John, does that sound okay?

Regards,
ojaswin
> 
> > 
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >  common/rc | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/common/rc b/common/rc
> > index 35a1c835..f45b9a38 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -5997,6 +5997,38 @@ _max() {
> >  	echo $ret
> >  }
> >  
> > +# Check the required fio version. Examples:
> > +#   _require_fio_version 3.38 (matches 3.38 only)
> > +#   _require_fio_version 3.38+ (matches 3.38 and above)
> > +#   _require_fio_version 3.38- (matches 3.38 and below)
> > +_require_fio_version() {
> > +	local req_ver="$1"
> > +	local fio_ver
> > +
> > +	_require_fio
> > +	_require_math
> > +
> > +	fio_ver=$(fio -v | cut -d"-" -f2)
> > +
> > +	case "$req_ver" in
> > +	*+)
> > +		req_ver=${req_ver%+}
> > +		test $(_math "$fio_ver >= $req_ver") -eq 1 || \
> > +			_notrun "need fio >= $req_ver (found $fio_ver)"
> > +		;;
> > +	*-)
> > +		req_ver=${req_ver%-}
> > +		test $(_math "$fio_ver <= $req_ver") -eq 1 || \
> > +			_notrun "need fio <= $req_ver (found $fio_ver)"
> > +		;;
> > +	*)
> > +		req_ver=${req_ver%-}
> > +		test $(_math "$fio_ver == $req_ver") -eq 1 || \
> > +			_notrun "need fio = $req_ver (found $fio_ver)"
> > +		;;
> > +	esac
> > +}
> > +
> >  ################################################################################
> >  # make sure this script returns success
> >  /bin/true
> > -- 
> > 2.49.0
> > 
>

On Wed, Aug 27, 2025 at 08:46:34PM +0530, Ojaswin Mujoo wrote:
> On Tue, Aug 26, 2025 at 12:08:01AM +0800, Zorro Lang wrote:
> > On Fri, Aug 22, 2025 at 01:32:01PM +0530, Ojaswin Mujoo wrote:
> > > The main motivation of adding this function on top of _require_fio is
> > > that there has been a case in fio where atomic= option was added but
> > > later it was changed to noop since kernel didn't yet have support for
> > > atomic writes. It was then again utilized to do atomic writes in a later
> > > version, once kernel got the support. Due to this there is a point in
> > > fio where _require_fio w/ atomic=1 will succeed even though it would
> > > not be doing atomic writes.
> > > 
> > > Hence, add an explicit helper to ensure tests to require specific
> > > versions of fio to work past such issues.
> > 
> > Actually I'm wondering if fstests really needs to care about this. This's
> > just a temporary issue of fio, not kernel or any fs usespace program. Do
> > we need to add a seperated helper only for a temporary fio issue? If fio
> > doesn't break fstests running, let it run. Just the testers install proper
> > fio (maybe latest) they need. What do you and others think?

Are there obvious failures if you try to run these new atomic write
tests on a system with the weird versions of fio that have the no-op
atomic= functionality?  I'm concerned that some QA person is going to do
that unwittingly and report that everything is ok when in reality they
didn't actually test anything.

--D

> > Thanks,
> > Zorro
> 
> Hey Zorro,
> 
> Sure I'm okay with not keeping the helper and letting the user make sure
> the fio version is correct.
> 
> @John, does that sound okay?
> 
> Regards,
> ojaswin
> > 
> > > 
> > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > ---
> > >  common/rc | 32 ++++++++++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 35a1c835..f45b9a38 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -5997,6 +5997,38 @@ _max() {
> > >  	echo $ret
> > >  }
> > >  
> > > +# Check the required fio version. Examples:
> > > +#   _require_fio_version 3.38 (matches 3.38 only)
> > > +#   _require_fio_version 3.38+ (matches 3.38 and above)
> > > +#   _require_fio_version 3.38- (matches 3.38 and below)
> > > +_require_fio_version() {
> > > +	local req_ver="$1"
> > > +	local fio_ver
> > > +
> > > +	_require_fio
> > > +	_require_math
> > > +
> > > +	fio_ver=$(fio -v | cut -d"-" -f2)
> > > +
> > > +	case "$req_ver" in
> > > +	*+)
> > > +		req_ver=${req_ver%+}
> > > +		test $(_math "$fio_ver >= $req_ver") -eq 1 || \
> > > +			_notrun "need fio >= $req_ver (found $fio_ver)"
> > > +		;;
> > > +	*-)
> > > +		req_ver=${req_ver%-}
> > > +		test $(_math "$fio_ver <= $req_ver") -eq 1 || \
> > > +			_notrun "need fio <= $req_ver (found $fio_ver)"
> > > +		;;
> > > +	*)
> > > +		req_ver=${req_ver%-}
> > > +		test $(_math "$fio_ver == $req_ver") -eq 1 || \
> > > +			_notrun "need fio = $req_ver (found $fio_ver)"
> > > +		;;
> > > +	esac
> > > +}
> > > +
> > >  ################################################################################
> > >  # make sure this script returns success
> > >  /bin/true
> > > -- 
> > > 2.49.0
> > > 
> > 
>

On Thu, Aug 28, 2025 at 08:09:05AM -0700, Darrick J. Wong wrote:
> On Wed, Aug 27, 2025 at 08:46:34PM +0530, Ojaswin Mujoo wrote:
> > On Tue, Aug 26, 2025 at 12:08:01AM +0800, Zorro Lang wrote:
> > > On Fri, Aug 22, 2025 at 01:32:01PM +0530, Ojaswin Mujoo wrote:
> > > > The main motivation of adding this function on top of _require_fio is
> > > > that there has been a case in fio where atomic= option was added but
> > > > later it was changed to noop since kernel didn't yet have support for
> > > > atomic writes. It was then again utilized to do atomic writes in a later
> > > > version, once kernel got the support. Due to this there is a point in
> > > > fio where _require_fio w/ atomic=1 will succeed even though it would
> > > > not be doing atomic writes.
> > > > 
> > > > Hence, add an explicit helper to ensure tests to require specific
> > > > versions of fio to work past such issues.
> > > 
> > > Actually I'm wondering if fstests really needs to care about this. This's
> > > just a temporary issue of fio, not kernel or any fs usespace program. Do
> > > we need to add a seperated helper only for a temporary fio issue? If fio
> > > doesn't break fstests running, let it run. Just the testers install proper
> > > fio (maybe latest) they need. What do you and others think?
> 
> Are there obvious failures if you try to run these new atomic write
> tests on a system with the weird versions of fio that have the no-op
> atomic= functionality?  I'm concerned that some QA person is going to do
> that unwittingly and report that everything is ok when in reality they
> didn't actually test anything.

I think John has a bit more background but afaict, RWF_ATOMIC support
was added (fio commit: d01612f3ae25) but then removed (commit:
a25ba6c64fe1) since the feature didn't make it to kernel in time.
However the option seemed to be kept in place. Later, commit 40f1fc11d
added the support back in a later version of fio. 

So yes, I think there are some version where fio will accept atomic=1
but not act upon it and the tests may start failing with no apparent
reason.

Regards,
ojaswin
> 
> --D
> 
> > > Thanks,
> > > Zorro
> > 
> > Hey Zorro,
> > 
> > Sure I'm okay with not keeping the helper and letting the user make sure
> > the fio version is correct.
> > 
> > @John, does that sound okay?
> > 
> > Regards,
> > ojaswin
> > > 
> > > > 
> > > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > > ---
> > > >  common/rc | 32 ++++++++++++++++++++++++++++++++
> > > >  1 file changed, 32 insertions(+)
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index 35a1c835..f45b9a38 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -5997,6 +5997,38 @@ _max() {
> > > >  	echo $ret
> > > >  }
> > > >  
> > > > +# Check the required fio version. Examples:
> > > > +#   _require_fio_version 3.38 (matches 3.38 only)
> > > > +#   _require_fio_version 3.38+ (matches 3.38 and above)
> > > > +#   _require_fio_version 3.38- (matches 3.38 and below)
> > > > +_require_fio_version() {
> > > > +	local req_ver="$1"
> > > > +	local fio_ver
> > > > +
> > > > +	_require_fio
> > > > +	_require_math
> > > > +
> > > > +	fio_ver=$(fio -v | cut -d"-" -f2)
> > > > +
> > > > +	case "$req_ver" in
> > > > +	*+)
> > > > +		req_ver=${req_ver%+}
> > > > +		test $(_math "$fio_ver >= $req_ver") -eq 1 || \
> > > > +			_notrun "need fio >= $req_ver (found $fio_ver)"
> > > > +		;;
> > > > +	*-)
> > > > +		req_ver=${req_ver%-}
> > > > +		test $(_math "$fio_ver <= $req_ver") -eq 1 || \
> > > > +			_notrun "need fio <= $req_ver (found $fio_ver)"
> > > > +		;;
> > > > +	*)
> > > > +		req_ver=${req_ver%-}
> > > > +		test $(_math "$fio_ver == $req_ver") -eq 1 || \
> > > > +			_notrun "need fio = $req_ver (found $fio_ver)"
> > > > +		;;
> > > > +	esac
> > > > +}
> > > > +
> > > >  ################################################################################
> > > >  # make sure this script returns success
> > > >  /bin/true
> > > > -- 
> > > > 2.49.0
> > > > 
> > > 
> >

On Fri, Aug 29, 2025 at 10:29:47PM +0530, Ojaswin Mujoo wrote:
> On Thu, Aug 28, 2025 at 08:09:05AM -0700, Darrick J. Wong wrote:
> > On Wed, Aug 27, 2025 at 08:46:34PM +0530, Ojaswin Mujoo wrote:
> > > On Tue, Aug 26, 2025 at 12:08:01AM +0800, Zorro Lang wrote:
> > > > On Fri, Aug 22, 2025 at 01:32:01PM +0530, Ojaswin Mujoo wrote:
> > > > > The main motivation of adding this function on top of _require_fio is
> > > > > that there has been a case in fio where atomic= option was added but
> > > > > later it was changed to noop since kernel didn't yet have support for
> > > > > atomic writes. It was then again utilized to do atomic writes in a later
> > > > > version, once kernel got the support. Due to this there is a point in
> > > > > fio where _require_fio w/ atomic=1 will succeed even though it would
> > > > > not be doing atomic writes.
> > > > > 
> > > > > Hence, add an explicit helper to ensure tests to require specific
> > > > > versions of fio to work past such issues.
> > > > 
> > > > Actually I'm wondering if fstests really needs to care about this. This's
> > > > just a temporary issue of fio, not kernel or any fs usespace program. Do
> > > > we need to add a seperated helper only for a temporary fio issue? If fio
> > > > doesn't break fstests running, let it run. Just the testers install proper
> > > > fio (maybe latest) they need. What do you and others think?
> > 
> > Are there obvious failures if you try to run these new atomic write
> > tests on a system with the weird versions of fio that have the no-op
> > atomic= functionality?  I'm concerned that some QA person is going to do
> > that unwittingly and report that everything is ok when in reality they
> > didn't actually test anything.
> 
> I think John has a bit more background but afaict, RWF_ATOMIC support
> was added (fio commit: d01612f3ae25) but then removed (commit:
> a25ba6c64fe1) since the feature didn't make it to kernel in time.
> However the option seemed to be kept in place. Later, commit 40f1fc11d
> added the support back in a later version of fio. 
> 
> So yes, I think there are some version where fio will accept atomic=1
> but not act upon it and the tests may start failing with no apparent
> reason.

The concern from Darrick might be a problem. May I ask which fio commit
brought in this issue, and which fio commit fixed it? If this issue be
brought in and fixed within a fio release, it might be better. But if it
crosses fio release, that might be bad, then we might be better to have
this helper.

Thanks,
Zorro

> 
> Regards,
> ojaswin
> > 
> > --D
> > 
> > > > Thanks,
> > > > Zorro
> > > 
> > > Hey Zorro,
> > > 
> > > Sure I'm okay with not keeping the helper and letting the user make sure
> > > the fio version is correct.
> > > 
> > > @John, does that sound okay?
> > > 
> > > Regards,
> > > ojaswin
> > > > 
> > > > > 
> > > > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > > > ---
> > > > >  common/rc | 32 ++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 32 insertions(+)
> > > > > 
> > > > > diff --git a/common/rc b/common/rc
> > > > > index 35a1c835..f45b9a38 100644
> > > > > --- a/common/rc
> > > > > +++ b/common/rc
> > > > > @@ -5997,6 +5997,38 @@ _max() {
> > > > >  	echo $ret
> > > > >  }
> > > > >  
> > > > > +# Check the required fio version. Examples:
> > > > > +#   _require_fio_version 3.38 (matches 3.38 only)
> > > > > +#   _require_fio_version 3.38+ (matches 3.38 and above)
> > > > > +#   _require_fio_version 3.38- (matches 3.38 and below)
> > > > > +_require_fio_version() {
> > > > > +	local req_ver="$1"
> > > > > +	local fio_ver
> > > > > +
> > > > > +	_require_fio
> > > > > +	_require_math
> > > > > +
> > > > > +	fio_ver=$(fio -v | cut -d"-" -f2)
> > > > > +
> > > > > +	case "$req_ver" in
> > > > > +	*+)
> > > > > +		req_ver=${req_ver%+}
> > > > > +		test $(_math "$fio_ver >= $req_ver") -eq 1 || \
> > > > > +			_notrun "need fio >= $req_ver (found $fio_ver)"
> > > > > +		;;
> > > > > +	*-)
> > > > > +		req_ver=${req_ver%-}
> > > > > +		test $(_math "$fio_ver <= $req_ver") -eq 1 || \
> > > > > +			_notrun "need fio <= $req_ver (found $fio_ver)"
> > > > > +		;;
> > > > > +	*)
> > > > > +		req_ver=${req_ver%-}
> > > > > +		test $(_math "$fio_ver == $req_ver") -eq 1 || \
> > > > > +			_notrun "need fio = $req_ver (found $fio_ver)"
> > > > > +		;;
> > > > > +	esac
> > > > > +}
> > > > > +
> > > > >  ################################################################################
> > > > >  # make sure this script returns success
> > > > >  /bin/true
> > > > > -- 
> > > > > 2.49.0
> > > > > 
> > > > 
> > > 
>

On 30/08/2025 18:09, Zorro Lang wrote:
>> I think John has a bit more background but afaict, RWF_ATOMIC support
>> was added (fio commit: d01612f3ae25) but then removed (commit:
>> a25ba6c64fe1) since the feature didn't make it to kernel in time.
>> However the option seemed to be kept in place. Later, commit 40f1fc11d
>> added the support back in a later version of fio.
>>
>> So yes, I think there are some version where fio will accept atomic=1
>> but not act upon it and the tests may start failing with no apparent
>> reason.
> The concern from Darrick might be a problem. May I ask which fio commit
> brought in this issue, and which fio commit fixed it? If this issue be
> brought in and fixed within a fio release, it might be better. But if it
> crosses fio release, that might be bad, then we might be better to have
> this helper.

The history is that fio atomic write support was originally added some 
time ago for out-of-kernel atomic write support, which was O_ATOMIC 
flag. Since O_ATOMIC never made it into the kernel, the feature was 
removed, but the plumbing for atomic writes stayed in fio - specifically 
the "atomic=" option. So I just reused that plumbing in d01612f3ae25 to 
support RWF_ATOMIC.

The point is that we should check the fio version, as different versions 
can give different behaviour for "atomic" option, those being:
a. O_ATOMIC (we definitely don't want this)
b. no nothing (bad)
c. use RWF_ATOMIC

Thanks,
John

On Sun, Aug 31, 2025 at 01:09:07AM +0800, Zorro Lang wrote:
> On Fri, Aug 29, 2025 at 10:29:47PM +0530, Ojaswin Mujoo wrote:
> > On Thu, Aug 28, 2025 at 08:09:05AM -0700, Darrick J. Wong wrote:
> > > On Wed, Aug 27, 2025 at 08:46:34PM +0530, Ojaswin Mujoo wrote:
> > > > On Tue, Aug 26, 2025 at 12:08:01AM +0800, Zorro Lang wrote:
> > > > > On Fri, Aug 22, 2025 at 01:32:01PM +0530, Ojaswin Mujoo wrote:
> > > > > > The main motivation of adding this function on top of _require_fio is
> > > > > > that there has been a case in fio where atomic= option was added but
> > > > > > later it was changed to noop since kernel didn't yet have support for
> > > > > > atomic writes. It was then again utilized to do atomic writes in a later
> > > > > > version, once kernel got the support. Due to this there is a point in
> > > > > > fio where _require_fio w/ atomic=1 will succeed even though it would
> > > > > > not be doing atomic writes.
> > > > > > 
> > > > > > Hence, add an explicit helper to ensure tests to require specific
> > > > > > versions of fio to work past such issues.
> > > > > 
> > > > > Actually I'm wondering if fstests really needs to care about this. This's
> > > > > just a temporary issue of fio, not kernel or any fs usespace program. Do
> > > > > we need to add a seperated helper only for a temporary fio issue? If fio
> > > > > doesn't break fstests running, let it run. Just the testers install proper
> > > > > fio (maybe latest) they need. What do you and others think?
> > > 
> > > Are there obvious failures if you try to run these new atomic write
> > > tests on a system with the weird versions of fio that have the no-op
> > > atomic= functionality?  I'm concerned that some QA person is going to do
> > > that unwittingly and report that everything is ok when in reality they
> > > didn't actually test anything.
> > 
> > I think John has a bit more background but afaict, RWF_ATOMIC support
> > was added (fio commit: d01612f3ae25) but then removed (commit:
> > a25ba6c64fe1) since the feature didn't make it to kernel in time.
> > However the option seemed to be kept in place. Later, commit 40f1fc11d
> > added the support back in a later version of fio. 
> > 
> > So yes, I think there are some version where fio will accept atomic=1
> > but not act upon it and the tests may start failing with no apparent
> > reason.
> 
> The concern from Darrick might be a problem. May I ask which fio commit
> brought in this issue, and which fio commit fixed it? If this issue be
> brought in and fixed within a fio release, it might be better. But if it
> crosses fio release, that might be bad, then we might be better to have
> this helper.

Hi Zorro, yes it does seem to cross version boundaries. The
functionality was removed in fio v3.33 and added back in v3.38.  I
confirmed this by running generic/1226 with both (for v3.33 run i
commented out a few fio options that were added later but kept
atomic=1):

Command: sudo perf record -e iomap:iomap_dio_rw_begin ./check generic/1226

perf script sample with fio v3.33:

fio    6626 [000]   777.668017: iomap:iomap_dio_rw_begin: <.sniip.> flags DIRECT|WRITE|AIO_RW dio_flags  aio 1

perf script sample with fio v3.39:

fio    9830 [000]   895.042747: iomap:iomap_dio_rw_begin: <.snip> flags ATOMIC|DIRECT|WRITE|AIO_RW dio_flags  aio 1

So as we can see, even though the test passes with atomic=1, fio is not
sending the RWF_ATOMIC flag in the older version.

In which case I believe it should be okay to keep the helper, right?

Thanks,
Ojaswin

> 
> Thanks,
> Zorro
> 
> > 
> > Regards,
> > ojaswin
> > > 
> > > --D
> > > 
> > > > > Thanks,
> > > > > Zorro

On Mon, Sep 01, 2025 at 05:10:01PM +0530, Ojaswin Mujoo wrote:
> On Sun, Aug 31, 2025 at 01:09:07AM +0800, Zorro Lang wrote:
> > On Fri, Aug 29, 2025 at 10:29:47PM +0530, Ojaswin Mujoo wrote:
> > > On Thu, Aug 28, 2025 at 08:09:05AM -0700, Darrick J. Wong wrote:
> > > > On Wed, Aug 27, 2025 at 08:46:34PM +0530, Ojaswin Mujoo wrote:
> > > > > On Tue, Aug 26, 2025 at 12:08:01AM +0800, Zorro Lang wrote:
> > > > > > On Fri, Aug 22, 2025 at 01:32:01PM +0530, Ojaswin Mujoo wrote:
> > > > > > > The main motivation of adding this function on top of _require_fio is
> > > > > > > that there has been a case in fio where atomic= option was added but
> > > > > > > later it was changed to noop since kernel didn't yet have support for
> > > > > > > atomic writes. It was then again utilized to do atomic writes in a later
> > > > > > > version, once kernel got the support. Due to this there is a point in
> > > > > > > fio where _require_fio w/ atomic=1 will succeed even though it would
> > > > > > > not be doing atomic writes.
> > > > > > > 
> > > > > > > Hence, add an explicit helper to ensure tests to require specific
> > > > > > > versions of fio to work past such issues.
> > > > > > 
> > > > > > Actually I'm wondering if fstests really needs to care about this. This's
> > > > > > just a temporary issue of fio, not kernel or any fs usespace program. Do
> > > > > > we need to add a seperated helper only for a temporary fio issue? If fio
> > > > > > doesn't break fstests running, let it run. Just the testers install proper
> > > > > > fio (maybe latest) they need. What do you and others think?
> > > > 
> > > > Are there obvious failures if you try to run these new atomic write
> > > > tests on a system with the weird versions of fio that have the no-op
> > > > atomic= functionality?  I'm concerned that some QA person is going to do
> > > > that unwittingly and report that everything is ok when in reality they
> > > > didn't actually test anything.
> > > 
> > > I think John has a bit more background but afaict, RWF_ATOMIC support
> > > was added (fio commit: d01612f3ae25) but then removed (commit:
> > > a25ba6c64fe1) since the feature didn't make it to kernel in time.
> > > However the option seemed to be kept in place. Later, commit 40f1fc11d
> > > added the support back in a later version of fio. 
> > > 
> > > So yes, I think there are some version where fio will accept atomic=1
> > > but not act upon it and the tests may start failing with no apparent
> > > reason.
> > 
> > The concern from Darrick might be a problem. May I ask which fio commit
> > brought in this issue, and which fio commit fixed it? If this issue be
> > brought in and fixed within a fio release, it might be better. But if it
> > crosses fio release, that might be bad, then we might be better to have
> > this helper.
> 
> Hi Zorro, yes it does seem to cross version boundaries. The
> functionality was removed in fio v3.33 and added back in v3.38.  I

Thanks, if so I think let's have this helper for that issue :) But I think
we still prioritize _require_fio. If it helpless, then call _require_fio_version.

Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks,
Zorro

> confirmed this by running generic/1226 with both (for v3.33 run i
> commented out a few fio options that were added later but kept
> atomic=1):
> 
> Command: sudo perf record -e iomap:iomap_dio_rw_begin ./check generic/1226
> 
> perf script sample with fio v3.33:
> 
> fio    6626 [000]   777.668017: iomap:iomap_dio_rw_begin: <.sniip.> flags DIRECT|WRITE|AIO_RW dio_flags  aio 1
> 
> perf script sample with fio v3.39:
> 
> fio    9830 [000]   895.042747: iomap:iomap_dio_rw_begin: <.snip> flags ATOMIC|DIRECT|WRITE|AIO_RW dio_flags  aio 1
> 
> So as we can see, even though the test passes with atomic=1, fio is not
> sending the RWF_ATOMIC flag in the older version.
> 
> In which case I believe it should be okay to keep the helper, right?
> 
> Thanks,
> Ojaswin
> 
> > 
> > Thanks,
> > Zorro
> > 
> > > 
> > > Regards,
> > > ojaswin
> > > > 
> > > > --D
> > > > 
> > > > > > Thanks,
> > > > > > Zorro
>