[PATCH 1/2] nvmet: Add fabrics ops to port

[PATCH 1/2] nvmet: Add fabrics ops to port

[Israel Rukshin]

Instead of getting the nvmet_fabrics_ops from nvmet_transports array
each time we want to use it, just take it directly from the port.

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/core.c  | 9 ++++-----
 drivers/nvme/target/nvmet.h | 1 +
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index a78029e..cd97ec93 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -189,21 +189,20 @@ int nvmet_enable_port(struct nvmet_port *port)
 		return ret;
 	}
 
+	port->ops = ops;
 	port->enabled = true;
 	return 0;
 }
 
 void nvmet_disable_port(struct nvmet_port *port)
 {
-	struct nvmet_fabrics_ops *ops;
-
 	lockdep_assert_held(&nvmet_config_sem);
 
 	port->enabled = false;
 
-	ops = nvmet_transports[port->disc_addr.trtype];
-	ops->remove_port(port);
-	module_put(ops->owner);
+	port->ops->remove_port(port);
+	module_put(port->ops->owner);
+	port->ops = NULL;
 }
 
 static void nvmet_keep_alive_timer(struct work_struct *work)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 417f6c0..a7f2620 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -98,6 +98,7 @@ struct nvmet_port {
 	struct list_head		referrals;
 	void				*priv;
 	bool				enabled;
+	struct nvmet_fabrics_ops 	*ops;
 };
 
 static inline struct nvmet_port *to_nvmet_port(struct config_item *item)
-- 
1.8.3.1

[PATCH 2/2] nvmet: Add port transport active flag

[Israel Rukshin]

tractive port flag means that nvmet transport is active and ready for
receiving requests from host.
It differ from enabled port state (port with subsystem symbolic link)
which doesn't guarantee this.
The tractive flag is necessary in case the physical ports become down
while nvmet ports are enabled.
In this case the port state remains enabled but tractive flag becomes false.

The tractive flag will help the administrator to monitor the port state
from transport point of view.
The commit also add the ability to activate the port by simply writing
to tractive at the configfs.
e.g.: echo 1 > config/nvmet/ports/1/tractive

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/configfs.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     |  8 ++++++++
 drivers/nvme/target/nvmet.h    |  2 ++
 drivers/nvme/target/rdma.c     |  6 ++++++
 4 files changed, 52 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index e6b2d2a..840556a 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -268,6 +268,41 @@ static ssize_t nvmet_addr_trtype_store(struct config_item *item,
 
 CONFIGFS_ATTR(nvmet_, addr_trtype);
 
+static ssize_t nvmet_tractive_show(struct config_item *item, char *page)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+
+	return sprintf(page, "%d\n", nvmet_is_port_active(port));
+}
+
+static ssize_t nvmet_tractive_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+	bool active;
+	int ret = 0;
+
+	if (strtobool(page, &active))
+		return -EINVAL;
+
+	if (!active) {
+		pr_err("Deactivate the port is not allowed\n");
+		return -EINVAL;
+	}
+
+	down_write(&nvmet_config_sem);
+	if (!port->enabled) {
+		pr_err("Can't activate disabled port\n");
+		ret = -EINVAL;
+	} else if (!nvmet_is_port_active(port))
+		ret = port->ops->add_port(port);
+	up_write(&nvmet_config_sem);
+
+	return ret ? ret : count;
+}
+
+CONFIGFS_ATTR(nvmet_, tractive);
+
 /*
  * Namespace structures & file operation functions below
  */
@@ -873,6 +908,7 @@ static void nvmet_port_release(struct config_item *item)
 	&nvmet_attr_addr_traddr,
 	&nvmet_attr_addr_trsvcid,
 	&nvmet_attr_addr_trtype,
+	&nvmet_attr_tractive,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index cd97ec93..fa2a02c 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -205,6 +205,14 @@ void nvmet_disable_port(struct nvmet_port *port)
 	port->ops = NULL;
 }
 
