[PATCH] generic/108: use sysfs values for logical,physical block size in scsi_debug

[PATCH] generic/108: use sysfs values for logical,physical block size in scsi_debug

[Nitesh Shetty]

scsi_debug device used for test, is created with assumption of 512 bytes
logical and physical block size.
This causes error in lvcreate step, when SCRATCH_DEV device lba is not
512 bytes. This can be solved by reading block size from sysfs of device.
If sysfs is missing fallback to 512 bytes as default.

Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
---
 tests/generic/108 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/generic/108 b/tests/generic/108
index ad43269f..db0e9bd0 100755
--- a/tests/generic/108
+++ b/tests/generic/108
@@ -42,8 +42,11 @@ _require_non_zoned_device $SCRATCH_DEV
 lvname=lv_$seq
 vgname=vg_$seq
 
+physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
+logical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/logical_block_size)
+
 # _get_scsi_debug_dev returns a scsi debug device with 128M in size by default
-SCSI_DEBUG_DEV=`_get_scsi_debug_dev 512 512 0 300`
+SCSI_DEBUG_DEV=`_get_scsi_debug_dev ${physical:-512} ${logical:-512} 0 300`
 test -b "$SCSI_DEBUG_DEV" || _notrun "Failed to initialize scsi debug device"
 echo "SCSI debug device $SCSI_DEBUG_DEV" >>$seqres.full
 

base-commit: 2ea74ba4e70b546279896e2a733c8c7f4b206193
-- 
2.25.1

Re: [PATCH] generic/108: use sysfs values for logical,physical block size in scsi_debug

[Luis Chamberlain]

On Wed, Mar 02, 2022 at 02:59:47AM +0530, Nitesh Shetty wrote:
> scsi_debug device used for test, is created with assumption of 512 bytes
> logical and physical block size.
> This causes error in lvcreate step, when SCRATCH_DEV device lba is not
> 512 bytes. This can be solved by reading block size from sysfs of device.
> If sysfs is missing fallback to 512 bytes as default.
> 
> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH] generic/108: use sysfs values for logical,physical block size in scsi_debug

[Darrick J. Wong]

On Wed, Mar 02, 2022 at 02:59:47AM +0530, Nitesh Shetty wrote:
> scsi_debug device used for test, is created with assumption of 512 bytes
> logical and physical block size.
> This causes error in lvcreate step, when SCRATCH_DEV device lba is not
> 512 bytes. This can be solved by reading block size from sysfs of device.
> If sysfs is missing fallback to 512 bytes as default.
> 
> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> ---
>  tests/generic/108 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/generic/108 b/tests/generic/108
> index ad43269f..db0e9bd0 100755
> --- a/tests/generic/108
> +++ b/tests/generic/108
> @@ -42,8 +42,11 @@ _require_non_zoned_device $SCRATCH_DEV
>  lvname=lv_$seq
>  vgname=vg_$seq
>  
> +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> +logical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/logical_block_size)

This causes a regression if $SCRATCH_DEV is not a raw block device:

# SCRATCH_DEV=/dev/sda4 ./check generic/108
...
--- generic/108.out
+++ generic/108.out.bad
@@ -1,2 +1,4 @@
 QA output created by 108
+cat: /sys/block/sda4/queue/physical_block_size: No such file or directory
+cat: /sys/block/sda4/queue/logical_block_size: No such file or directory
 fsync: Input/output error

--D

> +
>  # _get_scsi_debug_dev returns a scsi debug device with 128M in size by default
> -SCSI_DEBUG_DEV=`_get_scsi_debug_dev 512 512 0 300`
> +SCSI_DEBUG_DEV=`_get_scsi_debug_dev ${physical:-512} ${logical:-512} 0 300`
>  test -b "$SCSI_DEBUG_DEV" || _notrun "Failed to initialize scsi debug device"
>  echo "SCSI debug device $SCSI_DEBUG_DEV" >>$seqres.full
>  
> 
> base-commit: 2ea74ba4e70b546279896e2a733c8c7f4b206193
> -- 
> 2.25.1
> 

Re: [PATCH] generic/108: use sysfs values for logical,physical block size in scsi_debug

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

 Mon, Mar 21, 2022 at 01:21:33PM -0700, Darrick J. Wong wrote:
