Re: [PATCH 2/2] iscsi_tcp: disallow binding the same connection twice

[Mike Christie]

On 3/18/24 2:49 PM, Khazhismel Kumykov wrote:
> iscsi_sw_tcp_conn_bind does not check or cleanup previously bound
> sockets, nor should we allow binding the same connection twice.
> 

This looks like a problem for all the iscsi drivers.

I think you could:

1. Add a check for ISCSI_CONN_FLAG_BOUND in iscsi_conn_bind.
2. Have iscsi_sw_tcp_conn_stop do:

/* stop xmit side */
-   iscsi_suspend_tx(conn);
+   iscsi_conn_unbind(cls_conn, true);

to clear the flag when we clean up the conn for relogin.

3. Fix up the other iscsi drivers so they call:

iscsi_conn_unbind(cls_conn, true);

in their failure paths so when they fail they clear ISCSI_CONN_FLAG_BOUND and
iscsi_conn_bind can be called on the retry.



> Signed-off-by: Khazhismel Kumykov 
> ---
>  drivers/scsi/iscsi_tcp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index e8ed60b777c6..8cf5dc203a82 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -716,6 +716,9 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session 
> *cls_session,
>   struct socket *sock;
>   int err;
>  
> + if (tcp_sw_conn->sock)
> + return -EINVAL;
> +
>   /* lookup for existing socket */
>   sock = sockfd_lookup((int)transport_eph, );
>   if (!sock) {

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/6bc61553-6c8e-4705-9cbb-8e73d3f8c801%40oracle.com.

Re: [PATCH 1/2] iscsi_tcp: do not bind sockets that already have extra callbacks

[Mike Christie]

On 3/18/24 2:49 PM, Khazhismel Kumykov wrote:
> This attempts to avoid a situation where a misbehaving iscsi daemon
> passes a socket for a different iSCSI connection to BIND_CONN - which
> would result in infinite recursion and stack overflow. This will
> also prevent passing *other* sockets which had sk_user_data overridden,
> but that wouldn't have been safe anyways - since we throw away that
> pointer anyways. This does not cover all hypothetical scenarios where we
> pass bad sockets to BIND_CONN.
> 
> This also papers over a different bug - we allow a daemon to call
> BIND_CONN twice for the same connection - which would result in, at the
> least, failing to uninitialize/teardown the previous socket, which will
> be addressed separately.
> 
> Signed-off-by: Khazhismel Kumykov 
> ---
>  drivers/scsi/iscsi_tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 8e14cea15f98..e8ed60b777c6 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -725,7 +725,7 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session 
> *cls_session,
>   }
>  
>   err = -EINVAL;
> - if (!sk_is_tcp(sock->sk))
> + if (!sk_is_tcp(sock->sk) || sock->sk->sk_user_data)
>   goto free_socket;
>  
>   err = iscsi_conn_bind(cls_session, cls_conn, is_leading);


Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/3abd7f0b-dc88-4183-8762-f2f101402edd%40oracle.com.

Re: [PATCH] scsi: iscsi: fix iscsi ida memory leak

[Mike Christie]

On 1/29/24 3:04 AM, Guixin Liu wrote:
> The iscsi_sess_ida should be destroy when the iscsi module exit.
> 
> Signed-off-by: Guixin Liu 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 3075b2ddf7a6..3c5b42390c47 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -5046,6 +5046,7 @@ static void __exit iscsi_transport_exit(void)
>   class_unregister(_endpoint_class);
>   class_unregister(_iface_class);
>   class_unregister(_transport_class);
> + ida_destroy(_sess_ida);
>  }
>  
>  module_init(iscsi_transport_init);

When this is called the ida will be empty so I don't think we have to
call this. From the comments:

/**
 * ida_destroy() - Free all IDs.
 * @ida: IDA handle.
 *
 * Calling this function frees all IDs and releases all resources used
 * by an IDA.  When this call returns, the IDA is empty and can be reused
 * or freed.  If the IDA is already empty, there is no need to call this
 * function.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/9db727d3-1f82-4dd5-ad62-a04d17f6dfed%40oracle.com.

Re: [PATCH] scsi: iscsi_tcp: restrict to TCP sockets

[Mike Christie]

On 9/15/23 12:11 PM, Eric Dumazet wrote:
> Nothing prevents iscsi_sw_tcp_conn_bind() to receive file descriptor
> pointing to non TCP socket (af_unix for example).
> 
> Return -EINVAL if this is attempted, instead of crashing the kernel.
> 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ac3eb485-18be-4fcd-b0d4-8370aa64f38a%40oracle.com.

Re: [PATCH v2] scsi: iscsi: kfree_sensitive() in iscsi_session_free()

[Mike Christie]

On 7/19/23 2:45 AM, 杜敏杰 wrote:
> session might contain private part of the password, so better use
> kfree_sensitive() to free it.
> In iscsi_session_free() use kfree_sensitive() to free session->password,
> session->password_in, session->username, session->username_in.
> 
> Signed-off-by: Minjie Du 
> ---
>  drivers/scsi/libiscsi.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 0fda8905e..a307da898 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -3132,10 +3132,10 @@ void iscsi_session_free(struct iscsi_cls_session 
> *cls_session)
>   struct module *owner = cls_session->transport->owner;
>  
>   iscsi_pool_free(>cmdpool);
> - kfree(session->password);
> - kfree(session->password_in);
> - kfree(session->username);
> - kfree(session->username_in);
> + kfree_sensitive(session->password);
> + kfree_sensitive(session->password_in);
> + kfree_sensitive(session->username);
> + kfree_sensitive(session->username_in);
>   kfree(session->targetname);
>   kfree(session->targetalias);
>   kfree(session->initiatorname);

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/59d94e6e-27b6-5b12-d592-8f814ee1788f%40oracle.com.

Re: [PATCH v1] scsi: iscsi: use kfree_sensitive() in iscsi_session_free()

[Mike Christie]

On 7/17/23 4:26 AM, Minjie Du wrote:
> session might contain private part of the password, so better use
> kfree_sensitive() to free it.
> In iscsi_session_free() use kfree_sensitive() to free session->password.
> 
> Signed-off-by: Minjie Du 
> ---
>  drivers/scsi/libiscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 0fda8905e..2f273229c 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -3132,7 +3132,7 @@ void iscsi_session_free(struct iscsi_cls_session 
> *cls_session)
>   struct module *owner = cls_session->transport->owner;
>  
>   iscsi_pool_free(>cmdpool);
> - kfree(session->password);
> + kfree_sensitive(session->password);
>   kfree(session->password_in);

You then also want kfree_sensitive for password_in.

I would also use it for the usernames then too.

>   kfree(session->username);
>   kfree(session->username_in);

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/5bed6236-0db7-37fd-3d0a-4f51874f9c53%40oracle.com.

Re: [PATCH net-next v4 11/15] iscsi: Use sendmsg(MSG_SPLICE_PAGES) rather than sendpage

[Mike Christie]

On 6/23/23 6:44 AM, David Howells wrote:
> Use sendmsg() with MSG_SPLICE_PAGES rather than sendpage.  This allows
> multiple pages and multipage folios to be passed through.
> 
> TODO: iscsit_fe_sendpage_sg() should perhaps set up a bio_vec array for the
> entire set of pages it's going to transfer plus two for the header and
> trailer and page fragments to hold the header and trailer - and then call
> sendmsg once for the entire message.
> 
> Signed-off-by: David Howells 
> cc: Lee Duncan 
> cc: Chris Leech 
> cc: Mike Christie 
> cc: Maurizio Lombardi 
> cc: "James E.J. Bottomley" 
> cc: "Martin K. Petersen" 
> cc: "David S. Miller" 
> cc: Eric Dumazet 
> cc: Jakub Kicinski 
> cc: Paolo Abeni 
> cc: Jens Axboe 
> cc: Matthew Wilcox 
> cc: Al Viro 
> cc: open-iscsi@googlegroups.com
> cc: linux-s...@vger.kernel.org
> cc: target-de...@vger.kernel.org
> cc: net...@vger.kernel.org
> ---
> 
> Notes:
> ver #2)
>  - Wrap lines at 80.
> 
>  drivers/scsi/iscsi_tcp.c | 26 +---
>  drivers/scsi/iscsi_tcp.h |  2 --
>  drivers/target/iscsi/iscsi_target_util.c | 15 --

When you send this again can you split it into 2 patches?

drivers/scsi/iscsi_tcp.* is one driver. It's similar to a NFS client and
it has a set of maintainers that you saw in the MAINTAINER file.

drivers/target/iscsi/iscsi_target_util.c is another driver which is similar
to a NFS server. Martin is the overall maintainer but it's a group effort
to review patches.

I've tested and reviewed the iscsi_tcp parts. You can add my:

Reviewed-by: Mike Christie 

For the iscsi_target_util.c part, I'm reviewing it now and hoping Maurizio
and/or Dimitry might review as well.

One question on the target part I had is about the TODO above. Is that
something you were going to do, or is it something you are asking the target
people to do?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/27fd3750-7b9c-9638-26d8-0df3f0e33b81%40oracle.com.

Re: [PATCH 05/11] iscsi: check net namespace for all iscsi lookup

[Mike Christie]

On 5/6/23 6:29 PM, Chris Leech wrote:
> @@ -4065,8 +4108,10 @@ iscsi_if_recv_msg(struct net *net, struct sk_buff *skb,
> ev->u.c_session.cmds_max,
> ev->u.c_session.queue_depth);
>   break;
> + /* MARK */

Got an extra comment in there.

>   case ISCSI_UEVENT_CREATE_BOUND_SESSION:
> - ep = iscsi_lookup_endpoint(ev->u.c_bound_session.ep_handle);
> + ep = iscsi_lookup_endpoint(net,
> +ev->u.c_bound_session.ep_handle);

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/1bbea295-69f6-80ad-2000-a48fb1da8eda%40oracle.com.

Re: [PATCH] scsi: iscsi_tcp: Check the sock is correct before iscsi_set_param

[Mike Christie]

On 3/29/23 2:17 AM, Zhong Jinghua wrote:
> From: Zhong Jinghua 
> 
> The correctness of sock should be checked before assignment to avoid
> assigning wrong values.
> 
> Commit
> "scsi: iscsi: iscsi_tcp: Fix null-ptr-deref while calling getpeername()"
> introduced this change. This change may lead to inconsistent values of
> tcp_sw_conn->sendpage and conn->datadgst_en.
> 
> Fix it by moving the position of the assignment.
> 
> Fixes: 57569c37f0ad ("scsi: iscsi: iscsi_tcp: Fix null-ptr-deref while 
> calling getpeername()")
> Signed-off-by: Zhong Jinghua 

Thanks.

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/15e892ef-0ab4-b2c9-bfc7-6cc3ba221c4a%40oracle.com.

Re: [PATCH 1/2] scsi:core:Add sysfs interface to control if skip lun with PQ=1

[Mike Christie]

On 12/14/22 1:08 AM, Wenchao Hao wrote:
> 
> When iSCSI initiator logged in target, the target attached none valid
> lun but lun0. lun0 is not an valid disk, while it would response
> inquiry command with PQ=1 and other general scsi commands like probe lun.
> The others luns of target is added/removed dynamicly.
> 
> We want the lun0 to be mapped to an sg device in initiator, so we can
> probe luns of target based on lun0.

What do you want to do exactly? Is the idea with your patch we would create
an sg device, then in userpsace you would do some scan related commands. If
you find devices then you use sysfs to have scsi-ml scan/add a specific device
like

echo 5 0 0 8 > host5/scan

?

It's not really clear to me why you need the sg device, and can't just do?

echo - - - > .../hostN/scan

? Do you only want to add specific devices like you are doing some sort of
LUN masking on the initiator side?

Is the issue that you need the sg device there, so you can detect when a device
is no longer present on the target and then you will have userspace remove the
device via the sysfs delete file?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/536981a8-76a3-54b9-a70c-a86994c027dd%40oracle.com.

Re: [PATCH] scsi: iscsi_tcp: Fix UAF when access shost attr during session logout

[Mike Christie]

On 12/11/22 8:32 AM, Wenchao Hao wrote:
> On Sun, Dec 11, 2022 at 6:07 AM Mike Christie
>  wrote:
>>
>> On 12/9/22 2:22 AM, Ding Hui wrote:
>>> During iscsi session logout, if another task accessing shost ipaddress
>>> attr at this time, we can get a KASAN UAF report like this:
>>>
>>> [  276.941685] 
>>> ==
>>> [  276.942144] BUG: KASAN: use-after-free in _raw_spin_lock_bh+0x78/0xe0
>>> [  276.942535] Write of size 4 at addr 8881053b45b8 by task cat/4088
>>> [  276.943511] CPU: 2 PID: 4088 Comm: cat Tainted: GE  
>>> 6.1.0-rc8+ #3
>>> [  276.943997] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
>>> Desktop Reference Platform, BIOS 6.00 11/12/2020
>>> [  276.944470] Call Trace:
>>> [  276.944943]  
>>> [  276.945397]  dump_stack_lvl+0x34/0x48
>>> [  276.945887]  print_address_description.constprop.0+0x86/0x1e7
>>> [  276.946421]  print_report+0x36/0x4f
>>> [  276.947358]  kasan_report+0xad/0x130
>>> [  276.948234]  kasan_check_range+0x35/0x1c0
>>> [  276.948674]  _raw_spin_lock_bh+0x78/0xe0
>>> [  276.949989]  iscsi_sw_tcp_host_get_param+0xad/0x2e0 [iscsi_tcp]
>>> [  276.951765]  show_host_param_ISCSI_HOST_PARAM_IPADDRESS+0xe9/0x130 
>>> [scsi_transport_iscsi]
>>> [  276.952185]  dev_attr_show+0x3f/0x80
>>> [  276.953005]  sysfs_kf_seq_show+0x1fb/0x3e0
>>> [  276.953401]  seq_read_iter+0x402/0x1020
>>> [  276.954260]  vfs_read+0x532/0x7b0
>>> [  276.955113]  ksys_read+0xed/0x1c0
>>> [  276.955952]  do_syscall_64+0x38/0x90
>>> [  276.956347]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>> [  276.956769] RIP: 0033:0x7f5d3a679222
>>> [  276.957161] Code: c0 e9 b2 fe ff ff 50 48 8d 3d 32 c0 0b 00 e8 a5 fe 01 
>>> 00 0f 1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 
>>> <48> 3d 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24
>>> [  276.958009] RSP: 002b:7ffc864d16a8 EFLAGS: 0246 ORIG_RAX: 
>>> 
>>> [  276.958431] RAX: ffda RBX: 0002 RCX: 
>>> 7f5d3a679222
>>> [  276.958857] RDX: 0002 RSI: 7f5d3a4fe000 RDI: 
>>> 0003
>>> [  276.959281] RBP: 7f5d3a4fe000 R08:  R09: 
>>> 
>>> [  276.959682] R10: 0022 R11: 0246 R12: 
>>> 0002
>>> [  276.960126] R13: 0003 R14:  R15: 
>>> 557a26dada58
>>> [  276.960536]  
>>> [  276.961357] Allocated by task 2209:
>>> [  276.961756]  kasan_save_stack+0x1e/0x40
>>> [  276.962170]  kasan_set_track+0x21/0x30
>>> [  276.962557]  __kasan_kmalloc+0x7e/0x90
>>> [  276.962923]  __kmalloc+0x5b/0x140
>>> [  276.963308]  iscsi_alloc_session+0x28/0x840 [scsi_transport_iscsi]
>>> [  276.963712]  iscsi_session_setup+0xda/0xba0 [libiscsi]
>>> [  276.964078]  iscsi_sw_tcp_session_create+0x1fd/0x330 [iscsi_tcp]
>>> [  276.964431]  iscsi_if_create_session.isra.0+0x50/0x260 
>>> [scsi_transport_iscsi]
>>> [  276.964793]  iscsi_if_recv_msg+0xc5a/0x2660 [scsi_transport_iscsi]
>>> [  276.965153]  iscsi_if_rx+0x198/0x4b0 [scsi_transport_iscsi]
>>> [  276.965546]  netlink_unicast+0x4d5/0x7b0
>>> [  276.965905]  netlink_sendmsg+0x78d/0xc30
>>> [  276.966236]  sock_sendmsg+0xe5/0x120
>>> [  276.966576]  sys_sendmsg+0x5fe/0x860
>>> [  276.966923]  ___sys_sendmsg+0xe0/0x170
>>> [  276.967300]  __sys_sendmsg+0xc8/0x170
>>> [  276.967666]  do_syscall_64+0x38/0x90
>>> [  276.968028]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>> [  276.968773] Freed by task 2209:
>>> [  276.969111]  kasan_save_stack+0x1e/0x40
>>> [  276.969449]  kasan_set_track+0x21/0x30
>>> [  276.969789]  kasan_save_free_info+0x2a/0x50
>>> [  276.970146]  __kasan_slab_free+0x106/0x190
>>> [  276.970470]  __kmem_cache_free+0x133/0x270
>>> [  276.970816]  device_release+0x98/0x210
>>> [  276.971145]  kobject_cleanup+0x101/0x360
>>> [  276.971462]  iscsi_session_teardown+0x3fb/0x530 [libiscsi]
>>> [  276.971775]  iscsi_sw_tcp_session_destroy+0xd8/0x130 [iscsi_tcp]
>>> [  276.972143]  iscsi_if_recv_msg+0x1bf1/0x2660 [scsi_transport_iscsi]
>>> [  276.972485]  iscsi_if_rx+0x198/0x4b0 [scsi_transport_iscsi]
>>> [  276.972808]  netlink_unicast+0x4d5/0x7b0
>>> [  276.973201]  netlink_sendmsg+0x78d/0xc30
>&

Re: [PATCH] scsi: iscsi_tcp: Fix UAF when access shost attr during session logout

[Mike Christie]

On 12/9/22 2:22 AM, Ding Hui wrote:
> During iscsi session logout, if another task accessing shost ipaddress
> attr at this time, we can get a KASAN UAF report like this:
> 
> [  276.941685] 
> ==
> [  276.942144] BUG: KASAN: use-after-free in _raw_spin_lock_bh+0x78/0xe0
> [  276.942535] Write of size 4 at addr 8881053b45b8 by task cat/4088
> [  276.943511] CPU: 2 PID: 4088 Comm: cat Tainted: GE  
> 6.1.0-rc8+ #3
> [  276.943997] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
> Desktop Reference Platform, BIOS 6.00 11/12/2020
> [  276.944470] Call Trace:
> [  276.944943]  
> [  276.945397]  dump_stack_lvl+0x34/0x48
> [  276.945887]  print_address_description.constprop.0+0x86/0x1e7
> [  276.946421]  print_report+0x36/0x4f
> [  276.947358]  kasan_report+0xad/0x130
> [  276.948234]  kasan_check_range+0x35/0x1c0
> [  276.948674]  _raw_spin_lock_bh+0x78/0xe0
> [  276.949989]  iscsi_sw_tcp_host_get_param+0xad/0x2e0 [iscsi_tcp]
> [  276.951765]  show_host_param_ISCSI_HOST_PARAM_IPADDRESS+0xe9/0x130 
> [scsi_transport_iscsi]
> [  276.952185]  dev_attr_show+0x3f/0x80
> [  276.953005]  sysfs_kf_seq_show+0x1fb/0x3e0
> [  276.953401]  seq_read_iter+0x402/0x1020
> [  276.954260]  vfs_read+0x532/0x7b0
> [  276.955113]  ksys_read+0xed/0x1c0
> [  276.955952]  do_syscall_64+0x38/0x90
> [  276.956347]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [  276.956769] RIP: 0033:0x7f5d3a679222
> [  276.957161] Code: c0 e9 b2 fe ff ff 50 48 8d 3d 32 c0 0b 00 e8 a5 fe 01 00 
> 0f 1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 
> 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24
> [  276.958009] RSP: 002b:7ffc864d16a8 EFLAGS: 0246 ORIG_RAX: 
> 
> [  276.958431] RAX: ffda RBX: 0002 RCX: 
> 7f5d3a679222
> [  276.958857] RDX: 0002 RSI: 7f5d3a4fe000 RDI: 
> 0003
> [  276.959281] RBP: 7f5d3a4fe000 R08:  R09: 
> 
> [  276.959682] R10: 0022 R11: 0246 R12: 
> 0002
> [  276.960126] R13: 0003 R14:  R15: 
> 557a26dada58
> [  276.960536]  
> [  276.961357] Allocated by task 2209:
> [  276.961756]  kasan_save_stack+0x1e/0x40
> [  276.962170]  kasan_set_track+0x21/0x30
> [  276.962557]  __kasan_kmalloc+0x7e/0x90
> [  276.962923]  __kmalloc+0x5b/0x140
> [  276.963308]  iscsi_alloc_session+0x28/0x840 [scsi_transport_iscsi]
> [  276.963712]  iscsi_session_setup+0xda/0xba0 [libiscsi]
> [  276.964078]  iscsi_sw_tcp_session_create+0x1fd/0x330 [iscsi_tcp]
> [  276.964431]  iscsi_if_create_session.isra.0+0x50/0x260 
> [scsi_transport_iscsi]
> [  276.964793]  iscsi_if_recv_msg+0xc5a/0x2660 [scsi_transport_iscsi]
> [  276.965153]  iscsi_if_rx+0x198/0x4b0 [scsi_transport_iscsi]
> [  276.965546]  netlink_unicast+0x4d5/0x7b0
> [  276.965905]  netlink_sendmsg+0x78d/0xc30
> [  276.966236]  sock_sendmsg+0xe5/0x120
> [  276.966576]  sys_sendmsg+0x5fe/0x860
> [  276.966923]  ___sys_sendmsg+0xe0/0x170
> [  276.967300]  __sys_sendmsg+0xc8/0x170
> [  276.967666]  do_syscall_64+0x38/0x90
> [  276.968028]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [  276.968773] Freed by task 2209:
> [  276.969111]  kasan_save_stack+0x1e/0x40
> [  276.969449]  kasan_set_track+0x21/0x30
> [  276.969789]  kasan_save_free_info+0x2a/0x50
> [  276.970146]  __kasan_slab_free+0x106/0x190
> [  276.970470]  __kmem_cache_free+0x133/0x270
> [  276.970816]  device_release+0x98/0x210
> [  276.971145]  kobject_cleanup+0x101/0x360
> [  276.971462]  iscsi_session_teardown+0x3fb/0x530 [libiscsi]
> [  276.971775]  iscsi_sw_tcp_session_destroy+0xd8/0x130 [iscsi_tcp]
> [  276.972143]  iscsi_if_recv_msg+0x1bf1/0x2660 [scsi_transport_iscsi]
> [  276.972485]  iscsi_if_rx+0x198/0x4b0 [scsi_transport_iscsi]
> [  276.972808]  netlink_unicast+0x4d5/0x7b0
> [  276.973201]  netlink_sendmsg+0x78d/0xc30
> [  276.973544]  sock_sendmsg+0xe5/0x120
> [  276.973864]  sys_sendmsg+0x5fe/0x860
> [  276.974248]  ___sys_sendmsg+0xe0/0x170
> [  276.974583]  __sys_sendmsg+0xc8/0x170
> [  276.974891]  do_syscall_64+0x38/0x90
> [  276.975216]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> We can easily reproduce by two tasks:
> 1. while :; do iscsiadm -m node --login; iscsiadm -m node --logout; done
> 2. while :; do cat /sys/devices/platform/host*/iscsi_host/host*/ipaddress; 
> done
> 
> iscsid|cat
> --+-
> |- iscsi_sw_tcp_session_destroy   |
>   |- iscsi_session_teardown   |
> |- device_release |
>   |- iscsi_session_release|  |- dev_attr_show
> |- kfree  ||- 
> show_host_param_ISCSI_HOST_PARAM_IPADDRESS
>   |  |- iscsi_sw_tcp_host_get_param
>   ||- r/w 

Re: [PATCH v7] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Mike Christie]

On 11/25/22 7:07 PM, Wenchao Hao wrote:
> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
> for multiple times which should be fixed.
> 
> This patch introduce target_state in iscsi_cls_session to make
> sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.
> 
> But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
> Report unbind session event when the target has been removed"). The issue
> is iscsid died for any reason after it send unbind session to kernel, once
> iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.
> 
> Now kernel think iscsi_cls_session has already sent an
> ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
> would cause userspace unable to logout. Actually the session is in
> invalid state(it's target_id is INVALID), iscsid should not sync this
> session in it's restart.
> 
> So we need to check session's target state during iscsid restart,
> if session is in unbound state, do not sync this session and perform
> session teardown. It's reasonable because once a session is unbound, we
> can not recover it any more(mainly because it's target id is INVALID)
> 
> V7:
> - Define target state to string map and refer this map directly
> - Cleanup __iscsi_unbind_session, drop check for session's
>   target_id == ISCSI_MAX_TARGET since it can be handled by target_state
> 
> V6:
> - Set target state to ALLOCATED in iscsi_add_session
> - Rename state BOUND to SCANNED
> - Make iscsi_session_target_state_name() more efficient
> 
> V5:
> - Add ISCSI_SESSION_TARGET_ALLOCATED to indicate the session's
>   target has been allocated but not scanned yet. We should
>   sync this session and scan this session when iscsid restarted.
> 
> V4:
> - Move debug print out of spinlock critical section
> 
> V3:
> - Make target bind state to a state kind rather than a bool.
> 
> V2:
> - Using target_unbound rather than state to indicate session has been
>   unbound
> 
> Signed-off-by: Wenchao Hao 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/050e7458-c952-1b1f-a98e-6c12c5b30cdb%40oracle.com.

Re: [PATCH] scsi:iscsi: Record session's startup mode in kernel

[Mike Christie]

On 11/30/22 5:05 PM, Lee Duncan wrote:
> On 11/30/22 12:08, Mike Christie wrote:
>> On 11/30/22 1:53 PM, Lee Duncan wrote:
>>> Have you already worked on the open-iscsi side of this? No reason for 
>>> duplicate development.
>>
>> I think you missed his reply where he said he was missed the
>> iscsid.safe_logout setting.
>>
>>
>>
> 
> No, I saw that, but I thought there were some cases that might be missed by 
> checking for mounts. I also think having to set "safe" mode globally is 
> inefficient, but that's a separate issue.

The safe logout code also checks for holders which covers things like lvm and
raid use. It doesn't handle things like general device openings though. Maybe we
want a general block layer use count file for that if we are worried about
that type of thing.

> 
> Was the plan to retrace the changes submitted and accepted upstream for 
> adding a sysfs entry for node_startup that won't be used? Or is the plan to 
> populate that attribute for user consumption, even though it isn't needed for 
> this particular problem?

Are you asking about the patch in this thread? If so, I don't think it's
merged.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/905da4a0-3ecb-9bb9-75f3-79f7be19599b%40oracle.com.

Re: [PATCH] scsi:iscsi: Record session's startup mode in kernel

[Mike Christie]

On 11/30/22 1:53 PM, Lee Duncan wrote:
> Have you already worked on the open-iscsi side of this? No reason for 
> duplicate development.

I think you missed his reply where he said he was missed the
iscsid.safe_logout setting.



-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/5ae1b303-75c8-1dc8-d631-bff6db53081d%40oracle.com.

Re: [PATCH] scsi:iscsi: Record session's startup mode in kernel

[Mike Christie]

On 11/22/22 10:41 PM, Wenchao Hao wrote:
> Sorry I did not highlight the key points. The root reason we need to record
> node_startup mode in kernel is userspace's node_startup mode is unreliable in
> some scenarios:
> 
> 1. iscsi node and session is created in initrd, the configure files of these
>nodes would be lost after we switch to rootfs
> 2. someone do iscsiadm -m discovery but did not specify the operation mode,
>the iscsi node's node_startup would be updated to which specified in 
> iscsid.conf
> 3. someone do iscsiadm -m node -o update to update nodes' configure
> 
> What's more, it seems "iscsiadm/iscsid" only refuse to logout of an ONBOOT
> session when logout is specificed by "--logoutall". We still can logout an
> ONBOOT session with "iscsiadm -m node -u comamnd".

logout_by_startup does go by the startup setting, but I think you missed the
session_in_use related code. It checks the mounts and holders already. Just
change it for whatever you need. I think your lvm use case should be covered
by the holder check. If not, add it.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/717da158-5a7e-b478-61d3-3753b0b00e01%40oracle.com.

Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Mike Christie]

On 11/22/22 11:29 AM, Wenchao Hao wrote:
> On Wed, Nov 23, 2022 at 1:04 AM Mike Christie
>  wrote:
>>
>> On 11/21/22 8:17 AM, Wenchao Hao wrote:
>>> And the function looks like following after change:
>>>
>>> static void __iscsi_unbind_session(struct work_struct *work)
>>> {
>>>   struct iscsi_cls_session *session =
>>>   container_of(work, struct iscsi_cls_session,
>>>unbind_work);
>>>   struct Scsi_Host *shost = iscsi_session_to_shost(session);
>>>   struct iscsi_cls_host *ihost = shost->shost_data;
>>>   unsigned long flags;
>>>   unsigned int target_id;
>>>
>>>   ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>>>
>>>   /* Prevent new scans and make sure scanning is not in progress */
>>>   mutex_lock(>mutex);
>>>   spin_lock_irqsave(>lock, flags);
>>>   if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>>
>> What was the reason for not checking for ALLOCATED and freeing the ida
>> in that case?
>>
> 
> target_state would be in "ALLOCATED" state if iscsid died after add
> session successfully.
> When iscsid restarted, if the session's target_state is "ALLOCATED",
> it should scan
> the session and the target_state would switch to "SCANNED".
> 
> So I think we would not call in __iscsi_unbind_session() with
> session's target_state
> is ALLOCATED.

Makes sense for the normal case.

The only issue is when __iscsi_unbind_session is called via
iscsi_remove_session for the cases where userspace didn't do
the  UNBIND event. Some tools don't do unbind or open-iscsi
sometimes doesn't if the session is down. We will leak the ida,
so you need some code to handle that.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/856ccad2-19a4-32b4-b41f-5a230a55ee30%40oracle.com.

Re: [PATCH] scsi:iscsi: Record session's startup mode in kernel

[Mike Christie]

On 11/22/22 3:30 PM, Wenchao Hao wrote:
> There are 3 iscsi session's startup mode which are onboot, manual and
> automatic. We can boot from iSCSI disks with help of dracut's service
> in initrd, which would set node's startup mode to onboot, then create
> iSCSI sessions.
> 
> While the configure of onboot mode is recorded in file of initrd stage
> and would be lost when switch to rootfs. Even if we update the startup
> mode to onboot by hand after switch to rootfs, it is possible that the
> configure would be covered by another discovery command.
> 
> root would be mounted on iSCSI disks when boot from iSCSI disks, if the
> sessions is logged out, the related disks would be removed, which would
> cause the whole system halt.

The userspace tools check for this already don't they? Running iscsiadm
on the root disk returns a failure and message about it being in use.

Userspace can check the session's disks and see if they are mounted and
what they are being used for.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/11003745-2b2d-30cf-bf87-798f5175ae09%40oracle.com.

Re: [PATCH] scsi:iscsi: rename iscsi_set_param to iscsi_if_set_param

[Mike Christie]

On 11/22/22 12:11 PM, Wenchao Hao wrote:
> There are two iscsi_set_param() functions individually defined
> in libiscsi.c and scsi_transport_iscsi.c which is confused.
> 
> So rename the one in scsi_transport_iscsi.c to iscsi_if_set_param().
> 
> Signed-off-by: Wenchao Hao 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..c3fe5ecfee59 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2988,7 +2988,7 @@ iscsi_if_destroy_conn(struct iscsi_transport 
> *transport, struct iscsi_uevent *ev
>  }
>  
>  static int
> -iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
> +iscsi_if_set_param(struct iscsi_transport *transport, struct iscsi_uevent 
> *ev)
>  {
>   char *data = (char*)ev + sizeof(*ev);
>   struct iscsi_cls_conn *conn;
> @@ -3941,7 +3941,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr 
> *nlh, uint32_t *group)
>   err = -EINVAL;
>   break;
>   case ISCSI_UEVENT_SET_PARAM:
> - err = iscsi_set_param(transport, ev);
> + err = iscsi_if_set_param(transport, ev);
>   break;
>   case ISCSI_UEVENT_CREATE_CONN:
>   case ISCSI_UEVENT_DESTROY_CONN:

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/9aee9ea4-9644-3f77-4dea-11784868c885%40oracle.com.

Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Mike Christie]

On 11/21/22 8:17 AM, Wenchao Hao wrote:
> And the function looks like following after change:
> 
> static void __iscsi_unbind_session(struct work_struct *work)
> {
>   struct iscsi_cls_session *session =
>   container_of(work, struct iscsi_cls_session,
>unbind_work);
>   struct Scsi_Host *shost = iscsi_session_to_shost(session);
>   struct iscsi_cls_host *ihost = shost->shost_data;
>   unsigned long flags;
>   unsigned int target_id;
> 
>   ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
> 
>   /* Prevent new scans and make sure scanning is not in progress */
>   mutex_lock(>mutex);
>   spin_lock_irqsave(>lock, flags);
>   if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {

What was the reason for not checking for ALLOCATED and freeing the ida
in that case?

>   spin_unlock_irqrestore(>lock, flags);
>   mutex_unlock(>mutex);
>   ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: 
> Session is %s.\n",
>   
> iscsi_session_target_state_name[session->target_state]);
>   return;
>   }
>   target_id = session->target_id;
>   session->target_id = ISCSI_MAX_TARGET;
>   session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
>   spin_unlock_irqrestore(>lock, flags);
>   mutex_unlock(>mutex);
> 
>   scsi_remove_target(>dev);
> 
>   spin_lock_irqsave(>lock, flags);
>   session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
>   spin_unlock_irqrestore(>lock, flags);
> 
>   if (session->ida_used)
>   ida_free(_sess_ida, target_id);
> 
>   iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
>   ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
> }
> 
> 
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/418c7f6f-0bc3-45bb-2abf-e866df6f4b62%40oracle.com.

