[PATCH] nvme: make controller 'state' sysfs attribute pollable

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Johannes Thumshirn]

Notify sysfs about changes of a nvme controller so user-space can watch the
file via poll() or select() in order to react to a state change.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/nvme/host/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index acc816b67582..064d973f1e22 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -237,8 +237,10 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		break;
 	}
 
-	if (changed)
+	if (changed) {
 		ctrl->state = new_state;
+		sysfs_notify(&ctrl->dev->kobj, NULL, "state");
+	}
 
 	spin_unlock_irqrestore(&ctrl->lock, flags);
 
-- 
2.13.5

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Johannes Thumshirn]

Notify sysfs about changes of a nvme controller so user-space can watch the
file via poll() or select() in order to react to a state change.

Signed-off-by: Johannes Thumshirn <jthumshirn at suse.de>
---
 drivers/nvme/host/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index acc816b67582..064d973f1e22 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -237,8 +237,10 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		break;
 	}
 
-	if (changed)
+	if (changed) {
 		ctrl->state = new_state;
+		sysfs_notify(&ctrl->dev->kobj, NULL, "state");
+	}
 
 	spin_unlock_irqrestore(&ctrl->lock, flags);
 
-- 
2.13.5

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Sagi Grimberg]

Looks good,

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

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Sagi Grimberg]

Looks good,

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

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Christoph Hellwig]

On Wed, Sep 20, 2017 at 12:40:32PM +0200, Johannes Thumshirn wrote:
> Notify sysfs about changes of a nvme controller so user-space can watch the
> file via poll() or select() in order to react to a state change.

Userspace has no business polling for the state.

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Christoph Hellwig]

On Wed, Sep 20, 2017@12:40:32PM +0200, Johannes Thumshirn wrote:
> Notify sysfs about changes of a nvme controller so user-space can watch the
> file via poll() or select() in order to react to a state change.

Userspace has no business polling for the state.

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Johannes Thumshirn]

On Wed, Sep 20, 2017 at 04:59:31PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 20, 2017 at 12:40:32PM +0200, Johannes Thumshirn wrote:
> > Notify sysfs about changes of a nvme controller so user-space can watch the
> > file via poll() or select() in order to react to a state change.
> 
> Userspace has no business polling for the state.

So why exposing it then in the first time? I know you don't want dm-mpath in
NVMe (neither do I) but we have to have something until your patchset and ANA
is merged. And with this patch it's trivial to build a path checker that just
looks at the state attribute in sysfs.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Johannes Thumshirn]

On Wed, Sep 20, 2017@04:59:31PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 20, 2017@12:40:32PM +0200, Johannes Thumshirn wrote:
> > Notify sysfs about changes of a nvme controller so user-space can watch the
> > file via poll() or select() in order to react to a state change.
> 
> Userspace has no business polling for the state.

So why exposing it then in the first time? I know you don't want dm-mpath in
NVMe (neither do I) but we have to have something until your patchset and ANA
is merged. And with this patch it's trivial to build a path checker that just
looks at the state attribute in sysfs.

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Christoph Hellwig]

On Thu, Sep 21, 2017 at 07:19:15AM +0200, Johannes Thumshirn wrote:
> So why exposing it then in the first time?

It is a really nice debug aid, but the states really are an internal
detail of the implementation, and can (and probably will soon, see the
fc states discussion) change.

Maybe we need to move things like this to debugfs, but yet another
interface seems a little annoying.

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Christoph Hellwig]

On Thu, Sep 21, 2017@07:19:15AM +0200, Johannes Thumshirn wrote:
> So why exposing it then in the first time?

It is a really nice debug aid, but the states really are an internal
detail of the implementation, and can (and probably will soon, see the
fc states discussion) change.

Maybe we need to move things like this to debugfs, but yet another
interface seems a little annoying.

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Sagi Grimberg]

> So why exposing it then in the first time? I know you don't want dm-mpath in
> NVMe (neither do I) but we have to have something until your patchset and ANA
> is merged. And with this patch it's trivial to build a path checker that just
> looks at the state attribute in sysfs.

Can't we just not use path-checkers for nvme (we already have one in
nvme)?

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Sagi Grimberg]

