lkml.org 
[lkml]   [2022]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v3] scsi: support packing multi-segment in UNMAP command
From
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.

\
 
 \ /
  Last update: 2022-06-15 18:22    [W:0.422 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site