[PATCH] ublk: refuse to go live after an io command was canceled

[PATCH] ublk: refuse to go live after an io command was canceled

[Josef Bacik]

Since commit "ublk: keep a canceled FETCH round canceling until the
server is gone", a device whose FETCH round saw a cancel keeps its
queues canceling until the server goes away, but START_DEV and
END_USER_RECOVERY still bring it up. Without UBLK_F_USER_RECOVERY, or
with UBLK_F_USER_RECOVERY_FAIL_IO, every request of the new disk fails.
With UBLK_F_USER_RECOVERY, requests are requeued and never kicked: after
START_DEV the partition scan hangs under disk->open_mutex, and after
END_USER_RECOVERY every read parks while the command returned 0. The
server cannot fetch the canceled commands again, so the device can't
serve I/O until it restarts anyway.

Return -ENODEV from START_DEV and END_USER_RECOVERY while ub->canceling
is set. In ublk_ctrl_start_dev() check it and publish ub->ub_disk in one
cancel_mutex section, and have ublk_start_cancel() read the disk in its
cancel_mutex section. Today ublk_start_cancel() samples the disk before
taking the mutex, so a server dying during its own START_DEV can mark
the queues without quiescing a disk START_DEV published in between, with
its first I/O past the canceling check. Now either START_DEV sees the
cancel, or the cancel sees the disk and quiesces it before marking. The
END_USER_RECOVERY check is best effort: the disk exists there, and a
cancel after it is the ordinary death of the new server, which
ublk_start_cancel() handles by quiescing and marking.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
This applies on top of Ming's "[PATCH 0/8] ublk: don't dispatch to
canceled io commands" and needs patch 1 of it for ub->canceling to stay
set for the whole FETCH round. generic_18 still passes with it.

 Documentation/block/ublk.rst | 10 ++++++++--
 drivers/block/ublk_drv.c     | 38 ++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
index 28300fee22bf..b7875a3cf3fc 100644
--- a/Documentation/block/ublk.rst
+++ b/Documentation/block/ublk.rst
@@ -118,7 +118,11 @@ managing and controlling ublk devices with help of several control commands:
   After the server prepares userspace resources (such as creating I/O handler
   threads & io_uring for handling ublk IO), this command is sent to the
   driver for allocating & exposing ``/dev/ublkb*``. Parameters set via
-  ``UBLK_CMD_SET_PARAMS`` are applied for creating the device.
+  ``UBLK_CMD_SET_PARAMS`` are applied for creating the device. The command
+  fails with ``-ENODEV`` if an I/O command fetched by the current server
+  was canceled, because its io_uring is gone. The server can't fetch it
+  again, and the device can be started again once the server has closed
+  ``/dev/ublkc*``.
 
 - ``UBLK_CMD_STOP_DEV``
 
@@ -195,7 +199,9 @@ managing and controlling ublk devices with help of several control commands:
   command is accepted after ublk device is quiesced and a new process has
   opened ``/dev/ublkc*`` and get all ublk queues be ready. When this command
   returns, ublk device is unquiesced and new I/O requests are passed to the
-  new process.
+  new process. It fails with ``-ENODEV`` if an I/O command of the new
+  process was canceled already. The recovery can be started over once the
+  new process has closed ``/dev/ublkc*``.
 
 - user recovery feature description
 
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index f57d544c1da2..39eb7775a351 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2759,9 +2759,11 @@ static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq)
 
 static void ublk_start_cancel(struct ublk_device *ub)
 {
-	struct gendisk *disk = ublk_get_disk(ub);
+	struct gendisk *disk;
 
+	/* sync with ublk_ctrl_start_dev() publishing the disk */
 	mutex_lock(&ub->cancel_mutex);
+	disk = ublk_get_disk(ub);
 	if (ub->canceling)
 		goto out;
 
@@ -4575,6 +4577,7 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
 		.dma_alignment		= 3,
 	};
 	struct gendisk *disk;
+	bool canceled;
 	int ret = -EINVAL;
 
 	if (ublksrv_pid <= 0)
@@ -4665,8 +4668,24 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
 	disk->fops = &ub_fops;
 	disk->private_data = ub;
 
+	/*
+	 * A command of this FETCH round was canceled and can't be fetched
+	 * again, don't bring up a disk over it.  Check and publish the disk
+	 * in one cancel_mutex section: either this sees ub->canceling, or
+	 * ublk_start_cancel() sees the disk and quiesces it before marking
+	 * the queues.
+	 */
+	mutex_lock(&ub->cancel_mutex);
+	canceled = ub->canceling;
+	if (!canceled)
+		ub->ub_disk = disk;
+	mutex_unlock(&ub->cancel_mutex);
+	if (canceled) {
+		put_disk(disk);
+		ret = -ENODEV;
+		goto out_unlock;
+	}
 	ub->dev_info.ublksrv_pid = ub->ublksrv_tgid;
