common/rc: add the function _require_noatime

Commit Message

XiaoLi Feng May 18, 2018, 11:10 a.m. UTC

From: xiaoli feng <[e-mail]>

In the generic/120, it will make the test not-pass if the filesystem
mounts failed with noatime. Now change this result to norun. The
filesystem cifs doesn't support noatime. Just make the test norun
until it supports noatime.

Signed-off-by: xiaoli feng <[e-mail]>
---
 common/rc         | 8 +++++++-
 tests/generic/120 | 7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)

Comments

Eryu Guan May 21, 2018, 7:50 a.m. UTC | #1

[add [e-mail] to cc list]

On Fri, May 18, 2018 at 07:10:29PM +0800, Xiaoli Feng wrote:
> From: xiaoli feng <[e-mail]>
> 
> In the generic/120, it will make the test not-pass if the filesystem
> mounts failed with noatime. Now change this result to norun. The
> filesystem cifs doesn't support noatime. Just make the test norun
> until it supports noatime.
> 
> Signed-off-by: xiaoli feng <[e-mail]>
> ---
>  common/rc         | 8 +++++++-
>  tests/generic/120 | 7 +------
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index ffe5323..9c45f1b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3244,7 +3244,13 @@ _require_atime()
>  	_exclude_scratch_mount_option "noatime"
>  	if [ "$FSTYP" == "nfs" ]; then
>  		_notrun "atime related mount options have no effect on NFS"
> -	fi

I'm not sure what's the expected behavior from CIFS on atime/noatime, so
I added linux-cifs list for input.

If CIFS behaves similarly to NFS, looks like that you could simply add
another check for cifs in _require_atime(), as what we already do for
nfs, so all tests that _require_atime() will _notrun on cifs.

> +}
> +
> +_require_noatime()
> +{
> +	_exclude_scratch_mount_option "atime"
> +	 _try_scratch_mount -o noatime || \
> +                _notrun "noatime not supported by the current tested filesystem"
>  }
>  
>  _require_relatime()
> diff --git a/tests/generic/120 b/tests/generic/120
> index 1180c10..ddd61b3 100755
> --- a/tests/generic/120
> +++ b/tests/generic/120
> @@ -60,12 +60,7 @@ _compare_access_times()
>  
>  }
>  
> -if ! _try_scratch_mount "-o noatime" >$tmp.out 2>&1
> -then
> -    cat $tmp.out
> -    echo "!!! mount failed"
> -    exit
> -fi
> +_require_noatime

Anyway, failing the test when "noatime" mount fails is one of the
purposes of the test, and it shouldn't be removed, as we've already made
sure current FSTYP supports atime/noatime (by _require rules), so a
noatime mount failure indicates a bug in the filesystem.

Thanks,
Eryu

>  
>  #executable file
>  echo "*** copying file ***"
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

XiaoLi Feng May 21, 2018, 8:50 a.m. UTC | #2

[add [e-mail] to cc list]

----- Forwarded Message -----
From: "Eryu Guan" <[e-mail]>
To: "XiaoLi Feng" <[e-mail]>
Cc: [e-mail]
Sent: Monday, May 21, 2018 3:50:27 PM
Subject: Re: [PATCH] common/rc: add the function _require_noatime

[add [e-mail] to cc list]

On Fri, May 18, 2018 at 07:10:29PM +0800, Xiaoli Feng wrote:
> From: xiaoli feng <[e-mail]>
> 
> In the generic/120, it will make the test not-pass if the filesystem
> mounts failed with noatime. Now change this result to norun. The
> filesystem cifs doesn't support noatime. Just make the test norun
> until it supports noatime.
> 
> Signed-off-by: xiaoli feng <[e-mail]>
> ---
>  common/rc         | 8 +++++++-
>  tests/generic/120 | 7 +------
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index ffe5323..9c45f1b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3244,7 +3244,13 @@ _require_atime()
>  	_exclude_scratch_mount_option "noatime"
>  	if [ "$FSTYP" == "nfs" ]; then
>  		_notrun "atime related mount options have no effect on NFS"
> -	fi

I'm not sure what's the expected behavior from CIFS on atime/noatime, so
I added linux-cifs list for input.

If CIFS behaves similarly to NFS, looks like that you could simply add
another check for cifs in _require_atime(), as what we already do for
nfs, so all tests that _require_atime() will _notrun on cifs.

