Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Max Gurtovoy <maxg@mellanox.com>
Cc: sagi@grimberg.me, vladimirk@mellanox.com, idanb@mellanox.com,
	israelr@mellanox.com, linux-nvme@lists.infradead.org,
	shlomin@mellanox.com, oren@mellanox.com, kbusch@kernel.org,
	hch@lst.de
Subject: Re: [PATCH 02/15] nvme: Fail __nvme_revalidate_disk in case of a spec violation
Date: Tue, 5 Nov 2019 18:49:04 +0100
Message-ID: <20191105174904.GA18972@lst.de> (raw)
In-Reply-To: <20191105162026.183901-4-maxg@mellanox.com>

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

  reply index

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-05 16:20 [PATCH 00/15] nvme-rdma/nvmet-rdma: Add metadata/T10-PI support Max Gurtovoy
2019-11-05 16:20 ` [PATCH] nvme-cli/fabrics: Add pi_enable param to connect cmd Max Gurtovoy
2019-11-07  2:36   ` Martin K. Petersen
2019-11-07 12:02     ` Max Gurtovoy
2019-11-09  1:55       ` Martin K. Petersen
2019-11-10  9:25         ` Max Gurtovoy
2019-11-12 17:47           ` Sagi Grimberg
2019-11-12 18:14             ` James Smart
2019-11-12 18:23               ` Sagi Grimberg
2019-11-05 16:20 ` [PATCH 01/15] nvme-fabrics: allow user enabling metadata/T10-PI support Max Gurtovoy
2019-11-12 17:48   ` Sagi Grimberg
2019-11-05 16:20 ` [PATCH 02/15] nvme: Fail __nvme_revalidate_disk in case of a spec violation Max Gurtovoy
2019-11-05 17:49   ` Christoph Hellwig [this message]
2019-11-12 17:52     ` Sagi Grimberg
2019-11-05 16:20 ` [PATCH 03/15] nvme: Introduce max_integrity_segments ctrl attribute Max Gurtovoy
2019-11-05 17:49   ` Christoph Hellwig
2019-11-12 17:53   ` Sagi Grimberg
2019-11-05 16:20 ` [PATCH 04/15] nvme: Inline nvme_ns_has_pi function Max Gurtovoy
2019-11-05 17:50   ` Christoph Hellwig
2019-11-05 16:20 ` [PATCH 05/15] nvme-rdma: Add metadata/T10-PI support Max Gurtovoy
2019-11-05 17:58   ` Christoph Hellwig
2019-11-20 10:41     ` Max Gurtovoy
2019-11-12 18:22   ` Sagi Grimberg
2019-11-13 14:35     ` Max Gurtovoy
2019-11-14 23:57       ` Sagi Grimberg
2019-11-05 16:20 ` [PATCH 06/15] block: Introduce BIP_NOMAP_INTEGRITY bip_flag Max Gurtovoy
2019-11-05 17:52   ` Christoph Hellwig
2019-11-07  2:43   ` Martin K. Petersen
2019-11-07 13:29     ` Max Gurtovoy
2019-11-09  2:10       ` Martin K. Petersen
2019-11-12 10:40         ` Max Gurtovoy
2019-11-05 16:20 ` [PATCH 07/15] nvmet: Prepare metadata request Max Gurtovoy
2019-11-05 16:20 ` [PATCH 08/15] nvmet: Add metadata characteristics for a namespace Max Gurtovoy
2019-11-05 17:59   ` Christoph Hellwig
2019-11-12 18:38     ` Sagi Grimberg
2019-11-05 16:20 ` [PATCH 09/15] nvmet: Rename nvmet_rw_len to nvmet_rw_data_len Max Gurtovoy
2019-11-05 17:59   ` Christoph Hellwig
2019-11-12 18:39   ` Sagi Grimberg
2019-11-05 16:20 ` [PATCH 10/15] nvmet: Rename nvmet_check_data_len to nvmet_check_transfer_len Max Gurtovoy
2019-11-05 18:00   ` Christoph Hellwig
2019-11-12 18:43   ` Sagi Grimberg
2019-11-05 16:20 ` [PATCH 11/15] nvmet: Introduce nvmet_rw_prot_len and nvmet_ns_has_pi Max Gurtovoy
2019-11-05 18:00   ` Christoph Hellwig
2019-11-05 16:20 ` [PATCH 12/15] nvme: Add Metadata Capabilities enumerations Max Gurtovoy
2019-11-05 16:20 ` [PATCH 13/15] nvmet: Add metadata/T10-PI support Max Gurtovoy
2019-11-05 16:20 ` [PATCH 14/15] nvmet: Add metadata support for block devices Max Gurtovoy
2019-11-05 16:20 ` [PATCH 15/15] nvmet-rdma: Add metadata/T10-PI support Max Gurtovoy
2019-11-05 18:02   ` Christoph Hellwig
2019-11-07 13:43     ` Max Gurtovoy
2019-11-12 18:34   ` Sagi Grimberg
2019-11-13 13:56     ` Max Gurtovoy
2019-11-14 23:45       ` Sagi Grimberg

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191105174904.GA18972@lst.de \
    --to=hch@lst.de \
    --cc=idanb@mellanox.com \
    --cc=israelr@mellanox.com \
    --cc=kbusch@kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=maxg@mellanox.com \
    --cc=oren@mellanox.com \
    --cc=sagi@grimberg.me \
    --cc=shlomin@mellanox.com \
    --cc=vladimirk@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git