-	ub->ub_disk = disk;
 
 	ublk_apply_params(ub);
 
@@ -5238,6 +5257,7 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
 		const struct ublksrv_ctrl_cmd *header)
 {
 	int ublksrv_pid = (int)header->data[0];
+	bool canceled;
 	int ret = -EINVAL;
 
 	pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
@@ -5261,6 +5281,20 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
 		ret = -EBUSY;
 		goto out_unlock;
 	}
+
+	/*
+	 * As in ublk_ctrl_start_dev(), a canceled command can't be fetched
+	 * again.  Best effort: the disk exists here, and a cancel after this
+	 * check is the ordinary death of the new server, which
+	 * ublk_start_cancel() handles by quiescing and marking.
+	 */
+	mutex_lock(&ub->cancel_mutex);
+	canceled = ub->canceling;
+	mutex_unlock(&ub->cancel_mutex);
+	if (canceled) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
 	ub->dev_info.ublksrv_pid = ub->ublksrv_tgid;
 	ub->dev_info.state = UBLK_S_DEV_LIVE;
 	pr_devel("%s: new ublksrv_pid %d, dev id %d\n",
-- 
2.55.0

[PATCH v2] ublk: refuse to go live after an io command was canceled

[Josef Bacik]

Since commit "ublk: keep a canceled FETCH round canceling until the
server is gone", a device whose FETCH round saw a cancel keeps its
queues canceling until the server goes away, but START_DEV and
END_USER_RECOVERY still bring it up. Without UBLK_F_USER_RECOVERY, or
with UBLK_F_USER_RECOVERY_FAIL_IO, every request of the new disk fails.
With UBLK_F_USER_RECOVERY, requests are requeued and never kicked: after
START_DEV the partition scan hangs under disk->open_mutex, and after
END_USER_RECOVERY every read parks while the command returned 0. The
server cannot fetch the canceled commands again, so the device can't
serve I/O until it restarts anyway.

Return -EBUSY from START_DEV and END_USER_RECOVERY while ub->canceling
is set. In ublk_ctrl_start_dev() check it and publish ub->ub_disk in one
cancel_mutex section, and have ublk_start_cancel() read the disk in its
cancel_mutex section. Today ublk_start_cancel() samples the disk before
taking the mutex, so a server dying during its own START_DEV can mark
the queues without quiescing a disk START_DEV published in between, with
its first I/O past the canceling check. Now either START_DEV sees the
cancel, or the cancel sees the disk and quiesces it before marking. The
END_USER_RECOVERY check is best effort: the disk exists there, and a
cancel after it is the ordinary death of the new server, which
ublk_start_cancel() handles by quiescing and marking.

Assisted-by: LLM
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v2: -EBUSY instead of -ENODEV, the device isn't gone (Ming)

This applies on top of Ming's "[PATCH 0/8] ublk: don't dispatch to
canceled io commands" and needs patch 1 of it for ub->canceling to stay
set for the whole FETCH round.

 Documentation/block/ublk.rst | 10 ++++++++--
 drivers/block/ublk_drv.c     | 38 ++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
index 28300fee22bf..6c5536ebd987 100644
--- a/Documentation/block/ublk.rst
+++ b/Documentation/block/ublk.rst
@@ -118,7 +118,11 @@ managing and controlling ublk devices with help of several control commands:
   After the server prepares userspace resources (such as creating I/O handler
   threads & io_uring for handling ublk IO), this command is sent to the
   driver for allocating & exposing ``/dev/ublkb*``. Parameters set via
-  ``UBLK_CMD_SET_PARAMS`` are applied for creating the device.
+  ``UBLK_CMD_SET_PARAMS`` are applied for creating the device. The command
+  fails with ``-EBUSY`` if an I/O command fetched by the current server
+  was canceled, because its io_uring is gone. The server can't fetch it
+  again, and the device can be started again once the server has closed
+  ``/dev/ublkc*``.
 
 - ``UBLK_CMD_STOP_DEV``
 
@@ -195,7 +199,9 @@ managing and controlling ublk devices with help of several control commands:
   command is accepted after ublk device is quiesced and a new process has
   opened ``/dev/ublkc*`` and get all ublk queues be ready. When this command
   returns, ublk device is unquiesced and new I/O requests are passed to the
-  new process.
+  new process. It fails with ``-EBUSY`` if an I/O command of the new
+  process was canceled already. The recovery can be started over once the
+  new process has closed ``/dev/ublkc*``.
 
 - user recovery feature description
 
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index f57d544c1da2..015aff07703c 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2759,9 +2759,11 @@ static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq)
 
 static void ublk_start_cancel(struct ublk_device *ub)
 {
-	struct gendisk *disk = ublk_get_disk(ub);
+	struct gendisk *disk;
 
+	/* sync with ublk_ctrl_start_dev() publishing the disk */
 	mutex_lock(&ub->cancel_mutex);
+	disk = ublk_get_disk(ub);
 	if (ub->canceling)
 		goto out;
 
@@ -4575,6 +4577,7 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
 		.dma_alignment		= 3,
 	};
 	struct gendisk *disk;