> +}
> +
> +_require_noatime()
> +{
> +	_exclude_scratch_mount_option "atime"
> +	 _try_scratch_mount -o noatime || \
> +                _notrun "noatime not supported by the current tested filesystem"
>  }
>  
>  _require_relatime()
> diff --git a/tests/generic/120 b/tests/generic/120
> index 1180c10..ddd61b3 100755
> --- a/tests/generic/120
> +++ b/tests/generic/120
> @@ -60,12 +60,7 @@ _compare_access_times()
>  
>  }
>  
> -if ! _try_scratch_mount "-o noatime" >$tmp.out 2>&1
> -then
> -    cat $tmp.out
> -    echo "!!! mount failed"
> -    exit
> -fi
> +_require_noatime

Anyway, failing the test when "noatime" mount fails is one of the
purposes of the test, and it shouldn't be removed, as we've already made
sure current FSTYP supports atime/noatime (by _require rules), so a
noatime mount failure indicates a bug in the filesystem.

Thanks,
Eryu

>  
>  #executable file
>  echo "*** copying file ***"
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve French May 21, 2018, 11:54 p.m. UTC | #3

Should this be fixed by changing cifs.ko to accept the mount parm but ignore it?

If this test works on NFS (the noatime mount option has no meaning for
NFS apparently) we should do the same

Quoting the NFS man page:

       In particular, the atime/noatime, diratime/nodiratime, relatime/norela‐
       time, and strictatime/nostrictatime mount options have no effect on NFS
       mounts.

On Mon, May 21, 2018 at 3:50 AM, Xiaoli Feng <[e-mail]> wrote:
>
> [add [e-mail] to cc list]
>
> ----- Forwarded Message -----
> From: "Eryu Guan" <[e-mail]>
> To: "XiaoLi Feng" <[e-mail]>
> Cc: [e-mail]
> Sent: Monday, May 21, 2018 3:50:27 PM
> Subject: Re: [PATCH] common/rc: add the function _require_noatime
>
> [add [e-mail] to cc list]
>
> On Fri, May 18, 2018 at 07:10:29PM +0800, Xiaoli Feng wrote:
>> From: xiaoli feng <[e-mail]>
>>
>> In the generic/120, it will make the test not-pass if the filesystem
>> mounts failed with noatime. Now change this result to norun. The
>> filesystem cifs doesn't support noatime. Just make the test norun
>> until it supports noatime.
>>
>> Signed-off-by: xiaoli feng <[e-mail]>
>> ---
>>  common/rc         | 8 +++++++-
>>  tests/generic/120 | 7 +------
>>  2 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index ffe5323..9c45f1b 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -3244,7 +3244,13 @@ _require_atime()
>>       _exclude_scratch_mount_option "noatime"
>>       if [ "$FSTYP" == "nfs" ]; then
>>               _notrun "atime related mount options have no effect on NFS"
>> -     fi
>
> I'm not sure what's the expected behavior from CIFS on atime/noatime, so
> I added linux-cifs list for input.
>
> If CIFS behaves similarly to NFS, looks like that you could simply add
> another check for cifs in _require_atime(), as what we already do for
> nfs, so all tests that _require_atime() will _notrun on cifs.
>
>> +}
>> +
>> +_require_noatime()
>> +{
>> +     _exclude_scratch_mount_option "atime"
>> +      _try_scratch_mount -o noatime || \
>> +                _notrun "noatime not supported by the current tested filesystem"
>>  }
>>
>>  _require_relatime()
>> diff --git a/tests/generic/120 b/tests/generic/120
>> index 1180c10..ddd61b3 100755
>> --- a/tests/generic/120
>> +++ b/tests/generic/120
>> @@ -60,12 +60,7 @@ _compare_access_times()
>>
>>  }
>>
>> -if ! _try_scratch_mount "-o noatime" >$tmp.out 2>&1
>> -then
>> -    cat $tmp.out
>> -    echo "!!! mount failed"
>> -    exit
>> -fi
>> +_require_noatime
>
> Anyway, failing the test when "noatime" mount fails is one of the
> purposes of the test, and it shouldn't be removed, as we've already made
> sure current FSTYP supports atime/noatime (by _require rules), so a
> noatime mount failure indicates a bug in the filesystem.
>
> Thanks,
> Eryu
>
>>
>>  #executable file
>>  echo "*** copying file ***"
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eryu Guan May 22, 2018, 2:21 a.m. UTC | #4

On Mon, May 21, 2018 at 06:54:45PM -0500, Steve French wrote:
> Should this be fixed by changing cifs.ko to accept the mount parm but ignore it?
> 
> If this test works on NFS (the noatime mount option has no meaning for
> NFS apparently) we should do the same

NFS works because tests that call _require_atime will _notrun on NFS.

