| Message ID | 20230310123604.1820231-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v5] scsi: support packing multi-segment in UNMAP command | expand |
> - /* If command type is WRITE or DISCARD, set bitmap as dirty */ > - if (ufshpb_is_write_or_discard(cmd)) { > + /* If command type is WRITE, set bitmap as dirty */ > + if (op_is_write(req_op(scsi_cmd_to_rq(cmd)))) { Umm, a driver has absolutely no business poking into the UNMAP payload. Someone needs to fix the UFS driver first to not do this.
On 2023/3/10 22:02, Christoph Hellwig wrote: >> - /* If command type is WRITE or DISCARD, set bitmap as dirty */ >> - if (ufshpb_is_write_or_discard(cmd)) { >> + /* If command type is WRITE, set bitmap as dirty */ >> + if (op_is_write(req_op(scsi_cmd_to_rq(cmd)))) { > > Umm, a driver has absolutely no business poking into the UNMAP > payload. Someone needs to fix the UFS driver first to not do this. IIUC，originally, HPB driver tries to lookup LBA range{,s} from WRITE/DISCARD request, and will dirty mapped HPB regions based on LBA range{,s}, do you mean HPB driver should not parse DISCARD request?
On Mon, Mar 13, 2023 at 09:44:47AM +0800, Chao Yu wrote: > IIUC，originally, HPB driver tries to lookup LBA range{,s} from WRITE/DISCARD > request, and will dirty mapped HPB regions based on LBA range{,s}, do you mean > HPB driver should not parse DISCARD request? Eww.. This just means we really need to drop this bogus HPB code. It will otherwise break every single time when a new SCSI command get added that modifies LBA content.
SCSI maintainers, Any comments on this patch? To Christoph, should I just drop HPB part change? On 2023/3/10 20:36, 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 unmapping multiple LBA ranges with a single UNMAP > command. > > This patch sets max_discard_segments config according to block > limits of device, and supports unmapping multiple LBA ranges with > a single UNMAP command. > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > v5: > - fix to assign cmd->cmnd[7] and cmd->cmnd[8] w/ data_len correctly. > - fix to let ufshpb recognize and handle discard request which contains > multiple ranges correctly. > drivers/scsi/sd.c | 36 +++++++++++++++++++++++++----------- > drivers/scsi/sd.h | 1 + > drivers/ufs/core/ufshpb.c | 21 +++++++++++++++++++-- > 3 files changed, 45 insertions(+), 13 deletions(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 4bb87043e6db..1908b31c7342 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -792,6 +792,8 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode) > q->limits.discard_granularity = > max(sdkp->physical_block_size, > sdkp->unmap_granularity * logical_block_size); > + blk_queue_max_discard_segments(q, min_t(u32, U16_MAX, > + sdkp->max_unmap_block_desc_count)); > sdkp->provisioning_mode = mode; > > switch (mode) { > @@ -851,9 +853,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 descriptor_offset = 8; > + struct bio *bio; > char *buf; > > buf = sd_set_special_bvec(rq, data_len); > @@ -862,12 +865,20 @@ static blk_status_t sd_setup_unmap_cmnd(struct scsi_cmnd *cmd) > > cmd->cmd_len = 10; > cmd->cmnd[0] = UNMAP; > - cmd->cmnd[8] = 24; > + cmd->cmnd[7] = data_len >> 8; > + cmd->cmnd[8] = data_len & 0xff; > + > + put_unaligned_be16(6 + 16 * segments, &buf[0]); > + put_unaligned_be16(16 * segments, &buf[2]); > > - put_unaligned_be16(6 + 16, &buf[0]); > - put_unaligned_be16(16, &buf[2]); > - put_unaligned_be64(lba, &buf[8]); > - put_unaligned_be32(nr_blocks, &buf[16]); > + __rq_for_each_bio(bio, rq) { > + u64 lba = sectors_to_logical(sdp, bio->bi_iter.bi_sector); > + u32 nr_blocks = sectors_to_logical(sdp, bio_sectors(bio)); > + > + put_unaligned_be64(lba, &buf[descriptor_offset]); > + put_unaligned_be32(nr_blocks, &buf[descriptor_offset + 8]); > + descriptor_offset += 16; > + } > > cmd->allowed = sdkp->max_retries; > cmd->transfersize = data_len; > @@ -2917,7 +2928,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp) > sdkp->opt_xfer_blocks = get_unaligned_be32(&vpd->data[12]); > > if (vpd->len >= 64) { > - unsigned int lba_count, desc_count; > + unsigned int lba_count; > > sdkp->max_ws_blocks = (u32)get_unaligned_be64(&vpd->data[36]); > > @@ -2925,9 +2936,12 @@ 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]); > > - if (lba_count && desc_count) > + /* Extract the MAXIMUM UNMAP BLOCK DESCRIPTOR COUNT. */ > + sdkp->max_unmap_block_desc_count = > + get_unaligned_be32(&vpd->data[24]); > + > + if (lba_count && sdkp->max_unmap_block_desc_count) > sdkp->max_unmap_blocks = lba_count; > > sdkp->unmap_granularity = get_unaligned_be32(&vpd->data[28]); > diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h > index 5eea762f84d1..e7c51d23395b 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_unmap_block_desc_count; > u32 unmap_granularity; > u32 unmap_alignment; > u32 index; > diff --git a/drivers/ufs/core/ufshpb.c b/drivers/ufs/core/ufshpb.c > index a46a7666c891..327b29cf506f 100644 > --- a/drivers/ufs/core/ufshpb.c > +++ b/drivers/ufs/core/ufshpb.c > @@ -383,13 +383,30 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) > rgn = hpb->rgn_tbl + rgn_idx; > srgn = rgn->srgn_tbl + srgn_idx; > > - /* If command type is WRITE or DISCARD, set bitmap as dirty */ > - if (ufshpb_is_write_or_discard(cmd)) { > + /* If command type is WRITE, set bitmap as dirty */ > + if (op_is_write(req_op(scsi_cmd_to_rq(cmd)))) { > ufshpb_iterate_rgn(hpb, rgn_idx, srgn_idx, srgn_offset, > transfer_len, true); > return 0; > } > > + /* If command type is DISCARD, set bitmap as dirty */ > + if (op_is_discard(req_op(scsi_cmd_to_rq(cmd)))) { > + struct bio *bio; > + > + __rq_for_each_bio(bio, scsi_cmd_to_rq(cmd)) { > + lpn = sectors_to_logical(cmd->device, > + bio->bi_iter.bi_sector); > + transfer_len = sectors_to_logical(cmd->device, > + bio_sectors(bio)); > + ufshpb_get_pos_from_lpn(hpb, lpn, &rgn_idx, > + &srgn_idx, &srgn_offset); > + ufshpb_iterate_rgn(hpb, rgn_idx, srgn_idx, > + srgn_offset, transfer_len, true); > + } > + return 0; > + } > + > if (!ufshpb_is_supported_chunk(hpb, transfer_len)) > return 0; >
Chao, > SCSI maintainers, > > Any comments on this patch? I have an updated version of your original multi-segment UNMAP support queued in a discard topic branch that I intend to submit shortly. Without the UFS HPB bits.
On 2023/5/23 5:51, Martin K. Petersen wrote: > > Chao, > >> SCSI maintainers, >> >> Any comments on this patch? > > I have an updated version of your original multi-segment UNMAP support > queued in a discard topic branch that I intend to submit shortly. > Without the UFS HPB bits. > Martin, Any progress on this patch? https://git.kernel.org/pub/scm/linux/kernel/git/mkp/linux.git/commit/?h=5.20/discovery&id=834e3cef205d324c66bbc7edd85541be59f1f7b6
Chao,
> Any progress on this patch?
I'll resubmit this series for 6.6.
On 2023/6/29 10:02, Martin K. Petersen wrote: > > Chao, > >> Any progress on this patch? > > I'll resubmit this series for 6.6. Martin, Thank you! Thanks, >
On 2023/6/29 10:02, Martin K. Petersen wrote:
> I'll resubmit this series for 6.6.
Hi, Martin
Ping to avoid missing this patch in 6.6. :)
Thanks,
Ping, On 2023/6/29 10:02, Martin K. Petersen wrote: > > Chao, > >> Any progress on this patch? > > I'll resubmit this series for 6.6. >
Chao, >>> Any progress on this patch? >> I'll resubmit this series for 6.6. Been working on this series to address the reported regressions. Spent quite a bit of time on it last week.
On 2023/8/8 22:04, Martin K. Petersen wrote: > Been working on this series to address the reported regressions. Spent > quite a bit of time on it last week. Martin, thanks for the help. :) Could you please share the report? Maybe I can join to check it. Thanks,
On 2023/8/8 22:12, Chao Yu wrote: > On 2023/8/8 22:04, Martin K. Petersen wrote: >> Been working on this series to address the reported regressions. Spent >> quite a bit of time on it last week. > > Martin, thanks for the help. :) > > Could you please share the report? Maybe I can join to check it. Oh, I may misunderstand it, the reported regression wasn't caused by this patch, right? Thanks, > > Thanks,
On 2023/8/8 22:12, Chao Yu wrote: > On 2023/8/8 22:04, Martin K. Petersen wrote: >> Been working on this series to address the reported regressions. Spent >> quite a bit of time on it last week. > > Martin, thanks for the help. :) > > Could you please share the report? Maybe I can join to check it. It looks we will miss another merge window again... :( Not sure I understand it correctly... If this patch caused regression, could you please share the report w/ me? Thanks, > > Thanks,
Hi Martin, Is it possible to commit this patch only to catch up 6.7-rc1, w/o other changes in 5.20/discovery? I don't feel there is any strong correlation between them, or am I missing something? Thanks, On 2023/8/8 22:04, Martin K. Petersen wrote: > > Chao, > >>>> Any progress on this patch? >>> I'll resubmit this series for 6.6. > > Been working on this series to address the reported regressions. Spent > quite a bit of time on it last week. >
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 4bb87043e6db..1908b31c7342 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -792,6 +792,8 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode) q->limits.discard_granularity = max(sdkp->physical_block_size, sdkp->unmap_granularity * logical_block_size); + blk_queue_max_discard_segments(q, min_t(u32, U16_MAX, + sdkp->max_unmap_block_desc_count)); sdkp->provisioning_mode = mode; switch (mode) { @@ -851,9 +853,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 descriptor_offset = 8; + struct bio *bio; char *buf; buf = sd_set_special_bvec(rq, data_len); @@ -862,12 +865,20 @@ static blk_status_t sd_setup_unmap_cmnd(struct scsi_cmnd *cmd) cmd->cmd_len = 10; cmd->cmnd[0] = UNMAP; - cmd->cmnd[8] = 24; + cmd->cmnd[7] = data_len >> 8; + cmd->cmnd[8] = data_len & 0xff; + + put_unaligned_be16(6 + 16 * segments, &buf[0]); + put_unaligned_be16(16 * segments, &buf[2]); - put_unaligned_be16(6 + 16, &buf[0]); - put_unaligned_be16(16, &buf[2]); - put_unaligned_be64(lba, &buf[8]); - put_unaligned_be32(nr_blocks, &buf[16]); + __rq_for_each_bio(bio, rq) { + u64 lba = sectors_to_logical(sdp, bio->bi_iter.bi_sector); + u32 nr_blocks = sectors_to_logical(sdp, bio_sectors(bio)); + + put_unaligned_be64(lba, &buf[descriptor_offset]); + put_unaligned_be32(nr_blocks, &buf[descriptor_offset + 8]); + descriptor_offset += 16; + } cmd->allowed = sdkp->max_retries; cmd->transfersize = data_len; @@ -2917,7 +2928,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp) sdkp->opt_xfer_blocks = get_unaligned_be32(&vpd->data[12]); if (vpd->len >= 64) { - unsigned int lba_count, desc_count; + unsigned int lba_count; sdkp->max_ws_blocks = (u32)get_unaligned_be64(&vpd->data[36]); @@ -2925,9 +2936,12 @@ 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]); - if (lba_count && desc_count) + /* Extract the MAXIMUM UNMAP BLOCK DESCRIPTOR COUNT. */ + sdkp->max_unmap_block_desc_count = + get_unaligned_be32(&vpd->data[24]); + + if (lba_count && sdkp->max_unmap_block_desc_count) sdkp->max_unmap_blocks = lba_count; sdkp->unmap_granularity = get_unaligned_be32(&vpd->data[28]); diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h index 5eea762f84d1..e7c51d23395b 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_unmap_block_desc_count; u32 unmap_granularity; u32 unmap_alignment; u32 index; diff --git a/drivers/ufs/core/ufshpb.c b/drivers/ufs/core/ufshpb.c index a46a7666c891..327b29cf506f 100644 --- a/drivers/ufs/core/ufshpb.c +++ b/drivers/ufs/core/ufshpb.c @@ -383,13 +383,30 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) rgn = hpb->rgn_tbl + rgn_idx; srgn = rgn->srgn_tbl + srgn_idx; - /* If command type is WRITE or DISCARD, set bitmap as dirty */ - if (ufshpb_is_write_or_discard(cmd)) { + /* If command type is WRITE, set bitmap as dirty */ + if (op_is_write(req_op(scsi_cmd_to_rq(cmd)))) { ufshpb_iterate_rgn(hpb, rgn_idx, srgn_idx, srgn_offset, transfer_len, true); return 0; } + /* If command type is DISCARD, set bitmap as dirty */ + if (op_is_discard(req_op(scsi_cmd_to_rq(cmd)))) { + struct bio *bio; + + __rq_for_each_bio(bio, scsi_cmd_to_rq(cmd)) { + lpn = sectors_to_logical(cmd->device, + bio->bi_iter.bi_sector); + transfer_len = sectors_to_logical(cmd->device, + bio_sectors(bio)); + ufshpb_get_pos_from_lpn(hpb, lpn, &rgn_idx, + &srgn_idx, &srgn_offset); + ufshpb_iterate_rgn(hpb, rgn_idx, srgn_idx, + srgn_offset, transfer_len, true); + } + return 0; + } + if (!ufshpb_is_supported_chunk(hpb, transfer_len)) return 0;
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 unmapping multiple LBA ranges with a single UNMAP command. This patch sets max_discard_segments config according to block limits of device, and supports unmapping multiple LBA ranges with a single UNMAP command. Signed-off-by: Chao Yu <chao@kernel.org> --- v5: - fix to assign cmd->cmnd[7] and cmd->cmnd[8] w/ data_len correctly. - fix to let ufshpb recognize and handle discard request which contains multiple ranges correctly. drivers/scsi/sd.c | 36 +++++++++++++++++++++++++----------- drivers/scsi/sd.h | 1 + drivers/ufs/core/ufshpb.c | 21 +++++++++++++++++++-- 3 files changed, 45 insertions(+), 13 deletions(-)