diff mbox

fstests: btrfs/143: make test case more reliable

Message ID 20171023205733.14896-1-bo.li.liu@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Liu Bo Oct. 23, 2017, 8:57 p.m. UTC
Currently drop_caches is used to invalidate file's page cache so that
buffered read can hit disk, but the problem is that it may also
invalidate metadata's page cache, so the test case may not get read
errors (and repair) if reading metadata has consumed the injected
faults.

This changes it to do 'fadvise -d' to firstly access all metadata it
needs to locate the file and then only drops the test file's page
cache.  Also this changes it to read the file only if pid%2 == 1.

Reported-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 tests/btrfs/143 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Nikolay Borisov Oct. 24, 2017, 5:28 a.m. UTC | #1
On 23.10.2017 23:57, Liu Bo wrote:
> Currently drop_caches is used to invalidate file's page cache so that
> buffered read can hit disk, but the problem is that it may also
> invalidate metadata's page cache, so the test case may not get read
> errors (and repair) if reading metadata has consumed the injected
> faults.
> 
> This changes it to do 'fadvise -d' to firstly access all metadata it
> needs to locate the file and then only drops the test file's page
> cache.  Also this changes it to read the file only if pid%2 == 1.
> 
> Reported-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  tests/btrfs/143 | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/btrfs/143 b/tests/btrfs/143
> index da7bfd8..dabd03d 100755
> --- a/tests/btrfs/143
> +++ b/tests/btrfs/143
> @@ -127,16 +127,16 @@ echo "step 3......repair the bad copy" >>$seqres.full
>  # since raid1 consists of two copies, and the bad copy was put on stripe #1
>  # while the good copy lies on stripe #0, the bad copy only gets access when the
>  # reader's pid % 2 == 1 is true
> -while true; do
> -	# start_fail only fails the following buffered read so the repair is
> -	# supposed to work.
> -	echo 3 > /proc/sys/vm/drop_caches
> -	start_fail
> -	$XFS_IO_PROG -c "pread 0 4K" "$SCRATCH_MNT/foobar" > /dev/null &
> -	pid=$!
> -	wait
> -	stop_fail
> -	[ $((pid % 2)) == 1 ] && break
> +while [[ -z ${result} ]]; do
> +    # invalidate the page cache.
> +    $XFS_IO_PROG -c "fadvise -d 0 128K" $SCRATCH_MNT/foobar

I'm a bit worried about the expectations of the DONT_NEED:

https://linux.die.net/man/2/posix_fadvise:

The advice is not binding; it merely constitutes an expectation on
behalf of the application.

This might very well be a moot point but still



