ublk: improve detection and handling of ublk server exit

There are currently two ways in which ublk server exit is detected by
ublk_drv:

1. uring_cmd cancellation. If there are any outstanding uring_cmds which
   have not been completed to the ublk server when it exits, io_uring
   calls the uring_cmd callback with a special cancellation flag as the
   issuing task is exiting.
2. I/O timeout. This is needed in addition to the above to handle the
   "saturated queue" case, when all I/Os for a given queue are in the
   ublk server, and therefore there are no outstanding uring_cmds to
   cancel when the ublk server exits.

There are a couple of issues with this approach:

- It is complex and inelegant to have two methods to detect the same
  condition
- The second method detects ublk server exit only after a long delay
  (~30s, the default timeout assigned by the block layer). This delays
  the nosrv behavior from kicking in and potential subsequent recovery
  of the device.

The second issue is brought to light with the new test_generic_06 which
will be added in following patch. It fails before this fix:

selftests: ublk: test_generic_06.sh
dev id is 0
dd: error writing '/dev/ublkb0': Input/output error
1+0 records in
0+0 records out
0 bytes copied, 30.0611 s, 0.0 kB/s
DEAD
dd took 31 seconds to exit (>= 5s tolerance)!
generic_06 : [FAIL]

Fix this by instead detecting and handling ublk server exit in the
character file release callback. This has several advantages:

- This one place can handle both saturated and unsaturated queues. Thus,
  it replaces both preexisting methods of detecting ublk server exit.
- It runs quickly on ublk server exit - there is no 30s delay.
- It starts the process of removing task references in ublk_drv. This is
  needed if we want to relax restrictions in the driver like letting
  only one thread serve each queue

There is also the disadvantage that the character file release callback
can also be triggered by intentional close of the file, which is a
significant behavior change. Preexisting ublk servers (libublksrv) are
dependent on the ability to open/close the file multiple times. To
address this, only transition to a nosrv state if the file is released
while the ublk device is live. This allows for programs to open/close
the file multiple times during setup. It is still a behavior change if a
ublk server decides to close/reopen the file while the device is LIVE
(i.e. while it is responsible for serving I/O), but that would be highly
unusual. This behavior is in line with what is done by FUSE, which is
very similar to ublk in that a userspace daemon is providing services
traditionally provided by the kernel.

With this change in, the new test (and all other selftests, and all
ublksrv tests) pass:

selftests: ublk: test_generic_06.sh
dev id is 0
dd: error writing '/dev/ublkb0': Input/output error
1+0 records in
0+0 records out
0 bytes copied, 0.0376731 s, 0.0 kB/s
DEAD
generic_04 : [PASS]

Signed-off-by: Uday Shankar <ushankar@purestorage.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Link: https://lore.kernel.org/r/20250416035444.99569-6-ming.lei@redhat.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 1fe39cf85b2f4dfebee08cd24f6bd1a6deb0b17e..b0a7e5acb2eb1c95da9e4541f04f6d671cad6b33 100644 (file)
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -199,8 +199,6 @@ struct ublk_device {
        struct completion       completion;
        unsigned int            nr_queues_ready;
        unsigned int            nr_privileged_daemon;
-
-       struct work_struct      nosrv_work;
 };
 
 /* header of ublk_params */
@@ -209,8 +207,9 @@ struct ublk_params_header {
        __u32   types;
 };
 
-static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq);
-
+static void ublk_stop_dev_unlocked(struct ublk_device *ub);
+static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
+static void __ublk_quiesce_dev(struct ublk_device *ub);
 static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
                struct ublk_queue *ubq, int tag, size_t offset);
 static inline unsigned int ublk_req_build_flags(struct request *req);
