For performance reasons (minimizing the number of cache lines accessed
in the hot path), we store the "canceling" state redundantly - there is
one flag in the device, which can be considered the source of truth, and
per-queue copies of that flag. This redundancy can cause confusion, and
opens the door to bugs where the state is set inconsistently. Try to
guard against these bugs by introducing a ublk_set_canceling helper
which is the sole mutator of both the per-device and per-queue canceling
state. This helper always sets the state consistently. Use the helper in
all places where we need to modify the canceling state.
No functional changes are expected.
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
drivers/block/ublk_drv.c | 54 ++++++++++++++++++++++++++++++------------------
1 file changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 870d57a853a481c2443337717c50d39355804f66..a1a700c7e67a72597e740aaa60f5c3c73f0085e5 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1563,6 +1563,27 @@ static void ublk_put_disk(struct gendisk *disk)
put_device(disk_to_dev(disk));
}
+/*
+ * Use this function to ensure that ->canceling is consistently set for
+ * the device and all queues. Do not set these flags directly.
+ *
+ * Caller must ensure that:
+ * - cancel_mutex is held. This ensures that there is no concurrent
+ * access to ub->canceling and no concurrent writes to ubq->canceling.
+ * - there are no concurrent reads of ubq->canceling from the queue_rq
+ * path. This can be done by quiescing the queue, or through other
+ * means.
+ */
+static void ublk_set_canceling(struct ublk_device *ub, bool canceling)
+ __must_hold(&ub->cancel_mutex)
+{
+ int i;
+
+ ub->canceling = canceling;
+ for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
+ ublk_get_queue(ub, i)->canceling = canceling;
+}
+
static int ublk_ch_release(struct inode *inode, struct file *filp)
{
struct ublk_device *ub = filp->private_data;
@@ -1591,13 +1612,11 @@ static int ublk_ch_release(struct inode *inode, struct file *filp)
* All requests may be inflight, so ->canceling may not be set, set
* it now.
*/
- ub->canceling = true;
- 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);
- }
+ mutex_lock(&ub->cancel_mutex);
+ ublk_set_canceling(ub, true);
+ for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
+ ublk_abort_queue(ub, ublk_get_queue(ub, i));
+ mutex_unlock(&ub->cancel_mutex);
blk_mq_kick_requeue_list(disk->queue);
/*
@@ -1723,7 +1742,6 @@ 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);
- int i;
/* Our disk has been dead */
if (!disk)
@@ -1740,9 +1758,7 @@ static void ublk_start_cancel(struct ublk_device *ub)
* touch completed uring_cmd
*/
blk_mq_quiesce_queue(disk->queue);
- ub->canceling = true;
- for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
- ublk_get_queue(ub, i)->canceling = true;
+ ublk_set_canceling(ub, true);
blk_mq_unquiesce_queue(disk->queue);
out:
mutex_unlock(&ub->cancel_mutex);
@@ -1942,10 +1958,11 @@ static void ublk_reset_io_flags(struct ublk_device *ub)
for (j = 0; j < ubq->q_depth; j++)
ubq->ios[j].flags &= ~UBLK_IO_FLAG_CANCELED;
spin_unlock(&ubq->cancel_lock);
- ubq->canceling = false;
ubq->fail_io = false;
}
- ub->canceling = false;
+ mutex_lock(&ub->cancel_mutex);
+ ublk_set_canceling(ub, false);
+ mutex_unlock(&ub->cancel_mutex);
}
/* device can only be started after all IOs are ready */
@@ -3417,7 +3434,7 @@ static int ublk_ctrl_quiesce_dev(struct ublk_device *ub,
/* zero means wait forever */
u64 timeout_ms = header->data[0];
struct gendisk *disk;
- int i, ret = -ENODEV;
+ int ret = -ENODEV;
if (!(ub->dev_info.flags & UBLK_F_QUIESCE))
return -EOPNOTSUPP;
@@ -3435,14 +3452,11 @@ static int ublk_ctrl_quiesce_dev(struct ublk_device *ub,
goto put_disk;
/* Mark the device as canceling */
+ mutex_lock(&ub->cancel_mutex);
blk_mq_quiesce_queue(disk->queue);
- ub->canceling = true;
- for (i = 0; i < ub->dev_info.nr_hw_queues; i++) {
- struct ublk_queue *ubq = ublk_get_queue(ub, i);
-
- ubq->canceling = true;
- }
+ ublk_set_canceling(ub, true);
blk_mq_unquiesce_queue(disk->queue);
+ mutex_unlock(&ub->cancel_mutex);
if (!timeout_ms)
timeout_ms = UINT_MAX;
--
2.34.1On Fri, Jul 4, 2025 at 1:41 AM Uday Shankar <ushankar@purestorage.com> wrote:
>
> For performance reasons (minimizing the number of cache lines accessed
> in the hot path), we store the "canceling" state redundantly - there is
> one flag in the device, which can be considered the source of truth, and
> per-queue copies of that flag. This redundancy can cause confusion, and
> opens the door to bugs where the state is set inconsistently. Try to
> guard against these bugs by introducing a ublk_set_canceling helper
> which is the sole mutator of both the per-device and per-queue canceling
> state. This helper always sets the state consistently. Use the helper in
> all places where we need to modify the canceling state.
>
> No functional changes are expected.
>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
> drivers/block/ublk_drv.c | 54 ++++++++++++++++++++++++++++++------------------
> 1 file changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 870d57a853a481c2443337717c50d39355804f66..a1a700c7e67a72597e740aaa60f5c3c73f0085e5 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1563,6 +1563,27 @@ static void ublk_put_disk(struct gendisk *disk)
> put_device(disk_to_dev(disk));
> }
>
> +/*
> + * Use this function to ensure that ->canceling is consistently set for
> + * the device and all queues. Do not set these flags directly.
> + *
> + * Caller must ensure that:
> + * - cancel_mutex is held. This ensures that there is no concurrent
> + * access to ub->canceling and no concurrent writes to ubq->canceling.
> + * - there are no concurrent reads of ubq->canceling from the queue_rq
> + * path. This can be done by quiescing the queue, or through other
> + * means.
> + */
> +static void ublk_set_canceling(struct ublk_device *ub, bool canceling)
> + __must_hold(&ub->cancel_mutex)
> +{
> + int i;
> +
> + ub->canceling = canceling;
> + for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
> + ublk_get_queue(ub, i)->canceling = canceling;
> +}
> +
> static int ublk_ch_release(struct inode *inode, struct file *filp)
> {
> struct ublk_device *ub = filp->private_data;
> @@ -1591,13 +1612,11 @@ static int ublk_ch_release(struct inode *inode, struct file *filp)
> * All requests may be inflight, so ->canceling may not be set, set
> * it now.
> */
> - ub->canceling = true;
> - 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);
> - }
> + mutex_lock(&ub->cancel_mutex);
> + ublk_set_canceling(ub, true);
> + for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
> + ublk_abort_queue(ub, ublk_get_queue(ub, i));
> + mutex_unlock(&ub->cancel_mutex);
Pre-existing, but it doesn't look like the queue is quiesced yet here
(that happens later in this function).
Best,
Caleb
> blk_mq_kick_requeue_list(disk->queue);
>
> /*
> @@ -1723,7 +1742,6 @@ 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);
> - int i;
>
> /* Our disk has been dead */
> if (!disk)
> @@ -1740,9 +1758,7 @@ static void ublk_start_cancel(struct ublk_device *ub)
> * touch completed uring_cmd
> */
> blk_mq_quiesce_queue(disk->queue);
> - ub->canceling = true;
> - for (i = 0; i < ub->dev_info.nr_hw_queues; i++)
> - ublk_get_queue(ub, i)->canceling = true;
> + ublk_set_canceling(ub, true);
> blk_mq_unquiesce_queue(disk->queue);
> out:
> mutex_unlock(&ub->cancel_mutex);
> @@ -1942,10 +1958,11 @@ static void ublk_reset_io_flags(struct ublk_device *ub)
> for (j = 0; j < ubq->q_depth; j++)
> ubq->ios[j].flags &= ~UBLK_IO_FLAG_CANCELED;
> spin_unlock(&ubq->cancel_lock);
> - ubq->canceling = false;
> ubq->fail_io = false;
> }
> - ub->canceling = false;
> + mutex_lock(&ub->cancel_mutex);
> + ublk_set_canceling(ub, false);
> + mutex_unlock(&ub->cancel_mutex);
> }
>
> /* device can only be started after all IOs are ready */
> @@ -3417,7 +3434,7 @@ static int ublk_ctrl_quiesce_dev(struct ublk_device *ub,
> /* zero means wait forever */
> u64 timeout_ms = header->data[0];
> struct gendisk *disk;
> - int i, ret = -ENODEV;
> + int ret = -ENODEV;
>
> if (!(ub->dev_info.flags & UBLK_F_QUIESCE))
> return -EOPNOTSUPP;
> @@ -3435,14 +3452,11 @@ static int ublk_ctrl_quiesce_dev(struct ublk_device *ub,
> goto put_disk;
>
> /* Mark the device as canceling */
> + mutex_lock(&ub->cancel_mutex);
> blk_mq_quiesce_queue(disk->queue);
> - ub->canceling = true;
> - for (i = 0; i < ub->dev_info.nr_hw_queues; i++) {
> - struct ublk_queue *ubq = ublk_get_queue(ub, i);
> -
> - ubq->canceling = true;
> - }
> + ublk_set_canceling(ub, true);
> blk_mq_unquiesce_queue(disk->queue);
> + mutex_unlock(&ub->cancel_mutex);
>
> if (!timeout_ms)
> timeout_ms = UINT_MAX;
>
> --
> 2.34.1
>
On Thu, Jul 03, 2025 at 11:41:08PM -0600, Uday Shankar wrote: > For performance reasons (minimizing the number of cache lines accessed > in the hot path), we store the "canceling" state redundantly - there is > one flag in the device, which can be considered the source of truth, and > per-queue copies of that flag. This redundancy can cause confusion, and > opens the door to bugs where the state is set inconsistently. Try to > guard against these bugs by introducing a ublk_set_canceling helper > which is the sole mutator of both the per-device and per-queue canceling > state. This helper always sets the state consistently. Use the helper in > all places where we need to modify the canceling state. > > No functional changes are expected. > > Signed-off-by: Uday Shankar <ushankar@purestorage.com> Reviewed-by: Ming Lei <ming.lei@redhat.com> thanks, Ming
© 2016 - 2025 Red Hat, Inc.