misc nvme fixes

PATCH New Hannes Reinecke 5 participants 15 messages Jun 7, 2018 – Jun 12, 2018 NVMe

lore

Share & tools

lore

Hannes Reinecke

author

2018-06-07 08:38 UTC #0 lore raw

cover misc nvme fixes

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(-)

Signature · 1 line

2.12.3

Hannes Reinecke

author

2018-06-07 08:38 UTC #1 lore raw

PATCH 1/3 nvme: also check for RESETTING state in nvmf_check_if_ready()

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(+)

drivers/nvme/host/fabrics.c +1 -0

index fa32c1216409..909dd337221a 100644
@@ -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

Signature · 1 line

2.12.3

Sagi Grimberg

maintainer

2018-06-07 08:41 UTC #2 lore raw

PATCH 1/3 nvme: also check for RESETTING state in nvmf_check_if_ready()

Thank you Hannes - just spotted that...

Quoted 14 lines · Hannes Reinecke

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(+)
drivers/nvme/host/fabrics.c +0 -0
index fa32c1216409..909dd337221a 100644

drivers/nvme/host/fabrics.c blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, · line 551

Quoted 4 lines

drivers/nvme/host/fabrics.c +1 -0

@@ -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.

drivers/nvme/host/fabrics.c blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, · line 554

Quoted 3 lines

drivers/nvme/host/fabrics.c +0 -0

  		/*
  		 * 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)?

Christoph Hellwig

maintainer

2018-06-07 11:26 UTC #3 lore raw

PATCH 1/3 nvme: also check for RESETTING state in nvmf_check_if_ready()

Quoted 12 lines · Sagi Grimberg

Thank you Hannes - just spotted that...
Quoted 9 lines · Hannes Reinecke
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(+)

Quoted 11 lines

drivers/nvme/host/fabrics.c blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, · line 551
Quoted 9 lines
drivers/nvme/host/fabrics.c +1 -0
index fa32c1216409..909dd337221a 100644
@@ -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.

Quoted 7 lines

drivers/nvme/host/fabrics.c blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, · line 554
Quoted 4 lines
drivers/nvme/host/fabrics.c +0 -0
  		/*
  		 * 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..

James Smart

2018-06-12 21:20 UTC #4 lore raw

PATCH 1/3 nvme: also check for RESETTING state in nvmf_check_if_ready()

Quoted 7 lines · Christoph Hellwig

Quoted 6 lines · Sagi Grimberg
Thank you Hannes - just spotted that...
Quoted 3 lines · Hannes Reinecke
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.

Quoted 16 lines

Quoted 6 lines
Quoted 6 lines
Signed-off-by Hannes Reinecke <hare at suse.com>
---
   drivers/nvme/host/fabrics.c | 1 +
   1 file changed, 1 insertion(+)
Quoted 10 lines
drivers/nvme/host/fabrics.c blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, · line 551
Quoted 9 lines
drivers/nvme/host/fabrics.c +1 -0
index fa32c1216409..909dd337221a 100644
@@ -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.

Quoted 1 line

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.

Quoted 9 lines

Quoted 6 lines
drivers/nvme/host/fabrics.c blk_status_t nvmf_check_if_ready(struct nvme_ctrl *ctrl, struct request *rq, · line 554
Quoted 4 lines
drivers/nvme/host/fabrics.c +0 -0
   		/*
   		 * 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

Hannes Reinecke

author

2018-06-07 08:38 UTC #5 lore raw

PATCH 2/3 nvme: do not access request after calling blk_mq_end_request()

In reply to Hannes Reinecke

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(-)

drivers/nvme/host/multipath.c +2 -1

index 7f34ae260ca9..87bd60b49bbc 100644
@@ -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);

Signature · 1 line

2.12.3

Sagi Grimberg

maintainer

2018-06-07 08:42 UTC #6 lore raw

PATCH 2/3 nvme: do not access request after calling blk_mq_end_request()

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Christoph Hellwig

maintainer

2018-06-07 11:27 UTC #7 lore raw

PATCH 2/3 nvme: do not access request after calling blk_mq_end_request()

In reply to Hannes Reinecke

Quoted 4 lines · Hannes Reinecke

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..

Hannes Reinecke

author

2018-06-07 08:38 UTC #8 lore raw

PATCH 3/3 nvme: bio remapping tracepoint

In reply to Hannes Reinecke

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(+)

drivers/nvme/host/multipath.c +4 -0

index 87bd60b49bbc..620bcd749912 100644
@@ -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");

Signature · 1 line

2.12.3

Sagi Grimberg

maintainer

2018-06-07 08:41 UTC #9 lore raw

PATCH 3/3 nvme: bio remapping tracepoint

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Christoph Hellwig

maintainer

2018-06-07 11:27 UTC #10 lore raw

PATCH 3/3 nvme: bio remapping tracepoint

In reply to Hannes Reinecke

Quoted 1 line · Hannes Reinecke

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.

Hannes Reinecke

author

2018-06-07 11:33 UTC #11 lore raw

PATCH 3/3 nvme: bio remapping tracepoint

Quoted 7 lines · On Thu, 7 Jun 2018 13:27:59 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 2 lines · Hannes Reinecke
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

Christoph Hellwig

maintainer

2018-06-07 11:42 UTC #12 lore raw

PATCH 3/3 nvme: bio remapping tracepoint

Quoted 12 lines · Hannes Reinecke

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;

Quoted 2 lines

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..

Jens Axboe

maintainer

2018-06-07 16:21 UTC #13 lore raw

PATCH 3/3 nvme: bio remapping tracepoint

Quoted 13 lines · Christoph Hellwig

Quoted 12 lines · Hannes Reinecke

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;

Quoted 4 lines

Quoted 2 lines
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.

Signature · 1 line

Jens Axboe

Christoph Hellwig

maintainer

2018-06-11 14:26 UTC #14 lore raw

PATCH 3/3 nvme: bio remapping tracepoint

Quoted 4 lines · Jens Axboe

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.