common/rc: add the function _require_noatime

PATCH In review XiaoLi Feng 3 participants 7 messages May 18, 2018 – May 22, 2018 VFS

lore

Share & tools

lore

XiaoLi Feng

author

2018-05-18 11:10 UTC #0 lore raw

PATCH 1/1 common/rc: add the function _require_noatime

From: xiaoli feng <[email protected]>

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 <[email protected]>

---
 common/rc         | 8 +++++++-
 tests/generic/120 | 7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)

common/rc +7 -1

index ffe5323..9c45f1b 100644
@@ -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()

tests/generic/120 +1 -6

index 1180c10..ddd61b3 100755
@@ -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 ***"

Signature · 1 line

1.8.3.1

Eryu Guan

2018-05-21 07:50 UTC #1 lore raw

[add [email protected] to cc list]

Quoted 18 lines · Xiaoli Feng

From: xiaoli feng <[email protected]>
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 <[email protected]>
---
 common/rc         | 8 +++++++-
 tests/generic/120 | 7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)
common/rc +0 -0
index ffe5323..9c45f1b 100644

common/rc _require_atime() · line 3247

Quoted 4 lines

common/rc +0 -1

@@ -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.

Quoted 15 lines

common/rc +7 -0

+}
+
+_require_noatime()
+{
+	_exclude_scratch_mount_option "atime"
+	 _try_scratch_mount -o noatime || \
+                _notrun "noatime not supported by the current tested filesystem"
 }
 
 _require_relatime()

tests/generic/120 +0 -0

index 1180c10..ddd61b3 100755

tests/generic/120 _compare_access_times() · line 63

Quoted 10 lines

tests/generic/120 +1 -6

@@ -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

Quoted 10 lines

tests/generic/120 +0 -0

 #executable file
 echo "*** copying file ***"

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Xiaoli Feng

author

2018-05-21 08:50 UTC #2 lore raw

PATCH 1/1 common/rc: add the function _require_noatime

[add [email protected] to cc list]

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

[add [email protected] to cc list]

Quoted 18 lines · Xiaoli Feng

From: xiaoli feng <[email protected]>
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 <[email protected]>
---
 common/rc         | 8 +++++++-
 tests/generic/120 | 7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)
common/rc +0 -0
index ffe5323..9c45f1b 100644

common/rc _require_atime() · line 3247

Quoted 4 lines

common/rc +0 -1

@@ -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.

Quoted 15 lines

common/rc +7 -0

+}
+
+_require_noatime()
+{
+	_exclude_scratch_mount_option "atime"
+	 _try_scratch_mount -o noatime || \
+                _notrun "noatime not supported by the current tested filesystem"
 }
 
 _require_relatime()

tests/generic/120 +0 -0

index 1180c10..ddd61b3 100755

tests/generic/120 _compare_access_times() · line 63

Quoted 10 lines

tests/generic/120 +1 -6

@@ -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

Thanks,
Eryu

Quoted 10 lines

tests/generic/120 +0 -0

 #executable file
 echo "*** copying file ***"

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Steve French

2018-05-21 23:55 UTC #3 lore raw

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.

Quoted 95 lines · Xiaoli Feng

[add [email protected] to cc list]
----- Forwarded Message -----
From: "Eryu Guan" <[email protected]>
To: "XiaoLi Feng" <[email protected]>
Cc: [email protected]
Sent: Monday, May 21, 2018 3:50:27 PM
Subject: Re: [PATCH] common/rc: add the function _require_noatime
[add [email protected] to cc list]
Quoted 18 lines · Xiaoli Feng
From: xiaoli feng <[email protected]>
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 <[email protected]>
---
 common/rc         | 8 +++++++-
 tests/generic/120 | 7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)
common/rc +0 -0
index ffe5323..9c45f1b 100644
common/rc _require_atime() · line 3247
Quoted 4 lines
common/rc +0 -1
@@ -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.
Quoted 15 lines
common/rc +7 -0
+}
+
+_require_noatime()
+{
+     _exclude_scratch_mount_option "atime"
+      _try_scratch_mount -o noatime || \
+                _notrun "noatime not supported by the current tested filesystem"
 }
 
 _require_relatime()
tests/generic/120 +0 -0
index 1180c10..ddd61b3 100755
tests/generic/120 _compare_access_times() · line 63
Quoted 10 lines
tests/generic/120 +1 -6
@@ -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
Quoted 10 lines
tests/generic/120 +0 -1
 #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 [email protected]
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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

Thanks,

Steve

Steve French

2018-05-21 23:55 UTC #4 lore raw

PATCH 1/1 common/rc: add the function _require_noatime

Adding fsdevel for this question

---------- Forwarded message ----------
From: Steve French <[email protected]>
Date: Mon, May 21, 2018 at 6:54 PM
Subject: Re: [PATCH] common/rc: add the function _require_noatime
To: Xiaoli Feng <[email protected]>
Cc: Eryu Guan <[email protected]>, CIFS <[email protected]>, [email protected]

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.

