| Message ID | 20250107063120.1011593-7-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/8] block: fix docs for freezing of queue limits updates | expand |
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;
On Tue, Jan 07, 2025 at 12:28:29PM +0530, Nilay Shroff wrote: > > - 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(). Why?
On 1/7/25 1:52 PM, Christoph Hellwig wrote: > On Tue, Jan 07, 2025 at 12:28:29PM +0530, Nilay Shroff wrote: >>> - 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(). > > Why? > The nvme_set_ctrl_limits() sets certain queue limits which are also used during IO processing. For instance, ->max_segment_size is used while submitting bio. Also, if we look at the code before your patch, nvme_set_ctrl_limits() is called when the queue is freezed. Thanks, --Nilay
On Tue, Jan 07, 2025 at 02:15:05PM +0530, Nilay Shroff wrote: > The nvme_set_ctrl_limits() sets certain queue limits which are > also used during IO processing. For instance, ->max_segment_size > is used while submitting bio. nvme_set_ctrl_limits only sets them in the on-stack queue_limits structure, which is local to the calling thread.
On 1/7/25 2:28 PM, Christoph Hellwig wrote: > On Tue, Jan 07, 2025 at 02:15:05PM +0530, Nilay Shroff wrote: >> The nvme_set_ctrl_limits() sets certain queue limits which are >> also used during IO processing. For instance, ->max_segment_size >> is used while submitting bio. > > nvme_set_ctrl_limits only sets them in the on-stack queue_limits > structure, which is local to the calling thread. > > Oh yeah, I didn't notice it while reading patch. Agreed, we don't require freezing queue here. Sorry for the noise. Thanks, --Nilay
On 1/7/25 15:30, 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> Looks good to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Looks good to me.
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
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;
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(-)