+	bool canceled;
 	int ret = -EINVAL;
 
 	if (ublksrv_pid <= 0)
@@ -4665,8 +4668,24 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
 	disk->fops = &ub_fops;
 	disk->private_data = ub;
 
+	/*
+	 * A command of this FETCH round was canceled and can't be fetched
+	 * again, don't bring up a disk over it.  Check and publish the disk
+	 * in one cancel_mutex section: either this sees ub->canceling, or
+	 * ublk_start_cancel() sees the disk and quiesces it before marking
+	 * the queues.
+	 */
+	mutex_lock(&ub->cancel_mutex);
+	canceled = ub->canceling;
+	if (!canceled)
+		ub->ub_disk = disk;
+	mutex_unlock(&ub->cancel_mutex);
+	if (canceled) {
+		put_disk(disk);
+		ret = -EBUSY;
+		goto out_unlock;
+	}
 	ub->dev_info.ublksrv_pid = ub->ublksrv_tgid;
-	ub->ub_disk = disk;
 
 	ublk_apply_params(ub);
 
@@ -5238,6 +5257,7 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
 		const struct ublksrv_ctrl_cmd *header)
 {
 	int ublksrv_pid = (int)header->data[0];
+	bool canceled;
 	int ret = -EINVAL;
 
 	pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
@@ -5261,6 +5281,20 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
 		ret = -EBUSY;
 		goto out_unlock;
 	}
