Re: [PATCH 12/21] nvme: move common logic into nvme_update_ns_info

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 28/02/2024 20:12, Christoph Hellwig wrote:
> nvme_update_ns_info_generic and nvme_update_ns_info_block share a
> fair amount of logic related to not fully supported namespace
> formats and updating the multipath information.  Move this logic
> into the common caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/host/core.c | 79 +++++++++++++++++++---------------------
>   1 file changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index dcb8d0590ed0f3..78667ba89ec491 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2070,21 +2070,8 @@ static int nvme_update_ns_info_generic(struct nvme_ns *ns,
>   	set_disk_ro(ns->disk, nvme_ns_is_readonly(ns, info));
>   	blk_mq_unfreeze_queue(ns->disk->queue);
>   
> -	if (nvme_ns_head_multipath(ns->head)) {
> -		blk_mq_freeze_queue(ns->head->disk->queue);
> -		set_disk_ro(ns->head->disk, nvme_ns_is_readonly(ns, info));
> -		nvme_mpath_revalidate_paths(ns);
> -		blk_stack_limits(&ns->head->disk->queue->limits,
> -				 &ns->queue->limits, 0);
> -		ns->head->disk->flags |= GENHD_FL_HIDDEN;
> -		blk_mq_unfreeze_queue(ns->head->disk->queue);
> -	}
> -
>   	/* Hide the block-interface for these devices */
> -	ns->disk->flags |= GENHD_FL_HIDDEN;
> -	set_bit(NVME_NS_READY, &ns->flags);
> -
> -	return 0;
> +	return -ENODEV;
>   }
>   
>   static int nvme_update_ns_info_block(struct nvme_ns *ns,
> @@ -2104,7 +2091,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>   		/* namespace not allocated or attached */
>   		info->is_removed = true;
>   		ret = -ENODEV;
> -		goto error;
> +		goto out;
>   	}
>   
>   	blk_mq_freeze_queue(ns->disk->queue);
> @@ -2162,54 +2149,62 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>   			goto out;
>   	}
>   
> -	if (nvme_ns_head_multipath(ns->head)) {
> -		blk_mq_freeze_queue(ns->head->disk->queue);
> -		nvme_init_integrity(ns->head->disk, ns->head);
> -		set_capacity_and_notify(ns->head->disk, get_capacity(ns->disk));
> -		set_disk_ro(ns->head->disk, nvme_ns_is_readonly(ns, info));
> -		nvme_mpath_revalidate_paths(ns);
> -		blk_stack_limits(&ns->head->disk->queue->limits,
> -				 &ns->queue->limits, 0);
> -		disk_update_readahead(ns->head->disk);
> -		blk_mq_unfreeze_queue(ns->head->disk->queue);
> -	}
> -
>   	ret = 0;
>   out:
> -	/*
> -	 * If probing fails due an unsupported feature, hide the block device,
> -	 * but still allow other access.
> -	 */
> -	if (ret == -ENODEV) {
> -		ns->disk->flags |= GENHD_FL_HIDDEN;
> -		set_bit(NVME_NS_READY, &ns->flags);
> -		ret = 0;
> -	}
> -
> -error:
>   	kfree(id);
>   	return ret;
>   }
>   
>   static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
>   {
> +	int ret;
> +
>   	switch (info->ids.csi) {
>   	case NVME_CSI_ZNS:
>   		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
>   			dev_info(ns->ctrl->device,
>   	"block device for nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
>   				info->nsid);
> -			return nvme_update_ns_info_generic(ns, info);
> +			ret = nvme_update_ns_info_generic(ns, info);
> +			break;
>   		}
> -		return nvme_update_ns_info_block(ns, info);
> +		ret = nvme_update_ns_info_block(ns, info);
> +		break;
>   	case NVME_CSI_NVM:
> -		return nvme_update_ns_info_block(ns, info);
> +		ret = nvme_update_ns_info_block(ns, info);
> +		break;
>   	default:
>   		dev_info(ns->ctrl->device,
>   			"block device for nsid %u not supported (csi %u)\n",
>   			info->nsid, info->ids.csi);
> -		return nvme_update_ns_info_generic(ns, info);
> +		ret = nvme_update_ns_info_generic(ns, info);
> +		break;
> +	}
> +
> +	/*
> +	 * If probing fails due an unsupported feature, hide the block device,
> +	 * but still allow other access.
> +	 */
> +	if (ret == -ENODEV) {
> +		ns->disk->flags |= GENHD_FL_HIDDEN;
> +		set_bit(NVME_NS_READY, &ns->flags);
> +		ret = 0;
> +	}
> +
> +	if (!ret && nvme_ns_head_multipath(ns->head)) {
> +		blk_mq_freeze_queue(ns->head->disk->queue);
> +		if (!(ns->disk->flags & GENHD_FL_HIDDEN))
> +			nvme_init_integrity(ns->head->disk, ns->head);

the logic in "nvme_update_ns_info_generic()" today is to hide also 
ns->head->disk..


> +		set_capacity_and_notify(ns->head->disk, get_capacity(ns->disk));
> +		set_disk_ro(ns->head->disk, nvme_ns_is_readonly(ns, info));
> +		nvme_mpath_revalidate_paths(ns);
> +		blk_stack_limits(&ns->head->disk->queue->limits,
> +				 &ns->queue->limits, 0);
> +		disk_update_readahead(ns->head->disk);

we don't call disk_update_readahead() in "nvme_update_ns_info_generic()" 
today..

> +		blk_mq_unfreeze_queue(ns->head->disk->queue);
>   	}
> +
> +	return ret;
>   }
>   
>   #ifdef CONFIG_BLK_SED_OPAL