All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RESEND 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified size
@ 2022-10-23  6:48 yangx.jy
  2022-10-23  6:48 ` [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z yangx.jy
  2022-11-13 17:54 ` [PATCH RESEND 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified size Darrick J. Wong
  0 siblings, 2 replies; 10+ messages in thread
From: yangx.jy @ 2022-10-23  6:48 UTC (permalink / raw)
  To: djwong, zlang; +Cc: fstests, bfoster, Yasunori Gotou (Fujitsu), yangx.jy

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

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

diff --git a/common/dmlogwrites b/common/dmlogwrites
index 9fa1c977..d7b23cec 100644
--- a/common/dmlogwrites
+++ b/common/dmlogwrites
@@ -61,12 +61,20 @@ _require_log_writes_dax_mountopt()
 
 _log_writes_init()
 {
-	blkdev=$1
+	local blkdev=$1
+	local range=$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 "$range" ]; then
+		BLK_DEV_SIZE=`blockdev --getsz $blkdev`
+	else
+		local blksz=`blockdev --getss $blkdev`
+		BLK_DEV_SIZE=$((range / 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
  2022-10-23  6:48 [PATCH RESEND 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified size yangx.jy
@ 2022-10-23  6:48 ` yangx.jy
  2022-10-24  4:09   ` Darrick J. Wong
  2022-11-13 17:57   ` Darrick J. Wong
  2022-11-13 17:54 ` [PATCH RESEND 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified size Darrick J. Wong
  1 sibling, 2 replies; 10+ messages in thread
From: yangx.jy @ 2022-10-23  6:48 UTC (permalink / raw)
  To: djwong, zlang; +Cc: fstests, bfoster, Yasunori Gotou (Fujitsu), yangx.jy

generic/470 was original designed to verify mmap(MAP_SYNC) which
is only vaild 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], discarding 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 | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/tests/generic/470 b/tests/generic/470
index f3407511..d2846a38 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
+RANGE=$((512 * 1024 * 1024)) # 512 MiB
+LEN=$((1024 * 1024)) # 1 MiB
 
-_log_writes_init $DMTHIN_VOL_DEV
+_log_writes_init $SCRATCH_DEV $RANGE
 _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,16 @@ $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
+# Discard the mapped range of scratch device and destroy previous filesystem
+# so we can be sure our rebuild works
+$BLKDISCARD_PROG -fzl $RANGE $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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
  2022-10-23  6:48 ` [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z yangx.jy
@ 2022-10-24  4:09   ` Darrick J. Wong
  2022-10-24  7:15     ` Yang, Xiao/杨 晓
  2022-11-13 17:57   ` Darrick J. Wong
  1 sibling, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2022-10-24  4:09 UTC (permalink / raw)
  To: yangx.jy; +Cc: zlang, fstests, bfoster, Yasunori Gotou (Fujitsu)

On Sun, Oct 23, 2022 at 06:48:13AM +0000, yangx.jy@fujitsu.com wrote:
> generic/470 was original designed to verify mmap(MAP_SYNC) which
> is only vaild 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], discarding the entire mapped range of scartch device
> can fix the issue as well, so I try to use blkdiscard -z instead.

That might be ok for the *other* dm-logwrites tests, but isn't the
fundamental problem here (generic/470, specifically) that device mapper
cannot run on top of pmem?

--D

> [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 | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/generic/470 b/tests/generic/470
> index f3407511..d2846a38 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
> +RANGE=$((512 * 1024 * 1024)) # 512 MiB
> +LEN=$((1024 * 1024)) # 1 MiB
>  
> -_log_writes_init $DMTHIN_VOL_DEV
> +_log_writes_init $SCRATCH_DEV $RANGE
>  _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,16 @@ $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
> +# Discard the mapped range of scratch device and destroy previous filesystem
> +# so we can be sure our rebuild works
> +$BLKDISCARD_PROG -fzl $RANGE $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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
  2022-10-24  4:09   ` Darrick J. Wong
@ 2022-10-24  7:15     ` Yang, Xiao/杨 晓
  2022-10-30  7:30       ` yangx.jy
  0 siblings, 1 reply; 10+ messages in thread
From: Yang, Xiao/杨 晓 @ 2022-10-24  7:15 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, fstests, bfoster

On 2022/10/24 12:09, Darrick J. Wong wrote:
> On Sun, Oct 23, 2022 at 06:48:13AM +0000,yangx.jy@fujitsu.com  wrote:
>> generic/470 was original designed to verify mmap(MAP_SYNC) which
>> is only vaild 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], discarding the entire mapped range of scartch device
>> can fix the issue as well, so I try to use blkdiscard -z instead.
> That might be ok for the*other*  dm-logwrites tests, but isn't the
> fundamental problem here (generic/470, specifically) that device mapper
> cannot run on top of pmem?

Hi Darrick,

With the change，I didn't find any failure when running generic/470 in 
loops.
--------------------------------------------------------------
[root@fedora35 xfstests-dev]# ./check generic/470
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 fedora35 6.1.0-rc1+ #37 SMP 
PREEMPT_DYNAMIC Fri Oct 21 19:04:57 CST 2022
MKFS_OPTIONS  -- -f /dev/pmem0
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/pmem0 
/mnt/scratch

generic/470 6s
Ran: generic/470
Passed all 1 tests
--------------------------------------------------------------
Both dm-log-writes and PMEM support DAX so it's fine to verify 
mmap(MAP_SYNC) with the dm-log-writes device on top of PMEM.

Did I miss something? Why do you think there is a fundamental problem here?

Best Regards,
Xiao Yang

> 
> --D
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
  2022-10-24  7:15     ` Yang, Xiao/杨 晓
@ 2022-10-30  7:30       ` yangx.jy
  2022-11-01  3:48         ` Darrick J. Wong
  0 siblings, 1 reply; 10+ messages in thread
From: yangx.jy @ 2022-10-30  7:30 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, fstests, bfoster

Hi Darrick,

Ping, is there any feedback on the patch?

Best Regards,
Xiao Yang

-----Original Message-----
From: Yang, Xiao/杨 晓 <yangx.jy@fujitsu.com> 
Sent: 2022年10月24日 15:16
To: Darrick J. Wong <djwong@kernel.org>
Cc: zlang@redhat.com; fstests@vger.kernel.org; bfoster@redhat.com
Subject: Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z

On 2022/10/24 12:09, Darrick J. Wong wrote:
> On Sun, Oct 23, 2022 at 06:48:13AM +0000,yangx.jy@fujitsu.com  wrote:
>> generic/470 was original designed to verify mmap(MAP_SYNC) which is 
>> only vaild 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], discarding the entire mapped range of scartch device 
>> can fix the issue as well, so I try to use blkdiscard -z instead.
> That might be ok for the*other*  dm-logwrites tests, but isn't the 
> fundamental problem here (generic/470, specifically) that device 
> mapper cannot run on top of pmem?

Hi Darrick,

With the change，I didn't find any failure when running generic/470 in loops.
--------------------------------------------------------------
[root@fedora35 xfstests-dev]# ./check generic/470
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 fedora35 6.1.0-rc1+ #37 SMP 
PREEMPT_DYNAMIC Fri Oct 21 19:04:57 CST 2022 MKFS_OPTIONS  -- -f /dev/pmem0 MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/pmem0 /mnt/scratch

generic/470 6s
Ran: generic/470
Passed all 1 tests
--------------------------------------------------------------
Both dm-log-writes and PMEM support DAX so it's fine to verify
mmap(MAP_SYNC) with the dm-log-writes device on top of PMEM.

Did I miss something? Why do you think there is a fundamental problem here?

Best Regards,
Xiao Yang

> 
> --D
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
  2022-10-30  7:30       ` yangx.jy
@ 2022-11-01  3:48         ` Darrick J. Wong
  2022-11-12  5:48           ` Yang, Xiao/杨 晓
  0 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2022-11-01  3:48 UTC (permalink / raw)
  To: yangx.jy; +Cc: zlang, fstests, bfoster

On Sun, Oct 30, 2022 at 07:30:31AM +0000, yangx.jy@fujitsu.com wrote:
> Hi Darrick,
> 
> Ping, is there any feedback on the patch?
> 
> Best Regards,
> Xiao Yang
> 
> -----Original Message-----
> From: Yang, Xiao/杨 晓 <yangx.jy@fujitsu.com> 
> Sent: 2022年10月24日 15:16
> To: Darrick J. Wong <djwong@kernel.org>
> Cc: zlang@redhat.com; fstests@vger.kernel.org; bfoster@redhat.com
> Subject: Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
> 
> On 2022/10/24 12:09, Darrick J. Wong wrote:
> > On Sun, Oct 23, 2022 at 06:48:13AM +0000,yangx.jy@fujitsu.com  wrote:
> >> generic/470 was original designed to verify mmap(MAP_SYNC) which is 
> >> only vaild 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], discarding the entire mapped range of scartch device 
> >> can fix the issue as well, so I try to use blkdiscard -z instead.
> > That might be ok for the*other*  dm-logwrites tests, but isn't the 
> > fundamental problem here (generic/470, specifically) that device 
> > mapper cannot run on top of pmem?
> 
> Hi Darrick,
> 
> With the change，I didn't find any failure when running generic/470 in loops.
> --------------------------------------------------------------
> [root@fedora35 xfstests-dev]# ./check generic/470
> FSTYP         -- xfs (non-debug)
> PLATFORM      -- Linux/x86_64 fedora35 6.1.0-rc1+ #37 SMP 
> PREEMPT_DYNAMIC Fri Oct 21 19:04:57 CST 2022 MKFS_OPTIONS  -- -f /dev/pmem0 MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/pmem0 /mnt/scratch
> 
> generic/470 6s
> Ran: generic/470
> Passed all 1 tests
> --------------------------------------------------------------
> Both dm-log-writes and PMEM support DAX so it's fine to verify
> mmap(MAP_SYNC) with the dm-log-writes device on top of PMEM.
> 
> Did I miss something? Why do you think there is a fundamental problem here?

Nope, you're right.  fsdax works fine, at least on these simple(r)
device mapper devices:

$ git grep -c dax drivers/md/
drivers/md/dm-core.h:1
drivers/md/dm-linear.c:19
drivers/md/dm-log-writes.c:19
drivers/md/dm-stripe.c:19
drivers/md/dm-table.c:18
drivers/md/dm-target.c:4
drivers/md/dm-writecache.c:7
drivers/md/dm.c:36

(Most notably, dm-logwrites :))

I'll go look at the test in the morning.

--D

> Best Regards,
> Xiao Yang
> 
> > 
> > --D
> > 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
  2022-11-01  3:48         ` Darrick J. Wong
@ 2022-11-12  5:48           ` Yang, Xiao/杨 晓
  0 siblings, 0 replies; 10+ messages in thread
From: Yang, Xiao/杨 晓 @ 2022-11-12  5:48 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, fstests, bfoster

On 2022/11/1 11:48, Darrick J. Wong wrote:
> On Sun, Oct 30, 2022 at 07:30:31AM +0000, yangx.jy@fujitsu.com wrote:
>> Hi Darrick,
>>
>> Ping, is there any feedback on the patch?
>>
>> Best Regards,
>> Xiao Yang
>>
>> -----Original Message-----
>> From: Yang, Xiao/杨 晓 <yangx.jy@fujitsu.com>
>> Sent: 2022年10月24日 15:16
>> To: Darrick J. Wong <djwong@kernel.org>
>> Cc: zlang@redhat.com; fstests@vger.kernel.org; bfoster@redhat.com
>> Subject: Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
>>
>> On 2022/10/24 12:09, Darrick J. Wong wrote:
>>> On Sun, Oct 23, 2022 at 06:48:13AM +0000,yangx.jy@fujitsu.com  wrote:
>>>> generic/470 was original designed to verify mmap(MAP_SYNC) which is
>>>> only vaild 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], discarding the entire mapped range of scartch device
>>>> can fix the issue as well, so I try to use blkdiscard -z instead.
>>> That might be ok for the*other*  dm-logwrites tests, but isn't the
>>> fundamental problem here (generic/470, specifically) that device
>>> mapper cannot run on top of pmem?
>>
>> Hi Darrick,
>>
>> With the change，I didn't find any failure when running generic/470 in loops.
>> --------------------------------------------------------------
>> [root@fedora35 xfstests-dev]# ./check generic/470
>> FSTYP         -- xfs (non-debug)
>> PLATFORM      -- Linux/x86_64 fedora35 6.1.0-rc1+ #37 SMP
>> PREEMPT_DYNAMIC Fri Oct 21 19:04:57 CST 2022 MKFS_OPTIONS  -- -f /dev/pmem0 MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/pmem0 /mnt/scratch
>>
>> generic/470 6s
>> Ran: generic/470
>> Passed all 1 tests
>> --------------------------------------------------------------
>> Both dm-log-writes and PMEM support DAX so it's fine to verify
>> mmap(MAP_SYNC) with the dm-log-writes device on top of PMEM.
>>
>> Did I miss something? Why do you think there is a fundamental problem here?
> 
> Nope, you're right.  fsdax works fine, at least on these simple(r)
> device mapper devices:
> 
> $ git grep -c dax drivers/md/
> drivers/md/dm-core.h:1
> drivers/md/dm-linear.c:19
> drivers/md/dm-log-writes.c:19
> drivers/md/dm-stripe.c:19
> drivers/md/dm-table.c:18
> drivers/md/dm-target.c:4
> drivers/md/dm-writecache.c:7
> drivers/md/dm.c:36
> 
> (Most notably, dm-logwrites :))
> 
> I'll go look at the test in the morning.

Hi Darrick,

Do you have time to look at it?
I hope you can give me reviewed-by if you think this patch set is OK.

Best Regards,
Xiao Yang

> 
> --D
> 
>> Best Regards,
>> Xiao Yang
>>
>>>
>>> --D
>>>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified size
  2022-10-23  6:48 [PATCH RESEND 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified size yangx.jy
  2022-10-23  6:48 ` [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z yangx.jy
@ 2022-11-13 17:54 ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2022-11-13 17:54 UTC (permalink / raw)
  To: yangx.jy; +Cc: zlang, fstests, bfoster, Yasunori Gotou (Fujitsu)

On Sun, Oct 23, 2022 at 06:48:13AM +0000, yangx.jy@fujitsu.com wrote:
> It is unnecssary to always create a log-writes device based on
> the size of the entire underlying device.
> 
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
>  common/dmlogwrites | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/common/dmlogwrites b/common/dmlogwrites
> index 9fa1c977..d7b23cec 100644
> --- a/common/dmlogwrites
> +++ b/common/dmlogwrites
> @@ -61,12 +61,20 @@ _require_log_writes_dax_mountopt()
>  
>  _log_writes_init()
>  {
> -	blkdev=$1
> +	local blkdev=$1
> +	local range=$2

Range is a length in ... bytes?  The unit ought to be recorded in a
comment for this function.

With that fixed, this looks ok to me
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +	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 "$range" ]; then
> +		BLK_DEV_SIZE=`blockdev --getsz $blkdev`
> +	else
> +		local blksz=`blockdev --getss $blkdev`
> +		BLK_DEV_SIZE=$((range / 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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
  2022-10-23  6:48 ` [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z yangx.jy
  2022-10-24  4:09   ` Darrick J. Wong
@ 2022-11-13 17:57   ` Darrick J. Wong
  2022-11-14  8:38     ` Yang, Xiao/杨 晓
  1 sibling, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2022-11-13 17:57 UTC (permalink / raw)
  To: yangx.jy; +Cc: zlang, fstests, bfoster, Yasunori Gotou (Fujitsu)

On Sun, Oct 23, 2022 at 06:48:13AM +0000, yangx.jy@fujitsu.com wrote:
> generic/470 was original designed to verify mmap(MAP_SYNC) which
> is only vaild 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], discarding 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 | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/generic/470 b/tests/generic/470
> index f3407511..d2846a38 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
> +RANGE=$((512 * 1024 * 1024)) # 512 MiB
> +LEN=$((1024 * 1024)) # 1 MiB
>  
> -_log_writes_init $DMTHIN_VOL_DEV
> +_log_writes_init $SCRATCH_DEV $RANGE
>  _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,16 @@ $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
> +# Discard the mapped range of scratch device and destroy previous filesystem
> +# so we can be sure our rebuild works
> +$BLKDISCARD_PROG -fzl $RANGE $SCRATCH_DEV >> $seqres.full 2>&1

You used -z, which means the comment should say "Forcibly zero the
mapped range of the scratch device..." and note that blkdiscard -z will
fall back to writing buffers of zeroes if the device doesn't support
WRITE SAME with zeroes.

I think the logic in here looks good though.

--D

> +_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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z
  2022-11-13 17:57   ` Darrick J. Wong
@ 2022-11-14  8:38     ` Yang, Xiao/杨 晓
  0 siblings, 0 replies; 10+ messages in thread
From: Yang, Xiao/杨 晓 @ 2022-11-14  8:38 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, fstests, bfoster, Yasunori Gotou (Fujitsu)

On 2022/11/14 1:57, Darrick J. Wong wrote:
> You used -z, which means the comment should say "Forcibly zero the
> mapped range of the scratch device..." and note that blkdiscard -z will
> fall back to writing buffers of zeroes if the device doesn't support
> WRITE SAME with zeroes.
> 
> I think the logic in here looks good though.
> 
> --D

Hi Darrick,

Thanks a lot for your comment.
I have sent the v2 patch set[1][2] as you suggested. I hope you can 
review them again.

[1] 
https://lore.kernel.org/fstests/1668414903-13-1-git-send-email-yangx.jy@fujitsu.com/T/#t
[2] 
https://lore.kernel.org/fstests/1668414903-13-2-git-send-email-yangx.jy@fujitsu.com/T/#u

Best Regards,
Xiao Yang

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-11-14  8:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-23  6:48 [PATCH RESEND 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified size yangx.jy
2022-10-23  6:48 ` [PATCH RESEND 2/2] generic/470: Replace thin volume with blkdiscard -z yangx.jy
2022-10-24  4:09   ` Darrick J. Wong
2022-10-24  7:15     ` Yang, Xiao/杨 晓
2022-10-30  7:30       ` yangx.jy
2022-11-01  3:48         ` Darrick J. Wong
2022-11-12  5:48           ` Yang, Xiao/杨 晓
2022-11-13 17:57   ` Darrick J. Wong
2022-11-14  8:38     ` Yang, Xiao/杨 晓
2022-11-13 17:54 ` [PATCH RESEND 1/2] common/dmlogwrites: Extend _log_writes_init() to accept the specified size Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.