Date:   Fri, 22 Jan 2021 16:08:51 +0900
From:   Changheun Lee <nanich.lee@...sung.com>
To:     damien.lemoal@....com
Cc:     arnd@...db.de, hch@....de, jejb@...ux.ibm.com,
        jisoo2146.oh@...sung.com, junho89.kim@...sung.com,
        linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org,
        martin.petersen@...cle.com, michael.christie@...cle.com,
        mj0123.lee@...sung.com, nanich.lee@...sung.com, oneukum@...e.com,
        seunghwan.hyun@...sung.com, sookwan7.kim@...sung.com,
        woosung2.lee@...sung.com, yt0928.kim@...sung.com
Subject: Re: [PATCH 1/1] scsi: sd: use max_xfer_blocks for set rw_max if
 max_xfer_blocks is available

> On 2021/01/20 15:45, Manjong Lee wrote:
> > Add recipients for more reviews.
> 
> Please resend instead of replying to your own patch. The reply quoting corrupts
> the patch.
> 
> The patch title is very long.
> 
> > 
> >> SCSI device has max_xfer_size and opt_xfer_size,
> >> but current kernel uses only opt_xfer_size.
> >>
> >> It causes the limitation on setting IO chunk size,
> >> although it can support larger one.
> >>
> >> So, I propose this patch to use max_xfer_size in case it has valid value.
> >> It can support to use the larger chunk IO on SCSI device.
> >>
> >> For example,
> >> This patch is effective in case of some SCSI device like UFS
> >> with opt_xfer_size 512KB, queue depth 32 and max_xfer_size over 512KB.
> >>
> >> I expect both the performance improvement
> >> and the efficiency use of smaller command queue depth.
> 
> This can be measured, and this commit message should include results to show how
> effective this change is.
> 
> >>
> >> Signed-off-by: Manjong Lee <mj0123.lee@...sung.com>
> >> ---
> >> drivers/scsi/sd.c | 56 +++++++++++++++++++++++++++++++++++++++++++----
> >> 1 file changed, 52 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> >> index 679c2c025047..de59f01c1304 100644
> >> --- a/drivers/scsi/sd.c
> >> +++ b/drivers/scsi/sd.c
> >> @@ -3108,6 +3108,53 @@ static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer)
> >> sdkp->security = 1;
> >> }
> >>
> >> +static bool sd_validate_max_xfer_size(struct scsi_disk *sdkp,
> >> +				      unsigned int dev_max)
> >> +{
> >> +	struct scsi_device *sdp = sdkp->device;
> >> +	unsigned int max_xfer_bytes =
> >> +		logical_to_bytes(sdp, sdkp->max_xfer_blocks);
> >> +
> >> +	if (sdkp->max_xfer_blocks == 0)
> >> +		return false;
> >> +
> >> +	if (sdkp->max_xfer_blocks > SD_MAX_XFER_BLOCKS) {
> >> +		sd_first_printk(KERN_WARNING, sdkp,
> >> +				"Maximal transfer size %u logical blocks " \
> >> +				"> sd driver limit (%u logical blocks)\n",
> >> +				sdkp->max_xfer_blocks, SD_DEF_XFER_BLOCKS);
> >> +		return false;
> >> +	}
> >> +
> >> +	if (sdkp->max_xfer_blocks > dev_max) {
> >> +		sd_first_printk(KERN_WARNING, sdkp,
> >> +				"Maximal transfer size %u logical blocks "
> >> +				"> dev_max (%u logical blocks)\n",
> >> +				sdkp->max_xfer_blocks, dev_max);
> >> +		return false;
> >> +	}
> >> +
> >> +	if (max_xfer_bytes < PAGE_SIZE) {
> >> +		sd_first_printk(KERN_WARNING, sdkp,
> >> +				"Maximal transfer size %u bytes < " \
> >> +				"PAGE_SIZE (%u bytes)\n",
> >> +				max_xfer_bytes, (unsigned int)PAGE_SIZE);
> >> +		return false;
> >> +	}
> >> +
> >> +	if (max_xfer_bytes & (sdkp->physical_block_size - 1)) {
> >> +		sd_first_printk(KERN_WARNING, sdkp,
> >> +				"Maximal transfer size %u bytes not a " \
> >> +				"multiple of physical block size (%u bytes)\n",
> >> +				max_xfer_bytes, sdkp->physical_block_size);
> >> +		return false;
> >> +	}
> >> +
> >> +	sd_first_printk(KERN_INFO, sdkp, "Maximal transfer size %u bytes\n",
> >> +			max_xfer_bytes);
> >> +	return true;
> >> +}
> 
> Except for the order of the comparisons against SD_MAX_XFER_BLOCKS and dev_max,
> this function looks identical to sd_validate_opt_xfer_size(), modulo the use of
> max_xfer_blocks instead of opt_xfer_blocks. Can't you turn this into something like:
> 
> static bool sd_validate_max_xfer_size(struct scsi_disk *sdkp,
> const char *name,
> unsigned int xfer_blocks,
> unsigned int dev_max)
> 
> To allow checking both opt_xfer_blocks and max_xfer_blocks ?
> 
> >> +
> >> /*
> >> * Determine the device's preferred I/O size for reads and writes
> >> * unless the reported value is unreasonably small, large, not a
> >> @@ -3233,12 +3280,13 @@ static int sd_revalidate_disk(struct gendisk *disk)
> >>
> >> /* Initial block count limit based on CDB TRANSFER LENGTH field size. */
> >> dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
> 
> This looks weird: no indentation. Care to resend ?
> 
> >> -
> >> -	/* Some devices report a maximum block count for READ/WRITE requests. */
> >> -	dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
> >> q->limits.max_dev_sectors = logical_to_sectors(sdp, dev_max);
> >>
> >> -	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
> >> +	if (sd_validate_max_xfer_size(sdkp, dev_max)) {
> >> +		q->limits.io_opt = 0;
> >> +		rw_max = logical_to_sectors(sdp, sdkp->max_xfer_blocks);
> >> +		q->limits.max_dev_sectors = rw_max;
> >> +	} else if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
> 
> This does not look correct to me. This renders the device reported
> opt_xfer_blocks useless.
> 
> The unmodified code sets dev_max to the min of SD_MAX_XFER_BLOCKS or
> SD_DEF_XFER_BLOCKS and of the device reported max_xfer_blocks. The result of
> this is used as the device max_dev_sectors queue limit, which in turn is used to
> set the max_hw_sectors queue limit accounting for the adapter limits too.
> 
> opt_xfer_blocks, if it is valid, will be used to set the io_opt queue limit,
> which is a hint. This hint is used to optimize the "soft" max_sectors command
> limit used by the block layer to limit command size if the value of
> opt_xfer_blocks is smaller than the limit initially set with max_xfer_blocks.
> 
> So if for your device max_sectors end up being too small, it is likely because
> the device itself is reporting an opt_xfer_blocks value that is too small for
> its own good. The max_sectors limit can be manually increased with "echo xxx >
> /sys/block/sdX/queue/max_sectors_kb". A udev rule can be used to handle this
> autmatically if needed.
> 
> But to get a saner default for that device, I do not think that this patch is
> the right solution. Ideally, the device peculiarity should be handled with a
> quirk, but that is not used in scsi. So beside the udev rule trick, I am not
> sure what the right approach is here.
> 

This approach is for using sdkp->max_xfer_blocks as a rw_max.
There are no way to use it now when sdkp->opt_xfer_blocks is valid.
In my case, scsi device reports both of sdkp->max_xfer_blocks, and
sdkp->opt_xfer_blocks.

How about set larger valid value between sdkp->max_xfer_blocks,
and sdkp->opt_xfer_blocks to rw_max?

> >> q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
> >> rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> >> } else {
> >> -- 
> >> 2.29.0
> >>
> >>
> >

Powered by blists - more mailing lists