[PATCH v2 1/2] generic/608: Remove drop cache and umount & mount cycle

[PATCH v2 1/2] generic/608: Remove drop cache and umount & mount cycle

[Xiao Yang]

Since commit e7bed0b5bc4b, toggling FS_XFLAG_DAX on an existing file can make
S_DAX on the file change immediately when all applications close the file.
As a result, we don't need the redundant drop cache and umount & mount cycle.

Signed-off-by: Xiaoli Feng <xifeng@redhat.com>
Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
 tests/generic/608 | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/tests/generic/608 b/tests/generic/608
index 861bbff5..d81a2488 100755
--- a/tests/generic/608
+++ b/tests/generic/608
@@ -3,10 +3,10 @@
 # Copyright (c) 2020 Fujitsu.  All Rights Reserved.
 #
 # FS QA Test 608
-# Change FS_XFLAG_DAX on an existing file and check if S_DAX on
-# the file can take effect immediately by the following steps:
-# 1) Stop all applications which are using the file.
-# 2) Do drop_caches or umount & mount cycle.
+# Toggling FS_XFLAG_DAX on an existing file can make S_DAX on the
+# file change immediately when all applications close the file.
+# It's a regression test for:
+# 'commit e7bed0b5bc4b ("fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set")'
 
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
@@ -36,7 +36,7 @@ _require_dax_iflag
 _require_xfs_io_command "lsattr" "-v"
 _require_xfs_io_command "statx" "-r"
 
-test_drop_caches()
+test_enable_dax()
 {
 	local t_file=$SCRATCH_MNT/testfile
 
@@ -49,24 +49,19 @@ test_drop_caches()
 
 	$XFS_IO_PROG -c 'chattr +x' $t_file
 	_check_xflag $t_file 1
-	_check_s_dax $t_file 0
-
-	# One application is using test file and check if S_DAX on
-	# the file doesn't take effect immediately by drop_caches
-	echo 2 > /proc/sys/vm/drop_caches
+	# One application is using test file and S_DAX
+	# on the file is not changed immediately
 	_check_s_dax $t_file 0
 
 	exec 3<&-
 
-	# No application is using test file and check if S_DAX on
-	# the file takes effect immediately by drop_caches
-	echo 2 > /proc/sys/vm/drop_caches
+	# No application is using test file and S_DAX
+	# on the file is changed immediately
 	_check_s_dax $t_file 1
 }
 
-test_cycle_mount()
+test_disable_dax()
 {
-	local option=$1
 	local t_dir=$SCRATCH_MNT/testdir
 	local t_file=$t_dir/testfile
 
@@ -81,29 +76,29 @@ test_cycle_mount()
 
 	$XFS_IO_PROG -c 'chattr -x' $t_file
 	_check_xflag $t_file 0
+	# One application is using test file and S_DAX
+	# on the file is not changed immediately
 	_check_s_dax $t_file 1
 
 	exec 3<&-
 
-	# No application is using test file and check if S_DAX on
-	# the file takes effect immediately by umount & mount
-	_scratch_cycle_mount "$option"
+	# No application is using test file and S_DAX
+	# on the file is changed immediately
 	_check_s_dax $t_file 0
 }
 
 do_tests()
 {
 	local mount_option=$1
-	local cycle_mount_option=$2
 
 	_scratch_mount "$mount_option"
 
 	# Make sure the root dir doesn't have FS_XFLAG_DAX set before we start.
 	$XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT &>> $seqres.full
 
-	test_drop_caches
-
-	test_cycle_mount "$cycle_mount_option"
+	# Do test for commit e7bed0b5bc4b
+	test_enable_dax
+	test_disable_dax
 
 	_scratch_unmount
 }
@@ -111,7 +106,7 @@ do_tests()
 _scratch_mkfs >> $seqres.full 2>&1
 
 # Mount with dax option
-do_tests "-o dax=inode" "dax=inode"
+do_tests "-o dax=inode"
 
 # Mount without dax option
 export MOUNT_OPTIONS=""
-- 
2.21.0

[PATCH v2 2/2] generic/608: Add new regression test

[Xiao Yang]

