[PATCH 0/3] misc nvme fixes

[PATCH 0/3] misc nvme fixes

[Hannes Reinecke]

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

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

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

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

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

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

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

[PATCH 3/3] nvme: bio remapping tracepoint

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

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

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

[Sagi Grimberg]

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

[PATCH 3/3] nvme: bio remapping tracepoint

[Sagi Grimberg]

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

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

[Sagi Grimberg]

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

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

[Christoph Hellwig]

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

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

[Christoph Hellwig]

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

[PATCH 3/3] nvme: bio remapping tracepoint

[Christoph Hellwig]

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.

[PATCH 3/3] nvme: bio remapping tracepoint

[Hannes Reinecke]

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

[PATCH 3/3] nvme: bio remapping tracepoint

[Christoph Hellwig]

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

[PATCH 3/3] nvme: bio remapping tracepoint

[Jens Axboe]

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

[PATCH 3/3] nvme: bio remapping tracepoint

[Christoph Hellwig]

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.

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

[James Smart]

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