Re: [PATCH v2] scsi: iscsi: fix possible memory leak when device_register failed

[Mike Christie]

On 11/9/22 9:37 PM, Zhou Guanghui wrote:
> If device_register() returns error, the name allocated by the
> dev_set_name() need be freed. As described in the comment of
> device_register(), we should use put_device() to give up the
> reference in the error path.
> 
> Fix this by calling put_device(), the name will be freed in the
> kobject_cleanup(), and this patch modified resources will be
> released by calling the corresponding callback function in the
> device_release().
> 
> Signed-off-by: Zhou Guanghui 
> 

Thanks.

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/f1a0a01b-e934-1ef8-bd23-821d855e976c%40oracle.com.

Re: [-next] scsi: iscsi: fix possible memory leak in iscsi_register_transport

[Mike Christie]

On 11/9/22 2:19 AM, Zhou Guanghui wrote:
> "unreferenced object 0x888117908420 (size 16):
>   comm ""modprobe"", pid 18125, jiffies 4319017437 (age 73.039s)
>   hex dump (first 16 bytes):
> 62 65 32 69 73 63 73 69 00 84 90 17 81 88 ff ff  be2iscsi
>   backtrace:
> [<f78a13b3>] __kmem_cache_alloc_node+0x157/0x220
> [<200a51a4>] __kmalloc_node_track_caller+0x44/0x1b0
> [<33ea4d64>] kstrdup+0x3a/0x70
> [<ec6d2980>] kstrdup_const+0x41/0x60
> [<55015f6f>] kvasprintf_const+0xf5/0x180
> [<9dd443d2>] kobject_set_name_vargs+0x56/0x150
> [<f3448e98>] dev_set_name+0xab/0xe0
> [<80ab8992>] iscsi_register_transport+0x1f8/0x610 
> [scsi_transport_iscsi]
> [<5e2c324d>] 0xc1260012
> [<df6e6a36>] do_one_initcall+0xcb/0x4d0
> [<181109df>] do_init_module+0x1ca/0x5f0
> [<b3c4fec8>] load_module+0x6133/0x70f0
> [<feb08394>] __do_sys_finit_module+0x12f/0x1c0
> [<ca6af44d>] do_syscall_64+0x37/0x90
> [<132e1a8b>] entry_SYSCALL_64_after_hwframe+0x63/0xcd"
> 
> If device_register() returns error in iscsi_register_transport(),
> the name allocated by the dev_set_name() need be freed.
> 
> Fix this by calling put_device(), the name will be freed in the
> kobject_cleanup(), and the priv will be freed in
> iscsi_transport_release.
> 
> Signed-off-by: Zhou Guanghui 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..51e2c0f5e2d0 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -4815,7 +4815,7 @@ iscsi_register_transport(struct iscsi_transport *tt)
>   dev_set_name(>dev, "%s", tt->name);
>   err = device_register(>dev);
>   if (err)
> - goto free_priv;
> + goto put_dev;
>  
>   err = sysfs_create_group(>dev.kobj, _transport_group);
>   if (err)
> @@ -4850,8 +4850,8 @@ iscsi_register_transport(struct iscsi_transport *tt)
>  unregister_dev:
>   device_unregister(>dev);
>   return NULL;
> -free_priv:
> - kfree(priv);
> +put_dev:
> + put_device(>dev);
>   return NULL;
>  }
>  EXPORT_SYMBOL_GPL(iscsi_register_transport);

Reviewed-by: Mike Christie 

Shoot, I see the comment about using put_device in device_add.
I'm not sure what happened, but I made the same mistake above
in 4 other places.

Do you want to send patches for the other ones? If not, I'll do
it.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/c4b77a0f-c53d-42fa-8d42-a08a12f59667%40oracle.com.

Re: [PATCH] scsi: iscsi: fix possible memory leak when transport_register_device() fails

[Mike Christie]

On 11/9/22 3:24 AM, Yang Yingliang wrote:
> If transport_register_device() fails, transport_destroy_device() should
> be called to release the memory allocated in transport_setup_device().
> 
> Fixes: 0896b7523026 ("[SCSI] open-iscsi/linux-iscsi-5 Initiator: Transport 
> class update for iSCSI")
> Signed-off-by: Yang Yingliang 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..88add31a56e3 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2085,6 +2085,7 @@ int iscsi_add_session(struct iscsi_cls_session 
> *session, unsigned int target_id)
>   return 0;
>  
>  release_dev:
> + transport_destroy_device(>dev);
>   device_del(>dev);
>  release_ida:
>   if (session->ida_used)
> @@ -2462,6 +2463,7 @@ int iscsi_add_conn(struct iscsi_cls_conn *conn)
>   if (err) {
>   iscsi_cls_session_printk(KERN_ERR, session,
>"could not register transport's 
> dev\n");
> + transport_destroy_device(>dev);
>   device_del(>dev);
>   return err;

Why doesn't transport_register_device undo what it did and call
transport_destroy_device? The callers like iscsi don't know what
was done, so it seems odd to call transport_destroy_device when
we got a failure.


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/41b1cacb-94cc-ad69-11a7-b13452080389%40oracle.com.

Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Mike Christie]

On 11/7/22 7:44 PM, Wenchao Hao wrote:
> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
> for multiple times which should be fixed.
> 
> This patch introduce target_state in iscsi_cls_session to make
> sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.
> 
> But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
> Report unbind session event when the target has been removed"). The issue
> is iscsid died for any reason after it send unbind session to kernel, once
> iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.
> 
> Now kernel think iscsi_cls_session has already sent an
> ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
> would cause userspace unable to logout. Actually the session is in
> invalid state(it's target_id is INVALID), iscsid should not sync this
> session in it's restart.
> 
> So we need to check session's target state during iscsid restart,
> if session is in unbound state, do not sync this session and perform
> session teardown. It's reasonable because once a session is unbound, we
> can not recover it any more(mainly because it's target id is INVALID)
> 
> V6:
> - Set target state to ALLOCATED in iscsi_add_session
> - Rename state BOUND to SCANNED
> - Make iscsi_session_target_state_name() more efficient
> 
> V5:
> - Add ISCSI_SESSION_TARGET_ALLOCATED to indicate the session's
>   target has been allocated but not scanned yet. We should
>   sync this session and scan this session when iscsid restarted.
> 
> V4:
> - Move debug print out of spinlock critical section
> 
> V3:
> - Make target bind state to a state kind rather than a bool.
> 
> V2:
> - Using target_unbound rather than state to indicate session has been
>   unbound
> 
> Signed-off-by: Wenchao Hao 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 57 -
>  include/scsi/scsi_transport_iscsi.h |  9 +
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..4bbd1aa4a609 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1676,6 +1676,21 @@ static const char *iscsi_session_state_name(int state)
>   return name;
>  }
>  
> +static char *iscsi_session_target_state_names[] = {
> + "UNBOUND",
> + "ALLOCATED",
> + "SCANNED",
> + "UNBINDING",
> +};

I think maybe Lee meant you to do something like:

static int iscsi_target_state_to_name[] = {
[ISCSI_SESSION_TARGET_UNBOUND] = "UNBOUND",
[ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
.


> +
> +static const char *iscsi_session_target_state_name(int state)
> +{
> + if (state > ISCSI_SESSION_TARGET_MAX)
> + return NULL;
> +
> + return iscsi_session_target_state_names[state];
> +}
> +
>  int iscsi_session_chkready(struct iscsi_cls_session *session)
>  {
>   int err;
> @@ -1785,9 +1800,13 @@ static int iscsi_user_scan_session(struct device *dev, 
> void *data)
>   if ((scan_data->channel == SCAN_WILD_CARD ||
>scan_data->channel == 0) &&
>   (scan_data->id == SCAN_WILD_CARD ||
> -  scan_data->id == id))
> +  scan_data->id == id)) {
>   scsi_scan_target(>dev, 0, id,
>scan_data->lun, scan_data->rescan);
> + spin_lock_irqsave(>lock, flags);
> + session->target_state = ISCSI_SESSION_TARGET_SCANNED;
> + spin_unlock_irqrestore(>lock, flags);
> + }
>   }
>  
>  user_scan_exit:
> @@ -1961,6 +1980,21 @@ static void __iscsi_unbind_session(struct work_struct 
> *work)
>   unsigned long flags;
>   unsigned int target_id;
>  
> + spin_lock_irqsave(>lock, flags);
> + if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
> + spin_unlock_irqrestore(>lock, flags);
> + if (session->ida_used)
> + ida_free(_sess_ida, session->target_id);
> + ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: 
> allocated\n");

Could you change the error message to "Skipping target unbinding: Session not 
yet scanned.\n"

> + goto unbind_session_exit;
> + }

Just add a newline/return here.

> + if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
> + spin_unlock_irqrestore(>lock, flags);
> + ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: 
> unbinding or unbound\n");> +return;

Could you change the error message to "Skipping target unbinding: Session is 
unbound/unbinding.\n"

> + }
> + spin_unlock_irqrestore(>lock, flags);
> +
>   ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>  
>   /* Prevent new scans and make sure scanning is not in progress */


I think you want to move both state 

Re: [PATCH] recovery: remove onlining of devices via sysfs

[Mike Christie]

On 8/29/22 3:00 PM, Uday Shankar wrote:
>> I hit the hang below and it should be fixed in this set:
>>
>> https://urldefense.com/v3/__https://lore.kernel.org/all/20211105221048.6541-1-michael.chris...@oracle.com/__;!!ACWV5N9M2RV99hQ!L10OSfsVJ6Bssm8eT4oRSTBOLfCKp7-4cM5kGDF4aXnYuBfB0xlTEzbFTvMMw0nqmbfLqSpaXVl-E_tVvIc6ZQqBrA$
>>   
> 
> I hit the hang I described while running a kernel with your fix in. My

Ah yeah sorry. I didn't read your first mail well. I tried to do a
quick reply from my phone on vacation :)

> hang does not involve scsi-eh; it happens when a transport error is
> detected by the kernel while iscsid is in the process of handling a
> previous transport error. If the second transport error happens at an
> unlucky timing (after iscsid does start_conn when handling the first> error, 
> but before it sets the device state to running via sysfs), the
> device queues can be quiesced (in the block layer sense) when iscsid
> writes to sysfs and ends up in scsi_rescan_device.

Ok got it.

> 
> I wanted to avoid this by just removing the write to sysfs from iscsid,
> but it seems that interferes with scsi-eh flow:
> 
>> The user space online code in your patch handles the scsi-eh case.
> 
> So I will need to try another approach. Simply adding a call to
> scsi_start_queue like so
> 
> if (rescan_dev) {
>   /* ...
>*/
> + scsi_start_queue(sdev);
>   blk_mq_run_hw_queues(sdev->request_queue, true);
>   scsi_rescan_device(dev);
> }
> 
> does not fix the issue either, since your linked patch moves the
> run_hw_queues and rescan calls out of the area protected by state_mutex.
> A race window still exists where the kernel could detect the second
> transport error and quiesce sdev's request queue after we unquiesce it
> but before we run the queues and rescan the device above.
> 
> It seems that the most straightforward way to fix my issue without
> undoing your patch is to add another sync primitive that guards the sdev
> queue quiesced/unquiesced state so that we can guarantee the queues are
> unquiesced when we do the rescan above. Unless you have other
> suggestions, I'll try that approach.
> 

Here is an alternative I was thinking about when I did that other
patchset.

I think we could remove the session_online_devs call for newer kernels.

The session_online_devs call was handling the case where a scsi cmd
timesout because the network is lost, we run the scsi eh, that ends
up getting escalated to us dropping the session, then iscsi_eh_session_reset
fails due to the replacement_timeout expiring.

With the patch:

commit a0c2f8b6709a9a4af175497ca65f93804f57b248
Author: Mike Christie 
Date:   Fri Nov 5 17:10:47 2021 -0500

scsi: iscsi: Unblock session then wake up error handler


we should always end up in the transport-offline state for that
type of case. When we relogin, then the conn start's call to
__iscsi_unblock_session -> scsi_target_unblock will handle that
case and set the device to running. So we should not need the
userspace online/running call anymore.

So we could just add a CAP_SCSI_EH_TRANSPORT OFFLINE flag to
the iscsi_transport->caps struct. When userspace sees that then
it knows the kernel will now do the right thing.

The drawback is that we have to patch userspace and then also
get the the new CAP_SCSI_EH_TRANSPORT_OFFLINE patch in the stable
kernels.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/644defe5-2a33-9eda-ff7f-f600e635a48b%40oracle.com.

Re: [PATCH v3] scsi: iscsi: Fix multiple iscsi session unbind event sent to userspace

[Mike Christie]