> On Wed, Mar 02, 2022 at 02:59:47AM +0530, Nitesh Shetty wrote:
> > scsi_debug device used for test, is created with assumption of 512 bytes
> > logical and physical block size.
> > This causes error in lvcreate step, when SCRATCH_DEV device lba is not
> > 512 bytes. This can be solved by reading block size from sysfs of device.
> > If sysfs is missing fallback to 512 bytes as default.
> > 
> > Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> > ---
> >  tests/generic/108 | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/generic/108 b/tests/generic/108
> > index ad43269f..db0e9bd0 100755
> > --- a/tests/generic/108
> > +++ b/tests/generic/108
> > @@ -42,8 +42,11 @@ _require_non_zoned_device $SCRATCH_DEV
> >  lvname=lv_$seq
> >  vgname=vg_$seq
> >  
> > +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> > +logical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/logical_block_size)
> 
> This causes a regression if $SCRATCH_DEV is not a raw block device:
> 
Acked, I was testing for NVMe device, missed out sda sysfs.
Let me see, if I can use other sysfs/utility to get block size of device.

Also I see many 4k LBA tests failing mainly because of setup failure in
format command. This is not actually test failure, rather setup failure.
So what do you suggest for those type of tests?
Should I put them in not run status or just report to community so that
person with relevant expertise can add a fix?

--Nitesh Shetty


> # SCRATCH_DEV=/dev/sda4 ./check generic/108
> ...
> --- generic/108.out
> +++ generic/108.out.bad
> @@ -1,2 +1,4 @@
>  QA output created by 108
> +cat: /sys/block/sda4/queue/physical_block_size: No such file or directory
> +cat: /sys/block/sda4/queue/logical_block_size: No such file or directory
>  fsync: Input/output error
> 
> --D
> 
> > +
> >  # _get_scsi_debug_dev returns a scsi debug device with 128M in size by default
> > -SCSI_DEBUG_DEV=`_get_scsi_debug_dev 512 512 0 300`
> > +SCSI_DEBUG_DEV=`_get_scsi_debug_dev ${physical:-512} ${logical:-512} 0 300`
> >  test -b "$SCSI_DEBUG_DEV" || _notrun "Failed to initialize scsi debug device"
> >  echo "SCSI debug device $SCSI_DEBUG_DEV" >>$seqres.full
> >  
> > 
> > base-commit: 2ea74ba4e70b546279896e2a733c8c7f4b206193
> > -- 
> > 2.25.1
> > 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] generic/108: use sysfs values for logical,physical block size in scsi_debug

[Darrick J. Wong]

On Tue, Mar 22, 2022 at 01:56:29PM +0530, Nitesh Shetty wrote:
>  Mon, Mar 21, 2022 at 01:21:33PM -0700, Darrick J. Wong wrote:
> > On Wed, Mar 02, 2022 at 02:59:47AM +0530, Nitesh Shetty wrote:
> > > scsi_debug device used for test, is created with assumption of 512 bytes
> > > logical and physical block size.
> > > This causes error in lvcreate step, when SCRATCH_DEV device lba is not
> > > 512 bytes. This can be solved by reading block size from sysfs of device.
> > > If sysfs is missing fallback to 512 bytes as default.
> > > 
> > > Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> > > ---
> > >  tests/generic/108 | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/generic/108 b/tests/generic/108
> > > index ad43269f..db0e9bd0 100755
> > > --- a/tests/generic/108
> > > +++ b/tests/generic/108
> > > @@ -42,8 +42,11 @@ _require_non_zoned_device $SCRATCH_DEV
> > >  lvname=lv_$seq
> > >  vgname=vg_$seq
> > >  
> > > +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> > > +logical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/logical_block_size)
> > 
> > This causes a regression if $SCRATCH_DEV is not a raw block device:
> > 
> Acked, I was testing for NVMe device, missed out sda sysfs.
> Let me see, if I can use other sysfs/utility to get block size of device.
> 
> Also I see many 4k LBA tests failing mainly because of setup failure in
> format command. This is not actually test failure, rather setup failure.
> So what do you suggest for those type of tests?

Ideally, fix the ones that can be fixed, and _notrun the rest.

> Should I put them in not run status or just report to community so that
> person with relevant expertise can add a fix?

For this specific problem I suggest creating a function that finds the
/sys/block/XXX path for any given block device or partition, and then
update all the open coded logic to use it.  Something like:

