mbox series

[RFC,0/2] scsi: scsi_debug: Allow parameter changes through sysfs

Message ID 20240311065427.3006023-1-shinichiro.kawasaki@wdc.com
Headers show
Series scsi: scsi_debug: Allow parameter changes through sysfs | expand

Message

Shinichiro Kawasaki March 11, 2024, 6:54 a.m. UTC
Some of the blktests users run tests using kernels with built-in modules to run
tests easier and quicker. For that reason, it is desirable to run tests with the
built-in scsi_debug [1]. However, some of the scsi_debug parameters can be
modified only at module load, as module parameters. They are visible as sysfs
attribute files, but they are read-only and not writable. There are ten,
read-only scsi_debug parameters used in blktests [2]. Because of these
parameters, eight test cases are skipped when scsi_debug is built-in [3].

The values of those parameters are kept as global variables and referred to in
many places. Changes in the parameter values affect live scsi_debug hosts and
cause various troubles. That is why the parameters are read-only. I once tried
to make the parameters non-global and unique to each host, but found this
approach will need large code changes in scsi_debug.

To minimize the code changes for such parameter changes, I propose to allow the
parameter changes "only when scsi_debug has no hosts." Users need to remove all
hosts before changing the parameters. With this restriction, users can run tests
with any, desired parameter values using the built-in scsi_debug.

This series shows how the code will change with this approach. I chose
dev_size_mb as the example parameter, which is referred to in multiple blktests
test cases. The first patch in the series is preparation. The second patch makes
dev_size_mb writable through sysfs only when there is no host. This change will
allow the test case block/032 to run with the built-in scsi_debug. Corresponding
blktests side changes are available on github [4].

The work for the other listed parameters is in progress. While the work is on
going, I would like to ask for comments on this approach. Is this approach
acceptable? Is there any other better way?

[1] https://lore.kernel.org/linux-block/20220530130811.3006554-1-hch@lst.de/

[2] Parameters used in blktests which have read-only sysfs attribute files

  dev_size_mb
  sector_size
  lbpws
  lbpws10
  dix
  dif
  zbc
  zone_cap_mb
  zone_size_mb
  zone_nr_conv

[3] Test cases skipped with built-in scsi_debug

  block/009
  block/025
  block/028
  block/032
  loop/004
  zbd/008
  zbd/009
  zbd/010

[4] https://github.com/kawasaki/blktests/tree/scsi_debug

Shin'ichiro Kawasaki (2):
  scsi: scsi_debug: Factor out initialization of size parameters
  scsi: scsi_debug: Allow dev_size_mb changes through sysfs

 drivers/scsi/scsi_debug.c | 91 ++++++++++++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 21 deletions(-)

Comments