+bool nvmet_is_port_active(struct nvmet_port *port)
+{
+	if (port->ops && port->ops->is_port_active)
+		return port->ops->is_port_active(port);
+
+	return port->enabled;
+}
+
 static void nvmet_keep_alive_timer(struct work_struct *work)
 {
 	struct nvmet_ctrl *ctrl = container_of(to_delayed_work(work),
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index a7f2620..1615ed4 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -209,6 +209,7 @@ struct nvmet_fabrics_ops {
 	void (*queue_response)(struct nvmet_req *req);
 	int (*add_port)(struct nvmet_port *port);
 	void (*remove_port)(struct nvmet_port *port);
+	bool (*is_port_active)(struct nvmet_port *port);
 	void (*delete_ctrl)(struct nvmet_ctrl *ctrl);
 };
 
@@ -307,6 +308,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 
 int nvmet_enable_port(struct nvmet_port *port);
 void nvmet_disable_port(struct nvmet_port *port);
+bool nvmet_is_port_active(struct nvmet_port *port);
 
 void nvmet_referral_enable(struct nvmet_port *parent, struct nvmet_port *port);
 void nvmet_referral_disable(struct nvmet_port *port);
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 210e4e3..1686b6c 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1436,6 +1436,11 @@ static void nvmet_rdma_remove_port(struct nvmet_port *port)
 		rdma_destroy_id(cm_id);
 }
 
+static bool nvmet_rdma_is_port_active(struct nvmet_port *port)
+{
+	return port->priv ? true : false;
+}
+
 static struct nvmet_fabrics_ops nvmet_rdma_ops = {
 	.owner			= THIS_MODULE,
 	.type			= NVMF_TRTYPE_RDMA,
@@ -1444,6 +1449,7 @@ static void nvmet_rdma_remove_port(struct nvmet_port *port)
 	.has_keyed_sgls		= 1,
 	.add_port		= nvmet_rdma_add_port,
 	.remove_port		= nvmet_rdma_remove_port,
+	.is_port_active		= nvmet_rdma_is_port_active,
 	.queue_response		= nvmet_rdma_queue_response,
 	.delete_ctrl		= nvmet_rdma_delete_ctrl,
 };
-- 
1.8.3.1

[PATCH 1/2] nvmet: Add fabrics ops to port

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

[PATCH 2/2] nvmet: Add port transport active flag

[Johannes Thumshirn]

On Mon, Mar 19, 2018@09:47:47AM +0000, Israel Rukshin wrote:
> tractive port flag means that nvmet transport is active and ready for
> receiving requests from host.
> It differ from enabled port state (port with subsystem symbolic link)
differs^


Also, this would need an accompanying nvmetcli change as well.

Anyways,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

[PATCH 2/2] nvmet: Add port transport active flag

[Max Gurtovoy]

On 3/19/2018 12:11 PM, Johannes Thumshirn wrote:
> On Mon, Mar 19, 2018@09:47:47AM +0000, Israel Rukshin wrote:
>> tractive port flag means that nvmet transport is active and ready for
>> receiving requests from host.
>> It differ from enabled port state (port with subsystem symbolic link)
> differs^
> 
> 
> Also, this would need an accompanying nvmetcli change as well.

Are you reffereing to the current implementation ?
Which change is in your mind ?

Currently the tractive flag will become true if the initial port 
configuration will succeed (also using nvmetcli).
One can create a service/timer/script to check periodically the tractive 
flag and echo 1 to it in case needed (for persistent target configuration).

> 
> Anyways,
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> 

[PATCH 2/2] nvmet: Add port transport active flag

[Johannes Thumshirn]

On Mon, Mar 19, 2018@12:48:15PM +0200, Max Gurtovoy wrote:
> 
> 
> On 3/19/2018 12:11 PM, Johannes Thumshirn wrote:
> > On Mon, Mar 19, 2018@09:47:47AM +0000, Israel Rukshin wrote:
> > > tractive port flag means that nvmet transport is active and ready for
> > > receiving requests from host.
> > > It differ from enabled port state (port with subsystem symbolic link)
> > differs^
> > 
> > 
> > Also, this would need an accompanying nvmetcli change as well.
> 
> Are you reffereing to the current implementation ?
> Which change is in your mind ?
> 
> Currently the tractive flag will become true if the initial port
> configuration will succeed (also using nvmetcli).
> One can create a service/timer/script to check periodically the tractive
> flag and echo 1 to it in case needed (for persistent target configuration).

No I was thinking of a nvmetcli command to administratively toggle the
tractive flag from within nvmetcli.

It is exported via configfs 'CONFIGFS_ATTR(nvmet_, tractive);' so
what's the purpose of a user controllable knob that can't be
controlled by the tool that usually configures these?

Byte,
	   Johannes

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

[PATCH 2/2] nvmet: Add port transport active flag

[Max Gurtovoy]

On 3/19/2018 12:56 PM, Johannes Thumshirn wrote:
> On Mon, Mar 19, 2018@12:48:15PM +0200, Max Gurtovoy wrote:
>>
>>
>> On 3/19/2018 12:11 PM, Johannes Thumshirn wrote:
>>> On Mon, Mar 19, 2018@09:47:47AM +0000, Israel Rukshin wrote:
>>>> tractive port flag means that nvmet transport is active and ready for
>>>> receiving requests from host.
>>>> It differ from enabled port state (port with subsystem symbolic link)
>>> differs^
>>>
>>>
>>> Also, this would need an accompanying nvmetcli change as well.
>>
>> Are you reffereing to the current implementation ?
>> Which change is in your mind ?
>>
>> Currently the tractive flag will become true if the initial port
>> configuration will succeed (also using nvmetcli).
>> One can create a service/timer/script to check periodically the tractive
>> flag and echo 1 to it in case needed (for persistent target configuration).
> 
> No I was thinking of a nvmetcli command to administratively toggle the
> tractive flag from within nvmetcli.

Yup, we can add this incrementally.

> 
> It is exported via configfs 'CONFIGFS_ATTR(nvmet_, tractive);' so
> what's the purpose of a user controllable knob that can't be
> controlled by the tool that usually configures these?
> 
> Byte,
> 	   Johannes
> 

[PATCH 2/2] nvmet: Add port transport active flag

[Johannes Thumshirn]

On Mon, Mar 19, 2018@02:19:46PM +0200, Max Gurtovoy wrote:
> > No I was thinking of a nvmetcli command to administratively toggle the
> > tractive flag from within nvmetcli.
> 
> Yup, we can add this incrementally.

Thanks,
	Johannes
-- 
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

[PATCH 2/2] nvmet: Add port transport active flag

[Sagi Grimberg]

> tractive port flag means that nvmet transport is active and ready for
> receiving requests from host.
> It differ from enabled port state (port with subsystem symbolic link)
> which doesn't guarantee this.
> The tractive flag is necessary in case the physical ports become down
> while nvmet ports are enabled.
> In this case the port state remains enabled but tractive flag becomes false.
> 
> The tractive flag will help the administrator to monitor the port state
> from transport point of view.
> The commit also add the ability to activate the port by simply writing
> to tractive at the configfs.
> e.g.: echo 1 > config/nvmet/ports/1/tractive

Why this is a write-able variable? this is a state. I would rename it
to be trstate.

[PATCH 2/2] nvmet: Add port transport active flag

[Max Gurtovoy]

On 3/20/2018 12:45 PM, Sagi Grimberg wrote:
> 
>> tractive port flag means that nvmet transport is active and ready for
>> receiving requests from host.
>> It differ from enabled port state (port with subsystem symbolic link)
>> which doesn't guarantee this.
>> The tractive flag is necessary in case the physical ports become down
>> while nvmet ports are enabled.
>> In this case the port state remains enabled but tractive flag becomes 
>> false.
>>
>> The tractive flag will help the administrator to monitor the port state
>> from transport point of view.
>> The commit also add the ability to activate the port by simply writing
>> to tractive at the configfs.
>> e.g.: echo 1 > config/nvmet/ports/1/tractive
> 
> Why this is a write-able variable? this is a state. I would rename it
> to be trstate.

We need a write-able entry to bring-up a listener in case the port/link 
went down.
Would you prefer 2 new configfs entries ? trstate (RO) and tractivate (WO) ?

[PATCH 2/2] nvmet: Add port transport active flag

[Sagi Grimberg]

>>> tractive port flag means that nvmet transport is active and ready for
>>> receiving requests from host.
>>> It differ from enabled port state (port with subsystem symbolic link)
>>> which doesn't guarantee this.
>>> The tractive flag is necessary in case the physical ports become down
>>> while nvmet ports are enabled.
>>> In this case the port state remains enabled but tractive flag becomes 
>>> false.
>>>
>>> The tractive flag will help the administrator to monitor the port state
>>> from transport point of view.
>>> The commit also add the ability to activate the port by simply writing
>>> to tractive at the configfs.
>>> e.g.: echo 1 > config/nvmet/ports/1/tractive
>>
>> Why this is a write-able variable? this is a state. I would rename it
>> to be trstate.
> 
> We need a write-able entry to bring-up a listener in case the port/link 
> went down.
> Would you prefer 2 new configfs entries ? trstate (RO) and tractivate 
> (WO) ?

No, I would prefer a trstate which is RO and activation with a subsystem
symlink. We can easily take care of re-activation in the transport.

[PATCH 2/2] nvmet: Add port transport active flag

[Max Gurtovoy]

On 3/20/2018 1:30 PM, Sagi Grimberg wrote:
> 
>>>> tractive port flag means that nvmet transport is active and ready for
>>>> receiving requests from host.
>>>> It differ from enabled port state (port with subsystem symbolic link)
>>>> which doesn't guarantee this.
>>>> The tractive flag is necessary in case the physical ports become down
>>>> while nvmet ports are enabled.
>>>> In this case the port state remains enabled but tractive flag 
>>>> becomes false.
>>>>
>>>> The tractive flag will help the administrator to monitor the port state
>>>> from transport point of view.
>>>> The commit also add the ability to activate the port by simply writing
>>>> to tractive at the configfs.
>>>> e.g.: echo 1 > config/nvmet/ports/1/tractive
>>>
>>> Why this is a write-able variable? this is a state. I would rename it
>>> to be trstate.
>>
>> We need a write-able entry to bring-up a listener in case the 
>> port/link went down.
>> Would you prefer 2 new configfs entries ? trstate (RO) and tractivate 
>> (WO) ?
> 
> No, I would prefer a trstate which is RO and activation with a subsystem
> symlink. We can easily take care of re-activation in the transport.

The initial activation is done using a subsystem symlink.
Please share your idea for re-activation (IIRC you were against 
re-activation in the kernel and prefered a user space re-activation).
We are reffering to situations where the LLD is unloaded or some other 
fatal event occur (EEH on Power systems for example)...

[Suspected-Phishing]Re: [PATCH 2/2] nvmet: Add port transport active flag

[Max Gurtovoy]

Guys,
we have a V2 in the pipe with trstate (RO attr). do you prefer 1/0 
output or "UP/DOWN" ? I realy don't mind.
Second question is regarding the re-activation. We want to create a WO 
attr "tractivate" for users to be able to activate port that for some 
reason fall down. We can also send a simple script for systemd 
service/timer that will bring up a port that should be active but isn't.
Also patches for nvmetcli are ready, but we need to decide the wanted 
approach so we can integrate it all together.

-Max.

On 3/20/2018 2:10 PM, Max Gurtovoy wrote:
> 
> 
> On 3/20/2018 1:30 PM, Sagi Grimberg wrote:
>>
>>>>> tractive port flag means that nvmet transport is active and ready for
>>>>> receiving requests from host.
>>>>> It differ from enabled port state (port with subsystem symbolic link)
>>>>> which doesn't guarantee this.
>>>>> The tractive flag is necessary in case the physical ports become down
>>>>> while nvmet ports are enabled.
>>>>> In this case the port state remains enabled but tractive flag 
>>>>> becomes false.
>>>>>
>>>>> The tractive flag will help the administrator to monitor the port 
>>>>> state
>>>>> from transport point of view.
>>>>> The commit also add the ability to activate the port by simply writing
>>>>> to tractive at the configfs.
>>>>> e.g.: echo 1 > config/nvmet/ports/1/tractive
>>>>
>>>> Why this is a write-able variable? this is a state. I would rename it
>>>> to be trstate.
>>>
>>> We need a write-able entry to bring-up a listener in case the 
>>> port/link went down.
>>> Would you prefer 2 new configfs entries ? trstate (RO) and tractivate 
>>> (WO) ?
>>
>> No, I would prefer a trstate which is RO and activation with a subsystem
>> symlink. We can easily take care of re-activation in the transport.
> 
> The initial activation is done using a subsystem symlink.
> Please share your idea for re-activation (IIRC you were against 
> re-activation in the kernel and prefered a user space re-activation).
> We are reffering to situations where the LLD is unloaded or some other 
> fatal event occur (EEH on Power systems for example)...
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-nvme&data=02%7C01%7Cmaxg%40mellanox.com%7Cdffa06e2153a4878d57c08d58e5ba93c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636571446705152203&sdata=qZJNH2ZerwtdhgJSvA8Pb89L%2BUJbsf2MxLgx7eh%2BVZo%3D&reserved=0 
>