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

["Darrick J. Wong" <djwong@kernel.org>]

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