[PATCH v5 02/12] common/rc: Add _require_fio_version helper

Ojaswin Mujoo posted 11 patches 5 months, 2 weeks ago
There is a newer version of this series
[PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months, 2 weeks ago
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
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by John Garry 5 months, 1 week ago
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
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Zorro Lang 5 months ago
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?
> 
> 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.

(Sorry, I made a half reply in my last email)

This looks better than only using _require_fio_version. But the nature is still
checking fio version. If we don't have a better idea to check if fio really
support atomic writes, the _require_fio_version is still needed.
Or we rename it to "__require_fio_version" (one more "_"), to mark it's
not recommended using directly. But that looks a bit like a trick :-D

Thanks,
Zorro


> 
> Thanks,
> John
> 
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months ago
On Sun, Sep 07, 2025 at 01:29:43PM +0800, Zorro Lang wrote:
> 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?
> > 
> > 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.
> 
> (Sorry, I made a half reply in my last email)
> 
> This looks better than only using _require_fio_version. But the nature is still
> checking fio version. If we don't have a better idea to check if fio really
> support atomic writes, the _require_fio_version is still needed.
> Or we rename it to "__require_fio_version" (one more "_"), to mark it's
> not recommended using directly. But that looks a bit like a trick :-D
> 
> Thanks,
> Zorro

Hey Zorro, I agree with your points that version might not be the best
indicator esp for downstream software, but at this point I'm unsure
what's the workaround. 

One thing that comes to mind is to let fio do the atomic write and use
the tracepoints to confirm if RWF_ATOMIC was passed, but that adds a lot
of dependency on tracing framework being present (im unsure if something
like this is used somewhere in xfstests before). Further it's messy to
figure out that out of all the IO fio command will do, which one to
check for RWF_ATOMIC.

It can be done I suppose but is this sort of complexity something we
want to add is the question. Or do we just go ahead with the version
check.

Regards,
ojaswin

> 
> 
> > 
> > Thanks,
> > John
> > 
> > 
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by John Garry 5 months ago
On 09/09/2025 08:16, 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?
>>>
>>> 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.
>> (Sorry, I made a half reply in my last email)
>>
>> This looks better than only using _require_fio_version. But the nature is still
>> checking fio version. If we don't have a better idea to check if fio really
>> support atomic writes, the _require_fio_version is still needed.
>> Or we rename it to "__require_fio_version" (one more "_"), to mark it's
>> not recommended using directly. But that looks a bit like a trick 😂
>>
>> Thanks,
>> Zorro
> Hey Zorro, I agree with your points that version might not be the best
> indicator esp for downstream software, but at this point I'm unsure
> what's the workaround.
> 
> One thing that comes to mind is to let fio do the atomic write and use
> the tracepoints to confirm if RWF_ATOMIC was passed, but that adds a lot
> of dependency on tracing framework being present (im unsure if something
> like this is used somewhere in xfstests before). Further it's messy to
> figure out that out of all the IO fio command will do, which one to
> check for RWF_ATOMIC.
> 
> It can be done I suppose but is this sort of complexity something we
> want to add is the question. Or do we just go ahead with the version
> check.

I think that just checking the version is fine for this specific 
feature. But I still also think that versioning should be hidden from 
the end user, i.e. we should provide a helper like 
_require_fio_atomic_writes

thanks,
John
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months ago
On Tue, Sep 09, 2025 at 08:26:52AM +0100, John Garry wrote:
> On 09/09/2025 08:16, 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?
> > > > 
> > > > 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.
> > > (Sorry, I made a half reply in my last email)
> > > 
> > > This looks better than only using _require_fio_version. But the nature is still
> > > checking fio version. If we don't have a better idea to check if fio really
> > > support atomic writes, the _require_fio_version is still needed.
> > > Or we rename it to "__require_fio_version" (one more "_"), to mark it's
> > > not recommended using directly. But that looks a bit like a trick 😂
> > > 
> > > Thanks,
> > > Zorro
> > Hey Zorro, I agree with your points that version might not be the best
> > indicator esp for downstream software, but at this point I'm unsure
> > what's the workaround.
> > 
> > One thing that comes to mind is to let fio do the atomic write and use
> > the tracepoints to confirm if RWF_ATOMIC was passed, but that adds a lot
> > of dependency on tracing framework being present (im unsure if something
> > like this is used somewhere in xfstests before). Further it's messy to
> > figure out that out of all the IO fio command will do, which one to
> > check for RWF_ATOMIC.
> > 
> > It can be done I suppose but is this sort of complexity something we
> > want to add is the question. Or do we just go ahead with the version
> > check.
> 
> I think that just checking the version is fine for this specific feature.
> But I still also think that versioning should be hidden from the end user,
> i.e. we should provide a helper like _require_fio_atomic_writes

Sure, I'm okay. @Zorro, does that sound okay to you?
> 
> thanks,
> John
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Zorro Lang 5 months ago
On Tue, Sep 09, 2025 at 02:32:36PM +0530, Ojaswin Mujoo wrote:
> On Tue, Sep 09, 2025 at 08:26:52AM +0100, John Garry wrote:
> > On 09/09/2025 08:16, 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?
> > > > > 
> > > > > 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.
> > > > (Sorry, I made a half reply in my last email)
> > > > 
> > > > This looks better than only using _require_fio_version. But the nature is still
> > > > checking fio version. If we don't have a better idea to check if fio really
> > > > support atomic writes, the _require_fio_version is still needed.
> > > > Or we rename it to "__require_fio_version" (one more "_"), to mark it's
> > > > not recommended using directly. But that looks a bit like a trick 😂
> > > > 
> > > > Thanks,
> > > > Zorro
> > > Hey Zorro, I agree with your points that version might not be the best
> > > indicator esp for downstream software, but at this point I'm unsure
> > > what's the workaround.

Hi Ojaswin, I don't have better workaround than require_fio_version for now. I mean:
1) name _require_fio_version as __require_fio_version, to mark it's an internal function
   of another common function.
