Skip to contentSkip to conversation

nvmet: Don't queue fatal error work if csts.cfs is set

PATCH Superseded Sagi Grimberg 2 participants 3 messages Nov 6, 2016 – Nov 8, 2016

Subsystems (2)
NVMe NVMe target
lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

PATCH v2 1/1 nvmet: Don't queue fatal error work if csts.cfs is set

In the transport, in case of an interal queue error like error completion in rdma we trigger a fatal error. However, multiple queues in the same controller can serr error completions and we don't want to trigger fatal error work more than once.

Reviewed-by Christoph Hellwig <hch at lst.de>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
Changes from v1:
- reversed the if condition to avoid a goto statement

Changes from v0:
- protect with ctrl lock instead of abusing atomic bitops on

    ctrl->csts
 drivers/nvme/target/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/nvme/target/core.c +7 -3
index 7c0fe31ee654..55ce769cecee 100644
@@ -840,9 +840,13 @@ static void nvmet_fatal_error_handler(struct work_struct *work)
 
 void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl)
 {
-	ctrl->csts |= NVME_CSTS_CFS;
-	INIT_WORK(&ctrl->fatal_err_work, nvmet_fatal_error_handler);
-	schedule_work(&ctrl->fatal_err_work);
+	mutex_lock(&ctrl->lock);
+	if (!(ctrl->csts & NVME_CSTS_CFS)) {
+		ctrl->csts |= NVME_CSTS_CFS;
+		INIT_WORK(&ctrl->fatal_err_work, nvmet_fatal_error_handler);
+		schedule_work(&ctrl->fatal_err_work);
+	}
+	mutex_unlock(&ctrl->lock);
 }
 EXPORT_SYMBOL_GPL(nvmet_ctrl_fatal_error);
Signature · 1 line
2.7.4
#1 lore raw

PATCH v2 1/1 nvmet: Don't queue fatal error work if csts.cfs is set

Quoted 35 lines · Sagi Grimberg

In the transport, in case of an interal queue error like error completion in rdma we trigger a fatal error. However, multiple queues in the same controller can serr error completions and we don't want to trigger fatal error work more than once.

Reviewed-by Christoph Hellwig <hch at lst.de>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
Changes from v1:
- reversed the if condition to avoid a goto statement

Changes from v0:
- protect with ctrl lock instead of abusing atomic bitops on ????ctrl->csts

?drivers/nvme/target/core.c | 10 +++++++---
?1 file changed, 7 insertions(+), 3 deletions(-)

drivers/nvme/target/core.c +0 -0
index 7c0fe31ee654..55ce769cecee 100644
@@ -840,9 +840,13 @@ static void nvmet_fatal_error_handler(struct
 work_struct *work)

?
?void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl) ?{

drivers/nvme/target/core.c +4 -3
-	ctrl->csts |= NVME_CSTS_CFS;
-	INIT_WORK(&ctrl->fatal_err_work, nvmet_fatal_error_handler);
-	schedule_work(&ctrl->fatal_err_work);
+	mutex_lock(&ctrl->lock);
+	if (!(ctrl->csts & NVME_CSTS_CFS)) {
+		ctrl->csts |= NVME_CSTS_CFS;
+		INIT_WORK(&ctrl->fatal_err_work,

nvmet_fatal_error_handler);

drivers/nvme/target/core.c static void nvmet_fatal_error_handler(struct work_struct *work) · line 849

Quoted 3 lines
drivers/nvme/target/core.c +3 -0
+		schedule_work(&ctrl->fatal_err_work);
+	}
+	mutex_unlock(&ctrl->lock);

Surrounding the mutex_lock()/unlock() around the if() is definitely safer and a more conservative, but I would think it would be OK to stick the mutex_lock/unlock() within the if() statement (mutex_lock() as the first line after the if() check, mutex_unlock() as the last line of the if()), as trying to acquire a lock before checking the condition to see if CFS needs to be set is sub-optimal.

Quoted 3 lines

?}
?EXPORT_SYMBOL_GPL(nvmet_ctrl_fatal_error);
?

author maintainer
#2 lore raw

PATCH v2 1/1 nvmet: Don't queue fatal error work if csts.cfs is set

Quoted 13 lines
Quoted 13 lines

void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl) {

drivers/nvme/target/core.c +4 -3
-	ctrl->csts |= NVME_CSTS_CFS;
-	INIT_WORK(&ctrl->fatal_err_work, nvmet_fatal_error_handler);
-	schedule_work(&ctrl->fatal_err_work);
+	mutex_lock(&ctrl->lock);
+	if (!(ctrl->csts & NVME_CSTS_CFS)) {
+		ctrl->csts |= NVME_CSTS_CFS;
+		INIT_WORK(&ctrl->fatal_err_work,

nvmet_fatal_error_handler);

drivers/nvme/target/core.c +2 -0
+		schedule_work(&ctrl->fatal_err_work);
+	}
Quoted 8 lines

drivers/nvme/target/core.c static void nvmet_fatal_error_handler(struct work_struct *work) · line 849

Quoted 1 line
drivers/nvme/target/core.c +1 -0
+	mutex_unlock(&ctrl->lock);

Surrounding the mutex_lock()/unlock() around the if() is definitely safer and a more conservative, but I would think it would be OK to stick the mutex_lock/unlock() within the if() statement (mutex_lock() as the first line after the if() check, mutex_unlock() as the last line of the if()), as trying to acquire a lock before checking the condition to see if CFS needs to be set is sub-optimal.

I don't think this is worth optimizing, it is error flow after all..