queue ready fixes and cleanups v2

queue ready fixes and cleanups v2

[Christoph Hellwig]

Changes since v1:
 - fixed the inverted fc ready check 
 - squashed two patches
 - complete redo of the last patch to only allow the connect command on
   a non-live queue in the right states

[PATCH 1/3] nvme-fabrics: refactor queue ready check

[Christoph Hellwig]

Move the is_connected check to the fibre channel transport, as it has no
meaning for other transports.  To facilitate this split out a new
nvmf_fail_nonready_command helper that is called by the transport when
it is asked to handle a command on a queue that is not ready.

Also avoid a function call for the queue live fast path by inlining
the check.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/fabrics.c | 59 +++++++++++++++----------------------
 drivers/nvme/host/fabrics.h | 13 ++++++--
 drivers/nvme/host/fc.c      |  9 +++---
 drivers/nvme/host/rdma.c    |  7 ++---
 drivers/nvme/target/loop.c  |  7 ++---
 5 files changed, 45 insertions(+), 50 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index fa32c1216409..6b4e253b9347 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -536,30 +536,32 @@ static struct nvmf_transport_ops *nvmf_lookup_transport(
 	return NULL;
 }
 
-blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq,
-		bool queue_live, bool is_connected)
+/*
+ * For something we're not in a state to send to the device the default action
+ * is to busy it and retry it after the controller state is recovered.  However,
+ * anything marked for failfast or nvme multipath is immediately failed.
+ *
+ * Note: commands used to initialize the controller will be marked for failfast.
+ * Note: nvme cli/ioctl commands are marked for failfast.
+ */
+blk_status_t nvmf_fail_nonready_command(struct request *rq)
 {
-	struct nvme_command *cmd = nvme_req(rq)->cmd;
+	if (!blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
+		return BLK_STS_RESOURCE;
+	nvme_req(rq)->status = NVME_SC_ABORT_REQ;
+	return BLK_STS_IOERR;
+}
+EXPORT_SYMBOL_GPL(nvmf_fail_nonready_command);
 
-	if (likely(ctrl->state == NVME_CTRL_LIVE && is_connected))
-		return BLK_STS_OK;
+bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
+		bool queue_live)
+{
+	struct nvme_command *cmd = nvme_req(rq)->cmd;
 
 	switch (ctrl->state) {
 	case NVME_CTRL_NEW:
 	case NVME_CTRL_CONNECTING:
 	case NVME_CTRL_DELETING:
-		/*
-		 * This is the case of starting a new or deleting an association
-		 * but connectivity was lost before it was fully created or torn
-		 * down. We need to error the commands used to initialize the
-		 * controller so the reconnect can go into a retry attempt.  The
-		 * commands should all be marked REQ_FAILFAST_DRIVER, which will
-		 * hit the reject path below. Anything else will be queued while
-		 * the state settles.
-		 */
-		if (!is_connected)
-			break;
-
 		/*
 		 * If queue is live, allow only commands that are internally
 		 * generated pass through.  These are commands on the admin
@@ -567,7 +569,7 @@ blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq,
 		 * ioctl admin cmds received while initializing.
 		 */
 		if (queue_live && !(nvme_req(rq)->flags & NVME_REQ_USERCMD))
-			return BLK_STS_OK;
+			return true;
 
 		/*
 		 * If the queue is not live, allow only a connect command.  This
@@ -577,26 +579,13 @@ blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq,
 		if (!queue_live && blk_rq_is_passthrough(rq) &&
 		     cmd->common.opcode == nvme_fabrics_command &&
 		     cmd->fabrics.fctype == nvme_fabrics_type_connect)
-			return BLK_STS_OK;
-		break;
+			return true;
+		return false;
 	default:
-		break;
+		return false;
 	}
-
-	/*
-	 * Any other new io is something we're not in a state to send to the
-	 * device.  Default action is to busy it and retry it after the
-	 * controller state is recovered. However, anything marked for failfast
-	 * or nvme multipath is immediately failed.  Note: commands used to
-	 * initialize the controller will be marked for failfast.
-	 * Note: nvme cli/ioctl commands are marked for failfast.
-	 */
-	if (!blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
-		return BLK_STS_RESOURCE;
-	nvme_req(rq)->status = NVME_SC_ABORT_REQ;
-	return BLK_STS_IOERR;
 }
-EXPORT_SYMBOL_GPL(nvmf_check_if_ready);
+EXPORT_SYMBOL_GPL(__nvmf_check_ready);
 
 static const match_table_t opt_tokens = {
 	{ NVMF_OPT_TRANSPORT,		"transport=%s"		},
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 7491a0bbf711..2ea949a3868c 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -162,7 +162,16 @@ void nvmf_unregister_transport(struct nvmf_transport_ops *ops);
 void nvmf_free_options(struct nvmf_ctrl_options *opts);
 int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
 bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
-blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl,
-	struct request *rq, bool queue_live, bool is_connected);
+blk_status_t nvmf_fail_nonready_command(struct request *rq);
+bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
+		bool queue_live);
+
+static inline bool nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
+		bool queue_live)
+{
+	if (likely(ctrl->state == NVME_CTRL_LIVE))
+		return true;
+	return __nvmf_check_ready(ctrl, rq, queue_live);
+}
 
 #endif /* _NVME_FABRICS_H */
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 0bad65803271..99bb4672eb3d 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2279,14 +2279,13 @@ nvme_fc_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct nvme_fc_cmd_iu *cmdiu = &op->cmd_iu;
 	struct nvme_command *sqe = &cmdiu->sqe;
 	enum nvmefc_fcp_datadir	io_dir;
