Re: [PATCH 6/8] nvme: fix queue freeze vs limits lock order

Nilay Shroff
2025-01-07T06:58:43Z

On 1/7/25 12:00 PM, Christoph Hellwig wrote:
> 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>
> ---
> 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);

I think we should freeze queue before nvme_set_ctrl_limits().

> @@ -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;