nvme-rdma: Fix error status return in tagset allocation failure

PATCH New Sagi Grimberg 2 participants 10 messages Oct 19, 2017 NVMe

lore

Share & tools

lore

Sagi Grimberg

author maintainer

2017-10-19 06:58 UTC #0 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/host/rdma.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/nvme/host/rdma.c +6 -2

index 405895b1dff2..4948c0fb3d05 100644
@@ -765,8 +765,10 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl,
 
 	if (new) {
 		ctrl->ctrl.admin_tagset = nvme_rdma_alloc_tagset(&ctrl->ctrl, true);
-		if (IS_ERR(ctrl->ctrl.admin_tagset))
+		if (IS_ERR(ctrl->ctrl.admin_tagset)) {
+			error = PTR_ERR(ctrl->ctrl.admin_tagset);
 			goto out_free_queue;
+		}
 
 		ctrl->ctrl.admin_q = blk_mq_init_queue(&ctrl->admin_tag_set);
 		if (IS_ERR(ctrl->ctrl.admin_q)) {

@@ -845,8 +847,10 @@ static int nvme_rdma_configure_io_queues(struct nvme_rdma_ctrl *ctrl, bool new)
 
 	if (new) {
 		ctrl->ctrl.tagset = nvme_rdma_alloc_tagset(&ctrl->ctrl, false);
-		if (IS_ERR(ctrl->ctrl.tagset))
+		if (IS_ERR(ctrl->ctrl.tagset)) {
+			ret = PTR_ERR(ctrl->ctrl.tagset);
 			goto out_free_io_queues;
+		}
 
 		ctrl->ctrl.connect_q = blk_mq_init_queue(&ctrl->tag_set);
 		if (IS_ERR(ctrl->ctrl.connect_q)) {

Signature · 1 line

2.7.4

Christoph Hellwig

maintainer

2017-10-19 08:49 UTC #1 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

drivers/nvme/host/rdma.c static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl, · line 771

Quoted 7 lines

drivers/nvme/host/rdma.c +3 -1

 	if (new) {
 		ctrl->ctrl.admin_tagset = nvme_rdma_alloc_tagset(&ctrl->ctrl, true);
-		if (IS_ERR(ctrl->ctrl.admin_tagset))
+		if (IS_ERR(ctrl->ctrl.admin_tagset)) {
+			error = PTR_ERR(ctrl->ctrl.admin_tagset);
 			goto out_free_queue;
+		}

Can we return an error code instead and just pass in the tag set to be allocated instead of the admin flag?A E.g.:

	error = nvme_rdma_alloc_tagset(&ctrl->ctrl, &ctrl->ctrl.admin_tagset);
	if (error)
		goto out_free_queue;

Sagi Grimberg

author maintainer

2017-10-19 09:41 UTC #2 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Quoted 12 lines

drivers/nvme/host/rdma.c static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl, · line 771
Quoted 8 lines
  	if (new) {
  		ctrl->ctrl.admin_tagset = nvme_rdma_alloc_tagset(&ctrl->ctrl, true);
drivers/nvme/host/rdma.c +3 -1
-		if (IS_ERR(ctrl->ctrl.admin_tagset))
+		if (IS_ERR(ctrl->ctrl.admin_tagset)) {
+			error = PTR_ERR(ctrl->ctrl.admin_tagset);
  			goto out_free_queue;
+		}
Can we return an error code instead and just pass in the tag set to be allocated instead of the admin flag?A E.g.:

Quoted 3 lines

	error = nvme_rdma_alloc_tagset(&ctrl->ctrl, &ctrl->ctrl.admin_tagset);
	if (error)
		goto out_free_queue;

We could, but that would mean that the nvme_ctrl tagset pointer will need to be updated later (or from the routine itself).

I'm intending this code for nvme-core (and this would be ->alloc_tagset callout) so ultimately I'd like to move the tagset embedding in struct nvme_ctrl but given that its a hot-path reference, I suggest we hold it back until we make struct nvme_ctrl arranged better for the hot path dereferencing.

Christoph Hellwig

maintainer

2017-10-19 10:20 UTC #3 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Quoted 9 lines

Quoted 6 lines
Can we return an error code instead and just pass in the tag set to be allocated instead of the admin flag?A E.g.:
	error = nvme_rdma_alloc_tagset(&ctrl->ctrl, &ctrl->ctrl.admin_tagset);
	if (error)
		goto out_free_queue;
We could, but that would mean that the nvme_ctrl tagset pointer will need to be updated later (or from the routine itself).

That why I want to pass the address of the tagset pointers, so that it can be updated.

Sagi Grimberg

author maintainer

2017-10-19 10:28 UTC #4 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
to be allocated instead of the admin flag?A E.g.:
	error = nvme_rdma_alloc_tagset(&ctrl->ctrl, &ctrl->ctrl.admin_tagset);
	if (error)
		goto out_free_queue;

Quoted 5 lines

Quoted 2 lines
We could, but that would mean that the nvme_ctrl tagset pointer will need to be updated later (or from the routine itself).
That why I want to pass the address of the tagset pointers, so that it can be updated.

But I'm targeting this code to nvme-core which at this point, only has a pointer to the tagsets (which are embedded in the transport ctrl structs).

Christoph Hellwig

maintainer

2017-10-19 13:55 UTC #5 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Quoted 9 lines · Sagi Grimberg

Quoted 5 lines
Quoted 2 lines
We could, but that would mean that the nvme_ctrl tagset pointer will need to be updated later (or from the routine itself).
That why I want to pass the address of the tagset pointers, so that it can be updated.
But I'm targeting this code to nvme-core which at this point, only has a pointer to the tagsets (which are embedded in the transport ctrl structs).

But with a a double indirection you _can_ update the pointers.

Sagi Grimberg

author maintainer

2017-10-19 14:44 UTC #6 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
We could, but that would mean that the nvme_ctrl tagset pointer will need to be updated later (or from the routine itself).

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
That why I want to pass the address of the tagset pointers, so that it can be updated.
But I'm targeting this code to nvme-core which at this point, only has a pointer to the tagsets (which are embedded in the transport ctrl structs).
But with a a double indirection you _can_ update the pointers.

Oh, double indirection... I confused the ctrl structures in your suggestion...

Sure I can do that.

Is that still fine for for 4.14-rc?

Christoph Hellwig

maintainer

2017-10-19 14:49 UTC #7 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Quoted 2 lines · Sagi Grimberg

Oh, double indirection... I confused the ctrl structures in your suggestion...

Actually, let's skip the idea - I just noticed nvme_rdma_alloc_tagset does way too many different things for the admin vs io taget.

I'm fine with your original version, but once we refactor things in this area I have a few ideas how to improve things.

Sagi Grimberg

author maintainer

2017-10-19 14:58 UTC #8 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Quoted 6 lines

Actually, let's skip the idea - I just noticed nvme_rdma_alloc_tagset does way too many different things for the admin vs io taget.
I'm fine with your original version, but once we refactor things in this area I have a few ideas how to improve things.

I have no doubt you do...

Christoph Hellwig

maintainer

2017-10-19 15:04 UTC #9 lore raw

PATCH 1/1 nvme-rdma: Fix error status return in tagset allocation failure

Quoted 8 lines · Sagi Grimberg

Quoted 6 lines
Actually, let's skip the idea - I just noticed nvme_rdma_alloc_tagset does way too many different things for the admin vs io taget.
I'm fine with your original version, but once we refactor things in this area I have a few ideas how to improve things.
I have no doubt you do...

In the meantime can you write a more detailed changelog? For a late rc fix I'd really like to have a better description especially with Linus hating us block and nvme people at the moment :)