| Message ID | 1509703370-20379-2-git-send-email-javier@cnexlabs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> - if (ns && ns->ms && > + if (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; blk_rq_is_passthrough also can't be true here. How about: if (ns->ms && !blk_integrity_rq(req) && (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple))) return BLK_STS_NOTSUPP; Although I have to admit I don't really understand what this check is even trying to do. It basically checks for a namespace that has a format with metadata that is not T10 protection information and then rejects all I/O to it. Why are we even creating a block device node for such a thing?
> On 3 Nov 2017, at 13.53, Christoph Hellwig <hch@lst.de> wrote: > >> - if (ns && ns->ms && >> + if (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; > > blk_rq_is_passthrough also can't be true here. > > How about: > > if (ns->ms && !blk_integrity_rq(req) && > (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple))) > return BLK_STS_NOTSUPP; > Sure. > Although I have to admit I don't really understand what this check > is even trying to do. It basically checks for a namespace that has > a format with metadata that is not T10 protection information and > then rejects all I/O to it. Why are we even creating a block device > node for such a thing? Looking at the history (i) the check has changed location and (ii) some checks have been added through time. So it looks like leftovers from here and there. If we end up not needing these checks at all here, you can just fix it all in the same commit. Just wanted to get rid of sparse/smatch complains...
On Fri, Nov 03, 2017 at 01:53:40PM +0100, Christoph Hellwig wrote: > > - if (ns && ns->ms && > > + if (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; > > blk_rq_is_passthrough also can't be true here. > > How about: > > if (ns->ms && !blk_integrity_rq(req) && > (!ns->pi_type || ns->ms != sizeof(struct t10_pi_tuple))) > return BLK_STS_NOTSUPP; > > Although I have to admit I don't really understand what this check > is even trying to do. It basically checks for a namespace that has > a format with metadata that is not T10 protection information and > then rejects all I/O to it. Why are we even creating a block device > node for such a thing? If the namespace has metadata, but the request doesn't have a metadata payload attached to it for whatever reason, we can't construct the command for that format. We also can't have the controller strip/generate the payload with PRACT bit set if it's not a T10 format, so we just fail the command.
On Fri, Nov 03, 2017 at 09:02:04AM -0600, Keith Busch wrote: > If the namespace has metadata, but the request doesn't have a metadata > payload attached to it for whatever reason, we can't construct the command > for that format. We also can't have the controller strip/generate the > payload with PRACT bit set if it's not a T10 format, so we just fail > the command. The only metadata payload a READ or WRITE request can have is a protection information one. For a namespace formatted with protection information bio_integrity_prep as called from blk_mq_make_request will ensure we always have metadata attached. If a namespace is formatted with non-PI metadata we will never have metadata attached to the bio/request and should not even present the namespace to the kernel.
On Sat, Nov 04, 2017 at 09:18:25AM +0100, Christoph Hellwig wrote: > On Fri, Nov 03, 2017 at 09:02:04AM -0600, Keith Busch wrote: > > If the namespace has metadata, but the request doesn't have a metadata > > payload attached to it for whatever reason, we can't construct the command > > for that format. We also can't have the controller strip/generate the > > payload with PRACT bit set if it's not a T10 format, so we just fail > > the command. > > The only metadata payload a READ or WRITE request can have is a protection > information one. For a namespace formatted with protection information > bio_integrity_prep as called from blk_mq_make_request will ensure we > always have metadata attached. > > If a namespace is formatted with non-PI metadata we will never have > metadata attached to the bio/request and should not even present the > namespace to the kernel. That's not quite right. For non-PI metadata formats, we use the 'nop_profile', which gets the metadata buffer allocated so we can safely use a metadata formatted namespace. There's no in-kernel user of the allocated payload, but we still need the metadata buffer in order to use the namespace at all.
On Sat, Nov 04, 2017 at 09:38:45AM -0600, Keith Busch wrote: > That's not quite right. For non-PI metadata formats, we use the > 'nop_profile', which gets the metadata buffer allocated so we can safely > use a metadata formatted namespace. There's no in-kernel user of the > allocated payload, but we still need the metadata buffer in order to > use the namespace at all. You're right. But that means we will indeed always have a matching integrity payload here and the check should not be needed. Are you fine with turning it into something like: if (WARN_ON_ONCE(ns->ms && !blk_integrity_rq(req))) return BLK_STS_IOERR; ?
On Mon, Nov 06, 2017 at 10:13:24AM +0100, Christoph Hellwig wrote: > On Sat, Nov 04, 2017 at 09:38:45AM -0600, Keith Busch wrote: > > That's not quite right. For non-PI metadata formats, we use the > > 'nop_profile', which gets the metadata buffer allocated so we can safely > > use a metadata formatted namespace. There's no in-kernel user of the > > allocated payload, but we still need the metadata buffer in order to > > use the namespace at all. > > You're right. But that means we will indeed always have a matching > integrity payload here and the check should not be needed. > > Are you fine with turning it into something like: > > if (WARN_ON_ONCE(ns->ms && !blk_integrity_rq(req))) > return BLK_STS_IOERR; > > ? Yes, that looks fine. You're right that we are not supposed to be able to get into this path for read/write since the driver sets the capacity to 0 if a metadata format doesn't have integrity support, so hitting this warning would indicate something has gone wrong.
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 5a14cc7f28ee..bd1d5ff911c9 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -472,7 +472,7 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns, * unless this namespace is formated such that the metadata can be * stripped/generated by the controller with PRACT=1. */ - if (ns && ns->ms && + if (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;
On the rw path, the ns is assumed to be set. However, a check is still done, inherited from the time the code resided at nvme_queue_rq(). Eliminate this check, which also eliminates a smatch complain for not doing proper NULL checks on ns. Signed-off-by: Javier González <javier@cnexlabs.com> --- drivers/nvme/host/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)