On 8/1/22 10:47 PM, Wenchao Hao wrote:
> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
> for multiple times which should be fixed.
> 
> This patch introduce target_state in iscsi_cls_session to make
> sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.
> 
> But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
> Report unbind session event when the target has been removed"). The issue
> is iscsid died for any reason after it send unbind session to kernel, once
> iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.
> 
> Now kernel think iscsi_cls_session has already sent an
> ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
> would cause userspace unable to logout. Actually the session is in
> invalid state(it's target_id is INVALID), iscsid should not sync this
> session in it's restart.
> 
> So we need to check session's target state during iscsid restart,
> if session is in unbound state, do not sync this session and perform
> session teardown. It's reasonable because once a session is unbound, we
> can not recover it any more(mainly because it's target id is INVALID)
> 

I think we are ok now. Do you have a link to the userspace parts so
I can make sure we have everything covered now?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/3ba71030-982b-c98b-78ee-ceca74da3b57%40oracle.com.

Re: [PATCH] scsi: iscsi: iscsi_tcp: Fix null-ptr-deref while calling getpeername()

[Mike Christie]

On 8/2/22 6:23 AM, lijinlin (A) wrote:
> So sorry, this patch has problem, please ignore.
> 

Was the issue the fget use?

I know I gave the suggestion to do the get, but seeing it now makes
me think I was wrong and it's getting too messy.

Let's just add a mutex for getting/setting the tcp_sw_conn->sock in
the non-io paths (io paths are flushed/locked already). Something like
this (patch is only compile tested):

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 9fee70d6434a..c1696472965e 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -558,6 +558,8 @@ iscsi_sw_tcp_conn_create(struct iscsi_cls_session 
*cls_session,
tcp_conn = conn->dd_data;
tcp_sw_conn = tcp_conn->dd_data;
 
+   mutex_init(_sw_conn->sock_lock);
+
tfm = crypto_alloc_ahash("crc32c", 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm))
goto free_conn;
@@ -592,11 +594,15 @@ iscsi_sw_tcp_conn_create(struct iscsi_cls_session 
*cls_session,
 
 static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn)
 {
-   struct iscsi_session *session = conn->session;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
struct socket *sock = tcp_sw_conn->sock;
 
+   /*
+* The iscsi transport class will make sure we are not called in
+* parallel with start, stop, bind and destroys. However, this can be
+* called twice if userspace does a stop then a destroy.
+*/
if (!sock)
return;
 
@@ -610,9 +616,9 @@ static void iscsi_sw_tcp_release_conn(struct iscsi_conn 
*conn)
iscsi_sw_tcp_conn_restore_callbacks(conn);
sock_put(sock->sk);
 
-   spin_lock_bh(>frwd_lock);
+   mutex_lock(_sw_conn->sock_lock);
tcp_sw_conn->sock = NULL;
-   spin_unlock_bh(>frwd_lock);
+   mutex_unlock(_sw_conn->sock_lock);
sockfd_put(sock);
 }
 
@@ -664,7 +670,6 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session 
*cls_session,
   struct iscsi_cls_conn *cls_conn, uint64_t transport_eph,
   int is_leading)
 {
-   struct iscsi_session *session = cls_session->dd_data;
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
@@ -684,10 +689,10 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session 
*cls_session,
if (err)
goto free_socket;
 
-   spin_lock_bh(>frwd_lock);
+   mutex_lock(_sw_conn->sock_lock);
/* bind iSCSI connection and socket */
tcp_sw_conn->sock = sock;
-   spin_unlock_bh(>frwd_lock);
+   mutex_unlock(_sw_conn->sock_lock);
 
/* setup Socket parameters */
sk = sock->sk;
@@ -724,8 +729,15 @@ static int iscsi_sw_tcp_conn_set_param(struct 
iscsi_cls_conn *cls_conn,
break;
case ISCSI_PARAM_DATADGST_EN:
iscsi_set_param(cls_conn, param, buf, buflen);
+
+   mutex_lock(_sw_conn->sock_lock);
+   if (!tcp_sw_conn->sock) {
+   mutex_unlock(_sw_conn->sock_lock);
+   return -ENOTCONN;
+   }
tcp_sw_conn->sendpage = conn->datadgst_en ?
sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
+   mutex_unlock(_sw_conn->sock_lock);
break;
case ISCSI_PARAM_MAX_R2T:
return iscsi_tcp_set_max_r2t(conn, buf);
@@ -750,14 +762,20 @@ static int iscsi_sw_tcp_conn_get_param(struct 
iscsi_cls_conn *cls_conn,
case ISCSI_PARAM_CONN_PORT:
case ISCSI_PARAM_CONN_ADDRESS:
case ISCSI_PARAM_LOCAL_PORT:
-   spin_lock_bh(>session->frwd_lock);
-   if (!tcp_sw_conn || !tcp_sw_conn->sock) {
-   spin_unlock_bh(>session->frwd_lock);
+   /*
+* The conn's sysfs interface is exposed to userspace after
+* the tcp_sw_conn is setup, and the netlink interface will
+* make sure we don't do a get_param while setup is running.
+* We do need to make sure a user is not accessing sysfs while
+* the netlink interface is releasing the sock via
+* iscsi_sw_tcp_release_conn.
+*/
+   mutex_lock(_sw_conn->sock_lock);
+   sock = tcp_sw_conn->sock;
+   if (!sock) {
+   mutex_unlock(_sw_conn->sock_lock);
return -ENOTCONN;
}
-   sock = tcp_sw_conn->sock;
-   sock_hold(sock->sk);
-   spin_unlock_bh(>session->frwd_lock);
 
if (param == ISCSI_PARAM_LOCAL_PORT)
rc = kernel_getsockname(sock,
@@ -765,7 +783,7 @@ static int iscsi_sw_tcp_conn_get_param(struct 
iscsi_cls_conn *cls_conn,
   

Re: [PATCH] scsi: iscsi: prefer xmit of DataOut before new cmd

[Mike Christie]

On 6/7/22 8:19 AM, Dmitry Bogdanov wrote:
> In function iscsi_data_xmit (TX worker) there is walking through the
> queue of new SCSI commands that is replenished in parallell. And only
> after that queue got emptied the function will start sending pending
> DataOut PDUs. That lead to DataOut timer time out on target side and
> to connection reinstatment.
> 
> This patch swaps walking through the new commands queue and the pending
> DataOut queue. To make a preference to ongoing commands over new ones.
> 
> Reviewed-by: Konstantin Shelekhin 
> Signed-off-by: Dmitry Bogdanov 

Let's do this patch. I've tried so many combos of implementations and
they all have different perf gains or losses with different workloads.
I've already been going back and forth with myself for over a year
(the link for my patch in the other mail was version N) and I don't
think a common solution is going to happen.

You patch fixes the bug, and I've found a workaround for my issue
where I tweak the queue depth, so I think we will be ok.

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/237bed01-819a-55be-5163-274fac3b61e6%40oracle.com.

Re: [PATCH] scsi: iscsi: prefer xmit of DataOut before new cmd

[Mike Christie]

On 6/9/22 4:02 AM, Dmitriy Bogdanov wrote:
> Hi Mike,
> 
>>> On 6/8/22 9:16 AM, Dmitriy Bogdanov wrote:
>>>> On 6/7/22 10:55 AM, Mike Christie wrote:
>>>>> On 6/7/22 8:19 AM, Dmitry Bogdanov wrote:
>>>>>> In function iscsi_data_xmit (TX worker) there is walking through the
>>>>>> queue of new SCSI commands that is replenished in parallell. And only
>>>>>> after that queue got emptied the function will start sending pending
>>>>>> DataOut PDUs. That lead to DataOut timer time out on target side and
>>>>>> to connection reinstatment.
>>>>>>
>>>>>> This patch swaps walking through the new commands queue and the pending
>>>>>> DataOut queue. To make a preference to ongoing commands over new ones.
>>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>>  task = list_entry(conn->cmdqueue.next, struct iscsi_task,
>>>>>> @@ -1594,28 +1616,10 @@ static int iscsi_data_xmit(struct iscsi_conn 
>>>>>> *conn)
>>>>>>   */
>>>>>>  if (!list_empty(>mgmtqueue))
>>>>>>  goto check_mgmt;
>>>>>> +if (!list_empty(>requeue))
>>>>>> +goto check_requeue;
>>>>>
>>>>>
>>>>>
>>>>> Hey, I've been posting a similar patch:
>>>>>
>>>>> https://urldefense.com/v3/__https://www.spinics.net/lists/linux-scsi/msg156939.html__;!!ACWV5N9M2RV99hQ!LHLghPLuyBZadpsGme03-HBoowa8sNiZYMKxKoz5E_BNu-M9-BiuNV_JS9kFxhnumNfhrxuR7qVdIaOH5X7iTfMO$
>>>>>
>>>>> A problem I hit is a possible pref regression so I tried to allow
>>>>> us to start up a burst of cmds in parallel. It's pretty simple where
>>>>> we allow up to a queue's worth of cmds to start. It doesn't try to
>>>>> check that all cmds are from the same queue or anything fancy to try
>>>>> and keep the code simple. Mostly just assuming users might try to bunch
>>>>> cmds together during submission or they might hit the queue plugging
>>>>> code.
>>>>>
>>>>> What do you think?
>>>>
>>>> Oh yeah, what about a modparam batch_limit? It's between 0 and cmd_per_lun.
>>>> 0 would check after every transmission like above.
>>>
>>>  Did you really face with a perf regression? I could not imagine how it is
>>> possible.
>>> DataOut PDU contains a data too, so a throughput performance cannot be
>>> decreased by sending DataOut PDUs.
>>
>>
>> We can agree that queue plugging and batching improves throughput right?
>> The app or block layer may try to batch commands. It could be with something
>> like fio's batch args or you hit the block layer queue plugging.
> 
> I agree that those features 100% gives an improvement of a throughput on local
> devices on serial interfaces like SATA1. Since SATA2 (Native Command Queuing)
> devices can reorder incoming commmands to provide the best thoughput.
> SCSI I guess has similar feature from the very beginning.
> But on remote devices (iSCSI and FC) it is not 100% - initiators's command
> order may be reordered by the network protocol nature itself. I mean 1PDU vs
> R2T+DataOut PDUs, PDU resending due to crc errors or something like that.
I think we are talking about slightly different things. You are coming up with
different possible scenarios where it doesn't work. I get them. You are correct
for those cases. I'm not talking about those cases. I'm talking about the 
specific
case I described.

I'm saying we have targets where we use backends that get improved performance
when they get batched cmds. When the network is ok, and the user's app is
batching cmds, they come from the app down to the target's backend device as
a batch. My concern is that with your patch we will no longer get that behavior.

The reason is that the target and initiator can do:

1. initiator sends scsi cmd pdu1
2. target sends R2T
3. initiator sees R2T and hits the goto. Sends data
4. target reads in data. Sends cmd to block layer.
5. initiator sends scsi cmd pdu2
6. target sends R2T
7. initiator reads in R2T sends data.
8. target reads in data and sends cmd to block layer.

The problem here could be between 4 and 8 the block layer has run the queue
and sent that one cmd to the real device already because we have that extra
delay now. So when I implemented the fix in the same way as you and I run
iostat I would see lower aqu-sz for example.

With the current code we migh

Re: [PATCH] scsi: iscsi: prefer xmit of DataOut before new cmd

[Mike Christie]

On 6/8/22 9:16 AM, Dmitriy Bogdanov wrote:
> Hi Mike,
> 
>> On 6/7/22 10:55 AM, Mike Christie wrote:
>>> On 6/7/22 8:19 AM, Dmitry Bogdanov wrote:
>>>> In function iscsi_data_xmit (TX worker) there is walking through the
>>>> queue of new SCSI commands that is replenished in parallell. And only
>>>> after that queue got emptied the function will start sending pending
>>>> DataOut PDUs. That lead to DataOut timer time out on target side and
>>>> to connection reinstatment.
>>>>
>>>> This patch swaps walking through the new commands queue and the pending
>>>> DataOut queue. To make a preference to ongoing commands over new ones.
>>>>
>>>
>>> ...
>>>
>>>>  task = list_entry(conn->cmdqueue.next, struct iscsi_task,
>>>> @@ -1594,28 +1616,10 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
>>>>   */
>>>>  if (!list_empty(>mgmtqueue))
>>>>  goto check_mgmt;
>>>> +if (!list_empty(>requeue))
>>>> +goto check_requeue;
>>>
>>>
>>>
>>> Hey, I've been posting a similar patch:
>>>
>>> https://urldefense.com/v3/__https://www.spinics.net/lists/linux-scsi/msg156939.html__;!!ACWV5N9M2RV99hQ!LHLghPLuyBZadpsGme03-HBoowa8sNiZYMKxKoz5E_BNu-M9-BiuNV_JS9kFxhnumNfhrxuR7qVdIaOH5X7iTfMO$
>>>  
>>>
>>> A problem I hit is a possible pref regression so I tried to allow
>>> us to start up a burst of cmds in parallel. It's pretty simple where
>>> we allow up to a queue's worth of cmds to start. It doesn't try to
>>> check that all cmds are from the same queue or anything fancy to try
>>> and keep the code simple. Mostly just assuming users might try to bunch
>>> cmds together during submission or they might hit the queue plugging
>>> code.
>>>
>>> What do you think?
>>
>> Oh yeah, what about a modparam batch_limit? It's between 0 and cmd_per_lun.
>> 0 would check after every transmission like above.
> 
>  Did you really face with a perf regression? I could not imagine how it is
> possible.
> DataOut PDU contains a data too, so a throughput performance cannot be
> decreased by sending DataOut PDUs.


We can agree that queue plugging and batching improves throughput right?
The app or block layer may try to batch commands. It could be with something
like fio's batch args or you hit the block layer queue plugging.

With the current code we can end up sending all cmds to the target in a way
the target can send them to the real device batched. For example, we send off
the initial N scsi command PDUs in one run of iscsi_data_xmit. The target reads
them in, and sends off N R2Ts. We are able to read N R2Ts in the same call.
And again we are able to send the needed data for them in one call of
iscsi_data_xmit. The target is able to read in the data and send off the
WRITEs to the physical device in a batch.

With your patch, we can end up not batching them like the app/block layer
intended. For example, we now call iscsi_data_xmit and in the cmdqueue loop.
We've sent N - M scsi cmd PDUs, then see that we've got an incoming R2T to
handle. So we goto check_requeue. We send the needed data. The target then
starts to send the cmd to the physical device. If we have read in multiple
R2Ts then we will continue the requeue loop. And so we might be able to send
the data fast enough that the target can then send those commands to the
physical device. But we've now broken up the batching the upper layers sent
to us and we were doing before.

> 
>  The only thing is a latency performance. But that is not an easy question.

Agree latency is important and that's why I was saying we can make it config
option. Users can continue to try and batch their cmds and we don't break
them. We also fix the bug in that we don't get stuck in the cmdqueue loop
always taking in new cmds.

> IMHO, a system should strive to reduce a maximum value of the latency almost
> without impacting of a minimum value (prefer current commands) instead of
> to reduce a minimum value of the latency to the detriment of maximum value
> (prefer new commands).
> 
>  Any preference of new commands over current ones looks like an io scheduler

I can see your point of view where you see it as preferring new cmds
vs existing. It's probably due to my patch not hooking into commit_rqs
and trying to figure out the batching exactly. It's more of a simple
estimate.

However, that's not what I'm talking about. I'm talking about the block
layer / iosched has sent us these commands as a batch. We are now more
likely to break that up.

> functionality,

Re: [PATCH] scsi: iscsi: prefer xmit of DataOut before new cmd

[Mike Christie]

On 6/7/22 10:55 AM, Mike Christie wrote:
> On 6/7/22 8:19 AM, Dmitry Bogdanov wrote:
>> In function iscsi_data_xmit (TX worker) there is walking through the
>> queue of new SCSI commands that is replenished in parallell. And only
>> after that queue got emptied the function will start sending pending
>> DataOut PDUs. That lead to DataOut timer time out on target side and
>> to connection reinstatment.
>>
>> This patch swaps walking through the new commands queue and the pending
>> DataOut queue. To make a preference to ongoing commands over new ones.
>>
> 
> ...
> 
>>  task = list_entry(conn->cmdqueue.next, struct iscsi_task,
>> @@ -1594,28 +1616,10 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
>>   */
>>  if (!list_empty(>mgmtqueue))
>>  goto check_mgmt;
>> +if (!list_empty(>requeue))
>> +goto check_requeue;
> 
> 
> 
> Hey, I've been posting a similar patch:
> 
> https://www.spinics.net/lists/linux-scsi/msg156939.html
> 
> A problem I hit is a possible pref regression so I tried to allow
> us to start up a burst of cmds in parallel. It's pretty simple where
> we allow up to a queue's worth of cmds to start. It doesn't try to
> check that all cmds are from the same queue or anything fancy to try
> and keep the code simple. Mostly just assuming users might try to bunch
> cmds together during submission or they might hit the queue plugging
> code.
> 
> What do you think?

Oh yeah, what about a modparam batch_limit? It's between 0 and cmd_per_lun.
0 would check after every transmission like above.



-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/6a58acb4-e29e-e8c7-d85c-fe474670dad7%40oracle.com.

Re: [PATCH] scsi: iscsi: prefer xmit of DataOut before new cmd

[Mike Christie]

On 6/7/22 8:19 AM, Dmitry Bogdanov wrote:
> In function iscsi_data_xmit (TX worker) there is walking through the
> queue of new SCSI commands that is replenished in parallell. And only
> after that queue got emptied the function will start sending pending
> DataOut PDUs. That lead to DataOut timer time out on target side and
> to connection reinstatment.
> 
> This patch swaps walking through the new commands queue and the pending
> DataOut queue. To make a preference to ongoing commands over new ones.
> 

...

>   task = list_entry(conn->cmdqueue.next, struct iscsi_task,
> @@ -1594,28 +1616,10 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
>*/
>   if (!list_empty(>mgmtqueue))
>   goto check_mgmt;
> + if (!list_empty(>requeue))
> + goto check_requeue;



Hey, I've been posting a similar patch:

https://www.spinics.net/lists/linux-scsi/msg156939.html

A problem I hit is a possible pref regression so I tried to allow
us to start up a burst of cmds in parallel. It's pretty simple where
we allow up to a queue's worth of cmds to start. It doesn't try to
check that all cmds are from the same queue or anything fancy to try
and keep the code simple. Mostly just assuming users might try to bunch
cmds together during submission or they might hit the queue plugging
code.

What do you think?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/769c3acb-b515-7fd8-2450-4b6206436fde%40oracle.com.

Re: [bug report] scsi: iscsi: Remove iscsi_get_task back_lock requirement

[Mike Christie]

On 5/23/22 1:58 AM, Dan Carpenter wrote:
> Hello Mike Christie,
> 
> The patch a01ff1e161ea: "scsi: iscsi: Remove iscsi_get_task back_lock
> requirement" from May 18, 2022, leads to the following Smatch static
> checker warning:
> 
>   drivers/scsi/libiscsi_tcp.c:649 iscsi_tcp_r2t_rsp()
>   warn: inconsistent returns '>back_lock'.
> 

Thanks Dan. Yeah, it's a bug. I'm just waiting on Martin to see how he
wants to resolve it (new patchset, new patch, or patch on top of staging).

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/d0e54ea4-dfcd-8866-a736-d9b6de29ce9c%40oracle.com.

Re: [PATCH] scsi: iscsi: fix harmless double shift bug

[Mike Christie]

On 4/21/22 10:03 AM, Dan Carpenter wrote:
> These flags are supposed to be bit numbers.  Right now they cause a
> double shift bug where we use BIT(BIT(2)) instead of BIT(2).
> Fortunately, the bit numbers are small and it's done consistently so it
> does not cause an issue at run time.
> 
> Fixes: 5bd856256f8c ("scsi: iscsi: Merge suspend fields")
> Signed-off-by: Dan Carpenter 
> ---
>  include/scsi/libiscsi.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index d0a24779c52d..c0703cd20a99 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -54,9 +54,9 @@ enum {
>  #define ISID_SIZE6
>  
>  /* Connection flags */
> -#define ISCSI_CONN_FLAG_SUSPEND_TX   BIT(0)
> -#define ISCSI_CONN_FLAG_SUSPEND_RX   BIT(1)
> -#define ISCSI_CONN_FLAG_BOUNDBIT(2)
> +#define ISCSI_CONN_FLAG_SUSPEND_TX   0
> +#define ISCSI_CONN_FLAG_SUSPEND_RX   1
> +#define ISCSI_CONN_FLAG_BOUND2
>  
>  #define ISCSI_ITT_MASK       0x1fff
>  #define ISCSI_TOTAL_CMDS_MAX 4096

Thanks.

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/9a928aff-d577-6fcc-701b-cb2ac93da9bb%40oracle.com.

Re: [PATCH v2] scsi: iscsi: Fix multiple iscsi session unbind event sent to userspace

[Mike Christie]

On 4/17/22 7:06 PM, Wenchao Hao wrote:
> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
> for multiple times which should be fixed.
> 
> This patch introduce target_unbound in iscsi_cls_session to make
> sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.
> 
> But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
> Report unbind session event when the target has been removed"). The issue
> is iscsid died for any reason after it send unbind session to kernel, once
> iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.
> 
> Now kernel think iscsi_cls_session has already sent an
> ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
> would cause userspace unable to logout. Actually the session is in
> invalid state(it's target_id is INVALID), iscsid should not sync this
> session in it's restart.
> 
> So we need to check session's target unbound state during iscsid restart,
> if session is in unbound state, do not sync this session and perform
> session teardown. It's reasonable because once a session is unbound, we
> can not recover it any more(mainly because it's target id is INVALID)
> 
> Changes from V1:
> - Using target_unbound rather than state to indicate session has been
>   unbound
> 
> Signed-off-by: Wenchao Hao 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 21 +
>  include/scsi/scsi_transport_iscsi.h |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 2c0dd64159b0..43ba31e595b4 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1958,6 +1958,14 @@ static void __iscsi_unbind_session(struct work_struct 
> *work)
>  
>   ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>  
> + spin_lock_irqsave(>lock, flags);
> + if (session->target_unbound) {
> + spin_unlock_irqrestore(>lock, flags);
> + return;
> + }
> + session->target_unbound = 1;

Shoot, sorry, I think I gave you a bad review comment when I said we
could do a bool or state kind or variable.

If we set unbound here and iscsid was restarting at this point then
iscsid really only knows the target removal process is starting up. It
doesn't know that the target is not yet removed. We could be doing sync
caches and/or still tearing down scsi_devices/LUNs.

For the comments I gave you on the userspace PR parts, would it be
easier if this was a state type of value? Above you would set it to
REMOVING. When scsi_remove_target is done then we can set it to
REMOVED. That combined with the session and conn states we can detect
how far we got in the session removal process if iscsid dies in the
middle of it.

What do you think?


> + spin_unlock_irqrestore(>lock, flags);
> +
>   /* Prevent new scans and make sure scanning is not in progress */
>   mutex_lock(>mutex);
>   spin_lock_irqsave(>lock, flags)

...

> diff --git a/include/scsi/scsi_transport_iscsi.h 
> b/include/scsi/scsi_transport_iscsi.h
> index 9acb8422f680..877632c25e56 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -256,6 +256,7 @@ struct iscsi_cls_session {
>   struct workqueue_struct *workq;
>  
>   unsigned int target_id;
> + int target_unbound;   /* make sure unbind session only once */


We don't need the comment since the code using this is so simple
and the name of the variable tells us what it's for.


>   bool ida_used;
>  
>   /*

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/938bca13-2dcc-24c0-51b5-26f7e7238776%40oracle.com.

Re: [PATCH 2/2] iscsi: set session to FREE state after unbind session in remove session

[Mike Christie]

On 4/15/22 4:40 AM, Wenchao Hao wrote:
> On 2022/4/14 23:30, Mike Christie wrote:
>> On 4/13/22 8:49 PM, Wenchao Hao wrote:
>>> __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which
>>> would overwrite the ISCSI_SESSION_FREE state.
>>>
>>> Signed-off-by: Wenchao Hao 
>>> ---
>>>  drivers/scsi/scsi_transport_iscsi.c | 26 --
>>>  1 file changed, 16 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
>>> b/drivers/scsi/scsi_transport_iscsi.c
>>> index 97a9fee02efa..d8dd9279cea8 100644
>>> --- a/drivers/scsi/scsi_transport_iscsi.c
>>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>>> @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session 
>>> *session)
>>> if (!cancel_work_sync(>block_work))
>>> cancel_delayed_work_sync(>recovery_work);
>>> cancel_work_sync(>unblock_work);
>>> +
>>> +   scsi_target_unblock(>dev, SDEV_TRANSPORT_OFFLINE);
>>> +   /*
>>> +* qla4xxx can perform it's own scans when it runs in kernel only
>>> +* mode. Make sure to flush those scans.
>>> +*/
>>> +   flush_work(>scan_work);
>>> +
>>> +   /*
>>> +* flush running unbind operations
>>> +* if unbind work did not queued, call __iscsi_unbind_session
>>> +* directly to perform target remove
>>
>> We probably don't need the flush_work test because we are going to
>> normally call __iscsi_unbind_session.
>>
> 
> I think we still need calling flush_work here. The introduce of flush_work

Above I was saying we don't need to *test* if it returned true/false.
Just always flush and then call __iscsi_unbind_session like we did
originally below. The check you added in __iscsi_unbind_session in the
first patch will detect if it's been called or not.


> is to make sure sysfs objects are removed in an correct order. There is a
> very low probability that __iscsi_unbind_session() triggered by queue_work()
> has not been finished, and iscsi_remove_session() is called. So we need
> flush_work() to make sure __iscsi_unbind_session() has done if it has been
> activated by queue_work().
> 
>> If the unbind work had already run, which is the normal case, then
>> flush_work returns false and we end up calling __iscsi_unbind_session
>> like before. That function then checks if the target is really unbound.
>> So the extra check doesn't normally buy us anything with your patches
>> because in patch 1 you fixed it so __iscsi_unbind_session doesn't send
>> the extra event.
>>
>>
>>> +*/
>>> +   if (!flush_work(>unbind_work))
>>> +   __iscsi_unbind_session(>unbind_work);
>>> +
>>> /*
>>>  * If we are blocked let commands flow again. The lld or iscsi
>>>  * layer should set up the queuecommand to fail commands.
>>> @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session 
>>> *session)
>>> session->state = ISCSI_SESSION_FREE;
>>> spin_unlock_irqrestore(>lock, flags);
>>>  
>>> -   scsi_target_unblock(>dev, SDEV_TRANSPORT_OFFLINE);
>>> -   /*
>>> -* qla4xxx can perform it's own scans when it runs in kernel only
>>> -* mode. Make sure to flush those scans.
>>> -*/
>>> -   flush_work(>scan_work);
>>> -   /* flush running unbind operations */
>>> -   flush_work(>unbind_work);
>>> -   __iscsi_unbind_session(>unbind_work);
>>> -
>>> /* hw iscsi may not have removed all connections from session */
>>> err = device_for_each_child(>dev, NULL,
>>> iscsi_iter_destroy_conn_fn);
>>
>> .
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/8d3c80f0-7ec1-4b6e-3e3f-87be1ef8c8cb%40oracle.com.

Re: [PATCH 2/2] iscsi: set session to FREE state after unbind session in remove session

[Mike Christie]

On 4/13/22 8:49 PM, Wenchao Hao wrote:
> __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which
> would overwrite the ISCSI_SESSION_FREE state.
> 
> Signed-off-by: Wenchao Hao 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 26 --
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 97a9fee02efa..d8dd9279cea8 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session 
> *session)
>   if (!cancel_work_sync(>block_work))
>   cancel_delayed_work_sync(>recovery_work);
>   cancel_work_sync(>unblock_work);
> +
> + scsi_target_unblock(>dev, SDEV_TRANSPORT_OFFLINE);
> + /*
> +  * qla4xxx can perform it's own scans when it runs in kernel only
> +  * mode. Make sure to flush those scans.
> +  */
> + flush_work(>scan_work);
> +
> + /*
> +  * flush running unbind operations
> +  * if unbind work did not queued, call __iscsi_unbind_session
> +  * directly to perform target remove

We probably don't need the flush_work test because we are going to
normally call __iscsi_unbind_session.

If the unbind work had already run, which is the normal case, then
flush_work returns false and we end up calling __iscsi_unbind_session
like before. That function then checks if the target is really unbound.
So the extra check doesn't normally buy us anything with your patches
because in patch 1 you fixed it so __iscsi_unbind_session doesn't send
the extra event.


> +  */
> + if (!flush_work(>unbind_work))
> + __iscsi_unbind_session(>unbind_work);
> +
>   /*
>* If we are blocked let commands flow again. The lld or iscsi
>* layer should set up the queuecommand to fail commands.
> @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session 
> *session)
>   session->state = ISCSI_SESSION_FREE;
>   spin_unlock_irqrestore(>lock, flags);
>  
> - scsi_target_unblock(>dev, SDEV_TRANSPORT_OFFLINE);
> - /*
> -  * qla4xxx can perform it's own scans when it runs in kernel only
> -  * mode. Make sure to flush those scans.
> -  */
> - flush_work(>scan_work);
> - /* flush running unbind operations */
> - flush_work(>unbind_work);
> - __iscsi_unbind_session(>unbind_work);
> -
>   /* hw iscsi may not have removed all connections from session */
>   err = device_for_each_child(>dev, NULL,
>   iscsi_iter_destroy_conn_fn);

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/a8087705-2cea-f01c-ce67-639e97edc30a%40oracle.com.

Re: [PATCH 1/2] scsi: iscsi: introduce session UNBOUND state to avoid multiple unbind event

[Mike Christie]

On 4/13/22 8:49 PM, Wenchao Hao wrote:
> Fix the issue of kernel send multiple ISCSI_KEVENT_UNBIND_SESSION event.
> If session is in UNBOUND state, do not perform unbind operations anymore,
> else unbind session and set session to UNBOUND state.
> 

I don't think we want this to be a state because you can have a session
with no target or it could be partially deleted and it could be in the
logged in or failed state. If scsi-ml is sending SYNC_CACHEs as part of
the target/device removal operation, and we lose the session then we could
go through recovery and the state will go from failed to logged in, and
your new unbound state will have been overwritten.

I think it might be better to have a new sysfs file, target_state, for
this where you could have values like scanning, bound, unbinding, and
unbound, or just a sysfs file, target_bound, that is bool.

> Reference:https://github.com/open-iscsi/open-iscsi/issues/338
> 

You should add a description of the problem in the commit, because that
link might be gone one day.


> Signed-off-by: Wenchao Hao 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 19 +--
>  include/scsi/scsi_transport_iscsi.h |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 27951ea05dd4..97a9fee02efa 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1656,6 +1656,7 @@ static struct {
>   { ISCSI_SESSION_LOGGED_IN,  "LOGGED_IN" },
>   { ISCSI_SESSION_FAILED, "FAILED" },
>   { ISCSI_SESSION_FREE,   "FREE" },
> + { ISCSI_SESSION_UNBOUND,"UNBOUND" },
>  };
>  
>  static const char *iscsi_session_state_name(int state)
> @@ -1686,6 +1687,9 @@ int iscsi_session_chkready(struct iscsi_cls_session 
> *session)
>   case ISCSI_SESSION_FREE:
>   err = DID_TRANSPORT_FAILFAST << 16;
>   break;
> + case ISCSI_SESSION_UNBOUND:
> + err = DID_NO_CONNECT << 16;
> + break;
>   default:
>   err = DID_NO_CONNECT << 16;
>   break;
> @@ -1838,7 +1842,8 @@ int iscsi_block_scsi_eh(struct scsi_cmnd *cmd)
>  
>   spin_lock_irqsave(>lock, flags);
>   while (session->state != ISCSI_SESSION_LOGGED_IN) {
> - if (session->state == ISCSI_SESSION_FREE) {
> + if ((session->state == ISCSI_SESSION_FREE) ||
> + (session->state == ISCSI_SESSION_UNBOUND)) {
>   ret = FAST_IO_FAIL;
>   break;
>   }
> @@ -1869,6 +1874,7 @@ static void session_recovery_timedout(struct 
> work_struct *work)
>   break;
>   case ISCSI_SESSION_LOGGED_IN:
>   case ISCSI_SESSION_FREE:
> + case ISCSI_SESSION_UNBOUND:
>   /* we raced with the unblock's flush */
>   spin_unlock_irqrestore(>lock, flags);
>   return;
> @@ -1957,6 +1963,14 @@ static void __iscsi_unbind_session(struct work_struct 
> *work)
>   unsigned long flags;
>   unsigned int target_id;
>  
> + spin_lock_irqsave(>lock, flags);
> + if (session->state == ISCSI_SESSION_UNBOUND) {
> + spin_unlock_irqrestore(>lock, flags);
> + return;
> + }
> + session->state = ISCSI_SESSION_UNBOUND;
> + spin_unlock_irqrestore(>lock, flags);
> +
>   ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>  
>   /* Prevent new scans and make sure scanning is not in progress */
> @@ -4329,7 +4343,8 @@ store_priv_session_##field(struct device *dev,  
> \
>   struct iscsi_cls_session *session = \
>   iscsi_dev_to_session(dev->parent);  \
>   if ((session->state == ISCSI_SESSION_FREE) ||   \
> - (session->state == ISCSI_SESSION_FAILED))   \
> + (session->state == ISCSI_SESSION_FAILED) || \
> + (session->state == ISCSI_SESSION_UNBOUND))  \
>   return -EBUSY;  \
>   if (strncmp(buf, "off", 3) == 0) {  \
>   session->field = -1;\
> diff --git a/include/scsi/scsi_transport_iscsi.h 
> b/include/scsi/scsi_transport_iscsi.h
> index 38e4a67f5922..80149643cbcd 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -232,6 +232,7 @@ enum {
>   ISCSI_SESSION_LOGGED_IN,
>   ISCSI_SESSION_FAILED,
>   ISCSI_SESSION_FREE,
> + ISCSI_SESSION_UNBOUND,
>  };
>  
>  #define ISCSI_MAX_TARGET -1

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 

Re: [PATCH] scsi:libiscsi: remove unnecessary memset in iscsi_conn_setup

[Mike Christie]

On 3/17/22 10:01 AM, Wenchao Hao wrote:
> iscsi_cls_conn is alloced by kzalloc(), the whole iscsi_cls_conn is
> zero filled already including the dd_data. So it is unnecessary to
> call memset again.
> 
> Signed-off-by: Wenchao Hao 
> Reviewed-by: Wu Bo 
> Reviewed-by: Lee Duncan 
> ---
>  drivers/scsi/libiscsi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index d09926e6c8a8..cf4211c6500d 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -3045,7 +3045,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
> int dd_size,
>   if (!cls_conn)
>   return NULL;
>   conn = cls_conn->dd_data;
> - memset(conn, 0, sizeof(*conn) + dd_size);
>  
>   conn->dd_data = cls_conn->dd_data + sizeof(*conn);
>   conn->session = session;

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/82d04e94-86e1-fcd6-80c3-3b800f55cb7a%40oracle.com.

Re: [PATCH] scsi:libiscsi: remove unnecessary memset in iscsi_conn_setup

[Mike Christie]

On 3/16/22 4:02 AM, Wenchao Hao wrote:
> cc open-iscsi@googlegroups.com linux-s...@vger.kernel.org
> 
> On 2022/3/17 6:09, Wenchao Hao wrote:
>> iscsi_cls_conn is alloced by kzalloc(), the whole iscsi_cls_conn is
>> zero filled already including the dd_data. So it is unnecessary to
>> call memset again.
>>
>> Signed-off-by: Wenchao Hao 
>> Reviewed-by: Wu Bo 
>> ---
>>   drivers/scsi/libiscsi.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index d09926e6c8a8..cf4211c6500d 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -3045,7 +3045,6 @@ iscsi_conn_setup(struct iscsi_cls_session 
>> *cls_session, int dd_size,
>>   if (!cls_conn)
>>   return NULL;
>>   conn = cls_conn->dd_data;
>> -    memset(conn, 0, sizeof(*conn) + dd_size);
>>     conn->dd_data = cls_conn->dd_data + sizeof(*conn);
>>   conn->session = session;
>>
> 

The removal of the memset is ok, but you should resend the original
to the list because the formatting got messed up, and I think Martin
can't track this (The patch doesn't show up in patchwork/lore type of
things).

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/7ac617f1-0187-c296-bc5a-f3e26ef21488%40oracle.com.

Re: [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly

[Mike Christie]

On 3/9/22 7:57 PM, Wenchao Hao wrote:
> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.
> 
> The origin implement do device setup in iscsi_create_conn() which
> bind the alloc/init and add in one function; do device teardown in
> iscsi_destroy_conn() which bind remove and free in one function.
> 
> This implement makes it impossible to initialize resources of device
> before add it to sysfs during setup.
> 
> So this patchset splict both the setup and teradown of iscsi_cls_conn to
> 2 steps.
> 
> For setup flow, we should call iscsi_alloc_conn() and initialize some
> resources, then call iscsi_add_conn().
> 
> For teradown flow, we should call iscsi_remove_conn() to remove device
> and free resources which related to iscsi_cls_conn, then call
> iscsi_put_conn() to free iscsi_cls_conn.
> 
> V2 -> V3:
>   * Fix some bugs and optimization the code implement.
> 
> V1 -> V2:
>   * add two more iscsi_free_conn() and iscsi_remove_conn() than V1
>   * change the teardown flow of iscsi_cls_conn
> 
> Wenchao Hao (3):
>   scsi: iscsi: Add helper functions to manage iscsi_cls_conn
>   scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
>   scsi:libiscsi: teradown iscsi_cls_conn gracefully
> 
>  drivers/scsi/libiscsi.c | 23 +---
>  drivers/scsi/scsi_transport_iscsi.c | 90 +++--
>  include/scsi/scsi_transport_iscsi.h |  5 +-
>  3 files changed, 66 insertions(+), 52 deletions(-)
> 

Nice. Thanks.

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/823b30ce-c95e-4fc4-e634-f49c4433bcc9%40oracle.com.

Re: [PATCH v2 3/3] scsi:libiscsi: teardown iscsi_cls_conn gracefully

[Mike Christie]

On 3/8/22 9:09 PM, Wenchao Hao wrote:
> @@ -3143,8 +3145,6 @@ void iscsi_conn_teardown(struct iscsi_cls_conn 
> *cls_conn)
>   mutex_unlock(>eh_mutex);
>  
>   iscsi_destroy_conn(cls_conn);

This should then be a iscsi_put_conn.

So basically it balances out.

iscsi_alloc_conn requires iscsi_put_conn. The put releases what is
done in the alloc. The put on the parent is done in the release though.

iscsi_add_conn requires iscsi_remove_conn. The remove undoes what
was done in the add

We don't want a iscsi_destroy_conn which does a iscsi_free_conn
because it makes it confusing.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/e4499a17-0278-5d16-f225-82217fb4f866%40oracle.com.

Re: [PATCH v2 3/3] scsi:libiscsi: teardown iscsi_cls_conn gracefully

[Mike Christie]

On 3/8/22 9:09 PM, Wenchao Hao wrote:
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index ca724eed4f4d..7b4d998708e7 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2165,6 +2165,7 @@ static int iscsi_iter_destroy_conn_fn(struct device 
> *dev, void *data)
>  {
>   if (!iscsi_is_conn_dev(dev))
>   return 0;
> + iscsi_remove_conn(iscsi_dev_to_conn(dev));
>   return iscsi_destroy_conn(iscsi_dev_to_conn(dev));

Just do:

iscsi_remove_conn
iscsi_put_conn
return 0;



>  }
>  
> @@ -2463,7 +2464,7 @@ int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
>  
>   transport_unregister_device(>dev);
>   ISCSI_DBG_TRANS_CONN(conn, "Completing conn destruction\n");
> - device_unregister(>dev);
> + iscsi_free_conn(conn);
>   return 0;
>  }
>  EXPORT_SYMBOL_GPL(iscsi_destroy_conn);

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/bfd40073-62ae-e458-a869-b0eb60414632%40oracle.com.

Re: [PATCH v2 1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn

[Mike Christie]

On 3/8/22 9:09 PM, Wenchao Hao wrote:
> iscsi_alloc_conn(): alloc and initialize iscsi_cls_conn
> iscsi_add_conn(): expose iscsi_cls_conn to userspace's via sysfs.
> iscsi_remove_conn(): remove iscsi_cls_conn from sysfs
> iscsi_free_conn(): free iscsi_cls_conn
> 
> Signed-off-by: Wenchao Hao 
> Signed-off-by: Wu Bo 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 107 
>  include/scsi/scsi_transport_iscsi.h |   5 ++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 554b6f784223..8e97c6f88359 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2340,6 +2340,113 @@ void iscsi_free_session(struct iscsi_cls_session 
> *session)
>  }
>  EXPORT_SYMBOL_GPL(iscsi_free_session);
>  
> +/**
> + * iscsi_alloc_conn - alloc iscsi class connection
> + * @session: iscsi cls session
> + * @dd_size: private driver data size
> + * @cid: connection id
> + *
> + * This can be called from a LLD or iscsi_transport. The connection
> + * is child of the session so cid must be unique for all connections
> + * on the session.
> + *
> + * Since we do not support MCS, cid will normally be zero. In some cases
> + * for software iscsi we could be trying to preallocate a connection struct
> + * in which case there could be two connection structs and cid would be
> + * non-zero.

Is that with the upstream iscsi tools or your version? I don't think the comment
is needed or is needed somewhere else.

If this happens then they will have the same sysfs/device name so when we do the
device_add it will spit an error about duplicate names.


> + */
> +struct iscsi_cls_conn *
> +iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t 
> cid)
> +{
> + struct iscsi_transport *transport = session->transport;
> + struct iscsi_cls_conn *conn;
> +
> + conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
> + if (!conn)
> + return NULL;
> + if (dd_size)
> + conn->dd_data = [1];
> +
> + mutex_init(>ep_mutex);
> + INIT_LIST_HEAD(>conn_list);
> + INIT_WORK(>cleanup_work, iscsi_cleanup_conn_work_fn);
> + conn->transport = transport;
> + conn->cid = cid;
> + conn->state = ISCSI_CONN_DOWN;
> +
> + /* this is released in the dev's release function */
> + if (!get_device(>dev))
> + goto free_conn;
> +
> + dev_set_name(>dev, "connection%d:%u", session->sid, cid);
> + device_initialize(>dev);
> + conn->dev.parent = >dev;
> + conn->dev.release = iscsi_conn_release;
> +
> + return conn;
> +
> +free_conn:
> + kfree(conn);
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
> +
> +/**
> + * iscsi_add_conn - add iscsi class connection
> + * @conn: iscsi cls connection
> + *
> + * this would expose iscsi_cls_conn to sysfs, so make sure the related
> + * resources when access sysfs attributes are initialized before calling 
> this.
> + */
> +int iscsi_add_conn(struct iscsi_cls_conn *conn)
> +{
> + int err;
> + unsigned long flags;
> + struct iscsi_cls_session *session = 
> iscsi_dev_to_session(conn->dev.parent);
> +
> + err = device_add(>dev);
> + if (err) {
> + iscsi_cls_session_printk(KERN_ERR, session,
> +  "could not register connection's 
> dev\n");
> + put_device(>dev);

I would call iscsi_free_conn. instead of put_device.

> + return err;
> + }
> + err = transport_register_device(>dev);
> + if (err) {
> + iscsi_cls_session_printk(KERN_ERR, session,
> +  "could not register transport's 
> dev\n");
> + device_del(>dev);
> + put_device(>dev);


Is for the get_device(>dev) in iscsi_alloc_conn? If so you don't need 
to
do it because when the last put is done on the conn->dev, it will call
iscsi_conn_release which does the put on the session when it does 
"put_device(parent).

Or did you mean to call put_device on the conn->dev?

I would do device_el(>dev) then do a goto free_conn at the bottom which
does iscsi_free_conn. The place above should do the goto as well.


> + return err;
> + }
> +
> + spin_lock_irqsave(, flags);
> + list_add(>conn_list, );
> + spin_unlock_irqrestore(, flags);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_add_conn);
> +
> +/**
> + * iscsi_remove_conn - remove iscsi class connection from sysfs
> + * @conn: iscsi cls connection
> + *
> + * this would remove iscsi_cls_conn from sysfs, and wait for previous
> + * read/write of iscsi_cls_conn's attributes in sysfs finishing
> + */
> +void iscsi_remove_conn(struct iscsi_cls_conn *conn)
> +{
> + device_del(>dev);

This should have the guts of iscsi_destroy_conn which reverses what
the iscsi_add_conn did:

spin_lock_irqsave(, flags);
list_del(>conn_list);

Re: [PATCH 0/2]scsi:libiscsi: Add iscsi_cls_conn device to sysfs correctly

[Mike Christie]

On 3/7/22 6:56 PM, Wenchao Hao wrote:
> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.
> 
> iscsi_create_conn() expose iscsi_cls_conn to sysfs while the related
> resources are not initialized. So we should delay the calling of
> device_add() until these resources has been initialized.
> 
> This patchset solve this issue by changing iscsi_conn_setup() and works 
> well for iscsi_tcp.
> 

Overall I think you need to also fix up the drivers. It just makes it a
nicer driver API where the LLDs don't know about sysfs and doesn't have
to worry about it.

Let's start with just this first piece where we handle sysfs in the lib
and class like you are doing in this patchset. We can do the LLDs
interaction with the lib in a second patchset to make this easier and fix
the initial bug and cleanup some code.

In a separate patchset, we can then go deeper and maybe just merge/kill some
of the lib/class interface since every driver except qla4xxx hooks into the
lib. So we have this distinction just for that one driver's session mode
and that doesn't make a lot of sense.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/3e714766-f2cd-3bca-3611-22a87848fec7%40oracle.com.

Re: [PATCH 1/2] scsi: iscsi: Add helper functions to alloc and add iscsi_cls_conn

[Mike Christie]

On 3/7/22 6:56 PM, Wenchao Hao wrote:
> iscsi_alloc_conn() would alloc and initialize iscsi_cls_conn but do
> not expose it to userspace.
> iscsi_add_conn() would expose it to userspace.
> 
> LLDs should split the alloc and register to 2 steps.
> 
> And simplify iscsi_create_conn() with these helper functions.
> 
> Signed-off-by: Wenchao Hao 
> Signed-off-by: Wu Bo 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 85 +++--
>  include/scsi/scsi_transport_iscsi.h |  3 +
>  2 files changed, 72 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 554b6f784223..092d4429bb1d 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2341,7 +2341,7 @@ void iscsi_free_session(struct iscsi_cls_session 
> *session)
>  EXPORT_SYMBOL_GPL(iscsi_free_session);
>  
>  /**
> - * iscsi_create_conn - create iscsi class connection
> + * iscsi_alloc_conn - alloc iscsi class connection
>   * @session: iscsi cls session
>   * @dd_size: private driver data size
>   * @cid: connection id
> @@ -2356,12 +2356,10 @@ EXPORT_SYMBOL_GPL(iscsi_free_session);
>   * non-zero.
>   */
>  struct iscsi_cls_conn *
> -iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t 
> cid)
> +iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t 
> cid)
>  {
>   struct iscsi_transport *transport = session->transport;
>   struct iscsi_cls_conn *conn;
> - unsigned long flags;
> - int err;
>  
>   conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
>   if (!conn)
> @@ -2383,35 +2381,90 @@ iscsi_create_conn(struct iscsi_cls_session *session, 
> int dd_size, uint32_t cid)
>   dev_set_name(>dev, "connection%d:%u", session->sid, cid);
>   conn->dev.parent = >dev;
>   conn->dev.release = iscsi_conn_release;
> +
> + return conn;
> +
> +free_conn:
> + kfree(conn);
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
> +
> +/**
> + * iscsi_add_conn - add iscsi class connection
> + * @conn: iscsi cls connection
> + *
> + * this would expose iscsi_cls_conn to sysfs, so make sure the related
> + * resources when access sysfs attributes are initialized before calling 
> this.
> + */
> +int iscsi_add_conn(struct iscsi_cls_conn *conn)
> +{
> + int err;
> + unsigned long flags;
> + struct iscsi_cls_session *session = 
> iscsi_dev_to_session(conn->dev.parent);
> +
>   err = device_register(>dev);

You should use device_initialize in iscsi_alloc_conn. Here use device_add.

Then make a iscsi_remove_conn function which does device_del. The 
iscsi_free_conn
does a device_put.

Patch2 should not be doing kfree on the cls_conn because it has no idea what
the iscsi class needs to do to unravel things. For example, your patch leaks the
parent on failures because it doesn't do a put on the parent/session to handle
the get that's done in iscsi_alloc_conn.

In a patch3 you can then fix up iscsi_conn_teardown to remove the tmp_* kfree
code. So at the beginning of iscsi_conn_teardown do the iscsi_remove_conn.
Then after we have cleaned everything up in there do the iscsi_free_conn.

We then don't need the tmp_* code. We can just do:

   kfree(conn->persistent_address);
   kfree(conn->local_ipaddr);

anytime after the iscsi_remove_conn call.


>   if (err) {
>   iscsi_cls_session_printk(KERN_ERR, session, "could not "
>"register connection's dev\n");
> - goto release_parent_ref;
> + put_device(>dev);
> + return err;
>   }
>   err = transport_register_device(>dev);
>   if (err) {
>   iscsi_cls_session_printk(KERN_ERR, session, "could not "
>"register transport's dev\n");
> - goto release_conn_ref;
> + device_unregister(>dev);
> + put_device(>dev);
> + return err;
>   }
>  
>   spin_lock_irqsave(, flags);
>   list_add(>conn_list, );
>   spin_unlock_irqrestore(, flags);
>  
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_add_conn);
> +
> +/**
> + * iscsi_create_conn - create iscsi class connection
> + * @session: iscsi cls session
> + * @dd_size: private driver data size
> + * @cid: connection id
> + *
> + * This can be called from a LLD or iscsi_transport. The connection
> + * is child of the session so cid must be unique for all connections
> + * on the session.
> + *
> + * Since we do not support MCS, cid will normally be zero. In some cases
> + * for software iscsi we could be trying to preallocate a connection struct
> + * in which case there could be two connection structs and cid would be
> + * non-zero.
> + *
> + * Note: iscsi_cls_conn would be exposed to sysfs after this function, it
> + * means attributes of iscsi_cls_conn are accessible to userspace. So the
> + * caller must make sure everything 

Re: [PATCH 1/2] iscsi_tcp: Fix NULL pointer dereference in iscsi_sw_tcp_conn_get_param()

[Mike Christie]

On 3/3/22 8:56 PM, Wenchao Hao wrote:
> kernel might crash in iscsi_sw_tcp_conn_get_param() because it dereference
> an invalid address.
> 
> The initialization of iscsi_conn's dd_data is after device_register() of
> struct iscsi_cls_conn, so iscsi_conn's dd_data might not initialized when
> iscsi_sw_tcp_conn_get_param() is called.
> 
> Following stack would be reported and kernel would panic.
> 
> [449311.812887] Unable to handle kernel NULL pointer dereference at virtual 
> address 0008
> [449311.812893] Mem abort info:
> [449311.812895]   ESR = 0x9604
> [449311.812899]   Exception class = DABT (current EL), IL = 32 bits
> [449311.812901]   SET = 0, FnV = 0
> [449311.812903]   EA = 0, S1PTW = 0
> [449311.812905] Data abort info:
> [449311.812907]   ISV = 0, ISS = 0x0004
> [449311.812909]   CM = 0, WnR = 0
> [449311.812915] user pgtable: 4k pages, 48-bit VAs, pgdp = e26e7ace
> [449311.812918] [0008] pgd=
> [449311.812925] Internal error: Oops: 9604 [#1] SMP
> [449311.814974] Process iscsiadm (pid: 8286, stack limit = 0x800010f78000)
> [449311.815570] CPU: 0 PID: 8286 Comm: iscsiadm Kdump: loaded Tainted: GB 
>   W 4.19.90-vhulk2201.1.0.h1021.kasan.eulerosv2r10.aarch64 #1
> [449311.816584] sd 1:0:0:1: [sdg] Attached SCSI disk
> [449311.816695] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> [449311.817677] pstate: 4045 (nZcv daif +PAN -UAO)
> [449311.818121] pc : iscsi_sw_tcp_conn_get_param+0xec/0x300 [iscsi_tcp]
> [449311.818688] lr : iscsi_sw_tcp_conn_get_param+0xe8/0x300 [iscsi_tcp]
> [449311.819244] sp : 800010f7f890
> [449311.819542] x29: 800010f7f890 x28: 8000cb1bea38
> [449311.820025] x27: 800010911010 x26: 228887a4
> [449311.820500] x25: 89200d98 x24: 800010911000
> [449311.820973] x23:  x22: 8000cb1bea28
> [449311.821458] x21: 0015 x20: 200081afa000
> [449311.821934] x19: 121eff20 x18: 
> [449311.822414] x17:  x16: 200080618220
> [449311.822891] x15:  x14: 
> [449311.823413] x13:  x12: 
> [449311.823897] x11: 10001ab4f41f x10: 10001ab4f41f
> [449311.824373] x9 :  x8 : 8000d5a7a100
> [449311.824847] x7 :  x6 : dfff2000
> [449311.825329] x5 : 121eff20 x4 : 8000cb1bea30
> [449311.825806] x3 : 22911178 x2 : 2000841ff000
> [449311.826281] x1 : e0c234eab8420c00 x0 : 8000cb1bea38
> [449311.826756] Call trace:
> [449311.826987]  iscsi_sw_tcp_conn_get_param+0xec/0x300 [iscsi_tcp]
> [449311.827550]  show_conn_ep_param_ISCSI_PARAM_CONN_ADDRESS+0xe4/0x100 
> [scsi_transport_iscsi]
> [449311.828304]  dev_attr_show+0x58/0xb0
> [449311.828639]  sysfs_kf_seq_show+0x124/0x210
> [449311.829014]  kernfs_seq_show+0x8c/0xa0
> [449311.829362]  seq_read+0x188/0x8a0
> [449311.829667]  kernfs_fop_read+0x250/0x398
> [449311.830024]  __vfs_read+0xe0/0x350
> [449311.830339]  vfs_read+0xbc/0x1c0
> [449311.830635]  ksys_read+0xdc/0x1b8
> [449311.830941]  __arm64_sys_read+0x50/0x60
> [449311.831295]  el0_svc_common+0xc8/0x320
> [449311.831642]  el0_svc_handler+0xf8/0x160
> [449311.831998]  el0_svc+0x10/0x218
> [449311.832292] Code: f94006d7 910022e0 940007bb aa1c03e0 (f94006f9)
> 
> Signed-off-by: Wenchao Hao 
> Signed-off-by: Wu Bo 
> ---
>  drivers/scsi/iscsi_tcp.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 1bc37593c88f..14db224486be 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -741,11 +741,16 @@ static int iscsi_sw_tcp_conn_get_param(struct 
> iscsi_cls_conn *cls_conn,
>  {
>   struct iscsi_conn *conn = cls_conn->dd_data;
>   struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
> - struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
> + struct iscsi_sw_tcp_conn *tcp_sw_conn;
>   struct sockaddr_in6 addr;
>   struct socket *sock;
>   int rc;
>  
> + if (!tcp_conn)
> + return -ENOTCONN;
> +
> + tcp_sw_conn = tcp_conn->dd_data;
> +
>   switch(param) {
>   case ISCSI_PARAM_CONN_PORT:
>   case ISCSI_PARAM_CONN_ADDRESS:

We are actually doing sysfs/device addition wrong.

We should be doing the 2 step setup where in step 1 we alloc/init.
When everything is allocated and initialized, then we should do
device_add which exposes us to sysfs. On the teardown side, we are
then supposed to do 2 steps where the remove function does device_del
which waits until sysfs accesses are completed. We can then tear
the structs down and free them and call device_put.

The exposure to NL would be similar where it goes into the wrapper
around device_add. However, see my comments on the other patch where
I don't think we can hit the bug you mention because every nl cmd
that calls into the drivers is done under 

Re: [PATCH 2/2] iscsi_tcp: Check if tcp_conn is valid in

[Mike Christie]

On 3/3/22 8:56 PM, Wenchao Hao wrote:
> iscsi_create_conn() would add newly alloced iscsi_cls_conn to connlist,
> it means when userspace sends ISCSI_UEVENT_SET_PARAM, iscsi_conn_lookup()
> would found this iscsi_cls_conn and call the set_param callback which is
> iscsi_sw_tcp_conn_set_param(). While the iscsi_conn's dd_data might not
> been initialized.
> 
> Signed-off-by: Wenchao Hao 
> Signed-off-by: Wu Bo 
> ---
>  drivers/scsi/iscsi_tcp.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 14db224486be..a42449df6156 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -716,13 +716,17 @@ static int iscsi_sw_tcp_conn_set_param(struct 
> iscsi_cls_conn *cls_conn,
>  {
>   struct iscsi_conn *conn = cls_conn->dd_data;
>   struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
> - struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
> + struct iscsi_sw_tcp_conn *tcp_sw_conn;
>  
>   switch(param) {
>   case ISCSI_PARAM_HDRDGST_EN:
>   iscsi_set_param(cls_conn, param, buf, buflen);
>   break;
>   case ISCSI_PARAM_DATADGST_EN:
> + if (!tcp_conn || !tcp_conn->dd_data)
> + return -ENOTCONN;
> +
> + tcp_sw_conn = tcp_conn->dd_data;
>   iscsi_set_param(cls_conn, param, buf, buflen);
>   tcp_sw_conn->sendpage = conn->datadgst_en ?
>   sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;

Is this something you hit or from code review?

We have those state checks:

if ((conn->state == ISCSI_CONN_BOUND) ||
(conn->state == ISCSI_CONN_UP)) {
err = transport->set_param(conn, ev->u.set_param.param,

so we don't call set_param until after we have bound the
connection which will be after ISCSI_UEVENT_CREATE_CONN has returned.

Also for this specific bug iscsi_if_recv_msg is called with the
rx_queue_mutex, so set_param can only be called after the
ISCSI_UEVENT_CREATE_CONN cmd has returned.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/85a64450-99c8-268d-1ac7-86e70cbb3562%40oracle.com.

Re: Question about iscsi session block

[Mike Christie]

On 2/15/22 8:19 PM, michael.chris...@oracle.com wrote:
> On 2/15/22 7:28 PM, Zhengyuan Liu wrote:
>> On Wed, Feb 16, 2022 at 12:31 AM Mike Christie
>>  wrote:
>>>
>>> On 2/15/22 9:49 AM, Zhengyuan Liu wrote:
>>>> Hi, all
>>>>
>>>> We have an online server which uses multipath + iscsi to attach storage
>>>> from Storage Server. There are two NICs on the server and for each it
>>>> carries about 20 iscsi sessions and for each session it includes about 50
>>>>  iscsi devices (yes, there are totally about 2*20*50=2000 iscsi block 
>>>> devices
>>>>  on the server). The problem is: once a NIC gets faulted, it will take too 
>>>> long
>>>> (nearly 80s) for multipath to switch to another good NIC link, because it
>>>> needs to block all iscsi devices over that faulted NIC firstly. The 
>>>> callstack is
>>>>  shown below:
>>>>
>>>> void iscsi_block_session(struct iscsi_cls_session *session)
>>>> {
>>>> queue_work(iscsi_eh_timer_workq, >block_work);
>>>> }
>>>>
>>>>  __iscsi_block_session() -> scsi_target_block() -> target_block() ->
>>>>   device_block() ->  scsi_internal_device_block() -> scsi_stop_queue() ->
>>>>  blk_mq_quiesce_queue()>synchronize_rcu()
>>>>
>>>> For all sessions and all devices, it was processed sequentially, and we 
>>>> have
>>>> traced that for each synchronize_rcu() call it takes about 80ms, so
>>>> the total cost
>>>> is about 80s (80ms * 20 * 50). It's so long that the application can't
>>>> tolerate and
>>>> may interrupt service.
>>>>
>>>> So my question is that can we optimize the procedure to reduce the time 
>>>> cost on
>>>> blocking all iscsi devices?  I'm not sure if it is a good idea to increase 
>>>> the
>>>> workqueue's max_active of iscsi_eh_timer_workq to improve concurrency.
>>>
>>> We need a patch, so the unblock call waits/cancels/flushes the block call or
>>> they could be running in parallel.
>>>
>>> I'll send a patchset later today so you can test it.
>>
>> I'm glad to test once you push the patchset.
>>
>> Thank you, Mike.
> 
> I forgot I did this recently :)
> 
> commit 7ce9fc5ecde0d8bd64c29baee6c5e3ce7074ec9a
> Author: Mike Christie 
> Date:   Tue May 25 13:18:09 2021 -0500
> 
> scsi: iscsi: Flush block work before unblock
> 
> We set the max_active iSCSI EH works to 1, so all work is going to execute
> in order by default. However, userspace can now override this in sysfs. If
> max_active > 1, we can end up with the block_work on CPU1 and
> iscsi_unblock_session running the unblock_work on CPU2 and the session and
> target/device state will end up out of sync with each other.
> 
> This adds a flush of the block_work in iscsi_unblock_session.
> 
> 
> It was merged in 5.14.

Hey, I found one more bug when max_active > 1. While fixing it I decided to just
fix this so we can do the sessions recoveries in parallel and the user doesn't 
have
to worry about setting max_active.

I'll send a patchset and cc you.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ec8d0b97-e94a-21e1-acdb-e90a7df39b72%40oracle.com.

Re: Question about iscsi session block

[Mike Christie]

On 2/15/22 9:49 AM, Zhengyuan Liu wrote:
> Hi, all
> 
> We have an online server which uses multipath + iscsi to attach storage
> from Storage Server. There are two NICs on the server and for each it
> carries about 20 iscsi sessions and for each session it includes about 50
>  iscsi devices (yes, there are totally about 2*20*50=2000 iscsi block devices
>  on the server). The problem is: once a NIC gets faulted, it will take too 
> long
> (nearly 80s) for multipath to switch to another good NIC link, because it
> needs to block all iscsi devices over that faulted NIC firstly. The callstack 
> is
>  shown below:
> 
> void iscsi_block_session(struct iscsi_cls_session *session)
> {
> queue_work(iscsi_eh_timer_workq, >block_work);
> }
> 
>  __iscsi_block_session() -> scsi_target_block() -> target_block() ->
>   device_block() ->  scsi_internal_device_block() -> scsi_stop_queue() ->
>  blk_mq_quiesce_queue()>synchronize_rcu()
> 
> For all sessions and all devices, it was processed sequentially, and we have
> traced that for each synchronize_rcu() call it takes about 80ms, so
> the total cost
> is about 80s (80ms * 20 * 50). It's so long that the application can't
> tolerate and
> may interrupt service.
> 
> So my question is that can we optimize the procedure to reduce the time cost 
> on
> blocking all iscsi devices?  I'm not sure if it is a good idea to increase the
> workqueue's max_active of iscsi_eh_timer_workq to improve concurrency.

We need a patch, so the unblock call waits/cancels/flushes the block call or
they could be running in parallel.

I'll send a patchset later today so you can test it.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/828ac69a-fe28-0869-bc1f-7fd106dff0aa%40oracle.com.

Re: Antw: [EXT] Re: [PATCH] scsi scsi_transport_iscsi.c: fix misuse of %llu in scsi_transport_iscsi.c

[Mike Christie]

On 10/11/21 1:35 AM, Ulrich Windl wrote:
 Joe Perches  schrieb am 09.10.2021 um 05:14 in Nachricht
> <5daf69b365e23ceecee911c4d0f2f66a0b9ec95c.ca...@perches.com>:
>> On Sat, 2021-10-09 at 11:02 +0800, Guo Zhi wrote:
>>> Pointers should be printed with %p or %px rather than
>>> cast to (unsigned long long) and printed with %llu.
>>> Change %llu to %p to print the pointer into sysfs.
>> ][]
>>> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
>> b/drivers/scsi/scsi_transport_iscsi.c
>> []
>>> @@ -129,8 +129,8 @@ show_transport_handle(struct device *dev, struct 
>> device_attribute *attr,
>>>  
>>>
>>> if (!capable(CAP_SYS_ADMIN))
>>> return -EACCES;
>>> -   return sysfs_emit(buf, "%llu\n",
>>> - (unsigned long long)iscsi_handle(priv->iscsi_transport));
>>> +   return sysfs_emit(buf, "%p\n",
>>> +   iscsi_ptr(priv->iscsi_transport));
>>
>> iscsi_transport is a pointer isn't it?
>>
>> so why not just
>>
>>  return sysfs_emit(buf, "%p\n", priv->iscsi_transport);
> 
> Isn't the difference that %p outputs hex, while %u outputs decimal?
> 

Yeah, I think this patch will break userspace, because it doesn't know it's
a pointer. It could be doing:

sscanf(str, "%llu", );

The value is just later passed back to the kernel to look up a driver in
iscsi_if_transport_lookup:

list_for_each_entry(priv, _transports, list) {
if (tt == priv->iscsi_transport) {

so we could just replace priv->transport with an int and use an ida to assign
the value.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/ae7a82c2-5b19-493a-8d61-cdccb00cf46c%40oracle.com.

Re: [PATCH v2] scsi: libiscsi: move init ehwait to iscsi_session_setup()

[Mike Christie]

On 9/11/21 8:51 AM, Ding Hui wrote:
> The commit ec29d0ac29be ("scsi: iscsi: Fix conn use after free during
> resets") move member ehwait from conn to session, but left init ehwait
> in iscsi_conn_setup().
> 
> Although a session can only have 1 conn currently, it is better to
> do init ehwait in iscsi_session_setup() to prevent reinit by mistake,
> also in case we can handle multiple conns in the future.
> 
> Fixes: ec29d0ac29be ("scsi: iscsi: Fix conn use after free during resets")
> Signed-off-by: Ding Hui 
> ---
> v2:
>   update commit log
>  
>  drivers/scsi/libiscsi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 4683c183e9d4..712a45368385 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2947,6 +2947,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit, 
> struct Scsi_Host *shost,
>   session->tmf_state = TMF_INITIAL;
>   timer_setup(>tmf_timer, iscsi_tmf_timedout, 0);
>   mutex_init(>eh_mutex);
> + init_waitqueue_head(>ehwait);
>  
>   spin_lock_init(>frwd_lock);
>   spin_lock_init(>back_lock);
> @@ -3074,8 +3075,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
> int dd_size,
>   goto login_task_data_alloc_fail;
>   conn->login_task->data = conn->data = data;
>  
> - init_waitqueue_head(>ehwait);
> -
>   return cls_conn;
>  
>  login_task_data_alloc_fail:
> 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/3c40a1af-5f13-0e93-67ad-f1ccd4f1480c%40oracle.com.

Re: [PATCH 2/3] scsi: libiscsi: fix invalid pointer dereference in iscsi_eh_session_reset

[Mike Christie]

On 9/9/21 8:02 PM, Ding Hui wrote:
> like commit 5db6dd14b313 ("scsi: libiscsi: Fix NULL pointer dereference in
> iscsi_eh_session_reset"), access conn->persistent_address here is not safe
> too.
> 
> The persistent_address is independent of conn refcount, so maybe
> already freed by iscsi_conn_teardown(), also we put the refcount of conn
> above, the conn pointer may be invalid.

This shouldn't happen like you describe above, because when we wake up
we will see the session->state is ISCSI_STATE_TERMINATE. We will then
not reference the conn in that code below.

However, it looks like we could hit an issue where if a user was resetting
the persistent_address or targetname via iscsi_set_param -> 
iscsi_switch_str_param
then we could be accessing freed memory. I think we need the frwd_lock when 
swapping
the strings in iscsi_switch_str_param.


> 
> Signed-off-by: Ding Hui 
> ---
>  drivers/scsi/libiscsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 712a45368385..69b3b2148328 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2531,8 +2531,8 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
>   spin_lock_bh(>frwd_lock);
>   if (session->state == ISCSI_STATE_LOGGED_IN) {
>   ISCSI_DBG_EH(session,
> -  "session reset succeeded for %s,%s\n",
> -  session->targetname, conn->persistent_address);
> +  "session reset succeeded for %s\n",
> +  session->targetname);
>   } else
>   goto failed;
>   spin_unlock_bh(>frwd_lock);
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/302af74d-5b72-5b2f-050a-33f0978e321f%40oracle.com.

Re: [PATCH 1/3] scsi: libiscsi: move init ehwait to iscsi_session_setup()

[Mike Christie]

On 9/9/21 8:02 PM, Ding Hui wrote:
> commit ec29d0ac29be ("scsi: iscsi: Fix conn use after free during
> resets") move member ehwait from conn to session, but left init ehwait
> in iscsi_conn_setup().
> 
> Due to one session can be binded by multi conns, the conn after the

A session can only have 1 conn. There is some code that makes it look
like we can do multiple conns or swap the single conn, but it was never
fully implemented/supported upstream.

However, I like the patch. The init should be done in iscsi_session_setup,
so could you fix up the commit, so it's correct?

> first will reinit the session->ehwait, move init ehwait to
> iscsi_session_setup() to fix it.
> 
> Fixes: ec29d0ac29be ("scsi: iscsi: Fix conn use after free during resets")
> Signed-off-by: Ding Hui 
> ---
>  drivers/scsi/libiscsi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 4683c183e9d4..712a45368385 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2947,6 +2947,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit, 
> struct Scsi_Host *shost,
>   session->tmf_state = TMF_INITIAL;
>   timer_setup(>tmf_timer, iscsi_tmf_timedout, 0);
>   mutex_init(>eh_mutex);
> + init_waitqueue_head(>ehwait);
>  
>   spin_lock_init(>frwd_lock);
>   spin_lock_init(>back_lock);
> @@ -3074,8 +3075,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
> int dd_size,
>   goto login_task_data_alloc_fail;
>   conn->login_task->data = conn->data = data;
>  
> - init_waitqueue_head(>ehwait);
> -
>   return cls_conn;
>  
>  login_task_data_alloc_fail:
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/03817f8e-8fed-6e7a-e76f-8608f8cfd979%40oracle.com.

Re: [PATCH 2/2] scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param()

[Mike Christie]

On 4/6/21 8:24 PM, Wenchao Hao wrote:
> iscsi_sw_tcp_host_get_param() would access struct iscsi_session, while
> struct iscsi_session might be freed by session destroy flow in
> iscsi_free_session(). This commit fix this condition by freeing session
> after host has already been removed.
> 
> Signed-off-by: Wenchao Hao 
> ---
>  drivers/scsi/iscsi_tcp.c | 27 ++-
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index dd33ce0e3737..d559abd3694c 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -839,6 +839,18 @@ iscsi_sw_tcp_conn_get_stats(struct iscsi_cls_conn 
> *cls_conn,
>   iscsi_tcp_conn_get_stats(cls_conn, stats);
>  }
>  
> +static void
> +iscsi_sw_tcp_session_teardown(struct iscsi_cls_session *cls_session)
> +{
> + struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
> +
> + iscsi_session_destroy(cls_session);
> + iscsi_host_remove(shost);
> +
> + iscsi_free_session(cls_session);
> + iscsi_host_free(shost);
> +}

Can you add a comment about the iscsi_tcp dependency with the host
and session or maybe convert ib_iser too?

ib_iser does the same session per host scheme and so if you were
just scanning the code and going to make a API change, it's not
really clear why the drivers do it differently.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/719f91cb-769d-53c8-1b54-cb9ad38aba01%40oracle.com.

Re: [PATCH AUTOSEL 5.10 07/22] scsi: iscsi: Fix race condition between login and sync thread

[Mike Christie]

On 4/5/21 11:04 AM, Sasha Levin wrote:
> From: Gulam Mohamed 
> 
> [ Upstream commit 9e67600ed6b8565da4b85698ec659b5879a6c1c6 ]
> 
> A kernel panic was observed due to a timing issue between the sync thread
> and the initiator processing a login response from the target. The session
> reopen can be invoked both from the session sync thread when iscsid
> restarts and from iscsid through the error handler. Before the initiator
> receives the response to a login, another reopen request can be sent from
> the error handler/sync session. When the initial login response is
> subsequently processed, the connection has been closed and the socket has
> been released.
> 
> To fix this a new connection state, ISCSI_CONN_BOUND, is added:
> 
>  - Set the connection state value to ISCSI_CONN_DOWN upon
>iscsi_if_ep_disconnect() and iscsi_if_stop_conn()
> 
>  - Set the connection state to the newly created value ISCSI_CONN_BOUND
>after bind connection (transport->bind_conn())
> 
>  - In iscsi_set_param(), return -ENOTCONN if the connection state is not
>either ISCSI_CONN_BOUND or ISCSI_CONN_UP
> 
> Link: 
> https://urldefense.com/v3/__https://lore.kernel.org/r/20210325093248.284678-1-gulam.mohamed@oracle.com__;!!GqivPVa7Brio!Jiqrc6pu3EgrquzpG-KpNQkNebwKUgctkE0MN1MloQ2y5Y4OVOkKN0yCr2_W_CX2oRet$
>  
> Reviewed-by: Mike Christie 


There was a mistake in my review of this patch. It will also require
this "[PATCH 1/1] scsi: iscsi: fix iscsi cls conn state":

https://lore.kernel.org/linux-scsi/20210406171746.5016-1-michael.chris...@oracle.com/T/#u


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/be0783c0-4ca8-d7fd-ce97-c24c2f1d8e84%40oracle.com.

Re: [PATCH] iscsi: Do Not set param when sock is NULL

[Mike Christie]

On 1/7/21 9:48 AM, Gulam Mohamed wrote:
> Hi Michael,
> 
>  As per your suggestions in below mail, I have completed the 
> suggested changes and tested them. Can you please review and let me know your 
> comments? Here is the patch:
> 
> Description
> ===
> 1. This Kernel panic could be due to a timing issue when there is a race 
> between the sync thread and the initiator was processing of a login response 
> from the target. The session re-open can be invoked from two places
>   a. Sessions sync thread when the iscsid restart
>   b. From iscsid through iscsi error handler 2. The session reopen sequence 
> is as follows in user-space (iscsi-initiator-utils)
>a. Disconnect the connection
>b. Then send the stop connection request to the kernel which releases the 
> connection (releases the socket)
>c. Queues the reopen for 2 seconds delay
>d. Once the delay expires, create the TCP connection again by calling the 
> connect() call
>e. Poll for the connection
>f. When poll is successful i.e when the TCP connection is established, it 
> performs
>   i. Creation of session and connection data structures
>   ii. Bind the connection to the session. This is the place where we 
> assign the sock to tcp_sw_conn->sock
>   iii. Sets the parameters like target name, persistent address etc .
>   iv. Creates the login pdu
>v. Sends the login pdu to kernel
>   vi. Returns to the main loop to process further events. The kernel then 
> sends the login request over to the target node
>g. Once login response with success is received, it enters into full 
> feature phase and sets the negotiable parameters like max_recv_data_length, 
> max_transmit_length, data_digest etc .
> 3. While setting the negotiable parameters by calling 
> "iscsi_session_set_neg_params()", kernel panicked as sock was NULL
> 
> What happened here is
> -
> 1. Before initiator received the login response mentioned in above point 
> 2.f.v, another reopen request was sent from the error handler/sync session 
> for the same session, as the initiator utils was in main loop to process 
> further events (as mentioned in point 2.f.vi above).
> 2. While processing this reopen, it stopped the connection which released the 
> socket and queued this connection and at this point of time the login 
> response was received for the earlier on
> 
> Fix
> ---
> 
> 1. Create a flag "set_param_fail" in iscsi_cls_conn structure 2. On 
> ep_disconnect and stop_conn set this flag to indicate set_param calls for 
> connection level settings should fail 3. This way, scsi_transport_iscsi can 
> set and check this bit for all drivers 2. On bind_conn clear the bit
> 
> Signed-off-by: Gulam Mohamed 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 6 ++  
> include/scsi/scsi_transport_iscsi.h | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 2e68c0a87698..15c5a7223a40 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2473,6 +2473,8 @@ static void iscsi_if_stop_conn(struct iscsi_cls_conn 
> *conn, int flag)
>* it works.
>*/
>   mutex_lock(_mutex);
> + if (!test_bit(ISCSI_SET_PARAM_FAIL_BIT, >set_param_fail))
> + set_bit(ISCSI_SET_PARAM_FAIL_BIT, >set_param_fail);

You can just do a test_and_set_bit.

>   conn->transport->stop_conn(conn, flag);
>   mutex_unlock(_mutex);
>  
> @@ -2895,6 +2897,8 @@ iscsi_set_param(struct iscsi_transport *transport, 
> struct iscsi_uevent *ev)
>   session->recovery_tmo = value;
>   break;
>   default:
> + if (test_bit(ISCSI_SET_PARAM_FAIL_BIT, >set_param_fail))
> + return -ENOTCONN;
>   err = transport->set_param(conn, ev->u.set_param.param,
>  data, ev->u.set_param.len);
>   }
> @@ -2956,6 +2960,7 @@ static int iscsi_if_ep_disconnect(struct 
> iscsi_transport *transport,
>   mutex_lock(>ep_mutex);
>   conn->ep = NULL;
>   mutex_unlock(>ep_mutex);
> + set_bit(ISCSI_SET_PARAM_FAIL_BIT, >set_param_fail);
>   }
>  
>   transport->ep_disconnect(ep);
> @@ -3716,6 +3721,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr 
> *nlh, uint32_t *group)
>   ev->r.retcode = transport->bind_conn(session, conn,
>   ev->u.b_conn.transport_eph,
>   ev->u.b_conn.is_leading);
> + clear_bit(ISCSI_SET_PARAM_FAIL_BIT, >set_param_fail);

You should check retcode and only clear if it indicates success.


>   mutex_unlock(_mutex);
>  
>   if (ev->r.retcode || !transport->ep_connect) diff --git 
> a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> 

Re: [RFC PATCH] scsi:libiscsi:Fix possible NULL dereference in iscsi_eh_cmd_timed_out

[Mike Christie]

On 12/14/20 2:41 AM, Wu Bo wrote:
> When testing kernel 4.18 version, NULL pointer dereference problem occurs
> in iscsi_eh_cmd_timed_out function.
> 
> I think this bug in the upstream is still exists.
> 
> The analysis reasons are as follows:
> 1)  For some reason, I/O command did not complete within 
> the timeout period. The block layer timer works, 
> call scsi_times_out() to handle I/O timeout logic. 
> At the same time the command just completes.
> 
> 2)  scsi_times_out() call iscsi_eh_cmd_timed_out() 
> to processing timeout logic.  although there is an NULL judgment 
>   for the task, the task has not been released yet now.
> 
> 3)  iscsi_complete_task() call __iscsi_put_task(), 
> The task reference count reaches zero, the conditions for free task 
> is met, then iscsi_free_task () free the task, 
> and let sc->SCp.ptr = NULL. After iscsi_eh_cmd_timed_out passes 
> the task judgment check, there may be NULL dereference scenarios
> later.
>   

I have a patch for this I think. This is broken out of patchset I was
trying to fixup the back lock usage for offload drivers, so I have only
compile tested it.

There is another issue where the for lun reset cleanup we could race. The
comments mention suspending the rx side, but we only do that for session level
cleaup.

The basic idea is we don't want to add more frwd lock uses in the completion
patch like in your patch. In these non perf paths, like the tmf/timeout case
we can just take a ref to the cmd so it's not freed from under us.



diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index f9314f1..f07f8c1 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -573,18 +573,9 @@ void iscsi_complete_scsi_task(struct iscsi_task *task,
 static void fail_scsi_task(struct iscsi_task *task, int err)
 {
struct iscsi_conn *conn = task->conn;
-   struct scsi_cmnd *sc;
+   struct scsi_cmnd *sc = task->sc;
int state;
 
-   /*
-* if a command completes and we get a successful tmf response
-* we will hit this because the scsi eh abort code does not take
-* a ref to the task.
-*/
-   sc = task->sc;
-   if (!sc)
-   return;
-
if (task->state == ISCSI_TASK_PENDING) {
/*
 * cmd never made it to the xmit thread, so we should not count
@@ -1855,26 +1846,34 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn 
*conn,
 }
 
 /*
- * Fail commands. session lock held and recv side suspended and xmit
- * thread flushed
+ * Fail commands. session frwd lock held and and xmit thread flushed.
  */
 static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
 {
+   struct iscsi_session *session = conn->session;
struct iscsi_task *task;
int i;
 
-   for (i = 0; i < conn->session->cmds_max; i++) {
-   task = conn->session->cmds[i];
-   if (!task->sc || task->state == ISCSI_TASK_FREE)
+   for (i = 0; i < session->cmds_max; i++) {
+   spin_lock_bh(>back_lock);
+   task = session->cmds[i];
+   if (!task->sc || task->state == ISCSI_TASK_FREE) {
+   spin_unlock_bh(>back_lock);
continue;
+   }
 
-   if (lun != -1 && lun != task->sc->device->lun)
+   if (lun != -1 && lun != task->sc->device->lun) {
+   spin_unlock_bh(>back_lock);
continue;
+   }
+   __iscsi_get_task(task);
+   spin_unlock_bh(>back_lock);
 
-   ISCSI_DBG_SESSION(conn->session,
+   ISCSI_DBG_SESSION(session,
  "failing sc %p itt 0x%x state %d\n",
  task->sc, task->itt, task->state);
fail_scsi_task(task, error);
+   iscsi_put_task(task);
}
 }
 
@@ -1953,6 +1952,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct 
scsi_cmnd *sc)
ISCSI_DBG_EH(session, "scsi cmd %p timedout\n", sc);
 
spin_lock_bh(>frwd_lock);
+   spin_lock(>back_lock);
task = (struct iscsi_task *)sc->SCp.ptr;
if (!task) {
/*
@@ -1960,8 +1960,11 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct 
scsi_cmnd *sc)
 * so let timeout code complete it now.
 */
rc = BLK_EH_DONE;
+   spin_unlock(>back_lock);
goto done;
}
+   __iscsi_get_task(task);
+   spin_unlock(>back_lock);
 
if (session->state != ISCSI_STATE_LOGGED_IN) {
/*
@@ -2077,9 +2080,12 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct 
scsi_cmnd *sc)
rc = BLK_EH_RESET_TIMER;
 
 done:
-   if (task)
-   task->last_timeout = jiffies;
spin_unlock_bh(>frwd_lock);
+
+   if (task) {
+   task->last_timeout = 

Re: [PATCH] scsi: iscsi: fix inappropriate use of put_device

[Mike Christie]

On 11/20/20 1:48 AM, Qinglang Miao wrote:
> kfree(conn) is called inside put_device(>dev) so that
> another one would cause use-after-free. Besides, device_unregister
> should be used here rather than put_device.
> 
> Fixes: f3c893e3dbb5 ("scsi: iscsi: Fail session and connection on transport 
> registration failure")
> Reported-by: Hulk Robot 
> Signed-off-by: Qinglang Miao 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 2eb3e4f93..2e68c0a87 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2313,7 +2313,9 @@ iscsi_create_conn(struct iscsi_cls_session *session, 
> int dd_size, uint32_t cid)
>   return conn;
>  
>  release_conn_ref:
> - put_device(>dev);
> + device_unregister(>dev);
> + put_device(>dev);
> +     return NULL;
>  release_parent_ref:
>   put_device(>dev);
>  free_conn:
> 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/331a17b8-3219-cb73-90c8-4b814202a78f%40oracle.com.

Re: [PATCH v2] SCSI: libiscsi: fix NOP race condition

[Mike Christie]

On 11/6/20 1:33 PM, Lee Duncan wrote:

From: Lee Duncan 

iSCSI NOPs are sometimes "lost", mistakenly sent to the
user-land iscsid daemon instead of handled in the kernel,
as they should be, resulting in a message from the daemon like:


iscsid: Got nop in, but kernel supports nop handling.


This can occur because of the new forward- and back-locks,
and the fact that an iSCSI NOP response can occur before
processing of the NOP send is complete. This can result
in "conn->ping_task" being NULL in iscsi_nop_out_rsp(),
when the pointer is actually in the process of being set.

To work around this, we add a new state to the "ping_task"
pointer. In addition to NULL (not assigned) and a pointer
(assigned), we add the state "being set", which is signaled
with an INVALID pointer (using "-1").

Changes since V1:
  - expanded using READ_ONCE()/WRITE_ONCE() to the whole path

Signed-off-by: Lee Duncan 
---
  drivers/scsi/libiscsi.c | 23 +++
  include/scsi/libiscsi.h |  3 +++
  2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 1e9c3171fa9f..f9314f1393fb 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -533,8 +533,8 @@ static void iscsi_complete_task(struct iscsi_task *task, 
int state)
if (conn->task == task)
conn->task = NULL;
  
-	if (conn->ping_task == task)

-   conn->ping_task = NULL;
+   if (READ_ONCE(conn->ping_task) == task)
+   WRITE_ONCE(conn->ping_task, NULL);
  
  	/* release get from queueing */

__iscsi_put_task(task);
@@ -738,6 +738,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct 
iscsi_hdr *hdr,
   task->conn->session->age);
}
  
+	if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))

+   WRITE_ONCE(conn->ping_task, task);
+
if (!ihost->workq) {
if (iscsi_prep_mgmt_task(conn, task))
goto free_task;
@@ -941,8 +944,11 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
struct iscsi_nopin *rhdr)
  struct iscsi_nopout hdr;
struct iscsi_task *task;
  
-	if (!rhdr && conn->ping_task)

-   return -EINVAL;
+   if (!rhdr) {
+   if (READ_ONCE(conn->ping_task))
+   return -EINVAL;
+   WRITE_ONCE(conn->ping_task, INVALID_SCSI_TASK);
+   }
  
  	memset(, 0, sizeof(struct iscsi_nopout));

hdr.opcode = ISCSI_OP_NOOP_OUT | ISCSI_OP_IMMEDIATE;
@@ -957,11 +963,12 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
struct iscsi_nopin *rhdr)
  
  	task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *), NULL, 0);

if (!task) {
+   if (!rhdr)
+   WRITE_ONCE(conn->ping_task, NULL);
iscsi_conn_printk(KERN_ERR, conn, "Could not send nopout\n");
return -EIO;
} else if (!rhdr) {
/* only track our nops */
-   conn->ping_task = task;
conn->last_ping = jiffies;
}
  
@@ -984,7 +991,7 @@ static int iscsi_nop_out_rsp(struct iscsi_task *task,

struct iscsi_conn *conn = task->conn;
int rc = 0;
  
-	if (conn->ping_task != task) {

+   if (READ_ONCE(conn->ping_task) != task) {
/*
 * If this is not in response to one of our
 * nops then it must be from userspace.
@@ -1923,7 +1930,7 @@ static void iscsi_start_tx(struct iscsi_conn *conn)
   */
  static int iscsi_has_ping_timed_out(struct iscsi_conn *conn)
  {
-   if (conn->ping_task &&
+   if (READ_ONCE(conn->ping_task) &&
time_before_eq(conn->last_recv + (conn->recv_timeout * HZ) +
   (conn->ping_timeout * HZ), jiffies))
return 1;
@@ -2058,7 +2065,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct 
scsi_cmnd *sc)
 * Checking the transport already or nop from a cmd timeout still
 * running
 */
-   if (conn->ping_task) {
+   if (READ_ONCE(conn->ping_task)) {
task->have_checked_conn = true;
rc = BLK_EH_RESET_TIMER;
goto done;
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index c25fb86ffae9..b3bbd10eb3f0 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -132,6 +132,9 @@ struct iscsi_task {
void*dd_data;   /* driver/transport data */
  };
  
+/* invalid scsi_task pointer */

+#defineINVALID_SCSI_TASK   (struct iscsi_task *)-1l
+
  static inline int iscsi_task_has_unsol_data(struct iscsi_task *task)
  {
return task->unsol_r2t.data_length > task->unsol_r2t.sent;



Reviewed-by: M

Re: [PATCH] scsi: libiscsi: fix NOP race condition

[Mike Christie]

On 11/4/20 3:33 PM, Mike Christie wrote:

On 9/18/20 4:09 PM, Lee Duncan wrote:

From: Lee Duncan 

iSCSI NOPs are sometimes "lost", mistakenly sent to the
user-land iscsid daemon instead of handled in the kernel,
as they should be, resulting in a message from the daemon like:


iscsid: Got nop in, but kernel supports nop handling.


This can occur because of the new forward- and back-locks,
and the fact that an iSCSI NOP response can occur before
processing of the NOP send is complete. This can result
in "conn->ping_task" being NULL in iscsi_nop_out_rsp(),
when the pointer is actually in the process of being set.

To work around this, we add a new state to the "ping_task"
pointer. In addition to NULL (not assigned) and a pointer
(assigned), we add the state "being set", which is signaled
with an INVALID pointer (using "-1").

Signed-off-by: Lee Duncan 
---
  drivers/scsi/libiscsi.c | 11 ++-
  include/scsi/libiscsi.h |  3 +++
  2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 1e9c3171fa9f..5eb064787ee2 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -738,6 +738,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, 
struct iscsi_hdr *hdr,

 task->conn->session->age);
  }
+    if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))
+    WRITE_ONCE(conn->ping_task, task);
+
  if (!ihost->workq) {
  if (iscsi_prep_mgmt_task(conn, task))
  goto free_task;
@@ -941,6 +944,11 @@ static int iscsi_send_nopout(struct iscsi_conn 
*conn, struct iscsi_nopin *rhdr)

  struct iscsi_nopout hdr;
  struct iscsi_task *task;
+    if (!rhdr) {
+    if (READ_ONCE(conn->ping_task))
+    return -EINVAL;
+    WRITE_ONCE(conn->ping_task, INVALID_SCSI_TASK);
+    }
  if (!rhdr && conn->ping_task)
  return -EINVAL;
@@ -957,11 +965,12 @@ static int iscsi_send_nopout(struct iscsi_conn 
*conn, struct iscsi_nopin *rhdr)
  task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *), 
NULL, 0);

  if (!task) {
+    if (!rhdr)
+    WRITE_ONCE(conn->ping_task, NULL);


I don't think you need this. If __iscsi_conn_send_pdu returns NULL, it 
will have done __iscsi_put_task and done this already.


Ignore that. That is iscsi_complete_task that would do it.




  iscsi_conn_printk(KERN_ERR, conn, "Could not send nopout\n");
  return -EIO;
  } else if (!rhdr) {
  /* only track our nops */
-    conn->ping_task = task;
  conn->last_ping = jiffies;
  }


Why in the send path do we always use the READ_ONCE/WRITE_ONCE, but in 
the completion path like in iscsi_complete_task we don't.


--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/e8bc1385-7e8d-40e5-76ab-0086fcd1ea12%40oracle.com.

Re: [PATCH] scsi: libiscsi: fix NOP race condition

[Mike Christie]

On 9/18/20 4:09 PM, Lee Duncan wrote:

From: Lee Duncan 

iSCSI NOPs are sometimes "lost", mistakenly sent to the
user-land iscsid daemon instead of handled in the kernel,
as they should be, resulting in a message from the daemon like:


iscsid: Got nop in, but kernel supports nop handling.


This can occur because of the new forward- and back-locks,
and the fact that an iSCSI NOP response can occur before
processing of the NOP send is complete. This can result
in "conn->ping_task" being NULL in iscsi_nop_out_rsp(),
when the pointer is actually in the process of being set.

To work around this, we add a new state to the "ping_task"
pointer. In addition to NULL (not assigned) and a pointer
(assigned), we add the state "being set", which is signaled
with an INVALID pointer (using "-1").

Signed-off-by: Lee Duncan 
---
  drivers/scsi/libiscsi.c | 11 ++-
  include/scsi/libiscsi.h |  3 +++
  2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 1e9c3171fa9f..5eb064787ee2 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -738,6 +738,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct 
iscsi_hdr *hdr,
   task->conn->session->age);
}
  
+	if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))

+   WRITE_ONCE(conn->ping_task, task);
+
if (!ihost->workq) {
if (iscsi_prep_mgmt_task(conn, task))
goto free_task;
@@ -941,6 +944,11 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
struct iscsi_nopin *rhdr)
  struct iscsi_nopout hdr;
struct iscsi_task *task;
  
+	if (!rhdr) {

+   if (READ_ONCE(conn->ping_task))
+   return -EINVAL;
+   WRITE_ONCE(conn->ping_task, INVALID_SCSI_TASK);
+   }
if (!rhdr && conn->ping_task)
return -EINVAL;
  
@@ -957,11 +965,12 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr)
  
  	task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *), NULL, 0);

if (!task) {
+   if (!rhdr)
+   WRITE_ONCE(conn->ping_task, NULL);


I don't think you need this. If __iscsi_conn_send_pdu returns NULL, it 
will have done __iscsi_put_task and done this already.



iscsi_conn_printk(KERN_ERR, conn, "Could not send nopout\n");
return -EIO;
} else if (!rhdr) {
/* only track our nops */
-   conn->ping_task = task;
conn->last_ping = jiffies;
}


Why in the send path do we always use the READ_ONCE/WRITE_ONCE, but in 
the completion path like in iscsi_complete_task we don't.


--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/f0c9b3ff-7d93-9c2b-d405-e52fb4aa8c37%40oracle.com.

Re: [PATCH] scsi: libiscsi: Fix cmds hung when sd_shutdown

[Mike Christie]

On 10/31/20 3:23 AM, Wu Bo wrote:

For some reason, during reboot the system, iscsi.service failed to
logout all sessions. kernel will hang forever on its
sd_sync_cache() logic, after issuing the SYNCHRONIZE_CACHE cmd to all
still existent paths.

[ 1044.098991] reboot: Mddev shutdown finished.
[ 1044.099311] reboot: Usermodehelper disable finished.
[ 1050.611244]  connection2:0: ping timeout of 5 secs expired, recv timeout 5, 
last rx 4295152378, last ping 4295153633, now 4295154944
[ 1348.599676] Call trace:
[ 1348.599887]  __switch_to+0xe8/0x150
[ 1348.600113]  __schedule+0x33c/0xa08
[ 1348.600372]  schedule+0x2c/0x88
[ 1348.600567]  schedule_timeout+0x184/0x3a8
[ 1348.600820]  io_schedule_timeout+0x28/0x48
[ 1348.601089]  wait_for_common_io.constprop.2+0x168/0x258
[ 1348.601425]  wait_for_completion_io_timeout+0x28/0x38
[ 1348.601762]  blk_execute_rq+0x98/0xd8
[ 1348.602006]  __scsi_execute+0xe0/0x1e8
[ 1348.602262]  sd_sync_cache+0xd0/0x220 [sd_mod]
[ 1348.602551]  sd_shutdown+0x6c/0xf8 [sd_mod]
[ 1348.602826]  device_shutdown+0x13c/0x250
[ 1348.603078]  kernel_restart_prepare+0x5c/0x68
[ 1348.603400]  kernel_restart+0x20/0x98
[ 1348.603683]  __se_sys_reboot+0x214/0x260
[ 1348.603987]  __arm64_sys_reboot+0x24/0x30
[ 1348.604300]  el0_svc_common+0x80/0x1b8
[ 1348.604590]  el0_svc_handler+0x78/0xe0
[ 1348.604877]  el0_svc+0x10/0x260

d754941225 (scsi: libiscsi: Allow sd_shutdown on bad transport) Once
solved this problem. The iscsi_eh_cmd_timed_out() function add system_state
judgment, and will return BLK_EH_DONE and mark the result as
DID_NO_CONNECT when system_state is not SYSTEM_RUNNING,
To tell upper layers that the command was handled during
the transport layer error handler helper.

The scsi Mid Layer timeout handler function(scsi_times_out) will be
abort the cmd if the scsi LLD timeout handler return BLK_EH_DONE.
if abort cmd failed, will enter scsi EH logic.

Scsi EH will do reset target logic, if reset target failed, Will
call iscsi_eh_session_reset() function to drop the session.

The iscsi_eh_session_reset function will wait for a relogin,
session termination from userspace, or a recovery/replacement timeout.
But at this time, the app iscsid has exited, and the session was marked as
ISCSI_STATE_FAILED, So the SCSI EH process will never be
scheduled back again.

PID: 9123   TASK: 80020c1b4d80  CPU: 3   COMMAND: "scsi_eh_2"
  #0 [8632bb70] __switch_to at 80088738
  #1 [8632bb90] __schedule at 80a00480
  #2 [8632bc20] schedule at 80a00b58
  #3 [8632bc30] iscsi_eh_session_reset at 00d1ab9c [libiscsi]
  #4 [8632bcb0] iscsi_eh_recover_target at 00d1d1fc [libiscsi]
  #5 [8632bd00] scsi_try_target_reset at 806f0bac
  #6 [8632bd30] scsi_eh_ready_devs at 806f2724
  #7 [8632bde0] scsi_error_handler at 806f41d4
  #8 [8632be70] kthread at 80119ae0

Reported-by: Tianxiong Lu 
Signed-off-by: Wu Bo 
---
  drivers/scsi/libiscsi.c | 12 +++-
  1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 1e9c317..2570768 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2380,7 +2380,17 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
  
  	mutex_lock(>eh_mutex);

spin_lock_bh(>frwd_lock);
-   if (session->state == ISCSI_STATE_TERMINATE) {
+
+   /*
+* During shutdown, if session is prematurely disconnected
+* recovery won't happen and there will be hung cmds.
+* To solve this case, all cmds would be enter scsi EH.
+* But the EH path will wait for wait_event_interruptible() completed,
+* when the session state machine is not ISCSI_STATE_TERMINATE,
+* ISCSI_STATE_LOGGED_IN and ISCSI_STATE_RECOVERY_FAILED.
+*/
+   if (session->state == ISCSI_STATE_TERMINATE ||
+   unlikely(system_state != SYSTEM_RUNNING)) {
  failed:
ISCSI_DBG_EH(session,
 "failing session reset: Could not log back into "


Do you need this with the current code? If the system_state is not 
SYSTEM_RUNNING above, shouldn't we call


iscsi_conn_failure -> iscsi_conn_error_event ->
stop_conn_work_fn -> iscsi_if_stop_conn(STOP_CONN_TERM) -> iscsi_conn_stop.

iscsi_conn_stop will set session->state to ISCSI_STATE_TERMINATE, so 
when iscsi_eh_session_reset does:


wait_event_interruptible(conn->ehwait,
 session->state == ISCSI_STATE_TERMINATE ||



that will fail immediately right?

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/15627360-dd21-074c-868b-88d641372594%40oracle.com.

Re: [PATCH v2 1/1] scsi: libiscsi: fix NOP race condition

[Mike Christie]

On 10/8/20 12:11 PM, Mike Christie wrote:
> On 9/25/20 1:41 PM, ldun...@suse.com wrote:
>> From: Lee Duncan 
>>
>> iSCSI NOPs are sometimes "lost", mistakenly sent to the
>> user-land iscsid daemon instead of handled in the kernel,
>> as they should be, resulting in a message from the daemon like:
>>
>>> iscsid: Got nop in, but kernel supports nop handling.
>>
>> This can occur because of the forward- and back-locks
>> in the kernel iSCSI code, and the fact that an iSCSI NOP
>> response can be processed before processing of the NOP send
>> is complete. This can result in "conn->ping_task" being NULL
>> in iscsi_nop_out_rsp(), when the pointer is actually in
>> the process of being set.
>>
>> To work around this, we add a new state to the "ping_task"
>> pointer. In addition to NULL (not assigned) and a pointer
>> (assigned), we add the state "being set", which is signaled
>> with an INVALID pointer (using "-1").
>>
>> Signed-off-by: Lee Duncan 
>> ---
>>  drivers/scsi/libiscsi.c | 13 ++---
>>  include/scsi/libiscsi.h |  3 +++
>>  2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index 1e9c3171fa9f..cade108c33b6 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -738,6 +738,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct 
>> iscsi_hdr *hdr,
>> task->conn->session->age);
>>  }
>>  
>> +if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))
>> +WRITE_ONCE(conn->ping_task, task);
>> +
>>  if (!ihost->workq) {
>>  if (iscsi_prep_mgmt_task(conn, task))
>>  goto free_task;
> 
> I think the API gets a little weird now where in some cases
> __iscsi_conn_send_pdu checks the opcode to see what type of request
> it is but above we the caller sets the ping_task.
> 
> For login, tmfs and passthrough, we assume the __iscsi_conn_send_pdu
> has sent or cleaned up everything. I think it might be nicer to just
> have __iscsi_conn_send_pdu set the ping_task field before doing the
> xmit/queue call. It would then work similar to the conn->login_task
> case where that function knows about that special task too.
> 
> So in __iscsi_conn_send_pdu add a "if (opcode == ISCSI_OP_NOOP_OUT)",
> and check if it's a nop we need to track. If so set conn->ping_task.
> 
Ignore this. It won't work nicely either. To figure out if the nop is
our internal transport test ping vs a userspace ping that also needs
a reply, we would need to do something like you did above so there is
no point.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/47eca384-b54e-63cc-0f84-7ed6501f427e%40oracle.com.

Re: [PATCH v2 1/1] scsi: libiscsi: fix NOP race condition

[Mike Christie]

On 9/25/20 1:41 PM, ldun...@suse.com wrote:
> From: Lee Duncan 
> 
> iSCSI NOPs are sometimes "lost", mistakenly sent to the
> user-land iscsid daemon instead of handled in the kernel,
> as they should be, resulting in a message from the daemon like:
> 
>> iscsid: Got nop in, but kernel supports nop handling.
> 
> This can occur because of the forward- and back-locks
> in the kernel iSCSI code, and the fact that an iSCSI NOP
> response can be processed before processing of the NOP send
> is complete. This can result in "conn->ping_task" being NULL
> in iscsi_nop_out_rsp(), when the pointer is actually in
> the process of being set.
> 
> To work around this, we add a new state to the "ping_task"
> pointer. In addition to NULL (not assigned) and a pointer
> (assigned), we add the state "being set", which is signaled
> with an INVALID pointer (using "-1").
> 
> Signed-off-by: Lee Duncan 
> ---
>  drivers/scsi/libiscsi.c | 13 ++---
>  include/scsi/libiscsi.h |  3 +++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 1e9c3171fa9f..cade108c33b6 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -738,6 +738,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct 
> iscsi_hdr *hdr,
>  task->conn->session->age);
>   }
>  
> + if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))
> + WRITE_ONCE(conn->ping_task, task);
> +
>   if (!ihost->workq) {
>   if (iscsi_prep_mgmt_task(conn, task))
>   goto free_task;

I think the API gets a little weird now where in some cases
__iscsi_conn_send_pdu checks the opcode to see what type of request
it is but above we the caller sets the ping_task.

For login, tmfs and passthrough, we assume the __iscsi_conn_send_pdu
has sent or cleaned up everything. I think it might be nicer to just
have __iscsi_conn_send_pdu set the ping_task field before doing the
xmit/queue call. It would then work similar to the conn->login_task
case where that function knows about that special task too.

So in __iscsi_conn_send_pdu add a "if (opcode == ISCSI_OP_NOOP_OUT)",
and check if it's a nop we need to track. If so set conn->ping_task.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/5e1fb4eb-dd10-dbad-3da9-e8affc4f5cf0%40oracle.com.

Re: [PATCH] iscsi: iscsi_tcp: Avoid holding spinlock while calling getpeername

[Mike Christie]

On 9/27/20 11:33 PM, Mark Mielke wrote:
> Kernel may fail to boot or devices may fail to come up when
> initializing iscsi_tcp devices starting with Linux 5.8.
> 
> Marc Dionne identified the cause in RHBZ#1877345.
> 
> Commit a79af8a64d39 ("[SCSI] iscsi_tcp: use iscsi_conn_get_addr_param
> libiscsi function") introduced getpeername() within the session spinlock.
> 
> Commit 1b66d253610c ("bpf: Add get{peer, sock}name attach types for
> sock_addr") introduced BPF_CGROUP_RUN_SA_PROG_LOCK() within getpeername,
> which acquires a mutex and when used from iscsi_tcp devices can now lead
> to "BUG: scheduling while atomic:" and subsequent damage.
> 
> This commit ensures that the spinlock is released before calling
> getpeername() or getsockname(). sock_hold() and sock_put() are
> used to ensure that the socket reference is preserved until after
> the getpeername() or getsockname() complete.
> 
> Reported-by: Marc Dionne 
> Link: 
> https://urldefense.com/v3/__https://bugzilla.redhat.com/show_bug.cgi?id=1877345__;!!GqivPVa7Brio!IykqCqCEtE_EyrhXerYzj_cIlmkenkaAVddyoEOw9T6n4nExSaGFHkn0ZQrLBVSUtxQ_$
>  
> Link: 
> https://urldefense.com/v3/__https://lkml.org/lkml/2020/7/28/1085__;!!GqivPVa7Brio!IykqCqCEtE_EyrhXerYzj_cIlmkenkaAVddyoEOw9T6n4nExSaGFHkn0ZQrLBRT9NL69$
>  
> Link: 
> https://urldefense.com/v3/__https://lkml.org/lkml/2020/8/31/459__;!!GqivPVa7Brio!IykqCqCEtE_EyrhXerYzj_cIlmkenkaAVddyoEOw9T6n4nExSaGFHkn0ZQrLBfxZYLKs$
>  
> Fixes: a79af8a64d39 ("[SCSI] iscsi_tcp: use iscsi_conn_get_addr_param 
> libiscsi function")
> Fixes: 1b66d253610c ("bpf: Add get{peer, sock}name attach types for 
> sock_addr")
> Signed-off-by: Mark Mielke 
> Cc: sta...@vger.kernel.org
> ---
>  drivers/scsi/iscsi_tcp.c | 22 +++---
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index b5dd1caae5e9..d10efb66cf19 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -736,6 +736,7 @@ static int iscsi_sw_tcp_conn_get_param(struct 
> iscsi_cls_conn *cls_conn,
>   struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
>   struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
>   struct sockaddr_in6 addr;
> + struct socket *sock;
>   int rc;
>  
>   switch(param) {
> @@ -747,13 +748,17 @@ static int iscsi_sw_tcp_conn_get_param(struct 
> iscsi_cls_conn *cls_conn,
>   spin_unlock_bh(>session->frwd_lock);
>   return -ENOTCONN;
>   }
> + sock = tcp_sw_conn->sock;
> + sock_hold(sock->sk);
> + spin_unlock_bh(>session->frwd_lock);
> +
>   if (param == ISCSI_PARAM_LOCAL_PORT)
> - rc = kernel_getsockname(tcp_sw_conn->sock,
> + rc = kernel_getsockname(sock,
>   (struct sockaddr *));
>   else
> - rc = kernel_getpeername(tcp_sw_conn->sock,
> + rc = kernel_getpeername(sock,
>   (struct sockaddr *));
> - spin_unlock_bh(>session->frwd_lock);
> + sock_put(sock->sk);
>   if (rc < 0)
>   return rc;
>  
> @@ -775,6 +780,7 @@ static int iscsi_sw_tcp_host_get_param(struct Scsi_Host 
> *shost,
>   struct iscsi_tcp_conn *tcp_conn;
>   struct iscsi_sw_tcp_conn *tcp_sw_conn;
>   struct sockaddr_in6 addr;
> + struct socket *sock;
>   int rc;
>  
>   switch (param) {
> @@ -789,16 +795,18 @@ static int iscsi_sw_tcp_host_get_param(struct Scsi_Host 
> *shost,
>   return -ENOTCONN;
>   }
>   tcp_conn = conn->dd_data;
> -
>   tcp_sw_conn = tcp_conn->dd_data;
> - if (!tcp_sw_conn->sock) {
> + sock = tcp_sw_conn->sock;
> + if (!sock) {
>   spin_unlock_bh(>frwd_lock);
>   return -ENOTCONN;
>   }
> + sock_hold(sock->sk);
> + spin_unlock_bh(>frwd_lock);
>  
> - rc = kernel_getsockname(tcp_sw_conn->sock,
> + rc = kernel_getsockname(sock,
>   (struct sockaddr *));
> - spin_unlock_bh(>frwd_lock);
> + sock_put(sock->sk);
>   if (rc < 0)
>   return rc;
>  
> 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/aec41066-f5d3-c426-11c1-25e9b0a9ed44%40oracle.com.

Re: [PATCH] scsi: iscsi: Do not put host in iscsi_set_flashnode_param()

[Mike Christie]

On 6/15/20 3:12 AM, Jing Xiangfeng wrote:
> If scsi_host_lookup() failes we will jump to put_host, which may
> cause panic. Jump to exit_set_fnode to fix it.
> 
> Signed-off-by: Jing Xiangfeng 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index f4cc08e..c5e99f9 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -3291,7 +3291,7 @@ static int iscsi_set_flashnode_param(struct 
> iscsi_transport *transport,
>   pr_err("%s could not find host no %u\n",
>  __func__, ev->u.set_flashnode.host_no);
>   err = -ENODEV;
> - goto put_host;
> + goto exit_set_fnode;
>   }
>  
>   idx = ev->u.set_flashnode.flashnode_idx;
> 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/30b2622a-a04b-6b17-c50a-e6920be359ae%40oracle.com.

Re: [PATCH] scsi: iscsi: register sysfs for workqueue iscsi_destroy

[Mike Christie]

On 7/3/20 12:16 AM, Bob Liu wrote:

Register sysfs for workqueue iscsi_destroy, so that users can set cpu affinity
through "cpumask" for this workqueue to get better isolation in cloud
multi-tenant scenario.

This patch unfolded create_singlethread_workqueue(), added WQ_SYSFS and drop
__WQ_ORDERED_EXPLICIT since __WQ_ORDERED_EXPLICIT workqueue isn't allowed to
change "cpumask".

Suggested-by: Mike Christie 
Signed-off-by: Bob Liu 
---
  drivers/scsi/scsi_transport_iscsi.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 7ae5024..aa8d4a3 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -4766,7 +4766,9 @@ static __init int iscsi_transport_init(void)
goto release_nls;
}
  
-	iscsi_destroy_workq = create_singlethread_workqueue("iscsi_destroy");

+   iscsi_destroy_workq = alloc_workqueue("%s",
+   WQ_SYSFS | __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_UNBOUND,
+   1, "iscsi_destroy");
if (!iscsi_destroy_workq) {
err = -ENOMEM;
    goto destroy_wq;



Reviewed-by: Mike Christie 

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/a34c8886-a6dc-4ce0-5ffd-a09d913daa5d%40oracle.com.

Re: [PATCH 2/2] scsi: register sysfs for scsi workqueue

[Mike Christie]

On 6/30/20 10:07 PM, Bob Liu wrote:
> So that scsi_wq_xxx and scsi_tmf_xxx can be bind to different cpu to get 
> better
> isolation.
> 
> This patch unfolded create_singlethread_workqueue(), added WQ_SYSFS and 
> dropped
> __WQ_ORDERED_EXPLICIT since __WQ_ORDERED_EXPLICIT workqueue isn't allowed to
> change "cpumask".
> 
> Signed-off-by: Bob Liu 
> ---
>  drivers/scsi/hosts.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 7ec91c3..37d1c55 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -272,8 +272,10 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, 
> struct device *dev,
>   if (shost->transportt->create_work_queue) {
>   snprintf(shost->work_q_name, sizeof(shost->work_q_name),
>"scsi_wq_%d", shost->host_no);
> - shost->work_q = create_singlethread_workqueue(
> - shost->work_q_name);
> + shost->work_q = alloc_workqueue("%s",
> + WQ_SYSFS | __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_UNBOUND,
> + 1, shost->work_q_name);
> +
>   if (!shost->work_q) {
>   error = -EINVAL;
>   goto out_free_shost_data;
> @@ -487,7 +489,7 @@ struct Scsi_Host *scsi_host_alloc(struct 
> scsi_host_template *sht, int privsize)
>   }
>  
>   shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
> - WQ_UNBOUND | WQ_MEM_RECLAIM,
> + WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_SYSFS,
>  1, shost->host_no);
>   if (!shost->tmf_work_q) {
>   shost_printk(KERN_WARNING, shost,
> 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/fe5cb411-aed9-0bb0-e55e-18629e41eb9d%40oracle.com.

Re: [PATCH 1/2] scsi: iscsi: change back iscsi workqueue max_active argu to 1

[Mike Christie]

On 6/30/20 10:07 PM, Bob Liu wrote:
> Commit 3ce4196 (scsi: iscsi: Register sysfs for iscsi workqueue) enables
> 'cpumask' support for iscsi workqueues.
> 
> While there is a mistake in that commit, it's unnecessary to set
> max_active = 2 since 'cpumask' can be modified when max_active = 1.
> 
> This patch change back max_active to 1 so as to keep the same behaviour as
> before.
> 
> Signed-off-by: Bob Liu 
> ---
>  drivers/scsi/libiscsi.c | 2 +-
>  drivers/scsi/scsi_transport_iscsi.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index e5a64d4..49c8a18 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2629,7 +2629,7 @@ struct Scsi_Host *iscsi_host_alloc(struct 
> scsi_host_template *sht,
>   "iscsi_q_%d", shost->host_no);
>   ihost->workq = alloc_workqueue("%s",
>   WQ_SYSFS | __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_UNBOUND,
> - 2, ihost->workq_name);
> + 1, ihost->workq_name);
>   if (!ihost->workq)
>   goto free_host;
>   }
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index f4cc08e..7ae5024 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -4760,7 +4760,7 @@ static __init int iscsi_transport_init(void)
>  
>   iscsi_eh_timer_workq = alloc_workqueue("%s",
>   WQ_SYSFS | __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_UNBOUND,
> - 2, "iscsi_eh");
> + 1, "iscsi_eh");
>   if (!iscsi_eh_timer_workq) {
>   err = -ENOMEM;
>   goto release_nls;
> 

Reviewed-by: Mike Christie 

I think it should get it into 5.8 to fix the bug I mentioned in the other 
thread.

For 5.9, you'll want to send another patch to update the iscsi_destroy_workq 
that got added in 5.8.

scsi_transport_iscsi.c:

iscsi_destroy_workq = create_singlethread_workqueue("iscsi_destroy");

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/8e09f77c-ac01-358b-0451-d4107ef5cd34%40oracle.com.

Re: [PATCH 2/2] scsi: register sysfs for scsi/iscsi workqueues

[Mike Christie]

On 6/11/20 5:07 AM, Bob Liu wrote:

This patch enable setting cpu affinity through "cpumask" for below
scsi/iscsi workqueues, so as to get better isolation.
- scsi_wq_*
- scsi_tmf_*
- iscsi_q_xx
- iscsi_eh

Signed-off-by: Bob Liu 
---
  drivers/scsi/hosts.c| 4 ++--
  drivers/scsi/libiscsi.c | 2 +-
  drivers/scsi/scsi_transport_iscsi.c | 2 +-
  3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 1d669e4..4b9f80d 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -272,7 +272,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct 
device *dev,
if (shost->transportt->create_work_queue) {
snprintf(shost->work_q_name, sizeof(shost->work_q_name),
 "scsi_wq_%d", shost->host_no);
-   shost->work_q = create_singlethread_workqueue(
+   shost->work_q = create_singlethread_workqueue_noorder(
shost->work_q_name);
if (!shost->work_q) {
error = -EINVAL;


This patch seems ok for the iscsi, fc, tmf, and non transport class scan 
uses. We are either heavy handed with flushes or did not need ordering.


I don't know about the zfcp use though, so I cc'd  the developers listed 
as maintainers. It looks like for zfcp we can do:


zfcp_scsi_rport_register->fc_remote_port_add->fc_remote_port_create->scsi_queue_work 
to scan the scsi target on the rport.


and then zfcp_scsi_rport_register can call zfcp_unit_queue_scsi_scan->
scsi_queue_work which will scan for a specific lun.

It looks ok if those are not ordered, but I would get their review to 
make sure.


--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/cf9ae940-87b2-c8a1-3dba-4d2b57ebe9dd%40oracle.com.

Re: [PATCH] iscsi: Perform connection failure entirely in kernel space

[Mike Christie]

On 12/10/2019 06:05 PM, Mike Christie wrote:
> On 12/09/2019 12:20 PM, Gabriel Krisman Bertazi wrote:
>> From: Bharath Ravi 
>>
>> Connection failure processing depends on a daemon being present to (at
>> least) stop the connection and start recovery.  This is a problem on a
>> multipath scenario, where if the daemon failed for whatever reason, the
>> SCSI path is never marked as down, multipath won't perform the
>> failover and IO to the device will be forever waiting for that
>> connection to come back.
>>
>> This patch implements an optional feature in the iscsi module, to
>> perform the connection failure inside the kernel.  This way, the
>> failover can happen and pending IO can continue even if the daemon is
>> dead. Once the daemon comes alive again, it can perform recovery
>> procedures if applicable.
>>
>> Co-developed-by: Dave Clausen 
>> Signed-off-by: Dave Clausen 
>> Co-developed-by: Nick Black 
>> Signed-off-by: Nick Black 
>> Co-developed-by: Vaibhav Nagarnaik 
>> Signed-off-by: Vaibhav Nagarnaik 
>> Co-developed-by: Anatol Pomazau 
>> Signed-off-by: Anatol Pomazau 
>> Co-developed-by: Tahsin Erdogan 
>> Signed-off-by: Tahsin Erdogan 
>> Co-developed-by: Frank Mayhar 
>> Signed-off-by: Frank Mayhar 
>> Co-developed-by: Junho Ryu 
>> Signed-off-by: Junho Ryu 
>> Co-developed-by: Khazhismel Kumykov 
>> Signed-off-by: Khazhismel Kumykov 
>> Signed-off-by: Bharath Ravi 
>> Co-developed-by: Gabriel Krisman Bertazi 
>> Signed-off-by: Gabriel Krisman Bertazi 
>> ---
>>  drivers/scsi/scsi_transport_iscsi.c | 46 +
>>  include/scsi/scsi_transport_iscsi.h |  1 +
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
>> b/drivers/scsi/scsi_transport_iscsi.c
>> index 417b868d8735..7251b2b5b272 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -36,6 +36,12 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(iscsi_dbg_session);
>>  EXPORT_TRACEPOINT_SYMBOL_GPL(iscsi_dbg_tcp);
>>  EXPORT_TRACEPOINT_SYMBOL_GPL(iscsi_dbg_sw_tcp);
>>  
>> +static bool kern_conn_failure;
>> +module_param(kern_conn_failure, bool, S_IRUGO|S_IWUSR);
>> +MODULE_PARM_DESC(kern_conn_failure,
>> + "Allow the kernel to detect and disable broken connections "
>> + "without requiring userspace intervention");
>> +
>>  static int dbg_session;
>>  module_param_named(debug_session, dbg_session, int,
>> S_IRUGO | S_IWUSR);
>> @@ -84,6 +90,12 @@ struct iscsi_internal {
>>  struct transport_container session_cont;
>>  };
>>  
>> +/* Worker to perform connection failure on unresponsive connections
>> + * completely in kernel space.
>> + */
>> +static void stop_conn_work_fn(struct work_struct *work);
>> +static DECLARE_WORK(stop_conn_work, stop_conn_work_fn);
>> +
>>  static atomic_t iscsi_session_nr; /* sysfs session id for next new session 
>> */
>>  static struct workqueue_struct *iscsi_eh_timer_workq;
>>  
>> @@ -1609,6 +1621,7 @@ static DEFINE_MUTEX(rx_queue_mutex);
>>  static LIST_HEAD(sesslist);
>>  static DEFINE_SPINLOCK(sesslock);
>>  static LIST_HEAD(connlist);
>> +static LIST_HEAD(connlist_err);
>>  static DEFINE_SPINLOCK(connlock);
>>  
>>  static uint32_t iscsi_conn_get_sid(struct iscsi_cls_conn *conn)
>> @@ -2245,6 +2258,7 @@ iscsi_create_conn(struct iscsi_cls_session *session, 
>> int dd_size, uint32_t cid)
>>  
>>  mutex_init(>ep_mutex);
>>  INIT_LIST_HEAD(>conn_list);
>> +INIT_LIST_HEAD(>conn_list_err);
>>  conn->transport = transport;
>>  conn->cid = cid;
>>  
>> @@ -2291,6 +2305,7 @@ int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
>>  
>>  spin_lock_irqsave(, flags);
>>  list_del(>conn_list);
>> +list_del(>conn_list_err);
>>  spin_unlock_irqrestore(, flags);
>>  
>>  transport_unregister_device(>dev);
>> @@ -2405,6 +2420,28 @@ int iscsi_offload_mesg(struct Scsi_Host *shost,
>>  }
>>  EXPORT_SYMBOL_GPL(iscsi_offload_mesg);
>>  
>> +static void stop_conn_work_fn(struct work_struct *work)
>> +{
>> +struct iscsi_cls_conn *conn, *tmp;
>> +unsigned long flags;
>> +LIST_HEAD(recovery_list);
>> +
>> +spin_lock_irqsave(, flags);
>> +if (list_empty(_err)) {
>> +spin_unlock_irqrestore(, flags);
>> +return;
>&g

Re: [PATCH] iscsi: Perform connection failure entirely in kernel space

[Mike Christie]

On 12/09/2019 12:20 PM, Gabriel Krisman Bertazi wrote:
> From: Bharath Ravi 
> 
> Connection failure processing depends on a daemon being present to (at
> least) stop the connection and start recovery.  This is a problem on a
> multipath scenario, where if the daemon failed for whatever reason, the
> SCSI path is never marked as down, multipath won't perform the
> failover and IO to the device will be forever waiting for that
> connection to come back.
> 
> This patch implements an optional feature in the iscsi module, to
> perform the connection failure inside the kernel.  This way, the
> failover can happen and pending IO can continue even if the daemon is
> dead. Once the daemon comes alive again, it can perform recovery
> procedures if applicable.
> 
> Co-developed-by: Dave Clausen 
> Signed-off-by: Dave Clausen 
> Co-developed-by: Nick Black 
> Signed-off-by: Nick Black 
> Co-developed-by: Vaibhav Nagarnaik 
> Signed-off-by: Vaibhav Nagarnaik 
> Co-developed-by: Anatol Pomazau 
> Signed-off-by: Anatol Pomazau 
> Co-developed-by: Tahsin Erdogan 
> Signed-off-by: Tahsin Erdogan 
> Co-developed-by: Frank Mayhar 
> Signed-off-by: Frank Mayhar 
> Co-developed-by: Junho Ryu 
> Signed-off-by: Junho Ryu 
> Co-developed-by: Khazhismel Kumykov 
> Signed-off-by: Khazhismel Kumykov 
> Signed-off-by: Bharath Ravi 
> Co-developed-by: Gabriel Krisman Bertazi 
> Signed-off-by: Gabriel Krisman Bertazi 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 46 +
>  include/scsi/scsi_transport_iscsi.h |  1 +
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 417b868d8735..7251b2b5b272 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -36,6 +36,12 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(iscsi_dbg_session);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(iscsi_dbg_tcp);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(iscsi_dbg_sw_tcp);
>  
> +static bool kern_conn_failure;
> +module_param(kern_conn_failure, bool, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(kern_conn_failure,
> +  "Allow the kernel to detect and disable broken connections "
> +  "without requiring userspace intervention");
> +
>  static int dbg_session;
>  module_param_named(debug_session, dbg_session, int,
>  S_IRUGO | S_IWUSR);
> @@ -84,6 +90,12 @@ struct iscsi_internal {
>   struct transport_container session_cont;
>  };
>  
> +/* Worker to perform connection failure on unresponsive connections
> + * completely in kernel space.
> + */
> +static void stop_conn_work_fn(struct work_struct *work);
> +static DECLARE_WORK(stop_conn_work, stop_conn_work_fn);
> +
>  static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
>  static struct workqueue_struct *iscsi_eh_timer_workq;
>  
> @@ -1609,6 +1621,7 @@ static DEFINE_MUTEX(rx_queue_mutex);
>  static LIST_HEAD(sesslist);
>  static DEFINE_SPINLOCK(sesslock);
>  static LIST_HEAD(connlist);
> +static LIST_HEAD(connlist_err);
>  static DEFINE_SPINLOCK(connlock);
>  
>  static uint32_t iscsi_conn_get_sid(struct iscsi_cls_conn *conn)
> @@ -2245,6 +2258,7 @@ iscsi_create_conn(struct iscsi_cls_session *session, 
> int dd_size, uint32_t cid)
>  
>   mutex_init(>ep_mutex);
>   INIT_LIST_HEAD(>conn_list);
> + INIT_LIST_HEAD(>conn_list_err);
>   conn->transport = transport;
>   conn->cid = cid;
>  
> @@ -2291,6 +2305,7 @@ int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
>  
>   spin_lock_irqsave(, flags);
>   list_del(>conn_list);
> + list_del(>conn_list_err);
>   spin_unlock_irqrestore(, flags);
>  
>   transport_unregister_device(>dev);
> @@ -2405,6 +2420,28 @@ int iscsi_offload_mesg(struct Scsi_Host *shost,
>  }
>  EXPORT_SYMBOL_GPL(iscsi_offload_mesg);
>  
> +static void stop_conn_work_fn(struct work_struct *work)
> +{
> + struct iscsi_cls_conn *conn, *tmp;
> + unsigned long flags;
> + LIST_HEAD(recovery_list);
> +
> + spin_lock_irqsave(, flags);
> + if (list_empty(_err)) {
> + spin_unlock_irqrestore(, flags);
> + return;
> + }
> + list_splice_init(_err, _list);
> + spin_unlock_irqrestore(, flags);
> +
> + mutex_lock(_queue_mutex);
> + list_for_each_entry_safe(conn, tmp, _list, conn_list_err) {
> + conn->transport->stop_conn(conn, STOP_CONN_RECOVER);
> + list_del_init(>conn_list_err);
> + }
> + mutex_unlock(_queue_mutex);
> +}
> +
>  void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err 
> error)
>  {
>   struct nlmsghdr *nlh;
> @@ -2412,6 +2449,15 @@ void iscsi_conn_error_event(struct iscsi_cls_conn 
> *conn, enum iscsi_err error)
>   struct iscsi_uevent *ev;
>   struct iscsi_internal *priv;
>   int len = nlmsg_total_size(sizeof(*ev));
> + unsigned long flags;
> +
> + if (kern_conn_failure) {
> + spin_lock_irqsave(, flags);
> + 

Re: RFC: A problem with stale session information?

[Mike Christie]

On 09/19/2018 12:39 PM, Lee Duncan wrote:
> Hi All:
> 
> I have been tracking down an issue that has to do with iSCSI sessions going 
> away.
> 
> If an initiator and target have a session, then iscsid goes away (stops or 
> dies, without logging out of the session), and then the target goes away, 
> iscsid has an issue when it starts back up.
> 
> It looks like iscsid retries forever to reconnect the session. There doesn’t 
> appear to be any code that gives up and cleans up the session. Ever.
> 
> During this retry-forever period, any attempt to access important session 
> information from sysfs returns ENOTCONN — a pretty clear indication that the 
> node is not connected.
> 
> It seems to me like this is a bug. Or two:
> 
> 1 - The retries should be limited to something reasonable, then give up and 
> clean up the session, assuming it’s “bad”, and
> 
> 2 - When iscsiadm sees the ENOTCONN, it should print out a message saying “in 
> recovery to unconnected node, try again” while the retries are going on.
> 
> Another approach might be to let the retries go on forever, or until a user 
> runs some sort of “force the session to end” command?
> 
> I have been looking at this a while, and it’s not clear how simple it will be 
> to implement either of these approaches. Right now, this registers as a 
> “libopeniscsiusr BUG”, which is not acceptable.
> 

This is one of those things where I said I would fix if anyone ever
complains, but luckily I made it through my entire time as maintainer
and no one ever did. So, I guess basically tag you're it :)

Checkout node.session.initial_login_retry_max and iscsi_login_eh. The
current state machine detects the iscsid restart case as a relogin and
so that setting does not kick in. You could:

1. modify the state machine check so it does.
2. add a new setting that works like initial_login_retry_max but doesn't
really work like it :) What I mean is that initial_login_retry_max was
trying to handle a lot of different cases so it is a little odd. See the
iscsid.conf comments. You probably want to make something that is more
straightforward.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: max_sectors_kb so small, throughout so small

[Mike Christie]

On 09/12/2018 09:26 PM, 3kboy2...@gmail.com wrote:
> Thank you for your reply, Mike.
> Now my iscsi disk performance can be around 300MB/s in 4M sequence
> write(TCMU+LIO)
> It increase from 20MB/s to 300MB/s, after I can change max_data_area_mb
> from 8 to 256 && hw_max_sectors from 128 to 8192.
> To my cluster, after a lot of tests I found that I should keep
> "max_data_area_mb>128 &&  hw_max_sectors>=4096" in order to get a good
> performance.
> Does my setting can cause some side effects?

It depends on the kernel. For the RHEL/Centos kernel you are using the
kernel will preallocate max_data_area_mb of memory for each device. For
upstream, we no longer preallocate, but once it is allocated we do not
free the memory unless global_max_data_area_mb is hit or the device is
removed.

With a high hw_max_sectors latency will increase due to sending really
large commands, so it depends on your workload and what you need.

We used to set hw_max_sectors to the rbd object size (4MB by default),
but in our testing we would see throughput go down around 512k - 1MB.

> Are there any other parameters can improve the performance quite obvious?

The normal networking ones like using jumbo frames,
net.core.*/net.ipv4.*, etc. Check your nic's documentation for the best
settings.

There are some iscsi ones like the cmdsn/cmds_max I mentioned and then
also the segment related ones like MaxRecvDataSegmentLength,
MaxXmitDataSegmentLength, MaxBurstLength, FirstBurstLength and have
ImmediateData on.


> Why the default value of max_data_area_mb and hw_max_sectors is so
> small, and bad performance?

I do not know. It was just what the original author had used initially.

> Could you talk something about this?
> At least, max_data_area_mb>128 &&  hw_max_sectors>=4096, I can get a
> better performance seems to be acceptable.
> If my settings can give other users some help, I will be happy.
> 
> 在 2018年9月12日星期三 UTC+8上午12:39:16，Mike Christie写道：
> 
> On 09/11/2018 11:30 AM, Mike Christie wrote:
> > Hey,
> >
> > Cc mchr...@redhat.com , or I will not see these
> messages until I check
> > the list maybe once a week.
> >
> > On 09/05/2018 10:36 PM, 3kbo...@gmail.com  wrote:
> >> What lio fabric driver are you using? iSCSI? What kernel
> version
> >> and
> >> what version of tcmu-runner?
> >>
> >> io fabric driver :iscsi
> >>
> >> iscsid version:  2.0-873
> >>
> >> OS version:  CentOS Linux release 7.5.1804
> (Core)
> >>
> >> kernel version:  3.10.0-862.el7.x86_64
> >>
> >> tcmu-runner version:1.4.0-rc1
> >>
> >>
> 
> There is also a perf bug in that initiator if the node.session.cmds_max
> is greater than the LIO default_cmdsn_depth and your IO test tries to
> send cmds > node.session.cmds_max.
> 
> I have known the bug before, because I had google a lot.
> It increase from 320MB/s to 340MB/s (4M seq write), seems  like a stable
> promotion.
> 
> Settings Before: 320MB/s
>  node.session.cmds_max = 2048
>  default_cmdsn_depth = 64
> 
> Settings After: 340MB/s
>  node.session.cmds_max = 64
>  default_cmdsn_depth = 64
> 
> So set the node.session.cmds_max and default_cmdsn_depth to the same
> value. You can set the default_cmdsn_depth in saveconfig.json, and set
> cmds_max in the iscsiadm node db (after you set it make sure you logout
> and login the session again).
> 
> Now  I set set the node.session.cmds_max and default_cmdsn_depth both to
> be 64.
> 
> Thank you very much!
> 
> -- 
> You received this message because you are subscribed to the Google
> Groups "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to open-iscsi+unsubscr...@googlegroups.com
> <mailto:open-iscsi+unsubscr...@googlegroups.com>.
> To post to this group, send email to open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: max_sectors_kb so small, throughout so small

[Mike Christie]

On 09/11/2018 11:30 AM, Mike Christie wrote:
> Hey,
> 
> Cc mchri...@redhat.com, or I will not see these messages until I check
> the list maybe once a week.
> 
> On 09/05/2018 10:36 PM, 3kboy2...@gmail.com wrote:
>> What lio fabric driver are you using? iSCSI? What kernel version
>> and
>> what version of tcmu-runner?
>>
>> io fabric driver :iscsi
>>
>> iscsid version:  2.0-873
>>
>> OS version:  CentOS Linux release 7.5.1804 (Core)
>>
>> kernel version:  3.10.0-862.el7.x86_64 
>>
>> tcmu-runner version:1.4.0-rc1
>>
>>

There is also a perf bug in that initiator if the node.session.cmds_max
is greater than the LIO default_cmdsn_depth and your IO test tries to
send cmds > node.session.cmds_max.

So set the node.session.cmds_max and default_cmdsn_depth to the same
value. You can set the default_cmdsn_depth in saveconfig.json, and set
cmds_max in the iscsiadm node db (after you set it make sure you logout
and login the session again).


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: max_sectors_kb so small, throughout so small

[Mike Christie]

Hey,

Cc mchri...@redhat.com, or I will not see these messages until I check
the list maybe once a week.

On 09/05/2018 10:36 PM, 3kboy2...@gmail.com wrote:
> What lio fabric driver are you using? iSCSI? What kernel version
> and
> what version of tcmu-runner?
> 
> io fabric driver :iscsi
> 
> iscsid version:  2.0-873
> 
> OS version:  CentOS Linux release 7.5.1804 (Core)
> 
> kernel version:  3.10.0-862.el7.x86_64 
> 
> tcmu-runner version:1.4.0-rc1
> 
> 
> Target Build:
> 
> targetcli /iscsi create iqn.2018-09.com.test:target1
> 
> targetcli  /backstores/user:rbd create name=my_replicated_test
> size=1000G cfgstring=rbd_pool/replicated_image1 hw_max_sectors=8192

That kernel has a default data area (kernel buffer used to pass scsi
command data) of 8MB, so with a command size of 8192 sectors you could
only 2 commands at once.

When you create the backstore pass it a new max_data_area_mb value like
this (make sure you have the newest rtslib-fb, configshell-fb and
targetcli-fb from the upstream github repos):

targetcli  /backstores/user:rbd create name=my_replicated_test
size=1000G cfgstring=rbd_pool/replicated_image1 hw_max_sectors=8192
control=max_data_area_mb=32

This would increase the buffer to 32 MB.

Or for an existing setup add the control line in the saveconfig.json
between the config and hw_max_sectors line like this:

  "config": "rbd/rbd_pool/replicated_image"
  "control": "max_data_area_mb=32",
  "hw_max_sectors": 8192,

Note that this will prealloocate 32 MBs of memory for the device.

> 
> targetcli  /iscsi/iqn.2018-09.com.test:target1/tpg1/luns create
> /backstores/user:rbd/my_replicated_test
> 
> targetcli /iscsi/iqn.2018-09.com.test:target1/tpg1/portals create
> 10.0.1.111
> 
> targetcli /iscsi/iqn.2018-09.com.test:target1/tpg1 set auth
> userid=** password=**
> 
> targetcli /iscsi/iqn.2018-09.com.test:target1/tpg1 set attribute
> authentication=1 demo_mode_write_protect=0 generate_node_acls=1 
> 
> 
> Target Setting:
> 
>  
> 
> 屏幕快照 2018-09-06 上午10.43.11.png
> 
> 
> 
> 
> 
> 
> 
> 
> 
>  
> 
> > Could you give me some suggestions about improving the
> performance of
> >  /backstore/user:rbd/ device.
> >
> > Thanks very much!
> >
> > 在 2018年9月3日星期一 UTC+8上午10:31:48，3kbo...@gmail.com写道：
> >
> > Hello Mike,
> >
> > Thank you for your informative response.
> >
> >
> > 在 2018年8月28日星期二 UTC+8上午8:49:46，Mike Christie写道：
> >
> > On 08/21/2018 08:52 PM, 3kbo...@gmail.com wrote:
> > > Hi folks,
> > >
> > > I am newbie to open-iscsi.
> > > My case is I export ceph rbd by open-iscsi.
> > >
> > > I found the max_sectors_kb is 64, the value is so
> small, and
> > 4M sequence
> > > write is only about 10MB/s.
> > > I can not increase max_sectors_kb, if I do it will
> return
> > "bash: echo:
> > > write error: Invalid argument"(But I can change the
> value to a
> > small one
> > > < 64, max_hw_sectors_kb is 32767)
> > >
> >
> > In new version of the linux kernel the initiator will
> use the
> > optimal
> > value reported by the target and then uses the max
> reported as
> > the limit
> > that the user can set. It sounds like you are using
> rbd/ceph with
> > tcmu-runner which has a default limits of 64K.
> >
> > Yes, I am using  tcmu-runner and gwcli.
> >
> >
> > If you are using targetcli/lio directly then you can set
> > hw_max_sectors
> > through targcli when you create the device or in the
> > saveconfig.json file.
> >
> > If you are using the ceph-iscsi tools then I am
> actually working
> > on a
> > gwcli command to configure this right now.
> >
>

Re: max_sectors_kb so small, throughout so small

[Mike Christie]

On 09/03/2018 02:31 AM, 3kboy2...@gmail.com wrote:
> Hello Mike,
> 
> I am very appreciate for your instruction.
> Now I can  set hw_max_sectors through targcli when i create the device.
> I set it to 8192 same as raw rbd device.
> The performance improve a little, 4M seq write increase from 24MB/s to
> 40MB/s.(hw_max_sectors 64->8192, it is a /backstore/user:rbd/disk_xxx
> device)
> But it is far away from block device, if I usr /backstore/block 4M seq
> write it will be 500MB/s, performance is still a big problem.(first I
> should map a rbd device , then export  it by targetcli)
> 
> The performance difference between /backstore/user:rbd/ and
> /backstore/block is so big, is it normal?

backstore/user will be lower than block, but I do not think it would a
difference like you are seeing.

What lio fabric driver are you using? iSCSI? What kernel version and
what version of tcmu-runner?


> Could you give me some suggestions about improving the performance of
>  /backstore/user:rbd/ device.
> 
> Thanks very much!
> 
> 在 2018年9月3日星期一 UTC+8上午10:31:48，3kbo...@gmail.com写道：
> 
> Hello Mike,
> 
> Thank you for your informative response.
> 
> 
> 在 2018年8月28日星期二 UTC+8上午8:49:46，Mike Christie写道：
> 
> On 08/21/2018 08:52 PM, 3kbo...@gmail.com wrote:
> > Hi folks,
> >
> > I am newbie to open-iscsi.
> > My case is I export ceph rbd by open-iscsi.
> >
> > I found the max_sectors_kb is 64, the value is so small, and
> 4M sequence
> > write is only about 10MB/s.
> > I can not increase max_sectors_kb, if I do it will return
> "bash: echo:
> > write error: Invalid argument"(But I can change the value to a
> small one
> > < 64, max_hw_sectors_kb is 32767)
> >
> 
> In new version of the linux kernel the initiator will use the
> optimal
> value reported by the target and then uses the max reported as
> the limit
> that the user can set. It sounds like you are using rbd/ceph with
> tcmu-runner which has a default limits of 64K.
> 
> Yes, I am using  tcmu-runner and gwcli.
> 
> 
> If you are using targetcli/lio directly then you can set
> hw_max_sectors
> through targcli when you create the device or in the
> saveconfig.json file.
> 
> If you are using the ceph-iscsi tools then I am actually working
> on a
> gwcli command to configure this right now.
> 
> Yes, I am using  ceph-iscsi tools.How can I change the limit by
> ceph-iscsi tools?
> 
> Thanks.
> 
> -- 
> You received this message because you are subscribed to the Google
> Groups "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to open-iscsi+unsubscr...@googlegroups.com
> <mailto:open-iscsi+unsubscr...@googlegroups.com>.
> To post to this group, send email to open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: max_sectors_kb so small, throughout so small

[Mike Christie]

On 08/21/2018 08:52 PM, 3kboy2...@gmail.com wrote:
> Hi folks,
> 
> I am newbie to open-iscsi.
> My case is I export ceph rbd by open-iscsi.
> 
> I found the max_sectors_kb is 64, the value is so small, and 4M sequence
> write is only about 10MB/s.
> I can not increase max_sectors_kb, if I do it will return "bash: echo:
> write error: Invalid argument"(But I can change the value to a small one
> < 64, max_hw_sectors_kb is 32767)
> 

In new version of the linux kernel the initiator will use the optimal
value reported by the target and then uses the max reported as the limit
that the user can set. It sounds like you are using rbd/ceph with
tcmu-runner which has a default limits of 64K.

If you are using targetcli/lio directly then you can set hw_max_sectors
through targcli when you create the device or in the saveconfig.json file.

If you are using the ceph-iscsi tools then I am actually working on a
gwcli command to configure this right now.


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: iscsistart -N skips offload NICs causing boot to fail on debian

[Mike Christie]

On 12/20/2016 12:15 PM, Andrew Patterson wrote:
> On 12/12/2016 02:38 PM, Andrew Patterson wrote:
>>
>>
>> On Friday, December 9, 2016 at 3:33:57 PM UTC-7, Mike Christie wrote:
>>
>> On 12/08/2016 10:55 AM, andrew.p...@hpe.com  wrote:
>> > I am trying to get iSCSI boot working on a debian-based system
>> > using built-in uEFI iSCSI initiator firmware and broadcom
>> > NICs (bnx2x).
>> >
>> > The debian initramfs uses the ibft support in iscsistart to log
>> > into the root volume. The initramfs script uses iscsistart -N to
>> > bring up the NICs before logging in with iscsstart -b. This
>> > process works fine when using non-offload NICs, but fails when
>> > using NICs using the bnx2, bnx2x, cxgb3, and cxgb4 drivers due to
>> > the following code in
>> > utils/fwparam_ibft/fw_entry.c:fw_setup_nics():
>> >
>> > list_for_each_entry(context, , list) {  
>> > /* if it is a offload nic ignore it */
>> > if (!net_get_transport_name_from_netdev(context->iface,
>> > transport))
>> > continue
>> >
>> >
>> > Which does a lookup in the table in usr/iscsi_net_util.c
>> >
>> > static struct iscsi_net_driver net_drivers[] = {
>> > {"cxgb3", "cxgb3i" },
>> > {"cxgb4", "cxgb4i" },
>> > {"bnx2", "bnx2i" },
>> > {"bnx2x", "bnx2i"},
>> > {NULL, NULL}
>> > };
>> >
>> >
>> > to see if -N should skip this NIC.  I cannot determine the reason
>> > for this check. Do iscsi offload NICs not need to be configured
>> > for boot? The current code causes iscsi boots to fail. I removed the
>> > check and booting works fine.
>> >
>>
>> If you use the ibft net info for the nic, what do you use for the iscsi
>> offload engine? 
>>
>>
>> I am not configuring it at boot (at least not yet). I haven't tried
>> using the boot support in the NIC firmware yet, just the uEFI iSCSI firmware
>> which will work with any NIC.
>>
>>
>>  
>>
>> Do they both have the same IP, and are you creating boot
>> iscsi sessions through the offload engine?
>>
>>
>> I am not using offload yet. But I will probably need to get this working 
>> next.
>>
>>
>>
>> cxgb*i is able to share the net info. The normal old nic engine and
>> iscsi engine are able to use the same IP. That originally was not
>> supposed to be allowed upstream, but it got in.
>>
>> bnx2* is (or at least was when we wrote that code) not able to share the
>> net info. The nic and iscsi engines need different IPs.
>>
>>
>> That makes some sense. The firmware has two different MAC addresses,
>> one for the NIC and one for iSCSI.
>>
>>
>>  
>>
>>
>> So to cover both types we just use the ibft net info for the iscsi
>> offload engines.
>>
>> In the version of the code you are using is OFFLOAD_BOOT_SUPPORTED
>> defined in open-iscsi/usr/iface.c or not?
>>
>>
>> Version 2.0.874. OFFLOAD_BOOT_SUPPORTED is in the code but not defined.
>>
>>
>>
>> If it's not defined and you modified the code like you described, then I
>> think you would use the ibft info to setup the normal old nic, and then
>> you would end up creating a boot session using software iscsi. 
>>
>>
>> Yes, I believe that is the result I am getting.
>>
>>
>>  
>>
>> I am not
>> 100% sure about this last statement. I cannot remember and just looked
>> at the code for a couple minutes.
>>
>>
>> I will try defining OFFLOAD_BOOT_SUPPORTED to see if that brings up the NIC.
>>
>>  
>>
> 
> I tried defining OFFLOAD_BOOT_SUPPORTED. The NIC is brought up but no
> IP is assigned. Is that expected? I have been using the uEFI iSCSI

It is expected for what you are doing. I misunderstood what you were
trying to do.

> driver which does not support hardware offload. I also have at least
> one bnx2x NIC that does not support iSCSI hardware offload:
> 
> 02:00.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5719
> Gigabit Ethernet PCIe (rev 01)
> 02:00.0 0200: 14e4:1657 (rev 01)
> 
> Do we need some sort of whitelist or a command-line flag to support these sort
> of configurations?
> 

I thought you wanted to use a iscsi offload engine in the broadcom card.
If you wanted to use a card that is in those offload lists for normal
old networking, have iscsistart bring it up and do iscsi root using
software iscsi, then yeah, I think we need some sort of flag.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: iscsistart -N skips offload NICs causing boot to fail on debian

[Mike Christie]

On 12/08/2016 10:55 AM, andrew.patter...@hpe.com wrote:
> I am trying to get iSCSI boot working on a debian-based system
> using built-in uEFI iSCSI initiator firmware and broadcom
> NICs (bnx2x).
> 
> The debian initramfs uses the ibft support in iscsistart to log
> into the root volume. The initramfs script uses iscsistart -N to
> bring up the NICs before logging in with iscsstart -b. This
> process works fine when using non-offload NICs, but fails when
> using NICs using the bnx2, bnx2x, cxgb3, and cxgb4 drivers due to
> the following code in
> utils/fwparam_ibft/fw_entry.c:fw_setup_nics():
> 
> list_for_each_entry(context, , list) {   
> /* if it is a offload nic ignore it */
> if (!net_get_transport_name_from_netdev(context->iface,
> transport))
> continue
> 
> 
> Which does a lookup in the table in usr/iscsi_net_util.c
> 
> static struct iscsi_net_driver net_drivers[] = {
> {"cxgb3", "cxgb3i" },
> {"cxgb4", "cxgb4i" },
> {"bnx2", "bnx2i" },
> {"bnx2x", "bnx2i"},
> {NULL, NULL}
> };
> 
> 
> to see if -N should skip this NIC.  I cannot determine the reason
> for this check. Do iscsi offload NICs not need to be configured
> for boot? The current code causes iscsi boots to fail. I removed the
> check and booting works fine.
> 

If you use the ibft net info for the nic, what do you use for the iscsi
offload engine? Do they both have the same IP, and are you creating boot
iscsi sessions through the offload engine?

cxgb*i is able to share the net info. The normal old nic engine and
iscsi engine are able to use the same IP. That originally was not
supposed to be allowed upstream, but it got in.

bnx2* is (or at least was when we wrote that code) not able to share the
net info. The nic and iscsi engines need different IPs.

So to cover both types we just use the ibft net info for the iscsi
offload engines.

In the version of the code you are using is OFFLOAD_BOOT_SUPPORTED
defined in open-iscsi/usr/iface.c or not?

If it's not defined and you modified the code like you described, then I
think you would use the ibft info to setup the normal old nic, and then
you would end up creating a boot session using software iscsi. I am not
100% sure about this last statement. I cannot remember and just looked
at the code for a couple minutes.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: iscsi: make mutex for target scanning and unbinding per-session

[Mike Christie]

On 11/10/2016 07:13 PM, Chris Leech wrote:
> On Thu, Nov 10, 2016 at 05:22:44PM -0600, Mike Christie wrote:
>> > On 11/07/2016 12:22 PM, Chris Leech wrote:
>>> > > Currently the iSCSI transport class synchronises target scanning and
>>> > > unbinding with a host level mutex.  For multi-session hosts (offloading
>>> > > iSCSI HBAs) connecting to storage arrays that may implement one
>>> > > target-per-lun, this can result in the target scan work for hundreds of
>>> > > sessions being serialized behind a single mutex.  With slow enough
>> > 
>> > Does this patch alone help or is there a scsi piece too?
>  
> I had this tested when working a hung task timeout issue at boot, and
> was told that it fixed the issue.  The exact situation may be more
> complex, I think it was only 128 sessions which is surprising that it
> would hit a 2 minute timeout.  But every backtrace was at this mutex.
> 

I think you are also hitting a issue where the normal scan time is
higher than usual and that might be a userspace bug. I am not sure how
the mutex patch helps, but I have not thought about it.

I think iscsid will scan the entire host during login. We only want
iscsi_sysfs_scan_host to scan the specific target for the session we
just logged into.

In the kernel we are setting SCSI_SCAN_RESCAN for the scan and userspace
did echo - - - > /scan for the entire host, so iscsi_user_scan is
going to rescan every target that is setup. So once you get to 127
sessions/targets it could be a loonngg scan of all 127 of them, and
target 128 is going to have to wait a lnnggg time for that mutex and
then also execute a long scan.

If you have userspace do the single target scan, it should execute
faster. I know that does not solve the serialization problem. You will
still have lots of targets waiting to be scanned.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: iscsi: make mutex for target scanning and unbinding per-session

[Mike Christie]

On 11/07/2016 12:22 PM, Chris Leech wrote:
> Currently the iSCSI transport class synchronises target scanning and
> unbinding with a host level mutex.  For multi-session hosts (offloading
> iSCSI HBAs) connecting to storage arrays that may implement one
> target-per-lun, this can result in the target scan work for hundreds of
> sessions being serialized behind a single mutex.  With slow enough

Does this patch alone help or is there a scsi piece too?

There is also scsi_host->scan_mutex taken in the scsi layer during
scsi_scan_target, so it is serialized there too. It seems like this
patch would move that problem one layer down.

>  
> @@ -1801,10 +1798,7 @@ static int iscsi_user_scan_session(struct device *dev, 
> void *data)
>  
>   ISCSI_DBG_TRANS_SESSION(session, "Scanning session\n");
>  
> - shost = iscsi_session_to_shost(session);
> - ihost = shost->shost_data;
> -
> - mutex_lock(>mutex);
> + mutex_lock(>mutex);
>   spin_lock_irqsave(>lock, flags);
>   if (session->state != ISCSI_SESSION_LOGGED_IN) {
>   spin_unlock_irqrestore(>lock, flags);
> @@ -1823,7 +1817,7 @@ static int iscsi_user_scan_session(struct device *dev, 
> void *data)
>   }

The patch will allow you to remove other sessions while scanning is
running, so it could still be a good idea.

I think I originally added the mutex because we did our own loop over a
list of the host's sessions. If a unbind were to occur at the same time
then it would be freed while scanning. We changed the user scan to use
device_for_each_child so that will grab a reference to the session so
the memory will not be freed now. It now just makes sure that scsi
target removal and iscsi_remove_session wait until the scan is done.

On a related note, you can remove all the iscsi_scan_session code. We do
not use it anymore. qla4xxx used to do async scans in the kernel with
that code but does not anymore. In the future someone will also not ask
why we grab the mutex around one scan and not the other.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCHv2] MAINTAINERS: Update open-iscsi maintainers

[Mike Christie]

On 09/26/2016 05:25 PM, Lee Duncan wrote:
> Chris Leech and I are taking over as open-iscsi maintainers.
> 
> Changes since v1:
>  * Updated URL to open-iscsi.com
>  * Removed git repository, since code in tree
> ---
>  MAINTAINERS | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 01bff8ea28d8..81384a2562e7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6448,10 +6448,10 @@ S:Maintained
>  F:   drivers/firmware/iscsi_ibft*
>  
>  ISCSI
> -M:   Mike Christie <micha...@cs.wisc.edu>
> +M:   Lee Duncan <ldun...@suse.com>
> +M:   Chris Leech <cle...@redhat.com>
>  L:   open-iscsi@googlegroups.com
> -W:   www.open-iscsi.org
> -T:   git 
> git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git
> +W:   www.open-iscsi.com
>  S:   Maintained
>  F:   drivers/scsi/*iscsi*
>  F:   include/scsi/*iscsi*
> 

After over 10 years, I will get to take a vacation where I do not have
to think about iSCSI :)

Reviewed-by: Mike Christie <mchri...@redhat.com>

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: Kernel reported invalid or unknown error code?

[Mike Christie]

On 08/05/2016 07:34 PM, dbuech...@cablemo.com wrote:
> Hello All,
> 
> I'm seeing these messages in my messages log, as well as experiencing
> corosync problems around the same time.  I have searched high and low
> for information on this error code and have found very little.
> 
> From /var/log/messages:
> iscsid: Kernel reported iSCSI connection 1:0 error (1022 - Invalid or
> unknown error code) state (3)
> iscsid: connection1:0 is operational after recovery (1 attempts)
> iscsid: Kernel reported iSCSI connection 1:0 error (1022 - Invalid or
> unknown error code) state (3)
> iscsid: connection1:0 is operational after recovery (1 attempts)

Is there also something about a iscsi ping timing out in the initiator
kernel log? Something like:

"ping timeout of %d secs expired"

?

> 
> The system is CentOS 7.2, running iscsi-initiator-utils-6.2.0.873-32.
> I'm connecting to a QNAP TurboNAS 869 as the target.  The target isn't
> logging any sort of error messages that can help in the debugging of the
> issue.
> I did have Jumbo Frames enabled on the switch and set MTU to 9000 on the
> initiators and the target, but set everything back to 1500 after
> problems began with no discernible effect.
> 
> If anyone could provide some insight as to why I'm seeing those messages
> and how to correct the problem, I would appreciate it greatly.
> 
> Thank you for your time.
> 


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: Performance issues when logging into a large number of targets

[Mike Christie]

I think Chris is busy. Here is a link to his tree

https://github.com/cleech/open-iscsi

that has patches for large number of targets. Apply my patch over it and
try it out.

Do not do it on your production systems.

On 06/08/2016 05:48 PM, Michael Christie wrote:
> Wait to test with Chris's patches. If he does not reply I will dig them
> up from the list. His fixes should provide the best boost and I think he
> was saying there are some other patches that are needed.
> 
> On Jun 8, 2016, at 2:05 PM, Syed Mushtaq <syed1.mush...@gmail.com
> <mailto:syed1.mush...@gmail.com>> wrote:
> 
>> Thanks for the patch Mike. Will test it with this and let you know if
>> I see some improvement. And yes the gmon output is for iscsiadm and
>> not iscsid.
>>
>>
>> -Syed
>>
>> On Wed, Jun 8, 2016 at 1:36 PM, Mike Christie <micha...@cs.wisc.edu
>> <mailto:micha...@cs.wisc.edu>> wrote:
>>
>> Hey,
>>
>> Chris Leech has been working on this problem. He has some patches
>> which
>> help, but I do not see them. Chris?
>>
>> I am also attaching a hacky patch to remove another problem source
>> with
>> lots of targets. Could you run that with Chris's patches when he
>> responds?
>>
>> On 06/07/2016 02:24 PM, Syed Mushtaq wrote:
>> >
>> >
>> > I am attaching the gmon output here
>> >
>>
>> This was taken on iscsiadm right? Not on iscsid?
>>
>> --
>> You received this message because you are subscribed to a topic in
>> the Google Groups "open-iscsi" group.
>> To unsubscribe from this topic, visit
>> https://groups.google.com/d/topic/open-iscsi/XzT3IXcHlcs/unsubscribe.
>> To unsubscribe from this group and all its topics, send an email
>> to open-iscsi+unsubscr...@googlegroups.com
>> <mailto:open-iscsi%2bunsubscr...@googlegroups.com>.
>> To post to this group, send email to open-iscsi@googlegroups.com
>> <mailto:open-iscsi@googlegroups.com>.
>> Visit this group at https://groups.google.com/group/open-iscsi.
>> For more options, visit https://groups.google.com/d/optout.
>>
>>
>> -- 
>> You received this message because you are subscribed to the Google
>> Groups "open-iscsi" group.
>> To unsubscribe from this group and stop receiving emails from it, send
>> an email to open-iscsi+unsubscr...@googlegroups.com
>> <mailto:open-iscsi+unsubscr...@googlegroups.com>.
>> To post to this group, send email to open-iscsi@googlegroups.com
>> <mailto:open-iscsi@googlegroups.com>.
>> Visit this group at https://groups.google.com/group/open-iscsi.
>> For more options, visit https://groups.google.com/d/optout.
> 
> -- 
> You received this message because you are subscribed to the Google
> Groups "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to open-iscsi+unsubscr...@googlegroups.com
> <mailto:open-iscsi+unsubscr...@googlegroups.com>.
> To post to this group, send email to open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: iscsistart takeover vs iface names

[Mike Christie]

I think we can do something like the attached compile tested only patch
which has the boot code use the same iface name format as the normal setup.

My concern was compat support. The patch will use whatever the kernel
supports, and so it should just work for boot if you are using
iscsistart or "iscsiadm -m fw -l", but I am not sure if during a upgrade
things will work with mismatch of old and new kernel and tools and old
stored /etc/ifaces and how distros were using the tools.

I got busy with day time work stuff, and I killed my card (updated the
fw and now it only supports networking instead of iscsi and I cannot
covert back) so I was not able to go over all the combos. If you can
test it out it and think of the SUSE upgrade scenarios you guys support
it would help.


On 05/27/2016 01:13 PM, The Lee-Man wrote:
> Mike: I know you've been busy. Any progress on this? Can I help, as I
> have the same issue?
> 
> On Thursday, October 8, 2015 at 2:48:30 PM UTC-7, Mike Christie wrote:
> 
> On 10/08/2015 04:10 PM, Ferenc Wagner wrote:
> > Mike Christie <m*@cs.wisc.edu <mailto:micha...@cs.wisc.edu>> writes:
> >
> >> > On 10/7/15, 1:37 AM, Ferenc Wagner wrote:
> >> >
> >>> >> In a pristine system (iscsistart only, no targets configured by
> >>> >> iscsiadm) I only get two sessions (with short interfaces
> names).  With
> >>> >> an older open-iscsi version I could then add the four needed
> node, and
> >>> >> after login iscsid took over the two from iscsistart and also
> started
> >>> >> the two other.  Now, that iscsid uses long interface names,
> it creates
> >>> >> four new sessions and leaves the two from iscsistart
> unmanaged.  Than
> >> >
> >> > How do you know they are unmanaged by iscsid? Is the iscsid
> session
> >> > state in the iscsiadm -m session -P 1 info showing:
> >> >
> >> > Internal iscsid Session State: Unknown
> > Maybe I was wrong on this point.  After an iscsistart -b in the
> > initramfs, but with no ifaces or nodes configured later:
> 
> No problem. I think I fully understand the problem. I am working on a
> patch. I messed up the firmware on my card, so I cannot access the boot
> setup any more here, but am looking for one in the red hat lab.
> 
> -- 
> You received this message because you are subscribed to the Google
> Groups "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to open-iscsi+unsubscr...@googlegroups.com
> <mailto:open-iscsi+unsubscr...@googlegroups.com>.
> To post to this group, send email to open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
diff --git a/usr/iface.c b/usr/iface.c
index 0a7f0bb..acb61b6 100644
--- a/usr/iface.c
+++ b/usr/iface.c
@@ -463,6 +463,21 @@ int iface_get_iptype(struct iface_rec *iface)
 	}
 }
 
+static void iface_set_name(struct iface_rec *iface,
+			   struct iface_rec *kern_iface)
+{
+	if (kern_iface) {
+		snprintf(iface->name, sizeof(iface->name), "%s.%s.%s.%u",
+			 kern_iface->transport_name,
+			 kern_iface->hwaddress,
+			 iface_get_iptype(kern_iface) == ISCSI_IFACE_TYPE_IPV4 ?
+			 "ipv4" : "ipv6", kern_iface->iface_num);
+	} else {
+		snprintf(iface->name, sizeof(iface->name), "%s.%s",
+			 iface->transport_name, iface->hwaddress);
+	}
+}
+
 static int iface_setup_binding_from_kern_iface(void *data,
 	   struct iface_rec *kern_iface)
 {
@@ -478,21 +493,11 @@ static int iface_setup_binding_from_kern_iface(void *data,
 	}
 
 	memset(, 0, sizeof(struct iface_rec));
-	if (kern_iface) {
+	if (kern_iface)
 		memcpy(, kern_iface, sizeof(iface));
-
-		snprintf(iface.name, sizeof(iface.name), "%s.%s.%s.%u",
-			 kern_iface->transport_name,
-			 kern_iface->hwaddress,
-			 iface_get_iptype(kern_iface) == ISCSI_IFACE_TYPE_IPV4 ?
-			 "ipv4" : "ipv6", kern_iface->iface_num);
-	} else {
-		snprintf(iface.name, sizeof(iface.name), "%s.%s",
-			 hinfo->iface.transport_nam

Re: open-iscsi Ping timeout error.

[Mike Christie]

On 05/20/2016 04:14 PM, Mike Christie wrote:
> On 05/20/2016 11:39 AM, The Lee-Man wrote:
>> Hi:
>>
>> It seems like your backend is getting busy and not replying in time when
>> it gets very busy. You can disable the NOOP, or you can lengthen its
>> interval, I believe.
>>
>> If there is a bug, it would be in the kernel target subsystem. Have you
>> tried the target-devel @ vger kernel mailing list?
> 
> We are waiting to hear back from Nick
> 
> http://www.spinics.net/lists/linux-scsi/msg96904.html
> 


Oh yeah, another workaround might be to just modify the write back/flush
settings on the LIO target so there are not so many dirty pages to write
back when a sync is finally sent.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: open-iscsi Ping timeout error.

[Mike Christie]

On 05/20/2016 11:39 AM, The Lee-Man wrote:
> Hi:
> 
> It seems like your backend is getting busy and not replying in time when
> it gets very busy. You can disable the NOOP, or you can lengthen its
> interval, I believe.
> 
> If there is a bug, it would be in the kernel target subsystem. Have you
> tried the target-devel @ vger kernel mailing list?

We are waiting to hear back from Nick

http://www.spinics.net/lists/linux-scsi/msg96904.html

> 
> On Friday, May 13, 2016 at 9:52:46 AM UTC-7, Zhengyuan Liu wrote:
> 
> Hi everyone:
> I create a target using fileio as the backend storage on ARM64
> server. The initiator reported some errors showed bellow  while
> perform iozone test.
> 
> [178444.145679]  connection14:0: ping timeout of 5 secs expired,
> recv timeout 5, last rx 4339462894, last ping 4339464146, now 4339465400
> [178444.145706]  connection14:0: detected conn error (1011)
> [178469.674313]  connection14:0: detected conn error (1020)
> [178504.420979]  connection14:0: ping timeout of 5 secs expired,
> recv timeout 5, last rx 4339477953, last ping 4339479204, now 4339480456
> [178504.421001]  connection14:0: detected conn error (1011)
> [178532.064262]  connection14:0: detected conn error (1020)
> [178564.584087]  connection14:0: ping timeout of 5 secs expired,
> recv timeout 5, last rx 4339492980, last ping 4339494232, now 4339495484
> ..
> 
> I try to trace the function call of target iscsi. Then, I found the
>  receiving  thread of target iscsi blocked at fd_execute_sync_cache
> -> vfs_fsync_range. Further, vfs_fsync_range may takes more than 10
> seconds to return,while initiator Ping timeout would happened after
> 5 seconds.   vfs_fsync_range was call with the
> form vfs_fsync_range(fd_dev->fd_file, 0, LLONG_MAX, 1) every times
>  which means sync all device cache. 
> So, is this a bug?
> How  does Initiator send sync_cache scsi command? 
> Does it need to sync all device cache at once?
> Any reply would be thankful.
> 
> -- 
> You received this message because you are subscribed to the Google
> Groups "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to open-iscsi+unsubscr...@googlegroups.com
> .
> To post to this group, send email to open-iscsi@googlegroups.com
> .
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: Using all 24-bits of the ISID

[Mike Christie]

On 05/13/2016 11:58 AM, The Lee-Man wrote:
> When we made the most recent changes to open-iscsi:
> 
>   "make use of all 24 bits of ISID qualifier space"
> 
> we agreed it would be a "good thing" to modify the kernel
> to use the "id" routines instead of an atomic int.
> 
> I created a set of patches and submitted them, and they
> got comments, but the current version has sat for a month
> without comment.

Are you talking about the ida patches? I gave you comments, but never
got a response. I thought it could cause a regression. It probably got
messed up in the threading because I dropped linux-scsi and lkml since
it was userspace stuff. Will bounce it to you offlist.

> 
> I'd really like to either get these changes into the kernel
> if we want them there.
> 
> Can anyone on the list review them, please, if they get a chance?
> 
> On LKML or linux-scsi, the subject is:
> 
>   "Re: [PATCHv3 0/2] target: make location of /var/targets configurable"
>

You mean the "Use ida_simple for SCSI iSCSI transport session id" thread
right?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/3] scsi_transport_iscsi: Add stats support for iscsi host

[Mike Christie]

On 04/22/2016 08:39 AM, adheer.chandravan...@qlogic.com wrote:
> From: Adheer Chandravanshi 
> 
> Add stats for iscsi initiator that will be maintained at iscsi host level
> and will be exported as iscsi_host sysfs attributes.
> 
> Signed-off-by: Adheer Chandravanshi 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 51 
> +
>  include/scsi/iscsi_if.h | 24 +
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index 42bca61..076843c 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -4253,6 +4253,21 @@ iscsi_host_attr(ipaddress, ISCSI_HOST_PARAM_IPADDRESS);
>  iscsi_host_attr(initiatorname, ISCSI_HOST_PARAM_INITIATOR_NAME);
>  iscsi_host_attr(port_state, ISCSI_HOST_PARAM_PORT_STATE);
>  iscsi_host_attr(port_speed, ISCSI_HOST_PARAM_PORT_SPEED);
> +iscsi_host_attr(login_accept_rsps, ISCSI_HOST_PARAM_LOGIN_ACCEPT_RSPS);
> +iscsi_host_attr(login_other_fails, ISCSI_HOST_PARAM_LOGIN_OTHER_FAILS);
> +iscsi_host_attr(login_authentication_fails,
> + ISCSI_HOST_PARAM_LOGIN_AUTHENTICATION_FAILS);
> +iscsi_host_attr(login_authorization_fails,
> + ISCSI_HOST_PARAM_LOGIN_AUTHORIZATION_FAILS);
> +iscsi_host_attr(login_negotiation_fails,
> + ISCSI_HOST_PARAM_LOGIN_NEGOTIATION_FAILS);
> +iscsi_host_attr(login_redirect_rsps, ISCSI_HOST_PARAM_LOGIN_REDIRECT_RSPS);
> +iscsi_host_attr(logout_normal_rsps, ISCSI_HOST_PARAM_LOGOUT_NORMAL_RSPS);
> +iscsi_host_attr(logout_other_rsps, ISCSI_HOST_PARAM_LOGOUT_OTHER_RSPS);
> +iscsi_host_attr(digest_err, ISCSI_HOST_PARAM_DIGEST_ERR);
> +iscsi_host_attr(timeout_err, ISCSI_HOST_PARAM_TIMEOUT_ERR);
> +iscsi_host_attr(format_err, ISCSI_HOST_PARAM_FORMAT_ERR);
> +iscsi_host_attr(session_fails, ISCSI_HOST_PARAM_SESSION_FAILS);
>  
>  static struct attribute *iscsi_host_attrs[] = {
>   _attr_host_netdev.attr,
> @@ -4261,6 +4276,18 @@ static struct attribute *iscsi_host_attrs[] = {
>   _attr_host_initiatorname.attr,
>   _attr_host_port_state.attr,
>   _attr_host_port_speed.attr,
> + _attr_host_login_accept_rsps.attr,
> + _attr_host_login_other_fails.attr,
> + _attr_host_login_authentication_fails.attr,
> + _attr_host_login_authorization_fails.attr,
> + _attr_host_login_negotiation_fails.attr,
> + _attr_host_login_redirect_rsps.attr,
> + _attr_host_logout_normal_rsps.attr,
> + _attr_host_logout_other_rsps.attr,
> + _attr_host_digest_err.attr,
> + _attr_host_timeout_err.attr,
> + _attr_host_format_err.attr,
> + _attr_host_session_fails.attr,
>   NULL,
>  };
>  
> @@ -4284,6 +4311,30 @@ static umode_t iscsi_host_attr_is_visible(struct 
> kobject *kobj,
>   param = ISCSI_HOST_PARAM_PORT_STATE;
>   else if (attr == _attr_host_port_speed.attr)
>   param = ISCSI_HOST_PARAM_PORT_SPEED;
> + else if (attr == _attr_host_login_accept_rsps.attr)
> + param = ISCSI_HOST_PARAM_LOGIN_ACCEPT_RSPS;
> + else if (attr == _attr_host_login_other_fails.attr)
> + param = ISCSI_HOST_PARAM_LOGIN_OTHER_FAILS;
> + else if (attr == _attr_host_login_authentication_fails.attr)
> + param = ISCSI_HOST_PARAM_LOGIN_AUTHENTICATION_FAILS;
> + else if (attr == _attr_host_login_authorization_fails.attr)
> + param = ISCSI_HOST_PARAM_LOGIN_AUTHORIZATION_FAILS;
> + else if (attr == _attr_host_login_negotiation_fails.attr)
> + param = ISCSI_HOST_PARAM_LOGIN_NEGOTIATION_FAILS;
> + else if (attr == _attr_host_login_redirect_rsps.attr)
> + param = ISCSI_HOST_PARAM_LOGIN_REDIRECT_RSPS;
> + else if (attr == _attr_host_logout_normal_rsps.attr)
> + param = ISCSI_HOST_PARAM_LOGOUT_NORMAL_RSPS;
> + else if (attr == _attr_host_logout_other_rsps.attr)
> + param = ISCSI_HOST_PARAM_LOGOUT_OTHER_RSPS;
> + else if (attr == _attr_host_digest_err.attr)
> + param = ISCSI_HOST_PARAM_DIGEST_ERR;
> + else if (attr == _attr_host_timeout_err.attr)
> + param = ISCSI_HOST_PARAM_TIMEOUT_ERR;
> + else if (attr == _attr_host_format_err.attr)
> + param = ISCSI_HOST_PARAM_FORMAT_ERR;
> + else if (attr == _attr_host_session_fails.attr)
> + param = ISCSI_HOST_PARAM_SESSION_FAILS;
>   else {
>   WARN_ONCE(1, "Invalid host attr");
>   return 0;
> diff --git a/include/scsi/iscsi_if.h b/include/scsi/iscsi_if.h
> index d66c070..d6a3909 100644
> --- a/include/scsi/iscsi_if.h
> +++ b/include/scsi/iscsi_if.h
> @@ -632,6 +632,18 @@ enum iscsi_host_param {
>   ISCSI_HOST_PARAM_IPADDRESS,
>   ISCSI_HOST_PARAM_PORT_STATE,
>   ISCSI_HOST_PARAM_PORT_SPEED,
> + ISCSI_HOST_PARAM_LOGIN_ACCEPT_RSPS,
> + ISCSI_HOST_PARAM_LOGIN_OTHER_FAILS,
> + 

[ANNOUNCE] new open-iscsi maintainers

[Mike Christie]

Hi open-iscsi list,

I am very happy to announce that Chris Leech and Lee Duncan will be
taking over open-iscsi maintainership.

They have created a new github organization:

https://github.com/open-iscsi

that will host the userspace code.

Send all userspace patches to the list and to Chris Leech
(cle...@redhat.com) and Lee Duncan (ldun...@suse.com).

Send kernel patches to this list, myself, and also to Lee and Chris. In
the hopefully near future, they will be comfortable with taking over the
kernel code as well.

Thanks to Lee and Chris for taking this over!

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] iscsi tools: Add support for some host statistics

[Mike Christie]

On 03/25/2016 04:08 AM, Adheer Chandravanshi wrote:
> 
> 
> From: "open-iscsi@googlegroups.com <mailto:open-iscsi@googlegroups.com>"
> <open-iscsi@googlegroups.com <mailto:open-iscsi@googlegroups.com>> on
> behalf of Adheer Chandravanshi <adheer.chandravan...@qlogic.com
> <mailto:adheer.chandravan...@qlogic.com>>
> Reply-To: "open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>" <open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>>
> Date: Thursday, 10 March 2016 9:54 am
> To: Mike Christie <micha...@cs.wisc.edu <mailto:micha...@cs.wisc.edu>>,
> "open-iscsi@googlegroups.com <mailto:open-iscsi@googlegroups.com>"
> <open-iscsi@googlegroups.com <mailto:open-iscsi@googlegroups.com>>
> Cc: Shyam Sundar <shyam.sun...@qlogic.com
> <mailto:shyam.sun...@qlogic.com>>, Saurav Kashyap
> <saurav.kash...@qlogic.com <mailto:saurav.kash...@qlogic.com>>
> Subject: Re: [PATCH] iscsi tools: Add support for some host statistics
> 
> 
> 
> From: Mike Christie <micha...@cs.wisc.edu <mailto:micha...@cs.wisc.edu>>
> Date: Friday, 4 March 2016 6:11 am
> To: "open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>" <open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>>
> Cc: Shyam Sundar <shyam.sun...@qlogic.com
> <mailto:shyam.sun...@qlogic.com>>, Saurav Kashyap
> <saurav.kash...@qlogic.com <mailto:saurav.kash...@qlogic.com>>,
> Adheer Chandravanshi <adheer.chandravan...@qlogic.com
> <mailto:adheer.chandravan...@qlogic.com>>
> Subject: Re: [PATCH] iscsi tools: Add support for some host statistics
> 
> On 03/02/2016 02:04 AM, adheer.chandravan...@qlogic.com
> <mailto:adheer.chandravan...@qlogic.com> wrote:
> 
> From: Adheer Chandravanshi <adheer.chandravan...@qlogic.com
> <mailto:adheer.chandravan...@qlogic.com>>
> Add support to maintain and show some host statistics in iscsid.
> This provides following host specific stats:
> iscsi_login_accept_rsps
> iscsi_login_other_fail_rsps
> iscsi_login_negotiate_fails
> iscsi_login_authenticate_fails
> iscsi_login_auth_fail_rsps
> iscsi_login_redirect_rsps
> iscsi_logout_normals
> iscsi_logout_others
> iscsi_connection_timeout_errors
> iscsi_session_failures
> 
> 
> What happened? I actually liked your guys's originally attempt
> where it
> was kernel based better.
> 
> 
> We think that putting the host statistics in userspace looks cleaner
> and is also inline with current design of partial offload solutions
> (bnx2i and likes) and software initiator.
> The stats to be supported currently can be easily managed with this
> patch as they are updated with the current PDU parsing flow in
> userspace.
> 
> 
> How are you going to support qla4xxx session with this approach?
> Were
> you going to do this and also a kernel interface for that type
> of driver?
> 
> 
> For qla4xxx, it already has support for host stats through current
> offload host stats interface.
> And in case its needed, we can support extra stats using
> iscsi_host_stats_custom field of struct iscsi_offload_host_stats.
> 
> 
> For this userspace based patch, what was the last issue with it?
> Were
> you guys having some issue about when to add the host or delete
> it or
> something and did you solve the issue?
> 
> 
> Yes, it was related to host deletion and dropping the reference.
> It has been solved by listening to udev events for iscsi_host removal.
> 
> As correctly pointed out by Chris, following points should have been
> added to description.
> 1) adds a refcounted iscsi_host structure
> 2) adds a uevent listener to iscsid
> 
> 
> 
> Hello,
> 
> Any other feedback for this patch?
> 

Hey,

I think the patch is ugly. I know this is partially my fault because I
worked on it :) It is just a lot of code to add login stats. I can live
with it though. If however, we end up needing stats like these for
qla4xxx type drivers I will block any patches that try to do it using
custom stats. We will need to do it generically in kernel. In userspace
we have flexibility.

Could you break up the patch like Chris requested. I have not done a
line by line check of the patch.


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/5] Fix several static checker warnings reported against the iSCSI kernel code

[Mike Christie]

On 03/28/2016 03:39 PM, Bart Van Assche wrote:
> Hello Mike,
> 
> These five patches are what I came up with after analyzing the output of
> "make M=drivers/scsi W=1 C=2" for the iSCSI kernel code. Please consider
> these patches for inclusion in kernel v4.7. The actual patches are:
> 
> 0001-libiscsi-Unexport-iscsi_eh_target_reset.patch
> 0002-libiscsi-Remove-set-but-not-used-variables.patch
> 0003-scsi_transport_iscsi-Remove-set-but-not-used-variabl.patch
> 0004-scsi_transport_iscsi-Unexport-iscsi_is_flashnode_con.patch
> 0005-scsi_transport_iscsi-Declare-local-symbols-static.patch
> 

Thanks Bart. The patches look good to me.

Do you want to post them to linux-scsi? I will add my re
viewed-by/acked-by tag on the list (or you can just add it when you
resend if you want), so Martin or James can pick up into their trees.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: NoPortalsFound

[Mike Christie]

On 03/25/2016 01:27 PM, Graham Welsh wrote:
> Also, in my poking around, I discovered a debug mode :
> 
> iscsiadm -m discovery -t st -b8 -p 192.168.0.10:3260
> 
> 
> which fives the output :
> 
> iscsiadm: Connection to Discovery Address 192.168.0.10 closed
> iscsiadm: Login I/O error, failed to receive a PDU
> iscsiadm: retrying discovery login to 192.168.0.10
> iscsiadm: disconnecting conn 0x226bde0, fd 3
> iscsiadm: discovery session to 192.168.0.10:3260
>  sleeping for 1 seconds before next login attempt
> iscsiadm: connecting to 192.168.0.10:3260 
> iscsiadm: connected local port 43632 to 192.168.0.10:3260
> 
> iscsiadm: connected to discovery address 192.168.0.10
> iscsiadm: discovery session to 192.168.0.10:3260
>  starting iSCSI login
> iscsiadm: sending login PDU with current stage 1, next stage 3, transit
> 0x80, isid 0x00023d00 exp_statsn 0
> iscsiadm: >InitiatorName=iqn.2005-03.org.open-iscsi:9b96ddaa68f4
> iscsiadm: >InitiatorAlias=control
> iscsiadm: >SessionType=Discovery
> iscsiadm: >HeaderDigest=None
> iscsiadm: >DataDigest=None
> iscsiadm: >DefaultTime2Wait=2
> iscsiadm: >DefaultTime2Retain=0
> iscsiadm: >IFMarker=No
> iscsiadm: >OFMarker=No
> iscsiadm: >ErrorRecoveryLevel=0
> iscsiadm: >MaxRecvDataSegmentLength=32768
> iscsiadm: wrote 48 bytes of PDU header
> iscsiadm: wrote 252 bytes of PDU data
> iscsiadm: iscsi_login: Poll return 1
> 
> iscsiadm: Connection to Discovery Address 192.168.0.10 closed
> iscsiadm: Login I/O error, failed to receive a PDU
> iscsiadm: retrying discovery login to 192.168.0.10
> iscsiadm: connection login retries (reopen_max) 5 exceeded
> iscsiadm: disconnecting conn 0x226bde0, fd 3
> iscsiadm: Could not perform SendTargets discovery: encountered iSCSI
> login failure
> 

Something is not right on the target. It will let us connect, is on
responding to iscsi requests.

Is there anything in the target logs?

There is also a "show" type of command for tgt. Can you send the output
of that. It should list the targets that were setup and the LUNs mapped
and ACLs, etc.

Also, from your other mail:


 backing-store /iscsi_disks/disk01.img



This is wrong and might be a issue. You have two  lines there.
Not sure if just a cut and paste error.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: NoPortalsFound

[Mike Christie]

On 03/11/2016 01:42 PM, ZER0SEN wrote:
> Hi all,
> 
> I'm setting up a little test network and am trying to connect my server
> (control) to my storage (NAS)  via iSCSI. I'm having a little issue
> getting my initiator to see my target.
> 
> When I attempt to discover my target, all I get is :
> 
> root@control:/home/Administrator# iscsiadm -m discovery -t st -p
> 192.168.122.9
> iscsiadm: No portals found
> 
> Both machines can ping, and control can see port 3260 as open according
> to nmap :
> 
> root@control:/home/Administrator# nmap 192.168.122.9
> 
> Starting Nmap 6.00 ( http://nmap.org ) at 2016-03-11 11:35 AST
> Nmap scan report for NAS.seamine.local (192.168.122.9)
> Host is up (0.84s latency).
> Not shown: 995 closed ports
> PORT  STATE SERVICE
> 22/tcpopen  ssh
> 111/tcp   open  rpcbind
> 3260/tcp  open  iscsi <---
> 6000/tcp  open  X11
> 1/tcp open  snet-sensor-mgmt
> MAC Address: 00:24:E8:10:EF:82 (Dell)
> 
> Nmap done: 1 IP address (1 host up) scanned in 0.61 seconds
> 
> Interestingly, if I only type iscsiadm -m discovery i can see the NAS,
> but not the actual target :

This doesn't actually do discovery. It only prints the portals we have
done discovery to.

> 
> root@control:/home/Administrator# iscsiadm -m discovery
> NAS.seamine.local:3260 via sendtargets
> 192.168.122.9:3260 via sendtargets
> 
> So, everything seems like it should be go to me, but obviously I'm
> missing something.
> 
> Thus far I have disabled all firewalls and iptables services on both
> machines. I have not enabled CHAP on the target at all. I inputted
> control's ip so the target would recognize it in case that was an issue.
> I originally did not set this to leave it open.
> 
> In fact, here is the target config :
> 
> 
>  backing-store /iscsi_disks/disk01.img
>  initiator-address 192.168.122.1
>  initiator-name 192.168.122.1
> 

What target is this? tgt right?

Can you drop the initiator-* parts for a quick test? I forgot how it
does acls based on address and name. The initiator-name part is probably
wrong though. Do

cat /etc/iscsi/initiatorname.iscsi

that is the initiator name. It is not normally a ip address.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Mike Christie]

On 03/08/2016 11:21 AM, Chris Leech wrote:
> On Fri, Feb 12, 2016 at 09:38:53AM -0800, Lee Duncan wrote:
>> The scsi_transport_iscsi module already uses the ida_simple
>> routines for managing the target ID, if requested to do
>> so. This change replaces an ever-increasing atomic integer
>> that tracks the session ID itself with the ida_simple
>> family of routines. This means that the session ID
>> will be reclaimed and can be reused when the session
>> is freed.
>>
>> Note that no maximum is placed on this value, though
>> user-space currently only seems to use the lower 24-bits.
>> It seems better to handle this in user space, though,
>> than to limit the value range for the session ID here.
>>
>> Signed-off-by: Lee Duncan 
> 
> Acked-by: Chris Leech 
> 

Dropping lkml and linux-scsi for a moment for some userspace stuff.

If we do this patch, then we need Chris's sysfs attr cache removal and
in another patch I think we also need to drop the dev_list cache from
open-iscsi/usr/sysfs.c?

For example, I think we could hit a bug because we could match a session
id but the cached device's parent could be different than before.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: Issues on debian jessie and Dell MD3200i - connection pdu rejected

[Mike Christie]

On 03/08/2016 08:01 AM, Lars Schimmer wrote:
> On 2016-03-08 14:49, Lars Schimmer wrote:
>> Hi!
>>
>> We jsut upgraded one of our old wheezy linux server to Debian jessie.
>> Now Open-iscsi spits out this error on load:
>>
>>
>>  connection1:0: pdu (op 0x1 itt 0x33) rejected. Reason code 0x5
>> [  649.368791]  connection1:0: pdu (op 0x1 itt 0x1) rejected. Reason
>> code 0x5
>> [  649.368928]  connection1:0: pdu (op 0x1 itt 0x50) rejected. Reason
>> code 0x5

The command is being rejected by the target because the target does not
support it.

>> [  748.430030]  connection1:0: detected conn error (1020)
>> [  754.695618]  connection1:0: detected conn error (1021)
>>
>> and the co9nnection is broken.
>> Any idea what could be the reason?
>>
>> Thank you.
>>
>> (the storage I connect to is a Dell MD3200i storage, no issues before of
>> this kind).
> 
> Ok, some more information:
> 
> Error appeared in Debian with kernel linux-image-4.3.0-0.bpo.1-amd64
> (jessie-backports) and open-iscsi 2.0.873+git0.3b4b4500-14 (also
> appeared with open-iscsi 2.0.873+git0.3b4b4500-8+deb8u1).
> 
> Error is gone on debian jessie kernel 3.16+63.
> 
> FS is a ext4 on storage.
> 

I think this was caused due to newer ext4/block layer code sending a
command the storage did not support in newer kernels by default or maybe
the block/SCSI layer changed the format being used. I want to say it was
discard related, but I cannot remember for sure. Could you take a
tcpdump or writeshark trace when you run this test, so I can see what
SCSI command we are failing on?

Also, is there anything in the MD320 logs about a command being rejected
because it is not supported?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] iscsi_tcp set SO_LINGER to abort connection for error handling

[Mike Christie]

On 03/03/2016 06:09 PM, Chris Leech wrote:
> When requests are being failed it's important to abort the TCP
> connection rather than let TCP wait and attempt a graceful shutdown.
> 
> That can be accomplished by setting the SO_LINGER socket option with a
> linger time of 0 to drop queued data and close the connection with a RST
> instead of a FIN.
> 
> Signed-off-by: Chris Leech 
> ---
>  usr/io.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/usr/io.c b/usr/io.c
> index f552e1e..48b233c 100644
> --- a/usr/io.c
> +++ b/usr/io.c
> @@ -391,9 +391,24 @@ iscsi_io_tcp_poll(iscsi_conn_t *conn, int timeout_ms)
>  void
>  iscsi_io_tcp_disconnect(iscsi_conn_t *conn)
>  {
> + struct linger so_linger = { .l_onoff = 1, .l_linger = 0 };
> +
>   if (conn->socket_fd >= 0) {
>   log_debug(1, "disconnecting conn %p, fd %d", conn,
>conn->socket_fd);
> +
> + /* If the state is not IN_LOGOUT, this isn't a clean shutdown
> +  * and there's some sort of error handling going on. In that
> +  * case, set a 0 SO_LINGER to force an abortive close (RST) and
> +  * free whatever is sitting in the TCP transmit queue. This is
> +  * done to prevent stale data from being sent should the
> +  * network connection be restored before TCP times out.
> +  */
> + if (conn->state != ISCSI_CONN_STATE_IN_LOGOUT) {
> + setsockopt(conn->socket_fd, SOL_SOCKET, SO_LINGER,
> +_linger, sizeof(so_linger));
> + }
> +
>   close(conn->socket_fd);
>   conn->socket_fd = -1;
>   }
> 

Nice.

For maybe a slightly different problem, but hoping I get lucky and your
patch fixes it too, I thought the network layer was still accessing
pages that we tried to send and was causing a oops. I get the part where
with your patch the network layer will not try to send data anymore, but
I guess I am asking if the network layer could still be doing some sort
of delayed cleanup process after close() has returned?

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: iSCSI disconnections after running iscsistart

[Mike Christie]

On 03/03/2016 03:17 PM, Chris Leech wrote:
> On Thu, Mar 03, 2016 at 10:16:45PM +0200, Erez Zilber wrote:
>> Hi,
>>
>> I'm running iSCSI boot for RHEL & SUSE nodes. Sometimes, after
>> iscsistart is called, errors on the iSCSI target side occur (e.g. temp
>> network disconnection) and the iSCSI connection is disconnected. In
>> the boot log, it looks like this (on a RHEL node):
>>
>> connection1:0: detected conn error (1020)
>>
>> And then, of course, the node fails to boot.
>>
>> I would expect that iscsid would handle this and reconnect, but I
>> don't see in the boot log that iscsid was started. I also took a look
>> at /usr/share/dracut/modules.d/95iscsi/iscsiroot, but didn't find
>> iscsid there.
> 
> No, if the connection fails during boot I don't think it's covered as
> you found out.  Of course if you're booting from iSCSI, a network
> disruption while the iSCSI boot firmware or bootloader is running is
> also probably fatal.
> 
>> Is it possible to run iscsid as part of initrd, before iscsistart is 
>> executed?
> 
> Transitioning iscsid from the initrd is problematic. Processes started
> in the initrd don't have the right view of the filesystem, aren't in the
> proper security context, etc.  If we kill the initrd started process and
> restart, there's still a time gap.
> 
> And iscsistart does not play well with a running iscsid.

You do not really need both a iscsistart and iscsiadm+iscsid. iscsiadm
can log into ibft and fw boot targets too. iscsiadm just does not
support passing in boot values like when you use the dracut root=iscsi:
method. Feel free to add it since it might be easier. You could also
just run iscsiadm to create a tmp record for the target you want and
then just run iscsiadm login command for it.

The only other feature iscsistart has is that it can start up
networking. This is nice for a simple initramfs. dracut based ones like
in RHEL and SLES do not need that since it handles the networking.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.