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@xxxxxxxxxxx>
>> ---
>> 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.
>> 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
>>
>>
>
--
Damien Le Moal
Western Digital Research
