Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1735700 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] nvme: make controller 'state' sysfs attribute pollable Johannes Thumshirn <jthumshirn@suse.de> - 2017-09-20 12:50 +0200
    Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Sagi Grimberg <sagi@grimberg.me> - 2017-09-20 12:50 +0200
    Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Christoph Hellwig <hch@lst.de> - 2017-09-20 17:00 +0200
      Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Johannes Thumshirn <jthumshirn@suse.de> - 2017-09-21 07:20 +0200
        Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Christoph Hellwig <hch@lst.de> - 2017-09-21 16:20 +0200
        Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Sagi Grimberg <sagi@grimberg.me> - 2017-09-25 07:40 +0200
          Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Hannes Reinecke <hare@suse.de> - 2017-09-25 08:00 +0200
            Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Sagi Grimberg <sagi@grimberg.me> - 2017-09-25 09:10 +0200
              Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Hannes Reinecke <hare@suse.de> - 2017-09-25 10:30 +0200
                Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Christoph Hellwig <hch@lst.de> - 2017-09-25 15:00 +0200
      Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Guan Junxiong <guanjunxiong@huawei.com> - 2017-09-21 14:50 +0200
        Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable Sagi Grimberg <sagi@grimberg.me> - 2017-09-25 07:40 +0200

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

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

[toc] | [next] | [standalone]

#1735702

Looks good,

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

[toc] | [prev] | [next] | [standalone]

#1735811

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.

[toc] | [prev] | [next] | [standalone]

#1736362

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

[toc] | [prev] | [next] | [standalone]

#1736700

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.

[toc] | [prev] | [next] | [standalone]

#1738749

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

[toc] | [prev] | [next] | [standalone]

#1738752

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)

[toc] | [prev] | [next] | [standalone]

#1738777

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

[toc] | [prev] | [next] | [standalone]

#1738824

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)

[toc] | [prev] | [next] | [standalone]

#1739032

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.

[toc] | [prev] | [next] | [standalone]

#1736635

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

[toc] | [prev] | [next] | [standalone]

#1738748

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web