Write data into a file and then enable DAX on the file immediately,
the written data which is still in the buffer will be lost when
the corresponding inode is evicted.

It's a regression test for commit 26b15a042b8a.

Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
 tests/generic/608 | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tests/generic/608 b/tests/generic/608
index d81a2488..674bd2cf 100755
--- a/tests/generic/608
+++ b/tests/generic/608
@@ -7,6 +7,12 @@
 # file change immediately when all applications close the file.
 # It's a regression test for:
 # 'commit e7bed0b5bc4b ("fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set")'
+#
+# Write data into a file and then enable DAX on the file immediately,
+# the written data which is still in the buffer will be lost when the
+# corresponding inode is evicted.
+# It's a regression test for:
+# 'commit 26b15a042b8a ("fs: Handle I_DONTCACHE in iput_final() instead of generic_drop_inode()"'
 
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
@@ -87,6 +93,26 @@ test_disable_dax()
 	_check_s_dax $t_file 0
 }
 
+test_buffered_data_lost()
+{
+	local t_file=$SCRATCH_MNT/datafile
+
+	# Write data into a file
+	echo "Buffered data" > $t_file
+
+	# Then enable DAX on the file immediately
+	$XFS_IO_PROG -c 'chattr +x' $t_file
+
+	# Without commit e7bed0b5bc4b, ensure inode can
+	# be evicted by drop_caches
+	echo 2 > /proc/sys/vm/drop_caches
+
+	# The written data which is still in the buffer will be lost
+	grep -q "Buffered data" $t_file || echo "Buffered data is lost"
+
+	rm -f $t_file
+}
+
 do_tests()
 {
 	local mount_option=$1
@@ -100,6 +126,9 @@ do_tests()
 	test_enable_dax
 	test_disable_dax
 
+	# Do test for commit 26b15a042b8a
+	test_buffered_data_lost
+
 	_scratch_unmount
 }
 
-- 
2.21.0

Re: [PATCH v2 1/2] generic/608: Remove drop cache and umount & mount cycle

[Ira Weiny]

On Mon, Mar 29, 2021 at 05:20:53PM +0800, Xiao Yang wrote:
> Since commit e7bed0b5bc4b, toggling FS_XFLAG_DAX on an existing file can make
> S_DAX on the file change immediately when all applications close the file.
> As a result, we don't need the redundant drop cache and umount & mount cycle.
> 
> Signed-off-by: Xiaoli Feng <xifeng@redhat.com>
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>

LGTM

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  tests/generic/608 | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/generic/608 b/tests/generic/608
> index 861bbff5..d81a2488 100755
> --- a/tests/generic/608
> +++ b/tests/generic/608
> @@ -3,10 +3,10 @@
>  # Copyright (c) 2020 Fujitsu.  All Rights Reserved.
>  #
>  # FS QA Test 608
> -# Change FS_XFLAG_DAX on an existing file and check if S_DAX on
> -# the file can take effect immediately by the following steps:
> -# 1) Stop all applications which are using the file.
> -# 2) Do drop_caches or umount & mount cycle.
> +# Toggling FS_XFLAG_DAX on an existing file can make S_DAX on the
> +# file change immediately when all applications close the file.
> +# It's a regression test for:
> +# 'commit e7bed0b5bc4b ("fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set")'
>  
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
> @@ -36,7 +36,7 @@ _require_dax_iflag
>  _require_xfs_io_command "lsattr" "-v"
>  _require_xfs_io_command "statx" "-r"
>  
> -test_drop_caches()
> +test_enable_dax()
>  {
>  	local t_file=$SCRATCH_MNT/testfile
>  
> @@ -49,24 +49,19 @@ test_drop_caches()
>  
>  	$XFS_IO_PROG -c 'chattr +x' $t_file
>  	_check_xflag $t_file 1
> -	_check_s_dax $t_file 0
> -
> -	# One application is using test file and check if S_DAX on
> -	# the file doesn't take effect immediately by drop_caches
> -	echo 2 > /proc/sys/vm/drop_caches
> +	# One application is using test file and S_DAX
> +	# on the file is not changed immediately
>  	_check_s_dax $t_file 0
>  
>  	exec 3<&-
>  
> -	# No application is using test file and check if S_DAX on
> -	# the file takes effect immediately by drop_caches
> -	echo 2 > /proc/sys/vm/drop_caches
> +	# No application is using test file and S_DAX
> +	# on the file is changed immediately
>  	_check_s_dax $t_file 1
>  }
>  
> -test_cycle_mount()
> +test_disable_dax()
>  {
> -	local option=$1
>  	local t_dir=$SCRATCH_MNT/testdir
>  	local t_file=$t_dir/testfile
>  
> @@ -81,29 +76,29 @@ test_cycle_mount()
>  
>  	$XFS_IO_PROG -c 'chattr -x' $t_file
>  	_check_xflag $t_file 0
> +	# One application is using test file and S_DAX
> +	# on the file is not changed immediately
>  	_check_s_dax $t_file 1
>  
>  	exec 3<&-
>  
> -	# No application is using test file and check if S_DAX on
> -	# the file takes effect immediately by umount & mount
> -	_scratch_cycle_mount "$option"
> +	# No application is using test file and S_DAX
> +	# on the file is changed immediately
>  	_check_s_dax $t_file 0
>  }
>  
>  do_tests()
>  {
>  	local mount_option=$1
> -	local cycle_mount_option=$2
>  
>  	_scratch_mount "$mount_option"
>  
>  	# Make sure the root dir doesn't have FS_XFLAG_DAX set before we start.
>  	$XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT &>> $seqres.full
>  
> -	test_drop_caches
> -
> -	test_cycle_mount "$cycle_mount_option"
> +	# Do test for commit e7bed0b5bc4b
> +	test_enable_dax
> +	test_disable_dax
>  
>  	_scratch_unmount
>  }
> @@ -111,7 +106,7 @@ do_tests()
>  _scratch_mkfs >> $seqres.full 2>&1
>  
>  # Mount with dax option
> -do_tests "-o dax=inode" "dax=inode"
> +do_tests "-o dax=inode"
>  
>  # Mount without dax option
>  export MOUNT_OPTIONS=""
> -- 
> 2.21.0
> 
> 
> 

Re: [PATCH v2 2/2] generic/608: Add new regression test

[Ira Weiny]

On Mon, Mar 29, 2021 at 05:20:54PM +0800, Xiao Yang wrote:
> Write data into a file and then enable DAX on the file immediately,
> the written data which is still in the buffer will be lost when
> the corresponding inode is evicted.

This and the comment below are confusing.  Doesn't the buffered data get
written?  Which means the data is not lost?

IOW the test is checking to make sure the data does not get lost, not that it
does.  What am I missing?

Ira

> 
> It's a regression test for commit 26b15a042b8a.
> 
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
>  tests/generic/608 | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tests/generic/608 b/tests/generic/608
> index d81a2488..674bd2cf 100755
> --- a/tests/generic/608
> +++ b/tests/generic/608
> @@ -7,6 +7,12 @@
>  # file change immediately when all applications close the file.
>  # It's a regression test for:
>  # 'commit e7bed0b5bc4b ("fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set")'
> +#
> +# Write data into a file and then enable DAX on the file immediately,
> +# the written data which is still in the buffer will be lost when the
> +# corresponding inode is evicted.
> +# It's a regression test for:
> +# 'commit 26b15a042b8a ("fs: Handle I_DONTCACHE in iput_final() instead of generic_drop_inode()"'
>  
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
> @@ -87,6 +93,26 @@ test_disable_dax()
>  	_check_s_dax $t_file 0
>  }
>  
> +test_buffered_data_lost()
> +{
> +	local t_file=$SCRATCH_MNT/datafile
> +
> +	# Write data into a file
> +	echo "Buffered data" > $t_file
> +
> +	# Then enable DAX on the file immediately
> +	$XFS_IO_PROG -c 'chattr +x' $t_file
> +
> +	# Without commit e7bed0b5bc4b, ensure inode can
> +	# be evicted by drop_caches
> +	echo 2 > /proc/sys/vm/drop_caches
> +
> +	# The written data which is still in the buffer will be lost
> +	grep -q "Buffered data" $t_file || echo "Buffered data is lost"
> +
> +	rm -f $t_file
> +}
> +
>  do_tests()
>  {
>  	local mount_option=$1
> @@ -100,6 +126,9 @@ do_tests()
>  	test_enable_dax
>  	test_disable_dax
>  
> +	# Do test for commit 26b15a042b8a
> +	test_buffered_data_lost
> +
>  	_scratch_unmount
>  }
>  
> -- 
> 2.21.0
> 
> 
> 