> +
> +    start_fail
> +    result=$(bash -c "
> +        if [[ \$((\$\$ % 2)) -eq 1 ]]; then
> +                exec $XFS_IO_PROG -c \"pread 0 4K\" \"$SCRATCH_MNT/foobar\"
> +        fi");
> +    stop_fail
>  done
>  
>  _scratch_unmount
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nikolay Borisov Oct. 24, 2017, 8:51 a.m. UTC | #2
On 23.10.2017 23:57, Liu Bo wrote:
> Currently drop_caches is used to invalidate file's page cache so that
> buffered read can hit disk, but the problem is that it may also
> invalidate metadata's page cache, so the test case may not get read
> errors (and repair) if reading metadata has consumed the injected
> faults.
> 
> This changes it to do 'fadvise -d' to firstly access all metadata it
> needs to locate the file and then only drops the test file's page
> cache.  Also this changes it to read the file only if pid%2 == 1.
> 
> Reported-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  tests/btrfs/143 | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/btrfs/143 b/tests/btrfs/143
> index da7bfd8..dabd03d 100755
> --- a/tests/btrfs/143
> +++ b/tests/btrfs/143
> @@ -127,16 +127,16 @@ echo "step 3......repair the bad copy" >>$seqres.full
>  # since raid1 consists of two copies, and the bad copy was put on stripe #1
>  # while the good copy lies on stripe #0, the bad copy only gets access when the
>  # reader's pid % 2 == 1 is true
> -while true; do
> -	# start_fail only fails the following buffered read so the repair is
> -	# supposed to work.
> -	echo 3 > /proc/sys/vm/drop_caches
> -	start_fail
> -	$XFS_IO_PROG -c "pread 0 4K" "$SCRATCH_MNT/foobar" > /dev/null &
> -	pid=$!
> -	wait
> -	stop_fail
> -	[ $((pid % 2)) == 1 ] && break
> +while [[ -z ${result} ]]; do
> +    # invalidate the page cache.
> +    $XFS_IO_PROG -c "fadvise -d 0 128K" $SCRATCH_MNT/foobar
> +
> +    start_fail
> +    result=$(bash -c "
> +        if [[ \$((\$\$ % 2)) -eq 1 ]]; then
> +                exec $XFS_IO_PROG -c \"pread 0 4K\" \"$SCRATCH_MNT/foobar\"
> +        fi");
> +    stop_fail

Tested-by: Nikolay Borisov <nborisov@suse.com>


>  done
>  
>  _scratch_unmount
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nikolay Borisov Oct. 25, 2017, 10:32 a.m. UTC | #3
On 23.10.2017 23:57, Liu Bo wrote:
> Currently drop_caches is used to invalidate file's page cache so that
> buffered read can hit disk, but the problem is that it may also
> invalidate metadata's page cache, so the test case may not get read
> errors (and repair) if reading metadata has consumed the injected
> faults.
> 
> This changes it to do 'fadvise -d' to firstly access all metadata it
> needs to locate the file and then only drops the test file's page
> cache.  Also this changes it to read the file only if pid%2 == 1.
> 
> Reported-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  tests/btrfs/143 | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/btrfs/143 b/tests/btrfs/143
> index da7bfd8..dabd03d 100755
> --- a/tests/btrfs/143
> +++ b/tests/btrfs/143
> @@ -127,16 +127,16 @@ echo "step 3......repair the bad copy" >>$seqres.full
>  # since raid1 consists of two copies, and the bad copy was put on stripe #1
>  # while the good copy lies on stripe #0, the bad copy only gets access when the
>  # reader's pid % 2 == 1 is true
> -while true; do
> -	# start_fail only fails the following buffered read so the repair is
> -	# supposed to work.
> -	echo 3 > /proc/sys/vm/drop_caches
> -	start_fail
> -	$XFS_IO_PROG -c "pread 0 4K" "$SCRATCH_MNT/foobar" > /dev/null &
> -	pid=$!
> -	wait
> -	stop_fail
> -	[ $((pid % 2)) == 1 ] && break
> +while [[ -z ${result} ]]; do
> +    # invalidate the page cache.
> +    $XFS_IO_PROG -c "fadvise -d 0 128K" $SCRATCH_MNT/foobar

I think an even better approach would be to just unmount and then mount
it. That ensures the page cache is truncated.

> +
> +    start_fail
> +    result=$(bash -c "
> +        if [[ \$((\$\$ % 2)) -eq 1 ]]; then
> +                exec $XFS_IO_PROG -c \"pread 0 4K\" \"$SCRATCH_MNT/foobar\"
> +        fi");
> +    stop_fail
>  done
>  
>  _scratch_unmount
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/tests/btrfs/143 b/tests/btrfs/143
index da7bfd8..dabd03d 100755
--- a/tests/btrfs/143
+++ b/tests/btrfs/143
@@ -127,16 +127,16 @@  echo "step 3......repair the bad copy" >>$seqres.full
 # since raid1 consists of two copies, and the bad copy was put on stripe #1
 # while the good copy lies on stripe #0, the bad copy only gets access when the
 # reader's pid % 2 == 1 is true
-while true; do
-	# start_fail only fails the following buffered read so the repair is
-	# supposed to work.
-	echo 3 > /proc/sys/vm/drop_caches
-	start_fail
-	$XFS_IO_PROG -c "pread 0 4K" "$SCRATCH_MNT/foobar" > /dev/null &
-	pid=$!
-	wait
-	stop_fail
-	[ $((pid % 2)) == 1 ] && break
+while [[ -z ${result} ]]; do
+    # invalidate the page cache.
+    $XFS_IO_PROG -c "fadvise -d 0 128K" $SCRATCH_MNT/foobar
+
+    start_fail
+    result=$(bash -c "
+        if [[ \$((\$\$ % 2)) -eq 1 ]]; then
+                exec $XFS_IO_PROG -c \"pread 0 4K\" \"$SCRATCH_MNT/foobar\"
+        fi");
+    stop_fail
 done
 
 _scratch_unmount