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.