+
+	/*
+	 * As in ublk_ctrl_start_dev(), a canceled command can't be fetched
+	 * again.  Best effort: the disk exists here, and a cancel after this
+	 * check is the ordinary death of the new server, which
+	 * ublk_start_cancel() handles by quiescing and marking.
+	 */
+	mutex_lock(&ub->cancel_mutex);
+	canceled = ub->canceling;
+	mutex_unlock(&ub->cancel_mutex);
+	if (canceled) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
 	ub->dev_info.ublksrv_pid = ub->ublksrv_tgid;
 	ub->dev_info.state = UBLK_S_DEV_LIVE;
 	pr_devel("%s: new ublksrv_pid %d, dev id %d\n",
-- 
2.55.0

Re: [PATCH] ublk: refuse to go live after an io command was canceled

[Ming Lei]

On Mon, Oct 05, 2026 at 04:23:51PM +0000, Josef Bacik wrote:
> Since commit "ublk: keep a canceled FETCH round canceling until the
> server is gone", a device whose FETCH round saw a cancel keeps its
> queues canceling until the server goes away, but START_DEV and
> END_USER_RECOVERY still bring it up. Without UBLK_F_USER_RECOVERY, or
> with UBLK_F_USER_RECOVERY_FAIL_IO, every request of the new disk fails.
> With UBLK_F_USER_RECOVERY, requests are requeued and never kicked: after
> START_DEV the partition scan hangs under disk->open_mutex, and after
> END_USER_RECOVERY every read parks while the command returned 0. The
> server cannot fetch the canceled commands again, so the device can't
> serve I/O until it restarts anyway.
> 
> Return -ENODEV from START_DEV and END_USER_RECOVERY while ub->canceling
> is set. In ublk_ctrl_start_dev() check it and publish ub->ub_disk in one
> cancel_mutex section, and have ublk_start_cancel() read the disk in its
> cancel_mutex section. Today ublk_start_cancel() samples the disk before
> taking the mutex, so a server dying during its own START_DEV can mark
> the queues without quiescing a disk START_DEV published in between, with
> its first I/O past the canceling check. Now either START_DEV sees the
> cancel, or the cancel sees the disk and quiesces it before marking. The
> END_USER_RECOVERY check is best effort: the disk exists there, and a
> cancel after it is the ordinary death of the new server, which
> ublk_start_cancel() handles by quiescing and marking.
> 
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> This applies on top of Ming's "[PATCH 0/8] ublk: don't dispatch to
> canceled io commands" and needs patch 1 of it for ub->canceling to stay
> set for the whole FETCH round. generic_18 still passes with it.
> 
>  Documentation/block/ublk.rst | 10 ++++++++--
>  drivers/block/ublk_drv.c     | 38 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
> index 28300fee22bf..b7875a3cf3fc 100644
> --- a/Documentation/block/ublk.rst
> +++ b/Documentation/block/ublk.rst
> @@ -118,7 +118,11 @@ managing and controlling ublk devices with help of several control commands:
>    After the server prepares userspace resources (such as creating I/O handler
>    threads & io_uring for handling ublk IO), this command is sent to the
>    driver for allocating & exposing ``/dev/ublkb*``. Parameters set via
> -  ``UBLK_CMD_SET_PARAMS`` are applied for creating the device.
> +  ``UBLK_CMD_SET_PARAMS`` are applied for creating the device. The command
> +  fails with ``-ENODEV`` if an I/O command fetched by the current server
> +  was canceled, because its io_uring is gone. The server can't fetch it
> +  again, and the device can be started again once the server has closed
> +  ``/dev/ublkc*``.
>  
>  - ``UBLK_CMD_STOP_DEV``
>  
> @@ -195,7 +199,9 @@ managing and controlling ublk devices with help of several control commands:
>    command is accepted after ublk device is quiesced and a new process has
>    opened ``/dev/ublkc*`` and get all ublk queues be ready. When this command
>    returns, ublk device is unquiesced and new I/O requests are passed to the
> -  new process.
> +  new process. It fails with ``-ENODEV`` if an I/O command of the new
> +  process was canceled already. The recovery can be started over once the
> +  new process has closed ``/dev/ublkc*``.
>  
>  - user recovery feature description
>  
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index f57d544c1da2..39eb7775a351 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -2759,9 +2759,11 @@ static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq)
>  
>  static void ublk_start_cancel(struct ublk_device *ub)
>  {
> -	struct gendisk *disk = ublk_get_disk(ub);
> +	struct gendisk *disk;
>  
> +	/* sync with ublk_ctrl_start_dev() publishing the disk */
>  	mutex_lock(&ub->cancel_mutex);
> +	disk = ublk_get_disk(ub);
>  	if (ub->canceling)
>  		goto out;
>  
> @@ -4575,6 +4577,7 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
>  		.dma_alignment		= 3,
>  	};
>  	struct gendisk *disk;
> +	bool canceled;
>  	int ret = -EINVAL;
>  
>  	if (ublksrv_pid <= 0)
> @@ -4665,8 +4668,24 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
>  	disk->fops = &ub_fops;
>  	disk->private_data = ub;
>  
> +	/*
> +	 * A command of this FETCH round was canceled and can't be fetched
> +	 * again, don't bring up a disk over it.  Check and publish the disk
> +	 * in one cancel_mutex section: either this sees ub->canceling, or
> +	 * ublk_start_cancel() sees the disk and quiesces it before marking
> +	 * the queues.
> +	 */
> +	mutex_lock(&ub->cancel_mutex);
> +	canceled = ub->canceling;
> +	if (!canceled)
> +		ub->ub_disk = disk;
> +	mutex_unlock(&ub->cancel_mutex);
> +	if (canceled) {
> +		put_disk(disk);
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
>  	ub->dev_info.ublksrv_pid = ub->ublksrv_tgid;
> -	ub->ub_disk = disk;
>  
>  	ublk_apply_params(ub);
>  
> @@ -5238,6 +5257,7 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
>  		const struct ublksrv_ctrl_cmd *header)
>  {
>  	int ublksrv_pid = (int)header->data[0];
> +	bool canceled;
>  	int ret = -EINVAL;
>  
>  	pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
> @@ -5261,6 +5281,20 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
>  		ret = -EBUSY;
>  		goto out_unlock;
>  	}
> +
> +	/*
> +	 * As in ublk_ctrl_start_dev(), a canceled command can't be fetched
> +	 * again.  Best effort: the disk exists here, and a cancel after this
> +	 * check is the ordinary death of the new server, which
> +	 * ublk_start_cancel() handles by quiescing and marking.
> +	 */
> +	mutex_lock(&ub->cancel_mutex);
> +	canceled = ub->canceling;
> +	mutex_unlock(&ub->cancel_mutex);
> +	if (canceled) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
>  	ub->dev_info.ublksrv_pid = ub->ublksrv_tgid;
>  	ub->dev_info.state = UBLK_S_DEV_LIVE;
>  	pr_devel("%s: new ublksrv_pid %d, dev id %d\n",

Hi Josef,

Thanks for the follow-up. The check and the cancel_mutex ordering
look right to me, but I'd suggest -EBUSY instead of -ENODEV.

-ENODEV means the ublk device is gone, and it isn't true in the
START_DEV/END_RECOVERY cases.

With -EBUSY:

Reviewed-by: Ming Lei <tom.leiming@gmail.com>


thanks,
Ming