Skip to contentSkip to conversation

few small nvmet fixes/cleanups

PATCH New Sagi Grimberg 2 participants 8 messages Nov 8, 2017 – Nov 9, 2017

Subsystems (2)
NVMe NVMe target
lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

cover few small nvmet fixes/cleanups

Just some useful patches for 4.15

Sagi Grimberg (3):
nvmet-rdma: removed queues cleanup from module exit
nvmet: remove redundant memset if failed to get_smart_log failed
nvmet: remove redundant local variable

 drivers/nvme/target/admin-cmd.c | 17 +++++------------
 drivers/nvme/target/rdma.c      | 17 -----------------
 2 files changed, 5 insertions(+), 29 deletions(-)
Signature · 1 line
2.14.1
author maintainer
#1 lore raw

PATCH 1/3 nvmet-rdma: removed queues cleanup from module exit

We already do that when we are notified in device removal which is triggered when unregistering as an ib client.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/rdma.c | 17 -----------------
 1 file changed, 17 deletions(-)
drivers/nvme/target/rdma.c +0 -17
index 98d7f2ded511..efcdc4991359 100644
@@ -1537,24 +1537,7 @@ static int __init nvmet_rdma_init(void)
 
 static void __exit nvmet_rdma_exit(void)
 {
-	struct nvmet_rdma_queue *queue;
-
 	nvmet_unregister_transport(&nvmet_rdma_ops);
-
-	flush_scheduled_work();
-
-	mutex_lock(&nvmet_rdma_queue_mutex);
-	while ((queue = list_first_entry_or_null(&nvmet_rdma_queue_list,
-			struct nvmet_rdma_queue, queue_list))) {
-		list_del_init(&queue->queue_list);
-
-		mutex_unlock(&nvmet_rdma_queue_mutex);
-		__nvmet_rdma_queue_disconnect(queue);
-		mutex_lock(&nvmet_rdma_queue_mutex);
-	}
-	mutex_unlock(&nvmet_rdma_queue_mutex);
-
-	flush_scheduled_work();
 	ib_unregister_client(&nvmet_rdma_ib_client);
 	ida_destroy(&nvmet_rdma_queue_ida);
 }
Signature · 1 line
2.14.1
maintainer
#2 lore raw

PATCH 1/3 nvmet-rdma: removed queues cleanup from module exit

Quoted 24 lines · Sagi Grimberg

We already do that when we are notified in device removal which is triggered when unregistering as an ib client.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/rdma.c | 17 -----------------
 1 file changed, 17 deletions(-)
drivers/nvme/target/rdma.c +0 -7
index 98d7f2ded511..efcdc4991359 100644
@@ -1537,24 +1537,7 @@ static int __init nvmet_rdma_init(void)
 
 static void __exit nvmet_rdma_exit(void)
 {
-	struct nvmet_rdma_queue *queue;
-
 	nvmet_unregister_transport(&nvmet_rdma_ops);
-
-	flush_scheduled_work();
-
-	mutex_lock(&nvmet_rdma_queue_mutex);
-	while ((queue = list_first_entry_or_null(&nvmet_rdma_queue_list,

drivers/nvme/target/rdma.c static int __init nvmet_rdma_init(void) · line 1557

Quoted 10 lines
drivers/nvme/target/rdma.c +0 -10
-			struct nvmet_rdma_queue, queue_list))) {
-		list_del_init(&queue->queue_list);
-
-		mutex_unlock(&nvmet_rdma_queue_mutex);
-		__nvmet_rdma_queue_disconnect(queue);
-		mutex_lock(&nvmet_rdma_queue_mutex);
-	}
-	mutex_unlock(&nvmet_rdma_queue_mutex);
-
-	flush_scheduled_work();

Should we thrown in an assert here that the queue really is empty?

Otherwise this looks good to me:

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#3 lore raw

PATCH 2/3 nvmet: remove redundant memset if failed to get_smart_log failed

In reply to Sagi Grimberg

We already allocated the buffer with kzalloc.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/admin-cmd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/nvme/target/admin-cmd.c +1 -3
index 9b02a6bd58d6..871c2e5e17a0 100644
@@ -144,10 +144,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		}
 		smart_log = buf;
 		status = nvmet_get_smart_log(req, smart_log);
-		if (status) {
-			memset(buf, '\0', data_len);
+		if (status)
 			goto err;
-		}
 		break;
 	case NVME_LOG_FW_SLOT:
 		/*
Signature · 1 line
2.14.1
maintainer
#4 lore raw

PATCH 2/3 nvmet: remove redundant memset if failed to get_smart_log failed

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#6 lore raw

PATCH 3/3 nvmet: remove redundant local variable

In reply to Sagi Grimberg

the status is either success or some status id and we don't need a local variable for it.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/admin-cmd.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)
drivers/nvme/target/admin-cmd.c +4 -9
index 871c2e5e17a0..6b921c253618 100644
@@ -35,17 +35,14 @@ u32 nvmet_get_log_page_len(struct nvme_command *cmd)
 static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		struct nvme_smart_log *slog)
 {
-	u16 status;
 	struct nvmet_ns *ns;
 	u64 host_reads, host_writes, data_units_read, data_units_written;
 
-	status = NVME_SC_SUCCESS;
 	ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->get_log_page.nsid);
 	if (!ns) {
-		status = NVME_SC_INVALID_NS;
 		pr_err("nvmet : Could not find namespace id : %d\n",
 				le32_to_cpu(req->cmd->get_log_page.nsid));
-		goto out;
+		return NVME_SC_INVALID_NS;
 	}
 
 	host_reads = part_stat_read(ns->bdev->bd_part, ios[READ]);

@@ -58,20 +55,18 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 	put_unaligned_le64(host_writes, &slog->host_writes[0]);
 	put_unaligned_le64(data_units_written, &slog->data_units_written[0]);
 	nvmet_put_namespace(ns);
-out:
-	return status;
+
+	return NVME_SC_SUCCESS;
 }
 
 static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 		struct nvme_smart_log *slog)
 {
-	u16 status;
 	u64 host_reads = 0, host_writes = 0;
 	u64 data_units_read = 0, data_units_written = 0;
 	struct nvmet_ns *ns;
 	struct nvmet_ctrl *ctrl;
 
-	status = NVME_SC_SUCCESS;
 	ctrl = req->sq->ctrl;
 
 	rcu_read_lock();

@@ -91,7 +86,7 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 	put_unaligned_le64(host_writes, &slog->host_writes[0]);
 	put_unaligned_le64(data_units_written, &slog->data_units_written[0]);
 
-	return status;
+	return NVME_SC_SUCCESS;
 }
 
 static u16 nvmet_get_smart_log(struct nvmet_req *req,
Signature · 1 line
2.14.1
maintainer
#7 lore raw

PATCH 3/3 nvmet: remove redundant local variable

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>