@@ -1336,8 +1335,6 @@ static void ublk_queue_cmd_list(struct ublk_queue *ubq, struct rq_list *l)
 static enum blk_eh_timer_return ublk_timeout(struct request *rq)
 {
        struct ublk_queue *ubq = rq->mq_hctx->driver_data;
-       unsigned int nr_inflight = 0;
-       int i;
 
        if (ubq->flags & UBLK_F_UNPRIVILEGED_DEV) {
                if (!ubq->timeout) {
@@ -1348,26 +1345,6 @@ static enum blk_eh_timer_return ublk_timeout(struct request *rq)
                return BLK_EH_DONE;
        }
 
-       if (!ubq_daemon_is_dying(ubq))
-               return BLK_EH_RESET_TIMER;
-
-       for (i = 0; i < ubq->q_depth; i++) {
-               struct ublk_io *io = &ubq->ios[i];
-
-               if (!(io->flags & UBLK_IO_FLAG_ACTIVE))
-                       nr_inflight++;
-       }
-
-       /* cancelable uring_cmd can't help us if all commands are in-flight */
-       if (nr_inflight == ubq->q_depth) {
-               struct ublk_device *ub = ubq->dev;
-
-               if (ublk_abort_requests(ub, ubq)) {
-                       schedule_work(&ub->nosrv_work);
-               }
-               return BLK_EH_DONE;
-       }
-
        return BLK_EH_RESET_TIMER;
 }
 
@@ -1525,13 +1502,105 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
        ub->nr_privileged_daemon = 0;
 }
 
+static struct gendisk *ublk_get_disk(struct ublk_device *ub)
+{
+       struct gendisk *disk;
+
+       spin_lock(&ub->lock);
+       disk = ub->ub_disk;
+       if (disk)
+               get_device(disk_to_dev(disk));
+       spin_unlock(&ub->lock);
+
+       return disk;
+}
+
+static void ublk_put_disk(struct gendisk *disk)
+{
+       if (disk)
+               put_device(disk_to_dev(disk));
+}
+
 static int ublk_ch_release(struct inode *inode, struct file *filp)
 {
        struct ublk_device *ub = filp->private_data;
+       struct gendisk *disk;
+       int i;
+
+       /*
+        * disk isn't attached yet, either device isn't live, or it has
+        * been removed already, so we needn't to do anything
+        */
+       disk = ublk_get_disk(ub);
+       if (!disk)
+               goto out;
+
+       /*
+        * All uring_cmd are done now, so abort any request outstanding to
+        * the ublk server
+        *
+        * This can be done in lockless way because ublk server has been
+        * gone
+        *
+        * More importantly, we have to provide forward progress guarantee
+        * without holding ub->mutex, otherwise control task grabbing
+        * ub->mutex triggers deadlock
+        *
+        * All requests may be inflight, so ->canceling may not be set, set
+        * it now.
+        */
+       for (i = 0; i < ub->dev_info.nr_hw_queues; i++) {
+               struct ublk_queue *ubq = ublk_get_queue(ub, i);
+
+               ubq->canceling = true;
+               ublk_abort_queue(ub, ubq);
+       }
+       blk_mq_kick_requeue_list(disk->queue);
+
+       /*
+        * All infligh requests have been completed or requeued and any new
+        * request will be failed or requeued via `->canceling` now, so it is
+        * fine to grab ub->mutex now.
+        */
+       mutex_lock(&ub->mutex);
+
+       /* double check after grabbing lock */
+       if (!ub->ub_disk)
+               goto unlock;
+
+       /*
+        * Transition the device to the nosrv state. What exactly this
+        * means depends on the recovery flags
+        */
+       blk_mq_quiesce_queue(disk->queue);
+       if (ublk_nosrv_should_stop_dev(ub)) {
+               /*
+                * Allow any pending/future I/O to pass through quickly
+                * with an error. This is needed because del_gendisk
+                * waits for all pending I/O to complete
+                */
+               for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
+                       ublk_get_queue(ub, i)->force_abort = true;
+               blk_mq_unquiesce_queue(disk->queue);
+
+               ublk_stop_dev_unlocked(ub);
+       } else {
+               if (ublk_nosrv_dev_should_queue_io(ub)) {
+                       __ublk_quiesce_dev(ub);
+               } else {
+                       ub->dev_info.state = UBLK_S_DEV_FAIL_IO;
+                       for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
+                               ublk_get_queue(ub, i)->fail_io = true;
+               }
+               blk_mq_unquiesce_queue(disk->queue);
+       }
+unlock:
+       mutex_unlock(&ub->mutex);
+       ublk_put_disk(disk);
 
        /* all uring_cmd has been done now, reset device & ubq */
        ublk_reset_ch_dev(ub);
-
+out:
        clear_bit(UB_STATE_OPEN, &ub->state);
        return 0;
 }
