* [PATCH v2] nvme: split nvme_update_zone_info @ 2024-04-02 14:47 ` Christoph Hellwig 2024-04-02 15:12 ` Keith Busch ` (3 more replies) 0 siblings, 4 replies; 7+ messages in thread From: Christoph Hellwig @ 2024-04-02 14:47 UTC (permalink / raw) To: kbusch, sagi; +Cc: linux-nvme, Kanchan Joshi nvme_update_zone_info does (admin queue) I/O to the device and can fail. We fail to abort the queue limits update if that happen, but really should avoid with the frozen I/O queue as much as possible anyway. Split the logic into a helper to query the information that can be called on an unfrozen queue and one to apply it to the queue limits. Fixes: 9b130d681443 ("nvme: use the atomic queue limits update API") Reported-by: Kanchan Joshi <joshi.k@samsung.com> Signed-off-by: Christoph Hellwig <hch@lst.de> --- Changes since v1: - make sure lbaf is actually initialized drivers/nvme/host/core.c | 19 +++++++++++-------- drivers/nvme/host/nvme.h | 12 ++++++++++-- drivers/nvme/host/zns.c | 33 ++++++++++++++++++++------------- 3 files changed, 41 insertions(+), 23 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 943d72bdd794ca..3b0498f320e6b9 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -2076,6 +2076,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns, bool vwc = ns->ctrl->vwc & NVME_CTRL_VWC_PRESENT; struct queue_limits lim; struct nvme_id_ns_nvm *nvm = NULL; + struct nvme_zone_info zi = {}; struct nvme_id_ns *id; sector_t capacity; unsigned lbaf; @@ -2091,6 +2092,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns, ret = -ENODEV; goto out; } + lbaf = nvme_lbaf_index(id->flbas); if (ns->ctrl->ctratt & NVME_CTRL_ATTR_ELBAS) { ret = nvme_identify_ns_nvm(ns->ctrl, info->nsid, &nvm); @@ -2098,8 +2100,14 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns, goto out; } + if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && + ns->head->ids.csi == NVME_CSI_ZNS) { + ret = nvme_query_zone_info(ns, lbaf, &zi); + if (ret < 0) + goto out; + } + blk_mq_freeze_queue(ns->disk->queue); - lbaf = nvme_lbaf_index(id->flbas); 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)); @@ -2112,13 +2120,8 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns, capacity = 0; nvme_config_discard(ns, &lim); if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && - ns->head->ids.csi == NVME_CSI_ZNS) { - ret = nvme_update_zone_info(ns, lbaf, &lim); - if (ret) { - blk_mq_unfreeze_queue(ns->disk->queue); - goto out; - } - } + ns->head->ids.csi == NVME_CSI_ZNS) + nvme_update_zone_info(ns, &lim, &zi); ret = queue_limits_commit_update(ns->disk->queue, &lim); if (ret) { blk_mq_unfreeze_queue(ns->disk->queue); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 24193fcb8bd584..d0ed64dc7380e5 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -1036,10 +1036,18 @@ static inline bool nvme_disk_is_ns_head(struct gendisk *disk) } #endif /* CONFIG_NVME_MULTIPATH */ +struct nvme_zone_info { + u64 zone_size; + unsigned int max_open_zones; + unsigned int max_active_zones; +}; + int nvme_ns_report_zones(struct nvme_ns *ns, sector_t sector, unsigned int nr_zones, report_zones_cb cb, void *data); -int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf, - struct queue_limits *lim); +int nvme_query_zone_info(struct nvme_ns *ns, unsigned lbaf, + struct nvme_zone_info *zi); +void nvme_update_zone_info(struct nvme_ns *ns, struct queue_limits *lim, + struct nvme_zone_info *zi); #ifdef CONFIG_BLK_DEV_ZONED blk_status_t nvme_setup_zone_mgmt_send(struct nvme_ns *ns, struct request *req, struct nvme_command *cmnd, diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c index 722384bcc765cd..77aa0f440a6d2a 100644 --- a/drivers/nvme/host/zns.c +++ b/drivers/nvme/host/zns.c @@ -35,8 +35,8 @@ static int nvme_set_max_append(struct nvme_ctrl *ctrl) return 0; } -int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf, - struct queue_limits *lim) +int nvme_query_zone_info(struct nvme_ns *ns, unsigned lbaf, + struct nvme_zone_info *zi) { struct nvme_effects_log *log = ns->head->effects; struct nvme_command c = { }; @@ -89,27 +89,34 @@ int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf, goto free_data; } - ns->head->zsze = - nvme_lba_to_sect(ns->head, le64_to_cpu(id->lbafe[lbaf].zsze)); - if (!is_power_of_2(ns->head->zsze)) { + zi->zone_size = le64_to_cpu(id->lbafe[lbaf].zsze); + if (!is_power_of_2(zi->zone_size)) { dev_warn(ns->ctrl->device, - "invalid zone size:%llu for namespace:%u\n", - ns->head->zsze, ns->head->ns_id); + "invalid zone size: %llu for namespace: %u\n", + zi->zone_size, ns->head->ns_id); status = -ENODEV; goto free_data; } + zi->max_open_zones = le32_to_cpu(id->mor) + 1; + zi->max_active_zones = le32_to_cpu(id->mar) + 1; - blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, ns->queue); - lim->zoned = 1; - lim->max_open_zones = le32_to_cpu(id->mor) + 1; - lim->max_active_zones = le32_to_cpu(id->mar) + 1; - lim->chunk_sectors = ns->head->zsze; - lim->max_zone_append_sectors = ns->ctrl->max_zone_append; free_data: kfree(id); return status; } +void nvme_update_zone_info(struct nvme_ns *ns, struct queue_limits *lim, + struct nvme_zone_info *zi) +{ + lim->zoned = 1; + lim->max_open_zones = zi->max_open_zones; + lim->max_active_zones = zi->max_active_zones; + lim->max_zone_append_sectors = ns->ctrl->max_zone_append; + lim->chunk_sectors = ns->head->zsze = + nvme_lba_to_sect(ns->head, zi->zone_size); + blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, ns->queue); +} + static void *nvme_zns_alloc_report_buffer(struct nvme_ns *ns, unsigned int nr_zones, size_t *buflen) { -- 2.39.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: split nvme_update_zone_info 2024-04-02 14:47 ` [PATCH v2] nvme: split nvme_update_zone_info Christoph Hellwig @ 2024-04-02 15:12 ` Keith Busch 2024-04-02 15:13 ` Kanchan Joshi ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ messages in thread From: Keith Busch @ 2024-04-02 15:12 UTC (permalink / raw) To: Christoph Hellwig; +Cc: sagi, linux-nvme, Kanchan Joshi On Tue, Apr 02, 2024 at 04:47:54PM +0200, Christoph Hellwig wrote: > nvme_update_zone_info does (admin queue) I/O to the device and can fail. > We fail to abort the queue limits update if that happen, but really > should avoid with the frozen I/O queue as much as possible anyway. > > Split the logic into a helper to query the information that can be > called on an unfrozen queue and one to apply it to the queue limits. > > Fixes: 9b130d681443 ("nvme: use the atomic queue limits update API") > Reported-by: Kanchan Joshi <joshi.k@samsung.com> > Signed-off-by: Christoph Hellwig <hch@lst.de> Thanks, applied to nvme-6.9. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: split nvme_update_zone_info 2024-04-02 14:47 ` [PATCH v2] nvme: split nvme_update_zone_info Christoph Hellwig 2024-04-02 15:12 ` Keith Busch @ 2024-04-02 15:13 ` Kanchan Joshi 2024-04-02 19:58 ` Keith Busch 2024-04-03 3:14 ` Damien Le Moal 3 siblings, 0 replies; 7+ messages in thread From: Kanchan Joshi @ 2024-04-02 15:13 UTC (permalink / raw) To: Christoph Hellwig, kbusch, sagi; +Cc: linux-nvme On 4/2/2024 8:17 PM, Christoph Hellwig wrote: > nvme_update_zone_info does (admin queue) I/O to the device and can fail. > We fail to abort the queue limits update if that happen, but really > should avoid with the frozen I/O queue as much as possible anyway. > > Split the logic into a helper to query the information that can be > called on an unfrozen queue and one to apply it to the queue limits. > > Fixes: 9b130d681443 ("nvme: use the atomic queue limits update API") > Reported-by: Kanchan Joshi<joshi.k@samsung.com> > Signed-off-by: Christoph Hellwig<hch@lst.de> > --- Looks good. Reviewed-by: Kanchan Joshi <joshi.k@samsung.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: split nvme_update_zone_info 2024-04-02 14:47 ` [PATCH v2] nvme: split nvme_update_zone_info Christoph Hellwig 2024-04-02 15:12 ` Keith Busch 2024-04-02 15:13 ` Kanchan Joshi @ 2024-04-02 19:58 ` Keith Busch 2024-04-03 3:40 ` Christoph Hellwig 2024-04-03 3:14 ` Damien Le Moal 3 siblings, 1 reply; 7+ messages in thread From: Keith Busch @ 2024-04-02 19:58 UTC (permalink / raw) To: Christoph Hellwig; +Cc: sagi, linux-nvme, Kanchan Joshi On Tue, Apr 02, 2024 at 04:47:54PM +0200, Christoph Hellwig wrote: > + lim->chunk_sectors = ns->head->zsze = > + nvme_lba_to_sect(ns->head, zi->zone_size); Hey, since chuck_sectors is only 32 bits and zone_size is 64, do we need to worry about massive zones breaking this limit? Unlikely as we are to see a multi-terabyte zone, it's easy enough to check. We never have before though, so I guess we're confident it won't happen? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: split nvme_update_zone_info 2024-04-02 19:58 ` Keith Busch @ 2024-04-03 3:40 ` Christoph Hellwig 0 siblings, 0 replies; 7+ messages in thread From: Christoph Hellwig @ 2024-04-03 3:40 UTC (permalink / raw) To: Keith Busch; +Cc: Christoph Hellwig, sagi, linux-nvme, Kanchan Joshi On Tue, Apr 02, 2024 at 01:58:25PM -0600, Keith Busch wrote: > On Tue, Apr 02, 2024 at 04:47:54PM +0200, Christoph Hellwig wrote: > > + lim->chunk_sectors = ns->head->zsze = > > + nvme_lba_to_sect(ns->head, zi->zone_size); > > Hey, since chuck_sectors is only 32 bits and zone_size is 64, do we need > to worry about massive zones breaking this limit? Unlikely as we are to > see a multi-terabyte zone, it's easy enough to check. We never have > before though, so I guess we're confident it won't happen? Reasonable confident. Adding an overflow check probably would not be a bad idea. I can look into a patch for that. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: split nvme_update_zone_info 2024-04-02 14:47 ` [PATCH v2] nvme: split nvme_update_zone_info Christoph Hellwig ` (2 preceding siblings ...) 2024-04-02 19:58 ` Keith Busch @ 2024-04-03 3:14 ` Damien Le Moal 2024-04-03 3:40 ` Christoph Hellwig 3 siblings, 1 reply; 7+ messages in thread From: Damien Le Moal @ 2024-04-03 3:14 UTC (permalink / raw) To: Christoph Hellwig, kbusch, sagi; +Cc: linux-nvme, Kanchan Joshi On 4/2/24 23:47, Christoph Hellwig wrote: > nvme_update_zone_info does (admin queue) I/O to the device and can fail. > We fail to abort the queue limits update if that happen, but really > should avoid with the frozen I/O queue as much as possible anyway. > > Split the logic into a helper to query the information that can be > called on an unfrozen queue and one to apply it to the queue limits. > > Fixes: 9b130d681443 ("nvme: use the atomic queue limits update API") > Reported-by: Kanchan Joshi <joshi.k@samsung.com> > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > > Changes since v1: > - make sure lbaf is actually initialized > > drivers/nvme/host/core.c | 19 +++++++++++-------- > drivers/nvme/host/nvme.h | 12 ++++++++++-- > drivers/nvme/host/zns.c | 33 ++++++++++++++++++++------------- > 3 files changed, 41 insertions(+), 23 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 943d72bdd794ca..3b0498f320e6b9 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -2076,6 +2076,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns, > bool vwc = ns->ctrl->vwc & NVME_CTRL_VWC_PRESENT; > struct queue_limits lim; > struct nvme_id_ns_nvm *nvm = NULL; > + struct nvme_zone_info zi = {}; > struct nvme_id_ns *id; > sector_t capacity; > unsigned lbaf; > @@ -2091,6 +2092,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns, > ret = -ENODEV; > goto out; > } > + lbaf = nvme_lbaf_index(id->flbas); > > if (ns->ctrl->ctratt & NVME_CTRL_ATTR_ELBAS) { > ret = nvme_identify_ns_nvm(ns->ctrl, info->nsid, &nvm); > @@ -2098,8 +2100,14 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns, > goto out; > } > > + if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && > + ns->head->ids.csi == NVME_CSI_ZNS) { > + ret = nvme_query_zone_info(ns, lbaf, &zi); > + if (ret < 0) > + goto out; > + } > + > blk_mq_freeze_queue(ns->disk->queue); > - lbaf = nvme_lbaf_index(id->flbas); > 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)); > @@ -2112,13 +2120,8 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns, > capacity = 0; > nvme_config_discard(ns, &lim); > if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && > - ns->head->ids.csi == NVME_CSI_ZNS) { > - ret = nvme_update_zone_info(ns, lbaf, &lim); > - if (ret) { > - blk_mq_unfreeze_queue(ns->disk->queue); > - goto out; > - } > - } > + ns->head->ids.csi == NVME_CSI_ZNS) > + nvme_update_zone_info(ns, &lim, &zi); > ret = queue_limits_commit_update(ns->disk->queue, &lim); > if (ret) { > blk_mq_unfreeze_queue(ns->disk->queue); > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index 24193fcb8bd584..d0ed64dc7380e5 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -1036,10 +1036,18 @@ static inline bool nvme_disk_is_ns_head(struct gendisk *disk) > } > #endif /* CONFIG_NVME_MULTIPATH */ > > +struct nvme_zone_info { > + u64 zone_size; > + unsigned int max_open_zones; > + unsigned int max_active_zones; > +}; > + > int nvme_ns_report_zones(struct nvme_ns *ns, sector_t sector, > unsigned int nr_zones, report_zones_cb cb, void *data); > -int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf, > - struct queue_limits *lim); > +int nvme_query_zone_info(struct nvme_ns *ns, unsigned lbaf, > + struct nvme_zone_info *zi); > +void nvme_update_zone_info(struct nvme_ns *ns, struct queue_limits *lim, > + struct nvme_zone_info *zi); > #ifdef CONFIG_BLK_DEV_ZONED > blk_status_t nvme_setup_zone_mgmt_send(struct nvme_ns *ns, struct request *req, > struct nvme_command *cmnd, > diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c > index 722384bcc765cd..77aa0f440a6d2a 100644 > --- a/drivers/nvme/host/zns.c > +++ b/drivers/nvme/host/zns.c > @@ -35,8 +35,8 @@ static int nvme_set_max_append(struct nvme_ctrl *ctrl) > return 0; > } > > -int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf, > - struct queue_limits *lim) > +int nvme_query_zone_info(struct nvme_ns *ns, unsigned lbaf, > + struct nvme_zone_info *zi) > { > struct nvme_effects_log *log = ns->head->effects; > struct nvme_command c = { }; > @@ -89,27 +89,34 @@ int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf, > goto free_data; > } > > - ns->head->zsze = > - nvme_lba_to_sect(ns->head, le64_to_cpu(id->lbafe[lbaf].zsze)); > - if (!is_power_of_2(ns->head->zsze)) { > + zi->zone_size = le64_to_cpu(id->lbafe[lbaf].zsze); > + if (!is_power_of_2(zi->zone_size)) { > dev_warn(ns->ctrl->device, > - "invalid zone size:%llu for namespace:%u\n", > - ns->head->zsze, ns->head->ns_id); > + "invalid zone size: %llu for namespace: %u\n", > + zi->zone_size, ns->head->ns_id); > status = -ENODEV; > goto free_data; > } > + zi->max_open_zones = le32_to_cpu(id->mor) + 1; > + zi->max_active_zones = le32_to_cpu(id->mar) + 1; > > - blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, ns->queue); > - lim->zoned = 1; > - lim->max_open_zones = le32_to_cpu(id->mor) + 1; > - lim->max_active_zones = le32_to_cpu(id->mar) + 1; > - lim->chunk_sectors = ns->head->zsze; > - lim->max_zone_append_sectors = ns->ctrl->max_zone_append; > free_data: > kfree(id); > return status; > } > > +void nvme_update_zone_info(struct nvme_ns *ns, struct queue_limits *lim, > + struct nvme_zone_info *zi) > +{ > + lim->zoned = 1; Nit: "= true" would be better given that this limit is a bool. > + lim->max_open_zones = zi->max_open_zones; > + lim->max_active_zones = zi->max_active_zones; > + lim->max_zone_append_sectors = ns->ctrl->max_zone_append; > + lim->chunk_sectors = ns->head->zsze = > + nvme_lba_to_sect(ns->head, zi->zone_size); > + blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, ns->queue); > +} > + > static void *nvme_zns_alloc_report_buffer(struct nvme_ns *ns, > unsigned int nr_zones, size_t *buflen) > { -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: split nvme_update_zone_info 2024-04-03 3:14 ` Damien Le Moal @ 2024-04-03 3:40 ` Christoph Hellwig 0 siblings, 0 replies; 7+ messages in thread From: Christoph Hellwig @ 2024-04-03 3:40 UTC (permalink / raw) To: Damien Le Moal; +Cc: Christoph Hellwig, kbusch, sagi, linux-nvme, Kanchan Joshi On Wed, Apr 03, 2024 at 12:14:02PM +0900, Damien Le Moal wrote: > > +{ > > + lim->zoned = 1; > > Nit: "= true" would be better given that this limit is a bool. true :) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-03 3:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20240402144830epcas5p35bdc555afe12fc8eb52a5c8fe0894c38@epcas5p3.samsung.com>
2024-04-02 14:47 ` [PATCH v2] nvme: split nvme_update_zone_info Christoph Hellwig
2024-04-02 15:12 ` Keith Busch
2024-04-02 15:13 ` Kanchan Joshi
2024-04-02 19:58 ` Keith Busch
2024-04-03 3:40 ` Christoph Hellwig
2024-04-03 3:14 ` Damien Le Moal
2024-04-03 3:40 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.