Messages in this thread Patch in this message | | | Date | Wed, 15 Jun 2022 09:21:27 -0700 | Subject | Re: [PATCH v3] scsi: support packing multi-segment in UNMAP command | From | 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.
| |