On Fri, Mar 27, 2020 at 08:15:29PM +0300, Max Gurtovoy wrote:
> From: Israel Rukshin <israelr@xxxxxxxxxxxx>
>
> Centralize all the metadata checks to one place and make the code more
> readable. Introduce a new enum nvme_ns_features for that matter.
> The features flag description:
> - NVME_NS_EXT_LBAS - NVMe namespace supports extended LBA format.
> - NVME_NS_MD_HOST_SUPPORTED - NVMe namespace supports getting metadata
> from host's block layer.
> - NVME_NS_MD_CTRL_SUPPORTED - NVMe namespace supports metadata actions
> by the controller (generate/strip).
So whole I like the ->features flag, the defintion of these two
metadata related features really confuses me.
Here are my vague ideas to improve the situation:
> -static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
> -{
> - return ns->pi_type && ns->ms == sizeof(struct t10_pi_tuple);
> -}
This function I think is generally useful, I'd rather keep iţ, document
it with a comment and remove the new NVME_NS_MD_CTRL_SUPPORTED
flag.
> - if (ns->ms && !ns->ext &&
> - (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED))
> + if (ns->features & NVME_NS_MD_HOST_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)
> +
> + if ((ns->ms && !(ns->features & NVME_NS_MD_CTRL_SUPPORTED) &&
> + !(ns->features & NVME_NS_MD_HOST_SUPPORTED) &&
> + !blk_get_integrity(disk)) || ns->lba_shift > PAGE_SHIFT)
> capacity = 0;
I find this very confusing. Can we do something like:
/*
* 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 Typ 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_MD_HOST_SUPPORTED))
nvme_init_integrity(disk, ns->ms, ns->pi_type);
else if (!nvme_ns_has_pi(ns))
capacity = 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.
> + */
> + if (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED) {
> + if (!(ns->features & NVME_NS_EXT_LBAS))
> + ns->features |= NVME_NS_MD_HOST_SUPPORTED;
> + }
Maybe:
> + 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.
> + */
> + if (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED) {
> + if (!(ns->features & NVME_NS_EXT_LBAS))
> + ns->features |= NVME_NS_MD_HOST_SUPPORTED;
> + }
This looks a little strange now, but I guess it will make more sense
with the fabrics addition. I'll take another look later in the series.
> +enum nvme_ns_features {
> + NVME_NS_EXT_LBAS = 1 << 0,
> + NVME_NS_MD_HOST_SUPPORTED = 1 << 1,
> + NVME_NS_MD_CTRL_SUPPORTED = 1 << 2,
> +};
Please document the meaning of each flag. I also suspect that just
moving ext to a flag first and than adding the NVME_NS_MD_HOST_SUPPORTED
bit might make more sense. I'd also rename NVME_NS_MD_HOST_SUPPORTED
to NVME_NS_METADATA_SUPPORTED.