> 
> Quoting the NFS man page:
> 
>        In particular, the atime/noatime, diratime/nodiratime, relatime/norela‐
>        time, and strictatime/nostrictatime mount options have no effect on NFS
>        mounts.

Yeah, the NFS check in _require_atime was based on this description. If
atime related mount options have no effect on CIFS too, we could simply,
from fstests' prospect of view, _notrun for CIFS in _require_atime too.

Thanks,
Eryu

> 
> On Mon, May 21, 2018 at 3:50 AM, Xiaoli Feng <[e-mail]> wrote:
> >
> > [add [e-mail] to cc list]
> >
> > ----- Forwarded Message -----
> > From: "Eryu Guan" <[e-mail]>
> > To: "XiaoLi Feng" <[e-mail]>
> > Cc: [e-mail]
> > Sent: Monday, May 21, 2018 3:50:27 PM
> > Subject: Re: [PATCH] common/rc: add the function _require_noatime
> >
> > [add [e-mail] to cc list]
> >
> > On Fri, May 18, 2018 at 07:10:29PM +0800, Xiaoli Feng wrote:
> >> From: xiaoli feng <[e-mail]>
> >>
> >> In the generic/120, it will make the test not-pass if the filesystem
> >> mounts failed with noatime. Now change this result to norun. The
> >> filesystem cifs doesn't support noatime. Just make the test norun
> >> until it supports noatime.
> >>
> >> Signed-off-by: xiaoli feng <[e-mail]>
> >> ---
> >>  common/rc         | 8 +++++++-
> >>  tests/generic/120 | 7 +------
> >>  2 files changed, 8 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/common/rc b/common/rc
> >> index ffe5323..9c45f1b 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -3244,7 +3244,13 @@ _require_atime()
> >>       _exclude_scratch_mount_option "noatime"
> >>       if [ "$FSTYP" == "nfs" ]; then
> >>               _notrun "atime related mount options have no effect on NFS"
> >> -     fi
> >
> > I'm not sure what's the expected behavior from CIFS on atime/noatime, so
> > I added linux-cifs list for input.
> >
> > If CIFS behaves similarly to NFS, looks like that you could simply add
> > another check for cifs in _require_atime(), as what we already do for
> > nfs, so all tests that _require_atime() will _notrun on cifs.
> >
> >> +}
> >> +
> >> +_require_noatime()
> >> +{
> >> +     _exclude_scratch_mount_option "atime"
> >> +      _try_scratch_mount -o noatime || \
> >> +                _notrun "noatime not supported by the current tested filesystem"
> >>  }
> >>
> >>  _require_relatime()
> >> diff --git a/tests/generic/120 b/tests/generic/120
> >> index 1180c10..ddd61b3 100755
> >> --- a/tests/generic/120
> >> +++ b/tests/generic/120
> >> @@ -60,12 +60,7 @@ _compare_access_times()
> >>
> >>  }
> >>
> >> -if ! _try_scratch_mount "-o noatime" >$tmp.out 2>&1
> >> -then
> >> -    cat $tmp.out
> >> -    echo "!!! mount failed"
> >> -    exit
> >> -fi
> >> +_require_noatime
> >
> > Anyway, failing the test when "noatime" mount fails is one of the
> > purposes of the test, and it shouldn't be removed, as we've already made
> > sure current FSTYP supports atime/noatime (by _require rules), so a
> > noatime mount failure indicates a bug in the filesystem.
> >
> > Thanks,
> > Eryu
> >
> >>
> >>  #executable file
> >>  echo "*** copying file ***"
> >> --
> >> 1.8.3.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe fstests" in
> >> the body of a message to [e-mail]
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Thanks,
> 
> Steve
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

XiaoLi Feng May 22, 2018, 2:54 a.m. UTC | #5

----- Original Message -----
> From: "Steve French" <[e-mail]>
> To: "Xiaoli Feng" <[e-mail]>
> Cc: "Eryu Guan" <[e-mail]>, "CIFS" <[e-mail]>, [e-mail]
> Sent: Tuesday, May 22, 2018 7:54:45 AM
> Subject: Re: [PATCH] common/rc: add the function _require_noatime
> 
> Should this be fixed by changing cifs.ko to accept the mount parm but ignore
> it?

Yes, it should. For cifs, atime/diratime/relatime/norelatime/nostrictatime can be mounted without effect.
But mount failed with noatime/nodiatime/strictatime. Seems they don't maintain consistency. I will file a bug
about it. 

Thanks Steve & Eryu.