Quoted 87 lines · Xiaoli Feng

[add [email protected] to cc list]
----- Forwarded Message -----
From: "Eryu Guan" <[email protected]>
To: "XiaoLi Feng" <[email protected]>
Cc: [email protected]
Sent: Monday, May 21, 2018 3:50:27 PM
Subject: Re: [PATCH] common/rc: add the function _require_noatime
[add [email protected] to cc list]
Quoted 18 lines · Xiaoli Feng
From: xiaoli feng <[email protected]>
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 <[email protected]>
---
 common/rc         | 8 +++++++-
 tests/generic/120 | 7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)
common/rc +0 -0
index ffe5323..9c45f1b 100644
common/rc _require_atime() · line 3247
Quoted 4 lines
common/rc +0 -1
@@ -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.
Quoted 15 lines
common/rc +7 -0
+}
+
+_require_noatime()
+{
+     _exclude_scratch_mount_option "atime"
+      _try_scratch_mount -o noatime || \
+                _notrun "noatime not supported by the current tested filesystem"
 }
 
 _require_relatime()
tests/generic/120 +0 -0
index 1180c10..ddd61b3 100755
tests/generic/120 _compare_access_times() · line 63
Quoted 10 lines
tests/generic/120 +1 -6
@@ -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
Quoted 10 lines
tests/generic/120 +0 -1
 #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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines

--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Thanks,

Steve

Signature · 3 lines

Thanks,

Steve

Eryu Guan

2018-05-22 02:21 UTC #5 lore raw

In reply to Steve French

Quoted 4 lines · Steve French

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.

Quoted 6 lines

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

Quoted 104 lines · Xiaoli Feng

Quoted 95 lines
[add [email protected] to cc list]
----- Forwarded Message -----
From: "Eryu Guan" <[email protected]>
To: "XiaoLi Feng" <[email protected]>
Cc: [email protected]
Sent: Monday, May 21, 2018 3:50:27 PM
Subject: Re: [PATCH] common/rc: add the function _require_noatime
[add [email protected] to cc list]
Quoted 18 lines · Xiaoli Feng
From: xiaoli feng <[email protected]>
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 <[email protected]>
---
 common/rc         | 8 +++++++-
 tests/generic/120 | 7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)
common/rc +0 -0
index ffe5323..9c45f1b 100644
common/rc _require_atime() · line 3247
Quoted 4 lines
common/rc +0 -1
@@ -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.
Quoted 15 lines
common/rc +7 -0
+}
+
+_require_noatime()
+{
+     _exclude_scratch_mount_option "atime"
+      _try_scratch_mount -o noatime || \
+                _notrun "noatime not supported by the current tested filesystem"
 }
 
 _require_relatime()
tests/generic/120 +0 -0
index 1180c10..ddd61b3 100755
tests/generic/120 _compare_access_times() · line 63
Quoted 10 lines
tests/generic/120 +1 -6
@@ -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
Quoted 10 lines
tests/generic/120 +0 -1
 #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 [email protected]
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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Thanks,

Steve

Xiaoli Feng

author

2018-05-22 02:54 UTC #6 lore raw

In reply to Steve French

----- Original Message -----

Quoted 8 lines

From: "Steve French" <[email protected]>
To: "Xiaoli Feng" <[email protected]>
Cc: "Eryu Guan" <[email protected]>, "CIFS" <[email protected]>, [email protected]
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.

Quoted 121 lines

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.
Quoted 88 lines · Xiaoli Feng
[add [email protected] to cc list]
----- Forwarded Message -----
From: "Eryu Guan" <[email protected]>
To: "XiaoLi Feng" <[email protected]>
Cc: [email protected]
Sent: Monday, May 21, 2018 3:50:27 PM
Subject: Re: [PATCH] common/rc: add the function _require_noatime
[add [email protected] to cc list]
Quoted 18 lines · Xiaoli Feng
From: xiaoli feng <[email protected]>
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 <[email protected]>
---
 common/rc         | 8 +++++++-
 tests/generic/120 | 7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)
common/rc +0 -0
index ffe5323..9c45f1b 100644
common/rc _require_atime() · line 3247
Quoted 4 lines
common/rc +0 -1
@@ -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.
Quoted 16 lines
common/rc +7 -0
+}
+
+_require_noatime()
+{
+     _exclude_scratch_mount_option "atime"
+      _try_scratch_mount -o noatime || \
+                _notrun "noatime not supported by the current tested
filesystem"
}
_require_relatime()
tests/generic/120 +0 -0
index 1180c10..ddd61b3 100755
tests/generic/120 _compare_access_times() · line 63
Quoted 10 lines
tests/generic/120 +1 -6
@@ -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
Quoted 10 lines
tests/generic/120 +0 -1
 #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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 8 lines
--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html