* [PATCH] nvme: improve check for attached metadata buffers @ 2017-11-06 19:01 Christoph Hellwig 2017-11-06 19:55 ` Keith Busch 0 siblings, 1 reply; 4+ messages in thread From: Christoph Hellwig @ 2017-11-06 19:01 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] nvme: improve check for attached metadata buffers 2017-11-06 19:01 [PATCH] nvme: improve check for attached metadata buffers Christoph Hellwig @ 2017-11-06 19:55 ` Keith Busch 2017-11-06 20:26 ` Christoph Hellwig 0 siblings, 1 reply; 4+ messages in thread From: Keith Busch @ 2017-11-06 19:55 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] nvme: improve check for attached metadata buffers 2017-11-06 19:55 ` Keith Busch @ 2017-11-06 20:26 ` Christoph Hellwig 2017-11-07 0:00 ` Keith Busch 0 siblings, 1 reply; 4+ messages in thread From: Christoph Hellwig @ 2017-11-06 20:26 UTC (permalink / raw) 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); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] nvme: improve check for attached metadata buffers 2017-11-06 20:26 ` Christoph Hellwig @ 2017-11-07 0:00 ` Keith Busch 0 siblings, 0 replies; 4+ messages in thread From: Keith Busch @ 2017-11-07 0:00 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-11-07 0:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-06 19:01 [PATCH] nvme: improve check for attached metadata buffers Christoph Hellwig 2017-11-06 19:55 ` Keith Busch 2017-11-06 20:26 ` Christoph Hellwig 2017-11-07 0:00 ` Keith Busch
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.