[PATCH] nvme_fc: fix abort race on teardown with lld reject

[PATCH] nvme_fc: fix abort race on teardown with lld reject

[James Smart]

Another abort race: An io request is started, becomes active,
and is attempted to be started with the lldd. At the same time
the controller is stopped/torndown and an itterator is run to
abort the ios. As the io is active, it is added to the outstanding
aborted io count.  However on the original io request thread, the
driver ends up rejecting the io due to the condition that induced
the controller teardown. The driver reject path didn't check whether
it was in the outstanding io count. This left the count outstanding
stopping controller teardown.

Correct by, in the driver reject case, setting the state to
inactive and checking whether it was in the outstanding io count.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/fc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index bd72e8e4dfb7..8cacaccac90d 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2193,7 +2193,7 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 	struct nvme_fc_cmd_iu *cmdiu = &op->cmd_iu;
 	struct nvme_command *sqe = &cmdiu->sqe;
 	u32 csn;
-	int ret;
+	int ret, opstate;
 
 	/*
 	 * before attempting to send the io, check to see if we believe
@@ -2271,6 +2271,9 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 					queue->lldd_handle, &op->fcp_req);
 
 	if (ret) {
+		opstate = atomic_xchg(&op->state, FCPOP_STATE_COMPLETE);
+		__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
+
 		if (!(op->flags & FCOP_FLAGS_AEN))
 			nvme_fc_unmap_data(ctrl, op->rq, op);
 
-- 
2.13.1

[PATCH] nvmet_fc: prevent new io rqsts in possible isr completions

[James Smart]

When a bio completion calls back into the transport for a
back-end io device, the request completion path can free
the transport io job structure allowing it to be reused for
other operations. The transport has a defer_rcv queue which
holds temporary cmd rcv ops while waitng for io job structures.
when the job frees, if there's a cmd waiting, it is picked up
and submitted for processing, which can call back out to the
bio path if it's a read.  Unfortunately, what is unknown is the
context of the original bio done call, and it may be in a state
(softirq) that is not compatible with submitting the new bio in
the same calling sequence. This is especially true when using
scsi back-end devices as scsi is in softirq when it makes the
done call.

Correct by scheduling the io to be started via workq rather
than calling the start new io path inline to the original bio
done path.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/target/fc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 9b39a6cb1935..9f80f98d81d2 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -87,6 +87,7 @@ struct nvmet_fc_fcp_iod {
 	struct nvmet_req		req;
 	struct work_struct		work;
 	struct work_struct		done_work;
+	struct work_struct		defer_work;
 
 	struct nvmet_fc_tgtport		*tgtport;
 	struct nvmet_fc_tgt_queue	*queue;
@@ -224,6 +225,7 @@ static DEFINE_IDA(nvmet_fc_tgtport_cnt);
 static void nvmet_fc_handle_ls_rqst_work(struct work_struct *work);
 static void nvmet_fc_handle_fcp_rqst_work(struct work_struct *work);
 static void nvmet_fc_fcp_rqst_op_done_work(struct work_struct *work);
+static void nvmet_fc_fcp_rqst_op_defer_work(struct work_struct *work);
 static void nvmet_fc_tgt_a_put(struct nvmet_fc_tgt_assoc *assoc);
 static int nvmet_fc_tgt_a_get(struct nvmet_fc_tgt_assoc *assoc);
 static void nvmet_fc_tgt_q_put(struct nvmet_fc_tgt_queue *queue);
@@ -429,6 +431,7 @@ nvmet_fc_prep_fcp_iodlist(struct nvmet_fc_tgtport *tgtport,
 	for (i = 0; i < queue->sqsize; fod++, i++) {
 		INIT_WORK(&fod->work, nvmet_fc_handle_fcp_rqst_work);
 		INIT_WORK(&fod->done_work, nvmet_fc_fcp_rqst_op_done_work);
+		INIT_WORK(&fod->defer_work, nvmet_fc_fcp_rqst_op_defer_work);
 		fod->tgtport = tgtport;
 		fod->queue = queue;
 		fod->active = false;
@@ -512,6 +515,17 @@ nvmet_fc_queue_fcp_req(struct nvmet_fc_tgtport *tgtport,
 }
 
 static void
+nvmet_fc_fcp_rqst_op_defer_work(struct work_struct *work)
+{
+	struct nvmet_fc_fcp_iod *fod =
+		container_of(work, struct nvmet_fc_fcp_iod, defer_work);
+
+	/* Submit deferred IO for processing */
+	nvmet_fc_queue_fcp_req(fod->tgtport, fod->queue, fod->fcpreq);
+
+}
+
+static void
 nvmet_fc_free_fcp_iod(struct nvmet_fc_tgt_queue *queue,
 			struct nvmet_fc_fcp_iod *fod)
 {
@@ -568,13 +582,12 @@ nvmet_fc_free_fcp_iod(struct nvmet_fc_tgt_queue *queue,
 	/* inform LLDD IO is now being processed */
 	tgtport->ops->defer_rcv(&tgtport->fc_target_port, fcpreq);
 
-	/* Submit deferred IO for processing */
-	nvmet_fc_queue_fcp_req(tgtport, queue, fcpreq);
-
 	/*
 	 * Leave the queue lookup get reference taken when
 	 * fod was originally allocated.
 	 */
+
+	queue_work(queue->work_q, &fod->defer_work);
 }
 
 static int
-- 
2.13.1

[PATCH] nvmet_fc: prevent new io rqsts in possible isr completions

[Sagi Grimberg]

Hi James,

> When a bio completion calls back into the transport for a
> back-end io device, the request completion path can free
> the transport io job structure allowing it to be reused for
> other operations.

Is this true doe something that is not aborted?

> The transport has a defer_rcv queue which
> holds temporary cmd rcv ops while waitng for io job structures.
> when the job frees, if there's a cmd waiting, it is picked up
> and submitted for processing, which can call back out to the
> bio path if it's a read.  Unfortunately, what is unknown is the
> context of the original bio done call, and it may be in a state
> (softirq) that is not compatible with submitting the new bio in
> the same calling sequence. This is especially true when using
> scsi back-end devices as scsi is in softirq when it makes the
> done call.
> 
> Correct by scheduling the io to be started via workq rather
> than calling the start new io path inline to the original bio
> done path.

Isn't free_iod called for every rsp completion? Is triggering defer_work
unconditionally the optimal approach? In other words, won't that yield
extra overhead in the "normal" case?

[PATCH] nvme_fc: fix abort race on teardown with lld reject

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH] nvmet_fc: prevent new io rqsts in possible isr completions

