* [PATCH 01/11] block: fix docs for freezing of queue limits updates
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 5:57 ` [PATCH 02/11] block: add a queue_limits_commit_update_frozen helper Christoph Hellwig
` (10 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
queue_limits_commit_update is the function that needs to operate on a
frozen queue, not queue_limits_start_update. Update the kerneldoc
comments to reflect that.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
block/blk-settings.c | 3 ++-
include/linux/blkdev.h | 3 +--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 8f09e33f41f6..89d8366fd43c 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -413,7 +413,8 @@ int blk_set_default_limits(struct queue_limits *lim)
* @lim: limits to apply
*
* Apply the limits in @lim that were obtained from queue_limits_start_update()
- * and updated by the caller to @q.
+ * and updated by the caller to @q. The caller must have frozen the queue or
+ * ensure that there are no outstanding I/Os by other means.
*
* Returns 0 if successful, else a negative error code.
*/
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5d40af2ef971..e781d4e6f92d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -944,8 +944,7 @@ static inline unsigned int blk_boundary_sectors_left(sector_t offset,
* the caller can modify. The caller must call queue_limits_commit_update()
* to finish the update.
*
- * Context: process context. The caller must have frozen the queue or ensured
- * that there is outstanding I/O by other means.
+ * Context: process context.
*/
static inline struct queue_limits
queue_limits_start_update(struct request_queue *q)
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 02/11] block: add a queue_limits_commit_update_frozen helper
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
2025-01-09 5:57 ` [PATCH 01/11] block: fix docs for freezing of queue limits updates Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 11:33 ` John Garry
2025-01-09 5:57 ` [PATCH 03/11] block: check BLK_FEAT_POLL under q_usage_count Christoph Hellwig
` (9 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
Add a helper that freezes the queue, updates the queue limits and
unfreezes the queue and convert all open coded versions of that to the
new helper.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
block/blk-integrity.c | 4 +---
block/blk-settings.c | 24 ++++++++++++++++++++++++
block/blk-zoned.c | 7 +------
drivers/block/virtio_blk.c | 4 +---
drivers/scsi/sd.c | 17 +++++------------
drivers/scsi/sr.c | 5 +----
include/linux/blkdev.h | 2 ++
7 files changed, 35 insertions(+), 28 deletions(-)
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index b180cac61a9d..013469faa5e7 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -218,9 +218,7 @@ static ssize_t flag_store(struct device *dev, const char *page, size_t count,
else
lim.integrity.flags |= flag;
- blk_mq_freeze_queue(q);
- err = queue_limits_commit_update(q, &lim);
- blk_mq_unfreeze_queue(q);
+ err = queue_limits_commit_update_frozen(q, &lim);
if (err)
return err;
return count;
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 89d8366fd43c..7c099d686dd8 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -444,6 +444,30 @@ int queue_limits_commit_update(struct request_queue *q,
}
EXPORT_SYMBOL_GPL(queue_limits_commit_update);
+/**
+ * queue_limits_commit_update_frozen - commit an atomic update of queue limits
+ * @q: queue to update
+ * @lim: limits to apply
+ *
+ * Apply the limits in @lim that were obtained from queue_limits_start_update()
+ * and updated by the caller to @q. Freezes the queue before the updated and
+ * unfreezes it after.
+ *
+ * Returns 0 if successful, else a negative error code.
+ */
+int queue_limits_commit_update_frozen(struct request_queue *q,
+ struct queue_limits *lim)
+{
+ int ret;
+
+ blk_mq_freeze_queue(q);
+ ret = queue_limits_commit_update(q, lim);
+ blk_mq_unfreeze_queue(q);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(queue_limits_commit_update_frozen);
+
/**
* queue_limits_set - apply queue limits to queue
* @q: queue to update
diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 4b0be40a8ea7..9d08a54c201e 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -1444,7 +1444,6 @@ static int disk_update_zone_resources(struct gendisk *disk,
unsigned int nr_seq_zones, nr_conv_zones;
unsigned int pool_size;
struct queue_limits lim;
- int ret;
disk->nr_zones = args->nr_zones;
disk->zone_capacity = args->zone_capacity;
@@ -1495,11 +1494,7 @@ static int disk_update_zone_resources(struct gendisk *disk,
}
commit:
- blk_mq_freeze_queue(q);
- ret = queue_limits_commit_update(q, &lim);
- blk_mq_unfreeze_queue(q);
-
- return ret;
+ return queue_limits_commit_update_frozen(q, &lim);
}
static int blk_revalidate_conv_zone(struct blk_zone *zone, unsigned int idx,
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 71a7ffeafb32..bbaa26b523b8 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -1105,9 +1105,7 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
lim.features |= BLK_FEAT_WRITE_CACHE;
else
lim.features &= ~BLK_FEAT_WRITE_CACHE;
- blk_mq_freeze_queue(disk->queue);
- i = queue_limits_commit_update(disk->queue, &lim);
- blk_mq_unfreeze_queue(disk->queue);
+ i = queue_limits_commit_update_frozen(disk->queue, &lim);
if (i)
return i;
return count;
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8947dab132d7..af62a8ed8620 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -177,9 +177,8 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
lim = queue_limits_start_update(sdkp->disk->queue);
sd_set_flush_flag(sdkp, &lim);
- blk_mq_freeze_queue(sdkp->disk->queue);
- ret = queue_limits_commit_update(sdkp->disk->queue, &lim);
- blk_mq_unfreeze_queue(sdkp->disk->queue);
+ ret = queue_limits_commit_update_frozen(sdkp->disk->queue,
+ &lim);
if (ret)
return ret;
return count;
@@ -483,9 +482,7 @@ provisioning_mode_store(struct device *dev, struct device_attribute *attr,
lim = queue_limits_start_update(sdkp->disk->queue);
sd_config_discard(sdkp, &lim, mode);
- blk_mq_freeze_queue(sdkp->disk->queue);
- err = queue_limits_commit_update(sdkp->disk->queue, &lim);
- blk_mq_unfreeze_queue(sdkp->disk->queue);
+ err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
if (err)
return err;
return count;
@@ -594,9 +591,7 @@ max_write_same_blocks_store(struct device *dev, struct device_attribute *attr,
lim = queue_limits_start_update(sdkp->disk->queue);
sd_config_write_same(sdkp, &lim);
- blk_mq_freeze_queue(sdkp->disk->queue);
- err = queue_limits_commit_update(sdkp->disk->queue, &lim);
- blk_mq_unfreeze_queue(sdkp->disk->queue);
+ err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
if (err)
return err;
return count;
@@ -3803,9 +3798,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
sd_config_write_same(sdkp, &lim);
kfree(buffer);
- blk_mq_freeze_queue(sdkp->disk->queue);
- err = queue_limits_commit_update(sdkp->disk->queue, &lim);
- blk_mq_unfreeze_queue(sdkp->disk->queue);
+ err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
if (err)
return err;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 198bec87bb8e..b17796d5ee66 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -797,10 +797,7 @@ static int get_sectorsize(struct scsi_cd *cd)
lim = queue_limits_start_update(q);
lim.logical_block_size = sector_size;
- blk_mq_freeze_queue(q);
- err = queue_limits_commit_update(q, &lim);
- blk_mq_unfreeze_queue(q);
- return err;
+ return queue_limits_commit_update_frozen(q, &lim);
}
static int get_capabilities(struct scsi_cd *cd)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e781d4e6f92d..13d353351c37 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -952,6 +952,8 @@ queue_limits_start_update(struct request_queue *q)
mutex_lock(&q->limits_lock);
return q->limits;
}
+int queue_limits_commit_update_frozen(struct request_queue *q,
+ struct queue_limits *lim);
int queue_limits_commit_update(struct request_queue *q,
struct queue_limits *lim);
int queue_limits_set(struct request_queue *q, struct queue_limits *lim);
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 02/11] block: add a queue_limits_commit_update_frozen helper
2025-01-09 5:57 ` [PATCH 02/11] block: add a queue_limits_commit_update_frozen helper Christoph Hellwig
@ 2025-01-09 11:33 ` John Garry
0 siblings, 0 replies; 22+ messages in thread
From: John Garry @ 2025-01-09 11:33 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
On 09/01/2025 05:57, Christoph Hellwig wrote:
> Add a helper that freezes the queue, updates the queue limits and
> unfreezes the queue and convert all open coded versions of that to the
> new helper.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Ming Lei <ming.lei@redhat.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Apart from some nitpicking on the code comments, FWIW:
Reviewed-by: John Garry <john.g.garry@oracle.com>
> ---
> block/blk-integrity.c | 4 +---
> block/blk-settings.c | 24 ++++++++++++++++++++++++
> block/blk-zoned.c | 7 +------
> drivers/block/virtio_blk.c | 4 +---
> drivers/scsi/sd.c | 17 +++++------------
> drivers/scsi/sr.c | 5 +----
> include/linux/blkdev.h | 2 ++
> 7 files changed, 35 insertions(+), 28 deletions(-)
>
> diff --git a/block/blk-integrity.c b/block/blk-integrity.c
> index b180cac61a9d..013469faa5e7 100644
> --- a/block/blk-integrity.c
> +++ b/block/blk-integrity.c
> @@ -218,9 +218,7 @@ static ssize_t flag_store(struct device *dev, const char *page, size_t count,
> else
> lim.integrity.flags |= flag;
>
> - blk_mq_freeze_queue(q);
> - err = queue_limits_commit_update(q, &lim);
> - blk_mq_unfreeze_queue(q);
> + err = queue_limits_commit_update_frozen(q, &lim);
> if (err)
> return err;
> return count;
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 89d8366fd43c..7c099d686dd8 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -444,6 +444,30 @@ int queue_limits_commit_update(struct request_queue *q,
> }
> EXPORT_SYMBOL_GPL(queue_limits_commit_update);
>
> +/**
> + * queue_limits_commit_update_frozen - commit an atomic update of queue limits
nit: this has same brief description of queue_limits_commit_update(),
but it does not do the same thing. I know that space is limited here for
the comment, though.
> + * @q: queue to update
> + * @lim: limits to apply
> + *
> + * Apply the limits in @lim that were obtained from queue_limits_start_update()
> + * and updated by the caller to @q.
"updated by the caller to @q" - this sounds odd
> Freezes the queue before the updated and
> + * unfreezes it after.
should that be "before the update"?
> + *
> + * Returns 0 if successful, else a negative error code.
> + */
> +int queue_limits_commit_update_frozen(struct request_queue *q,
> + struct queue_limits *lim)
> +{
> + int ret;
> +
> + blk_mq_freeze_queue(q);
> + ret = queue_limits_commit_update(q, lim);
> + blk_mq_unfreeze_queue(q);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(queue_limits_commit_update_frozen);
> +
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 03/11] block: check BLK_FEAT_POLL under q_usage_count
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
2025-01-09 5:57 ` [PATCH 01/11] block: fix docs for freezing of queue limits updates Christoph Hellwig
2025-01-09 5:57 ` [PATCH 02/11] block: add a queue_limits_commit_update_frozen helper Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 8:14 ` Ming Lei
2025-01-09 9:03 ` Nilay Shroff
2025-01-09 5:57 ` [PATCH 04/11] block: don't update BLK_FEAT_POLL in __blk_mq_update_nr_hw_queues Christoph Hellwig
` (8 subsequent siblings)
11 siblings, 2 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage
Otherwise feature reconfiguration can race with I/O submission.
Also drop the bio_clear_polled in the error path, as the flag does not
matter for instant error completions, it is a left over from when we
allowed polled I/O to proceed unpolled in this case.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-core.c | 22 ++++++++++++----------
block/blk-mq.c | 12 ++++++++++--
2 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 666efe8fa202..6309b3f5a89d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -629,8 +629,14 @@ static void __submit_bio(struct bio *bio)
blk_mq_submit_bio(bio);
} else if (likely(bio_queue_enter(bio) == 0)) {
struct gendisk *disk = bio->bi_bdev->bd_disk;
-
- disk->fops->submit_bio(bio);
+
+ if ((bio->bi_opf & REQ_POLLED) &&
+ !(disk->queue->limits.features & BLK_FEAT_POLL)) {
+ bio->bi_status = BLK_STS_NOTSUPP;
+ bio_endio(bio);
+ } else {
+ disk->fops->submit_bio(bio);
+ }
blk_queue_exit(disk->queue);
}
@@ -805,12 +811,6 @@ void submit_bio_noacct(struct bio *bio)
}
}
- if (!(q->limits.features & BLK_FEAT_POLL) &&
- (bio->bi_opf & REQ_POLLED)) {
- bio_clear_polled(bio);
- goto not_supported;
- }
-
switch (bio_op(bio)) {
case REQ_OP_READ:
break;
@@ -935,7 +935,7 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags)
return 0;
q = bdev_get_queue(bdev);
- if (cookie == BLK_QC_T_NONE || !(q->limits.features & BLK_FEAT_POLL))
+ if (cookie == BLK_QC_T_NONE)
return 0;
blk_flush_plug(current->plug, false);
@@ -951,7 +951,9 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags)
*/
if (!percpu_ref_tryget(&q->q_usage_counter))
return 0;
- if (queue_is_mq(q)) {
+ if (!(q->limits.features & BLK_FEAT_POLL)) {
+ ret = 0;
+ } else if (queue_is_mq(q)) {
ret = blk_mq_poll(q, cookie, iob, flags);
} else {
struct gendisk *disk = q->disk;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2e6132f778fd..02c9232a8fff 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3096,14 +3096,22 @@ void blk_mq_submit_bio(struct bio *bio)
}
/*
- * Device reconfiguration may change logical block size, so alignment
- * check has to be done with queue usage counter held
+ * Device reconfiguration may change logical block size or reduce the
+ * number of poll queues, so the checks for alignment and poll support
+ * have to be done with queue usage counter held.
*/
if (unlikely(bio_unaligned(bio, q))) {
bio_io_error(bio);
goto queue_exit;
}
+ if ((bio->bi_opf & REQ_POLLED) &&
+ !(q->limits.features & BLK_FEAT_POLL)) {
+ bio->bi_status = BLK_STS_NOTSUPP;
+ bio_endio(bio);
+ goto queue_exit;
+ }
+
bio = __bio_split_to_limits(bio, &q->limits, &nr_segs);
if (!bio)
goto queue_exit;
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 03/11] block: check BLK_FEAT_POLL under q_usage_count
2025-01-09 5:57 ` [PATCH 03/11] block: check BLK_FEAT_POLL under q_usage_count Christoph Hellwig
@ 2025-01-09 8:14 ` Ming Lei
2025-01-09 9:03 ` Nilay Shroff
1 sibling, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-01-09 8:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Damien Le Moal, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage
On Thu, Jan 09, 2025 at 06:57:24AM +0100, Christoph Hellwig wrote:
> Otherwise feature reconfiguration can race with I/O submission.
>
> Also drop the bio_clear_polled in the error path, as the flag does not
> matter for instant error completions, it is a left over from when we
> allowed polled I/O to proceed unpolled in this case.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 03/11] block: check BLK_FEAT_POLL under q_usage_count
2025-01-09 5:57 ` [PATCH 03/11] block: check BLK_FEAT_POLL under q_usage_count Christoph Hellwig
2025-01-09 8:14 ` Ming Lei
@ 2025-01-09 9:03 ` Nilay Shroff
1 sibling, 0 replies; 22+ messages in thread
From: Nilay Shroff @ 2025-01-09 9:03 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Damien Le Moal, Ming Lei, linux-block, linux-nvme, nbd,
linux-scsi, usb-storage
On 1/9/25 11:27 AM, Christoph Hellwig wrote:
> Otherwise feature reconfiguration can race with I/O submission.
>
> Also drop the bio_clear_polled in the error path, as the flag does not
> matter for instant error completions, it is a left over from when we
> allowed polled I/O to proceed unpolled in this case.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 04/11] block: don't update BLK_FEAT_POLL in __blk_mq_update_nr_hw_queues
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (2 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 03/11] block: check BLK_FEAT_POLL under q_usage_count Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 8:16 ` Ming Lei
2025-01-09 9:01 ` Nilay Shroff
2025-01-09 5:57 ` [PATCH 05/11] block: add a store_limit operations for sysfs entries Christoph Hellwig
` (7 subsequent siblings)
11 siblings, 2 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
When __blk_mq_update_nr_hw_queues changes the number of tag sets, it
might have to disable poll queues. Currently it does so by adjusting
the BLK_FEAT_POLL, which is a bit against the intent of features that
describe hardware / driver capabilities, but more importantly causes
nasty lock order problems with the broadly held freeze when updating the
number of hardware queues and the limits lock. Fix this by leaving
BLK_FEAT_POLL alone, and instead check for the number of poll queues in
the bio submission and poll handlers. While this adds extra work to the
fast path, the variables are in cache lines used by these operations
anyway, so it should be cheap enough.
Fixes: 8023e144f9d6 ("block: move the poll flag to queue_limits")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
block/blk-core.c | 7 +++----
block/blk-mq.c | 26 +++++---------------------
block/blk-mq.h | 6 ++++++
block/blk-sysfs.c | 9 ++++++++-
4 files changed, 22 insertions(+), 26 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 6309b3f5a89d..32fb28a6372c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -951,14 +951,13 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags)
*/
if (!percpu_ref_tryget(&q->q_usage_counter))
return 0;
- if (!(q->limits.features & BLK_FEAT_POLL)) {
- ret = 0;
- } else if (queue_is_mq(q)) {
+ if (queue_is_mq(q)) {
ret = blk_mq_poll(q, cookie, iob, flags);
} else {
struct gendisk *disk = q->disk;
- if (disk && disk->fops->poll_bio)
+ if ((q->limits.features & BLK_FEAT_POLL) && disk &&
+ disk->fops->poll_bio)
ret = disk->fops->poll_bio(bio, iob, flags);
}
blk_queue_exit(q);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 02c9232a8fff..655dcc16db76 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3105,8 +3105,7 @@ void blk_mq_submit_bio(struct bio *bio)
goto queue_exit;
}
- if ((bio->bi_opf & REQ_POLLED) &&
- !(q->limits.features & BLK_FEAT_POLL)) {
+ if ((bio->bi_opf & REQ_POLLED) && !blk_mq_can_poll(q)) {
bio->bi_status = BLK_STS_NOTSUPP;
bio_endio(bio);
goto queue_exit;
@@ -4328,12 +4327,6 @@ void blk_mq_release(struct request_queue *q)
blk_mq_sysfs_deinit(q);
}
-static bool blk_mq_can_poll(struct blk_mq_tag_set *set)
-{
- return set->nr_maps > HCTX_TYPE_POLL &&
- set->map[HCTX_TYPE_POLL].nr_queues;
-}
-
struct request_queue *blk_mq_alloc_queue(struct blk_mq_tag_set *set,
struct queue_limits *lim, void *queuedata)
{
@@ -4344,7 +4337,7 @@ struct request_queue *blk_mq_alloc_queue(struct blk_mq_tag_set *set,
if (!lim)
lim = &default_lim;
lim->features |= BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT;
- if (blk_mq_can_poll(set))
+ if (set->nr_maps > HCTX_TYPE_POLL)
lim->features |= BLK_FEAT_POLL;
q = blk_alloc_queue(lim, set->numa_node);
@@ -5032,8 +5025,6 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
fallback:
blk_mq_update_queue_map(set);
list_for_each_entry(q, &set->tag_list, tag_set_list) {
- struct queue_limits lim;
-
blk_mq_realloc_hw_ctxs(set, q);
if (q->nr_hw_queues != set->nr_hw_queues) {
@@ -5047,13 +5038,6 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
set->nr_hw_queues = prev_nr_hw_queues;
goto fallback;
}
- lim = queue_limits_start_update(q);
- if (blk_mq_can_poll(set))
- lim.features |= BLK_FEAT_POLL;
- else
- lim.features &= ~BLK_FEAT_POLL;
- if (queue_limits_commit_update(q, &lim) < 0)
- pr_warn("updating the poll flag failed\n");
blk_mq_map_swqueue(q);
}
@@ -5113,9 +5097,9 @@ static int blk_hctx_poll(struct request_queue *q, struct blk_mq_hw_ctx *hctx,
int blk_mq_poll(struct request_queue *q, blk_qc_t cookie,
struct io_comp_batch *iob, unsigned int flags)
{
- struct blk_mq_hw_ctx *hctx = xa_load(&q->hctx_table, cookie);
-
- return blk_hctx_poll(q, hctx, iob, flags);
+ if (!blk_mq_can_poll(q))
+ return 0;
+ return blk_hctx_poll(q, xa_load(&q->hctx_table, cookie), iob, flags);
}
int blk_rq_poll(struct request *rq, struct io_comp_batch *iob,
diff --git a/block/blk-mq.h b/block/blk-mq.h
index c872bbbe6411..44979e92b79f 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -448,4 +448,10 @@ do { \
#define blk_mq_run_dispatch_ops(q, dispatch_ops) \
__blk_mq_run_dispatch_ops(q, true, dispatch_ops) \
+static inline bool blk_mq_can_poll(struct request_queue *q)
+{
+ return (q->limits.features & BLK_FEAT_POLL) &&
+ q->tag_set->map[HCTX_TYPE_POLL].nr_queues;
+}
+
#endif
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 767598e719ab..e9f1c82b2f3e 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -245,10 +245,17 @@ static ssize_t queue_##_name##_show(struct gendisk *disk, char *page) \
!!(disk->queue->limits.features & _feature)); \
}
-QUEUE_SYSFS_FEATURE_SHOW(poll, BLK_FEAT_POLL);
QUEUE_SYSFS_FEATURE_SHOW(fua, BLK_FEAT_FUA);
QUEUE_SYSFS_FEATURE_SHOW(dax, BLK_FEAT_DAX);
+static ssize_t queue_poll_show(struct gendisk *disk, char *page)
+{
+ if (queue_is_mq(disk->queue))
+ return sysfs_emit(page, "%u\n", blk_mq_can_poll(disk->queue));
+ return sysfs_emit(page, "%u\n",
+ !!(disk->queue->limits.features & BLK_FEAT_POLL));
+}
+
static ssize_t queue_zoned_show(struct gendisk *disk, char *page)
{
if (blk_queue_is_zoned(disk->queue))
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 04/11] block: don't update BLK_FEAT_POLL in __blk_mq_update_nr_hw_queues
2025-01-09 5:57 ` [PATCH 04/11] block: don't update BLK_FEAT_POLL in __blk_mq_update_nr_hw_queues Christoph Hellwig
@ 2025-01-09 8:16 ` Ming Lei
2025-01-09 9:01 ` Nilay Shroff
1 sibling, 0 replies; 22+ messages in thread
From: Ming Lei @ 2025-01-09 8:16 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Damien Le Moal, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
On Thu, Jan 09, 2025 at 06:57:25AM +0100, Christoph Hellwig wrote:
> When __blk_mq_update_nr_hw_queues changes the number of tag sets, it
> might have to disable poll queues. Currently it does so by adjusting
> the BLK_FEAT_POLL, which is a bit against the intent of features that
> describe hardware / driver capabilities, but more importantly causes
> nasty lock order problems with the broadly held freeze when updating the
> number of hardware queues and the limits lock. Fix this by leaving
> BLK_FEAT_POLL alone, and instead check for the number of poll queues in
> the bio submission and poll handlers. While this adds extra work to the
> fast path, the variables are in cache lines used by these operations
> anyway, so it should be cheap enough.
>
> Fixes: 8023e144f9d6 ("block: move the poll flag to queue_limits")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Thanks,
Ming
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 04/11] block: don't update BLK_FEAT_POLL in __blk_mq_update_nr_hw_queues
2025-01-09 5:57 ` [PATCH 04/11] block: don't update BLK_FEAT_POLL in __blk_mq_update_nr_hw_queues Christoph Hellwig
2025-01-09 8:16 ` Ming Lei
@ 2025-01-09 9:01 ` Nilay Shroff
1 sibling, 0 replies; 22+ messages in thread
From: Nilay Shroff @ 2025-01-09 9:01 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Damien Le Moal, Ming Lei, linux-block, linux-nvme, nbd,
linux-scsi, usb-storage, Johannes Thumshirn
On 1/9/25 11:27 AM, Christoph Hellwig wrote:
> When __blk_mq_update_nr_hw_queues changes the number of tag sets, it
> might have to disable poll queues. Currently it does so by adjusting
> the BLK_FEAT_POLL, which is a bit against the intent of features that
> describe hardware / driver capabilities, but more importantly causes
> nasty lock order problems with the broadly held freeze when updating the
> number of hardware queues and the limits lock. Fix this by leaving
> BLK_FEAT_POLL alone, and instead check for the number of poll queues in
> the bio submission and poll handlers. While this adds extra work to the
> fast path, the variables are in cache lines used by these operations
> anyway, so it should be cheap enough.
>
> Fixes: 8023e144f9d6 ("block: move the poll flag to queue_limits")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Looks good to me, also with this new change in v3, I re-tested use case:
When a driver announces the support of polled I/O during initialization
and later resizing the queue so that there's no poll queue left. Both
these cases works fine. So:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 05/11] block: add a store_limit operations for sysfs entries
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (3 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 04/11] block: don't update BLK_FEAT_POLL in __blk_mq_update_nr_hw_queues Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 5:57 ` [PATCH 06/11] block: fix queue freeze vs limits lock order in sysfs store methods Christoph Hellwig
` (6 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
De-duplicate the code for updating queue limits by adding a store_limit
method that allows having common code handle the actual queue limits
update.
Note that this is a pure refactoring patch and does not address the
existing freeze vs limits lock order problem in the refactored code,
which will be addressed next.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
block/blk-sysfs.c | 128 ++++++++++++++++++++++------------------------
1 file changed, 61 insertions(+), 67 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e9f1c82b2f3e..d2aa2177e4ba 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -24,6 +24,8 @@ struct queue_sysfs_entry {
struct attribute attr;
ssize_t (*show)(struct gendisk *disk, char *page);
ssize_t (*store)(struct gendisk *disk, const char *page, size_t count);
+ int (*store_limit)(struct gendisk *disk, const char *page,
+ size_t count, struct queue_limits *lim);
void (*load_module)(struct gendisk *disk, const char *page, size_t count);
};
@@ -153,13 +155,11 @@ QUEUE_SYSFS_SHOW_CONST(discard_zeroes_data, 0)
QUEUE_SYSFS_SHOW_CONST(write_same_max, 0)
QUEUE_SYSFS_SHOW_CONST(poll_delay, -1)
-static ssize_t queue_max_discard_sectors_store(struct gendisk *disk,
- const char *page, size_t count)
+static int queue_max_discard_sectors_store(struct gendisk *disk,
+ const char *page, size_t count, struct queue_limits *lim)
{
unsigned long max_discard_bytes;
- struct queue_limits lim;
ssize_t ret;
- int err;
ret = queue_var_store(&max_discard_bytes, page, count);
if (ret < 0)
@@ -171,38 +171,28 @@ static ssize_t queue_max_discard_sectors_store(struct gendisk *disk,
if ((max_discard_bytes >> SECTOR_SHIFT) > UINT_MAX)
return -EINVAL;
- lim = queue_limits_start_update(disk->queue);
- lim.max_user_discard_sectors = max_discard_bytes >> SECTOR_SHIFT;
- err = queue_limits_commit_update(disk->queue, &lim);
- if (err)
- return err;
- return ret;
+ lim->max_user_discard_sectors = max_discard_bytes >> SECTOR_SHIFT;
+ return 0;
}
-static ssize_t
-queue_max_sectors_store(struct gendisk *disk, const char *page, size_t count)
+static int
+queue_max_sectors_store(struct gendisk *disk, const char *page, size_t count,
+ struct queue_limits *lim)
{
unsigned long max_sectors_kb;
- struct queue_limits lim;
ssize_t ret;
- int err;
ret = queue_var_store(&max_sectors_kb, page, count);
if (ret < 0)
return ret;
- lim = queue_limits_start_update(disk->queue);
- lim.max_user_sectors = max_sectors_kb << 1;
- err = queue_limits_commit_update(disk->queue, &lim);
- if (err)
- return err;
- return ret;
+ lim->max_user_sectors = max_sectors_kb << 1;
+ return 0;
}
static ssize_t queue_feature_store(struct gendisk *disk, const char *page,
- size_t count, blk_features_t feature)
+ size_t count, struct queue_limits *lim, blk_features_t feature)
{
- struct queue_limits lim;
unsigned long val;
ssize_t ret;
@@ -210,15 +200,11 @@ static ssize_t queue_feature_store(struct gendisk *disk, const char *page,
if (ret < 0)
return ret;
- lim = queue_limits_start_update(disk->queue);
if (val)
- lim.features |= feature;
+ lim->features |= feature;
else
- lim.features &= ~feature;
- ret = queue_limits_commit_update(disk->queue, &lim);
- if (ret)
- return ret;
- return count;
+ lim->features &= ~feature;
+ return 0;
}
#define QUEUE_SYSFS_FEATURE(_name, _feature) \
@@ -227,10 +213,10 @@ static ssize_t queue_##_name##_show(struct gendisk *disk, char *page) \
return sysfs_emit(page, "%u\n", \
!!(disk->queue->limits.features & _feature)); \
} \
-static ssize_t queue_##_name##_store(struct gendisk *disk, \
- const char *page, size_t count) \
+static int queue_##_name##_store(struct gendisk *disk, \
+ const char *page, size_t count, struct queue_limits *lim) \
{ \
- return queue_feature_store(disk, page, count, _feature); \
+ return queue_feature_store(disk, page, count, lim, _feature); \
}
QUEUE_SYSFS_FEATURE(rotational, BLK_FEAT_ROTATIONAL)
@@ -273,10 +259,9 @@ static ssize_t queue_iostats_passthrough_show(struct gendisk *disk, char *page)
return queue_var_show(!!blk_queue_passthrough_stat(disk->queue), page);
}
-static ssize_t queue_iostats_passthrough_store(struct gendisk *disk,
- const char *page, size_t count)
+static int queue_iostats_passthrough_store(struct gendisk *disk,
+ const char *page, size_t count, struct queue_limits *lim)
{
- struct queue_limits lim;
unsigned long ios;
ssize_t ret;
@@ -284,18 +269,13 @@ static ssize_t queue_iostats_passthrough_store(struct gendisk *disk,
if (ret < 0)
return ret;
- lim = queue_limits_start_update(disk->queue);
if (ios)
- lim.flags |= BLK_FLAG_IOSTATS_PASSTHROUGH;
+ lim->flags |= BLK_FLAG_IOSTATS_PASSTHROUGH;
else
- lim.flags &= ~BLK_FLAG_IOSTATS_PASSTHROUGH;
-
- ret = queue_limits_commit_update(disk->queue, &lim);
- if (ret)
- return ret;
-
- return count;
+ lim->flags &= ~BLK_FLAG_IOSTATS_PASSTHROUGH;
+ return 0;
}
+
static ssize_t queue_nomerges_show(struct gendisk *disk, char *page)
{
return queue_var_show((blk_queue_nomerges(disk->queue) << 1) |
@@ -398,12 +378,10 @@ static ssize_t queue_wc_show(struct gendisk *disk, char *page)
return sysfs_emit(page, "write through\n");
}
-static ssize_t queue_wc_store(struct gendisk *disk, const char *page,
- size_t count)
+static int queue_wc_store(struct gendisk *disk, const char *page,
+ size_t count, struct queue_limits *lim)
{
- struct queue_limits lim;
bool disable;
- int err;
if (!strncmp(page, "write back", 10)) {
disable = false;
@@ -414,15 +392,11 @@ static ssize_t queue_wc_store(struct gendisk *disk, const char *page,
return -EINVAL;
}
- lim = queue_limits_start_update(disk->queue);
if (disable)
- lim.flags |= BLK_FLAG_WRITE_CACHE_DISABLED;
+ lim->flags |= BLK_FLAG_WRITE_CACHE_DISABLED;
else
- lim.flags &= ~BLK_FLAG_WRITE_CACHE_DISABLED;
- err = queue_limits_commit_update(disk->queue, &lim);
- if (err)
- return err;
- return count;
+ lim->flags &= ~BLK_FLAG_WRITE_CACHE_DISABLED;
+ return 0;
}
#define QUEUE_RO_ENTRY(_prefix, _name) \
@@ -438,6 +412,13 @@ static struct queue_sysfs_entry _prefix##_entry = { \
.store = _prefix##_store, \
};
+#define QUEUE_LIM_RW_ENTRY(_prefix, _name) \
+static struct queue_sysfs_entry _prefix##_entry = { \
+ .attr = { .name = _name, .mode = 0644 }, \
+ .show = _prefix##_show, \
+ .store_limit = _prefix##_store, \
+}
+
#define QUEUE_RW_LOAD_MODULE_ENTRY(_prefix, _name) \
static struct queue_sysfs_entry _prefix##_entry = { \
.attr = { .name = _name, .mode = 0644 }, \
@@ -448,7 +429,7 @@ static struct queue_sysfs_entry _prefix##_entry = { \
QUEUE_RW_ENTRY(queue_requests, "nr_requests");
QUEUE_RW_ENTRY(queue_ra, "read_ahead_kb");
-QUEUE_RW_ENTRY(queue_max_sectors, "max_sectors_kb");
+QUEUE_LIM_RW_ENTRY(queue_max_sectors, "max_sectors_kb");
QUEUE_RO_ENTRY(queue_max_hw_sectors, "max_hw_sectors_kb");
QUEUE_RO_ENTRY(queue_max_segments, "max_segments");
QUEUE_RO_ENTRY(queue_max_integrity_segments, "max_integrity_segments");
@@ -464,7 +445,7 @@ QUEUE_RO_ENTRY(queue_io_opt, "optimal_io_size");
QUEUE_RO_ENTRY(queue_max_discard_segments, "max_discard_segments");
QUEUE_RO_ENTRY(queue_discard_granularity, "discard_granularity");
QUEUE_RO_ENTRY(queue_max_hw_discard_sectors, "discard_max_hw_bytes");
-QUEUE_RW_ENTRY(queue_max_discard_sectors, "discard_max_bytes");
+QUEUE_LIM_RW_ENTRY(queue_max_discard_sectors, "discard_max_bytes");
QUEUE_RO_ENTRY(queue_discard_zeroes_data, "discard_zeroes_data");
QUEUE_RO_ENTRY(queue_atomic_write_max_sectors, "atomic_write_max_bytes");
@@ -484,11 +465,11 @@ QUEUE_RO_ENTRY(queue_max_open_zones, "max_open_zones");
QUEUE_RO_ENTRY(queue_max_active_zones, "max_active_zones");
QUEUE_RW_ENTRY(queue_nomerges, "nomerges");
-QUEUE_RW_ENTRY(queue_iostats_passthrough, "iostats_passthrough");
+QUEUE_LIM_RW_ENTRY(queue_iostats_passthrough, "iostats_passthrough");
QUEUE_RW_ENTRY(queue_rq_affinity, "rq_affinity");
QUEUE_RW_ENTRY(queue_poll, "io_poll");
QUEUE_RW_ENTRY(queue_poll_delay, "io_poll_delay");
-QUEUE_RW_ENTRY(queue_wc, "write_cache");
+QUEUE_LIM_RW_ENTRY(queue_wc, "write_cache");
QUEUE_RO_ENTRY(queue_fua, "fua");
QUEUE_RO_ENTRY(queue_dax, "dax");
QUEUE_RW_ENTRY(queue_io_timeout, "io_timeout");
@@ -501,10 +482,10 @@ static struct queue_sysfs_entry queue_hw_sector_size_entry = {
.show = queue_logical_block_size_show,
};
-QUEUE_RW_ENTRY(queue_rotational, "rotational");
-QUEUE_RW_ENTRY(queue_iostats, "iostats");
-QUEUE_RW_ENTRY(queue_add_random, "add_random");
-QUEUE_RW_ENTRY(queue_stable_writes, "stable_writes");
+QUEUE_LIM_RW_ENTRY(queue_rotational, "rotational");
+QUEUE_LIM_RW_ENTRY(queue_iostats, "iostats");
+QUEUE_LIM_RW_ENTRY(queue_add_random, "add_random");
+QUEUE_LIM_RW_ENTRY(queue_stable_writes, "stable_writes");
#ifdef CONFIG_BLK_WBT
static ssize_t queue_var_store64(s64 *var, const char *page)
@@ -702,7 +683,7 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
struct request_queue *q = disk->queue;
ssize_t res;
- if (!entry->store)
+ if (!entry->store_limit && !entry->store)
return -EIO;
/*
@@ -713,11 +694,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
if (entry->load_module)
entry->load_module(disk, page, length);
- blk_mq_freeze_queue(q);
mutex_lock(&q->sysfs_lock);
- res = entry->store(disk, page, length);
- mutex_unlock(&q->sysfs_lock);
+ blk_mq_freeze_queue(q);
+ if (entry->store_limit) {
+ struct queue_limits lim = queue_limits_start_update(q);
+
+ res = entry->store_limit(disk, page, length, &lim);
+ if (res < 0) {
+ queue_limits_cancel_update(q);
+ } else {
+ res = queue_limits_commit_update(q, &lim);
+ if (!res)
+ res = length;
+ }
+ } else {
+ res = entry->store(disk, page, length);
+ }
blk_mq_unfreeze_queue(q);
+ mutex_unlock(&q->sysfs_lock);
return res;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 06/11] block: fix queue freeze vs limits lock order in sysfs store methods
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (4 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 05/11] block: add a store_limit operations for sysfs entries Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 13:07 ` John Garry
2025-01-09 5:57 ` [PATCH 07/11] nvme: fix queue freeze vs limits lock order Christoph Hellwig
` (5 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
queue_attr_store() always freezes a device queue before calling the
attribute store operation. For attributes that control queue limits, the
store operation will also lock the queue limits with a call to
queue_limits_start_update(). However, some drivers (e.g. SCSI sd) may
need to issue commands to a device to obtain limit values from the
hardware with the queue limits locked. This creates a potential ABBA
deadlock situation if a user attempts to modify a limit (thus freezing
the device queue) while the device driver starts a revalidation of the
device queue limits.
Avoid such deadlock by not freezing the queue before calling the
->store_limit() method in struct queue_sysfs_entry and instead use the
queue_limits_commit_update_frozen helper to freeze the queue after taking
the limits lock.
This also removes taking the sysfs lock for the store_limit method as
it doesn't protect anything here, but creates even more nesting.
Hopefully it will go away from the actual sysfs methods entirely soon.
(commit log adapted from a similar patch from Damien Le Moal)
Fixes: ff956a3be95b ("block: use queue_limits_commit_update in queue_discard_max_store")
Fixes: 0327ca9d53bf ("block: use queue_limits_commit_update in queue_max_sectors_store")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
block/blk-sysfs.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index d2aa2177e4ba..e828be777206 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -694,22 +694,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
if (entry->load_module)
entry->load_module(disk, page, length);
- mutex_lock(&q->sysfs_lock);
- blk_mq_freeze_queue(q);
if (entry->store_limit) {
struct queue_limits lim = queue_limits_start_update(q);
res = entry->store_limit(disk, page, length, &lim);
if (res < 0) {
queue_limits_cancel_update(q);
- } else {
- res = queue_limits_commit_update(q, &lim);
- if (!res)
- res = length;
+ return res;
}
- } else {
- res = entry->store(disk, page, length);
+
+ res = queue_limits_commit_update_frozen(q, &lim);
+ if (res)
+ return res;
+ return length;
}
+
+ mutex_lock(&q->sysfs_lock);
+ blk_mq_freeze_queue(q);
+ res = entry->store(disk, page, length);
blk_mq_unfreeze_queue(q);
mutex_unlock(&q->sysfs_lock);
return res;
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 06/11] block: fix queue freeze vs limits lock order in sysfs store methods
2025-01-09 5:57 ` [PATCH 06/11] block: fix queue freeze vs limits lock order in sysfs store methods Christoph Hellwig
@ 2025-01-09 13:07 ` John Garry
2025-01-10 9:33 ` Christoph Hellwig
0 siblings, 1 reply; 22+ messages in thread
From: John Garry @ 2025-01-09 13:07 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
On 09/01/2025 05:57, Christoph Hellwig wrote:
> queue_attr_store() always freezes a device queue before calling the
> attribute store operation. For attributes that control queue limits, the
> store operation will also lock the queue limits with a call to
> queue_limits_start_update(). However, some drivers (e.g. SCSI sd) may
> need to issue commands to a device to obtain limit values from the
> hardware with the queue limits locked. This creates a potential ABBA
> deadlock situation if a user attempts to modify a limit (thus freezing
> the device queue) while the device driver starts a revalidation of the
> device queue limits.
>
> Avoid such deadlock by not freezing the queue before calling the
> ->store_limit() method in struct queue_sysfs_entry and instead use the
> queue_limits_commit_update_frozen helper to freeze the queue after taking
> the limits lock.
>
> This also removes taking the sysfs lock for the store_limit method as
> it doesn't protect anything here, but creates even more nesting.
> Hopefully it will go away from the actual sysfs methods entirely soon.
Do you mean that the sysfs_lock could be removed in future? I would have
thought that queue limits lock could be used for the same thing, but I
am probably failing to see some lock nesting/ordering issues...
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 06/11] block: fix queue freeze vs limits lock order in sysfs store methods
2025-01-09 13:07 ` John Garry
@ 2025-01-10 9:33 ` Christoph Hellwig
0 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-10 9:33 UTC (permalink / raw)
To: John Garry
Cc: Christoph Hellwig, Jens Axboe, Damien Le Moal, Ming Lei,
Nilay Shroff, linux-block, linux-nvme, nbd, linux-scsi,
usb-storage, Johannes Thumshirn
On Thu, Jan 09, 2025 at 01:07:47PM +0000, John Garry wrote:
> Do you mean that the sysfs_lock could be removed in future? I would have
> thought that queue limits lock could be used for the same thing, but I am
> probably failing to see some lock nesting/ordering issues...
More or less. Think about it: what does it even try to protect?
Readіng/writing sysfs files vs itself and file removal it serialized by
sysfs/kernfs internally.
Any information tweaked in sysfs usually also has other places that can
modify it. So we'll need a lock independent of sysfs for that anyway.
A big part, buy by far all of that is covered by limits_lock.
Serializing creating/removing sysfs attribues is supposed to be
serialized using sysfs_dir_lock, although that needs a careful audit.
It's also used to serialize a few debugfs things, but we'll need to look
carefully for what exactly and switch that over to debugfs_mutex or
something new.
And then there's a bunch of misc cruft that also needs a careful look.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 07/11] nvme: fix queue freeze vs limits lock order
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (5 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 06/11] block: fix queue freeze vs limits lock order in sysfs store methods Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 5:57 ` [PATCH 08/11] nbd: " Christoph Hellwig
` (4 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
Match the locking order used by the core block code by only freezing
the queue after taking the limits lock.
Unlike most queue updates this does not use the
queue_limits_commit_update_frozen helper as the nvme driver want the
queue frozen for more than just the limits update.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
drivers/nvme/host/core.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2250ddef5a2..1ccf17f6ea7f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2128,9 +2128,10 @@ static int nvme_update_ns_info_generic(struct nvme_ns *ns,
struct queue_limits lim;
int ret;
- blk_mq_freeze_queue(ns->disk->queue);
lim = queue_limits_start_update(ns->disk->queue);
nvme_set_ctrl_limits(ns->ctrl, &lim);
+
+ blk_mq_freeze_queue(ns->disk->queue);
ret = queue_limits_commit_update(ns->disk->queue, &lim);
set_disk_ro(ns->disk, nvme_ns_is_readonly(ns, info));
blk_mq_unfreeze_queue(ns->disk->queue);
@@ -2177,12 +2178,12 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
goto out;
}
+ lim = queue_limits_start_update(ns->disk->queue);
+
blk_mq_freeze_queue(ns->disk->queue);
ns->head->lba_shift = id->lbaf[lbaf].ds;
ns->head->nuse = le64_to_cpu(id->nuse);
capacity = nvme_lba_to_sect(ns->head, le64_to_cpu(id->nsze));
-
- lim = queue_limits_start_update(ns->disk->queue);
nvme_set_ctrl_limits(ns->ctrl, &lim);
nvme_configure_metadata(ns->ctrl, ns->head, id, nvm, info);
nvme_set_chunk_sectors(ns, id, &lim);
@@ -2285,6 +2286,7 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
struct queue_limits *ns_lim = &ns->disk->queue->limits;
struct queue_limits lim;
+ lim = queue_limits_start_update(ns->head->disk->queue);
blk_mq_freeze_queue(ns->head->disk->queue);
/*
* queue_limits mixes values that are the hardware limitations
@@ -2301,7 +2303,6 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
* the splitting limits in to make sure we still obey possibly
* lower limitations of other controllers.
*/
- lim = queue_limits_start_update(ns->head->disk->queue);
lim.logical_block_size = ns_lim->logical_block_size;
lim.physical_block_size = ns_lim->physical_block_size;
lim.io_min = ns_lim->io_min;
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 08/11] nbd: fix queue freeze vs limits lock order
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (6 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 07/11] nvme: fix queue freeze vs limits lock order Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 5:57 ` [PATCH 09/11] usb-storage: " Christoph Hellwig
` (3 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
Match the locking order used by the core block code by only freezing
the queue after taking the limits lock using the
queue_limits_commit_update_frozen helper.
This also allows removes the need for the separate __nbd_set_size helper,
so remove it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
drivers/block/nbd.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 259bd57fc529..efa05c3c06bf 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -327,8 +327,7 @@ static void nbd_mark_nsock_dead(struct nbd_device *nbd, struct nbd_sock *nsock,
nsock->sent = 0;
}
-static int __nbd_set_size(struct nbd_device *nbd, loff_t bytesize,
- loff_t blksize)
+static int nbd_set_size(struct nbd_device *nbd, loff_t bytesize, loff_t blksize)
{
struct queue_limits lim;
int error;
@@ -368,7 +367,7 @@ static int __nbd_set_size(struct nbd_device *nbd, loff_t bytesize,
lim.logical_block_size = blksize;
lim.physical_block_size = blksize;
- error = queue_limits_commit_update(nbd->disk->queue, &lim);
+ error = queue_limits_commit_update_frozen(nbd->disk->queue, &lim);
if (error)
return error;
@@ -379,18 +378,6 @@ static int __nbd_set_size(struct nbd_device *nbd, loff_t bytesize,
return 0;
}
-static int nbd_set_size(struct nbd_device *nbd, loff_t bytesize,
- loff_t blksize)
-{
- int error;
-
- blk_mq_freeze_queue(nbd->disk->queue);
- error = __nbd_set_size(nbd, bytesize, blksize);
- blk_mq_unfreeze_queue(nbd->disk->queue);
-
- return error;
-}
-
static void nbd_complete_rq(struct request *req)
{
struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 09/11] usb-storage: fix queue freeze vs limits lock order
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (7 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 08/11] nbd: " Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 5:57 ` [PATCH 10/11] loop: refactor queue limits updates Christoph Hellwig
` (2 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
Match the locking order used by the core block code by only freezing
the queue after taking the limits lock using the
queue_limits_commit_update_frozen helper.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
drivers/usb/storage/scsiglue.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 8c8b5e6041cc..dc98ceecb724 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -592,12 +592,9 @@ static ssize_t max_sectors_store(struct device *dev, struct device_attribute *at
if (sscanf(buf, "%hu", &ms) <= 0)
return -EINVAL;
- blk_mq_freeze_queue(sdev->request_queue);
lim = queue_limits_start_update(sdev->request_queue);
lim.max_hw_sectors = ms;
- ret = queue_limits_commit_update(sdev->request_queue, &lim);
- blk_mq_unfreeze_queue(sdev->request_queue);
-
+ ret = queue_limits_commit_update_frozen(sdev->request_queue, &lim);
if (ret)
return ret;
return count;
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 10/11] loop: refactor queue limits updates
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (8 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 09/11] usb-storage: " Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 9:11 ` Nilay Shroff
2025-01-09 5:57 ` [PATCH 11/11] loop: fix queue freeze vs limits lock order Christoph Hellwig
2025-01-09 14:57 ` fix queue freeze and limit locking order v3 Martin K. Petersen
11 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
Replace loop_reconfigure_limits with a slightly less encompassing
loop_update_limits that expects the aller to acquire and commit the
queue limits to prepare for sorting out the freeze vs limits lock
ordering.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
drivers/block/loop.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 836a53eef4b4..560d6d5879d6 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -977,12 +977,12 @@ static unsigned int loop_default_blocksize(struct loop_device *lo,
return SECTOR_SIZE;
}
-static int loop_reconfigure_limits(struct loop_device *lo, unsigned int bsize)
+static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim,
+ unsigned int bsize)
{
struct file *file = lo->lo_backing_file;
struct inode *inode = file->f_mapping->host;
struct block_device *backing_bdev = NULL;
- struct queue_limits lim;
u32 granularity = 0, max_discard_sectors = 0;
if (S_ISBLK(inode->i_mode))
@@ -995,22 +995,20 @@ static int loop_reconfigure_limits(struct loop_device *lo, unsigned int bsize)
loop_get_discard_config(lo, &granularity, &max_discard_sectors);
- lim = queue_limits_start_update(lo->lo_queue);
- lim.logical_block_size = bsize;
- lim.physical_block_size = bsize;
- lim.io_min = bsize;
- lim.features &= ~(BLK_FEAT_WRITE_CACHE | BLK_FEAT_ROTATIONAL);
+ lim->logical_block_size = bsize;
+ lim->physical_block_size = bsize;
+ lim->io_min = bsize;
+ lim->features &= ~(BLK_FEAT_WRITE_CACHE | BLK_FEAT_ROTATIONAL);
if (file->f_op->fsync && !(lo->lo_flags & LO_FLAGS_READ_ONLY))
- lim.features |= BLK_FEAT_WRITE_CACHE;
+ lim->features |= BLK_FEAT_WRITE_CACHE;
if (backing_bdev && !bdev_nonrot(backing_bdev))
- lim.features |= BLK_FEAT_ROTATIONAL;
- lim.max_hw_discard_sectors = max_discard_sectors;
- lim.max_write_zeroes_sectors = max_discard_sectors;
+ lim->features |= BLK_FEAT_ROTATIONAL;
+ lim->max_hw_discard_sectors = max_discard_sectors;
+ lim->max_write_zeroes_sectors = max_discard_sectors;
if (max_discard_sectors)
- lim.discard_granularity = granularity;
+ lim->discard_granularity = granularity;
else
- lim.discard_granularity = 0;
- return queue_limits_commit_update(lo->lo_queue, &lim);
+ lim->discard_granularity = 0;
}
static int loop_configure(struct loop_device *lo, blk_mode_t mode,
@@ -1019,6 +1017,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
{
struct file *file = fget(config->fd);
struct address_space *mapping;
+ struct queue_limits lim;
int error;
loff_t size;
bool partscan;
@@ -1090,7 +1089,9 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
lo->old_gfp_mask = mapping_gfp_mask(mapping);
mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
- error = loop_reconfigure_limits(lo, config->block_size);
+ lim = queue_limits_start_update(lo->lo_queue);
+ loop_update_limits(lo, &lim, config->block_size);
+ error = queue_limits_commit_update(lo->lo_queue, &lim);
if (error)
goto out_unlock;
@@ -1458,6 +1459,7 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
{
+ struct queue_limits lim;
int err = 0;
if (lo->lo_state != Lo_bound)
@@ -1470,7 +1472,9 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
invalidate_bdev(lo->lo_device);
blk_mq_freeze_queue(lo->lo_queue);
- err = loop_reconfigure_limits(lo, arg);
+ lim = queue_limits_start_update(lo->lo_queue);
+ loop_update_limits(lo, &lim, arg);
+ err = queue_limits_commit_update(lo->lo_queue, &lim);
loop_update_dio(lo);
blk_mq_unfreeze_queue(lo->lo_queue);
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 10/11] loop: refactor queue limits updates
2025-01-09 5:57 ` [PATCH 10/11] loop: refactor queue limits updates Christoph Hellwig
@ 2025-01-09 9:11 ` Nilay Shroff
0 siblings, 0 replies; 22+ messages in thread
From: Nilay Shroff @ 2025-01-09 9:11 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Damien Le Moal, Ming Lei, linux-block, linux-nvme, nbd,
linux-scsi, usb-storage, Johannes Thumshirn
On 1/9/25 11:27 AM, Christoph Hellwig wrote:
> Replace loop_reconfigure_limits with a slightly less encompassing
> loop_update_limits that expects the aller to acquire and commit the
> queue limits to prepare for sorting out the freeze vs limits lock
> ordering.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Ming Lei <ming.lei@redhat.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Maybe a typo: s/aller/caller/g, otherwise looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 11/11] loop: fix queue freeze vs limits lock order
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (9 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 10/11] loop: refactor queue limits updates Christoph Hellwig
@ 2025-01-09 5:57 ` Christoph Hellwig
2025-01-09 14:57 ` fix queue freeze and limit locking order v3 Martin K. Petersen
11 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2025-01-09 5:57 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, Ming Lei, Nilay Shroff, linux-block, linux-nvme,
nbd, linux-scsi, usb-storage, Johannes Thumshirn
Match the locking order used by the core block code by only freezing
the queue after taking the limits lock using the
queue_limits_commit_update_frozen helper and document the callers that
do not freeze the queue at all.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
drivers/block/loop.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 560d6d5879d6..15e486baa223 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -311,6 +311,13 @@ static void loop_clear_limits(struct loop_device *lo, int mode)
lim.discard_granularity = 0;
}
+ /*
+ * XXX: this updates the queue limits without freezing the queue, which
+ * is against the locking protocol and dangerous. But we can't just
+ * freeze the queue as we're inside the ->queue_rq method here. So this
+ * should move out into a workqueue unless we get the file operations to
+ * advertise if they support specific fallocate operations.
+ */
queue_limits_commit_update(lo->lo_queue, &lim);
}
@@ -1091,6 +1098,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
lim = queue_limits_start_update(lo->lo_queue);
loop_update_limits(lo, &lim, config->block_size);
+ /* No need to freeze the queue as the device isn't bound yet. */
error = queue_limits_commit_update(lo->lo_queue, &lim);
if (error)
goto out_unlock;
@@ -1151,7 +1159,12 @@ static void __loop_clr_fd(struct loop_device *lo)
lo->lo_sizelimit = 0;
memset(lo->lo_file_name, 0, LO_NAME_SIZE);
- /* reset the block size to the default */
+ /*
+ * Reset the block size to the default.
+ *
+ * No queue freezing needed because this is called from the final
+ * ->release call only, so there can't be any outstanding I/O.
+ */
lim = queue_limits_start_update(lo->lo_queue);
lim.logical_block_size = SECTOR_SIZE;
lim.physical_block_size = SECTOR_SIZE;
@@ -1471,9 +1484,10 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
sync_blockdev(lo->lo_device);
invalidate_bdev(lo->lo_device);
- blk_mq_freeze_queue(lo->lo_queue);
lim = queue_limits_start_update(lo->lo_queue);
loop_update_limits(lo, &lim, arg);
+
+ blk_mq_freeze_queue(lo->lo_queue);
err = queue_limits_commit_update(lo->lo_queue, &lim);
loop_update_dio(lo);
blk_mq_unfreeze_queue(lo->lo_queue);
--
2.45.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: fix queue freeze and limit locking order v3
2025-01-09 5:57 fix queue freeze and limit locking order v3 Christoph Hellwig
` (10 preceding siblings ...)
2025-01-09 5:57 ` [PATCH 11/11] loop: fix queue freeze vs limits lock order Christoph Hellwig
@ 2025-01-09 14:57 ` Martin K. Petersen
11 siblings, 0 replies; 22+ messages in thread
From: Martin K. Petersen @ 2025-01-09 14:57 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Damien Le Moal, Ming Lei, Nilay Shroff, linux-block,
linux-nvme, nbd, linux-scsi, usb-storage
Christoph,
> this is my version of Damien's "Fix queue freeze and limit locking
> order". A lot looks very similar, but it was done independently based
> on the previous discussion.
With the minor editorial changes addressed:
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 22+ messages in thread