> 
> If this test works on NFS (the noatime mount option has no meaning for
> NFS apparently) we should do the same
> 
> Quoting the NFS man page:
> 
>        In particular, the atime/noatime, diratime/nodiratime,
>        relatime/norela‐
>        time, and strictatime/nostrictatime mount options have no effect on
>        NFS
>        mounts.
> 
> On Mon, May 21, 2018 at 3:50 AM, Xiaoli Feng <[e-mail]> wrote:
> >
> > [add [e-mail] to cc list]
> >
> > ----- Forwarded Message -----
> > From: "Eryu Guan" <[e-mail]>
> > To: "XiaoLi Feng" <[e-mail]>
> > Cc: [e-mail]
> > Sent: Monday, May 21, 2018 3:50:27 PM
> > Subject: Re: [PATCH] common/rc: add the function _require_noatime
> >
> > [add [e-mail] to cc list]
> >
> > On Fri, May 18, 2018 at 07:10:29PM +0800, Xiaoli Feng wrote:
> >> From: xiaoli feng <[e-mail]>
> >>
> >> In the generic/120, it will make the test not-pass if the filesystem
> >> mounts failed with noatime. Now change this result to norun. The
> >> filesystem cifs doesn't support noatime. Just make the test norun
> >> until it supports noatime.
> >>
> >> Signed-off-by: xiaoli feng <[e-mail]>
> >> ---
> >>  common/rc         | 8 +++++++-
> >>  tests/generic/120 | 7 +------
> >>  2 files changed, 8 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/common/rc b/common/rc
> >> index ffe5323..9c45f1b 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -3244,7 +3244,13 @@ _require_atime()
> >>       _exclude_scratch_mount_option "noatime"
> >>       if [ "$FSTYP" == "nfs" ]; then
> >>               _notrun "atime related mount options have no effect on NFS"
> >> -     fi
> >
> > I'm not sure what's the expected behavior from CIFS on atime/noatime, so
> > I added linux-cifs list for input.
> >
> > If CIFS behaves similarly to NFS, looks like that you could simply add
> > another check for cifs in _require_atime(), as what we already do for
> > nfs, so all tests that _require_atime() will _notrun on cifs.
> >
> >> +}
> >> +
> >> +_require_noatime()
> >> +{
> >> +     _exclude_scratch_mount_option "atime"
> >> +      _try_scratch_mount -o noatime || \
> >> +                _notrun "noatime not supported by the current tested
> >> filesystem"
> >>  }
> >>
> >>  _require_relatime()
> >> diff --git a/tests/generic/120 b/tests/generic/120
> >> index 1180c10..ddd61b3 100755
> >> --- a/tests/generic/120
> >> +++ b/tests/generic/120
> >> @@ -60,12 +60,7 @@ _compare_access_times()
> >>
> >>  }
> >>
> >> -if ! _try_scratch_mount "-o noatime" >$tmp.out 2>&1
> >> -then
> >> -    cat $tmp.out
> >> -    echo "!!! mount failed"
> >> -    exit
> >> -fi
> >> +_require_noatime
> >
> > Anyway, failing the test when "noatime" mount fails is one of the
> > purposes of the test, and it shouldn't be removed, as we've already made
> > sure current FSTYP supports atime/noatime (by _require rules), so a
> > noatime mount failure indicates a bug in the filesystem.
> >
> > Thanks,
> > Eryu
> >
> >>
> >>  #executable file
> >>  echo "*** copying file ***"
> >> --
> >> 1.8.3.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe fstests" in
> >> the body of a message to [e-mail]
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> --
> Thanks,
> 
> Steve
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/common/rc b/common/rc
index ffe5323..9c45f1b 100644
--- a/common/rc
+++ b/common/rc
@@ -3244,7 +3244,13 @@  _require_atime()
 	_exclude_scratch_mount_option "noatime"
 	if [ "$FSTYP" == "nfs" ]; then
 		_notrun "atime related mount options have no effect on NFS"
-	fi
+}
+
+_require_noatime()
+{
+	_exclude_scratch_mount_option "atime"
+	 _try_scratch_mount -o noatime || \
+                _notrun "noatime not supported by the current tested filesystem"
 }
 
 _require_relatime()
diff --git a/tests/generic/120 b/tests/generic/120
index 1180c10..ddd61b3 100755
--- a/tests/generic/120
+++ b/tests/generic/120
@@ -60,12 +60,7 @@  _compare_access_times()
 
 }
 
-if ! _try_scratch_mount "-o noatime" >$tmp.out 2>&1
-then
-    cat $tmp.out
-    echo "!!! mount failed"
-    exit
-fi
+_require_noatime
 
 #executable file
 echo "*** copying file ***"