+	bool queue_ready = test_bit(NVME_FC_Q_LIVE, &queue->flags);
 	u32 data_len;
 	blk_status_t ret;
 
-	ret = nvmf_check_if_ready(&queue->ctrl->ctrl, rq,
-		test_bit(NVME_FC_Q_LIVE, &queue->flags),
-		ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE);
-	if (unlikely(ret))
-		return ret;
+	if (ctrl->rport->remoteport.port_state != FC_OBJSTATE_ONLINE ||
+	    !nvmf_check_ready(&queue->ctrl->ctrl, rq, queue_ready))
+		return nvmf_fail_nonready_command(rq);
 
 	ret = nvme_setup_cmd(ns, rq, sqe);
 	if (ret)
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 7cd4199db225..c9424da0d23e 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1630,15 +1630,14 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct nvme_rdma_qe *sqe = &req->sqe;
 	struct nvme_command *c = sqe->data;
 	struct ib_device *dev;
+	bool queue_ready = test_bit(NVME_RDMA_Q_LIVE, &queue->flags);
 	blk_status_t ret;
 	int err;
 
 	WARN_ON_ONCE(rq->tag < 0);
 
-	ret = nvmf_check_if_ready(&queue->ctrl->ctrl, rq,
-		test_bit(NVME_RDMA_Q_LIVE, &queue->flags), true);
-	if (unlikely(ret))
-		return ret;
+	if (!nvmf_check_ready(&queue->ctrl->ctrl, rq, queue_ready))
+		return nvmf_fail_nonready_command(rq);
 
 	dev = queue->device->dev;
 	ib_dma_sync_single_for_cpu(dev, sqe->dma,
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 1304ec3a7ede..d8d91f04bd7e 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -158,12 +158,11 @@ static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct nvme_loop_queue *queue = hctx->driver_data;
 	struct request *req = bd->rq;
 	struct nvme_loop_iod *iod = blk_mq_rq_to_pdu(req);
+	bool queue_ready = test_bit(NVME_LOOP_Q_LIVE, &queue->flags);
 	blk_status_t ret;
 
-	ret = nvmf_check_if_ready(&queue->ctrl->ctrl, req,
-		test_bit(NVME_LOOP_Q_LIVE, &queue->flags), true);
-	if (unlikely(ret))
-		return ret;
+	if (!nvmf_check_ready(&queue->ctrl->ctrl, req, queue_ready))
+		return nvmf_fail_nonready_command(req);
 
 	ret = nvme_setup_cmd(ns, req, &iod->cmd);
 	if (ret)
-- 
2.17.1

[PATCH 2/3] nvme-fabrics: handle the admin-only case properly in nvmf_check_ready

[Christoph Hellwig]

In the ADMIN_ONLY state we don't have any I/O queues, but we should accept
all admin commands without further checks.

Signed-off-by: Christoph Hellwig <hch at lst.de>
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by:?James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/fabrics.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 2ea949a3868c..e1818a27aa2d 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -169,7 +169,8 @@ bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 static inline bool nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 		bool queue_live)
 {
-	if (likely(ctrl->state == NVME_CTRL_LIVE))
+	if (likely(ctrl->state == NVME_CTRL_LIVE ||
+		   ctrl->state == NVME_CTRL_ADMIN_ONLY))
 		return true;
 	return __nvmf_check_ready(ctrl, rq, queue_live);
 }
