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

[Nitesh Shetty <nj.shetty@samsung.com>]

[-- 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 --]