From mboxrd@z Thu Jan  1 00:00:00 1970
From: axboe@kernel.dk (Jens Axboe)
Date: Wed, 2 May 2018 11:05:17 -0600
Subject: [PATCH] nvme: configure discard at init time
In-Reply-To: <20180502165408.GJ5938@localhost.localdomain>
References: <b3e2875b-f58a-f49c-1ded-a8c362b026ca@kernel.dk>
 <20180502163258.GI5938@localhost.localdomain>
 <42987d9b-7e55-940c-79a3-c0454358d342@kernel.dk>
 <20180502165408.GJ5938@localhost.localdomain>
Message-ID: <fc28981a-0a09-b4c9-cb36-1b8e227a0f11@kernel.dk>

On 5/2/18 10:54 AM, Keith Busch wrote:
> On Wed, May 02, 2018@10:41:03AM -0600, Jens Axboe wrote:
>> If we add discard through a firmware upgrade, we should also handle
>> the case where we lose discard through a firmware upgrade/downgrade.
>> Not that any of them are likely to ever happen, but...
>>
>> How about this? Also handles the case where streams values are updated.
> 
> Good call. You also have it updating granularity when a format changes
> the logical block size, so that's also a good thing.
> 
> One minor issue below:
>  
>> -static void nvme_config_discard(struct nvme_ctrl *ctrl,
>> -		unsigned stream_alignment, struct request_queue *queue)
>> +static void nvme_config_discard(struct nvme_ns *ns)
>>  {
>> +	struct nvme_ctrl *ctrl = ns->ctrl;
>> +	struct request_queue *queue = ns->queue;
>>  	u32 size = queue_logical_block_size(queue);
>>  
>> -	if (stream_alignment)
>> -		size *= stream_alignment;
>> +	if (!(ctrl->oncs & NVME_CTRL_ONCS_DSM)) {
>> +		blk_queue_flag_clear(QUEUE_FLAG_DISCARD, queue);
>> +		return;
>> +	}
>> +
>> +	if (ctrl->nr_streams && ns->sws && ns->sgs)
>> +		size *= ns->sws * ns->sgs;
>>  
>>  	BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) <
>>  			NVME_DSM_MAX_RANGES);
>> @@ -1361,6 +1367,10 @@ static void nvme_config_discard(struct nvme_ctrl *ctrl,
>>  	queue->limits.discard_alignment = 0;
>>  	queue->limits.discard_granularity = size;
>>  
>> +	/* If discard is already enabled, don't reset queue limits */
>> +	if (blk_queue_flag_test_and_set(QUEUE_FLAG_DISCARD, queue))
>> +		return;
>> +
>>  	blk_queue_max_discard_sectors(queue, UINT_MAX);
>>  	blk_queue_max_discard_segments(queue, NVME_DSM_MAX_RANGES);
>>  	blk_queue_flag_set(QUEUE_FLAG_DISCARD, queue);
>> @@ -1407,10 +1417,6 @@ static void nvme_update_disk_info(struct gendisk *disk,
>>  {
>>  	sector_t capacity = le64_to_cpup(&id->nsze) << (ns->lba_shift - 9);
>>  	unsigned short bs = 1 << ns->lba_shift;
>> -	unsigned stream_alignment = 0;
>> -
>> -	if (ns->ctrl->nr_streams && ns->sws && ns->sgs)
>> -		stream_alignment = ns->sws * ns->sgs;
>>  
>>  	blk_mq_freeze_queue(disk->queue);
>>  	blk_integrity_unregister(disk);
>> @@ -1427,7 +1433,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
>>  	set_capacity(disk, capacity);
>>  
>>  	if (ns->ctrl->oncs & NVME_CTRL_ONCS_DSM)
>> -		nvme_config_discard(ns->ctrl, stream_alignment, disk->queue);
>> +		nvme_config_discard(ns);
> 
> Since nvme_config_discard now handles disabling the queue limit, we need
> to call this unconditionally regardless of the ONCS_DSM

Oh yeah, I forgot making that change. I'll send out a proper version.

-- 
Jens Axboe