Re: [PATCH v2 2/2] generic/608: Add new regression test

[yangx.jy]

On 2021/3/30 2:23, Ira Weiny wrote:
> On Mon, Mar 29, 2021 at 05:20:54PM +0800, Xiao Yang wrote:
>> Write data into a file and then enable DAX on the file immediately,
>> the written data which is still in the buffer will be lost when
>> the corresponding inode is evicted.
> This and the comment below are confusing.  Doesn't the buffered data get
> written?  Which means the data is not lost?
Hi Ira,

Thanks for your quick comment.

How about the following commit message:
-------------------------------------------------------------------------

Write data into a file and then enable DAX on the file immediately,
the written data which is still in the buffer should be synchronized
to disk instead of discarded when the corresponding inode is evicted.

-------------------------------------------------------------------------
> IOW the test is checking to make sure the data does not get lost, not that it
> does.  What am I missing?

Right, the data should not get lost.

Best Regards,
Xiao Yang
> Ira
>
>> It's a regression test for commit 26b15a042b8a.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
>> ---
>>   tests/generic/608 | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/tests/generic/608 b/tests/generic/608
>> index d81a2488..674bd2cf 100755
>> --- a/tests/generic/608
>> +++ b/tests/generic/608
>> @@ -7,6 +7,12 @@
>>   # file change immediately when all applications close the file.
>>   # It's a regression test for:
>>   # 'commit e7bed0b5bc4b ("fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set")'
>> +#
>> +# Write data into a file and then enable DAX on the file immediately,
>> +# the written data which is still in the buffer will be lost when the
>> +# corresponding inode is evicted.
>> +# It's a regression test for:
>> +# 'commit 26b15a042b8a ("fs: Handle I_DONTCACHE in iput_final() instead of generic_drop_inode()"'
>>
>>   seq=`basename $0`
>>   seqres=$RESULT_DIR/$seq
>> @@ -87,6 +93,26 @@ test_disable_dax()
>>   	_check_s_dax $t_file 0
>>   }
>>
>> +test_buffered_data_lost()
>> +{
>> +	local t_file=$SCRATCH_MNT/datafile
>> +
>> +	# Write data into a file
>> +	echo "Buffered data">  $t_file
>> +
>> +	# Then enable DAX on the file immediately
>> +	$XFS_IO_PROG -c 'chattr +x' $t_file
>> +
>> +	# Without commit e7bed0b5bc4b, ensure inode can
>> +	# be evicted by drop_caches
>> +	echo 2>  /proc/sys/vm/drop_caches
>> +
>> +	# The written data which is still in the buffer will be lost
>> +	grep -q "Buffered data" $t_file || echo "Buffered data is lost"
>> +
>> +	rm -f $t_file
>> +}
>> +
>>   do_tests()
>>   {
>>   	local mount_option=$1
>> @@ -100,6 +126,9 @@ do_tests()
>>   	test_enable_dax
>>   	test_disable_dax
>>
>> +	# Do test for commit 26b15a042b8a
>> +	test_buffered_data_lost
>> +
>>   	_scratch_unmount
>>   }
>>
>> -- 
>> 2.21.0
>>
>>
>>
> .
>

Re: [PATCH v2 1/2] generic/608: Remove drop cache and umount & mount cycle

[yangx.jy]

Hi All,

Sorry for the wrong commit ids on the v2 patchset.
I will correct them on v3 patchset soon.

