Messages in this thread | | | Date | Thu, 16 Jun 2022 09:38:27 +0800 | Subject | Re: [PATCH v3] scsi: support packing multi-segment in UNMAP command | From | Chao Yu <> |
| |
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.
| |