Re: [PATCH 02/15] nvme: Fail __nvme_revalidate_disk in case of a spec violation

[Christoph Hellwig <hch@lst.de>]

On Tue, Nov 05, 2019 at 06:20:13PM +0200, 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 init the integrity profile for the block device for
> fabrics ctrl.

I don't think the subject describes very well what this patch does,
as it really wires up parsing and checking the ext flag for fabrics.

> +	if (ns->ms && (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED)) {
> +		if ((ns->ctrl->opts && ns->ctrl->opts->pi_enable) || !ns->ext)
> +			nvme_init_integrity(disk, ns->ms, ns->pi_type);
> +	}

Can we just have a flag that says we have working metadata support instead
of having to duplicate the checks all over?

>  	ns->noiob = le16_to_cpu(id->noiob);
>  	ns->ms = le16_to_cpu(id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ms);
>  	ns->ext = ns->ms && (id->flbas & NVME_NS_FLBAS_META_EXT);
> +
> +	/*
> +	 * For Fabrics, only metadata as part of extended data LBA is supported.
> +	 * fail in case of a spec violation.
> +	 */
> +	if (ns->ms && (ns->ctrl->ops->flags & NVME_F_FABRICS)) {
> +		if (WARN_ON_ONCE(!ns->ext))
> +			return -EINVAL;
> +	}
> +
>  	/* the PI implementation requires metadata equal t10 pi tuple size */
>  	if (ns->ms == sizeof(struct t10_pi_tuple))

This is getting a little convoluted.  I think this should rely on the
fact that we zero the whole namespace structure and clean a lot of the
mess up.  Also I think we should reject non-external metadata on PCIe
as we can't really properly support it right here.  Also instead of
using NVME_F_FABRICS I'd rather split NVME_F_METADATA_SUPPORTED into
a flag each for external or inline metadata supported.  That also means
your patch 1 can be moved much later in series where it belongs.

	if (ns->ms) {
		ns->ext = id->flbas & NVME_NS_FLBAS_META_EXT;
		if (ns->ext) {
			if (!(ns->ctrl->ops->flags & NVME_F_EXTENDED_LBA))
				return -EINVAL;
		} else {
			if (!(ns->ctrl->ops->flags & NVME_F_METADATA_P)))
				return -EINVL;
		}
		ns->metadata_supported = true;

		/*
		 * 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;
	}

> - out_put_disk:
> +out_put_disk:
>  	put_disk(ns->disk);
> - out_unlink_ns:
> +out_free_disk:
> +	del_gendisk(ns->disk);
> +out_unlink_ns:
>  	mutex_lock(&ctrl->subsys->lock);
>  	list_del_rcu(&ns->siblings);
>  	mutex_unlock(&ctrl->subsys->lock);
>  	nvme_put_ns_head(ns->head);
> - out_free_id:
> +out_free_id:
>  	kfree(id);
> - out_free_queue:
> +out_free_queue:
>  	blk_cleanup_queue(ns->queue);
> - out_free_ns:
> +out_free_ns:

No real need to reformat this.

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