@@ -1627,37 +1696,22 @@ static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq)
 }
 
 /* Must be called when queue is frozen */
-static bool ublk_mark_queue_canceling(struct ublk_queue *ubq)
+static void ublk_mark_queue_canceling(struct ublk_queue *ubq)
 {
-       bool canceled;
-
        spin_lock(&ubq->cancel_lock);
-       canceled = ubq->canceling;
-       if (!canceled)
+       if (!ubq->canceling)
                ubq->canceling = true;
        spin_unlock(&ubq->cancel_lock);
-
-       return canceled;
 }
 
-static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
+static void ublk_start_cancel(struct ublk_queue *ubq)
 {
-       bool was_canceled = ubq->canceling;
-       struct gendisk *disk;
-
-       if (was_canceled)
-               return false;
-
-       spin_lock(&ub->lock);
-       disk = ub->ub_disk;
-       if (disk)
-               get_device(disk_to_dev(disk));
-       spin_unlock(&ub->lock);
+       struct ublk_device *ub = ubq->dev;
+       struct gendisk *disk = ublk_get_disk(ub);
 
        /* Our disk has been dead */
        if (!disk)
-               return false;
-
+               return;
        /*
         * Now we are serialized with ublk_queue_rq()
         *
@@ -1666,15 +1720,9 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
         * touch completed uring_cmd
         */
        blk_mq_quiesce_queue(disk->queue);
-       was_canceled = ublk_mark_queue_canceling(ubq);
-       if (!was_canceled) {
-               /* abort queue is for making forward progress */
-               ublk_abort_queue(ub, ubq);
-       }
+       ublk_mark_queue_canceling(ubq);
        blk_mq_unquiesce_queue(disk->queue);
-       put_device(disk_to_dev(disk));
-
-       return !was_canceled;
+       ublk_put_disk(disk);
 }
 
 static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
@@ -1698,6 +1746,17 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
 /*
  * The ublk char device won't be closed when calling cancel fn, so both
  * ublk device and queue are guaranteed to be live
+ *
+ * Two-stage cancel:
+ *
+ * - make every active uring_cmd done in ->cancel_fn()
+ *
+ * - aborting inflight ublk IO requests in ublk char device release handler,
+ *   which depends on 1st stage because device can only be closed iff all
+ *   uring_cmd are done
+ *
+ * Do _not_ try to acquire ub->mutex before all inflight requests are
+ * aborted, otherwise deadlock may be caused.
  */
 static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
                unsigned int issue_flags)
@@ -1705,8 +1764,6 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
        struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
        struct ublk_queue *ubq = pdu->ubq;
        struct task_struct *task;
-       struct ublk_device *ub;
-       bool need_schedule;
        struct ublk_io *io;
 
        if (WARN_ON_ONCE(!ubq))
@@ -1719,16 +1776,12 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
        if (WARN_ON_ONCE(task && task != ubq->ubq_daemon))
                return;
 
-       ub = ubq->dev;
-       need_schedule = ublk_abort_requests(ub, ubq);
+       if (!ubq->canceling)
+               ublk_start_cancel(ubq);
 
        io = &ubq->ios[pdu->tag];
        WARN_ON_ONCE(io->cmd != cmd);
        ublk_cancel_cmd(ubq, io, issue_flags);
