[PATCH] nvme_fc: replace ioabort msleep loop with completion

[PATCH] nvme_fc: replace ioabort msleep loop with completion

[jsmart2021]

From: James Smart <jsmart2021@gmail.com>

Per the recommendation by Sagi on:
http://lists.infradead.org/pipermail/linux-nvme/2017-April/009261.html

Wait for io aborts to complete wait converted from msleep look to
using a struct completion.

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

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 16a658c..2690824 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -167,6 +167,7 @@ struct nvme_fc_ctrl {
 	struct kref		ref;
 	u32			flags;
 	u32			iocnt;
+	struct completion	ioaborts_done;
 
 	struct nvme_fc_fcp_op	aen_ops[NVME_FC_NR_AEN_COMMANDS];
 
@@ -1241,8 +1242,10 @@ __nvme_fc_fcpop_chk_teardowns(struct nvme_fc_ctrl *ctrl,
 
 	spin_lock_irqsave(&ctrl->lock, flags);
 	if (unlikely(op->flags & FCOP_FLAGS_TERMIO)) {
-		if (ctrl->flags & FCCTRL_TERMIO)
-			ctrl->iocnt--;
+		if (ctrl->flags & FCCTRL_TERMIO) {
+			if (!(--ctrl->iocnt))
+				complete(&ctrl->ioaborts_done);
+		}
 	}
 	if (op->flags & FCOP_FLAGS_RELEASED)
 		complete_rq = true;
@@ -2488,10 +2491,14 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 
 	/* wait for all io that had to be aborted */
 	spin_lock_irqsave(&ctrl->lock, flags);
-	while (ctrl->iocnt) {
+	if (ctrl->iocnt) {
+		init_completion(&ctrl->ioaborts_done);
 		spin_unlock_irqrestore(&ctrl->lock, flags);
-		msleep(1000);
+
+		wait_for_completion(&ctrl->ioaborts_done);
+
 		spin_lock_irqsave(&ctrl->lock, flags);
+		WARN_ON(ctrl->iocnt);
 	}
 	ctrl->flags &= ~FCCTRL_TERMIO;
 	spin_unlock_irqrestore(&ctrl->lock, flags);
-- 
2.9.3

[PATCH] nvme_fc: replace ioabort msleep loop with completion

[Johannes Thumshirn]

On 04/26/2017 12:32 AM, jsmart2021@gmail.com wrote:
> From: James Smart <jsmart2021 at gmail.com>
> 
> Per the recommendation by Sagi on:
> http://lists.infradead.org/pipermail/linux-nvme/2017-April/009261.html
> 
> Wait for io aborts to complete wait converted from msleep look to
> using a struct completion.
> 
> Signed-off-by: James Smart <james.smart at broadcom.com>
> ---

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] nvme_fc: replace ioabort msleep loop with completion

[Christoph Hellwig]

> +			if (!(--ctrl->iocnt))

No need for the inner braces here.

> -	while (ctrl->iocnt) {
> +	if (ctrl->iocnt) {
> +		init_completion(&ctrl->ioaborts_done);
>  		spin_unlock_irqrestore(&ctrl->lock, flags);
> -		msleep(1000);
> +
> +		wait_for_completion(&ctrl->ioaborts_done);
> +
>  		spin_lock_irqsave(&ctrl->lock, flags);
> +		WARN_ON(ctrl->iocnt);

But more importantly the single wait is potentially wrong.  Please
switch to a waitqueue and use wait_event_lock_irq here.

[PATCH] nvme_fc: replace ioabort msleep loop with completion

[James Smart]

On 5/10/2017 10:22 AM, Christoph Hellwig wrote:

>> -	while (ctrl->iocnt) {
>> +	if (ctrl->iocnt) {
>> +		init_completion(&ctrl->ioaborts_done);
>>  		spin_unlock_irqrestore(&ctrl->lock, flags);
>> -		msleep(1000);
>> +
>> +		wait_for_completion(&ctrl->ioaborts_done);
>> +
>>  		spin_lock_irqsave(&ctrl->lock, flags);
>> +		WARN_ON(ctrl->iocnt);
>
> But more importantly the single wait is potentially wrong.  Please
> switch to a waitqueue and use wait_event_lock_irq here.
>

Why do you think a single wait is wrong ?

All aborts have been scheduled, which is what ctrl->iocnt tracks, and 
will not increase. Thus ctrl->iocnt is set to its max prior to init'ing 
the completion and releasing the lock. The completion only triggers on 
decrement of ctrl->iocnt to zero checked under lock.

-- james

[PATCH] nvme_fc: replace ioabort msleep loop with completion

[James Smart]

On 4/25/2017 3:32 PM, jsmart2021@gmail.com wrote:
> From: James Smart <jsmart2021 at gmail.com>
>
> Per the recommendation by Sagi on:
> http://lists.infradead.org/pipermail/linux-nvme/2017-April/009261.html
>
> An extra reference was pointed out.  There's no issue with the
> references, but rather a literal interpretation of what the comment
> is saying.
>
> Reword the comment to avoid confusion.
>
> Signed-off-by: James Smart <james.smart at broadcom.com>
> ---
>  drivers/nvme/host/fc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 2690824..e917d91 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2540,10 +2540,10 @@ nvme_fc_delete_ctrl_work(struct work_struct *work)
>
>  	/*
>  	 * tear down the controller
> -	 * This will result in the last reference on the nvme ctrl to
> -	 * expire, calling the transport nvme_fc_nvme_ctrl_freed() callback.
> -	 * From there, the transport will tear down it's logical queues and
> -	 * association.
> +	 * After the last reference on the nvme ctrl is removed,
> +	 * the transport nvme_fc_nvme_ctrl_freed() callback will be
> +	 * invoked. From there, the transport will tear down it's
> +	 * logical queues and association.
>  	 */
>  	nvme_uninit_ctrl(&ctrl->ctrl);
>
>
sorry for noise- will fix title

-- james

[PATCH] nvme_fc: replace ioabort msleep loop with completion

[jsmart2021]

From: James Smart <jsmart2021@gmail.com>

Per the recommendation by Sagi on:
http://lists.infradead.org/pipermail/linux-nvme/2017-April/009261.html

An extra reference was pointed out.  There's no issue with the
references, but rather a literal interpretation of what the comment
is saying.

Reword the comment to avoid confusion.

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

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 2690824..e917d91 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2540,10 +2540,10 @@ nvme_fc_delete_ctrl_work(struct work_struct *work)
 
 	/*
 	 * tear down the controller
-	 * This will result in the last reference on the nvme ctrl to
-	 * expire, calling the transport nvme_fc_nvme_ctrl_freed() callback.
-	 * From there, the transport will tear down it's logical queues and
-	 * association.
+	 * After the last reference on the nvme ctrl is removed,
+	 * the transport nvme_fc_nvme_ctrl_freed() callback will be
+	 * invoked. From there, the transport will tear down it's
+	 * logical queues and association.
 	 */
 	nvme_uninit_ctrl(&ctrl->ctrl);
 
-- 
2.9.3