Best Regards,
Xiao Yang
On 2021/3/29 17:20, Xiao Yang wrote:
> Since commit e7bed0b5bc4b, toggling FS_XFLAG_DAX on an existing file can make
> S_DAX on the file change immediately when all applications close the file.
> As a result, we don't need the redundant drop cache and umount & mount cycle.
>
> Signed-off-by: Xiaoli Feng <xifeng@redhat.com>
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
>  tests/generic/608 | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/tests/generic/608 b/tests/generic/608
> index 861bbff5..d81a2488 100755
> --- a/tests/generic/608
> +++ b/tests/generic/608
> @@ -3,10 +3,10 @@
>  # Copyright (c) 2020 Fujitsu.  All Rights Reserved.
>  #
>  # FS QA Test 608
> -# Change FS_XFLAG_DAX on an existing file and check if S_DAX on
> -# the file can take effect immediately by the following steps:
> -# 1) Stop all applications which are using the file.
> -# 2) Do drop_caches or umount & mount cycle.
> +# Toggling FS_XFLAG_DAX on an existing file can make S_DAX on the
> +# file change immediately when all applications close the file.
> +# It's a regression test for:
> +# 'commit e7bed0b5bc4b ("fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set")'
>  
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
> @@ -36,7 +36,7 @@ _require_dax_iflag
>  _require_xfs_io_command "lsattr" "-v"
>  _require_xfs_io_command "statx" "-r"
>  
> -test_drop_caches()
> +test_enable_dax()
>  {
>  	local t_file=$SCRATCH_MNT/testfile
>  
> @@ -49,24 +49,19 @@ test_drop_caches()
>  
>  	$XFS_IO_PROG -c 'chattr +x' $t_file
>  	_check_xflag $t_file 1
> -	_check_s_dax $t_file 0
> -
> -	# One application is using test file and check if S_DAX on
> -	# the file doesn't take effect immediately by drop_caches
> -	echo 2 > /proc/sys/vm/drop_caches
> +	# One application is using test file and S_DAX
> +	# on the file is not changed immediately
>  	_check_s_dax $t_file 0
>  
>  	exec 3<&-
>  
> -	# No application is using test file and check if S_DAX on
> -	# the file takes effect immediately by drop_caches
> -	echo 2 > /proc/sys/vm/drop_caches
> +	# No application is using test file and S_DAX
> +	# on the file is changed immediately
>  	_check_s_dax $t_file 1
>  }
>  
> -test_cycle_mount()
> +test_disable_dax()
>  {
> -	local option=$1
>  	local t_dir=$SCRATCH_MNT/testdir
>  	local t_file=$t_dir/testfile
>  
> @@ -81,29 +76,29 @@ test_cycle_mount()
>  
>  	$XFS_IO_PROG -c 'chattr -x' $t_file
>  	_check_xflag $t_file 0
> +	# One application is using test file and S_DAX
> +	# on the file is not changed immediately
>  	_check_s_dax $t_file 1
>  
>  	exec 3<&-
>  
> -	# No application is using test file and check if S_DAX on
> -	# the file takes effect immediately by umount & mount
> -	_scratch_cycle_mount "$option"
> +	# No application is using test file and S_DAX
> +	# on the file is changed immediately
>  	_check_s_dax $t_file 0
>  }
>  
>  do_tests()
>  {
>  	local mount_option=$1
> -	local cycle_mount_option=$2
>  
>  	_scratch_mount "$mount_option"
>  
>  	# Make sure the root dir doesn't have FS_XFLAG_DAX set before we start.
>  	$XFS_IO_PROG -c "chattr -x" $SCRATCH_MNT &>> $seqres.full
>  
> -	test_drop_caches
> -
> -	test_cycle_mount "$cycle_mount_option"
> +	# Do test for commit e7bed0b5bc4b
> +	test_enable_dax
> +	test_disable_dax
>  
>  	_scratch_unmount
>  }
> @@ -111,7 +106,7 @@ do_tests()
>  _scratch_mkfs >> $seqres.full 2>&1
>  
>  # Mount with dax option
> -do_tests "-o dax=inode" "dax=inode"
> +do_tests "-o dax=inode"
>  
>  # Mount without dax option
>  export MOUNT_OPTIONS=""