Re: [PATCH 09/20] nvme: factor out a nvme_configure_metadata helper

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2020/09/28 21:35, Christoph Hellwig wrote:
> Factor out a helper from nvme_update_ns_info that configures the
> per-namespaces metadata and PI settings.  Also make sure the helpers
> clear the flags explicitly instead of all of ->features to allow for
> potentially reusing ->features for future non-metadata flags.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/core.c | 78 ++++++++++++++++++++++++----------------
>  1 file changed, 47 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4a5c4d45755b55..443aba9c9fdd11 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1966,6 +1966,50 @@ static int nvme_setup_streams_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>  	return 0;
>  }
>  
> +static int nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
> +{
> +	struct nvme_ctrl *ctrl = ns->ctrl;
> +
> +	/*
> +	 * The PI implementation requires the metadata size to be equal to the
> +	 * t10 pi tuple size.
> +	 */
> +	ns->ms = le16_to_cpu(id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ms);
> +	if (ns->ms == sizeof(struct t10_pi_tuple))
> +		ns->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
> +	else
> +		ns->pi_type = 0;
> +
> +	ns->features &= ~(NVME_NS_METADATA_SUPPORTED | NVME_NS_EXT_LBAS);
> +	if (!ns->ms || !(ctrl->ops->flags & NVME_F_METADATA_SUPPORTED))
> +		return 0;
> +	if (ctrl->ops->flags & NVME_F_FABRICS) {
> +		/*
> +		 * The NVMe over Fabrics specification only supports metadata as
> +		 * part of the extended data LBA.  We rely on HCA/HBA support to
> +		 * remap the separate metadata buffer from the block layer.
> +		 */
> +		if (WARN_ON_ONCE(!(id->flbas & NVME_NS_FLBAS_META_EXT)))
> +			return -EINVAL;
> +		if (ctrl->max_integrity_segments)
> +			ns->features |=
> +				(NVME_NS_METADATA_SUPPORTED | NVME_NS_EXT_LBAS);
> +	} else {
> +		/*
> +		 * For PCIe controllers, we can't easily remap the separate
> +		 * metadata buffer from the block layer and thus require a
> +		 * separate metadata buffer for block layer metadata/PI support.
> +		 * We allow extended LBAs for the passthrough interface, though.
> +		 */
> +		if (id->flbas & NVME_NS_FLBAS_META_EXT)
> +			ns->features |= NVME_NS_EXT_LBAS;
> +		else
> +			ns->features |= NVME_NS_METADATA_SUPPORTED;
> +	}
> +
> +	return 0;
> +}
> +
>  static void nvme_update_disk_info(struct gendisk *disk,
>  		struct nvme_ns *ns, struct nvme_id_ns *id)
>  {
> @@ -2115,37 +2159,9 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>  		return -ENODEV;
>  	}
>  
> -	ns->features = 0;
> -	ns->ms = le16_to_cpu(id->lbaf[lbaf].ms);
> -	/* the PI implementation requires metadata equal t10 pi tuple size */
> -	if (ns->ms == sizeof(struct t10_pi_tuple))
> -		ns->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
> -	else
> -		ns->pi_type = 0;
> -
> -	if (ns->ms) {
> -		/*
> -		 * For PCIe only the separate metadata pointer is supported,
> -		 * as the block layer supplies metadata in a separate bio_vec
> -		 * chain. For Fabrics, only metadata as part of extended data
> -		 * LBA is supported on the wire per the Fabrics specification,
> -		 * but the HBA/HCA will do the remapping from the separate
> -		 * metadata buffers for us.
> -		 */
> -		if (id->flbas & NVME_NS_FLBAS_META_EXT) {
> -			ns->features |= NVME_NS_EXT_LBAS;
> -			if ((ctrl->ops->flags & NVME_F_FABRICS) &&
> -			    (ctrl->ops->flags & NVME_F_METADATA_SUPPORTED) &&
> -			    ctrl->max_integrity_segments)
> -				ns->features |= NVME_NS_METADATA_SUPPORTED;
> -		} else {
> -			if (WARN_ON_ONCE(ctrl->ops->flags & NVME_F_FABRICS))
> -				return -EINVAL;
> -			if (ctrl->ops->flags & NVME_F_METADATA_SUPPORTED)
> -				ns->features |= NVME_NS_METADATA_SUPPORTED;
> -		}
> -	}
> -
> +	ret = nvme_configure_metadata(ns, id);
> +	if (ret)
> +		return ret;
>  	nvme_set_chunk_sectors(ns, id);
>  	nvme_update_disk_info(ns->disk, ns, id);
>  #ifdef CONFIG_NVME_MULTIPATH
> 

Looks OK to me.

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