| Message ID | 20230707094616.108430-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] block: don't unconditionally set max_discard_sectors in blk_queue_max_discard_sectors | expand |
On 7/7/23 18:46, Christoph Hellwig wrote: > ctrl->max_discard_sectors stores a value that is potentially based of > the DMRSL field in Identify Controller, which is in units of LBAs and > thus dependent on the Format of a namespace. > > Fix this by moving the calculation of max_discard_sectors entirely > into nvme_config_discard and replacing the ctrl->max_discard_sectors > value with a local variable so that the calculation is always I do not see a local variable replacement... May be you meant direct calls to blk_queue_max_discard_sectors() ? Other than that, looks OK to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > namespace-specific. > > Fixes: 1a86924e4f46 ("nvme: fix interpretation of DMRSL") > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/nvme/host/core.c | 22 ++++++++++------------ > drivers/nvme/host/nvme.h | 1 - > 2 files changed, 10 insertions(+), 13 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 2d6c1f4ad7f5c8..05372bec3b7aff 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1721,20 +1721,21 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns) > struct request_queue *queue = disk->queue; > u32 size = queue_logical_block_size(queue); > > - if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns, UINT_MAX)) > - ctrl->max_discard_sectors = nvme_lba_to_sect(ns, ctrl->dmrsl); > + BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) < > + NVME_DSM_MAX_RANGES); > > - if (ctrl->max_discard_sectors == 0) { > + if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns, UINT_MAX)) { > + blk_queue_max_discard_sectors(queue, > + nvme_lba_to_sect(ns, ctrl->dmrsl)); > + } else if (ctrl->oncs & NVME_CTRL_ONCS_DSM) { > + blk_queue_max_discard_sectors(queue, UINT_MAX); > + } else { > blk_queue_max_discard_sectors(queue, 0); > return; > } > > - BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) < > - NVME_DSM_MAX_RANGES); > - > queue->limits.discard_granularity = size; > > - blk_queue_max_discard_sectors(queue, ctrl->max_discard_sectors); > blk_queue_max_discard_segments(queue, ctrl->max_discard_segments); > > if (ctrl->quirks & NVME_QUIRK_DEALLOCATE_ZEROES) > @@ -2870,13 +2871,10 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl) > struct nvme_id_ctrl_nvm *id; > int ret; > > - if (ctrl->oncs & NVME_CTRL_ONCS_DSM) { > - ctrl->max_discard_sectors = UINT_MAX; > + if (ctrl->oncs & NVME_CTRL_ONCS_DSM) > ctrl->max_discard_segments = NVME_DSM_MAX_RANGES; > - } else { > - ctrl->max_discard_sectors = 0; > + else > ctrl->max_discard_segments = 0; > - } > > /* > * Even though NVMe spec explicitly states that MDTS is not applicable > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index f35647c470afad..d59ed2ba1c37ca 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -296,7 +296,6 @@ struct nvme_ctrl { > u32 max_hw_sectors; > u32 max_segments; > u32 max_integrity_segments; > - u32 max_discard_sectors; > u32 max_discard_segments; > u32 max_zeroes_sectors; > #ifdef CONFIG_BLK_DEV_ZONED
On Mon, Jul 10, 2023 at 12:57:56PM +0900, Damien Le Moal wrote: > On 7/7/23 18:46, Christoph Hellwig wrote: > > ctrl->max_discard_sectors stores a value that is potentially based of > > the DMRSL field in Identify Controller, which is in units of LBAs and > > thus dependent on the Format of a namespace. > > > > Fix this by moving the calculation of max_discard_sectors entirely > > into nvme_config_discard and replacing the ctrl->max_discard_sectors > > value with a local variable so that the calculation is always > > I do not see a local variable replacement... May be you meant direct calls to > blk_queue_max_discard_sectors() ? Yeah, I used a local variable first, but then noticed they are pointless as we can just call blk_queue_max_discard_sectors directly and didn't update the commit log.
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 2d6c1f4ad7f5c8..05372bec3b7aff 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1721,20 +1721,21 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns) struct request_queue *queue = disk->queue; u32 size = queue_logical_block_size(queue); - if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns, UINT_MAX)) - ctrl->max_discard_sectors = nvme_lba_to_sect(ns, ctrl->dmrsl); + BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) < + NVME_DSM_MAX_RANGES); - if (ctrl->max_discard_sectors == 0) { + if (ctrl->dmrsl && ctrl->dmrsl <= nvme_sect_to_lba(ns, UINT_MAX)) { + blk_queue_max_discard_sectors(queue, + nvme_lba_to_sect(ns, ctrl->dmrsl)); + } else if (ctrl->oncs & NVME_CTRL_ONCS_DSM) { + blk_queue_max_discard_sectors(queue, UINT_MAX); + } else { blk_queue_max_discard_sectors(queue, 0); return; } - BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) < - NVME_DSM_MAX_RANGES); - queue->limits.discard_granularity = size; - blk_queue_max_discard_sectors(queue, ctrl->max_discard_sectors); blk_queue_max_discard_segments(queue, ctrl->max_discard_segments); if (ctrl->quirks & NVME_QUIRK_DEALLOCATE_ZEROES) @@ -2870,13 +2871,10 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl) struct nvme_id_ctrl_nvm *id; int ret; - if (ctrl->oncs & NVME_CTRL_ONCS_DSM) { - ctrl->max_discard_sectors = UINT_MAX; + if (ctrl->oncs & NVME_CTRL_ONCS_DSM) ctrl->max_discard_segments = NVME_DSM_MAX_RANGES; - } else { - ctrl->max_discard_sectors = 0; + else ctrl->max_discard_segments = 0; - } /* * Even though NVMe spec explicitly states that MDTS is not applicable diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index f35647c470afad..d59ed2ba1c37ca 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -296,7 +296,6 @@ struct nvme_ctrl { u32 max_hw_sectors; u32 max_segments; u32 max_integrity_segments; - u32 max_discard_sectors; u32 max_discard_segments; u32 max_zeroes_sectors; #ifdef CONFIG_BLK_DEV_ZONED
ctrl->max_discard_sectors stores a value that is potentially based of the DMRSL field in Identify Controller, which is in units of LBAs and thus dependent on the Format of a namespace. Fix this by moving the calculation of max_discard_sectors entirely into nvme_config_discard and replacing the ctrl->max_discard_sectors value with a local variable so that the calculation is always namespace-specific. Fixes: 1a86924e4f46 ("nvme: fix interpretation of DMRSL") Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/nvme/host/core.c | 22 ++++++++++------------ drivers/nvme/host/nvme.h | 1 - 2 files changed, 10 insertions(+), 13 deletions(-)