* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
@ 2017-10-19 6:58 Sagi Grimberg
2017-10-19 8:49 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Sagi Grimberg @ 2017-10-19 6:58 UTC (permalink / raw)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 6:58 [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure Sagi Grimberg
@ 2017-10-19 8:49 ` Christoph Hellwig
2017-10-19 9:41 ` Sagi Grimberg
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2017-10-19 8:49 UTC (permalink / raw)
> 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;
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 8:49 ` Christoph Hellwig
@ 2017-10-19 9:41 ` Sagi Grimberg
2017-10-19 10:20 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Sagi Grimberg @ 2017-10-19 9:41 UTC (permalink / raw)
>> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 9:41 ` Sagi Grimberg
@ 2017-10-19 10:20 ` Christoph Hellwig
2017-10-19 10:28 ` Sagi Grimberg
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2017-10-19 10:20 UTC (permalink / raw)
>> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 10:20 ` Christoph Hellwig
@ 2017-10-19 10:28 ` Sagi Grimberg
2017-10-19 13:55 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Sagi Grimberg @ 2017-10-19 10:28 UTC (permalink / raw)
>>> 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).
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 10:28 ` Sagi Grimberg
@ 2017-10-19 13:55 ` Christoph Hellwig
2017-10-19 14:44 ` Sagi Grimberg
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2017-10-19 13:55 UTC (permalink / raw)
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 13:55 ` Christoph Hellwig
@ 2017-10-19 14:44 ` Sagi Grimberg
2017-10-19 14:49 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Sagi Grimberg @ 2017-10-19 14:44 UTC (permalink / raw)
>>>> 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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 14:44 ` Sagi Grimberg
@ 2017-10-19 14:49 ` Christoph Hellwig
2017-10-19 14:58 ` Sagi Grimberg
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2017-10-19 14:49 UTC (permalink / raw)
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 14:49 ` Christoph Hellwig
@ 2017-10-19 14:58 ` Sagi Grimberg
2017-10-19 15:04 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Sagi Grimberg @ 2017-10-19 14:58 UTC (permalink / raw)
> 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...
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure
2017-10-19 14:58 ` Sagi Grimberg
@ 2017-10-19 15:04 ` Christoph Hellwig
0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2017-10-19 15:04 UTC (permalink / raw)
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 :)
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-10-19 15:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-19 6:58 [PATCH 4.14-rc] nvme-rdma: Fix error status return in tagset allocation failure Sagi Grimberg
2017-10-19 8:49 ` Christoph Hellwig
2017-10-19 9:41 ` Sagi Grimberg
2017-10-19 10:20 ` Christoph Hellwig
2017-10-19 10:28 ` Sagi Grimberg
2017-10-19 13:55 ` Christoph Hellwig
2017-10-19 14:44 ` Sagi Grimberg
2017-10-19 14:49 ` Christoph Hellwig
2017-10-19 14:58 ` Sagi Grimberg
2017-10-19 15:04 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).