-- 
2.17.1

[PATCH 3/3] nvme-fabrics: fix and refine state checks in __nvmf_check_ready

[Christoph Hellwig]

 - make sure we only allow internally generates commands in any non-live
   state
 - only allow connect commands on non-live queues when actually in the
   new or connecting states
 - treat all other non-live, non-dead states the same as a default
   cach-all

This fixes a regression where we could not shutdown a controller
orderly as we didn't allow the internal generated Property Set
command, and also ensures we don't accidentally let a Connect command
through in the wrong state.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/fabrics.c | 39 ++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 6b4e253b9347..903eb4545e26 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -556,34 +556,33 @@ EXPORT_SYMBOL_GPL(nvmf_fail_nonready_command);
 bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 		bool queue_live)
 {
-	struct nvme_command *cmd = nvme_req(rq)->cmd;
+	struct nvme_request *req = nvme_req(rq);
 
+	/*
+	 * If we are in some state of setup or teardown only allow
+	 * internally generated commands.
+	 */
+	if (!blk_rq_is_passthrough(rq) || (req->flags & NVME_REQ_USERCMD))
+		return false;
+
+	/*
+	 * Only allow commands on a live queue, except for the connect command,
+	 * which is require to set the queue live in the appropinquate states.
+	 */
 	switch (ctrl->state) {
 	case NVME_CTRL_NEW:
 	case NVME_CTRL_CONNECTING:
-	case NVME_CTRL_DELETING:
-		/*
-		 * If queue is live, allow only commands that are internally
-		 * generated pass through.  These are commands on the admin
-		 * queue to initialize the controller. This will reject any
-		 * ioctl admin cmds received while initializing.
-		 */
-		if (queue_live && !(nvme_req(rq)->flags & NVME_REQ_USERCMD))
+		if (req->cmd->common.opcode == nvme_fabrics_command &&
+		    req->cmd->fabrics.fctype == nvme_fabrics_type_connect)
 			return true;
-
-		/*
-		 * If the queue is not live, allow only a connect command.  This
-		 * will reject any ioctl admin cmd as well as initialization
-		 * commands if the controller reverted the queue to non-live.
-		 */
-		if (!queue_live && blk_rq_is_passthrough(rq) &&
-		     cmd->common.opcode == nvme_fabrics_command &&
-		     cmd->fabrics.fctype == nvme_fabrics_type_connect)
-			return true;
-		return false;
+		break;
 	default:
+		break;
+	case NVME_CTRL_DEAD:
 		return false;
 	}
+
+	return queue_live;
 }
 EXPORT_SYMBOL_GPL(__nvmf_check_ready);
 
-- 
2.17.1

[PATCH 3/3] nvme-fabrics: fix and refine state checks in __nvmf_check_ready

[James Smart]

