[PATCH] nvme: improve check for attached metadata buffers

[PATCH] nvme: improve check for attached metadata buffers

[Christoph Hellwig]

Remove a convoluted check for metadata enabled namespaces, and
turn it into a debug assert that the block layer always attaches
metadata in this case, as it should.

Reported-by: Javier Gonz?lez <javier at cnexlabs.com>
Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 64744355aa88..7b413d5480cf 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -476,14 +476,11 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 	u32 dsmgmt = 0;
 
 	/*
-	 * If formated with metadata, require the block layer provide a buffer
-	 * unless this namespace is formated such that the metadata can be
-	 * stripped/generated by the controller with PRACT=1.
+	 * The block layer should always provide a metadata buffer if the
+	 * namespace is formatted with metadata.
 	 */
-	if (ns && ns->ms &&
-	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)) &&
-	    !blk_integrity_rq(req) && !blk_rq_is_passthrough(req))
-		return BLK_STS_NOTSUPP;
+	if (WARN_ON_ONCE(ns->ms && !blk_integrity_rq(req)))
+		return BLK_STS_IOERR;
 
 	if (req->cmd_flags & REQ_FUA)
 		control |= NVME_RW_FUA;
-- 
2.14.2

[PATCH] nvme: improve check for attached metadata buffers

[Keith Busch]

On Mon, Nov 06, 2017@08:01:52PM +0100, Christoph Hellwig wrote:
> @@ -476,14 +476,11 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
>  	u32 dsmgmt = 0;
>  
>  	/*
> -	 * If formated with metadata, require the block layer provide a buffer
> -	 * unless this namespace is formated such that the metadata can be
> -	 * stripped/generated by the controller with PRACT=1.
> +	 * The block layer should always provide a metadata buffer if the
> +	 * namespace is formatted with metadata.
>  	 */
> -	if (ns && ns->ms &&
> -	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)) &&
> -	    !blk_integrity_rq(req) && !blk_rq_is_passthrough(req))
> -		return BLK_STS_NOTSUPP;
> +	if (WARN_ON_ONCE(ns->ms && !blk_integrity_rq(req)))
> +		return BLK_STS_IOERR;

Sorry for missing this case earlier: this will break namespace usage
when CONFIG_BLK_DEV_INTEGRITY is not set, and the format allows PRACT=1
to strip/generate.

[PATCH] nvme: improve check for attached metadata buffers

[Christoph Hellwig]

On Mon, Nov 06, 2017@12:55:53PM -0700, Keith Busch wrote:
> Sorry for missing this case earlier: this will break namespace usage
> when CONFIG_BLK_DEV_INTEGRITY is not set, and the format allows PRACT=1
> to strip/generate.

Indeed, I completely forgot about the CONFIG_BLK_DEV_INTEGRITY=n case.

How about something like this should do the right thing.  But while
looking over that I'm not sure our !CONFIG_BLK_DEV_INTEGRITY case
for inserting/stripping PI really works - as far as I can tell we
should never set NVME_RW_PRINFO_PRCHK_GUARD in that case, or am I
missing something?

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 66211617e7e8..3a58a07d2a53 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -137,6 +137,11 @@ int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
 
+static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
+{
+	return ns->pi_type && ns->ms == sizeof(struct t10_pi_tuple);
+}
+
 static blk_status_t nvme_error_status(struct request *req)
 {
 	switch (nvme_req(req)->status & 0x7ff) {
@@ -472,16 +477,6 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 	u16 control = 0;
 	u32 dsmgmt = 0;
 
-	/*
-	 * If formated with metadata, require the block layer provide a buffer
-	 * unless this namespace is formated such that the metadata can be
-	 * stripped/generated by the controller with PRACT=1.
-	 */
-	if (ns && ns->ms &&
-	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)) &&
-	    !blk_integrity_rq(req) && !blk_rq_is_passthrough(req))
-		return BLK_STS_NOTSUPP;
-
 	if (req->cmd_flags & REQ_FUA)
 		control |= NVME_RW_FUA;
 	if (req->cmd_flags & (REQ_FAILFAST_DEV | REQ_RAHEAD))
@@ -500,6 +495,18 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 		nvme_assign_write_stream(ctrl, req, &control, &dsmgmt);
 
 	if (ns->ms) {
+		/*
+		 * If formated with metadata, the block layer always provides a
+		 * metadata buffer if CONFIG_BLK_DEV_INTEGRITY is enabled.  Else
+		 * we enable the PRACT bit for protection information or set the
+		 * namespace capacity to zero to prevent any I/O.
+		 */
+		if (!blk_integrity_rq(req)) {
+			if (WARN_ON_ONCE(!nvme_ns_has_pi(ns)))
+				return BLK_STS_NOTSUPP;
+			control |= NVME_RW_PRINFO_PRACT;
+		}
+
 		switch (ns->pi_type) {
 		case NVME_NS_DPS_PI_TYPE3:
 			control |= NVME_RW_PRINFO_PRCHK_GUARD;
@@ -512,8 +519,6 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 					nvme_block_nr(ns, blk_rq_pos(req)));
 			break;
 		}