-
-       if (need_schedule) {
-               schedule_work(&ub->nosrv_work);
-       }
 }
 
 static inline bool ublk_queue_ready(struct ublk_queue *ubq)
@@ -1787,13 +1840,11 @@ static void __ublk_quiesce_dev(struct ublk_device *ub)
                        __func__, ub->dev_info.dev_id,
                        ub->dev_info.state == UBLK_S_DEV_LIVE ?
                        "LIVE" : "QUIESCED");
-       blk_mq_quiesce_queue(ub->ub_disk->queue);
        /* mark every queue as canceling */
        for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
                ublk_get_queue(ub, i)->canceling = true;
        ublk_wait_tagset_rqs_idle(ub);
        ub->dev_info.state = UBLK_S_DEV_QUIESCED;
-       blk_mq_unquiesce_queue(ub->ub_disk->queue);
 }
 
 static void ublk_force_abort_dev(struct ublk_device *ub)
@@ -1830,50 +1881,25 @@ static struct gendisk *ublk_detach_disk(struct ublk_device *ub)
        return disk;
 }
 
-static void ublk_stop_dev(struct ublk_device *ub)
+static void ublk_stop_dev_unlocked(struct ublk_device *ub)
+       __must_hold(&ub->mutex)
 {
        struct gendisk *disk;
 
-       mutex_lock(&ub->mutex);
        if (ub->dev_info.state == UBLK_S_DEV_DEAD)
-               goto unlock;
+               return;
+
        if (ublk_nosrv_dev_should_queue_io(ub))
                ublk_force_abort_dev(ub);
        del_gendisk(ub->ub_disk);
        disk = ublk_detach_disk(ub);
        put_disk(disk);
- unlock:
-       mutex_unlock(&ub->mutex);
-       ublk_cancel_dev(ub);
 }
 
-static void ublk_nosrv_work(struct work_struct *work)
+static void ublk_stop_dev(struct ublk_device *ub)
 {
-       struct ublk_device *ub =
-               container_of(work, struct ublk_device, nosrv_work);
-       int i;
-
-       if (ublk_nosrv_should_stop_dev(ub)) {
-               ublk_stop_dev(ub);
-               return;
-       }
-
        mutex_lock(&ub->mutex);
-       if (ub->dev_info.state != UBLK_S_DEV_LIVE)
-               goto unlock;
-
-       if (ublk_nosrv_dev_should_queue_io(ub)) {
-               __ublk_quiesce_dev(ub);
-       } else {
-               blk_mq_quiesce_queue(ub->ub_disk->queue);
-               ub->dev_info.state = UBLK_S_DEV_FAIL_IO;
-               for (i = 0; i < ub->dev_info.nr_hw_queues; i++) {
-                       ublk_get_queue(ub, i)->fail_io = true;
-               }
-               blk_mq_unquiesce_queue(ub->ub_disk->queue);
-       }
-
- unlock:
+       ublk_stop_dev_unlocked(ub);
        mutex_unlock(&ub->mutex);
        ublk_cancel_dev(ub);
 }
@@ -2502,7 +2528,6 @@ static void ublk_remove(struct ublk_device *ub)
        bool unprivileged;
 
        ublk_stop_dev(ub);
-       cancel_work_sync(&ub->nosrv_work);
        cdev_device_del(&ub->cdev, &ub->cdev_dev);
        unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
        ublk_put_device(ub);
@@ -2787,7 +2812,6 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
                goto out_unlock;
        mutex_init(&ub->mutex);
        spin_lock_init(&ub->lock);
-       INIT_WORK(&ub->nosrv_work, ublk_nosrv_work);
 
        ret = ublk_alloc_dev_number(ub, header->dev_id);
        if (ret < 0)
@@ -2919,7 +2943,6 @@ static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
 static int ublk_ctrl_stop_dev(struct ublk_device *ub)
 {
        ublk_stop_dev(ub);
-       cancel_work_sync(&ub->nosrv_work);
        return 0;
 }