Re: [PATCH 15/20] nvme: remove nvme_update_formats

From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Christoph Hellwig <hch@lst.de>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
Cc: Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@kernel.dk>,
	Sagi Grimberg <sagi@grimberg.me>
Subject: Re: [PATCH 15/20] nvme: remove nvme_update_formats
Date: Mon, 28 Sep 2020 15:02:48 +0000	[thread overview]
Message-ID: <CY4PR04MB3751105F9967301E5670C5C6E7350@CY4PR04MB3751.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20200928123502.435373-16-hch@lst.de

On 2020/09/28 21:35, Christoph Hellwig wrote:
> Now that the queue is frozen before updating ->lba_shift we can't hit the
> invalid references mentioned in the comment any more.  More importantly
> this code would not have helped us if the format was changed by another
> controller or through implementation defined back channels.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/core.c | 32 ++------------------------------
>  1 file changed, 2 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4737591c1143ae..f19f6c7c5b1242 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -89,7 +89,6 @@ static dev_t nvme_chr_devt;
>  static struct class *nvme_class;
>  static struct class *nvme_subsys_class;
>  
> -static int nvme_validate_ns(struct nvme_ns *ns);
>  static void nvme_put_subsystem(struct nvme_subsystem *subsys);
>  static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
>  					   unsigned nsid);
> @@ -1009,7 +1008,7 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>  	 * For simplicity, IO to all namespaces is quiesced even if the command
>  	 * effects say only one namespace is affected.
>  	 */
> -	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK)) {
> +	if (effects & NVME_CMD_EFFECTS_CSE_MASK) {
>  		mutex_lock(&ctrl->scan_lock);
>  		mutex_lock(&ctrl->subsys->lock);
>  		nvme_mpath_start_freeze(ctrl->subsys);
> @@ -1020,36 +1019,9 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>  	return effects;
>  }
>  
> -static void nvme_update_formats(struct nvme_ctrl *ctrl, u32 *effects)
> -{
> -	struct nvme_ns *ns;
> -
> -	down_read(&ctrl->namespaces_rwsem);
> -	list_for_each_entry(ns, &ctrl->namespaces, list)
> -		if (nvme_validate_ns(ns))
> -			nvme_set_queue_dying(ns);
> -		else if (blk_queue_is_zoned(ns->disk->queue)) {
> -			/*
> -			 * IO commands are required to fully revalidate a zoned
> -			 * device. Force the command effects to trigger rescan
> -			 * work so report zones can run in a context with
> -			 * unfrozen IO queues.
> -			 */
> -			*effects |= NVME_CMD_EFFECTS_NCC;
> -		}
> -	up_read(&ctrl->namespaces_rwsem);
> -}
> -
>  static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
>  {
> -	/*
> -	 * Revalidate LBA changes prior to unfreezing. This is necessary to
> -	 * prevent memory corruption if a logical block size was changed by
> -	 * this command.
> -	 */
> -	if (effects & NVME_CMD_EFFECTS_LBCC)
> -		nvme_update_formats(ctrl, &effects);
> -	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK)) {
> +	if (effects & NVME_CMD_EFFECTS_CSE_MASK) {
>  		nvme_unfreeze(ctrl);
>  		nvme_mpath_unfreeze(ctrl->subsys);
>  		mutex_unlock(&ctrl->subsys->lock);
> 

Looks OK, but I am not so knowledgeable in this area...
Anyway it does look consistent with the effect flags change in patch 14, so:

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme