[PATCH blktests 0/3] fix for xfs log size change from new version of xfsprogs

[PATCH blktests 0/3] fix for xfs log size change from new version of xfsprogs

[Yi Zhang]

Hi

This first patch addressed nvme/012 nvme/013 failure which introduced from xfsprogs
v5.19.0, the minimum xfs log size changed to 64m.

The second patch introduced one new function _get_test_dev_size_mb.

The third patch updated _xfs_run_fio_verify_io to accept one new
parameter size which will be used for nvme/012 nvme/013 nvme/015



Yi Zhang (3):
  common/xfs: set the minimal log size 64m during mkfs.xfs
  common/rc: add one function to get test dev size in mb
  common/xfs: update _xfs_run_fio_verify_io to accept the size parameter

 common/rc      | 8 ++++++++
 common/xfs     | 5 +++--
 tests/nvme/012 | 2 +-
 tests/nvme/013 | 2 +-
 tests/nvme/035 | 9 ++++++++-
 5 files changed, 21 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH blktests 1/3] common/xfs: set the minimal log size 64m during mkfs.xfs

[Yi Zhang]

Update to use the new minimum xfs log size 64MB which introudced from
xfsprogs v5.19.0:

$ mkfs.xfs -l size=32m -f /dev/nvme0n1
Log size must be at least 64MB.
Usage: mkfs.xfs
/* blocksize */		[-b size=num]
/* config file */	[-c options=xxx]
/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1,
			    inobtcount=0|1,bigtime=0|1]
/* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,
			    (sunit=value,swidth=value|su=num,sw=num|noalign),
			    sectsize=num
/* force overwrite */	[-f]
/* inode size */	[-i perblock=n|size=num,maxpct=n,attr=0|1|2,
			    projid32bit=0|1,sparse=0|1,nrext64=0|1]
/* no discard */	[-K]
/* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n
			    sunit=value|su=num,sectsize=num,lazy-count=0|1]
/* label */		[-L label (maximum 12 characters)]
/* naming */		[-n size=num,version=2|ci,ftype=0|1]
/* no-op info only */	[-N]
/* prototype file */	[-p fname]
/* quiet */		[-q]
/* realtime subvol */	[-r extsize=num,size=num,rtdev=xxx]
/* sectorsize */	[-s size=num]
/* version */		[-V]
			devicename
<devicename> is required unless -d name=xxx is given.
<num> is xxx (bytes), xxxs (sectors), xxxb (fs blocks), xxxk (xxx KiB),
      xxxm (xxx MiB), xxxg (xxx GiB), xxxt (xxx TiB) or xxxp (xxx PiB).
<value> is xxx (512 byte blocks).

Signed-off-by: Yi Zhang <yi.zhang@redhat.com>
---
 common/xfs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/xfs b/common/xfs
index 210c924..846a5ef 100644
--- a/common/xfs
+++ b/common/xfs
@@ -16,7 +16,7 @@ _xfs_mkfs_and_mount() {
 
 	mkdir -p "${mount_dir}"
 	umount "${mount_dir}"
-	mkfs.xfs -l size=32m -f "${bdev}"
+	mkfs.xfs -l size=64m -f "${bdev}"
 	mount "${bdev}" "${mount_dir}"
 }
 
-- 
2.34.1

[PATCH blktests 2/3] common/rc: add one function to get test dev size in mb

[Yi Zhang]

Signed-off-by: Yi Zhang <yi.zhang@redhat.com>
---
 common/rc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/common/rc b/common/rc
index e490041..847be1b 100644
--- a/common/rc
+++ b/common/rc
@@ -324,6 +324,14 @@ _get_pci_parent_from_blkdev() {
 		tail -2 | head -1
 }
 
+_get_test_dev_size_mb() {
+	local test_dev_sz
+	test_dev_sz=$(blockdev --getsize64 "$TEST_DEV")
+
+	echo $((test_dev_sz / 1024 / 1024))
+
+}
+
 _require_test_dev_in_hotplug_slot() {
 	local parent
 	parent="$(_get_pci_parent_from_blkdev)"
-- 
2.34.1

Re: [PATCH blktests 2/3] common/rc: add one function to get test dev size in mb

[Shinichiro Kawasaki]

On Oct 24, 2022 / 14:13, Yi Zhang wrote:

Short explanation will help to understand why we do this: something like,

  nvme/035 has minimum TEST_DEV size requirement. Add a helper
  function to check it.

> Signed-off-by: Yi Zhang <yi.zhang@redhat.com>
> ---
>  common/rc | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index e490041..847be1b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -324,6 +324,14 @@ _get_pci_parent_from_blkdev() {
>  		tail -2 | head -1
>  }
>  
> +_get_test_dev_size_mb() {
> +	local test_dev_sz

Nit: one empty line will make it easier to read.

> +	test_dev_sz=$(blockdev --getsize64 "$TEST_DEV")
> +
> +	echo $((test_dev_sz / 1024 / 1024))
> +

Nit: an empty line not needed.

> +}
> +

I suggest to improve this new function to _require_test_dev_size_mb(). It takes
1st argument as the minimum size size in MB, and if TEST_DEV size is smaller
than that, it set SKIP_REASON and return 1. We can add device_requires() to
nvme/035 to call _require_test_dev_size_mb(). This will skip the test case when
the TEST_DEV is small, and do not report it as a failure.

I also suggest to include nvme/035 change for the size check in this patch. I
think one shot change for function addition and function call will be simpler
for this charge.

>  _require_test_dev_in_hotplug_slot() {
>  	local parent
>  	parent="$(_get_pci_parent_from_blkdev)"
> -- 
> 2.34.1
> 

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 2/3] common/rc: add one function to get test dev size in mb

[Yi Zhang]

On Tue, Oct 25, 2022 at 10:29 AM Shinichiro Kawasaki
<shinichiro.kawasaki@wdc.com> wrote:
>
> On Oct 24, 2022 / 14:13, Yi Zhang wrote:
>
> Short explanation will help to understand why we do this: something like,
>
>   nvme/035 has minimum TEST_DEV size requirement. Add a helper
>   function to check it.
>
> > Signed-off-by: Yi Zhang <yi.zhang@redhat.com>
> > ---
> >  common/rc | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/common/rc b/common/rc
> > index e490041..847be1b 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -324,6 +324,14 @@ _get_pci_parent_from_blkdev() {
> >               tail -2 | head -1
> >  }
> >
> > +_get_test_dev_size_mb() {
> > +     local test_dev_sz
>
> Nit: one empty line will make it easier to read.
>
> > +     test_dev_sz=$(blockdev --getsize64 "$TEST_DEV")
> > +
> > +     echo $((test_dev_sz / 1024 / 1024))
> > +
>
> Nit: an empty line not needed.
>
> > +}
> > +
>
> I suggest to improve this new function to _require_test_dev_size_mb(). It takes
> 1st argument as the minimum size size in MB, and if TEST_DEV size is smaller
> than that, it set SKIP_REASON and return 1. We can add device_requires() to
> nvme/035 to call _require_test_dev_size_mb(). This will skip the test case when
> the TEST_DEV is small, and do not report it as a failure.
>
> I also suggest to include nvme/035 change for the size check in this patch. I
> think one shot change for function addition and function call will be simpler
> for this charge.

Yeah, that looks better, I already send V2 to fix it, thanks.

>
> >  _require_test_dev_in_hotplug_slot() {
> >       local parent
> >       parent="$(_get_pci_parent_from_blkdev)"
> > --
> > 2.34.1
> >
>
> --
> Shin'ichiro Kawasaki
>


-- 
Best Regards,
  Yi Zhang

[PATCH blktests 3/3] common/xfs: update _xfs_run_fio_verify_io to accept the size parameter

[Yi Zhang]

This commit alo updated nvme/012 nvme/013 nvme/035 to pass the size
parameter to _xfs_run_fio_verify_io

Signed-off-by: Yi Zhang <yi.zhang@redhat.com>
---
 common/xfs     | 3 ++-
 tests/nvme/012 | 2 +-
 tests/nvme/013 | 2 +-
 tests/nvme/035 | 9 ++++++++-
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/common/xfs b/common/xfs
index 846a5ef..2c5d961 100644
--- a/common/xfs
+++ b/common/xfs
@@ -23,10 +23,11 @@ _xfs_mkfs_and_mount() {
 _xfs_run_fio_verify_io() {
 	local mount_dir="/mnt/blktests"
 	local bdev=$1
+	local sz=$2
 
 	_xfs_mkfs_and_mount "${bdev}" "${mount_dir}" >> "${FULL}" 2>&1
 
-	_run_fio_verify_io --size=950m --directory="${mount_dir}/"
+	_run_fio_verify_io --size="$sz" --directory="${mount_dir}/"
 
 	umount "${mount_dir}" >> "${FULL}" 2>&1
 	rm -fr "${mount_dir}"
diff --git a/tests/nvme/012 b/tests/nvme/012
index c9d2438..e60082c 100755
--- a/tests/nvme/012
+++ b/tests/nvme/012
@@ -44,7 +44,7 @@ test() {
 	cat "/sys/block/${nvmedev}n1/uuid"
 	cat "/sys/block/${nvmedev}n1/wwid"
 
-	_xfs_run_fio_verify_io "/dev/${nvmedev}n1"
+	_xfs_run_fio_verify_io "/dev/${nvmedev}n1" "900m"
 
 	_nvme_disconnect_subsys "${subsys_name}"
 
diff --git a/tests/nvme/013 b/tests/nvme/013
index 265b696..9d60a7d 100755
--- a/tests/nvme/013
+++ b/tests/nvme/013
@@ -41,7 +41,7 @@ test() {
 	cat "/sys/block/${nvmedev}n1/uuid"
 	cat "/sys/block/${nvmedev}n1/wwid"
 
-	_xfs_run_fio_verify_io "/dev/${nvmedev}n1"
+	_xfs_run_fio_verify_io "/dev/${nvmedev}n1" "900m"
 
 	_nvme_disconnect_subsys "${subsys_name}"
 
diff --git a/tests/nvme/035 b/tests/nvme/035
index ee78a75..31de0d1 100755
--- a/tests/nvme/035
+++ b/tests/nvme/035
@@ -21,14 +21,21 @@ test_device() {
 	local ctrldev
 	local nsdev
 	local port
+	local test_dev_sz
 
 	echo "Running ${TEST_NAME}"
 
 	_setup_nvmet
 	port=$(_nvmet_passthru_target_setup "${subsys}")
 	nsdev=$(_nvmet_passthru_target_connect "${nvme_trtype}" "${subsys}")
+	test_dev_sz=$(_get_test_dev_size_mb)
 
-	_xfs_run_fio_verify_io "${nsdev}"
+	if (( "$test_dev_sz" < 1024 )); then
+		echo "Test dev: $TEST_DEV should at leat 1024m"
+		return 1
+
+	fi
+	_xfs_run_fio_verify_io "${nsdev}" "900m"
 
 	_nvme_disconnect_subsys "${subsys}"
 	_nvmet_passthru_target_cleanup "${port}" "${subsys}"
-- 
2.34.1

Re: [PATCH blktests 3/3] common/xfs: update _xfs_run_fio_verify_io to accept the size parameter

[Shinichiro Kawasaki]

On Oct 24, 2022 / 14:13, Yi Zhang wrote:
> This commit alo updated nvme/012 nvme/013 nvme/035 to pass the size

Let's note why we do this. How about this descprtion?

  Change fio I/O size of nvme/012,013,035 from 950m to 900m, since recent
  change increased the xfs log size and it caused fio failure with I/O
  size 950m.

  Also add size parameter to _run_fio_verify_io. This allows to move the
  fio I/O size definition from common/xfs to the test case, so that device
  size and fio I/O size are both defined at single place.

I think the commit title needs to reflect the motivations above, like:

  nvme/012,013,035: change fio I/O size and move size definition place

> parameter to _xfs_run_fio_verify_io
> 
> Signed-off-by: Yi Zhang <yi.zhang@redhat.com>

How about to add "Link:" tag to refer our discussion?

  Link: https://lore.kernel.org/linux-block/20221019051244.810755-1-yi.zhang@redhat.com/


> ---
>  common/xfs     | 3 ++-
>  tests/nvme/012 | 2 +-
>  tests/nvme/013 | 2 +-
>  tests/nvme/035 | 9 ++++++++-
>  4 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/common/xfs b/common/xfs
> index 846a5ef..2c5d961 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -23,10 +23,11 @@ _xfs_mkfs_and_mount() {
>  _xfs_run_fio_verify_io() {
>  	local mount_dir="/mnt/blktests"
>  	local bdev=$1
> +	local sz=$2
>  
>  	_xfs_mkfs_and_mount "${bdev}" "${mount_dir}" >> "${FULL}" 2>&1
>  
> -	_run_fio_verify_io --size=950m --directory="${mount_dir}/"
> +	_run_fio_verify_io --size="$sz" --directory="${mount_dir}/"
>  
>  	umount "${mount_dir}" >> "${FULL}" 2>&1
>  	rm -fr "${mount_dir}"
> diff --git a/tests/nvme/012 b/tests/nvme/012
> index c9d2438..e60082c 100755
> --- a/tests/nvme/012
> +++ b/tests/nvme/012
> @@ -44,7 +44,7 @@ test() {
>  	cat "/sys/block/${nvmedev}n1/uuid"
>  	cat "/sys/block/${nvmedev}n1/wwid"
>  
> -	_xfs_run_fio_verify_io "/dev/${nvmedev}n1"
> +	_xfs_run_fio_verify_io "/dev/${nvmedev}n1" "900m"
>  
>  	_nvme_disconnect_subsys "${subsys_name}"
>  
> diff --git a/tests/nvme/013 b/tests/nvme/013
> index 265b696..9d60a7d 100755
> --- a/tests/nvme/013
> +++ b/tests/nvme/013
> @@ -41,7 +41,7 @@ test() {
>  	cat "/sys/block/${nvmedev}n1/uuid"
>  	cat "/sys/block/${nvmedev}n1/wwid"
>  
> -	_xfs_run_fio_verify_io "/dev/${nvmedev}n1"
> +	_xfs_run_fio_verify_io "/dev/${nvmedev}n1" "900m"
>  
>  	_nvme_disconnect_subsys "${subsys_name}"
>  
> diff --git a/tests/nvme/035 b/tests/nvme/035
> index ee78a75..31de0d1 100755
> --- a/tests/nvme/035
> +++ b/tests/nvme/035
> @@ -21,14 +21,21 @@ test_device() {
>  	local ctrldev
>  	local nsdev
>  	local port
> +	local test_dev_sz
>  
>  	echo "Running ${TEST_NAME}"
>  
>  	_setup_nvmet
>  	port=$(_nvmet_passthru_target_setup "${subsys}")
>  	nsdev=$(_nvmet_passthru_target_connect "${nvme_trtype}" "${subsys}")
> +	test_dev_sz=$(_get_test_dev_size_mb)
>  
> -	_xfs_run_fio_verify_io "${nsdev}"
> +	if (( "$test_dev_sz" < 1024 )); then
> +		echo "Test dev: $TEST_DEV should at leat 1024m"
> +		return 1
> +
> +	fi

As I commented on the second patch, I suggest to move this device size check
part to the second patch.

Other changes looks good tome.

> +	_xfs_run_fio_verify_io "${nsdev}" "900m"
>  
>  	_nvme_disconnect_subsys "${subsys}"
>  	_nvmet_passthru_target_cleanup "${port}" "${subsys}"
> -- 
> 2.34.1
> 

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 3/3] common/xfs: update _xfs_run_fio_verify_io to accept the size parameter

[Yi Zhang]

On Tue, Oct 25, 2022 at 10:49 AM Shinichiro Kawasaki
<shinichiro.kawasaki@wdc.com> wrote:
>
> On Oct 24, 2022 / 14:13, Yi Zhang wrote:
> > This commit alo updated nvme/012 nvme/013 nvme/035 to pass the size
>
> Let's note why we do this. How about this descprtion?
>
>   Change fio I/O size of nvme/012,013,035 from 950m to 900m, since recent
>   change increased the xfs log size and it caused fio failure with I/O
>   size 950m.
>
>   Also add size parameter to _run_fio_verify_io. This allows to move the
>   fio I/O size definition from common/xfs to the test case, so that device
>   size and fio I/O size are both defined at single place.
>
> I think the commit title needs to reflect the motivations above, like:

Agree, updated the title and description in V2.

>
>   nvme/012,013,035: change fio I/O size and move size definition place
>
> > parameter to _xfs_run_fio_verify_io
> >
> > Signed-off-by: Yi Zhang <yi.zhang@redhat.com>
>
> How about to add "Link:" tag to refer our discussion?
>
>   Link: https://lore.kernel.org/linux-block/20221019051244.810755-1-yi.zhang@redhat.com/

Added in V2.
Again, thanks Shinichiro for the review.


>
>
> > ---
> >  common/xfs     | 3 ++-
> >  tests/nvme/012 | 2 +-
> >  tests/nvme/013 | 2 +-
> >  tests/nvme/035 | 9 ++++++++-
> >  4 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/common/xfs b/common/xfs
> > index 846a5ef..2c5d961 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -23,10 +23,11 @@ _xfs_mkfs_and_mount() {
> >  _xfs_run_fio_verify_io() {
> >       local mount_dir="/mnt/blktests"
> >       local bdev=$1
> > +     local sz=$2
> >
> >       _xfs_mkfs_and_mount "${bdev}" "${mount_dir}" >> "${FULL}" 2>&1
> >
> > -     _run_fio_verify_io --size=950m --directory="${mount_dir}/"
> > +     _run_fio_verify_io --size="$sz" --directory="${mount_dir}/"
> >
> >       umount "${mount_dir}" >> "${FULL}" 2>&1
> >       rm -fr "${mount_dir}"
> > diff --git a/tests/nvme/012 b/tests/nvme/012
> > index c9d2438..e60082c 100755
> > --- a/tests/nvme/012
> > +++ b/tests/nvme/012
> > @@ -44,7 +44,7 @@ test() {
> >       cat "/sys/block/${nvmedev}n1/uuid"
> >       cat "/sys/block/${nvmedev}n1/wwid"
> >
> > -     _xfs_run_fio_verify_io "/dev/${nvmedev}n1"
> > +     _xfs_run_fio_verify_io "/dev/${nvmedev}n1" "900m"
> >
> >       _nvme_disconnect_subsys "${subsys_name}"
> >
> > diff --git a/tests/nvme/013 b/tests/nvme/013
> > index 265b696..9d60a7d 100755
> > --- a/tests/nvme/013
> > +++ b/tests/nvme/013
> > @@ -41,7 +41,7 @@ test() {
> >       cat "/sys/block/${nvmedev}n1/uuid"
> >       cat "/sys/block/${nvmedev}n1/wwid"
> >
> > -     _xfs_run_fio_verify_io "/dev/${nvmedev}n1"
> > +     _xfs_run_fio_verify_io "/dev/${nvmedev}n1" "900m"
> >
> >       _nvme_disconnect_subsys "${subsys_name}"
> >
> > diff --git a/tests/nvme/035 b/tests/nvme/035
> > index ee78a75..31de0d1 100755
> > --- a/tests/nvme/035
> > +++ b/tests/nvme/035
> > @@ -21,14 +21,21 @@ test_device() {
> >       local ctrldev
> >       local nsdev
> >       local port
> > +     local test_dev_sz
> >
> >       echo "Running ${TEST_NAME}"
> >
> >       _setup_nvmet
> >       port=$(_nvmet_passthru_target_setup "${subsys}")
> >       nsdev=$(_nvmet_passthru_target_connect "${nvme_trtype}" "${subsys}")
> > +     test_dev_sz=$(_get_test_dev_size_mb)
> >
> > -     _xfs_run_fio_verify_io "${nsdev}"
> > +     if (( "$test_dev_sz" < 1024 )); then
> > +             echo "Test dev: $TEST_DEV should at leat 1024m"
> > +             return 1
> > +
> > +     fi
>
> As I commented on the second patch, I suggest to move this device size check
> part to the second patch.

Done in V2.

>
> Other changes looks good tome.
>
> > +     _xfs_run_fio_verify_io "${nsdev}" "900m"
> >
> >       _nvme_disconnect_subsys "${subsys}"
> >       _nvmet_passthru_target_cleanup "${port}" "${subsys}"
> > --
> > 2.34.1
> >
>
> --
> Shin'ichiro Kawasaki
>


-- 
Best Regards,
  Yi Zhang