# Map a block device to its counterpart in sysfs
_sysfs_block() {
	local dev="$1"
	local shortdev="$(_short_dev "$dev")"

	# Full block devices are simple
	local ret="/sys/block/$shortdev"
	if [ -e "$ret" ]; then
		readlink -m "$ret"
		return 0
	fi

	# Partitions are a little trickier
	ret="/sys/class/block/$shortdev"
	if [ -e "$ret/partition" ]; then
		readlink -m "$ret/.."
		return 0
	fi

	# ???
	return 1
}

sysfsb=$(_sysfs_block $SCRATCH_DEV)
physical=$(cat $sysfsb/queue/physical_block_size)

--D

> 
> --Nitesh Shetty
> 
> 
> > # SCRATCH_DEV=/dev/sda4 ./check generic/108
> > ...
> > --- generic/108.out
> > +++ generic/108.out.bad
> > @@ -1,2 +1,4 @@
> >  QA output created by 108
> > +cat: /sys/block/sda4/queue/physical_block_size: No such file or directory
> > +cat: /sys/block/sda4/queue/logical_block_size: No such file or directory
> >  fsync: Input/output error
> > 
> > --D
> > 
> > > +
> > >  # _get_scsi_debug_dev returns a scsi debug device with 128M in size by default
> > > -SCSI_DEBUG_DEV=`_get_scsi_debug_dev 512 512 0 300`
> > > +SCSI_DEBUG_DEV=`_get_scsi_debug_dev ${physical:-512} ${logical:-512} 0 300`
> > >  test -b "$SCSI_DEBUG_DEV" || _notrun "Failed to initialize scsi debug device"
> > >  echo "SCSI debug device $SCSI_DEBUG_DEV" >>$seqres.full
> > >  
> > > 
> > > base-commit: 2ea74ba4e70b546279896e2a733c8c7f4b206193
> > > -- 
> > > 2.25.1
> > > 
> > 

Re: [PATCH] generic/108: use sysfs values for logical,physical block size in scsi_debug

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 3086 bytes --]

On Tue, Mar 22, 2022 at 09:07:56AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 22, 2022 at 01:56:29PM +0530, Nitesh Shetty wrote:
> >  Mon, Mar 21, 2022 at 01:21:33PM -0700, Darrick J. Wong wrote:
> > > On Wed, Mar 02, 2022 at 02:59:47AM +0530, Nitesh Shetty wrote:
> > > > scsi_debug device used for test, is created with assumption of 512 bytes
> > > > logical and physical block size.
> > > > This causes error in lvcreate step, when SCRATCH_DEV device lba is not
> > > > 512 bytes. This can be solved by reading block size from sysfs of device.
> > > > If sysfs is missing fallback to 512 bytes as default.
> > > > 
> > > > Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> > > > ---
> > > >  tests/generic/108 | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tests/generic/108 b/tests/generic/108
> > > > index ad43269f..db0e9bd0 100755
> > > > --- a/tests/generic/108
> > > > +++ b/tests/generic/108
> > > > @@ -42,8 +42,11 @@ _require_non_zoned_device $SCRATCH_DEV
> > > >  lvname=lv_$seq
> > > >  vgname=vg_$seq
> > > >  
> > > > +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> > > > +logical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/logical_block_size)
> > > 
> > > This causes a regression if $SCRATCH_DEV is not a raw block device:
> > > 
> > Acked, I was testing for NVMe device, missed out sda sysfs.
> > Let me see, if I can use other sysfs/utility to get block size of device.
> > 
> > Also I see many 4k LBA tests failing mainly because of setup failure in
> > format command. This is not actually test failure, rather setup failure.
> > So what do you suggest for those type of tests?
> 
> Ideally, fix the ones that can be fixed, and _notrun the rest.
> 

Sure, will send a different patchset.

> > Should I put them in not run status or just report to community so that
> > person with relevant expertise can add a fix?
> 
> For this specific problem I suggest creating a function that finds the
> /sys/block/XXX path for any given block device or partition, and then
> update all the open coded logic to use it.  Something like:
> 
> # Map a block device to its counterpart in sysfs
> _sysfs_block() {
> 	local dev="$1"
> 	local shortdev="$(_short_dev "$dev")"
> 
> 	# Full block devices are simple
> 	local ret="/sys/block/$shortdev"
> 	if [ -e "$ret" ]; then
> 		readlink -m "$ret"
> 		return 0
> 	fi
> 
> 	# Partitions are a little trickier
> 	ret="/sys/class/block/$shortdev"
> 	if [ -e "$ret/partition" ]; then
> 		readlink -m "$ret/.."
> 		return 0
> 	fi
> 
> 	# ???
> 	return 1
> }
> 
> sysfsb=$(_sysfs_block $SCRATCH_DEV)
> physical=$(cat $sysfsb/queue/physical_block_size)
> 
> --D
>