[James Smart]

On 3/1/2018 4:04 AM, Sagi Grimberg wrote:
> Hi James,
>
>> When a bio completion calls back into the transport for a
>> back-end io device, the request completion path can free
>> the transport io job structure allowing it to be reused for
>> other operations.
>
> Is this true doe something that is not aborted?
??? I think -??? it's true regardless of whether the link-side io was 
aborted or not.
This is simply the bio completion path for say a read op, to get the 
data so the
transport can send it.

>
>> The transport has a defer_rcv queue which
>> holds temporary cmd rcv ops while waitng for io job structures.
>> when the job frees, if there's a cmd waiting, it is picked up
>> and submitted for processing, which can call back out to the
>> bio path if it's a read.? Unfortunately, what is unknown is the
>> context of the original bio done call, and it may be in a state
>> (softirq) that is not compatible with submitting the new bio in
>> the same calling sequence. This is especially true when using
>> scsi back-end devices as scsi is in softirq when it makes the
>> done call.
>>
>> Correct by scheduling the io to be started via workq rather
>> than calling the start new io path inline to the original bio
>> done path.
>
> Isn't free_iod called for every rsp completion? Is triggering defer_work
> unconditionally the optimal approach? In other words, won't that yield
> extra overhead in the "normal" case?

Yes, free_iod is on every completion. The check for defer_work is the 
overhead on every io. However, in most cases, there is never deferred 
work so the workq scheduling is rarely taken.? If it is taken, I'm fine 
with it always being a longer workq path.

The defer_rcv thing is an oddity about FC. In FC we really don't have 
dedicated landing buffers for all cmds. The transport allocates a io job 
struct for each SQ entry. But we found, the interaction with the lldd 
around sending the rsp back allowed the rsp to go on the wire and the 
initiator to send a new FC command, which could arrive before the 
adapter serviced its interrupt and did all the handshaking with the 
transport to return the job structure. So new cmd arrival beat free_iod 
even when under queue size limits.? This only happens when you are 
hovering at 100% full on the SQ.

-- james