| Message ID | 20170320204319.12628-7-hch@lst.de |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2017-03-20 at 16:43 -0400, Christoph Hellwig wrote: > @@ -698,13 +698,19 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode) > break; > > case SD_LBP_ATA_TRIM: > - max_blocks = 65535 * (512 / sizeof(__le64)); > + max_ranges = 512 / sizeof(__le64); > + max_range_size = USHRT_MAX; > + max_blocks = max_ranges * max_range_size; > if (sdkp->device->ata_trim_zeroes_data) > q->limits.discard_zeroes_data = 1; > break; > } > > blk_queue_max_discard_sectors(q, max_blocks * (logical_block_size >> 9)); > + if (max_ranges) > + blk_queue_max_discard_segments(q, max_ranges); > + if (max_range_size) > + blk_queue_max_discard_segment_size(q, max_range_size); > queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q); > } Hello Christoph, Does blk_queue_max_discard_segment_size() expect a second argument that is a number of bytes? Is max_range_size a number of logical blocks that each have a size 1 << sector_shift? > @@ -826,14 +832,21 @@ static int sd_setup_ata_trim_cmnd(struct scsi_cmnd *cmd) > cmd->cmnd[8] = data_len; > > buf = page_address(rq->special_vec.bv_page); > - for (i = 0; i < (data_len >> 3); i++) { > - u64 n = min(nr_sectors, 0xffffu); > + __rq_for_each_bio(bio, rq) { > + u64 sector = bio->bi_iter.bi_sector >> (sector_shift - 9); > + u32 nr_sectors = bio->bi_iter.bi_size >> sector_shift; > > - buf[i] = cpu_to_le64(sector | (n << 48)); > - if (nr_sectors <= 0xffff) > - break; > - sector += 0xffff; > - nr_sectors -= 0xffff; > + do { > + u64 n = min(nr_sectors, 0xffffu); > + > + buf[i] = cpu_to_le64(sector | (n << 48)); > + if (nr_sectors <= 0xffff) > + break; > + sector += 0xffff; > + nr_sectors -= 0xffff; > + i++; > + > + } while (!WARN_ON_ONCE(i >= data_len >> 3)); > } > > cmd->allowed = SD_MAX_RETRIES; It's unfortunate that the loop-end test occurs twice (i < data_len >> 3). Please consider using put_unaligned_le64() instead of cpu_to_le64() such that the data type of buf can be changed from __le64* into void *. With that change and by introducing e.g. the following: void *end; [ ... ] end = (void *)buf + data_len; the loop variable 'i' can be eliminated. If buf[i++] = ... would be changed into *buf++ = ... then that would allow to change the two occurrences of (i < data_len >> 3) into (buf < end). Thanks, Bart.-- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index cc8684818305..c18fe9ff1f8f 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -643,7 +643,7 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode) { struct request_queue *q = sdkp->disk->queue; unsigned int logical_block_size = sdkp->device->sector_size; - unsigned int max_blocks = 0; + unsigned int max_blocks = 0, max_ranges = 0, max_range_size = 0; q->limits.discard_zeroes_data = 0; @@ -698,13 +698,19 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode) break; case SD_LBP_ATA_TRIM: - max_blocks = 65535 * (512 / sizeof(__le64)); + max_ranges = 512 / sizeof(__le64); + max_range_size = USHRT_MAX; + max_blocks = max_ranges * max_range_size; if (sdkp->device->ata_trim_zeroes_data) q->limits.discard_zeroes_data = 1; break; } blk_queue_max_discard_sectors(q, max_blocks * (logical_block_size >> 9)); + if (max_ranges) + blk_queue_max_discard_segments(q, max_ranges); + if (max_range_size) + blk_queue_max_discard_segment_size(q, max_range_size); queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q); } @@ -805,9 +811,9 @@ static int sd_setup_ata_trim_cmnd(struct scsi_cmnd *cmd) { struct scsi_device *sdp = cmd->device; struct request *rq = cmd->request; - u64 sector = blk_rq_pos(rq) >> (ilog2(sdp->sector_size) - 9); - u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9); - u32 data_len = sdp->sector_size, i; + u32 sector_shift = ilog2(sdp->sector_size); + u32 data_len = sdp->sector_size, i = 0; + struct bio *bio; __le64 *buf; rq->special_vec.bv_page = alloc_page(GFP_ATOMIC | __GFP_ZERO); @@ -826,14 +832,21 @@ static int sd_setup_ata_trim_cmnd(struct scsi_cmnd *cmd) cmd->cmnd[8] = data_len; buf = page_address(rq->special_vec.bv_page); - for (i = 0; i < (data_len >> 3); i++) { - u64 n = min(nr_sectors, 0xffffu); + __rq_for_each_bio(bio, rq) { + u64 sector = bio->bi_iter.bi_sector >> (sector_shift - 9); + u32 nr_sectors = bio->bi_iter.bi_size >> sector_shift; - buf[i] = cpu_to_le64(sector | (n << 48)); - if (nr_sectors <= 0xffff) - break; - sector += 0xffff; - nr_sectors -= 0xffff; + do { + u64 n = min(nr_sectors, 0xffffu); + + buf[i] = cpu_to_le64(sector | (n << 48)); + if (nr_sectors <= 0xffff) + break; + sector += 0xffff; + nr_sectors -= 0xffff; + i++; + + } while (!WARN_ON_ONCE(i >= data_len >> 3)); } cmd->allowed = SD_MAX_RETRIES;
Almost the same scheme as the older multi-range support for NVMe. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/scsi/sd.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)