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