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

[Ming Lei <tom.leiming@gmail.com>]

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