[PATCH v2 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified length

[PATCH v2 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified length

[Xiao Yang]

It is unnecssary to always create a dm-log-writes device
based on the entire size of the target/underlying device.

Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
 common/dmlogwrites | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/common/dmlogwrites b/common/dmlogwrites
index 9fa1c977..c1c85de9 100644
--- a/common/dmlogwrites
+++ b/common/dmlogwrites
@@ -59,14 +59,28 @@ _require_log_writes_dax_mountopt()
 	fi
 }
 
+# Set up a dm-log-writes device
+#
+# blkdev: the specified target device
+# length(optional): the mapped length in bytes
+# Note that the entire size of the target device will be used
+# if length is not specified.
 _log_writes_init()
 {
-	blkdev=$1
+	local blkdev=$1
+	local length=$2
+	local BLK_DEV_SIZE
 
 	[ -z "$blkdev" ] && _fail \
 	"block dev must be specified for _log_writes_init"
 
-	local BLK_DEV_SIZE=`blockdev --getsz $blkdev`
+	if [ -z "$length" ]; then
+		BLK_DEV_SIZE=`blockdev --getsz $blkdev`
+	else
+		local blksz=`blockdev --getss $blkdev`
+		BLK_DEV_SIZE=$((length / blksz))
+	fi
+
 	LOGWRITES_NAME=logwrites-test
 	LOGWRITES_DMDEV=/dev/mapper/$LOGWRITES_NAME
 	LOGWRITES_TABLE="0 $BLK_DEV_SIZE log-writes $blkdev $LOGWRITES_DEV"
-- 
2.34.1

[PATCH v2 2/2] generic/470: Replace thin volume with blkdiscard -z

[Xiao Yang]

generic/470 was original designed to verify mmap(MAP_SYNC) which
is only valid to the DAX capable device(e.g. PMEM). Thin volume[1] was
introduced to fix the inconsistent filesystem issue[2] but it make
the test become not run because it doesn't support DAX. As Darrick
mentioned[3], zeroing the entire mapped range of scartch device
can fix the issue as well, so I try to use blkdiscard -z instead.

[1]: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=fc5870da485aec0f9196a0f2bed32f73f6b2c664
[2]: https://lore.kernel.org/fstests/20190227061529.GF16436@dastard/
[3]: https://lore.kernel.org/linux-xfs/Y1NRNtToQTjs0Dbd@magnolia/T/#me0e77cb0ecd80bf4b5109e4433cb4863ae6e6727

Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
 tests/generic/470 | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/tests/generic/470 b/tests/generic/470
index f3407511..6da2ec22 100755
--- a/tests/generic/470
+++ b/tests/generic/470
@@ -15,38 +15,29 @@ _cleanup()
 {
 	cd /
 	_log_writes_cleanup
-	_dmthin_cleanup
 	rm -f $tmp.*
 }
 
 # Import common functions.
 . ./common/filter
-. ./common/dmthin
 . ./common/dmlogwrites
 
 # real QA test starts here
 _supported_fs generic
-_require_scratch_nocheck
+_require_scratch
 _require_no_logdev
 _require_log_writes_dax_mountopt "dax"
-_require_dm_target thin-pool
 _require_xfs_io_command "mmap" "-S"
 _require_xfs_io_command "log_writes"
+_require_command "$BLKDISCARD_PROG" blkdiscard
 
-devsize=$((1024*1024*200 / 512))        # 200m phys/virt size
-csize=$((1024*64 / 512))                # 64k cluster size
-lowspace=$((1024*1024 / 512))           # 1m low space threshold
-
-# Use a thin device to provide deterministic discard behavior. Discards are used
-# by the log replay tool for fast zeroing to prevent out-of-order replay issues.
-_dmthin_init $devsize $devsize $csize $lowspace
+MAPPED_LEN=$((512 * 1024 * 1024)) # 512 MiB
+LEN=$((1024 * 1024)) # 1 MiB
 
-_log_writes_init $DMTHIN_VOL_DEV
+_log_writes_init $SCRATCH_DEV $MAPPED_LEN
 _log_writes_mkfs >> $seqres.full 2>&1
 _log_writes_mount -o dax
 
-LEN=$((1024 * 1024)) # 1 MiB
-
 $XFS_IO_PROG -t -c "truncate $LEN" -c "mmap -S 0 $LEN" -c "mwrite 0 $LEN" \
 	-c "log_writes -d $LOGWRITES_NAME -m preunmap" \
 	-f $SCRATCH_MNT/test
@@ -54,14 +45,19 @@ $XFS_IO_PROG -t -c "truncate $LEN" -c "mmap -S 0 $LEN" -c "mwrite 0 $LEN" \
 # Unmount the scratch dir and tear down the log writes target
 _log_writes_unmount
 _log_writes_remove
-_dmthin_check_fs
+_check_scratch_fs
 
-# destroy previous filesystem so we can be sure our rebuild works
-_mkfs_dev $DMTHIN_VOL_DEV >> $seqres.full 2>&1
+# Forcibly zero the mapped range of scratch device and destroy
+# previous filesystem so we can be sure our rebuild works.
+# Note that blkdiscard -z will fall back to writing buffers of zeroes
+# if scratch device doesn't support write zeroes operation(i.e.
+# REQ_OP_WRITE_ZEROES).
+$BLKDISCARD_PROG -fzl $MAPPED_LEN $SCRATCH_DEV >> $seqres.full 2>&1
+_scratch_mkfs >> $seqres.full 2>&1
 
 # check pre-unmap state
-_log_writes_replay_log preunmap $DMTHIN_VOL_DEV
-_dmthin_mount
+_log_writes_replay_log preunmap $SCRATCH_DEV
+_scratch_mount
 
 # We should see $SCRATCH_MNT/test as having 1 MiB in block allocations
 du -sh $SCRATCH_MNT/test | _filter_scratch | _filter_spaces
-- 
2.34.1

Re: [PATCH v2 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified length

[Zorro Lang]

On Mon, Nov 14, 2022 at 08:35:02AM +0000, Xiao Yang wrote:
> It is unnecssary to always create a dm-log-writes device
> based on the entire size of the target/underlying device.
> 
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---

Darrick has given his RVB [1]:

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

I think you can keep it in this V2 patch.

Thanks,
Zorro

[1]
https://lore.kernel.org/fstests/Y3EvUgtux4tP1tdY@magnolia/T/#m83b97790ba6570874f36cd0601747e1b6b155de2

>  common/dmlogwrites | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/common/dmlogwrites b/common/dmlogwrites
> index 9fa1c977..c1c85de9 100644
> --- a/common/dmlogwrites
> +++ b/common/dmlogwrites
> @@ -59,14 +59,28 @@ _require_log_writes_dax_mountopt()
>  	fi
>  }
>  
> +# Set up a dm-log-writes device
> +#
> +# blkdev: the specified target device
> +# length(optional): the mapped length in bytes
> +# Note that the entire size of the target device will be used
> +# if length is not specified.
>  _log_writes_init()
>  {
> -	blkdev=$1
> +	local blkdev=$1
> +	local length=$2
> +	local BLK_DEV_SIZE
>  
>  	[ -z "$blkdev" ] && _fail \
>  	"block dev must be specified for _log_writes_init"
>  
> -	local BLK_DEV_SIZE=`blockdev --getsz $blkdev`
> +	if [ -z "$length" ]; then
> +		BLK_DEV_SIZE=`blockdev --getsz $blkdev`
> +	else
> +		local blksz=`blockdev --getss $blkdev`
> +		BLK_DEV_SIZE=$((length / blksz))
> +	fi
> +
>  	LOGWRITES_NAME=logwrites-test
>  	LOGWRITES_DMDEV=/dev/mapper/$LOGWRITES_NAME
>  	LOGWRITES_TABLE="0 $BLK_DEV_SIZE log-writes $blkdev $LOGWRITES_DEV"
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified length

[Darrick J. Wong]

On Mon, Nov 14, 2022 at 08:35:02AM +0000, Xiao Yang wrote:
> It is unnecssary to always create a dm-log-writes device
> based on the entire size of the target/underlying device.
> 
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  common/dmlogwrites | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/common/dmlogwrites b/common/dmlogwrites
> index 9fa1c977..c1c85de9 100644
> --- a/common/dmlogwrites
> +++ b/common/dmlogwrites
> @@ -59,14 +59,28 @@ _require_log_writes_dax_mountopt()
>  	fi
>  }
>  
> +# Set up a dm-log-writes device
> +#
> +# blkdev: the specified target device
> +# length(optional): the mapped length in bytes
> +# Note that the entire size of the target device will be used
> +# if length is not specified.
>  _log_writes_init()
>  {
> -	blkdev=$1
> +	local blkdev=$1
> +	local length=$2
> +	local BLK_DEV_SIZE
>  
>  	[ -z "$blkdev" ] && _fail \
>  	"block dev must be specified for _log_writes_init"
>  
> -	local BLK_DEV_SIZE=`blockdev --getsz $blkdev`
> +	if [ -z "$length" ]; then
> +		BLK_DEV_SIZE=`blockdev --getsz $blkdev`
> +	else
> +		local blksz=`blockdev --getss $blkdev`
> +		BLK_DEV_SIZE=$((length / blksz))
> +	fi
> +
>  	LOGWRITES_NAME=logwrites-test
>  	LOGWRITES_DMDEV=/dev/mapper/$LOGWRITES_NAME
>  	LOGWRITES_TABLE="0 $BLK_DEV_SIZE log-writes $blkdev $LOGWRITES_DEV"
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/2] generic/470: Replace thin volume with blkdiscard -z

[Darrick J. Wong]

On Mon, Nov 14, 2022 at 08:35:03AM +0000, Xiao Yang wrote:
> generic/470 was original designed to verify mmap(MAP_SYNC) which
> is only valid to the DAX capable device(e.g. PMEM). Thin volume[1] was
> introduced to fix the inconsistent filesystem issue[2] but it make
> the test become not run because it doesn't support DAX. As Darrick
> mentioned[3], zeroing the entire mapped range of scartch device
> can fix the issue as well, so I try to use blkdiscard -z instead.
> 
> [1]: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=fc5870da485aec0f9196a0f2bed32f73f6b2c664
> [2]: https://lore.kernel.org/fstests/20190227061529.GF16436@dastard/
> [3]: https://lore.kernel.org/linux-xfs/Y1NRNtToQTjs0Dbd@magnolia/T/#me0e77cb0ecd80bf4b5109e4433cb4863ae6e6727
> 
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
>  tests/generic/470 | 34 +++++++++++++++-------------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/generic/470 b/tests/generic/470
> index f3407511..6da2ec22 100755
> --- a/tests/generic/470
> +++ b/tests/generic/470
> @@ -15,38 +15,29 @@ _cleanup()
>  {
>  	cd /
>  	_log_writes_cleanup
> -	_dmthin_cleanup
>  	rm -f $tmp.*
>  }
>  
>  # Import common functions.
>  . ./common/filter
> -. ./common/dmthin
>  . ./common/dmlogwrites
>  
>  # real QA test starts here
>  _supported_fs generic
> -_require_scratch_nocheck
> +_require_scratch
>  _require_no_logdev
>  _require_log_writes_dax_mountopt "dax"
> -_require_dm_target thin-pool
>  _require_xfs_io_command "mmap" "-S"
>  _require_xfs_io_command "log_writes"
> +_require_command "$BLKDISCARD_PROG" blkdiscard
>  
> -devsize=$((1024*1024*200 / 512))        # 200m phys/virt size
> -csize=$((1024*64 / 512))                # 64k cluster size
> -lowspace=$((1024*1024 / 512))           # 1m low space threshold
> -
> -# Use a thin device to provide deterministic discard behavior. Discards are used
> -# by the log replay tool for fast zeroing to prevent out-of-order replay issues.
> -_dmthin_init $devsize $devsize $csize $lowspace
> +MAPPED_LEN=$((512 * 1024 * 1024)) # 512 MiB
> +LEN=$((1024 * 1024)) # 1 MiB
>  
> -_log_writes_init $DMTHIN_VOL_DEV
> +_log_writes_init $SCRATCH_DEV $MAPPED_LEN
>  _log_writes_mkfs >> $seqres.full 2>&1
>  _log_writes_mount -o dax
>  
> -LEN=$((1024 * 1024)) # 1 MiB
> -
>  $XFS_IO_PROG -t -c "truncate $LEN" -c "mmap -S 0 $LEN" -c "mwrite 0 $LEN" \
>  	-c "log_writes -d $LOGWRITES_NAME -m preunmap" \
>  	-f $SCRATCH_MNT/test
> @@ -54,14 +45,19 @@ $XFS_IO_PROG -t -c "truncate $LEN" -c "mmap -S 0 $LEN" -c "mwrite 0 $LEN" \
>  # Unmount the scratch dir and tear down the log writes target
>  _log_writes_unmount
>  _log_writes_remove
> -_dmthin_check_fs
> +_check_scratch_fs
>  
> -# destroy previous filesystem so we can be sure our rebuild works
> -_mkfs_dev $DMTHIN_VOL_DEV >> $seqres.full 2>&1
> +# Forcibly zero the mapped range of scratch device and destroy
> +# previous filesystem so we can be sure our rebuild works.
> +# Note that blkdiscard -z will fall back to writing buffers of zeroes
> +# if scratch device doesn't support write zeroes operation(i.e.
> +# REQ_OP_WRITE_ZEROES).
> +$BLKDISCARD_PROG -fzl $MAPPED_LEN $SCRATCH_DEV >> $seqres.full 2>&1
> +_scratch_mkfs >> $seqres.full 2>&1

Thanks for fixing the comment, apologies for the delays.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  
>  # check pre-unmap state
> -_log_writes_replay_log preunmap $DMTHIN_VOL_DEV
> -_dmthin_mount
> +_log_writes_replay_log preunmap $SCRATCH_DEV
> +_scratch_mount
>  
>  # We should see $SCRATCH_MNT/test as having 1 MiB in block allocations
>  du -sh $SCRATCH_MNT/test | _filter_scratch | _filter_spaces
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified length

[Yang, Xiao/杨 晓]

On 2022/11/14 22:56, Zorro Lang wrote:
> On Mon, Nov 14, 2022 at 08:35:02AM +0000, Xiao Yang wrote:
>> It is unnecssary to always create a dm-log-writes device
>> based on the entire size of the target/underlying device.
>>
>> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
>> ---
> 
> Darrick has given his RVB [1]:
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> I think you can keep it in this V2 patch.

Hi Zorro,

Thanks for your reminder.

As Darrick suggested, I actually added a comment to explain the blkdev 
and length arguments. So I hope Darrick can review it again.

Fortunately, Darrick has added reviewed-by on the v2 patch set. ^_^

Best Regards,
Xiao Yang
> 
> Thanks,
> Zorro
> 
> [1]
> https://lore.kernel.org/fstests/Y3EvUgtux4tP1tdY@magnolia/T/#m83b97790ba6570874f36cd0601747e1b6b155de2
> 
>>   common/dmlogwrites | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/dmlogwrites b/common/dmlogwrites
>> index 9fa1c977..c1c85de9 100644
>> --- a/common/dmlogwrites
>> +++ b/common/dmlogwrites
>> @@ -59,14 +59,28 @@ _require_log_writes_dax_mountopt()
>>   	fi
>>   }
>>   
>> +# Set up a dm-log-writes device
>> +#
>> +# blkdev: the specified target device
>> +# length(optional): the mapped length in bytes
>> +# Note that the entire size of the target device will be used
>> +# if length is not specified.
>>   _log_writes_init()
>>   {
>> -	blkdev=$1
>> +	local blkdev=$1
>> +	local length=$2
>> +	local BLK_DEV_SIZE
>>   
>>   	[ -z "$blkdev" ] && _fail \
>>   	"block dev must be specified for _log_writes_init"
>>   
>> -	local BLK_DEV_SIZE=`blockdev --getsz $blkdev`
>> +	if [ -z "$length" ]; then
>> +		BLK_DEV_SIZE=`blockdev --getsz $blkdev`
>> +	else
>> +		local blksz=`blockdev --getss $blkdev`
>> +		BLK_DEV_SIZE=$((length / blksz))
>> +	fi
>> +
>>   	LOGWRITES_NAME=logwrites-test
>>   	LOGWRITES_DMDEV=/dev/mapper/$LOGWRITES_NAME
>>   	LOGWRITES_TABLE="0 $BLK_DEV_SIZE log-writes $blkdev $LOGWRITES_DEV"
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v2 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified length

[Zorro Lang]

On Tue, Nov 15, 2022 at 03:05:10PM +0800, Yang, Xiao/杨 晓 wrote:
> On 2022/11/14 22:56, Zorro Lang wrote:
> > On Mon, Nov 14, 2022 at 08:35:02AM +0000, Xiao Yang wrote:
> > > It is unnecssary to always create a dm-log-writes device
> > > based on the entire size of the target/underlying device.
> > > 
> > > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> > > ---
> > 
> > Darrick has given his RVB [1]:
> > 
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > 
> > I think you can keep it in this V2 patch.
> 
> Hi Zorro,
> 
> Thanks for your reminder.

np, I've tested and merged this patchset, and will push this weekend.

Thanks,
Zorro

> 
> As Darrick suggested, I actually added a comment to explain the blkdev and
> length arguments. So I hope Darrick can review it again.
> 
> Fortunately, Darrick has added reviewed-by on the v2 patch set. ^_^
> 
> Best Regards,
> Xiao Yang
> > 
> > Thanks,
> > Zorro
> > 
> > [1]
> > https://lore.kernel.org/fstests/Y3EvUgtux4tP1tdY@magnolia/T/#m83b97790ba6570874f36cd0601747e1b6b155de2
> > 
> > >   common/dmlogwrites | 18 ++++++++++++++++--
> > >   1 file changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/common/dmlogwrites b/common/dmlogwrites
> > > index 9fa1c977..c1c85de9 100644
> > > --- a/common/dmlogwrites
> > > +++ b/common/dmlogwrites
> > > @@ -59,14 +59,28 @@ _require_log_writes_dax_mountopt()
> > >   	fi
> > >   }
> > > +# Set up a dm-log-writes device
> > > +#
> > > +# blkdev: the specified target device
> > > +# length(optional): the mapped length in bytes
> > > +# Note that the entire size of the target device will be used
> > > +# if length is not specified.
> > >   _log_writes_init()
> > >   {
> > > -	blkdev=$1
> > > +	local blkdev=$1
> > > +	local length=$2
> > > +	local BLK_DEV_SIZE
> > >   	[ -z "$blkdev" ] && _fail \
> > >   	"block dev must be specified for _log_writes_init"
> > > -	local BLK_DEV_SIZE=`blockdev --getsz $blkdev`
> > > +	if [ -z "$length" ]; then
> > > +		BLK_DEV_SIZE=`blockdev --getsz $blkdev`
> > > +	else
> > > +		local blksz=`blockdev --getss $blkdev`
> > > +		BLK_DEV_SIZE=$((length / blksz))
> > > +	fi
> > > +
> > >   	LOGWRITES_NAME=logwrites-test
> > >   	LOGWRITES_DMDEV=/dev/mapper/$LOGWRITES_NAME
> > >   	LOGWRITES_TABLE="0 $BLK_DEV_SIZE log-writes $blkdev $LOGWRITES_DEV"
> > > -- 
> > > 2.34.1
> > > 
> > 
>