* [PATCH 0/3] misc nvme fixes @ 2018-06-07 8:38 Hannes Reinecke 2018-06-07 8:38 ` [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() Hannes Reinecke ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Hannes Reinecke @ 2018-06-07 8:38 UTC (permalink / raw) Hi all, here are some small fixes I've found required when testing ANA support. They are not directly related to ANA itself so they're send as a separate patchset. As usual, comments and reviews are welcome. Hannes Reinecke (3): nvme: also check for RESETTING state in nvmf_check_if_ready() nvme: do not access request after calling blk_mq_end_request() nvme: bio remapping tracepoint drivers/nvme/host/fabrics.c | 1 + drivers/nvme/host/multipath.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) -- 2.12.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() 2018-06-07 8:38 [PATCH 0/3] misc nvme fixes Hannes Reinecke @ 2018-06-07 8:38 ` Hannes Reinecke 2018-06-07 8:41 ` Sagi Grimberg 2018-06-07 8:38 ` [PATCH 2/3] nvme: do not access request after calling blk_mq_end_request() Hannes Reinecke 2018-06-07 8:38 ` [PATCH 3/3] nvme: bio remapping tracepoint Hannes Reinecke 2 siblings, 1 reply; 15+ messages in thread From: Hannes Reinecke @ 2018-06-07 8:38 UTC (permalink / raw) When resetting the controller some transports go into 'RESETTING' state before issuing the actual 'reset' command. So add the 'RESETTING' state to nvmf_check_if_ready() to give them a chance to submit it. Signed-off-by: Hannes Reinecke <hare at suse.com> --- drivers/nvme/host/fabrics.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index fa32c1216409..909dd337221a 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -548,6 +548,7 @@ blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, case NVME_CTRL_NEW: case NVME_CTRL_CONNECTING: case NVME_CTRL_DELETING: + case NVME_CTRL_RESETTING: /* * This is the case of starting a new or deleting an association * but connectivity was lost before it was fully created or torn -- 2.12.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() 2018-06-07 8:38 ` [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() Hannes Reinecke @ 2018-06-07 8:41 ` Sagi Grimberg 2018-06-07 11:26 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Sagi Grimberg @ 2018-06-07 8:41 UTC (permalink / raw) Thank you Hannes - just spotted that... On 06/07/2018 11:38 AM, Hannes Reinecke wrote: > When resetting the controller some transports go into 'RESETTING' state > before issuing the actual 'reset' command. So add the 'RESETTING' state > to nvmf_check_if_ready() to give them a chance to submit it. > > Signed-off-by: Hannes Reinecke <hare at suse.com> > --- > drivers/nvme/host/fabrics.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c > index fa32c1216409..909dd337221a 100644 > --- a/drivers/nvme/host/fabrics.c > +++ b/drivers/nvme/host/fabrics.c > @@ -548,6 +548,7 @@ blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, > case NVME_CTRL_NEW: > case NVME_CTRL_CONNECTING: > case NVME_CTRL_DELETING: > + case NVME_CTRL_RESETTING: I guess ADMIN_ONLY should also allow commands to pass. > /* > * This is the case of starting a new or deleting an association > * but connectivity was lost before it was fully created or torn Looks like we have almost all the states? Shouldn't we reverse such that we check only for DEAD (and WARN_ON for state NEW)? ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() 2018-06-07 8:41 ` Sagi Grimberg @ 2018-06-07 11:26 ` Christoph Hellwig 2018-06-12 21:20 ` James Smart 0 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2018-06-07 11:26 UTC (permalink / raw) On Thu, Jun 07, 2018@11:41:11AM +0300, Sagi Grimberg wrote: > Thank you Hannes - just spotted that... > > On 06/07/2018 11:38 AM, Hannes Reinecke wrote: >> When resetting the controller some transports go into 'RESETTING' state >> before issuing the actual 'reset' command. So add the 'RESETTING' state >> to nvmf_check_if_ready() to give them a chance to submit it. >> >> Signed-off-by: Hannes Reinecke <hare at suse.com> >> --- >> drivers/nvme/host/fabrics.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c >> index fa32c1216409..909dd337221a 100644 >> --- a/drivers/nvme/host/fabrics.c >> +++ b/drivers/nvme/host/fabrics.c >> @@ -548,6 +548,7 @@ blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, >> case NVME_CTRL_NEW: >> case NVME_CTRL_CONNECTING: >> case NVME_CTRL_DELETING: >> + case NVME_CTRL_RESETTING: > > I guess ADMIN_ONLY should also allow commands to pass. Only admin commands, though. > >> /* >> * This is the case of starting a new or deleting an association >> * but connectivity was lost before it was fully created or torn > > Looks like we have almost all the states? Shouldn't we reverse such that > we check only for DEAD (and WARN_ON for state NEW)? Sounds like an idea. I'd like to see it in patch form first before agreeing.. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() 2018-06-07 11:26 ` Christoph Hellwig @ 2018-06-12 21:20 ` James Smart 0 siblings, 0 replies; 15+ messages in thread From: James Smart @ 2018-06-12 21:20 UTC (permalink / raw) On 6/7/2018 4:26 AM, Christoph Hellwig wrote: > On Thu, Jun 07, 2018@11:41:11AM +0300, Sagi Grimberg wrote: >> Thank you Hannes - just spotted that... >> >> On 06/07/2018 11:38 AM, Hannes Reinecke wrote: >>> When resetting the controller some transports go into 'RESETTING' state >>> before issuing the actual 'reset' command. So add the 'RESETTING' state >>> to nvmf_check_if_ready() to give them a chance to submit it. I disagree with the base patch, as it can let other things through while in a reset when there was only 1 command to get through.? I would want to see a check for the property_set() command only as writing the CC.EN=0 is the only thing that should be sent on the wire if in a resetting state. >>> >>> Signed-off-by: Hannes Reinecke <hare at suse.com> >>> --- >>> drivers/nvme/host/fabrics.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c >>> index fa32c1216409..909dd337221a 100644 >>> --- a/drivers/nvme/host/fabrics.c >>> +++ b/drivers/nvme/host/fabrics.c >>> @@ -548,6 +548,7 @@ blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, >>> case NVME_CTRL_NEW: >>> case NVME_CTRL_CONNECTING: >>> case NVME_CTRL_DELETING: >>> + case NVME_CTRL_RESETTING: >> I guess ADMIN_ONLY should also allow commands to pass. > Only admin commands, though. Huh - why do you want any commands, beyond a CC.EN=0 write, in a resetting state ? Isn't ADMIN_ONLY just a form of LIVE with no io queues ???? why would you lump in this section that's trying to restrict commands to initialization things only ? note: only pci sets things to ADMIN_ONLY. The fabric transports happily run with things w/o io queues (discovery controllers) in a LIVE state, not ADMIN_ONLY. > >>> /* >>> * This is the case of starting a new or deleting an association >>> * but connectivity was lost before it was fully created or torn >> Looks like we have almost all the states? Shouldn't we reverse such that >> we check only for DEAD (and WARN_ON for state NEW)? > Sounds like an idea. I'd like to see it in patch form first before > agreeing.. > It appears the whole idea of what is being filtered and why, which also means we aren't specific for what should be allowed in the different states, has been forgotten.? I'll go look at Christoph's reworked patch and comment there. -- james ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] nvme: do not access request after calling blk_mq_end_request() 2018-06-07 8:38 [PATCH 0/3] misc nvme fixes Hannes Reinecke 2018-06-07 8:38 ` [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() Hannes Reinecke @ 2018-06-07 8:38 ` Hannes Reinecke 2018-06-07 8:42 ` Sagi Grimberg 2018-06-07 11:27 ` Christoph Hellwig 2018-06-07 8:38 ` [PATCH 3/3] nvme: bio remapping tracepoint Hannes Reinecke 2 siblings, 2 replies; 15+ messages in thread From: Hannes Reinecke @ 2018-06-07 8:38 UTC (permalink / raw) After calling blk_mq_end_request() the request should be considered freed, so accessing it afterwards might lead to use-after-free error. Signed-off-by: Hannes Reinecke <hare at suse.com> --- drivers/nvme/host/multipath.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index 7f34ae260ca9..87bd60b49bbc 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -51,6 +51,7 @@ void nvme_failover_req(struct request *req) struct nvme_ns *ns = req->q->queuedata; struct device *dev = disk_to_dev(ns->disk); unsigned long flags; + unsigned int nvme_status = nvme_req(req)->status & 0x7ff; enum nvme_ana_state ana_state; bool ana_state_changed = false; @@ -64,7 +65,7 @@ void nvme_failover_req(struct request *req) * caused the error: */ ana_state = READ_ONCE(ns->ctrl->ana_state[ns->anagrpid]); - switch (nvme_req(req)->status & 0x7ff) { + switch (nvme_status) { case NVME_SC_ANA_TRANSITION: if (ana_state != NVME_ANA_CHANGE) { nvme_update_ana_state(ns, NVME_ANA_CHANGE); -- 2.12.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] nvme: do not access request after calling blk_mq_end_request() 2018-06-07 8:38 ` [PATCH 2/3] nvme: do not access request after calling blk_mq_end_request() Hannes Reinecke @ 2018-06-07 8:42 ` Sagi Grimberg 2018-06-07 11:27 ` Christoph Hellwig 1 sibling, 0 replies; 15+ messages in thread From: Sagi Grimberg @ 2018-06-07 8:42 UTC (permalink / raw) Reviewed-by: Sagi Grimberg <sagi at grimberg.me> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] nvme: do not access request after calling blk_mq_end_request() 2018-06-07 8:38 ` [PATCH 2/3] nvme: do not access request after calling blk_mq_end_request() Hannes Reinecke 2018-06-07 8:42 ` Sagi Grimberg @ 2018-06-07 11:27 ` Christoph Hellwig 1 sibling, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2018-06-07 11:27 UTC (permalink / raw) On Thu, Jun 07, 2018@10:38:46AM +0200, Hannes Reinecke wrote: > After calling blk_mq_end_request() the request should be considered > freed, so accessing it afterwards might lead to use-after-free error. > > Signed-off-by: Hannes Reinecke <hare at suse.com> Looks fine, although I'd call the variable status instead of nvme_status. I'll fold this into the next ANA series given that as-is it isn't going to apply anyway.. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] nvme: bio remapping tracepoint 2018-06-07 8:38 [PATCH 0/3] misc nvme fixes Hannes Reinecke 2018-06-07 8:38 ` [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() Hannes Reinecke 2018-06-07 8:38 ` [PATCH 2/3] nvme: do not access request after calling blk_mq_end_request() Hannes Reinecke @ 2018-06-07 8:38 ` Hannes Reinecke 2018-06-07 8:41 ` Sagi Grimberg 2018-06-07 11:27 ` Christoph Hellwig 2 siblings, 2 replies; 15+ messages in thread From: Hannes Reinecke @ 2018-06-07 8:38 UTC (permalink / raw) Adding a tracepoint to trace bio remapping for native nvme multipath. Signed-off-by: Hannes Reinecke <hare at suse.com> --- drivers/nvme/host/multipath.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index 87bd60b49bbc..620bcd749912 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -12,6 +12,7 @@ */ #include <linux/moduleparam.h> +#include <trace/events/block.h> #include "nvme.h" static bool multipath = true; @@ -181,6 +182,9 @@ static blk_qc_t nvme_ns_head_make_request(struct request_queue *q, if (likely(ns)) { bio->bi_disk = ns->disk; bio->bi_opf |= REQ_NVME_MPATH; + trace_block_bio_remap(bio->bi_disk->queue, bio, + disk_devt(ns->head->disk), + bio->bi_iter.bi_sector); ret = direct_make_request(bio); } else if (!list_empty_careful(&head->list)) { dev_warn_ratelimited(dev, "no path available - requeuing I/O\n"); -- 2.12.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] nvme: bio remapping tracepoint 2018-06-07 8:38 ` [PATCH 3/3] nvme: bio remapping tracepoint Hannes Reinecke @ 2018-06-07 8:41 ` Sagi Grimberg 2018-06-07 11:27 ` Christoph Hellwig 1 sibling, 0 replies; 15+ messages in thread From: Sagi Grimberg @ 2018-06-07 8:41 UTC (permalink / raw) Reviewed-by: Sagi Grimberg <sagi at grimberg.me> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] nvme: bio remapping tracepoint 2018-06-07 8:38 ` [PATCH 3/3] nvme: bio remapping tracepoint Hannes Reinecke 2018-06-07 8:41 ` Sagi Grimberg @ 2018-06-07 11:27 ` Christoph Hellwig 2018-06-07 11:33 ` Hannes Reinecke 1 sibling, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2018-06-07 11:27 UTC (permalink / raw) On Thu, Jun 07, 2018@10:38:47AM +0200, Hannes Reinecke wrote: > Adding a tracepoint to trace bio remapping for native nvme multipath. I guess we should just move the trace point into direct_make_request? Otherwise this looks sane to me. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] nvme: bio remapping tracepoint 2018-06-07 11:27 ` Christoph Hellwig @ 2018-06-07 11:33 ` Hannes Reinecke 2018-06-07 11:42 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Hannes Reinecke @ 2018-06-07 11:33 UTC (permalink / raw) On Thu, 7 Jun 2018 13:27:59 +0200 Christoph Hellwig <hch@lst.de> wrote: > On Thu, Jun 07, 2018@10:38:47AM +0200, Hannes Reinecke wrote: > > Adding a tracepoint to trace bio remapping for native nvme > > multipath. > > I guess we should just move the trace point into direct_make_request? > > Otherwise this looks sane to me. That would run afoul with device-mapper usage: case DM_MAPIO_REMAPPED: /* the bio has been remapped so dispatch it */ trace_block_bio_remap(clone->bi_disk->queue, clone, bio_dev(io->orig_bio), sector); if (md->type == DM_TYPE_NVME_BIO_BASED) ret = direct_make_request(clone); else ret = generic_make_request(clone); break; and we can't move the call into generic_make_request(). So I fear it need to stay there. Cheers, Hannes ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] nvme: bio remapping tracepoint 2018-06-07 11:33 ` Hannes Reinecke @ 2018-06-07 11:42 ` Christoph Hellwig 2018-06-07 16:21 ` Jens Axboe 0 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2018-06-07 11:42 UTC (permalink / raw) On Thu, Jun 07, 2018@01:33:32PM +0200, Hannes Reinecke wrote: > That would run afoul with device-mapper usage: > > case DM_MAPIO_REMAPPED: > /* the bio has been remapped so dispatch it */ > trace_block_bio_remap(clone->bi_disk->queue, clone, > bio_dev(io->orig_bio), sector); > if (md->type == DM_TYPE_NVME_BIO_BASED) > ret = direct_make_request(clone); > else > ret = generic_make_request(clone); > break; > > and we can't move the call into generic_make_request(). > So I fear it need to stay there. Or you could move it from dm to where it belongs.. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] nvme: bio remapping tracepoint 2018-06-07 11:42 ` Christoph Hellwig @ 2018-06-07 16:21 ` Jens Axboe 2018-06-11 14:26 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Jens Axboe @ 2018-06-07 16:21 UTC (permalink / raw) On 6/7/18 5:42 AM, Christoph Hellwig wrote: > On Thu, Jun 07, 2018@01:33:32PM +0200, Hannes Reinecke wrote: >> That would run afoul with device-mapper usage: >> >> case DM_MAPIO_REMAPPED: >> /* the bio has been remapped so dispatch it */ >> trace_block_bio_remap(clone->bi_disk->queue, clone, >> bio_dev(io->orig_bio), sector); >> if (md->type == DM_TYPE_NVME_BIO_BASED) >> ret = direct_make_request(clone); >> else >> ret = generic_make_request(clone); >> break; >> >> and we can't move the call into generic_make_request(). >> So I fear it need to stay there. > > Or you could move it from dm to where it belongs.. I was going to suggest that too, but there's really nothing in direct_make_request() that implies that it's necessarily a remap of an existing bio. The two current callers implies that, but that's really not a given. So maybe it'd be better to let it live in the caller. -- Jens Axboe ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] nvme: bio remapping tracepoint 2018-06-07 16:21 ` Jens Axboe @ 2018-06-11 14:26 ` Christoph Hellwig 0 siblings, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2018-06-11 14:26 UTC (permalink / raw) On Thu, Jun 07, 2018@10:21:42AM -0600, Jens Axboe wrote: > I was going to suggest that too, but there's really nothing > in direct_make_request() that implies that it's necessarily > a remap of an existing bio. The two current callers implies > that, but that's really not a given. I don't really think it makes much sense in a different context. I've applied the patch for now, but I think this area could use ?ome improvement. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-06-12 21:20 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-07 8:38 [PATCH 0/3] misc nvme fixes Hannes Reinecke 2018-06-07 8:38 ` [PATCH 1/3] nvme: also check for RESETTING state in nvmf_check_if_ready() Hannes Reinecke 2018-06-07 8:41 ` Sagi Grimberg 2018-06-07 11:26 ` Christoph Hellwig 2018-06-12 21:20 ` James Smart 2018-06-07 8:38 ` [PATCH 2/3] nvme: do not access request after calling blk_mq_end_request() Hannes Reinecke 2018-06-07 8:42 ` Sagi Grimberg 2018-06-07 11:27 ` Christoph Hellwig 2018-06-07 8:38 ` [PATCH 3/3] nvme: bio remapping tracepoint Hannes Reinecke 2018-06-07 8:41 ` Sagi Grimberg 2018-06-07 11:27 ` Christoph Hellwig 2018-06-07 11:33 ` Hannes Reinecke 2018-06-07 11:42 ` Christoph Hellwig 2018-06-07 16:21 ` Jens Axboe 2018-06-11 14:26 ` Christoph Hellwig
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.