[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 405895b1dff2..4948c0fb3d05 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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)) {
-- 
2.7.4

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Christoph Hellwig]

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

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Sagi Grimberg]

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

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.

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Christoph Hellwig]

>> 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.

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Sagi Grimberg]

>>> 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.

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

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Christoph Hellwig]

On Thu, Oct 19, 2017@01:28:42PM +0300, Sagi Grimberg wrote:
>>> 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.

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Sagi Grimberg]

>>>> 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.

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?

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Christoph Hellwig]

On Thu, Oct 19, 2017@05:44:57PM +0300, Sagi Grimberg wrote:
> 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.

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Sagi Grimberg]

> 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...

[PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure

[Christoph Hellwig]

On Thu, Oct 19, 2017@05:58:16PM +0300, Sagi Grimberg wrote:
>
>> 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 :)