-		if (!blk_integrity_rq(req))
-			control |= NVME_RW_PRINFO_PRACT;
 	}
 
 	cmnd->rw.control = cpu_to_le16(control);
@@ -1173,7 +1178,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	if (ns->ms && !ns->ext &&
 	    (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED))
 		nvme_init_integrity(disk, ns->ms, ns->pi_type);
-	if (ns->ms && !(ns->ms == 8 && ns->pi_type) && !blk_get_integrity(disk))
+	if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk))
 		capacity = 0;
 	set_capacity(disk, capacity);
 

[PATCH] nvme: improve check for attached metadata buffers

[Keith Busch]

On Mon, Nov 06, 2017@09:26:16PM +0100, Christoph Hellwig wrote:
> How about something like this should do the right thing.  But while
> looking over that I'm not sure our !CONFIG_BLK_DEV_INTEGRITY case
> for inserting/stripping PI really works - as far as I can tell we
> should never set NVME_RW_PRINFO_PRCHK_GUARD in that case, or am I
> missing something?

I think it should be okay as-is. With PRACT, the PRCHK fields should
tell the controller what to generate on writes and what to check on
reads before stripping. See section 8.3.1.1 and 8.3.1.2.

Just noticed potential ECN material in the above sections: 8.3.1.1
lables the two cases '1' and '2', where 8.3.1.2 labels them 'a' and
'b'. Poor consistency, there. :)

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 66211617e7e8..3a58a07d2a53 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -137,6 +137,11 @@ int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(nvme_delete_ctrl_sync);
>  
> +static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
> +{
> +	return ns->pi_type && ns->ms == sizeof(struct t10_pi_tuple);
> +}
> +
>  static blk_status_t nvme_error_status(struct request *req)
>  {
>  	switch (nvme_req(req)->status & 0x7ff) {
> @@ -472,16 +477,6 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
>  	u16 control = 0;
>  	u32 dsmgmt = 0;
>  
> -	/*
> -	 * If formated with metadata, require the block layer provide a buffer
> -	 * unless this namespace is formated such that the metadata can be
> -	 * stripped/generated by the controller with PRACT=1.
> -	 */
> -	if (ns && ns->ms &&
> -	    (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple)) &&
> -	    !blk_integrity_rq(req) && !blk_rq_is_passthrough(req))
> -		return BLK_STS_NOTSUPP;
> -
>  	if (req->cmd_flags & REQ_FUA)
>  		control |= NVME_RW_FUA;
>  	if (req->cmd_flags & (REQ_FAILFAST_DEV | REQ_RAHEAD))
> @@ -500,6 +495,18 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
>  		nvme_assign_write_stream(ctrl, req, &control, &dsmgmt);
>  
>  	if (ns->ms) {
> +		/*
> +		 * If formated with metadata, the block layer always provides a
> +		 * metadata buffer if CONFIG_BLK_DEV_INTEGRITY is enabled.  Else
> +		 * we enable the PRACT bit for protection information or set the
> +		 * namespace capacity to zero to prevent any I/O.
> +		 */
> +		if (!blk_integrity_rq(req)) {
> +			if (WARN_ON_ONCE(!nvme_ns_has_pi(ns)))
> +				return BLK_STS_NOTSUPP;
> +			control |= NVME_RW_PRINFO_PRACT;
> +		}
> +
>  		switch (ns->pi_type) {
>  		case NVME_NS_DPS_PI_TYPE3:
>  			control |= NVME_RW_PRINFO_PRCHK_GUARD;
> @@ -512,8 +519,6 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
>  					nvme_block_nr(ns, blk_rq_pos(req)));
>  			break;
>  		}
> -		if (!blk_integrity_rq(req))
> -			control |= NVME_RW_PRINFO_PRACT;
>  	}
>  
>  	cmnd->rw.control = cpu_to_le16(control);
> @@ -1173,7 +1178,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
>  	if (ns->ms && !ns->ext &&
>  	    (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED))
>  		nvme_init_integrity(disk, ns->ms, ns->pi_type);
> -	if (ns->ms && !(ns->ms == 8 && ns->pi_type) && !blk_get_integrity(disk))
> +	if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk))
>  		capacity = 0;
>  	set_capacity(disk, capacity);
>  

This looks good.