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

[Bart Van Assche]

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.

Thanks,

Bart.