[PATCH 1/2] nvme-core: Use nvme_wq to queue async events and fw activation

[PATCH 1/2] nvme-core: Use nvme_wq to queue async events and fw activation

[Sagi Grimberg]

async_event_work might race as it is executed from two different
workqueues at the moment.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 5589f67d2cd8..bb2aad078637 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2676,7 +2676,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 	case NVME_SC_ABORT_REQ:
 		++ctrl->event_limit;
 		if (ctrl->state == NVME_CTRL_LIVE)
-			schedule_work(&ctrl->async_event_work);
+			queue_work(nvme_wq, &ctrl->async_event_work);
 		break;
 	default:
 		break;
@@ -2691,7 +2691,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		nvme_queue_scan(ctrl);
 		break;
 	case NVME_AER_NOTICE_FW_ACT_STARTING:
-		schedule_work(&ctrl->fw_act_work);
+		queue_work(nvme_wq, &ctrl->fw_act_work);
 		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
-- 
2.7.4

[PATCH 2/2] nvme-rdma: give up reconnect if state change fails

[Sagi Grimberg]

If we failed to transition to state LIVE after a successful
reconnect, then controller deletion already started. In this
case there is no point moving forward with reconnect.

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

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 58983000964b..4460ec3a2c0f 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -942,7 +942,11 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 	}
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
-	WARN_ON_ONCE(!changed);
+	if (!changed) {
+		/* We are probably in DELETING state */
+		return;
+	}
+
 	ctrl->ctrl.nr_reconnects = 0;
 
 	nvme_start_ctrl(&ctrl->ctrl);
-- 
2.7.4

[PATCH 2/2] nvme-rdma: give up reconnect if state change fails

[Johannes Thumshirn]

On Wed, Sep 20, 2017@02:06:13PM +0300, Sagi Grimberg wrote:
>  	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
> -	WARN_ON_ONCE(!changed);
> +	if (!changed) {
> +		/* We are probably in DELETING state */

Shouldn't we check if we're really in DELETING instead of just speculating we
are?

Byte,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 1/2] nvme-core: Use nvme_wq to queue async events and fw activation

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 2/2] nvme-rdma: give up reconnect if state change fails

[Sagi Grimberg]

>>   	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>> -	WARN_ON_ONCE(!changed);
>> +	if (!changed) {
>> +		/* We are probably in DELETING state */
> 
> Shouldn't we check if we're really in DELETING instead of just speculating we
> are?

I can add a WARN_ONCE.

[PATCH 2/2] nvme-rdma: give up reconnect if state change fails

[Johannes Thumshirn]

On Wed, Sep 20, 2017@02:29:18PM +0300, Sagi Grimberg wrote:
> 
> > >   	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
> > > -	WARN_ON_ONCE(!changed);
> > > +	if (!changed) {
> > > +		/* We are probably in DELETING state */
> > 
> > Shouldn't we check if we're really in DELETING instead of just speculating we
> > are?
> 
> I can add a WARN_ONCE.

Works for me.

With that change,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 1/2] nvme-core: Use nvme_wq to queue async events and fw activation

[Christoph Hellwig]

Looks fine,

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

[PATCH 2/2] nvme-rdma: give up reconnect if state change fails

[Christoph Hellwig]

Looks ok, but I'd also prefer the additional WARN_ON.