Re: [PATCH 03/16] nvme: introduce NVME_NS_METADATA_SUPPORTED flag

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

On 5/4/2020 8:57 AM, Max Gurtovoy wrote:
> This is a preparation for adding support for metadata in fabric
> controllers. New flag will imply that NVMe namespace supports getting
> metadata that was originally generated by host's block layer.
>
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Israel Rukshin <israelr@mellanox.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/host/core.c | 41 ++++++++++++++++++++++++++++++++++-------
>   drivers/nvme/host/nvme.h |  1 +
>   2 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 1d226cc..4b7faf9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1882,13 +1882,27 @@ static void nvme_update_disk_info(struct gendisk *disk,
>   	blk_queue_io_min(disk->queue, phys_bs);
>   	blk_queue_io_opt(disk->queue, io_opt);
>   
> -	if (ns->ms && !(ns->features & NVME_NS_EXT_LBAS) &&
> -	    (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED))
> -		nvme_init_integrity(disk, ns->ms, ns->pi_type);
> -	if ((ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk)) ||
> -	    ns->lba_shift > PAGE_SHIFT)
> +	/*
> +	 * The block layer can't support LBA sizes larger than the page size
> +	 * yet, so catch this early and don't allow block I/O.
> +	 */
> +	if (ns->lba_shift > PAGE_SHIFT)
>   		capacity = 0;
>   
> +	/*
> +	 * Register a metadata profile for PI, or the plain non-integrity NVMe
> +	 * metadata masquerading as Type 0 if supported, otherwise reject block
> +	 * I/O to namespaces with metadata except when the namespace supports
> +	 * PI, as it can strip/insert in that case.
> +	 */
> +	if (ns->ms) {
> +		if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) &&
> +		    (ns->features & NVME_NS_METADATA_SUPPORTED))
> +			nvme_init_integrity(disk, ns->ms, ns->pi_type);
> +		else if (!nvme_ns_has_pi(ns))
> +			capacity = 0;
> +	}
> +
Look below for how I interpret the meaning of the 
NVME_NS_METADATA_SUPPORTED flag. It's a rollup of several conditions. 
Not all of those conditions are considered in the else clause.

The "else if" clause looks too light to address all the cases where 
capacity should be set to 0. Probably shouldn't be an else.
Examples:
- ! IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) & meta is pi   (aka nvme_hs_has_pi)
- meta is not pi (thus pi_type=0 in call to nvme_init_integrity()), 
which results in  !blk_get_integrity(disk) which is not checked.
- meta is pi and:
   - !ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED
   - !ns->features & NVME_NS_EXT_LBAS

may be a couple others.

>   	set_capacity_revalidate_and_notify(disk, capacity, false);
>   
>   	nvme_config_discard(disk, ns);
> @@ -1923,14 +1937,27 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
>   
>   	ns->features = 0;
>   	ns->ms = le16_to_cpu(id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ms);
> -	if (ns->ms && (id->flbas & NVME_NS_FLBAS_META_EXT))
> -		ns->features |= NVME_NS_EXT_LBAS;
>   	/* 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) {
> +		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.
> +		 */
> +		if (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED) {
> +			if (!(ns->features & NVME_NS_EXT_LBAS))
> +				ns->features |= NVME_NS_METADATA_SUPPORTED;

So I interpret this flag to mean:
1) namespace has metadata,
2) controller transport supports metadata (and implicitly: since it's 
pci so far it's via separate buffer only)
3) the nvme controller requires a separate metadata buffer (thus it 
matches the transport).
4) and implicitly, if we're going to have DIF, the block layer will give 
us a separate buffer.
note: all bets are off if the metadata isn't for DIF. but that's not 
handled here.....

I wish the comment explained that.

And of course these will change with fabrics as it will be expected that 
ns's on fabrics will be ns->features & NVME_NS_EXT_LBAS. But - that 
isn't part of this commit.  So doesn't count against this patch.

> +		}
> +	}
> +
>   	if (iob)
>   		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
>   	nvme_update_disk_info(disk, ns, id);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 110577c7..0dda145 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -366,6 +366,7 @@ struct nvme_ns_head {
>   
>   enum nvme_ns_features {
>   	NVME_NS_EXT_LBAS = 1 << 0, /* support extended LBA format */
> +	NVME_NS_METADATA_SUPPORTED = 1 << 1, /* support getting generated md */
>   };
>   
>   struct nvme_ns {


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