> So why exposing it then in the first time? I know you don't want dm-mpath in
> NVMe (neither do I) but we have to have something until your patchset and ANA
> is merged. And with this patch it's trivial to build a path checker that just
> looks at the state attribute in sysfs.

Can't we just not use path-checkers for nvme (we already have one in
nvme)?

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Hannes Reinecke]

On 09/25/2017 07:37 AM, Sagi Grimberg wrote:
> 
>> So why exposing it then in the first time? I know you don't want
>> dm-mpath in
>> NVMe (neither do I) but we have to have something until your patchset
>> and ANA
>> is merged. And with this patch it's trivial to build a path checker
>> that just
>> looks at the state attribute in sysfs.
> 
> Can't we just not use path-checkers for nvme (we already have one in
> nvme)?

Really? For NVMe?
How would you do that, then?

Anyway: the entire point is that you don't _need_ a path checker for NVMe.
The primary reason for path checkers is to check with the transport
layer if the remote endpoint is reachable.
(I know, that's not quite what they're doing now, but that's beside the
point).
For NVMf we do have KATO, so the NVMe subsystem knows exactly if the
connection is live or not. So it should be perfectly sufficient to check
the connection state instead of running a path checker of sorts.
But for doing so we need something in sysfs which we could check.

Mind you, I wouldn't be adverse to have some common sysfs attribute,
with some common values (eg path up, path down, path blocked), and have
NVMf translating the internal state into that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Hannes Reinecke]

On 09/25/2017 07:37 AM, Sagi Grimberg wrote:
> 
>> So why exposing it then in the first time? I know you don't want
>> dm-mpath in
>> NVMe (neither do I) but we have to have something until your patchset
>> and ANA
>> is merged. And with this patch it's trivial to build a path checker
>> that just
>> looks at the state attribute in sysfs.
> 
> Can't we just not use path-checkers for nvme (we already have one in
> nvme)?

Really? For NVMe?
How would you do that, then?

Anyway: the entire point is that you don't _need_ a path checker for NVMe.
The primary reason for path checkers is to check with the transport
layer if the remote endpoint is reachable.
(I know, that's not quite what they're doing now, but that's beside the
point).
For NVMf we do have KATO, so the NVMe subsystem knows exactly if the
connection is live or not. So it should be perfectly sufficient to check
the connection state instead of running a path checker of sorts.
But for doing so we need something in sysfs which we could check.

Mind you, I wouldn't be adverse to have some common sysfs attribute,
with some common values (eg path up, path down, path blocked), and have
NVMf translating the internal state into that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Sagi Grimberg]

On 25/09/17 08:59, Hannes Reinecke wrote:
> On 09/25/2017 07:37 AM, Sagi Grimberg wrote:
>>
>>> So why exposing it then in the first time? I know you don't want
>>> dm-mpath in
>>> NVMe (neither do I) but we have to have something until your patchset
>>> and ANA
>>> is merged. And with this patch it's trivial to build a path checker
>>> that just
>>> looks at the state attribute in sysfs.
>>
>> Can't we just not use path-checkers for nvme (we already have one in
>> nvme)?
> 
> Really? For NVMe?
> How would you do that, then?

Quick and dirty is to have a path-checker that returns path-up always,
when the path go down, nvme will detect it and fast-fail the io.

> Anyway: the entire point is that you don't _need_ a path checker for NVMe.
> The primary reason for path checkers is to check with the transport
> layer if the remote endpoint is reachable.
> (I know, that's not quite what they're doing now, but that's beside the
> point).
> For NVMf we do have KATO, so the NVMe subsystem knows exactly if the
> connection is live or not. So it should be perfectly sufficient to check
> the connection state instead of running a path checker of sorts.
> But for doing so we need something in sysfs which we could check.
> 
> Mind you, I wouldn't be adverse to have some common sysfs attribute,
> with some common values (eg path up, path down, path blocked), and have
> NVMf translating the internal state into that.

We could have such an interface I assume. But it would suck to maintain
yet another state (we are already having enough trouble to have a
coherent controller state machine).

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Sagi Grimberg]

On 25/09/17 08:59, Hannes Reinecke wrote:
> On 09/25/2017 07:37 AM, Sagi Grimberg wrote:
>>
>>> So why exposing it then in the first time? I know you don't want
>>> dm-mpath in
>>> NVMe (neither do I) but we have to have something until your patchset
>>> and ANA
>>> is merged. And with this patch it's trivial to build a path checker
>>> that just
>>> looks at the state attribute in sysfs.
>>
>> Can't we just not use path-checkers for nvme (we already have one in
>> nvme)?
> 
> Really? For NVMe?
> How would you do that, then?

