Re: [PATCH 06/16] nvme: enforce extended LBA format for fabrics metadata

[James Smart <james.smart@broadcom.com>]

On 5/4/2020 8:57 AM, Max Gurtovoy wrote:
> An extended LBA is a larger LBA that is created when metadata associated
> with the LBA is transferred contiguously with the LBA data (AKA
> interleaved). The metadata may be either transferred as part of the LBA
> (creating an extended LBA) or it may be transferred as a separate
> contiguous buffer of data. According to the NVMeoF spec, a fabrics ctrl
> supports only an Extended LBA format. Fail revalidation in case we have a
> spec violation. Also add a flag that will imply on capable transports and
> controllers as part of a preparation for allowing end-to-end protection
> information for fabric controllers.
>
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Signed-off-by: Israel Rukshin <israelr@mellanox.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/host/core.c | 41 +++++++++++++++++++++++++++--------------
>   1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 89b1bfcb..f565281 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1941,16 +1942,24 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
>   		ns->pi_type = 0;
>   
>   	if (ns->ms) {
> -		if (id->flbas & NVME_NS_FLBAS_META_EXT)
> -			ns->features |= NVME_NS_EXT_LBAS;
> -
>   		/*
> -		 * For PCI, Extended logical block will be generated by the
> -		 * controller. Non-extended format can be generated by the
> -		 * block layer.
> +		 * 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 (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED) {
> -			if (!(ns->features & NVME_NS_EXT_LBAS))
> +		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;

This is ok.  Later patches will should deal with no block-layer 
integrity buffer, but having the transport port auto-generate/strip 
interleaved pi data, as we will want to protect during transmission on 
the wire, not just PRACT=1 it.

> +		} 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;
>   		}

The NVME_F_METADATA_SUPPORTED flag should be on a per-ctrl basis. There 
will be different transport ports under the same transport with 
different abilities.

I'm ok if these patches go in, then a later patch moves the flags (might 
as well move NVME_F_FABRICS too).


Reviewed-by:  James Smart <james.smart@broadcom.com>

-- james



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