Re: [PATCH 6/7] sd: support multi-range TRIM for ATA disks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux