Re: [PATCH v3] scsi: support packing multi-segment in UNMAP command

[Chao Yu <chao@kernel.org>]

On 2022/6/16 0:21, Bart Van Assche wrote:
> On 6/15/22 08:34, Chao Yu wrote:
>> As SCSI SBC4 specification section 5.30.2 describes that it can
>> support unmapping one or more LBA range in single UNMAP command,
>> however, previously we only pack one LBA range in UNMAP command
>> by default no matter device gives the block limits that says it
>> can support in-batch UNMAP.
> 
> The above sentence is too long. Please split it.
> 
>> This patch tries to set max_discard_segments config according to
>               ^^^^^^^^^^^^
> Consider changing "tries to set" into "sets".
> 
>> block limits of device, and supports in-batch UNMAP.
> 
> Consider changing "in-batch UNMAP" into "unmapping multiple LBA ranges with a single UNMAP command".
> 
>> +    blk_queue_max_discard_segments(q, sdkp->max_block_desc_count);
> 
> sdkp->max_block_desc_count is 32 bits wide while blk_queue_max_discard_segments() accepts an unsigned short as second argument. So the value 0x10002 will be converted into 2, which is not correct. Consider changing the second argument into min(U16_MAX, sdkp->max_block_desc_count).
> 
>>       sdkp->provisioning_mode = mode;
>>       switch (mode) {
>> @@ -836,9 +837,10 @@ static blk_status_t sd_setup_unmap_cmnd(struct scsi_cmnd *cmd)
>>       struct scsi_device *sdp = cmd->device;
>>       struct request *rq = scsi_cmd_to_rq(cmd);
>>       struct scsi_disk *sdkp = scsi_disk(rq->q->disk);
>> -    u64 lba = sectors_to_logical(sdp, blk_rq_pos(rq));
>> -    u32 nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq));
>> -    unsigned int data_len = 24;
>> +    unsigned short segments = blk_rq_nr_discard_segments(rq);
>> +    unsigned int data_len = 8 + 16 * segments;
>> +    unsigned int data_offset = 8;
> 
> Please rename 'data_offset' into 'descriptor_offset' to match the SBC-4 terminology.
> 
>> @@ -2870,9 +2879,9 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
>>               goto out;
>>           lba_count = get_unaligned_be32(&vpd->data[20]);
>> -        desc_count = get_unaligned_be32(&vpd->data[24]);
>> +        sdkp->max_block_desc_count = get_unaligned_be32(&vpd->data[24]);
> 
> Consider adding /* Extract the MAXIMUM UNMAP BLOCK DESCRIPTOR COUNT. */ above the above statement.
> 
>> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
>> index 5eea762f84d1..bda9db5e2322 100644
>> --- a/drivers/scsi/sd.h
>> +++ b/drivers/scsi/sd.h
>> @@ -119,6 +119,7 @@ struct scsi_disk {
>>       u32        opt_xfer_blocks;
>>       u32        max_ws_blocks;
>>       u32        max_unmap_blocks;
>> +    u32        max_block_desc_count;
> 
> I do not agree with the choice of the name of this member variable. The name used in SBC-4 is "MAXIMUM UNMAP BLOCK DESCRIPTOR COUNT". Leaving out "unmap" when abbreviating that description into a member name makes it impossible to guess what the purpose of that member variable is.

Agreed, thanks for the review. I've updated the patch as you suggested.

Thanks,

> 
> Thanks,
> 
> Bart.