Quick and dirty is to have a path-checker that returns path-up always,
when the path go down, nvme will detect it and fast-fail the io.

> Anyway: the entire point is that you don't _need_ a path checker for NVMe.
> The primary reason for path checkers is to check with the transport
> layer if the remote endpoint is reachable.
> (I know, that's not quite what they're doing now, but that's beside the
> point).
> For NVMf we do have KATO, so the NVMe subsystem knows exactly if the
> connection is live or not. So it should be perfectly sufficient to check
> the connection state instead of running a path checker of sorts.
> But for doing so we need something in sysfs which we could check.
> 
> Mind you, I wouldn't be adverse to have some common sysfs attribute,
> with some common values (eg path up, path down, path blocked), and have
> NVMf translating the internal state into that.

We could have such an interface I assume. But it would suck to maintain
yet another state (we are already having enough trouble to have a
coherent controller state machine).

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Hannes Reinecke]

On 09/25/2017 09:09 AM, Sagi Grimberg wrote:
> 
> 
> On 25/09/17 08:59, Hannes Reinecke wrote:
>> On 09/25/2017 07:37 AM, Sagi Grimberg wrote:
>>>
>>>> So why exposing it then in the first time? I know you don't want
>>>> dm-mpath in
>>>> NVMe (neither do I) but we have to have something until your patchset
>>>> and ANA
>>>> is merged. And with this patch it's trivial to build a path checker
>>>> that just
>>>> looks at the state attribute in sysfs.
>>>
>>> Can't we just not use path-checkers for nvme (we already have one in
>>> nvme)?
>>
>> Really? For NVMe?
>> How would you do that, then?
> 
> Quick and dirty is to have a path-checker that returns path-up always,
> when the path go down, nvme will detect it and fast-fail the io.
> 
Well, yes; that's the trivial part.
But how do you know the path has become LIVE again?

>> Anyway: the entire point is that you don't _need_ a path checker for
>> NVMe.
>> The primary reason for path checkers is to check with the transport
>> layer if the remote endpoint is reachable.
>> (I know, that's not quite what they're doing now, but that's beside the
>> point).
>> For NVMf we do have KATO, so the NVMe subsystem knows exactly if the
>> connection is live or not. So it should be perfectly sufficient to check
>> the connection state instead of running a path checker of sorts.
>> But for doing so we need something in sysfs which we could check.
>>
>> Mind you, I wouldn't be adverse to have some common sysfs attribute,
>> with some common values (eg path up, path down, path blocked), and have
>> NVMf translating the internal state into that.
> 
> We could have such an interface I assume. But it would suck to maintain
> yet another state (we are already having enough trouble to have a
> coherent controller state machine).

Weell ... we could be using a notifier chain for that.
I think I should post my patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Hannes Reinecke]

On 09/25/2017 09:09 AM, Sagi Grimberg wrote:
> 
> 
> On 25/09/17 08:59, Hannes Reinecke wrote:
>> On 09/25/2017 07:37 AM, Sagi Grimberg wrote:
>>>
>>>> So why exposing it then in the first time? I know you don't want
>>>> dm-mpath in
>>>> NVMe (neither do I) but we have to have something until your patchset
>>>> and ANA
>>>> is merged. And with this patch it's trivial to build a path checker
>>>> that just
>>>> looks at the state attribute in sysfs.
>>>
>>> Can't we just not use path-checkers for nvme (we already have one in
>>> nvme)?
>>
>> Really? For NVMe?
>> How would you do that, then?
> 
> Quick and dirty is to have a path-checker that returns path-up always,
> when the path go down, nvme will detect it and fast-fail the io.
> 
Well, yes; that's the trivial part.
But how do you know the path has become LIVE again?

