* [PATCH] nvme: make controller 'state' sysfs attribute pollable @ 2017-09-20 10:40 Johannes Thumshirn 2017-09-20 10:49 ` Sagi Grimberg 2017-09-20 14:59 ` Christoph Hellwig 0 siblings, 2 replies; 12+ messages in thread From: Johannes Thumshirn @ 2017-09-20 10:40 UTC (permalink / raw) To: Christoph Hellwig, Sagi Grimberg, Keith Busch Cc: Hannes Reinecke, Linux NVMe Mailinglist, Linux Kernel Mailinglist, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-20 10:40 [PATCH] nvme: make controller 'state' sysfs attribute pollable Johannes Thumshirn @ 2017-09-20 10:49 ` Sagi Grimberg 2017-09-20 14:59 ` Christoph Hellwig 1 sibling, 0 replies; 12+ messages in thread From: Sagi Grimberg @ 2017-09-20 10:49 UTC (permalink / raw) To: Johannes Thumshirn, Christoph Hellwig, Keith Busch Cc: Hannes Reinecke, Linux NVMe Mailinglist, Linux Kernel Mailinglist Looks good, Reviewed-by: Sagi Grimberg <sagi@grimberg.me> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-20 10:40 [PATCH] nvme: make controller 'state' sysfs attribute pollable Johannes Thumshirn 2017-09-20 10:49 ` Sagi Grimberg @ 2017-09-20 14:59 ` Christoph Hellwig 2017-09-21 5:19 ` Johannes Thumshirn 2017-09-21 12:47 ` Guan Junxiong 1 sibling, 2 replies; 12+ messages in thread From: Christoph Hellwig @ 2017-09-20 14:59 UTC (permalink / raw) To: Johannes Thumshirn Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Hannes Reinecke, Linux NVMe Mailinglist, Linux Kernel Mailinglist 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-20 14:59 ` Christoph Hellwig @ 2017-09-21 5:19 ` Johannes Thumshirn 2017-09-21 14:10 ` Christoph Hellwig 2017-09-25 5:37 ` Sagi Grimberg 2017-09-21 12:47 ` Guan Junxiong 1 sibling, 2 replies; 12+ messages in thread From: Johannes Thumshirn @ 2017-09-21 5:19 UTC (permalink / raw) To: Christoph Hellwig Cc: Sagi Grimberg, Keith Busch, Hannes Reinecke, Linux NVMe Mailinglist, Linux Kernel Mailinglist 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-21 5:19 ` Johannes Thumshirn @ 2017-09-21 14:10 ` Christoph Hellwig 2017-09-25 5:37 ` Sagi Grimberg 1 sibling, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2017-09-21 14:10 UTC (permalink / raw) To: Johannes Thumshirn Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Hannes Reinecke, Linux NVMe Mailinglist, Linux Kernel Mailinglist 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-21 5:19 ` Johannes Thumshirn 2017-09-21 14:10 ` Christoph Hellwig @ 2017-09-25 5:37 ` Sagi Grimberg 2017-09-25 5:59 ` Hannes Reinecke 1 sibling, 1 reply; 12+ messages in thread From: Sagi Grimberg @ 2017-09-25 5:37 UTC (permalink / raw) To: Johannes Thumshirn, Christoph Hellwig Cc: Keith Busch, Hannes Reinecke, Linux NVMe Mailinglist, Linux Kernel Mailinglist > 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)? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-25 5:37 ` Sagi Grimberg @ 2017-09-25 5:59 ` Hannes Reinecke 2017-09-25 7:09 ` Sagi Grimberg 0 siblings, 1 reply; 12+ messages in thread From: Hannes Reinecke @ 2017-09-25 5:59 UTC (permalink / raw) To: Sagi Grimberg, Johannes Thumshirn, Christoph Hellwig Cc: Keith Busch, Linux NVMe Mailinglist, Linux Kernel Mailinglist 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-25 5:59 ` Hannes Reinecke @ 2017-09-25 7:09 ` Sagi Grimberg 2017-09-25 8:23 ` Hannes Reinecke 0 siblings, 1 reply; 12+ messages in thread From: Sagi Grimberg @ 2017-09-25 7:09 UTC (permalink / raw) To: Hannes Reinecke, Johannes Thumshirn, Christoph Hellwig Cc: Keith Busch, Linux NVMe Mailinglist, Linux Kernel Mailinglist 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). ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-25 7:09 ` Sagi Grimberg @ 2017-09-25 8:23 ` Hannes Reinecke 2017-09-25 12:52 ` Christoph Hellwig 0 siblings, 1 reply; 12+ messages in thread From: Hannes Reinecke @ 2017-09-25 8:23 UTC (permalink / raw) To: Sagi Grimberg, Johannes Thumshirn, Christoph Hellwig Cc: Keith Busch, Linux NVMe Mailinglist, Linux Kernel Mailinglist 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-25 8:23 ` Hannes Reinecke @ 2017-09-25 12:52 ` Christoph Hellwig 0 siblings, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2017-09-25 12:52 UTC (permalink / raw) To: Hannes Reinecke Cc: Sagi Grimberg, Johannes Thumshirn, Christoph Hellwig, Keith Busch, Linux NVMe Mailinglist, Linux Kernel Mailinglist 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-20 14:59 ` Christoph Hellwig 2017-09-21 5:19 ` Johannes Thumshirn @ 2017-09-21 12:47 ` Guan Junxiong 2017-09-25 5:36 ` Sagi Grimberg 1 sibling, 1 reply; 12+ messages in thread From: Guan Junxiong @ 2017-09-21 12:47 UTC (permalink / raw) To: Christoph Hellwig, Johannes Thumshirn Cc: Sagi Grimberg, Linux Kernel Mailinglist, Linux NVMe Mailinglist, Keith Busch, Hannes Reinecke, Shenhong (C), niuhaoxin 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvme: make controller 'state' sysfs attribute pollable 2017-09-21 12:47 ` Guan Junxiong @ 2017-09-25 5:36 ` Sagi Grimberg 0 siblings, 0 replies; 12+ messages in thread From: Sagi Grimberg @ 2017-09-25 5:36 UTC (permalink / raw) To: Guan Junxiong, Christoph Hellwig, Johannes Thumshirn Cc: Linux Kernel Mailinglist, Linux NVMe Mailinglist, Keith Busch, Hannes Reinecke, Shenhong (C), niuhaoxin >>> 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... ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-09-25 12:52 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-20 10:40 [PATCH] nvme: make controller 'state' sysfs attribute pollable Johannes Thumshirn 2017-09-20 10:49 ` Sagi Grimberg 2017-09-20 14:59 ` Christoph Hellwig 2017-09-21 5:19 ` Johannes Thumshirn 2017-09-21 14:10 ` Christoph Hellwig 2017-09-25 5:37 ` Sagi Grimberg 2017-09-25 5:59 ` Hannes Reinecke 2017-09-25 7:09 ` Sagi Grimberg 2017-09-25 8:23 ` Hannes Reinecke 2017-09-25 12:52 ` Christoph Hellwig 2017-09-21 12:47 ` Guan Junxiong 2017-09-25 5:36 ` Sagi Grimberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).