On 6/14/2018 5:22 AM, Christoph Hellwig wrote:
>   - make sure we only allow internally generates commands in any non-live
>     state
>   - only allow connect commands on non-live queues when actually in the
>     new or connecting states
>   - treat all other non-live, non-dead states the same as a default
>     cach-all
>
> This fixes a regression where we could not shutdown a controller
> orderly as we didn't allow the internal generated Property Set
> command, and also ensures we don't accidentally let a Connect command
> through in the wrong state.
>
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>   drivers/nvme/host/fabrics.c | 39 ++++++++++++++++++-------------------
>   1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 6b4e253b9347..903eb4545e26 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -556,34 +556,33 @@ EXPORT_SYMBOL_GPL(nvmf_fail_nonready_command);
>   bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
>   		bool queue_live)
>   {
> -	struct nvme_command *cmd = nvme_req(rq)->cmd;
> +	struct nvme_request *req = nvme_req(rq);
>   
> +	/*
> +	 * If we are in some state of setup or teardown only allow
> +	 * internally generated commands.
> +	 */
> +	if (!blk_rq_is_passthrough(rq) || (req->flags & NVME_REQ_USERCMD))
> +		return false;
> +
> +	/*
> +	 * Only allow commands on a live queue, except for the connect command,
> +	 * which is require to set the queue live in the appropinquate states.
> +	 */
>   	switch (ctrl->state) {
>   	case NVME_CTRL_NEW:
>   	case NVME_CTRL_CONNECTING:
> -	case NVME_CTRL_DELETING:
> -		/*
> -		 * If queue is live, allow only commands that are internally
> -		 * generated pass through.  These are commands on the admin
> -		 * queue to initialize the controller. This will reject any
> -		 * ioctl admin cmds received while initializing.
> -		 */
> -		if (queue_live && !(nvme_req(rq)->flags & NVME_REQ_USERCMD))
> +		if (req->cmd->common.opcode == nvme_fabrics_command &&
> +		    req->cmd->fabrics.fctype == nvme_fabrics_type_connect)
>   			return true;
> -
> -		/*
> -		 * If the queue is not live, allow only a connect command.  This
> -		 * will reject any ioctl admin cmd as well as initialization
> -		 * commands if the controller reverted the queue to non-live.
> -		 */
> -		if (!queue_live && blk_rq_is_passthrough(rq) &&
> -		     cmd->common.opcode == nvme_fabrics_command &&
> -		     cmd->fabrics.fctype == nvme_fabrics_type_connect)
> -			return true;
> -		return false;
> +		break;
>   	default:
> +		break;
> +	case NVME_CTRL_DEAD:
>   		return false;
>   	}
> +
> +	return queue_live;
>   }
>   EXPORT_SYMBOL_GPL(__nvmf_check_ready);
>   

I like what's here and think it's pretty conclusive. Enough that I'm 
good with a Reviewed-by for me to be added.

In thinking this through: There's still the issue of queue_live being 
set true when in a RESETTING/DELETING state as the workqueue element to 
enact the reset/delete has yet to be run or is just starting to run. 
Since above restricts it to kernel-generated commands only, the only 
things that should get through in this case are:
1) a property_set command to write CC.NE=0 is issued by the transport 
reset/delete handler. Note: it's unclear whether the queue will be live 
or not.
2) controller was in a NEW/CONNECTING state and was sending a command 
used to init the controller.
3) a keep alive

(1) is desired behavior, assuming queue is live to let it through.
(2) and (3) - I assume the transport reset/delete handler will freeze 
the queues, cycle through any outstanding command and will terminate 
them. So it should be ok. I assume, based on the time needed to freeze 
the queues, that there shouldn't be a possibility for the delete/reset 
handler queue freeze code to race with an io allowed by the checks (it 
should be long gone).?? If there's not agreement on that assumption, 
then a clause should be added after the switch case that checks for 
RESETTING or DELETING and if so, only allows a Property_Set for CC.EN=0 
command.

-- james

[PATCH 1/3] nvme-fabrics: refactor queue ready check

[James Smart]

On 6/14/2018 5:22 AM, Christoph Hellwig wrote:
> Move the is_connected check to the fibre channel transport, as it has no
> meaning for other transports.  To facilitate this split out a new
> nvmf_fail_nonready_command helper that is called by the transport when
> it is asked to handle a command on a queue that is not ready.
>
> Also avoid a function call for the queue live fast path by inlining
> the check.
>
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>   drivers/nvme/host/fabrics.c | 59 +++++++++++++++----------------------
>   drivers/nvme/host/fabrics.h | 13 ++++++--
>   drivers/nvme/host/fc.c      |  9 +++---
>   drivers/nvme/host/rdma.c    |  7 ++---
>   drivers/nvme/target/loop.c  |  7 ++---
>   5 files changed, 45 insertions(+), 50 deletions(-)
>
>

