Re: [PATCH 20/98] mtip32xx: don't mark the disk dead in the irq handler

[Bart Van Assche <bvanassche@acm.org>]

On 10/28/26 09:41, Jens Axboe wrote:
> @@ -2492,6 +2512,9 @@ static int mtip_service_thread(void *data)
>  
>  		set_bit(MTIP_PF_SVC_THD_ACTIVE_BIT, &port->flags);
>  
> +		if (test_and_clear_bit(MTIP_PF_SR_BIT, &port->flags))
> +			mtip_check_surprise_removal(dd);
> +
>  restart_eh:

Deferring blk_mark_disk_dead() out of the hardirq handler is needed, but
calling blk_mark_disk_dead(dd->disk) in mtip_service_thread() before
aborting in-flight NCQ commands can deadlock when a filesystem is
mounted on the device:

1. blk_mark_disk_dead() invokes fs_bdev_mark_dead(), which takes
   sb->s_umount via bdev_super_lock() and calls sync_filesystem(). If
   another task already holds s_umount while waiting for in-flight I/O
   on the dead PCIe device, mtip_service_thread() blocks on s_umount.
2. In patch 22/98, mtip_pci_remove() calls mtip_stop_service_thread(dd)
   (kthread_stop()) before aborting in-flight commands, so
   mtip_pci_remove() blocks waiting for mtip_service_thread() before it
   can abort the commands holding up s_umount.

Also, mtip_abort_cmd() currently calls mtip_softirq_done_fn(req)
directly without transitioning the request through
blk_mq_complete_request().

How about keeping mtip_check_surprise_removal() non-sleeping everywhere
(setting dd->sr, a new MTIP_DDF_DEAD_BIT to fail new normal and reserved
requests immediately with BLK_STS_IOERR, and waking the service thread
with MTIP_PF_SR_BIT), and having a helper mtip_handle_surprise_removal()
quiesce dd->queue and abort in-flight requests via
blk_mq_tagset_busy_iter() + blk_mq_complete_request() before calling
blk_mark_disk_dead(dd->disk)?

Patch 21/98 still applies cleanly on top of this replacement, and my
reply to patch 22/98 has the matching follow-up change. Checked with
checkpatch.pl and syntax-checked with Clang; not runtime-tested on
mtip32xx hardware:

---
diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 113bdb868c46..35568d0e74cf 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -141,8 +141,11 @@ static bool mtip_check_surprise_removal(struct driver_data *dd)
 	pci_read_config_word(dd->pdev, 0x00, &vendor_id);
 	if (vendor_id == 0xFFFF) {
 		dd->sr = true;
-		if (dd->disk)
-			blk_mark_disk_dead(dd->disk);
+		set_bit(MTIP_DDF_DEAD_BIT, &dd->dd_flag);
+		if (dd->port) {
+			set_bit(MTIP_PF_SR_BIT, &dd->port->flags);
+			wake_up_interruptible(&dd->port->svc_wait);
+		}
 		return true; /* device removed */
 	}
 
@@ -2438,9 +2441,12 @@ static bool mtip_abort_cmd(struct request *req, void *data)
 
 	dbg_printk(MTIP_DRV_NAME " Aborting request, tag = %d\n", req->tag);
 
+	if (blk_mq_request_completed(req))
+		return true;
+
 	clear_bit(req->tag, dd->port->cmds_to_issue);
 	cmd->status = BLK_STS_IOERR;
-	mtip_softirq_done_fn(req);
+	blk_mq_complete_request(req);
 	return true;
 }
 
@@ -2462,6 +2468,18 @@ static bool mtip_queue_cmd(struct request *req, void *data)
  *	0
  */
 
