Re: [PATCH 01/15] nvme: Introduce namespace features flag

From: Max Gurtovoy <maxg@mellanox.com>
To: Keith Busch <kbusch@kernel.org>
Cc: axboe@kernel.dk, sagi@grimberg.me, martin.petersen@oracle.com,
	shlomin@mellanox.com, israelr@mellanox.com,
	vladimirk@mellanox.com, linux-nvme@lists.infradead.org,
	idanb@mellanox.com, oren@mellanox.com, hch@lst.de
Subject: Re: [PATCH 01/15] nvme: Introduce namespace features flag
Date: Wed, 8 Jan 2020 14:00:53 +0200	[thread overview]
Message-ID: <a243f1cb-3564-d839-7a5c-44c19409ce79@mellanox.com> (raw)
In-Reply-To: <20200107180716.GA603371@chuupie.wdl.wdc.com>

On 1/7/2020 8:07 PM, Keith Busch wrote:
> On Mon, Jan 06, 2020 at 03:37:22PM +0200, Max Gurtovoy wrote:
>> From: Israel Rukshin <israelr@mellanox.com>
>>
>> Centralize all the integrity checks to one place and make the code more
>> readable. Also add has_pi field to the nvme_req structure as well, so the
>> transport drivers could use it.
> I think the two changes should be in different patches, splitting the
> namespace "features" flags from nvme_request "has_pi". I'm not even
> sure there's a need for the per-IO settings and checks for "has_pi",
> though. I don't find any transport driver changes in this series using
> this flag outside this patch.

Sure, we can split it to 2 commits.

"has_pi" flag is used by RDMA transport (commit 7/15 "nvme-rdma: Add 
metadata/T10-PI support")

>   
>> @@ -1834,12 +1831,29 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
>> +	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))
> A little simpler:
>
> 		if (id->flbas & NVME_NS_FLBAS_META_EXT)
> 			ns->features |= NVME_NS_EXT_LBAS;
> 		else if (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED)
> 			ns->features |= NVME_NS_DIX_SUPPORTED;

In fabrics case this will not be true...

>
>> +				ns->features |= NVME_NS_DIX_SUPPORTED;
>> +		}
>> +	}
> It's not really a "DIX" flag since we can observe separate metadata
> formats for non-DIX related use. Can we use a more generic name for this
> feature flag? maybe "NVME_NS_MS_SEPARATE".

The meaning of NVME_NS_DIX_SUPPORTED is that the block layer is 
*allowed* to send metadata to the namespace (and we later on will call 
nvme_init_integrity).

In case of PCI transport, this will be set only if the SSD use 
"separate" mode since Linux block layer only support "separate" mode and 
the nvme_pci driver pass these buffers to the drive (no conversion to 
extended mode if needed).

In case of fabrics transport, this will be set only if the user asked 
for metadata support in the connect command (pi_enable). Again, Linux 
block layer will send "separate" buffers and the transport driver will 
transform it to "extended" mode (this is a *must* since in fabrics, 
according to the spec, only extended mode supported).

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