>> Anyway: the entire point is that you don't _need_ a path checker for
>> NVMe.
>> The primary reason for path checkers is to check with the transport
>> layer if the remote endpoint is reachable.
>> (I know, that's not quite what they're doing now, but that's beside the
>> point).
>> For NVMf we do have KATO, so the NVMe subsystem knows exactly if the
>> connection is live or not. So it should be perfectly sufficient to check
>> the connection state instead of running a path checker of sorts.
>> But for doing so we need something in sysfs which we could check.
>>
>> Mind you, I wouldn't be adverse to have some common sysfs attribute,
>> with some common values (eg path up, path down, path blocked), and have
>> NVMf translating the internal state into that.
> 
> We could have such an interface I assume. But it would suck to maintain
> yet another state (we are already having enough trouble to have a
> coherent controller state machine).

Weell ... we could be using a notifier chain for that.
I think I should post my patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Christoph Hellwig]

On Mon, Sep 25, 2017 at 10:23:16AM +0200, Hannes Reinecke wrote:
> > We could have such an interface I assume. But it would suck to maintain
> > yet another state (we are already having enough trouble to have a
> > coherent controller state machine).
> 
> Weell ... we could be using a notifier chain for that.
> I think I should post my patchset.

The point is: if you want proper multipath behavior use the nvme
in-kernel multipath code.  If you want to weird crap in userspace don't
expect any help from the kernel driver.

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Christoph Hellwig]

On Mon, Sep 25, 2017@10:23:16AM +0200, Hannes Reinecke wrote:
> > We could have such an interface I assume. But it would suck to maintain
> > yet another state (we are already having enough trouble to have a
> > coherent controller state machine).
> 
> Weell ... we could be using a notifier chain for that.
> I think I should post my patchset.

The point is: if you want proper multipath behavior use the nvme
in-kernel multipath code.  If you want to weird crap in userspace don't
expect any help from the kernel driver.

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Guan Junxiong]

On 2017/9/20 22:59, Christoph Hellwig wrote:
> On Wed, Sep 20, 2017 at 12:40:32PM +0200, Johannes Thumshirn wrote:
>> Notify sysfs about changes of a nvme controller so user-space can watch the
>> file via poll() or select() in order to react to a state change.
> 
> Userspace has no business polling for the state.
> 

Please consider this patch. At least upstream multipath-tools is using the sysfs state now:
[1] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commitdiff;h=29c3b0446c4d919859f9e87b291563d483aab594
[2] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commitdiff;h=d2561442cc0b444e8a728bac2c1466468816ee9d

> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
> 
> 

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Guan Junxiong]

On 2017/9/20 22:59, Christoph Hellwig wrote:
> On Wed, Sep 20, 2017@12:40:32PM +0200, Johannes Thumshirn wrote:
>> Notify sysfs about changes of a nvme controller so user-space can watch the
>> file via poll() or select() in order to react to a state change.
> 
> Userspace has no business polling for the state.
> 

Please consider this patch. At least upstream multipath-tools is using the sysfs state now:
[1] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commitdiff;h=29c3b0446c4d919859f9e87b291563d483aab594
[2] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commitdiff;h=d2561442cc0b444e8a728bac2c1466468816ee9d

> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
> 
> 

Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable

[Sagi Grimberg]

>>> Notify sysfs about changes of a nvme controller so user-space can watch the
>>> file via poll() or select() in order to react to a state change.
>>
>> Userspace has no business polling for the state.
>>
> 
> Please consider this patch. At least upstream multipath-tools is using the sysfs state now:
> [1] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commitdiff;h=29c3b0446c4d919859f9e87b291563d483aab594
> [2] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commitdiff;h=d2561442cc0b444e8a728bac2c1466468816ee9d

I have to agree with Christoph that we need to be able to keep the
controller states internal as they are bound to change at any point.

We do need to move into debugfs to avoid the confusion...

[PATCH] nvme: make controller 'state' sysfs attribute pollable

[Sagi Grimberg]

>>> Notify sysfs about changes of a nvme controller so user-space can watch the
>>> file via poll() or select() in order to react to a state change.
>>
>> Userspace has no business polling for the state.
>>
> 
> Please consider this patch. At least upstream multipath-tools is using the sysfs state now:
> [1] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commitdiff;h=29c3b0446c4d919859f9e87b291563d483aab594
> [2] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=commitdiff;h=d2561442cc0b444e8a728bac2c1466468816ee9d

I have to agree with Christoph that we need to be able to keep the
controller states internal as they are bound to change at any point.

We do need to move into debugfs to avoid the confusion...