[PATCH] nvme_fc: avoid workqueue flush stalls

[PATCH] nvme_fc: avoid workqueue flush stalls

[James Smart]

There's no need to wait for the full nvme_wq, which is now shared,
to flush. flush only the delete_work item.

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

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index ac898571b1e5..2bd94de09b8a 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2846,7 +2846,7 @@ nvme_fc_del_nvme_ctrl(struct nvme_ctrl *nctrl)
 	ret = __nvme_fc_del_ctrl(ctrl);
 
 	if (!ret)
-		flush_workqueue(nvme_wq);
+		flush_work(&ctrl->delete_work);
 
 	nvme_put_ctrl(&ctrl->ctrl);
 
-- 
2.13.1

[PATCH] nvme_fc: avoid workqueue flush stalls

[Sagi Grimberg]

> There's no need to wait for the full nvme_wq, which is now shared,
> to flush. flush only the delete_work item.

The patch looks correct, but can you explain what stalled that this
patch fixes?

[PATCH] nvme_fc: avoid workqueue flush stalls

[James Smart]

On 9/30/2017 10:18 AM, Sagi Grimberg wrote:
>
>> There's no need to wait for the full nvme_wq, which is now shared,
>> to flush. flush only the delete_work item.
>
> The patch looks correct, but can you explain what stalled that this
> patch fixes?

I don't have an explicit failure case. While reviewing the code for the 
other stall issue with fatal_err_work, I realized that this flush is now 
waiting on a work queue shared by everything nvmf.? It used to be solely 
a nvme-fc transport workqueue. There's no need for waiting on anything 
more than the delete work item. And having it raise in scope of the nvmf 
thread perhaps makes it wait for a lot of other unnecessary things - 
slowing it down.

If you're ok with it - can you Ack/Review it ?

-- james

[PATCH] nvme_fc: avoid workqueue flush stalls

[Sagi Grimberg]

>>> There's no need to wait for the full nvme_wq, which is now shared,
>>> to flush. flush only the delete_work item.
>>
>> The patch looks correct, but can you explain what stalled that this
>> patch fixes?
> 
> I don't have an explicit failure case. While reviewing the code for the 
> other stall issue with fatal_err_work, I realized that this flush is now 
> waiting on a work queue shared by everything nvmf.? It used to be solely 
> a nvme-fc transport workqueue. There's no need for waiting on anything 
> more than the delete work item. And having it raise in scope of the nvmf 
> thread perhaps makes it wait for a lot of other unnecessary things - 
> slowing it down.
> 
> If you're ok with it - can you Ack/Review it ?

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