nvme: configure discard at init time

Currently nvme reconfigures discard for every disk revalidation. This is problematic because any O_WRONLY or O_RDWR open will trigger a partition scan through udev/systemd, and we will reconfigure discard. This blows away any user settings, like discard_max_bytes.

Configure discard at init time instead.

Signed-off-by Jens Axboe <axboe at kernel.dk>

---

I'm open to other suggestions as well, currently it sucks that you'd have to continually re-configure the discard settings when someone opens the device for writing.

Quoted 7 lines · Jens Axboe

Currently nvme reconfigures discard for every disk revalidation. This is problematic because any O_WRONLY or O_RDWR open will trigger a partition scan through udev/systemd, and we will reconfigure discard. This blows away any user settings, like discard_max_bytes.

Configure discard at init time instead.

Quoted 7 lines

Signed-off-by Jens Axboe <axboe at kernel.dk>

---

I'm open to other suggestions as well, currently it sucks that you'd have to continually re-configure the discard settings when someone opens the device for writing.

Your suggestion is probably fine. The only problem I can think of is a _very_ unlikely scenario where a firmware update adds discard support, then a user would have to reload the module in order to expose the capability.

How about this?

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.

drivers/nvme/host/core.c +14 -8

-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);

Quoted 5 lines · Jens Axboe

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:

Quoted 45 lines

drivers/nvme/host/core.c +14 -8

-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);
 

drivers/nvme/host/core.c static void nvme_update_disk_info(struct gendisk *disk, · line 1436

Quoted 3 lines

drivers/nvme/host/core.c +1 -1

 	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