As a alternate, using blockdev utility might reduce and simplify the
changes required. how about below changes ?

-physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
-logical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/logical_block_size)
+physical=`blockdev --getpbsz $SCRATCH_DEV`
+logical=`blockdev --getss $SCRATCH_DEV`

--Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] generic/108: use sysfs values for logical,physical block size in scsi_debug

[Darrick J. Wong]

On Wed, Mar 23, 2022 at 05:37:18PM +0530, Nitesh Shetty wrote:
> On Tue, Mar 22, 2022 at 09:07:56AM -0700, Darrick J. Wong wrote:
> > On Tue, Mar 22, 2022 at 01:56:29PM +0530, Nitesh Shetty wrote:
> > >  Mon, Mar 21, 2022 at 01:21:33PM -0700, Darrick J. Wong wrote:
> > > > On Wed, Mar 02, 2022 at 02:59:47AM +0530, Nitesh Shetty wrote:
> > > > > scsi_debug device used for test, is created with assumption of 512 bytes
> > > > > logical and physical block size.
> > > > > This causes error in lvcreate step, when SCRATCH_DEV device lba is not
> > > > > 512 bytes. This can be solved by reading block size from sysfs of device.
> > > > > If sysfs is missing fallback to 512 bytes as default.
> > > > > 
> > > > > Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> > > > > ---
> > > > >  tests/generic/108 | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tests/generic/108 b/tests/generic/108
> > > > > index ad43269f..db0e9bd0 100755
> > > > > --- a/tests/generic/108
> > > > > +++ b/tests/generic/108
> > > > > @@ -42,8 +42,11 @@ _require_non_zoned_device $SCRATCH_DEV
> > > > >  lvname=lv_$seq
> > > > >  vgname=vg_$seq
> > > > >  
> > > > > +physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> > > > > +logical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/logical_block_size)
> > > > 
> > > > This causes a regression if $SCRATCH_DEV is not a raw block device:
> > > > 
> > > Acked, I was testing for NVMe device, missed out sda sysfs.
> > > Let me see, if I can use other sysfs/utility to get block size of device.
> > > 
> > > Also I see many 4k LBA tests failing mainly because of setup failure in
> > > format command. This is not actually test failure, rather setup failure.
> > > So what do you suggest for those type of tests?
> > 
> > Ideally, fix the ones that can be fixed, and _notrun the rest.
> > 
> 
> Sure, will send a different patchset.
> 
> > > Should I put them in not run status or just report to community so that
> > > person with relevant expertise can add a fix?
> > 
> > For this specific problem I suggest creating a function that finds the
> > /sys/block/XXX path for any given block device or partition, and then
> > update all the open coded logic to use it.  Something like:
> > 
> > # Map a block device to its counterpart in sysfs
> > _sysfs_block() {
> > 	local dev="$1"
> > 	local shortdev="$(_short_dev "$dev")"
> > 
> > 	# Full block devices are simple
> > 	local ret="/sys/block/$shortdev"
> > 	if [ -e "$ret" ]; then
> > 		readlink -m "$ret"
> > 		return 0
> > 	fi
> > 
> > 	# Partitions are a little trickier
> > 	ret="/sys/class/block/$shortdev"
> > 	if [ -e "$ret/partition" ]; then
> > 		readlink -m "$ret/.."
> > 		return 0
> > 	fi
> > 
> > 	# ???
> > 	return 1
> > }
> > 
> > sysfsb=$(_sysfs_block $SCRATCH_DEV)
> > physical=$(cat $sysfsb/queue/physical_block_size)
> > 
> > --D
> >
> 
> As a alternate, using blockdev utility might reduce and simplify the
> changes required. how about below changes ?
> 
> -physical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/physical_block_size)
> -logical=$(cat /sys/block/$(_short_dev $SCRATCH_DEV)/queue/logical_block_size)
> +physical=`blockdev --getpbsz $SCRATCH_DEV`
> +logical=`blockdev --getss $SCRATCH_DEV`

Good idea! :)

--D

> --Nitesh Shetty