+static void mtip_handle_surprise_removal(struct driver_data *dd)
+{
+	if (dd->queue) {
+		blk_mq_quiesce_queue(dd->queue);
+		blk_mq_tagset_busy_iter(&dd->tags, mtip_abort_cmd, dd);
+		blk_mq_unquiesce_queue(dd->queue);
+	}
+	if (test_bit(MTIP_DDF_INIT_DONE_BIT, &dd->dd_flag) &&
+	    !test_and_set_bit(MTIP_DDF_DISK_DEAD_BIT, &dd->dd_flag))
+		blk_mark_disk_dead(dd->disk);
+}
+
 static int mtip_service_thread(void *data)
 {
 	struct driver_data *dd = (struct driver_data *)data;
@@ -2492,6 +2510,15 @@ static int mtip_service_thread(void *data)
 
 		set_bit(MTIP_PF_SVC_THD_ACTIVE_BIT, &port->flags);
 
+		if (test_and_clear_bit(MTIP_PF_SR_BIT, &port->flags)) {
+			mtip_handle_surprise_removal(dd);
+			clear_bit(MTIP_PF_EH_ACTIVE_BIT, &port->flags);
+			clear_bit(MTIP_PF_TO_ACTIVE_BIT, &port->flags);
+			clear_bit(MTIP_PF_ISSUE_CMDS_BIT, &port->flags);
+			clear_bit(MTIP_PF_REBUILD_BIT, &port->flags);
+			continue;
+		}
+
 restart_eh:
 		/* Demux bits: start with error handling */
 		if (test_bit(MTIP_PF_EH_ACTIVE_BIT, &port->flags)) {
@@ -3241,6 +3268,8 @@ static inline bool is_stopped(struct driver_data *dd, struct request *rq)
 		return true;
 	if (test_bit(MTIP_DDF_REBUILD_FAILED_BIT, &dd->dd_flag))
 		return true;
+	if (test_bit(MTIP_DDF_DEAD_BIT, &dd->dd_flag))
+		return true;
 
 	return false;
 }
@@ -3279,6 +3308,9 @@ static blk_status_t mtip_issue_reserved_cmd(struct blk_mq_hw_ctx *hctx,
 		dd->port->command_list + sizeof(struct mtip_cmd_hdr) * rq->tag;
 	struct mtip_cmd_sg *command_sg;
 
+	if (test_bit(MTIP_DDF_DEAD_BIT, &dd->dd_flag))
+		return BLK_STS_IOERR;
+
 	if (mtip_commands_active(dd->port))
 		return BLK_STS_DEV_RESOURCE;
 
diff --git a/drivers/block/mtip32xx/mtip32xx.h b/drivers/block/mtip32xx/mtip32xx.h
index 0963c07b5845..af0249e3c550 100644
--- a/drivers/block/mtip32xx/mtip32xx.h
+++ b/drivers/block/mtip32xx/mtip32xx.h
@@ -134,11 +134,13 @@ enum {
 	MTIP_PF_ISSUE_CMDS_BIT      = 5,
 	MTIP_PF_REBUILD_BIT         = 6,
 	MTIP_PF_SVC_THD_STOP_BIT    = 8,
+	MTIP_PF_SR_BIT              = 11, /* surprise removal seen */
 
 	MTIP_PF_SVC_THD_WORK	= ((1 << MTIP_PF_EH_ACTIVE_BIT) |
 				  (1 << MTIP_PF_ISSUE_CMDS_BIT) |
 				  (1 << MTIP_PF_REBUILD_BIT) |
 				  (1 << MTIP_PF_SVC_THD_STOP_BIT) |
+				  (1 << MTIP_PF_SR_BIT) |
 				  (1 << MTIP_PF_TO_ACTIVE_BIT)),
 
 	/* below are bit numbers in 'dd_flag' defined in driver_data */
@@ -150,12 +152,15 @@ enum {
 	MTIP_DDF_RESUME_BIT         = 6,
 	MTIP_DDF_INIT_DONE_BIT      = 7,
 	MTIP_DDF_REBUILD_FAILED_BIT = 8,
+	MTIP_DDF_DEAD_BIT           = 9,
+	MTIP_DDF_DISK_DEAD_BIT      = 10,
 
 	MTIP_DDF_STOP_IO      = ((1 << MTIP_DDF_REMOVE_PENDING_BIT) |
 				(1 << MTIP_DDF_SEC_LOCK_BIT) |
 				(1 << MTIP_DDF_OVER_TEMP_BIT) |
 				(1 << MTIP_DDF_WRITE_PROTECT_BIT) |
-				(1 << MTIP_DDF_REBUILD_FAILED_BIT)),
+				(1 << MTIP_DDF_REBUILD_FAILED_BIT) |
+				(1 << MTIP_DDF_DEAD_BIT)),
 
 };