Bart Van Assche March 11, 2024, 5:22 p.m. UTC | #1
On 3/10/24 23:54, Shin'ichiro Kawasaki wrote:
> As the preparation for the dev_size_mb parameter changes through sysfs,
> factor out the initialization of parameters affected by the dev_size_mb
> changes.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>   drivers/scsi/scsi_debug.c | 52 ++++++++++++++++++++++++---------------
>   1 file changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index d03d66f11493..c6b32f07a82c 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -7234,10 +7234,39 @@ ATTRIBUTE_GROUPS(sdebug_drv);
>   
>   static struct device *pseudo_primary;
>   
> +/*
> + * Calculate size related parameters from sdebug_dev_zize_mb and
> + * sdebug_sector_size.
> + */
> +static void scsi_debug_init_size_parameters(void)
> +{
> +	unsigned long sz;
> +
> +	sz = (unsigned long)sdebug_dev_size_mb * 1048576;
> +	sdebug_store_sectors = sz / sdebug_sector_size;
> +	sdebug_capacity = get_sdebug_capacity();
> +
> +	/* play around with geometry, don't waste too much on track 0 */
> +	sdebug_heads = 8;
> +	sdebug_sectors_per = 32;
> +	if (sdebug_dev_size_mb >= 256)
> +		sdebug_heads = 64;
> +	else if (sdebug_dev_size_mb >= 16)
> +		sdebug_heads = 32;
> +	sdebug_cylinders_per = (unsigned long)sdebug_capacity /
> +		(sdebug_sectors_per * sdebug_heads);
> +	if (sdebug_cylinders_per >= 1024) {
> +		/* other LLDs do this; implies >= 1GB ram disk ... */
> +		sdebug_heads = 255;
> +		sdebug_sectors_per = 63;
> +		sdebug_cylinders_per = (unsigned long)sdebug_capacity /
> +			(sdebug_sectors_per * sdebug_heads);
> +	}
> +}
> +
>   static int __init scsi_debug_init(void)
>   {
>   	bool want_store = (sdebug_fake_rw == 0);
> -	unsigned long sz;
>   	int k, ret, hosts_to_add;
>   	int idx = -1;
>   
> @@ -7369,26 +7398,9 @@ static int __init scsi_debug_init(void)
>   		sdebug_dev_size_mb = DEF_DEV_SIZE_MB;
>   	if (sdebug_dev_size_mb < 1)
>   		sdebug_dev_size_mb = 1;  /* force minimum 1 MB ramdisk */
> -	sz = (unsigned long)sdebug_dev_size_mb * 1048576;
> -	sdebug_store_sectors = sz / sdebug_sector_size;
> -	sdebug_capacity = get_sdebug_capacity();
>   
> -	/* play around with geometry, don't waste too much on track 0 */
> -	sdebug_heads = 8;
> -	sdebug_sectors_per = 32;
> -	if (sdebug_dev_size_mb >= 256)
> -		sdebug_heads = 64;
> -	else if (sdebug_dev_size_mb >= 16)
> -		sdebug_heads = 32;
> -	sdebug_cylinders_per = (unsigned long)sdebug_capacity /
> -			       (sdebug_sectors_per * sdebug_heads);
> -	if (sdebug_cylinders_per >= 1024) {
> -		/* other LLDs do this; implies >= 1GB ram disk ... */
> -		sdebug_heads = 255;
> -		sdebug_sectors_per = 63;
> -		sdebug_cylinders_per = (unsigned long)sdebug_capacity /
> -			       (sdebug_sectors_per * sdebug_heads);
> -	}
> +	scsi_debug_init_size_parameters();
> +
>   	if (scsi_debug_lbp()) {
>   		sdebug_unmap_max_blocks =
>   			clamp(sdebug_unmap_max_blocks, 0U, 0xffffffffU);

Please remove sdebug_heads, sdebug_cylinders_per and sdebug_sectors_per
instead of making this change. While these values are reported in a
MODE SENSE response, I don't think that it is valuable to keep support
for heads, cylinders and sectors in the scsi_debug driver.

Thanks,

Bart.
Shinichiro Kawasaki March 20, 2024, 1:46 a.m. UTC | #2
On Mar 11, 2024 / 10:22, Bart Van Assche wrote:
> On 3/10/24 23:54, Shin'ichiro Kawasaki wrote:
> > As the preparation for the dev_size_mb parameter changes through sysfs,
> > factor out the initialization of parameters affected by the dev_size_mb
> > changes.
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >   drivers/scsi/scsi_debug.c | 52 ++++++++++++++++++++++++---------------
> >   1 file changed, 32 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > index d03d66f11493..c6b32f07a82c 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> > @@ -7234,10 +7234,39 @@ ATTRIBUTE_GROUPS(sdebug_drv);
> >   static struct device *pseudo_primary;
> > +/*
> > + * Calculate size related parameters from sdebug_dev_zize_mb and
> > + * sdebug_sector_size.
> > + */
> > +static void scsi_debug_init_size_parameters(void)
> > +{
> > +	unsigned long sz;
> > +
> > +	sz = (unsigned long)sdebug_dev_size_mb * 1048576;
> > +	sdebug_store_sectors = sz / sdebug_sector_size;
> > +	sdebug_capacity = get_sdebug_capacity();
> > +
> > +	/* play around with geometry, don't waste too much on track 0 */
> > +	sdebug_heads = 8;
> > +	sdebug_sectors_per = 32;
> > +	if (sdebug_dev_size_mb >= 256)
> > +		sdebug_heads = 64;
> > +	else if (sdebug_dev_size_mb >= 16)
> > +		sdebug_heads = 32;
> > +	sdebug_cylinders_per = (unsigned long)sdebug_capacity /
> > +		(sdebug_sectors_per * sdebug_heads);
> > +	if (sdebug_cylinders_per >= 1024) {
> > +		/* other LLDs do this; implies >= 1GB ram disk ... */
> > +		sdebug_heads = 255;
> > +		sdebug_sectors_per = 63;
> > +		sdebug_cylinders_per = (unsigned long)sdebug_capacity /
> > +			(sdebug_sectors_per * sdebug_heads);
> > +	}
> > +}
> > +
> >   static int __init scsi_debug_init(void)
> >   {
> >   	bool want_store = (sdebug_fake_rw == 0);
> > -	unsigned long sz;
> >   	int k, ret, hosts_to_add;
> >   	int idx = -1;
> > @@ -7369,26 +7398,9 @@ static int __init scsi_debug_init(void)
> >   		sdebug_dev_size_mb = DEF_DEV_SIZE_MB;
> >   	if (sdebug_dev_size_mb < 1)
> >   		sdebug_dev_size_mb = 1;  /* force minimum 1 MB ramdisk */
> > -	sz = (unsigned long)sdebug_dev_size_mb * 1048576;
> > -	sdebug_store_sectors = sz / sdebug_sector_size;
> > -	sdebug_capacity = get_sdebug_capacity();
> > -	/* play around with geometry, don't waste too much on track 0 */
> > -	sdebug_heads = 8;
> > -	sdebug_sectors_per = 32;
> > -	if (sdebug_dev_size_mb >= 256)
> > -		sdebug_heads = 64;
> > -	else if (sdebug_dev_size_mb >= 16)
> > -		sdebug_heads = 32;
> > -	sdebug_cylinders_per = (unsigned long)sdebug_capacity /
> > -			       (sdebug_sectors_per * sdebug_heads);
> > -	if (sdebug_cylinders_per >= 1024) {
> > -		/* other LLDs do this; implies >= 1GB ram disk ... */
> > -		sdebug_heads = 255;
> > -		sdebug_sectors_per = 63;
> > -		sdebug_cylinders_per = (unsigned long)sdebug_capacity /
> > -			       (sdebug_sectors_per * sdebug_heads);
> > -	}
> > +	scsi_debug_init_size_parameters();
> > +
> >   	if (scsi_debug_lbp()) {
> >   		sdebug_unmap_max_blocks =
> >   			clamp(sdebug_unmap_max_blocks, 0U, 0xffffffffU);
> 
> Please remove sdebug_heads, sdebug_cylinders_per and sdebug_sectors_per
> instead of making this change. While these values are reported in a
> MODE SENSE response, I don't think that it is valuable to keep support
> for heads, cylinders and sectors in the scsi_debug driver.

I see. I guess we can return just zero as sdebug_sectors_per in the MODE
SENSE response instead.

I noticed that the three variables you suggest to remove are used in
sdebug_build_parts() also. It is not a good idea to remove the function
and drop or modify the partition table generation feature, probably. I
think we can make the three variables non-global, local variables in the
function. What do you think?

> 
> Thanks,
> 
> Bart.
Bart Van Assche March 20, 2024, 3:14 p.m. UTC | #3
On 3/19/24 18:46, Shinichiro Kawasaki wrote:
> On Mar 11, 2024 / 10:22, Bart Van Assche wrote:
>> Please remove sdebug_heads, sdebug_cylinders_per and sdebug_sectors_per
>> instead of making this change. While these values are reported in a
>> MODE SENSE response, I don't think that it is valuable to keep support
>> for heads, cylinders and sectors in the scsi_debug driver.
> 
> I see. I guess we can return just zero as sdebug_sectors_per in the MODE
> SENSE response instead.
> 
> I noticed that the three variables you suggest to remove are used in
> sdebug_build_parts() also. It is not a good idea to remove the function
> and drop or modify the partition table generation feature, probably. I
> think we can make the three variables non-global, local variables in the
> function. What do you think?

I propose to rework sdebug_build_parts() such that it aligns partitions
on logical block boundaries instead of cylinder boundaries. That will
make sdebug_build_parts() independent of sdebug_heads,
sdebug_cylinders_per and sdebug_sectors and hence will allow these three
variables to be removed.

Thanks,

Bart.