2) only call __require_fio_version in _require_fio_atomic_writes for now, don't use it
   in any test cases directly.

> > > 
> > > One thing that comes to mind is to let fio do the atomic write and use
> > > the tracepoints to confirm if RWF_ATOMIC was passed, but that adds a lot
> > > of dependency on tracing framework being present (im unsure if something
> > > like this is used somewhere in xfstests before). Further it's messy to
> > > figure out that out of all the IO fio command will do, which one to
> > > check for RWF_ATOMIC.
> > > 
> > > It can be done I suppose but is this sort of complexity something we
> > > want to add is the question. Or do we just go ahead with the version
> > > check.
> > 
> > I think that just checking the version is fine for this specific feature.
> > But I still also think that versioning should be hidden from the end user,
> > i.e. we should provide a helper like _require_fio_atomic_writes
> 
> Sure, I'm okay. @Zorro, does that sound okay to you?

Sure, that's what I tried to say as above, sorry if I made you misunderstand :)

Thanks,
Zorro

> > 
> > thanks,
> > John
> 

Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months ago
On Wed, Sep 10, 2025 at 02:07:15PM +0800, Zorro Lang wrote:
> On Tue, Sep 09, 2025 at 02:32:36PM +0530, Ojaswin Mujoo wrote:
> > On Tue, Sep 09, 2025 at 08:26:52AM +0100, John Garry wrote:
> > > On 09/09/2025 08:16, 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?
> > > > > > 
> > > > > > 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.
> > > > > (Sorry, I made a half reply in my last email)
> > > > > 
> > > > > This looks better than only using _require_fio_version. But the nature is still
> > > > > checking fio version. If we don't have a better idea to check if fio really
> > > > > support atomic writes, the _require_fio_version is still needed.
> > > > > Or we rename it to "__require_fio_version" (one more "_"), to mark it's
> > > > > not recommended using directly. But that looks a bit like a trick 😂
> > > > > 
> > > > > Thanks,
> > > > > Zorro
> > > > Hey Zorro, I agree with your points that version might not be the best
> > > > indicator esp for downstream software, but at this point I'm unsure
> > > > what's the workaround.
> 
> Hi Ojaswin, I don't have better workaround than require_fio_version for now. I mean:
> 1) name _require_fio_version as __require_fio_version, to mark it's an internal function
>    of another common function.
> 2) only call __require_fio_version in _require_fio_atomic_writes for now, don't use it
>    in any test cases directly.

Got it, I'll make this change, thanks

> 
> > > > 
> > > > One thing that comes to mind is to let fio do the atomic write and use
> > > > the tracepoints to confirm if RWF_ATOMIC was passed, but that adds a lot
> > > > of dependency on tracing framework being present (im unsure if something
> > > > like this is used somewhere in xfstests before). Further it's messy to
> > > > figure out that out of all the IO fio command will do, which one to
> > > > check for RWF_ATOMIC.
> > > > 
> > > > It can be done I suppose but is this sort of complexity something we
> > > > want to add is the question. Or do we just go ahead with the version
> > > > check.
> > > 
> > > I think that just checking the version is fine for this specific feature.
> > > But I still also think that versioning should be hidden from the end user,
> > > i.e. we should provide a helper like _require_fio_atomic_writes
> > 
> > Sure, I'm okay. @Zorro, does that sound okay to you?
> 
> Sure, that's what I tried to say as above, sorry if I made you misunderstand :)

Right I was just confirming. Thanks for the review :)

Regards,
ojaswin
> 
> Thanks,
> Zorro
> 
> > > 
> > > thanks,
> > > John
> > 
> 
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Zorro Lang 5 months ago
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?

I don't like to use "version" to be the _require_ condition either. fstests always
recommend "checking if the feature/behavior is really supported" at first, not a
hard version limitation. Some old downstream software might backport new upstream
commits, the version is useless for them.

Thanks,
Zorro

> 
> 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
> 
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months ago
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

> 
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by John Garry 5 months ago
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
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months ago
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
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Zorro Lang 5 months, 2 weeks ago
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
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months, 1 week ago
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
> > 
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Darrick J. Wong 5 months, 1 week ago
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
> > > 
> > 
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months, 1 week ago
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
> > > > 
> > > 
> >
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Zorro Lang 5 months, 1 week ago
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
> > > > > 
> > > > 
> > > 
>
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by John Garry 5 months, 1 week ago
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
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Ojaswin Mujoo 5 months, 1 week ago
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
Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper
Posted by Zorro Lang 5 months, 1 week ago
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
>