looks good

Reviewed-by:? James Smart?? <james.smart at broadcom.com>

[PATCH 3/3] nvme-fabrics: fix and refine state checks in __nvmf_check_ready

[Christoph Hellwig]

On Thu, Jun 14, 2018@10:37:01AM -0700, James Smart wrote:
> I like what's here and think it's pretty conclusive. Enough that I'm good 
> with a Reviewed-by for me to be added.

Thanks.

> In thinking this through: There's still the issue of queue_live being set 
> true when in a RESETTING/DELETING state as the workqueue element to enact 
> the reset/delete has yet to be run or is just starting to run. Since above 
> restricts it to kernel-generated commands only, the only things that should 
> get through in this case are:
> 1) a property_set command to write CC.NE=0 is issued by the transport 
> reset/delete handler. Note: it's unclear whether the queue will be live or 
> not.
> 2) controller was in a NEW/CONNECTING state and was sending a command used 
> to init the controller.
> 3) a keep alive
>
> (1) is desired behavior, assuming queue is live to let it through.
> (2) and (3) - I assume the transport reset/delete handler will freeze the 
> queues, cycle through any outstanding command and will terminate them. So 
> it should be ok.

Yes.

> I assume, based on the time needed to freeze the queues, 
> that there shouldn't be a possibility for the delete/reset handler queue 
> freeze code to race with an io allowed by the checks (it should be long 
> gone).?? If there's not agreement on that assumption, then a clause 
> should be added after the switch case that checks for RESETTING or DELETING 
> and if so, only allows a Property_Set for CC.EN=0 command.

Note that in PCIe until recently we actually had other commands in
the shutdown sequence, and at least for an oderly shutdown other commands
might reappear.  So I'm not really sure adding additional restrictions
is a good idea, as we'd need to update the state machine every time
the shutdown sequence changes.  Especially given that the queue should
be frozen for external I/O anyway.

>
> -- james
---end quoted text---

[PATCH 3/3] nvme-fabrics: fix and refine state checks in __nvmf_check_ready

[James Smart]

On 6/15/2018 2:32 AM, Christoph Hellwig wrote:
> On Thu, Jun 14, 2018@10:37:01AM -0700, James Smart wrote:
>> I like what's here and think it's pretty conclusive. Enough that I'm good
>> with a Reviewed-by for me to be added.
> Thanks.
>
>> In thinking this through: There's still the issue of queue_live being set
>> true when in a RESETTING/DELETING state as the workqueue element to enact
>> the reset/delete has yet to be run or is just starting to run. Since above
>> restricts it to kernel-generated commands only, the only things that should
>> get through in this case are:
>> 1) a property_set command to write CC.NE=0 is issued by the transport
>> reset/delete handler. Note: it's unclear whether the queue will be live or
>> not.
>> 2) controller was in a NEW/CONNECTING state and was sending a command used
>> to init the controller.
>> 3) a keep alive
>>
>> (1) is desired behavior, assuming queue is live to let it through.
>> (2) and (3) - I assume the transport reset/delete handler will freeze the
>> queues, cycle through any outstanding command and will terminate them. So
>> it should be ok.
> Yes.
>
>> I assume, based on the time needed to freeze the queues,
>> that there shouldn't be a possibility for the delete/reset handler queue
>> freeze code to race with an io allowed by the checks (it should be long
>> gone).?? If there's not agreement on that assumption, then a clause
>> should be added after the switch case that checks for RESETTING or DELETING
>> and if so, only allows a Property_Set for CC.EN=0 command.
> Note that in PCIe until recently we actually had other commands in
> the shutdown sequence, and at least for an oderly shutdown other commands
> might reappear.  So I'm not really sure adding additional restrictions
> is a good idea, as we'd need to update the state machine every time
> the shutdown sequence changes.  Especially given that the queue should
> be frozen for external I/O anyway.
>
>> -- james
> ---end quoted text---

Sounds good.

-- james